[PATCH xserver] modesetting: allow switching from software to hardware cursors (v3).

[Hans de Goede]

Hi,

On 15-09-16 20:53, Michael Thayer wrote:
> Hello,
>
> On 15.09.2016 19:18, Hans de Goede wrote:
>> Hi,

<snip>

>>>>>  * 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 ?   :
>>>>
>>>> https://cgit.freedesktop.org/xorg/xserver/commit/hw/xfree86/drivers/modesetting?id=af916477c65a083ec496ac3f088d766b410e8b6e
>>>>
>>>>
>>>>
>>>> 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:

https://cgit.freedesktop.org/xorg/xserver/commit/hw/xfree86/drivers/modesetting?id=af916477c65a083ec496ac3f088d766b410e8b6e

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,

Hans


<more snip>