[RESEND PATCH v3 05/11] drm: add Atmel HLCDC Display Controller support
Rob Clark
robdclark at gmail.com
Tue Jul 8 08:41:32 PDT 2014
On Tue, Jul 8, 2014 at 10:37 AM, Boris BREZILLON
<boris.brezillon at free-electrons.com> wrote:
> On Tue, 8 Jul 2014 08:49:41 -0400
> Rob Clark <robdclark at gmail.com> wrote:
>
>> On Tue, Jul 8, 2014 at 3:23 AM, Boris BREZILLON
>> <boris.brezillon at free-electrons.com> wrote:
>> > Hello Rob,
>> >
>> > On Mon, 7 Jul 2014 23:45:54 -0400
>> > Rob Clark <robdclark at gmail.com> wrote:
>> >
>> >> On Mon, Jul 7, 2014 at 12:42 PM, Boris BREZILLON
>> >> <boris.brezillon at 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 at 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).
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
More information about the dri-devel
mailing list