On 15-09-16 20:53, Michael Thayer wrote:

On 15.09.2016 19:18, Hans de Goede wrote:


 * drop the special case for loading a cursor sprite when the cursor is
   hidden: we set HARDWARE_CURSOR_UPDATE_UNHIDDEN, so all calls to
   load_cursor will be followed by calls to show_cursor unless the load
   fails (when a call to hide_cursor should follow)

Nack again, have you read the actual commit msg of the commit which
introduced the change ?   :


If we revert this change (which really should be in a separate patch
btw) then software cursor fallback will not work reliably because
the first drmmode_load_cursor_argb_check() will succeed as
it is a nop when the cursor is hidden, at which point the server
will continue trying to use the hw-cursor until the cursor
is changed to a different cursor.

I did actually read the commit message, as well as the patch itself.
My patch removes the problem which that patch was intended to fix,

I do not think so, the problem as described there is that the
first call to drmmode_load_cursor_argb_check() will succeed
on systems without hw-cursor capability at all, causing
the server to stay in hw-cursor mode.

I honestly did understand what the problem is.  But the reason that the
> call will succeed at all is because of a short section of code in
> modesetting/drmmode_display.c, which my patch removes.

/me goes and looks at the v3 patch again.

Ah I see now, you do not just revert:


You fix the issue it was fixing be replacing the:

if (cursor_up) return drmmode_set_cursor()
else return TRUE

With an unconditional:

return drmmode_set_cursor()

Now the downside of that is that this will cause a cursor to show
even if show_cursor was never called / the current cursor state
is hidden. Reading between the lines you're claiming that this
never happens. If that indeed never happens then your fix indeed
is a good idea.

Notice that I read over the replacing of "return TRUE" with
return drmmode_set_cursor() the first time, this is my mistake,
but this is also caused by you doing too much in a single commit.

If you believe that the entire first_time dance is not
necessary and that instead drmmode_load_cursor_argb_check()
should always call drmmode_set_cursor(), not only loading
the cursor but also showing it, then please submit a separate
patch doing that, and only that.

So assuming I've understood your intentions correctly, here
is how I would like to see things move forward with this patch,
turn it into a 3 patch patch-set:

Patch 1: Re-introduce the -EINVAL check for trying the non
"2" version of the drmcursor ioctl, with the reasoning you
send me off-list in the commit msg

Patch 2: Replace the first_time dance in
drmmode_load_cursor_argb_check() with an unconditional
call to drmmode_set_cursor(). Note I'm not yet convinced
this is the right thing todo, I assume you've done some
analysis of all the call paths to load_cursor_argb_check()
which have made you come to the conclusion that this
is the right thing todo, include that analysis in your
commit msg please.

Patch 3: Drop set_cursor2_failed caching and setting
of cursor_info->MaxWidth = cursor_info->MaxHeight = 0; /
drmmode_crtc->drmmode->sw_cursor = TRUE; from
drmmode_set_cursor() so that a later
drmmode_load_cursor_argb_check() call can upgrade the
cursor from a sw cursor to a hw cursor.

Thanks & Regards,


<more snip>
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Reply via email to