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=c394c2b08e247c32ef292b75fd8b34312465f8ae
        http://cgit.freedesktop.org/drm-intel/commit/?h=drm-intel-nightly&id=b36552b32aa9c69e83a3a20bda56379fb9e52435
        http://cgit.freedesktop.org/drm-intel/commit/?h=drm-intel-nightly&id=161d0dc1dccb17ff7a38f462c7c0d4ef8bcc5662
        http://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