| Message ID | 20230418130525.128733-1-mcanal@igalia.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | drm/vkms: introduce plane rotation property | expand |
On 04/18, Maíra Canal wrote: > This patchset implements all possible rotation value in vkms. All operations > were implemented by software by changing the way the pixels are read. The way > the blending is performed can be depicted as: > > - rotate-0: > (x) ----> > ---------------------- > (y) | | > | | | > | | | > ˇ | | > ---------------------- > > - rotate-90: > <---- (y) > ---------------------- > (x) | | > | | | > | | | > ˇ | | > ---------------------- > > - rotate-180: > <---- (x) > ---------------------- > (y) | | > ^ | | > | | | > | | | > ---------------------- > > - rotate-270: > (y) ----> > ---------------------- > (x) | | > ^ | | > | | | > | | | > ---------------------- > > - reflect-x: > <---- (x) > ---------------------- > (y) | | > | | | > | | | > ˇ | | > ---------------------- > > - reflect-y: > (x) ----> > ---------------------- > (y) | | > ^ | | > | | | > | | | > ---------------------- > > The patchset was tested with IGT's kms_rotation_crc tests and also with some > additional tests [1] for the reflection operations. > > In order to avoid code duplication, I introduced a patch that isolates the > pixel format convertion and wraps it in a single loop. > > I tried to apply Ville's suggestion to avoid hand rolled coordinate > calculation stuff. Although I couldn't apply all the code suggested by > Ville, I was able to remove all the hardcoded code related to the x-offset. > As VKMS' composition is performed by line, I still need to indicate the > right pixel, which means that I still have some hardcoded code. Thanks for > the suggestion, Ville! It really reduced the code complexity. > > v1 -> v2: https://lore.kernel.org/dri-devel/20230406130138.70752-1-mcanal@igalia.com/T/ > > * Add patch to isolate pixel format conversion (Arthur Grillo). > > v2 -> v3: https://lore.kernel.org/dri-devel/20230414135151.75975-1-mcanal@igalia.com/T/ > > * Use cpp to calculate pixel size instead of hard-coding (Arthur Grillo). > * Don't use the x coordinate in the pixel_read functions (Arthur Grillo). > * Use drm_rotate_simplify() to avoid hard-coding rotate-180 (Ville Syrjälä). > * Use u8* to input the src_pixels instead of using u16*. > > v3 -> v4: https://lore.kernel.org/dri-devel/20230417121056.63917-1-mcanal@igalia.com/T/ > > * Create a original rectangle and a rotated rectangle and use the original > rectangle to offset the x-axis (Ville Syrjälä). > > [1] https://patchwork.freedesktop.org/series/116025/ Hi Maíra, Thanks for adding rotation properties to VKMS! Overall, LGTM and this series is: Reviewed-by: Melissa Wen <mwen@igalia.com> As you already applied the first patch, not a big issue, but I see new function documentation is missing. Could you send a follow-up patch documenting "vkms_compose_row()"? Also, would be good to apply the same improvement for the remaining conversion functions too. Thanks, Melissa > > Best Regards, > - Maíra Canal > > Maíra Canal (6): > drm/vkms: isolate pixel conversion functionality > drm/vkms: add rotate-0 and reflect-x property > drm/vkms: add reflect-y and rotate-180 property > drm/vkms: add rotate-90 property > drm/vkms: add rotate-270 property > drm/vkms: drop "Rotation" TODO > > Documentation/gpu/vkms.rst | 2 +- > drivers/gpu/drm/vkms/vkms_composer.c | 38 ++++++-- > drivers/gpu/drm/vkms/vkms_drv.h | 6 +- > drivers/gpu/drm/vkms/vkms_formats.c | 139 +++++++++++++-------------- > drivers/gpu/drm/vkms/vkms_formats.h | 2 +- > drivers/gpu/drm/vkms/vkms_plane.c | 16 ++- > 6 files changed, 117 insertions(+), 86 deletions(-) > > -- > 2.39.2 >
On 5/7/23 17:54, Melissa Wen wrote: > On 04/18, Maíra Canal wrote: >> This patchset implements all possible rotation value in vkms. All operations >> were implemented by software by changing the way the pixels are read. The way >> the blending is performed can be depicted as: >> >> - rotate-0: >> (x) ----> >> ---------------------- >> (y) | | >> | | | >> | | | >> ˇ | | >> ---------------------- >> >> - rotate-90: >> <---- (y) >> ---------------------- >> (x) | | >> | | | >> | | | >> ˇ | | >> ---------------------- >> >> - rotate-180: >> <---- (x) >> ---------------------- >> (y) | | >> ^ | | >> | | | >> | | | >> ---------------------- >> >> - rotate-270: >> (y) ----> >> ---------------------- >> (x) | | >> ^ | | >> | | | >> | | | >> ---------------------- >> >> - reflect-x: >> <---- (x) >> ---------------------- >> (y) | | >> | | | >> | | | >> ˇ | | >> ---------------------- >> >> - reflect-y: >> (x) ----> >> ---------------------- >> (y) | | >> ^ | | >> | | | >> | | | >> ---------------------- >> >> The patchset was tested with IGT's kms_rotation_crc tests and also with some >> additional tests [1] for the reflection operations. >> >> In order to avoid code duplication, I introduced a patch that isolates the >> pixel format convertion and wraps it in a single loop. >> >> I tried to apply Ville's suggestion to avoid hand rolled coordinate >> calculation stuff. Although I couldn't apply all the code suggested by >> Ville, I was able to remove all the hardcoded code related to the x-offset. >> As VKMS' composition is performed by line, I still need to indicate the >> right pixel, which means that I still have some hardcoded code. Thanks for >> the suggestion, Ville! It really reduced the code complexity. >> v1 -> v2: https://lore.kernel.org/dri-devel/20230406130138.70752-1-mcanal@igalia.com/T/ >> >> * Add patch to isolate pixel format conversion (Arthur Grillo). >> >> v2 -> v3: https://lore.kernel.org/dri-devel/20230414135151.75975-1-mcanal@igalia.com/T/ >> >> * Use cpp to calculate pixel size instead of hard-coding (Arthur Grillo). >> * Don't use the x coordinate in the pixel_read functions (Arthur Grillo). >> * Use drm_rotate_simplify() to avoid hard-coding rotate-180 (Ville Syrjälä). >> * Use u8* to input the src_pixels instead of using u16*. >> >> v3 -> v4: https://lore.kernel.org/dri-devel/20230417121056.63917-1-mcanal@igalia.com/T/ >> >> * Create a original rectangle and a rotated rectangle and use the original >> rectangle to offset the x-axis (Ville Syrjälä). >> >> [1] https://patchwork.freedesktop.org/series/116025/ > > Hi Maíra, > > Thanks for adding rotation properties to VKMS! > Overall, LGTM and this series is: > > Reviewed-by: Melissa Wen <mwen@igalia.com> Thanks for the review, Melissa! Applied to drm/drm-misc (drm-misc-next). > > As you already applied the first patch, not a big issue, but I see new > function documentation is missing. Could you send a follow-up patch > documenting "vkms_compose_row()"? Also, would be good to apply the same > improvement for the remaining conversion functions too. I'll try to work on some follow-up patches ASAP. Best Regards, - Maíra Canal > > Thanks, > > Melissa > >> >> Best Regards, >> - Maíra Canal >> >> Maíra Canal (6): >> drm/vkms: isolate pixel conversion functionality >> drm/vkms: add rotate-0 and reflect-x property >> drm/vkms: add reflect-y and rotate-180 property >> drm/vkms: add rotate-90 property >> drm/vkms: add rotate-270 property >> drm/vkms: drop "Rotation" TODO >> >> Documentation/gpu/vkms.rst | 2 +- >> drivers/gpu/drm/vkms/vkms_composer.c | 38 ++++++-- >> drivers/gpu/drm/vkms/vkms_drv.h | 6 +- >> drivers/gpu/drm/vkms/vkms_formats.c | 139 +++++++++++++-------------- >> drivers/gpu/drm/vkms/vkms_formats.h | 2 +- >> drivers/gpu/drm/vkms/vkms_plane.c | 16 ++- >> 6 files changed, 117 insertions(+), 86 deletions(-) >> >> -- >> 2.39.2 >>
This patchset implements all possible rotation value in vkms. All operations were implemented by software by changing the way the pixels are read. The way the blending is performed can be depicted as: - rotate-0: (x) ----> ---------------------- (y) | | | | | | | | ˇ | | ---------------------- - rotate-90: <---- (y) ---------------------- (x) | | | | | | | | ˇ | | ---------------------- - rotate-180: <---- (x) ---------------------- (y) | | ^ | | | | | | | | ---------------------- - rotate-270: (y) ----> ---------------------- (x) | | ^ | | | | | | | | ---------------------- - reflect-x: <---- (x) ---------------------- (y) | | | | | | | | ˇ | | ---------------------- - reflect-y: (x) ----> ---------------------- (y) | | ^ | | | | | | | | ---------------------- The patchset was tested with IGT's kms_rotation_crc tests and also with some additional tests [1] for the reflection operations. In order to avoid code duplication, I introduced a patch that isolates the pixel format convertion and wraps it in a single loop. I tried to apply Ville's suggestion to avoid hand rolled coordinate calculation stuff. Although I couldn't apply all the code suggested by Ville, I was able to remove all the hardcoded code related to the x-offset. As VKMS' composition is performed by line, I still need to indicate the right pixel, which means that I still have some hardcoded code. Thanks for the suggestion, Ville! It really reduced the code complexity. v1 -> v2: https://lore.kernel.org/dri-devel/20230406130138.70752-1-mcanal@igalia.com/T/ * Add patch to isolate pixel format conversion (Arthur Grillo). v2 -> v3: https://lore.kernel.org/dri-devel/20230414135151.75975-1-mcanal@igalia.com/T/ * Use cpp to calculate pixel size instead of hard-coding (Arthur Grillo). * Don't use the x coordinate in the pixel_read functions (Arthur Grillo). * Use drm_rotate_simplify() to avoid hard-coding rotate-180 (Ville Syrjälä). * Use u8* to input the src_pixels instead of using u16*. v3 -> v4: https://lore.kernel.org/dri-devel/20230417121056.63917-1-mcanal@igalia.com/T/ * Create a original rectangle and a rotated rectangle and use the original rectangle to offset the x-axis (Ville Syrjälä). [1] https://patchwork.freedesktop.org/series/116025/ Best Regards, - Maíra Canal Maíra Canal (6): drm/vkms: isolate pixel conversion functionality drm/vkms: add rotate-0 and reflect-x property drm/vkms: add reflect-y and rotate-180 property drm/vkms: add rotate-90 property drm/vkms: add rotate-270 property drm/vkms: drop "Rotation" TODO Documentation/gpu/vkms.rst | 2 +- drivers/gpu/drm/vkms/vkms_composer.c | 38 ++++++-- drivers/gpu/drm/vkms/vkms_drv.h | 6 +- drivers/gpu/drm/vkms/vkms_formats.c | 139 +++++++++++++-------------- drivers/gpu/drm/vkms/vkms_formats.h | 2 +- drivers/gpu/drm/vkms/vkms_plane.c | 16 ++- 6 files changed, 117 insertions(+), 86 deletions(-)