On Tue, Jul 8, 2014 at 10:37 AM, Boris BREZILLON
[off-list ref] wrote:

On Tue, 8 Jul 2014 08:49:41 -0400
Rob Clark [off-list ref] wrote:

quoted

On Tue, Jul 8, 2014 at 3:23 AM, Boris BREZILLON
[off-list ref] wrote:

quoted

Hello Rob,

On Mon, 7 Jul 2014 23:45:54 -0400
Rob Clark [off-list ref] wrote:

quoted

On Mon, Jul 7, 2014 at 12:42 PM, Boris BREZILLON
[off-list ref] wrote:

quoted

The Atmel HLCDC (HLCD Controller) IP available on some Atmel SoCs (i.e.
at91sam9n12, at91sam9x5 family or sama5d3 family) provides a display
controller device.

This display controller supports at least one primary plane and might
provide several overlays and an hardware cursor depending on the IP
version.

At the moment, this driver only implements an RGB connector to interface
with LCD panels, but support for other kind of external devices (like DRM
bridges) might be added later.

Signed-off-by: Boris BREZILLON <redacted>
---
 drivers/gpu/drm/Kconfig                         |   2 +
 drivers/gpu/drm/Makefile                        |   1 +
 drivers/gpu/drm/atmel-hlcdc/Kconfig             |  11 +
 drivers/gpu/drm/atmel-hlcdc/Makefile            |   7 +
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c  | 469 +++++++++++++++
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c    | 474 +++++++++++++++
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h    | 210 +++++++
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_layer.c | 706 +++++++++++++++++++++++
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_layer.h | 422 ++++++++++++++
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_panel.c | 351 ++++++++++++
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c | 729 ++++++++++++++++++++++++
 11 files changed, 3382 insertions(+)
 create mode 100644 drivers/gpu/drm/atmel-hlcdc/Kconfig
 create mode 100644 drivers/gpu/drm/atmel-hlcdc/Makefile
 create mode 100644 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
 create mode 100644 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
 create mode 100644 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h
 create mode 100644 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_layer.c
 create mode 100644 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_layer.h
 create mode 100644 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_panel.c
 create mode 100644 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index d1cc2f6..df6f0c1 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig

@@ -182,6 +182,8 @@ source "drivers/gpu/drm/cirrus/Kconfig"

[...]

quoted

quoted

+
+/**
+ * Atmel HLCDC Plane properties.
+ *
+ * This structure stores plane property definitions.
+ *
+ * @alpha: alpha blending (or transparency) property
+ * @csc: YUV to RGB conversion factors property
+ */
+struct atmel_hlcdc_plane_properties {
+       struct drm_property *alpha;
+       struct drm_property *csc;

appears like csc is not used yet, so I suppose you can drop it for
now..  when you do add it, don't forget to update drm.tmp.  But for
now it at least makes review easier when the driver doesn't add new
userspace interfaces :-)

Sure, I guess this is a leftover from another patch I made to add Color
Space Conversion configuration.

I'll remove it for the next version.

quoted

quoted

+};
+
+/**
+ * Atmel HLCDC Plane.
+ *
+ * @base: base DRM plane structure
+ * @layer: HLCDC layer structure
+ * @properties: pointer to the property definitions structure
+ * @alpha: current alpha blending (or transparency) status
+ */
+struct atmel_hlcdc_plane {
+       struct drm_plane base;
+       struct atmel_hlcdc_layer layer;
+       struct atmel_hlcdc_plane_properties *properties;
+};
+
+static inline struct atmel_hlcdc_plane *
+drm_plane_to_atmel_hlcdc_plane(struct drm_plane *p)
+{
+       return container_of(p, struct atmel_hlcdc_plane, base);
+}
+
+static inline struct atmel_hlcdc_plane *
+atmel_hlcdc_layer_to_plane(struct atmel_hlcdc_layer *l)
+{
+       return container_of(l, struct atmel_hlcdc_plane, layer);
+}
+
+/**
+ * Atmel HLCDC Plane update request structure.
+ *
+ * @crtc_x: x position of the plane relative to the CRTC
+ * @crtc_y: y position of the plane relative to the CRTC
+ * @crtc_w: visible width of the plane
+ * @crtc_h: visible height of the plane
+ * @src_x: x buffer position
+ * @src_y: y buffer position
+ * @src_w: buffer width
+ * @src_h: buffer height
+ * @pixel_format: pixel format
+ * @gems: GEM object object containing image buffers
+ * @offsets: offsets to apply to the GEM buffers
+ * @pitches: line size in bytes
+ * @crtc: crtc to display on
+ * @finished: finished callback
+ * @finished_data: data passed to the finished callback
+ * @bpp: bytes per pixel deduced from pixel_format
+ * @xstride: value to add to the pixel pointer between each line
+ * @pstride: value to add to the pixel pointer between each pixel
+ * @nplanes: number of planes (deduced from pixel_format)
+ */
+struct atmel_hlcdc_plane_update_req {
+       int crtc_x;
+       int crtc_y;
+       unsigned int crtc_w;
+       unsigned int crtc_h;
+       uint32_t src_x;
+       uint32_t src_y;
+       uint32_t src_w;
+       uint32_t src_h;
+       uint32_t pixel_format;
+       struct drm_gem_cma_object *gems[ATMEL_HLCDC_MAX_PLANES];
+       unsigned int offsets[ATMEL_HLCDC_MAX_PLANES];
+       unsigned int pitches[ATMEL_HLCDC_MAX_PLANES];

tbh, I've only looked closely, but I don't completely follow all the
layering here..  I wonder if we'd be better off just moving 'struct
drm_fb_cma' to header file so you could get the gem objects / pixel
fmt / width / height directly from the fb object (and then you can
reference count things at the fb level, rather than at the individual
gem obj level, too)

Actually, the HW cursor is a drm_plane too, and in this case I cannot
retrieve a drm_fb_cma object, but just a single GEM object (see
atmel_hlcdc_crtc_cursor_set function in atmel_hlcdc_crtc.c).

oh, right..  well maybe for the cursor case it would be possible to
wrap up the gem bo with an internally created fb?  Not sure if that
ends up simplifying things or not, so it is definitely your call.  But
at least my experience with other drivers (that did not use a plane as
a cursor internally) was that I could simplify things after fb gained
refcnt'ing.

Unless I'm missing something, I'd say moving to fb objects instead of
GEM objects won't simplify the code much (I'm already refcnt'ing GEM
objects when launching a DMA transfer for a plane update).

yeah, mostly just saves you a bit of bookkeeping.  Ie. ref/unref one
thing instead of loop over N objects, etc.  Nothing earth-shattering.

BR,
-R

The only benefit I can see is consistency with other drivers (which in
fact is a good point).
Indeed atmel_hlcdc_plane_update_req redefines some fields which are
already availables in drm_fb_cma or drm_framebuffer:

- gems (called objs in drm_fb_cma)
- pixel_format
- pitches (offsets must be redefined because it is modified in
  atmel_hlcdc_plane_prepare_update_req)

Anyway, I'll give it a try.

Thanks for your review.

In the meantime, I realized I hadn't subscribed to the dri-devel
ML, which might explain why I didn't get any reviews from DRM
maintainers/developers so far :-).

Best Regards,

Boris

--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com