[PATCH v2 6/7] drm/vkms: add reflect-y property

[Maíra Canal]

On 4/14/23 11:46, Ville Syrjälä wrote:
> On Fri, Apr 14, 2023 at 11:37:17AM -0300, Maíra Canal wrote:
>> On 4/14/23 11:24, Ville Syrjälä wrote:
>>> On Fri, Apr 14, 2023 at 10:51:50AM -0300, Maíra Canal wrote:
>>>> Currently, vkms only support the reflect-x property. Therefore, add the
>>>> reflect-y property to vkms through a software implementation of the
>>>> operation. This is possible by reverse reading the y axis during the
>>>> blending.
>>>>
>>>> Now, vkms support all possible rotation values.
>>>>
>>>> Tested with igt at kms_rotation_crc@primary-reflect-y and
>>>> igt at kms_rotation_crc@sprite-reflect-y [1].
>>>>
>>>> [1] https://patchwork.freedesktop.org/series/116025/
>>>>
>>>> Signed-off-by: Maíra Canal <mcanal at igalia.com>
>>>> ---
>>>>    drivers/gpu/drm/vkms/vkms_composer.c |  7 ++++++-
>>>>    drivers/gpu/drm/vkms/vkms_plane.c    | 16 ++++------------
>>>>    2 files changed, 10 insertions(+), 13 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
>>>> index b05bd008aeab..19d1078e9d34 100644
>>>> --- a/drivers/gpu/drm/vkms/vkms_composer.c
>>>> +++ b/drivers/gpu/drm/vkms/vkms_composer.c
>>>> @@ -92,8 +92,13 @@ static int get_y_pos(struct vkms_frame_info *frame_info, int y)
>>>>    			return -1;
>>>>    		return y + frame_info->dst.x1;
>>>>    	default:
>>>> -		return y;
>>>> +		break;
>>>>    	}
>>>> +
>>>> +	if (frame_info->rotation & DRM_MODE_REFLECT_Y)
>>>> +		return drm_rect_height(&frame_info->dst) - y - 1;
>>>> +
>>>> +	return y;
>>>>    }
>>>>    
>>>>    static bool check_limit(struct vkms_frame_info *frame_info, int pos)
>>>> diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
>>>> index 11662afa9fe4..d08bda869a24 100644
>>>> --- a/drivers/gpu/drm/vkms/vkms_plane.c
>>>> +++ b/drivers/gpu/drm/vkms/vkms_plane.c
>>>> @@ -121,12 +121,8 @@ static void vkms_plane_atomic_update(struct drm_plane *plane,
>>>>    	frame_info->fb = fb;
>>>>    	memcpy(&frame_info->map, &shadow_plane_state->data, sizeof(frame_info->map));
>>>>    	drm_framebuffer_get(frame_info->fb);
>>>> -	frame_info->rotation = drm_rotation_simplify(new_state->rotation,
>>>> -						     DRM_MODE_ROTATE_0 |
>>>> -						     DRM_MODE_ROTATE_90 |
>>>> -						     DRM_MODE_ROTATE_180 |
>>>> -						     DRM_MODE_ROTATE_270 |
>>>> -						     DRM_MODE_REFLECT_X);
>>>> +	frame_info->rotation = drm_rotation_simplify(new_state->rotation, DRM_MODE_ROTATE_MASK |
>>>> +						     DRM_MODE_REFLECT_MASK);
>>>
>>> What are you trying to achieve with that?
>>
>> Yeah, seeing it right now I can see that this is not achieving anything.
>> I will remove it in the next version.
> 
> I think using it might still make sense to eg. remove the 180/270
> cases from your actual rendering code.

I will remove it on the next version.

> 
> I'm also a bit uneasy about all that hand rolled coordinate calculation
> stuff. Ideally drm_rect_rotate*() should handle all that for you, and
> make sure the rotate vs. reflect actually get applied in the correct
> order.
> 

At first, I had a similar idea that drm_rect_rotate() would handle all the
rotation. But, this turn out to be untrue. From what I can see, although
the drm_rect_rotate() helps by performing the coordinates rotation, we also
need to change the way the blending occurs. Although the coordinates have
changed, the vmap stays the same and we will still start reading the vmap
by the first line (y = 0). That's why we need to change the iteration
boundaries in the blending loops.

So, drm_rect_rotate() is a part of the solution, but it is not able to handle
it completely.

If you have any suggestions on how to perform the rotation without changing
the way the blending occurs, I would be glad to hear. Unfortunately, just
performing drm_rect_rotate() doesn't seem to be a solution.

Best Regards,
- Maíra Canal