Re: [PATCH v4 0/6] drm/vkms: introduce plane rotation property

[Melissa Wen <mwen@igalia.com>]

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
>