On Tue, 26 Mar 2024 16:57:03 +0100 Louis Chauvet <louis.chauvet@xxxxxxxxxxx> wrote: > Le 25/03/24 - 11:26, Maíra Canal a écrit : > > On 3/13/24 14:45, Louis Chauvet wrote: > > > From: Arthur Grillo <arthurgrillo@xxxxxxxxxx> > > > > > > Add support to the YUV formats bellow: > > > > > > - NV12/NV16/NV24 > > > - NV21/NV61/NV42 > > > - YUV420/YUV422/YUV444 > > > - YVU420/YVU422/YVU444 > > > > > > The conversion from yuv to rgb is done with fixed-point arithmetic, using > > > 32.32 floats and the drm_fixed helpers. > > > > > > To do the conversion, a specific matrix must be used for each color range > > > (DRM_COLOR_*_RANGE) and encoding (DRM_COLOR_*). This matrix is stored in > > > the `conversion_matrix` struct, along with the specific y_offset needed. > > > This matrix is queried only once, in `vkms_plane_atomic_update` and > > > stored in a `vkms_plane_state`. Those conversion matrices of each > > > encoding and range were obtained by rounding the values of the original > > > conversion matrices multiplied by 2^32. This is done to avoid the use of > > > floating point operations. > > > > > > The same reading function is used for YUV and YVU formats. As the only > > > difference between those two category of formats is the order of field, a > > > simple swap in conversion matrix columns allows using the same function. > > > > > > Signed-off-by: Arthur Grillo <arthurgrillo@xxxxxxxxxx> > > > [Louis Chauvet: > > > - Adapted Arthur's work > > > - Implemented the read_line_t callbacks for yuv > > > - add struct conversion_matrix > > > - remove struct pixel_yuv_u8 > > > - update the commit message > > > - Merge the modifications from Arthur] > > > > A Co-developed-by tag would be more appropriate. > > I am not the main author of this part, I only applied a few simple > suggestions, the complex part was done by Arthur. > > I will wait for Arthur's confirmation to change it to Co-developed by if > he agrees. Co-developed-by is an additional tag, and does not replace S-o-b. To my understanding, the kernel rules and Developers' Certificate of Origin require S-o-b to be added by anyone who has taken a patch and re-submitted it, regardless of who the original author is, and especially if the patch was modified. Personally I also like to keep the list of changes like Louis added, to credit people better. > > > Signed-off-by: Louis Chauvet <louis.chauvet@xxxxxxxxxxx> > > > --- > > > drivers/gpu/drm/vkms/vkms_drv.h | 22 ++ > > > drivers/gpu/drm/vkms/vkms_formats.c | 431 ++++++++++++++++++++++++++++++++++++ > > > drivers/gpu/drm/vkms/vkms_formats.h | 4 + > > > drivers/gpu/drm/vkms/vkms_plane.c | 17 +- > > > 4 files changed, 473 insertions(+), 1 deletion(-) > > > ... > > > }; > > > > > > static struct drm_plane_state * > > > @@ -117,12 +129,15 @@ static void vkms_plane_atomic_update(struct drm_plane *plane, > > > 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 | > > > > Why do we need to add DRM_MODE_ROTATE_180 here? Isn't the same as > > reflecting both along the X and Y axis? > > Oops, I had no intention of putting that change here. I will move it to > another patch. > > I don't understand why DRM_MODE_ROTATE_180 isn't in this list. If I read > the drm_rotation_simplify documentation, it explains that this argument > should contain all supported rotations and reflections, and ROT_180 is > supported by VKMS. Perhaps this call is unnecessary because all > combinations are supported by vkms? If you truly handle all bit patterns that the rotation bitfield can have, then yes, the call seems unnecessary. However, as documented, the bitfield contains redundancy: the same orientation can be expressed in more than one bit pattern. One example is that ROTATE_180 is equivalent to REFLECT_X | REFLECT_Y. Since it's a bitmask, userspace can give you funny values like ROTATE_0 | ROTATE_90 | ROTATE_180. That is a valid orientation of 270-degree rotation (according to UAPI doc), but it is very awkwardly expressed, hence the need to normalise it into a minimal bit pattern. It does not look like drm_rotation_simplify() actually does this minimisation! I was not able to tell if DRM common code actually stops userspace from combining multiple ROTATE bits in the same value. I suspect it must stop them, or perhaps all code dealing with rotation is actually broken. drm_rotation_simplify() is useful for cases where your hardware does not have exactly the same flexibility. Maybe it cannot do REFLECT_Y but it can do everything else? Then drm_rotation_simplify() gives you a bit pattern that you can use directly, or fails if the orientation is not representable with what your hardware can do. At least, that's my understanding of quickly glancing over it. IOW, if you wanted to never have to deal with REFLECT_Y bit, you could leave it out here. Or, if you never want to deal with ROTATE_180, leave that out - you will get REFLECT_X | REFLECT_Y instead. In theory. Thanks, pq
Attachment:
pgp2dfLF21dF0.pgp
Description: OpenPGP digital signature
