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:
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,
email@example.com: X.Org development