GNOME Bugzilla – Bug 707573
wayland: implement HW cursors
Last modified: 2013-09-16 07:32:44 UTC
Use the DRM API and libgbm to upload cursor buffers to the appropriate HW plane, saving on GL calls and compositing.
Created attachment 254189 [details] [review] wayland: implement HW cursors
Created attachment 254445 [details] [review] MetaCursorTracker: add support for loading cursors from the theme Not only this way we get the right Adwaita cursor as the default (instead of shipping our own in png format), but we also add support for all MetaCursors as root cursor (which most important should allow us to have I-beams in shell entries)
Marking this 3.10 for the PR implications of having HW cursors (we could say we're as fast as X)
Review of attachment 254189 [details] [review]: ::: src/core/meta-cursor-tracker.c @@ +128,3 @@ + +static MetaCursorReference * +meta_cursor_reference_load_file (MetaCursorTracker *tracker, from_file ? @@ +158,3 @@ + { + g_assert (n_channels == 4); + cogl_format = COGL_PIXEL_FORMAT_RGBA_8888; Shouldn't this take check for big vs. little endian? @@ +240,3 @@ + +static MetaCursorReference * +meta_cursor_reference_take_texture (CoglTexture2D *texture) Can we call this from_texture to be consistent with from_buffer ? @@ +320,3 @@ + if (width > 64 || height > 64) + { + meta_warning ("Invalid cursor size (must be at most 64x64), falling back to GL cursors\n"); Nitpick: I'd say "software cursor" .. it is a widely used term. Not sure people know what "GL cursors" mean. @@ +352,3 @@ + height = cogl_texture_get_height (COGL_TEXTURE (self->texture)); + + /* We can't complete EGL buffers, so they must be the right size from the start */ What does "complete buffer" mean in that context? @@ +355,3 @@ + if (width != 64 || height != 64) + { + meta_warning ("Invalid cursor size (must be 64x64), falling back to GL cursors\n"); Same re "software" vs. "GL" @@ +925,3 @@ +meta_cursor_tracker_set_crtc_has_hw_cursor (MetaCursorTracker *tracker, + MetaCRTC *crtc, + gboolean has) I don't really like the name "has" ... just call it has_hw_cursor ::: src/core/monitor.c @@ +1365,3 @@ unsigned int *n_outputs) { + if (modes) Unrelated?
Review of attachment 254445 [details] [review]: OK.
(In reply to comment #4) > Review of attachment 254189 [details] [review]: > > ::: src/core/meta-cursor-tracker.c > @@ +128,3 @@ > + > +static MetaCursorReference * > +meta_cursor_reference_load_file (MetaCursorTracker *tracker, > > from_file ? > > @@ +158,3 @@ > + { > + g_assert (n_channels == 4); > + cogl_format = COGL_PIXEL_FORMAT_RGBA_8888; > > Shouldn't this take check for big vs. little endian? No, COGL_PIXEL_FORMAT_RGBA_8888 is GL_BYTE+GL_RGBA, not GL_UNSIGNED_INT+GL_RGBA, ie, the bytes are always red-green-blue-alpha, which is what GdkPixbuf uses too. > @@ +240,3 @@ > + > +static MetaCursorReference * > +meta_cursor_reference_take_texture (CoglTexture2D *texture) > > Can we call this from_texture to be consistent with from_buffer ? It's take_texture() because it adopts the reference (from_buffer() makes a copy instead) > @@ +320,3 @@ > + if (width > 64 || height > 64) > + { > + meta_warning ("Invalid cursor size (must be at most 64x64), falling > back to GL cursors\n"); > > Nitpick: I'd say "software cursor" .. it is a widely used term. Not sure people > know what "GL cursors" mean. IIRC weston uses "GL cursors" too, and I feared "software cursors" could be read as software-rendered cursors, even though they're still rendered by the GPU, just in a different HW component. > @@ +352,3 @@ > + height = cogl_texture_get_height (COGL_TEXTURE (self->texture)); > + > + /* We can't complete EGL buffers, so they must be the right size from > the start */ > > What does "complete buffer" mean in that context? HW cursors must be 64x64, but 64x64 is huge, and no cursor theme actually uses that, so themed cursors must be padded with transparent pixels to fill the overlay. This is trivial if we have CPU access to the data, but it's not possible if the buffer is in GPU memory (and possibly tiled too) > ::: src/core/monitor.c > @@ +1365,3 @@ > unsigned int *n_outputs) > { > + if (modes) > > Unrelated? Not really. MetaMonitorManager assumed that the get_resources() caller was interested in all resource types (which is true for MetaMonitorConfig), but MetaCursorTracker only cares about CRTCs, and wants to pass NULL in the other out arguments.
(In reply to comment #6) > (In reply to comment #4) > > Review of attachment 254189 [details] [review] [details]: > > > > ::: src/core/meta-cursor-tracker.c > > @@ +128,3 @@ > > + > > +static MetaCursorReference * > > +meta_cursor_reference_load_file (MetaCursorTracker *tracker, > > > > from_file ? > > > > @@ +158,3 @@ > > + { > > + g_assert (n_channels == 4); > > + cogl_format = COGL_PIXEL_FORMAT_RGBA_8888; > > > > Shouldn't this take check for big vs. little endian? > > No, COGL_PIXEL_FORMAT_RGBA_8888 is GL_BYTE+GL_RGBA, not > GL_UNSIGNED_INT+GL_RGBA, ie, the bytes are always red-green-blue-alpha, which > is what GdkPixbuf uses too. Oh OK. > > @@ +240,3 @@ > > + > > +static MetaCursorReference * > > +meta_cursor_reference_take_texture (CoglTexture2D *texture) > > > > Can we call this from_texture to be consistent with from_buffer ? > > It's take_texture() because it adopts the reference (from_buffer() makes a copy > instead) OK. > > @@ +320,3 @@ > > + if (width > 64 || height > 64) > > + { > > + meta_warning ("Invalid cursor size (must be at most 64x64), falling > > back to GL cursors\n"); > > > > Nitpick: I'd say "software cursor" .. it is a widely used term. Not sure people > > know what "GL cursors" mean. > > IIRC weston uses "GL cursors" too, and I feared "software cursors" could be > read as software-rendered cursors, even though they're still rendered by the > GPU, just in a different HW component. OK fair enough ... never heard / read the term "GL cursor" before (but is not that far fetched so ok). > > @@ +352,3 @@ > > + height = cogl_texture_get_height (COGL_TEXTURE (self->texture)); > > + > > + /* We can't complete EGL buffers, so they must be the right size from > > the start */ > > > > What does "complete buffer" mean in that context? > > HW cursors must be 64x64, but 64x64 is huge, and no cursor theme actually uses > that, so themed cursors must be padded with transparent pixels to fill the > overlay. This is trivial if we have CPU access to the data, but it's not > possible if the buffer is in GPU memory (and possibly tiled too) OK, can you add something like that to the comment? "complete EGL buffer" is not really telling anything. > > ::: src/core/monitor.c > > @@ +1365,3 @@ > > unsigned int *n_outputs) > > { > > + if (modes) > > > > Unrelated? > > Not really. MetaMonitorManager assumed that the get_resources() caller was > interested in all resource types (which is true for MetaMonitorConfig), but > MetaCursorTracker only cares about CRTCs, and wants to pass NULL in the other > out arguments. Oh OK.
Review of attachment 254189 [details] [review]: OK, with a less vague comment re "complete buffers".
I should probably chime in that this can't land in its current state because it does not do hotspot manipulation correctly.
Review of attachment 254189 [details] [review]: See comment 9
(In reply to comment #9) > I should probably chime in that this can't land in its current state because it > does not do hotspot manipulation correctly. Mh? What are you talking about?
(In reply to comment #11) > (In reply to comment #9) > > I should probably chime in that this can't land in its current state because it > > does not do hotspot manipulation correctly. > > Mh? What are you talking about? Oh, I see, I fixed it locally but then never updated the patch
Pushed with some suggested changes, and with the hotspot problem fixed. Attachment 254189 [details] pushed as a3e44d1 - wayland: implement HW cursors Attachment 254445 [details] pushed as 7baf687 - MetaCursorTracker: add support for loading cursors from the theme