From: Boris BREZILLON <hidden> Date: 2014-07-08 07:23:48
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
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
From: Rob Clark <hidden> Date: 2014-07-08 12:49:41
On Tue, Jul 8, 2014 at 3:23 AM, Boris BREZILLON
[off-list ref] wrote:
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
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.
BR,
-R
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
From: Boris BREZILLON <hidden> Date: 2014-07-08 14:37:31
On Tue, 8 Jul 2014 08:49:41 -0400
Rob Clark [off-list ref] wrote:
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
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).
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
From: Rob Clark <hidden> Date: 2014-07-08 15:41:32
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
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
From: Boris BREZILLON <hidden> Date: 2014-07-08 17:08:20
On Tue, 8 Jul 2014 11:41:32 -0400
Rob Clark [off-list ref] wrote:
On Tue, Jul 8, 2014 at 10:37 AM, Boris BREZILLON
[off-list ref] wrote:
quoted
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
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.
Okay, my bad, this is definitely simpler with fb objects (I made use of
drm_fb_cma_create to create an fb object when cursor_set is called)
than it was when using GEM objects.
I might even be able to simplify the layer update mechanism with this
approach.
Thanks for the advice.
Best Regards,
Boris
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
From: Matt Roper <hidden> Date: 2014-07-08 23:51:24
On Tue, Jul 08, 2014 at 07:08:20PM +0200, Boris BREZILLON wrote:
On Tue, 8 Jul 2014 11:41:32 -0400
Rob Clark [off-list ref] wrote:
quoted
On Tue, Jul 8, 2014 at 10:37 AM, Boris BREZILLON
[off-list ref] wrote:
quoted
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
quoted
quoted
quoted
quoted
quoted
+/**
+ * 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.
Okay, my bad, this is definitely simpler with fb objects (I made use of
drm_fb_cma_create to create an fb object when cursor_set is called)
than it was when using GEM objects.
I might even be able to simplify the layer update mechanism with this
approach.
Thanks for the advice.
Best Regards,
Boris
Hi Boris.
I haven't really looked at any of your driver in depth, but from a quick
glance it looks like you're registering a cursor drm_plane (i.e., making
use of the new universal plane infrastructure), but you're also
providing an implementation of the legacy cursor ioctls (cursor_set and
cursor_move). There's some patches working their way through the
pipeline that should make this unnecessary and hopefully simplify your
life a bit:
http://cgit.freedesktop.org/drm-intel/commit/?h=drm-intel-nightly&id=c394c2b08e247c32ef292b75fd8b34312465f8aehttp://cgit.freedesktop.org/drm-intel/commit/?h=drm-intel-nightly&id=b36552b32aa9c69e83a3a20bda56379fb9e52435http://cgit.freedesktop.org/drm-intel/commit/?h=drm-intel-nightly&id=161d0dc1dccb17ff7a38f462c7c0d4ef8bcc5662http://cgit.freedesktop.org/drm-intel/commit/?h=drm-intel-nightly&id=fc1d3e44ef7c1db93384150fdbf8948dcf949f15
The third patch there is the one that's really important for your work.
When a driver provides a cursor plane via the universal plane interface,
cursor_set and cursor_move are automatically implemented for you by
drm_mode_cursor_universal() in drivers/gpu/drm/drm_crtc.c and your
legacy handlers will never get called. drm_mode_cursor_universal() will
take care of wrapping the bo's into a drm_framebuffer for you.
When I added the universal cursor stuff, I wanted to make sure that as
soon as a driver starts supporting universal planes it can stop
supporting the legacy ioctls directly; otherwise handling refcounting
when userspace switches back and forth between calling legacy ioctl's
and calling setplane() on a cursor plane would be a nightmare.
I think those patches are only available in drm-intel-nightly at the
moment and haven't moved on to drm-next and such yet, since i915 is the
only driver that currently has patches to make use of cursors via the
univeral plane interface (probably landing for kernel 3.17).
Matt
--
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
From: Boris BREZILLON <hidden> Date: 2014-07-09 07:14:24
Hi Matt,
On Tue, 8 Jul 2014 16:51:24 -0700
Matt Roper [off-list ref] wrote:
Hi Boris.
I haven't really looked at any of your driver in depth, but from a quick
glance it looks like you're registering a cursor drm_plane (i.e., making
use of the new universal plane infrastructure), but you're also
providing an implementation of the legacy cursor ioctls (cursor_set and
cursor_move). There's some patches working their way through the
pipeline that should make this unnecessary and hopefully simplify your
life a bit:
http://cgit.freedesktop.org/drm-intel/commit/?h=drm-intel-nightly&id=c394c2b08e247c32ef292b75fd8b34312465f8aehttp://cgit.freedesktop.org/drm-intel/commit/?h=drm-intel-nightly&id=b36552b32aa9c69e83a3a20bda56379fb9e52435http://cgit.freedesktop.org/drm-intel/commit/?h=drm-intel-nightly&id=161d0dc1dccb17ff7a38f462c7c0d4ef8bcc5662http://cgit.freedesktop.org/drm-intel/commit/?h=drm-intel-nightly&id=fc1d3e44ef7c1db93384150fdbf8948dcf949f15
The third patch there is the one that's really important for your work.
When a driver provides a cursor plane via the universal plane interface,
cursor_set and cursor_move are automatically implemented for you by
drm_mode_cursor_universal() in drivers/gpu/drm/drm_crtc.c and your
legacy handlers will never get called. drm_mode_cursor_universal() will
take care of wrapping the bo's into a drm_framebuffer for you.
When I added the universal cursor stuff, I wanted to make sure that as
soon as a driver starts supporting universal planes it can stop
supporting the legacy ioctls directly; otherwise handling refcounting
when userspace switches back and forth between calling legacy ioctl's
and calling setplane() on a cursor plane would be a nightmare.
I think those patches are only available in drm-intel-nightly at the
moment and haven't moved on to drm-next and such yet, since i915 is the
only driver that currently has patches to make use of cursors via the
univeral plane interface (probably landing for kernel 3.17).
That's great news. I knew there were some work in progress on this
topic, but didn't know it was planned for 3.17.
I'll move to this solution.
Thanks,
Boris
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
From: Daniel Vetter <hidden> Date: 2014-07-09 14:02:51
On Wed, Jul 09, 2014 at 09:14:24AM +0200, Boris BREZILLON wrote:
Hi Matt,
On Tue, 8 Jul 2014 16:51:24 -0700
Matt Roper [off-list ref] wrote:
quoted
Hi Boris.
I haven't really looked at any of your driver in depth, but from a quick
glance it looks like you're registering a cursor drm_plane (i.e., making
use of the new universal plane infrastructure), but you're also
providing an implementation of the legacy cursor ioctls (cursor_set and
cursor_move). There's some patches working their way through the
pipeline that should make this unnecessary and hopefully simplify your
life a bit:
http://cgit.freedesktop.org/drm-intel/commit/?h=drm-intel-nightly&id=c394c2b08e247c32ef292b75fd8b34312465f8aehttp://cgit.freedesktop.org/drm-intel/commit/?h=drm-intel-nightly&id=b36552b32aa9c69e83a3a20bda56379fb9e52435http://cgit.freedesktop.org/drm-intel/commit/?h=drm-intel-nightly&id=161d0dc1dccb17ff7a38f462c7c0d4ef8bcc5662http://cgit.freedesktop.org/drm-intel/commit/?h=drm-intel-nightly&id=fc1d3e44ef7c1db93384150fdbf8948dcf949f15
The third patch there is the one that's really important for your work.
When a driver provides a cursor plane via the universal plane interface,
cursor_set and cursor_move are automatically implemented for you by
drm_mode_cursor_universal() in drivers/gpu/drm/drm_crtc.c and your
legacy handlers will never get called. drm_mode_cursor_universal() will
take care of wrapping the bo's into a drm_framebuffer for you.
When I added the universal cursor stuff, I wanted to make sure that as
soon as a driver starts supporting universal planes it can stop
supporting the legacy ioctls directly; otherwise handling refcounting
when userspace switches back and forth between calling legacy ioctl's
and calling setplane() on a cursor plane would be a nightmare.
I think those patches are only available in drm-intel-nightly at the
moment and haven't moved on to drm-next and such yet, since i915 is the
only driver that currently has patches to make use of cursors via the
univeral plane interface (probably landing for kernel 3.17).
That's great news. I knew there were some work in progress on this
topic, but didn't know it was planned for 3.17.
I'll move to this solution.
As of today those patches have landed in Dave's drm-next branch.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch