| Message ID | 20230414135151.75975-7-mcanal@igalia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/vkms: introduce plane rotation property | expand |
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@kms_rotation_crc@primary-reflect-y and > igt@kms_rotation_crc@sprite-reflect-y [1]. > > [1] https://patchwork.freedesktop.org/series/116025/ > > Signed-off-by: Maíra Canal <mcanal@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?
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@kms_rotation_crc@primary-reflect-y and >> igt@kms_rotation_crc@sprite-reflect-y [1]. >> >> [1] https://patchwork.freedesktop.org/series/116025/ >> >> Signed-off-by: Maíra Canal <mcanal@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. Best Regards, - Maíra Canal >
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@kms_rotation_crc@primary-reflect-y and > >> igt@kms_rotation_crc@sprite-reflect-y [1]. > >> > >> [1] https://patchwork.freedesktop.org/series/116025/ > >> > >> Signed-off-by: Maíra Canal <mcanal@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'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.
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@kms_rotation_crc@primary-reflect-y and >>>> igt@kms_rotation_crc@sprite-reflect-y [1]. >>>> >>>> [1] https://patchwork.freedesktop.org/series/116025/ >>>> >>>> Signed-off-by: Maíra Canal <mcanal@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
On Fri, Apr 14, 2023 at 02:53:55PM -0300, Maíra Canal wrote: > 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@kms_rotation_crc@primary-reflect-y and > >>>> igt@kms_rotation_crc@sprite-reflect-y [1]. > >>>> > >>>> [1] https://patchwork.freedesktop.org/series/116025/ > >>>> > >>>> Signed-off-by: Maíra Canal <mcanal@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. Dunno how vkms does this currently, but the most straightforward solution I can think of would be something like this: struct drm_rect src; drm_rect_fp_to_int(&src, plane_state->src); drm_rect_rotate(&src, fb->width, fb->height, rotation); for (dy = dst->y1; dy < dst->y2; dy++) { for (dx = dst->x1; dx < dst->x2; dx++) { int _sx = dx - dst->x1 + src.x1; int _sy = dy - dst->y1 + src.y1; struct drm_rect s; drm_rect_init(&s, _sx, _sy, 1, 1); drm_rect_rotate_inv(&s, fb->width, fb->height, rotation); output_pixel(dx, dy, s.x1, s.y1); } }
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); drm_rect_rotate(&frame_info->dst, drm_rect_width(&frame_info->dst), drm_rect_height(&frame_info->dst), frame_info->rotation); @@ -240,12 +236,8 @@ struct vkms_plane *vkms_plane_init(struct vkms_device *vkmsdev, drm_plane_helper_add(&plane->base, funcs); - drm_plane_create_rotation_property(&plane->base, DRM_MODE_ROTATE_0, - DRM_MODE_ROTATE_0 | - DRM_MODE_ROTATE_90 | - DRM_MODE_ROTATE_180 | - DRM_MODE_ROTATE_270 | - DRM_MODE_REFLECT_X); + drm_plane_create_rotation_property(&plane->base, DRM_MODE_ROTATE_0, DRM_MODE_ROTATE_MASK | + DRM_MODE_REFLECT_MASK); return plane; }
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@kms_rotation_crc@primary-reflect-y and igt@kms_rotation_crc@sprite-reflect-y [1]. [1] https://patchwork.freedesktop.org/series/116025/ Signed-off-by: Maíra Canal <mcanal@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(-)