<html> <head> <base href="https://bugzilla.gnome.org/" /> </head> <body> <div> <a class="bz_bug_link bz_status_NEW " title="NEW - Wrong (ultra tiny/small) cursor size on HiDPI screen" href="https://bugzilla.gnome.org/show_bug.cgi?id=744932#c117">Comment # 117</a> on <a class="bz_bug_link bz_status_NEW " title="NEW - Wrong (ultra tiny/small) cursor size on HiDPI screen" href="https://bugzilla.gnome.org/show_bug.cgi?id=744932">bug 744932</a> from <a href="page.cgi?id=describeuser.html&login=jadahl%40gmail.com" title="Jonas Ådahl <jadahl@gmail.com>"> Jonas Ådahl</a> <pre>I have just pushed a new version of the wip/hidpi-cursor branch to github. Do you want me to update the patches in this bug? In summary, it fixes most of the issues you raised, but some things I will comment on more explicitly: I did not change nor rename any of the role-ness of XWayland wl_surfaces. I believe making it a role internally still makes most sense. (In reply to Owen Taylor from <a href="show_bug.cgi?id=744932#c110">comment #110</a>) > Review of <a href="attachment.cgi?id=309538&action=diff" name="attach_309538" title="Support scaling of cursor sprites given what output they are on">attachment 309538</a> <a href="attachment.cgi?id=309538&action=edit" title="Support scaling of cursor sprites given what output they are on">[details]</a> <a href='review?bug=744932&attachment=309538'>[review]</a> [review]: > > ::: src/backends/meta-cursor-renderer.c > @@ +65,2 @@ > if (priv->displayed_cursor && !priv->handled_by_backend) > + texture = meta_cursor_sprite_get_cogl_texture (priv->displayed_cursor); > > Shouldn't this use cursor_sprite? - or alternatively, why is cursor_sprite > passed in when it seems like it will always be priv->displayed_cursor? I still avoid using renderer_priv->displayed_cursor or meta_cursor_renderer_get_cursor() because my longer-term intention is to get rid of that state, considering that it is just a duplication of the same state tracked by meta-cursor-tracker.c. I avoid it even further in the newer version, but not completely. > ::: src/backends/meta-cursor-renderer.h > @@ +44,3 @@ > > + gboolean (* update_cursor) (MetaCursorRenderer *renderer, > + MetaCursorSprite *cursor_sprite); > > I found it confusing reading the code that: > > update_cursor > > Updates the internal state of the *renderer* based on the cursor_sprite. > While > > realize_cursor_for_wl_buffer > > Updates the state of the *cursor*. > > I don't really understand why the change to add the cursor_sprite parameter > was needed, and I think it contributes to the problem by making the > non-parallel methods in the interface look more parallel. > I pass the sprite because I don't want to add more uses of priv->displayed_cursor (explained above). > ::: src/backends/native/meta-cursor-renderer-native.c > @@ +62,3 @@ > +{ > + guint current_bo; > + struct gbm_bo *bos[2]; > > A comment about why the two bos would be *very* useful. I actually needed to use an array of three, along with an explanation in the relevant patch. > > This was discussed some in IRC and email, and it turns out that the likely > situation that was going wrong and causing glitches was: > > We call drmModeSetCursor2() with buffer A > We call gbm_bo_destroy() on the old buffer > Instead of freeing the old buffer, the intel libdrm driver stores it in a > client-side pool. > We call gbm_bo_create() and get the *same* BO back - which is still being > scanned out as the cursor > We modify it, causing the glitch > > I think for now we can consider that drmModeSetCursor2() completely replaces > any references to previously set cursors and at that point the old buffer > can be reused, so that mostly the approach you are taking here should work. > > As was noted in your email, the logic here isn't really right for handling > that ,because the "flip" is done when the cursor is changed, rather than at > the time the drmModeSetCursor2() call is called. To me, this makes the code > here extremely hard to understand ... it logically doesn't hang together. > Honestly, I'd like to see that fixed before this is committed even if it > mostly works now. The new version of the big-chunk-patch does this now. It could even more refactoring, but in summary, drmModeSetCursor2() is now called once per frame only, and updating the cursor will queue a redraw which will cause calling of the whole stage to be redrawn (well, not really, because there may be no damage) with all the drm ioctls with it. (In reply to Owen Taylor from <a href="show_bug.cgi?id=744932#c116">comment #116</a>) > (In reply to Jonas Ådahl from <a href="show_bug.cgi?id=744932#c114">comment #114</a>) > > (In reply to Owen Taylor from <a href="show_bug.cgi?id=744932#c111">comment #111</a>) > > > Review of <a href="attachment.cgi?id=309538&action=diff" name="attach_309538" title="Support scaling of cursor sprites given what output they are on">attachment 309538</a> <a href="attachment.cgi?id=309538&action=edit" title="Support scaling of cursor sprites given what output they are on">[details]</a> <a href='review?bug=744932&attachment=309538'>[review]</a> [review] [review] [review]: > > > > > > I have this feeling that it somehow could be simpler (like why does > > > realization have to be triggered throughout the code - couldn't it just be > > > done by the backend when needed?) but as far as I can follow the code it > > > seems like it should work. Some more style comments follow: > > > > This is because the data backing the cursor may not part of the actual > > cursor (the wl_surface) so if we'd want to realize it just before drawing, > > we wouldn't know how to realize it, since the cursor doesn't know about the > > wl_surface. We could probably add a signal instead that the creator needs to > > set, or a "backing object" so that the renderer can call > > "cursor->backing->realize()" or something. With the current approach the > > renderer specific realization (which is what realize does) is done at > > creation when the source of the data is known. > > That's not really what "realize" means in a GTK+/Clutter context - realize > means "create the resources now that everything is set up". It would seem to > me there would be more flexibility from cursor_sprite_set_wl_buffer/xcursor > that stored them, and let the cursor renderer pick up the data and create > further backend resources at need. But anyways, let's get this landed. > That is kind of why I used that naming. It creates the renderer specific backing after all the available information is available. But in the future, I'd like to make this happen just before the drmModeSetCursor2 once per frame, and avoid creating any unnecessary buffers, but I believe that can wait.</pre> </div> <hr> You are receiving this mail because: <ul> <li>You are on the CC list for the bug.</li> </ul> </body> </html>