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

[Hans de Goede]

Hi,

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:
>>>>
>>>> xf86_crtc_rotate_coord_back() is described as "Given a cursor source
>>>> coordinate, rotate to a screen coordinate" which is exactly what we
>>>> need to properly position the cursor on a slave output, we start with
>>>> cursor coordinates and need to adjust those for the screen's rotation.
>>>
>>> No, transform_(gpu_)cursor_position[0] take screen space coordinates and
>>> transform them to CRTC space.
>>
>> Ah, ok. I'm not all that familiar with this code, so I'm going to take your
>> word for this.
>
> I was hoping my explanation would be clear enough that you don't have to
> take my word. :}
>
>
>> I'll do a new patch fixing xf86_crtc_rotate_coord instead of
>> xf86_crtc_rotate_coord_back, and also fix patch 6/7 to do things
>> differently.
>
> Thanks.
>
>
>>> [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. 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.

Regards,

Hans