Hello Rob,

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

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

+
+/**
+ * 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

+};
+
+/**
+ * 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).

Let me know if you see a better solution.

Best Regards,

Boris

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