On 2018-12-12 3:58 p.m., Kazlauskas, Nicholas wrote:
> On 12/12/18 9:40 AM, Michel Dänzer wrote:
>> On 2018-12-12 3:17 p.m., Kazlauskas, Nicholas wrote:
>>> On 12/12/18 9:10 AM, Michel Dänzer wrote:
>>>> On 2018-12-12 3:04 p.m., Nicholas Kazlauskas wrote:
>>>>> [Why]
>>>>> The cursor calculations in amdgpu_dm incorrectly assume that the
>>>>> cursor hotspot is always (0, 0) and don't respect the hot_x and hot_y
>>>>> attributes that can be passed in via the drm_mode_cursor2_ioctl.
>>>>>
>>>>> The DC hotspot parameters are also incorrectly used to offset the
>>>>> cursor when it goes beyond the bounds of the screen instead of
>>>>> being clamped.
>>>>>
>>>>> [How]
>>>>> Use the hot_x and hot_y attributes from the fb to directly fill in the
>>>>> DC hotspot attributes.
>>>>>
>>>>> Clamp the cursor position on the edges of the screen instead of using
>>>>> the hotspot to offset it back in.
>>>>>
>>>>> Cc: Leo Li <sunpeng.li@xxxxxxx>
>>>>> Cc: Harry Wentland <harry.wentland@xxxxxxx>
>>>>> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@xxxxxxx>
>>>>> ---
>>>>> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 23 +++++++------------
>>>>> 1 file changed, 8 insertions(+), 15 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>>> index 01badda14079..61f2eae0b67f 100644
>>>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>>> @@ -4266,7 +4266,6 @@ static int get_cursor_position(struct drm_plane *plane, struct drm_crtc *crtc,
>>>>> {
>>>>> struct amdgpu_crtc *amdgpu_crtc = to_amdgpu_crtc(crtc);
>>>>> int x, y;
>>>>> - int xorigin = 0, yorigin = 0;
>>>>>
>>>>> if (!crtc || !plane->state->fb) {
>>>>> position->enable = false;
>>>>> @@ -4284,24 +4283,18 @@ static int get_cursor_position(struct drm_plane *plane, struct drm_crtc *crtc,
>>>>> return -EINVAL;
>>>>> }
>>>>>
>>>>> - x = plane->state->crtc_x;
>>>>> - y = plane->state->crtc_y;
>>>>> + x = plane->state->crtc_x + plane->state->fb->hot_x;
>>>>> + y = plane->state->crtc_y + plane->state->fb->hot_y;
>>>>> +
>>>>> /* avivo cursor are offset into the total surface */
>>>>> x += crtc->primary->state->src_x >> 16;
>>>>> y += crtc->primary->state->src_y >> 16;
>>>>> - if (x < 0) {
>>>>> - xorigin = min(-x, amdgpu_crtc->max_cursor_width - 1);
>>>>> - x = 0;
>>>>> - }
>>>>> - if (y < 0) {
>>>>> - yorigin = min(-y, amdgpu_crtc->max_cursor_height - 1);
>>>>> - y = 0;
>>>>> - }
>>>>> +
>>>>> position->enable = true;
>>>>> - position->x = x;
>>>>> - position->y = y;
>>>>> - position->x_hotspot = xorigin;
>>>>> - position->y_hotspot = yorigin;
>>>>> + position->x = x >= 0 ? x : 0;
>>>>> + position->y = y >= 0 ? y : 0;
>>>>> + position->x_hotspot = plane->state->fb->hot_x;
>>>>> + position->y_hotspot = plane->state->fb->hot_y;
>>>>
>>>> I don't see how this could work correctly. Try moving a cursor which
>>>> doesn't have the hot spot at the top/left edge (e.g. the X-shaped cursor
>>>> used by an X server without any clients) beyond the top/left edge of the
>>>> monitor.
>>>>
>>>>
>>> Do you mean the clamping? I was just keeping the driver's previous
>>> behavior regarding this.
>>>
>>> It previously forced the position to 0 but offset the cursor using the
>>> hotspot attribute the more it sunk into the top or left edge. You can
>>> see this with the default GNOME3 cursor (which doesn't have a hotspot of
>>> 0, 0) and it sinks into the edge on the screen by a few pixels.
>>>
>>> With this patch the cursor position is now always aligned with the first
>>> black pixel instead of the white border - and not just whenever it
>>> starts sinking into the top or left edge.
>>
>> I think you need to at least remove the clamping to 0:
>>
>> position->x = x >= 0 ? x : 0;
>> position->y = y >= 0 ? y : 0;
>>
>> Otherwise the cursor cannot move up/left beyond its hotspot, but that
>> can be necessary e.g. when moving the cursor between monitors.
>>
>>
>> That is assuming other code handles position position->x/y and
>> position->x/y_hotspot correctly in all cases other than that.
>>
>>
>
> Ah, I see what you mean.
>
> The X cursor makes it really obvious as to what's actually going on
> regrading pos/hotspot calculations too. The reason why this looked
> correct before is that the destination position seems to come pre-offset
> by the cursor hotspot. So DC hotspot parameters of (0, 0) still look
> correct even if that doesn't reflect what's actually happening (though
> certainly looks wrong from first glance, especially with the min(-x,
> ...) part).
Beware that 'hotspot' has slightly different meanings at the X / KMS /
hardware level.
At the KMS API level, the hotspot is just a hint, which can be used for
automatically re-positioning the HW cursor when the hotspot changes. The
cursor position in the API always refers to the top/left corner of the
cursor regardless of the hotspot.
Assuming the position->x/y(_hotspot) values are programmed to the
CUR_POSITION/HOT_SPOT registers directly, the code is correct as is.
--
Earthling Michel Dänzer | http://www.amd.com
Libre software enthusiast | Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
