[RFCv3 12/14] drm: Specify cursor plane at CRTC initialization

[Daniel Vetter]

On Tue, Mar 18, 2014 at 05:22:57PM -0700, Matt Roper wrote:
> Add cursor plane as a parameter to drm_crtc_init() and update all
> existing DRM drivers to use a helper-provided primary plane.  Passing
> NULL for this parameter indicates that there is no hardware cursor
> supported by the driver and no cursor plane should be provided to
> userspace.
> 
> Signed-off-by: Matt Roper <matthew.d.roper at intel.com>

Ok, cursor planes. I've poked around in this a lot and unfortunately I
don't think we can achieve nirvana :(

The first direction is automatically getting cursor plane support from
existing drivers using the existing callbacks. The irky thing is that we
don't have any means to sanely unwrap the framebuffer since both the bo
handle used by the legacy cursor ioctl and how a framebuffer would wrap
such a handle isn't generic. And e.g. vmwgfx doesn't even use gem for
those.

So I think we'll have to give up on that and drivers which want to expose
cursors with the atomic ioctls simply have to have proper support.

The other direction is that converted drivers don't need to have support
for legacy ioctls should be possible and is imo something we want - at
least for i915 I don't want to carry around two interfaces doing the same.

Which means we need some way to forward the legacy cursor ioctls to the
new plane callbacks exposed when using cursor planes. Unfortunately we
have a bit an api mismatch between the legacy cursor interfaces:

int (*cursor_set2)(struct drm_crtc *crtc, struct drm_file *file_priv,
		   uint32_t handle, uint32_t width, uint32_t height,
		   int32_t hot_x, int32_t hot_y);
int (*cursor_move)(struct drm_crtc *crtc, int x, int y);

And the plane interfaces:

int (*update_plane)(struct drm_plane *plane,
		    struct drm_crtc *crtc, struct drm_framebuffer *fb,
		    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);
int (*disable_plane)(struct drm_plane *plane);

cursor_set2 with handle == 0 simply maps to a disable_plane call. But any
other call of the legacy ioctls simply maps to an update_plane, but we
don't have all the information available all the time.

- width, height and handle isn't a real concern - we can just wrap this
  all up into a drm private drm framebuffer. As long as the only reference
  to that framebuffer is held by crtc->cursor->fb it will also get cleaned
  up at the right time and so won't extend the lifetime of the underlying
  buffer object. And we don't need to cache width/height anywhere since
  they're accessible through crtc->cursor->fb.

- For the pixel format I think it's ok to always assume RGBA.

- hot_x and hot_y can simply be mapped to new plane properties. I think
  it'd be good for this to be generally available, just to have a
  consistent interface.

- The x/y coordinates of cursor_move are more annoying - userspace
  potentially doesn't supply them, so we need to cache them in the crtc
  struct somewhere. Originally I've thought it would be good to have a
  special struct drm_cursor_plane and use that as the blessed cursor plane
  object in drm_crtc_init_with_planes. But that will wreak havoc with hw
  platforms which have fully unified planes and want to use the same
  struct everywhere. So I think we need to add crtc->cursor_x/y fields.

With these bits we should be able to always create a valid call to
update_plane. Which allows drivers to not implement the legacy
cursor_set/move interface. Of course we also need to go through the drm
core to make sure that all the cursor code also works when crtc->cursor is
set.

To make driver writers life as easy as possible I think we should provide
a default helper for their cursor_update_plane callback which checks that
the update_plane call is indeed valid for a cursor:

- Checks that widht == pitch*bpp since that's what we assume with cursors.

- Checks that there's no scaling going on.

- Checks that the viewport matches the full cursor size.

- Checks that there's no subpixel positioning.

With that the only thing drivers need to do is:
- Kill the handle to bo pointer lookup, just use the one wrapped up in the
  framebuffer object.
- Call the above helper.
- Call into the low-level set_cursor_bo/move_cursor functions - all the
  arguments of the old ioctls have a 1:1 mapping in update_plane, so this
  should be simple.

... and of course they need to set up the cursor plane object.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch