Re: [PATCH v5 05/11] drm: add Atmel HLCDC Display Controller support

Hi Rob,

On Fri, 26 Sep 2014 17:10:49 -0400
Rob Clark [off-list ref] wrote:

On Mon, Sep 8, 2014 at 4:43 AM, 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 might be
added later.

Signed-off-by: Boris BREZILLON <redacted>
Tested-by: Ludovic Desroches <redacted>

A few small comments below, but with those addressed it has my

Reviewed-by: Rob Clark <redacted>

Thanks for your review.

quoted

---
 drivers/gpu/drm/Kconfig                          |   2 +
 drivers/gpu/drm/Makefile                         |   1 +
 drivers/gpu/drm/atmel-hlcdc/Kconfig              |  13 +
 drivers/gpu/drm/atmel-hlcdc/Makefile             |   7 +
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c   | 286 ++++++++
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c     | 488 +++++++++++++
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h     | 224 ++++++
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_layer.c  | 656 ++++++++++++++++++
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_layer.h  | 403 +++++++++++
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c | 476 +++++++++++++
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c  | 836 +++++++++++++++++++++++
 11 files changed, 3392 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_output.c
 create mode 100644 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c

[snip]

quoted

+
+/**
+ * Atmel HLCDC plane rotation enum
+ *
+ * TODO: export DRM_ROTATE_XX macros defined by omap driver and use them
+ * instead of defining this enum.
+ */
+enum atmel_hlcdc_plane_rotation {
+       ATMEL_HLCDC_PLANE_NO_ROTATION,
+       ATMEL_HLCDC_PLANE_90DEG_ROTATION,
+       ATMEL_HLCDC_PLANE_180DEG_ROTATION,
+       ATMEL_HLCDC_PLANE_270DEG_ROTATION,
+};

DRM_ROTATE_* are already in drm_crtc.h

[snip]

Yep, I changed that, but won't be able to test until next week.

quoted

+static int atmel_hlcdc_rgb_mode_valid(struct drm_connector *connector,
+                                     struct drm_display_mode *mode)
+{
+       return MODE_OK;
+}

your _mode_valid() should perhaps somehow check the constraints in
atmel_hlcdc_crtc_mode_set()?  This way invalid modes get filtered out
earlier..

I'm not sure, the test done in atmel_hlcdc_crtc_mode_set are not
connector related, but rather imposed by the display controller
limitations.
Anyway, let me know if you still think I should move those tests in the
connector mode_valid implementation.

[snip]

quoted

+static struct atmel_hlcdc_plane_properties *
+atmel_hlcdc_plane_create_properties(struct drm_device *dev)
+{
+       struct atmel_hlcdc_plane_properties *props;
+       const struct drm_prop_enum_list rotations[] = {
+               { ATMEL_HLCDC_PLANE_NO_ROTATION,   "rotate-0" },
+               { ATMEL_HLCDC_PLANE_90DEG_ROTATION,  "rotate-90" },
+               { ATMEL_HLCDC_PLANE_180DEG_ROTATION, "rotate-180" },
+               { ATMEL_HLCDC_PLANE_270DEG_ROTATION, "rotate-270" },
+       };
+

you could just use drm_mode_create_rotation_property() now

Yep, I changed that too. Thanks for the tip.

Best Regards,

Boris

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