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

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

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).

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