Le 09/04/24 - 10:35, Pekka Paalanen a écrit : > On Mon, 8 Apr 2024 09:50:18 +0200 > Louis Chauvet <louis.chauvet@xxxxxxxxxxx> wrote: > > > Le 27/03/24 - 14:16, Pekka Paalanen a écrit : > > > On Tue, 26 Mar 2024 16:57:00 +0100 > > > Louis Chauvet <louis.chauvet@xxxxxxxxxxx> wrote: > > > > > > > Le 25/03/24 - 15:11, Pekka Paalanen a écrit : > > > > > On Wed, 13 Mar 2024 18:45:03 +0100 > > > > > Louis Chauvet <louis.chauvet@xxxxxxxxxxx> wrote: > > > > > > > > > > > The pixel_read_direction enum is useful to describe the reading direction > > > > > > in a plane. It avoids using the rotation property of DRM, which not > > > > > > practical to know the direction of reading. > > > > > > This patch also introduce two helpers, one to compute the > > > > > > pixel_read_direction from the DRM rotation property, and one to compute > > > > > > the step, in byte, between two successive pixel in a specific direction. > > > > > > > > > > > > Signed-off-by: Louis Chauvet <louis.chauvet@xxxxxxxxxxx> > > > > > > --- > > > > > > drivers/gpu/drm/vkms/vkms_composer.c | 36 ++++++++++++++++++++++++++++++++++++ > > > > > > drivers/gpu/drm/vkms/vkms_drv.h | 11 +++++++++++ > > > > > > drivers/gpu/drm/vkms/vkms_formats.c | 30 ++++++++++++++++++++++++++++++ > > > > > > 3 files changed, 77 insertions(+) > > > > > > > > > > > > diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c > > > > > > index 9254086f23ff..989bcf59f375 100644 > > > > > > --- a/drivers/gpu/drm/vkms/vkms_composer.c > > > > > > +++ b/drivers/gpu/drm/vkms/vkms_composer.c > > > > > > I hope IGT uses FB patterns instead of solid color in its tests of > > > > > rotation to be able to detect the difference. > > > > > > > > They use solid colors, and even my new rotation test [3] use solid colors. > > > > > > That will completely fail to detect rotation and reflection bugs then. > > > E.g. userspace asks for 180-degree rotation, and the driver does not > > > rotate at all. Or rotate-180 getting confused with one reflection. > > > > I think I missunderstood what you means with "solid colors". > > > > The tests uses a plane with multiple solid colors: > > > > +-------+-------+ > > | White | Red | > > +-------+-------+ > > | Blue | Green | > > +-------+-------+ > > > > But it don't use gradients because of YUV. > > > > Oh, that works. No worries then. > > > > > It is mainly for yuv formats with subsampling: if you have formats with > > > > subsampling, a "software rotated buffer" and a "hardware rotated buffer" > > > > will not apply the same subsampling, so the colors will be slightly > > > > different. > > > > > > Why would they not use the same subsampling? > > > > YUV422, for each pair of pixels along a horizontal line, the U and V > > components are shared between those two pixels. However, along a vertical > > line, each pixel has its own U and V components. > > > > When you rotate an image by 90 degrees: > > - Hardware Rotation: If you use hardware rotation, the YUV subsampling > > axis will align with what was previously the "White-Red" axis. The > > hardware will handle the rotation. > > - Software Rotation: If you use software rotation, the YUV subsampling > > axis will align with what was previously the "Red-Green" axis. > > That would be a bug in the software rotation. Yes, but it is very complex to fix I think, so I did not chose this path :) > > Because the subsampling compression axis changes depending on whether > > you're using hardware or software rotation, the compression effect on > > colors will differ. Specifically: > > - Hardware rotation, a gradient along the "White-Red" axis may be > > compressed (i.e same UV component for multiple pixels along the > > gradient). > > - Software rotation, the same gradient will not be compressed (i.e, each > > different color in the gradient have dedicated UV component) > > > > The same reasoning also apply for "color borders", and my series [3] avoid > > this issue by choosing the right number of pixels. > > What is [3]? I don't know why I put [3] here, I probably mixed references between mails [3]: https://lore.kernel.org/all/20240313-new_rotation-v2-0-6230fd5cae59@xxxxxxxxxxx/ > I've used similar tactics in the Weston test suite, when I have no > implementation for chroma siting: the input and reference images > consist of 2x2 equal color pixel groups, so that chroma siting makes no > difference. When chroma siting will be implemented, the tests will be > extended. > > Is there a TODO item to fix the software rotation bug and make the > tests more sensitive? > > I think documenting this would be an ok intermediate solution. > > > > The framebuffer contents are defined in its natural orientation, and > > > the subsampling applies in the natural orientation. If such a FB > > > is on a rotated plane, one must account for subsampling first, and > > > rotate second. 90-degree rotation does not change the encoded color. > > > > > > Getting the subsampling exactly right is going to be necessary sooner > > > or later. There is no UAPI for setting chroma siting yet, but ideally > > > there should be. > > > > > > > > The return values do seem correct to me, assuming I have guessed > > > > > correctly what "X" and "Y" refer to when combined with rotation. I did > > > > > not find good documentation about that. > > > > > > > > Yes, it is difficult to understand how rotation and reflexion should > > > > works in drm. I spend half a day testing all the combination in drm_rect_* > > > > helpers to understand how this works. According to the code: > > > > - If only rotation or only reflexion, easy as expected > > > > - If reflexion and rotation are mixed, the source buffer is first > > > > reflected and then rotated. > > > > > > Now that you know, you could send a documentation patch. :-) > > > > And now I'm not sure about it :) > > You'll have people review the patch and confirm your understanding or > point out a mistake. A doc patch it easier to notice and jump in than > this series. I just send it [4], you are in copy. [4]: https://lore.kernel.org/all/20240409-google-drm-doc-v1-0-033d55cc8250@xxxxxxxxxxx/ > > I was running the tests on my v6, and for the first time ran my new > > rotation [3] on the previous VKMS code. None of the tests for > > ROT_90+reflexion and ROT_270+reflexion are passing... > > > > So, either the previous vkms implementation was wrong, or mine is wrong :) > > > > So, if a DRM expert can explain this, it could be nice. > > > > To have a common example, if I take the same buffer as above > > (white+red+blue+green), if I create a plane with rotation = > > ROTATION_90 | REFLECTION_X, what is the expected result? > > > > 1 - rotation then reflection > > > > +-------+-------+ > > | Green | Red | > > +-------+-------+ > > | Blue | White | > > +-------+-------+ > > > > 2 - reflection then rotation (my vkms implementation) > > > > +-------+-------+ > > | White | Blue | > > +-------+-------+ > > | Red | Green | > > +-------+-------+ > > > > I wish I knew. :-) > > Thanks, > pq > > > > > For me as a userspace developer, the important place is > > > https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#standard-plane-properties > > >
