On Mon, May 30, 2016 at 03:50:36PM +0200, Gerd Hoffmann wrote:> Hi, > > > - add a small core function to registerr HOT_X/HOT_Y for a (cursor) plane, > > e.g. drm_plane_register_hotspot(). That should allocate the properties > > (if they don't exist yet) and then attach those props to the cursor. We > > don't want those props everywhere, but only on drivers that support/need > > them, aka virtual hw. > > Hmm, why is this special to virtual hw? > > > if (crtc->cursor) { > > - ret = drm_mode_cursor_universal(crtc, req, file_priv); > > + if (drm_core_check_feature(DRIVER_ATOMIC)) > > + ret = drm_mode_cursor_atomic(crtc, req, file_priv); > > + else > > + ret = drm_mode_cursor_universal(crtc, req, file_priv); > > goto out; > > > drm_mode_cursor_atomic would simply be a fusing of > > drm_mode_cursor_universal + drm_atomic_helper_update_plane (dump all the > > intermediate variables and store directly in the plane state), with the > > addition of also storing hot_x/y into the plane state. > > Hmm, that'll either make drm_mode_cursor_atomic a big cut+pasted > function, or need quite some refactoring to move common code into > functions callable from both drm_mode_cursor_atomic > +drm_mode_cursor_universal ... > > Why attach the hotspot to the plane? Wouldn't it make more sense to > make it a framebuffer property?We don't have properties on the framebuffer. I guess you /could/ just add it internally to struct drm_framebuffer, and not bother exposing to userspace. I guess that would be a lot simpler, but it also means that atomic userspace can't use hotspots before we add properties to fbs. And doing that is a bit tricky since drm_framebuffer objects are meant to be invariant - this assumption is deeply in-grained into the code all over the place, everything just compares pointers when semantically it means to compare the entire fb (including backing storage pointer/offsets and everything). So would be a bit more work to wire up for atomic userspace, but indeed a lot less work to implement. I'm totally happy if you go with that tradeoff ;-) Cheers, Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Hi,> > Why attach the hotspot to the plane? Wouldn't it make more sense to > > make it a framebuffer property? > > We don't have properties on the framebuffer. I guess you /could/ just add > it internally to struct drm_framebuffer, and not bother exposing to > userspace. I guess that would be a lot simpler,Yes. I can simply stick the hotspot into drm_framebuffer in drm_mode_cursor_universal() and pick up the values in the driver's plane update function.> but it also means that > atomic userspace can't use hotspots before we add properties to fbs. And > doing that is a bit tricky since drm_framebuffer objects are meant to be > invariant - this assumption is deeply in-grained into the code all over > the place, everything just compares pointers when semantically it means to > compare the entire fb (including backing storage pointer/offsets and > everything).Hmm, the hotspot location for a given cursor image is invariant too, so I don't think that would be a big issue. I'd expect userspace define a bunch of cursors, then switch between them (instead of defining a single cursor, then constantly updating it). cheers, Gerd
On Tue, May 31, 2016 at 8:29 AM, Gerd Hoffmann <kraxel at redhat.com> wrote:>> but it also means that >> atomic userspace can't use hotspots before we add properties to fbs. And >> doing that is a bit tricky since drm_framebuffer objects are meant to be >> invariant - this assumption is deeply in-grained into the code all over >> the place, everything just compares pointers when semantically it means to >> compare the entire fb (including backing storage pointer/offsets and >> everything). > > Hmm, the hotspot location for a given cursor image is invariant too, so > I don't think that would be a big issue. > > I'd expect userspace define a bunch of cursors, then switch between them > (instead of defining a single cursor, then constantly updating it).Agreed, conceptually it would be a really nice fit to put the hotspot into the invariant drm_framebuffer. I just meant to say that you need to add a bit of new uapi to make it happen, since we can't reuse our existing get/set_prop functions (since those only change props after the object is created). And we also can't use our existing ioctls to enumerate properties of an object (since chicken-egg: you want the list of properties before creating a new framebuffer, so can't use that framebuffer to enumerate them). But really not a big concern. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Tue, 31 May 2016 08:29:20 +0200 Gerd Hoffmann <kraxel at redhat.com> wrote:> Hi, > > > > Why attach the hotspot to the plane? Wouldn't it make more sense to > > > make it a framebuffer property? > > > > We don't have properties on the framebuffer. I guess you /could/ just add > > it internally to struct drm_framebuffer, and not bother exposing to > > userspace. I guess that would be a lot simpler, > > Yes. I can simply stick the hotspot into drm_framebuffer in > drm_mode_cursor_universal() and pick up the values in the driver's plane > update function. > > > but it also means that > > atomic userspace can't use hotspots before we add properties to fbs. And > > doing that is a bit tricky since drm_framebuffer objects are meant to be > > invariant - this assumption is deeply in-grained into the code all over > > the place, everything just compares pointers when semantically it means to > > compare the entire fb (including backing storage pointer/offsets and > > everything). > > Hmm, the hotspot location for a given cursor image is invariant too, so > I don't think that would be a big issue. > > I'd expect userspace define a bunch of cursors, then switch between them > (instead of defining a single cursor, then constantly updating it).Except updating a single cursor (well, two alternating buffers) is exactly what Weston does, since there is no "set of cursors". On Wayland, a cursor is just a regular surface like any other with arbitrary content from a client, except it happens to be associated with a pointer device. Furthermore, in Weston a cursor plane is not special in any way. *Any* client surface can go on the cursor plane if it fits. Universal planes, and all that. That's one existing userspace. I suppose that is sub-optimal for virtual drivers, isn't it? But what else could Weston do without having separate paths for "normal DRM" vs. "virtual DRM"? Thanks, pq -------------- next part -------------- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 811 bytes Desc: OpenPGP digital signature URL: <http://lists.linuxfoundation.org/pipermail/virtualization/attachments/20160531/f396e1cc/attachment.sig>