[PATCH v5 xserver 5/7] xf86Cursor: Fix xf86_crtc_rotate_coord_back using width/height wrongly

[Hans de Goede]

Hi,

On 08-09-16 09:24, Michel Dänzer wrote:
> On 08/09/16 04:18 PM, Hans de Goede wrote:
>> On 08-09-16 07:59, Michel Dänzer wrote:
>>> On 08/09/16 02:32 PM, Hans de Goede wrote:
>>>> On 08-09-16 03:10, Michel Dänzer wrote:
>>>>> On 08/09/16 12:43 AM, Hans de Goede wrote:
>>>>>>
>>>>> [0] BTW, do we really need two functions? The method used in
>>>>> xf86_crtc_transform_gpu_cursor_position should work for non-GPU screens
>>>>> as well, doesn't it?
>>>>
>>>> That uses crtc->f_framebuffer_to_crtc which gets setup by
>>>> xf86CrtcRotate(),
>>>> which at the very top has:
>>>>
>>>>     if (pScreen->isGPU)
>>>>         return TRUE;
>>>>
>>>> Which is why I went the route I went. I'll see if instead I can safely
>>>> modify xf86CrtcRotate() to still setup crtc->f_framebuffer_to_crtc,
>>>> without
>>>> it having any side-effects.
>>>
>>> What I mean is making xf86_crtc_transform_cursor_position use the same
>>> method as xf86_crtc_transform_gpu_cursor_position in your patch, instead
>>> of using crtc->f_framebuffer_to_crtc.
>>
>> I see, yes that is an interesting idea.
>>
>>> Then it should be usable for both cases?
>>
>> Well crtc->f_framebuffer_to_crtc also takes crtc->transform into account if
>> not NULL, so your suggested change would change the behavior of
>> xf86_crtc_transform_cursor_position in that case. Now with that said,
>> xf86_use_hw_cursor() does contain:
>>
>>         if (crtc->transformPresent)
>>             return FALSE;
>>
>> So if a transform is in use we should never end up in
>> xf86_crtc_transform_cursor_position, so your suggestion should probably
>> work.
>
> Exactly.
>
>
>> But I'm afraid that despite that it might still cause regressions
>> for non slave outputs, so I'm going to play it safe and keep the
>> separate function for now. We could try switching entirely to the new
>> version at the start of the next xserver devel cycle.
>
> What's the problem with testing non-slave outputs?

There is no problem I just prefer to be cautious. I'll give your suggested
approach a try and also test on non slave outputs for the next revision
of the patch-set.

Regards,

Hans