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

[Boris BREZILLON <boris.brezillon@free-electrons.com>]

On Tue, 8 Jul 2014 08:49:41 -0400
Rob Clark <robdclark@gmail.com> wrote:

> On Tue, Jul 8, 2014 at 3:23 AM, Boris BREZILLON
> <boris.brezillon@free-electrons.com> wrote:
> > Hello Rob,
> >
> > On Mon, 7 Jul 2014 23:45:54 -0400
> > Rob Clark <robdclark@gmail.com> wrote:
> >
> >> On Mon, Jul 7, 2014 at 12:42 PM, Boris BREZILLON
> >> <boris.brezillon@free-electrons.com> wrote:
> >> > 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 <boris.brezillon@free-electrons.com>
> >> > ---
> >> >  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"
> >
> > [...]
> >
> >> > +
> >> > +/**
> >> > + * 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.
> >
> >>
> >> > +};
> >> > +
> >> > +/**
> >> > + * 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