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@xxxxxxxxx> 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 _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
