[v5,09/16] drm/vkms: Introduce pixel_read_direction enum

Commit Message

Louis Chauvet March 13, 2024, 5:45 p.m. UTC

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 <[e-mail]>
---
 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(+)

Comments

Pekka Paalanen March 25, 2024, 1:11 p.m. UTC | #1

On Wed, 13 Mar 2024 18:45:03 +0100
Louis Chauvet <[e-mail]> 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 <[e-mail]>
> ---
>  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
> @@ -159,6 +159,42 @@ static void apply_lut(const struct vkms_crtc_state *crtc_state, struct line_buff
>  	}
>  }
>  
> +/**
> + * direction_for_rotation() - Get the correct reading direction for a given rotation
> + *
> + * This function will use the @rotation setting of a source plane to compute the reading
> + * direction in this plane which correspond to a "left to right writing" in the CRTC.
> + * For example, if the buffer is reflected on X axis, the pixel must be read from right to left
> + * to be written from left to right on the CRTC.

That is a well written description.

> + *
> + * @rotation: Rotation to analyze. It correspond the field @frame_info.rotation.
> + */
> +static enum pixel_read_direction direction_for_rotation(unsigned int rotation)
> +{
> +	if (rotation & DRM_MODE_ROTATE_0) {
> +		if (rotation & DRM_MODE_REFLECT_X)
> +			return READ_RIGHT_TO_LEFT;
> +		else
> +			return READ_LEFT_TO_RIGHT;
> +	} else if (rotation & DRM_MODE_ROTATE_90) {
> +		if (rotation & DRM_MODE_REFLECT_Y)
> +			return READ_BOTTOM_TO_TOP;
> +		else
> +			return READ_TOP_TO_BOTTOM;
> +	} else if (rotation & DRM_MODE_ROTATE_180) {
> +		if (rotation & DRM_MODE_REFLECT_X)
> +			return READ_LEFT_TO_RIGHT;
> +		else
> +			return READ_RIGHT_TO_LEFT;
> +	} else if (rotation & DRM_MODE_ROTATE_270) {
> +		if (rotation & DRM_MODE_REFLECT_Y)
> +			return READ_TOP_TO_BOTTOM;
> +		else
> +			return READ_BOTTOM_TO_TOP;
> +	}
> +	return READ_LEFT_TO_RIGHT;

I'm a little worried seeing REFLECT_X is supported only for some
rotations, and REFLECT_Y for other rotations. Why is an analysis of all
combinations not necessary?

I hope IGT uses FB patterns instead of solid color in its tests of
rotation to be able to detect the difference.

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.

Btw. if there are already functions that are able to transform
coordinates based on the rotation bitfield, you could alternatively use
them. Transform CRTC point (0, 0) to A, and (1, 0) to B. Now A and B
are in plane coordinate system, and vector B - A gives you the
direction. The reason I'm mentioning this is that then you don't have
to implement yet another copy of the rotation bitfield semantics from
scratch.


> +}
> +
>  /**
>   * blend - blend the pixels from all planes and compute crc
>   * @wb: The writeback frame buffer metadata
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> index 3ead8b39af4a..985e7a92b7bc 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.h
> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> @@ -69,6 +69,17 @@ struct vkms_writeback_job {
>  	pixel_write_t pixel_write;
>  };
>  
> +/**
> + * enum pixel_read_direction - Enum used internaly by VKMS to represent a reading direction in a
> + * plane.
> + */
> +enum pixel_read_direction {
> +	READ_BOTTOM_TO_TOP,
> +	READ_TOP_TO_BOTTOM,
> +	READ_RIGHT_TO_LEFT,
> +	READ_LEFT_TO_RIGHT
> +};
> +
>  /**
>   * typedef pixel_read_t - These functions are used to read a pixel in the source frame,
>   * convert it to `struct pixel_argb_u16` and write it to @out_pixel.
> diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c
> index 649d75d05b1f..743b6fd06db5 100644
> --- a/drivers/gpu/drm/vkms/vkms_formats.c
> +++ b/drivers/gpu/drm/vkms/vkms_formats.c
> @@ -75,6 +75,36 @@ static void packed_pixels_addr(const struct vkms_frame_info *frame_info,
>  	*addr = (u8 *)frame_info->map[0].vaddr + offset;
>  }
>  
> +/**
> + * get_step_next_block() - Common helper to compute the correct step value between each pixel block
> + * to read in a certain direction.
> + *
> + * As the returned offset is the number of bytes between two consecutive blocks in a direction,
> + * the caller may have to read multiple pixel before using the next one (for example, to read from
> + * left to right in a DRM_FORMAT_R1 plane, each block contains 8 pixels, so the step must be used
> + * only every 8 pixels.
> + *
> + * @fb: Framebuffer to iter on
> + * @direction: Direction of the reading
> + * @plane_index: Plane to get the step from
> + */
> +static int get_step_next_block(struct drm_framebuffer *fb, enum pixel_read_direction direction,
> +			       int plane_index)
> +{

I would have called this something like get_block_step_bytes() for
example. That makes it clear it returns bytes (not e.g. pixels). "next"
implies to me that I tell the function the current block, and then it
gets me the next one. It does not do that, so I'd not use "next".

> +	switch (direction) {
> +	case READ_LEFT_TO_RIGHT:
> +		return fb->format->char_per_block[plane_index];
> +	case READ_RIGHT_TO_LEFT:
> +		return -fb->format->char_per_block[plane_index];
> +	case READ_TOP_TO_BOTTOM:
> +		return (int)fb->pitches[plane_index];
> +	case READ_BOTTOM_TO_TOP:
> +		return -(int)fb->pitches[plane_index];
> +	}
> +
> +	return 0;
> +}

Looks good.


Thanks,
pq

> +
>  static void *get_packed_src_addr(const struct vkms_frame_info *frame_info, int y,
>  				 int plane_index)
>  {
>

Maíra Canal March 25, 2024, 2:07 p.m. UTC | #2

On 3/13/24 14:45, Louis Chauvet 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 <[e-mail]>
> ---
>   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
> @@ -159,6 +159,42 @@ static void apply_lut(const struct vkms_crtc_state *crtc_state, struct line_buff
>   	}
>   }
>   
> +/**
> + * direction_for_rotation() - Get the correct reading direction for a given rotation
> + *
> + * This function will use the @rotation setting of a source plane to compute the reading
> + * direction in this plane which correspond to a "left to right writing" in the CRTC.
> + * For example, if the buffer is reflected on X axis, the pixel must be read from right to left
> + * to be written from left to right on the CRTC.
> + *
> + * @rotation: Rotation to analyze. It correspond the field @frame_info.rotation.

A bit unusual to see arguments after the description.

> + */
> +static enum pixel_read_direction direction_for_rotation(unsigned int rotation)
> +{
> +	if (rotation & DRM_MODE_ROTATE_0) {
> +		if (rotation & DRM_MODE_REFLECT_X)
> +			return READ_RIGHT_TO_LEFT;
> +		else
> +			return READ_LEFT_TO_RIGHT;
> +	} else if (rotation & DRM_MODE_ROTATE_90) {
> +		if (rotation & DRM_MODE_REFLECT_Y)
> +			return READ_BOTTOM_TO_TOP;
> +		else
> +			return READ_TOP_TO_BOTTOM;
> +	} else if (rotation & DRM_MODE_ROTATE_180) {
> +		if (rotation & DRM_MODE_REFLECT_X)
> +			return READ_LEFT_TO_RIGHT;
> +		else
> +			return READ_RIGHT_TO_LEFT;
> +	} else if (rotation & DRM_MODE_ROTATE_270) {
> +		if (rotation & DRM_MODE_REFLECT_Y)
> +			return READ_TOP_TO_BOTTOM;
> +		else
> +			return READ_BOTTOM_TO_TOP;
> +	}
> +	return READ_LEFT_TO_RIGHT;
> +}
> +
>   /**
>    * blend - blend the pixels from all planes and compute crc
>    * @wb: The writeback frame buffer metadata
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> index 3ead8b39af4a..985e7a92b7bc 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.h
> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> @@ -69,6 +69,17 @@ struct vkms_writeback_job {
>   	pixel_write_t pixel_write;
>   };
>   
> +/**
> + * enum pixel_read_direction - Enum used internaly by VKMS to represent a reading direction in a
> + * plane.
> + */
> +enum pixel_read_direction {
> +	READ_BOTTOM_TO_TOP,
> +	READ_TOP_TO_BOTTOM,
> +	READ_RIGHT_TO_LEFT,
> +	READ_LEFT_TO_RIGHT
> +};
> +
>   /**
>    * typedef pixel_read_t - These functions are used to read a pixel in the source frame,
>    * convert it to `struct pixel_argb_u16` and write it to @out_pixel.
> diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c
> index 649d75d05b1f..743b6fd06db5 100644
> --- a/drivers/gpu/drm/vkms/vkms_formats.c
> +++ b/drivers/gpu/drm/vkms/vkms_formats.c
> @@ -75,6 +75,36 @@ static void packed_pixels_addr(const struct vkms_frame_info *frame_info,
>   	*addr = (u8 *)frame_info->map[0].vaddr + offset;
>   }
>   
> +/**
> + * get_step_next_block() - Common helper to compute the correct step value between each pixel block
> + * to read in a certain direction.
> + *
> + * As the returned offset is the number of bytes between two consecutive blocks in a direction,
> + * the caller may have to read multiple pixel before using the next one (for example, to read from
> + * left to right in a DRM_FORMAT_R1 plane, each block contains 8 pixels, so the step must be used
> + * only every 8 pixels.
> + *
> + * @fb: Framebuffer to iter on
> + * @direction: Direction of the reading
> + * @plane_index: Plane to get the step from

Same.

Best Regards,
- Maíra

> + */
> +static int get_step_next_block(struct drm_framebuffer *fb, enum pixel_read_direction direction,
> +			       int plane_index)
> +{
> +	switch (direction) {
> +	case READ_LEFT_TO_RIGHT:
> +		return fb->format->char_per_block[plane_index];
> +	case READ_RIGHT_TO_LEFT:
> +		return -fb->format->char_per_block[plane_index];
> +	case READ_TOP_TO_BOTTOM:
> +		return (int)fb->pitches[plane_index];
> +	case READ_BOTTOM_TO_TOP:
> +		return -(int)fb->pitches[plane_index];
> +	}
> +
> +	return 0;
> +}
> +
>   static void *get_packed_src_addr(const struct vkms_frame_info *frame_info, int y,
>   				 int plane_index)
>   {
>

Louis Chauvet March 26, 2024, 3:57 p.m. UTC | #3

Le 25/03/24 - 15:11, Pekka Paalanen a écrit :
> On Wed, 13 Mar 2024 18:45:03 +0100
> Louis Chauvet <[e-mail]> 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 <[e-mail]>
> > ---
> >  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
> > @@ -159,6 +159,42 @@ static void apply_lut(const struct vkms_crtc_state *crtc_state, struct line_buff
> >  	}
> >  }
> >  
> > +/**
> > + * direction_for_rotation() - Get the correct reading direction for a given rotation
> > + *
> > + * This function will use the @rotation setting of a source plane to compute the reading
> > + * direction in this plane which correspond to a "left to right writing" in the CRTC.
> > + * For example, if the buffer is reflected on X axis, the pixel must be read from right to left
> > + * to be written from left to right on the CRTC.
> 
> That is a well written description.

Thanks
 
> > + *
> > + * @rotation: Rotation to analyze. It correspond the field @frame_info.rotation.
> > + */
> > +static enum pixel_read_direction direction_for_rotation(unsigned int rotation)
> > +{
> > +	if (rotation & DRM_MODE_ROTATE_0) {
> > +		if (rotation & DRM_MODE_REFLECT_X)
> > +			return READ_RIGHT_TO_LEFT;
> > +		else
> > +			return READ_LEFT_TO_RIGHT;
> > +	} else if (rotation & DRM_MODE_ROTATE_90) {
> > +		if (rotation & DRM_MODE_REFLECT_Y)
> > +			return READ_BOTTOM_TO_TOP;
> > +		else
> > +			return READ_TOP_TO_BOTTOM;
> > +	} else if (rotation & DRM_MODE_ROTATE_180) {
> > +		if (rotation & DRM_MODE_REFLECT_X)
> > +			return READ_LEFT_TO_RIGHT;
> > +		else
> > +			return READ_RIGHT_TO_LEFT;
> > +	} else if (rotation & DRM_MODE_ROTATE_270) {
> > +		if (rotation & DRM_MODE_REFLECT_Y)
> > +			return READ_TOP_TO_BOTTOM;
> > +		else
> > +			return READ_BOTTOM_TO_TOP;
> > +	}
> > +	return READ_LEFT_TO_RIGHT;
> 
> I'm a little worried seeing REFLECT_X is supported only for some
> rotations, and REFLECT_Y for other rotations. Why is an analysis of all
> combinations not necessary?

I don't need to manage all the combination because this is only about 
the "horizontal writing".

So, if you want to write a line in the CRTC, with:
- ROT_0 || REF_X => You need to read the source line from right to left
- ROT_0 => You need to read source buffer from left to right
- ROT_0 || REF_Y => You need to read the source line from left to right

In this case, REF_Y only have an effect on the "column reading". It is not 
needed here because the new version of the blend function will use the 
drm_rect_* helpers to compute the correct y coordinate.

If you think it's clearer, I can create a big switch(rotation) like this:

	switch (rotation) {
	case ROT_0:
	case ROT_0 || REF_X:
		return L2R;
	case ROT_0 || REF_Y:
		return R2L;
	case ROT_90:
	case ROT_90 || REF_X:
		return T2B;
	[...]
	}

So all cases are clearly covered?

> 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. 
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.

> 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.
 
> Btw. if there are already functions that are able to transform
> coordinates based on the rotation bitfield, you could alternatively use
> them. Transform CRTC point (0, 0) to A, and (1, 0) to B. Now A and B
> are in plane coordinate system, and vector B - A gives you the
> direction. The reason I'm mentioning this is that then you don't have
> to implement yet another copy of the rotation bitfield semantics from
> scratch.

You are totaly right. I will try this elegant method. Yes, there are some 
helpers (drm_rect_rotate_inv), so I will try to do something.

> 
> > +}
> > +
> >  /**
> >   * blend - blend the pixels from all planes and compute crc
> >   * @wb: The writeback frame buffer metadata
> > diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> > index 3ead8b39af4a..985e7a92b7bc 100644
> > --- a/drivers/gpu/drm/vkms/vkms_drv.h
> > +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> > @@ -69,6 +69,17 @@ struct vkms_writeback_job {
> >  	pixel_write_t pixel_write;
> >  };
> >  
> > +/**
> > + * enum pixel_read_direction - Enum used internaly by VKMS to represent a reading direction in a
> > + * plane.
> > + */
> > +enum pixel_read_direction {
> > +	READ_BOTTOM_TO_TOP,
> > +	READ_TOP_TO_BOTTOM,
> > +	READ_RIGHT_TO_LEFT,
> > +	READ_LEFT_TO_RIGHT
> > +};
> > +
> >  /**
> >   * typedef pixel_read_t - These functions are used to read a pixel in the source frame,
> >   * convert it to `struct pixel_argb_u16` and write it to @out_pixel.
> > diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c
> > index 649d75d05b1f..743b6fd06db5 100644
> > --- a/drivers/gpu/drm/vkms/vkms_formats.c
> > +++ b/drivers/gpu/drm/vkms/vkms_formats.c
> > @@ -75,6 +75,36 @@ static void packed_pixels_addr(const struct vkms_frame_info *frame_info,
> >  	*addr = (u8 *)frame_info->map[0].vaddr + offset;
> >  }
> >  
> > +/**
> > + * get_step_next_block() - Common helper to compute the correct step value between each pixel block
> > + * to read in a certain direction.
> > + *
> > + * As the returned offset is the number of bytes between two consecutive blocks in a direction,
> > + * the caller may have to read multiple pixel before using the next one (for example, to read from
> > + * left to right in a DRM_FORMAT_R1 plane, each block contains 8 pixels, so the step must be used
> > + * only every 8 pixels.
> > + *
> > + * @fb: Framebuffer to iter on
> > + * @direction: Direction of the reading
> > + * @plane_index: Plane to get the step from
> > + */
> > +static int get_step_next_block(struct drm_framebuffer *fb, enum pixel_read_direction direction,
> > +			       int plane_index)
> > +{
> 
> I would have called this something like get_block_step_bytes() for
> example. That makes it clear it returns bytes (not e.g. pixels). "next"
> implies to me that I tell the function the current block, and then it
> gets me the next one. It does not do that, so I'd not use "next".

Nice name, I will took it for the v6.

Thanks,
Louis Chauvet

> > +	switch (direction) {
> > +	case READ_LEFT_TO_RIGHT:
> > +		return fb->format->char_per_block[plane_index];
> > +	case READ_RIGHT_TO_LEFT:
> > +		return -fb->format->char_per_block[plane_index];
> > +	case READ_TOP_TO_BOTTOM:
> > +		return (int)fb->pitches[plane_index];
> > +	case READ_BOTTOM_TO_TOP:
> > +		return -(int)fb->pitches[plane_index];
> > +	}
> > +
> > +	return 0;
> > +}
> 
> Looks good.
> 
> 
> Thanks,
> pq
> 
> > +
> >  static void *get_packed_src_addr(const struct vkms_frame_info *frame_info, int y,
> >  				 int plane_index)
> >  {
> > 
>

Louis Chauvet March 26, 2024, 3:57 p.m. UTC | #4

Le 25/03/24 - 11:07, Maíra Canal a écrit :
> On 3/13/24 14:45, Louis Chauvet 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 <[e-mail]>
> > ---
> >   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
> > @@ -159,6 +159,42 @@ static void apply_lut(const struct vkms_crtc_state *crtc_state, struct line_buff
> >   	}
> >   }
> >   
> > +/**
> > + * direction_for_rotation() - Get the correct reading direction for a given rotation
> > + *
> > + * This function will use the @rotation setting of a source plane to compute the reading
> > + * direction in this plane which correspond to a "left to right writing" in the CRTC.
> > + * For example, if the buffer is reflected on X axis, the pixel must be read from right to left
> > + * to be written from left to right on the CRTC.
> > + *
> > + * @rotation: Rotation to analyze. It correspond the field @frame_info.rotation.
> 
> A bit unusual to see arguments after the description.

Fixed in v6.
 
> > + */
> > +static enum pixel_read_direction direction_for_rotation(unsigned int rotation)
> > +{
> > +	if (rotation & DRM_MODE_ROTATE_0) {
> > +		if (rotation & DRM_MODE_REFLECT_X)
> > +			return READ_RIGHT_TO_LEFT;
> > +		else
> > +			return READ_LEFT_TO_RIGHT;
> > +	} else if (rotation & DRM_MODE_ROTATE_90) {
> > +		if (rotation & DRM_MODE_REFLECT_Y)
> > +			return READ_BOTTOM_TO_TOP;
> > +		else
> > +			return READ_TOP_TO_BOTTOM;
> > +	} else if (rotation & DRM_MODE_ROTATE_180) {
> > +		if (rotation & DRM_MODE_REFLECT_X)
> > +			return READ_LEFT_TO_RIGHT;
> > +		else
> > +			return READ_RIGHT_TO_LEFT;
> > +	} else if (rotation & DRM_MODE_ROTATE_270) {
> > +		if (rotation & DRM_MODE_REFLECT_Y)
> > +			return READ_TOP_TO_BOTTOM;
> > +		else
> > +			return READ_BOTTOM_TO_TOP;
> > +	}
> > +	return READ_LEFT_TO_RIGHT;
> > +}
> > +
> >   /**
> >    * blend - blend the pixels from all planes and compute crc
> >    * @wb: The writeback frame buffer metadata
> > diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> > index 3ead8b39af4a..985e7a92b7bc 100644
> > --- a/drivers/gpu/drm/vkms/vkms_drv.h
> > +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> > @@ -69,6 +69,17 @@ struct vkms_writeback_job {
> >   	pixel_write_t pixel_write;
> >   };
> >   
> > +/**
> > + * enum pixel_read_direction - Enum used internaly by VKMS to represent a reading direction in a
> > + * plane.
> > + */
> > +enum pixel_read_direction {
> > +	READ_BOTTOM_TO_TOP,
> > +	READ_TOP_TO_BOTTOM,
> > +	READ_RIGHT_TO_LEFT,
> > +	READ_LEFT_TO_RIGHT
> > +};
> > +
> >   /**
> >    * typedef pixel_read_t - These functions are used to read a pixel in the source frame,
> >    * convert it to `struct pixel_argb_u16` and write it to @out_pixel.
> > diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c
> > index 649d75d05b1f..743b6fd06db5 100644
> > --- a/drivers/gpu/drm/vkms/vkms_formats.c
> > +++ b/drivers/gpu/drm/vkms/vkms_formats.c
> > @@ -75,6 +75,36 @@ static void packed_pixels_addr(const struct vkms_frame_info *frame_info,
> >   	*addr = (u8 *)frame_info->map[0].vaddr + offset;
> >   }
> >   
> > +/**
> > + * get_step_next_block() - Common helper to compute the correct step value between each pixel block
> > + * to read in a certain direction.
> > + *
> > + * As the returned offset is the number of bytes between two consecutive blocks in a direction,
> > + * the caller may have to read multiple pixel before using the next one (for example, to read from
> > + * left to right in a DRM_FORMAT_R1 plane, each block contains 8 pixels, so the step must be used
> > + * only every 8 pixels.
> > + *
> > + * @fb: Framebuffer to iter on
> > + * @direction: Direction of the reading
> > + * @plane_index: Plane to get the step from
> 
> Same.

Fixed in v6.

Thanks,
Louis Chauvet
 
> Best Regards,
> - Maíra
> 
> > + */
> > +static int get_step_next_block(struct drm_framebuffer *fb, enum pixel_read_direction direction,
> > +			       int plane_index)
> > +{
> > +	switch (direction) {
> > +	case READ_LEFT_TO_RIGHT:
> > +		return fb->format->char_per_block[plane_index];
> > +	case READ_RIGHT_TO_LEFT:
> > +		return -fb->format->char_per_block[plane_index];
> > +	case READ_TOP_TO_BOTTOM:
> > +		return (int)fb->pitches[plane_index];
> > +	case READ_BOTTOM_TO_TOP:
> > +		return -(int)fb->pitches[plane_index];
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >   static void *get_packed_src_addr(const struct vkms_frame_info *frame_info, int y,
> >   				 int plane_index)
> >   {
> >

Pekka Paalanen March 27, 2024, 12:16 p.m. UTC | #5

On Tue, 26 Mar 2024 16:57:00 +0100
Louis Chauvet <[e-mail]> wrote:

> Le 25/03/24 - 15:11, Pekka Paalanen a écrit :
> > On Wed, 13 Mar 2024 18:45:03 +0100
> > Louis Chauvet <[e-mail]> 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 <[e-mail]>
> > > ---
> > >  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
> > > @@ -159,6 +159,42 @@ static void apply_lut(const struct vkms_crtc_state *crtc_state, struct line_buff
> > >  	}
> > >  }
> > >  
> > > +/**
> > > + * direction_for_rotation() - Get the correct reading direction for a given rotation
> > > + *
> > > + * This function will use the @rotation setting of a source plane to compute the reading
> > > + * direction in this plane which correspond to a "left to right writing" in the CRTC.
> > > + * For example, if the buffer is reflected on X axis, the pixel must be read from right to left
> > > + * to be written from left to right on the CRTC.  
> > 
> > That is a well written description.  
> 
> Thanks
>  
> > > + *
> > > + * @rotation: Rotation to analyze. It correspond the field @frame_info.rotation.
> > > + */
> > > +static enum pixel_read_direction direction_for_rotation(unsigned int rotation)
> > > +{
> > > +	if (rotation & DRM_MODE_ROTATE_0) {
> > > +		if (rotation & DRM_MODE_REFLECT_X)
> > > +			return READ_RIGHT_TO_LEFT;
> > > +		else
> > > +			return READ_LEFT_TO_RIGHT;
> > > +	} else if (rotation & DRM_MODE_ROTATE_90) {
> > > +		if (rotation & DRM_MODE_REFLECT_Y)
> > > +			return READ_BOTTOM_TO_TOP;
> > > +		else
> > > +			return READ_TOP_TO_BOTTOM;
> > > +	} else if (rotation & DRM_MODE_ROTATE_180) {
> > > +		if (rotation & DRM_MODE_REFLECT_X)
> > > +			return READ_LEFT_TO_RIGHT;
> > > +		else
> > > +			return READ_RIGHT_TO_LEFT;
> > > +	} else if (rotation & DRM_MODE_ROTATE_270) {
> > > +		if (rotation & DRM_MODE_REFLECT_Y)
> > > +			return READ_TOP_TO_BOTTOM;
> > > +		else
> > > +			return READ_BOTTOM_TO_TOP;
> > > +	}
> > > +	return READ_LEFT_TO_RIGHT;  
> > 
> > I'm a little worried seeing REFLECT_X is supported only for some
> > rotations, and REFLECT_Y for other rotations. Why is an analysis of all
> > combinations not necessary?  
> 
> I don't need to manage all the combination because this is only about 
> the "horizontal writing".
> 
> So, if you want to write a line in the CRTC, with:
> - ROT_0 || REF_X => You need to read the source line from right to left
> - ROT_0 => You need to read source buffer from left to right
> - ROT_0 || REF_Y => You need to read the source line from left to right

That is true, indeed.

> In this case, REF_Y only have an effect on the "column reading". It is not 
> needed here because the new version of the blend function will use the 
> drm_rect_* helpers to compute the correct y coordinate.
> 
> If you think it's clearer, I can create a big switch(rotation) like this:
> 
> 	switch (rotation) {
> 	case ROT_0:
> 	case ROT_0 || REF_X:
> 		return L2R;
> 	case ROT_0 || REF_Y:
> 		return R2L;
> 	case ROT_90:
> 	case ROT_90 || REF_X:
> 		return T2B;
> 	[...]
> 	}
> 
> So all cases are clearly covered?

I think that would suit my personal taste better. It would not raise
questions nor need a comment. It does become a long function, but I
tend to favour long and clear more than short and needs thinking to
figure out if it works, everything else being equivalent.

I wonder how DRM maintainers feel.

> > 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.

> 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?

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. :-)

For me as a userspace developer, the important place is
https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#standard-plane-properties

>  
> > Btw. if there are already functions that are able to transform
> > coordinates based on the rotation bitfield, you could alternatively use
> > them. Transform CRTC point (0, 0) to A, and (1, 0) to B. Now A and B
> > are in plane coordinate system, and vector B - A gives you the
> > direction. The reason I'm mentioning this is that then you don't have
> > to implement yet another copy of the rotation bitfield semantics from
> > scratch.  
> 
> You are totaly right. I will try this elegant method. Yes, there are some 
> helpers (drm_rect_rotate_inv), so I will try to do something.


Cool, thanks,
pq

> >   
> > > +}
> > > +
> > >  /**
> > >   * blend - blend the pixels from all planes and compute crc
> > >   * @wb: The writeback frame buffer metadata
> > > diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> > > index 3ead8b39af4a..985e7a92b7bc 100644
> > > --- a/drivers/gpu/drm/vkms/vkms_drv.h
> > > +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> > > @@ -69,6 +69,17 @@ struct vkms_writeback_job {
> > >  	pixel_write_t pixel_write;
> > >  };
> > >  
> > > +/**
> > > + * enum pixel_read_direction - Enum used internaly by VKMS to represent a reading direction in a
> > > + * plane.
> > > + */
> > > +enum pixel_read_direction {
> > > +	READ_BOTTOM_TO_TOP,
> > > +	READ_TOP_TO_BOTTOM,
> > > +	READ_RIGHT_TO_LEFT,
> > > +	READ_LEFT_TO_RIGHT
> > > +};
> > > +
> > >  /**
> > >   * typedef pixel_read_t - These functions are used to read a pixel in the source frame,
> > >   * convert it to `struct pixel_argb_u16` and write it to @out_pixel.
> > > diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c
> > > index 649d75d05b1f..743b6fd06db5 100644
> > > --- a/drivers/gpu/drm/vkms/vkms_formats.c
> > > +++ b/drivers/gpu/drm/vkms/vkms_formats.c
> > > @@ -75,6 +75,36 @@ static void packed_pixels_addr(const struct vkms_frame_info *frame_info,
> > >  	*addr = (u8 *)frame_info->map[0].vaddr + offset;
> > >  }
> > >  
> > > +/**
> > > + * get_step_next_block() - Common helper to compute the correct step value between each pixel block
> > > + * to read in a certain direction.
> > > + *
> > > + * As the returned offset is the number of bytes between two consecutive blocks in a direction,
> > > + * the caller may have to read multiple pixel before using the next one (for example, to read from
> > > + * left to right in a DRM_FORMAT_R1 plane, each block contains 8 pixels, so the step must be used
> > > + * only every 8 pixels.
> > > + *
> > > + * @fb: Framebuffer to iter on
> > > + * @direction: Direction of the reading
> > > + * @plane_index: Plane to get the step from
> > > + */
> > > +static int get_step_next_block(struct drm_framebuffer *fb, enum pixel_read_direction direction,
> > > +			       int plane_index)
> > > +{  
> > 
> > I would have called this something like get_block_step_bytes() for
> > example. That makes it clear it returns bytes (not e.g. pixels). "next"
> > implies to me that I tell the function the current block, and then it
> > gets me the next one. It does not do that, so I'd not use "next".  
> 
> Nice name, I will took it for the v6.
> 
> Thanks,
> Louis Chauvet
> 
> > > +	switch (direction) {
> > > +	case READ_LEFT_TO_RIGHT:
> > > +		return fb->format->char_per_block[plane_index];
> > > +	case READ_RIGHT_TO_LEFT:
> > > +		return -fb->format->char_per_block[plane_index];
> > > +	case READ_TOP_TO_BOTTOM:
> > > +		return (int)fb->pitches[plane_index];
> > > +	case READ_BOTTOM_TO_TOP:
> > > +		return -(int)fb->pitches[plane_index];
> > > +	}
> > > +
> > > +	return 0;
> > > +}  
> > 
> > Looks good.
> > 
> > 
> > Thanks,
> > pq
> >   
> > > +
> > >  static void *get_packed_src_addr(const struct vkms_frame_info *frame_info, int y,
> > >  				 int plane_index)
> > >  {
> > >   
> >   
> 
> 
>

Louis Chauvet April 8, 2024, 7:50 a.m. UTC | #6

Le 27/03/24 - 14:16, Pekka Paalanen a écrit :
> On Tue, 26 Mar 2024 16:57:00 +0100
> Louis Chauvet <[e-mail]> wrote:
> 
> > Le 25/03/24 - 15:11, Pekka Paalanen a écrit :
> > > On Wed, 13 Mar 2024 18:45:03 +0100
> > > Louis Chauvet <[e-mail]> 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 <[e-mail]>
> > > > ---
> > > >  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
> > > > @@ -159,6 +159,42 @@ static void apply_lut(const struct vkms_crtc_state *crtc_state, struct line_buff
> > > >  	}
> > > >  }
> > > >  
> > > > +/**
> > > > + * direction_for_rotation() - Get the correct reading direction for a given rotation
> > > > + *
> > > > + * This function will use the @rotation setting of a source plane to compute the reading
> > > > + * direction in this plane which correspond to a "left to right writing" in the CRTC.
> > > > + * For example, if the buffer is reflected on X axis, the pixel must be read from right to left
> > > > + * to be written from left to right on the CRTC.  
> > > 
> > > That is a well written description.  
> > 
> > Thanks
> >  
> > > > + *
> > > > + * @rotation: Rotation to analyze. It correspond the field @frame_info.rotation.
> > > > + */
> > > > +static enum pixel_read_direction direction_for_rotation(unsigned int rotation)
> > > > +{
> > > > +	if (rotation & DRM_MODE_ROTATE_0) {
> > > > +		if (rotation & DRM_MODE_REFLECT_X)
> > > > +			return READ_RIGHT_TO_LEFT;
> > > > +		else
> > > > +			return READ_LEFT_TO_RIGHT;
> > > > +	} else if (rotation & DRM_MODE_ROTATE_90) {
> > > > +		if (rotation & DRM_MODE_REFLECT_Y)
> > > > +			return READ_BOTTOM_TO_TOP;
> > > > +		else
> > > > +			return READ_TOP_TO_BOTTOM;
> > > > +	} else if (rotation & DRM_MODE_ROTATE_180) {
> > > > +		if (rotation & DRM_MODE_REFLECT_X)
> > > > +			return READ_LEFT_TO_RIGHT;
> > > > +		else
> > > > +			return READ_RIGHT_TO_LEFT;
> > > > +	} else if (rotation & DRM_MODE_ROTATE_270) {
> > > > +		if (rotation & DRM_MODE_REFLECT_Y)
> > > > +			return READ_TOP_TO_BOTTOM;
> > > > +		else
> > > > +			return READ_BOTTOM_TO_TOP;
> > > > +	}
> > > > +	return READ_LEFT_TO_RIGHT;  
> > > 
> > > I'm a little worried seeing REFLECT_X is supported only for some
> > > rotations, and REFLECT_Y for other rotations. Why is an analysis of all
> > > combinations not necessary?  
> > 
> > I don't need to manage all the combination because this is only about 
> > the "horizontal writing".
> > 
> > So, if you want to write a line in the CRTC, with:
> > - ROT_0 || REF_X => You need to read the source line from right to left
> > - ROT_0 => You need to read source buffer from left to right
> > - ROT_0 || REF_Y => You need to read the source line from left to right
> 
> That is true, indeed.
> 
> > In this case, REF_Y only have an effect on the "column reading". It is not 
> > needed here because the new version of the blend function will use the 
> > drm_rect_* helpers to compute the correct y coordinate.
> > 
> > If you think it's clearer, I can create a big switch(rotation) like this:
> > 
> > 	switch (rotation) {
> > 	case ROT_0:
> > 	case ROT_0 || REF_X:
> > 		return L2R;
> > 	case ROT_0 || REF_Y:
> > 		return R2L;
> > 	case ROT_90:
> > 	case ROT_90 || REF_X:
> > 		return T2B;
> > 	[...]
> > 	}
> > 
> > So all cases are clearly covered?
> 
> I think that would suit my personal taste better. It would not raise
> questions nor need a comment. It does become a long function, but I
> tend to favour long and clear more than short and needs thinking to
> figure out if it works, everything else being equivalent.

I will change it to switch case.
 
> I wonder how DRM maintainers feel.
> 
> > > 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.
 
> > 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.

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.

> 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 :)

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 |
+-------+-------+

Thanks a lot,
Louis Chauvet

> For me as a userspace developer, the important place is
> https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#standard-plane-properties
> 
> >  
> > > Btw. if there are already functions that are able to transform
> > > coordinates based on the rotation bitfield, you could alternatively use
> > > them. Transform CRTC point (0, 0) to A, and (1, 0) to B. Now A and B
> > > are in plane coordinate system, and vector B - A gives you the
> > > direction. The reason I'm mentioning this is that then you don't have
> > > to implement yet another copy of the rotation bitfield semantics from
> > > scratch.  
> > 
> > You are totaly right. I will try this elegant method. Yes, there are some 
> > helpers (drm_rect_rotate_inv), so I will try to do something.

It works fine, a bit more tricky to get the right implementation. It will 
be in the v6.
 
> Cool, thanks,
> pq
> 
   
[...]

Pekka Paalanen April 9, 2024, 7:35 a.m. UTC | #7

On Mon, 8 Apr 2024 09:50:18 +0200
Louis Chauvet <[e-mail]> wrote:

> Le 27/03/24 - 14:16, Pekka Paalanen a écrit :
> > On Tue, 26 Mar 2024 16:57:00 +0100
> > Louis Chauvet <[e-mail]> wrote:
> >   
> > > Le 25/03/24 - 15:11, Pekka Paalanen a écrit :  
> > > > On Wed, 13 Mar 2024 18:45:03 +0100
> > > > Louis Chauvet <[e-mail]> 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 <[e-mail]>
> > > > > ---
> > > > >  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.

> 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'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 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
> >

Louis Chauvet April 9, 2024, 10:06 a.m. UTC | #8

Le 09/04/24 - 10:35, Pekka Paalanen a écrit :
> On Mon, 8 Apr 2024 09:50:18 +0200
> Louis Chauvet <[e-mail]> wrote:
> 
> > Le 27/03/24 - 14:16, Pekka Paalanen a écrit :
> > > On Tue, 26 Mar 2024 16:57:00 +0100
> > > Louis Chauvet <[e-mail]> wrote:
> > >   
> > > > Le 25/03/24 - 15:11, Pekka Paalanen a écrit :  
> > > > > On Wed, 13 Mar 2024 18:45:03 +0100
> > > > > Louis Chauvet <[e-mail]> 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 <[e-mail]>
> > > > > > ---
> > > > > >  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/[e-mail]/
 
> 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/[e-mail]/

> > 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
> > >

Patch

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
@@ -159,6 +159,42 @@  static void apply_lut(const struct vkms_crtc_state *crtc_state, struct line_buff
 	}
 }
 
+/**
+ * direction_for_rotation() - Get the correct reading direction for a given rotation
+ *
+ * This function will use the @rotation setting of a source plane to compute the reading
+ * direction in this plane which correspond to a "left to right writing" in the CRTC.
+ * For example, if the buffer is reflected on X axis, the pixel must be read from right to left
+ * to be written from left to right on the CRTC.
+ *
+ * @rotation: Rotation to analyze. It correspond the field @frame_info.rotation.
+ */
+static enum pixel_read_direction direction_for_rotation(unsigned int rotation)
+{
+	if (rotation & DRM_MODE_ROTATE_0) {
+		if (rotation & DRM_MODE_REFLECT_X)
+			return READ_RIGHT_TO_LEFT;
+		else
+			return READ_LEFT_TO_RIGHT;
+	} else if (rotation & DRM_MODE_ROTATE_90) {
+		if (rotation & DRM_MODE_REFLECT_Y)
+			return READ_BOTTOM_TO_TOP;
+		else
+			return READ_TOP_TO_BOTTOM;
+	} else if (rotation & DRM_MODE_ROTATE_180) {
+		if (rotation & DRM_MODE_REFLECT_X)
+			return READ_LEFT_TO_RIGHT;
+		else
+			return READ_RIGHT_TO_LEFT;
+	} else if (rotation & DRM_MODE_ROTATE_270) {
+		if (rotation & DRM_MODE_REFLECT_Y)
+			return READ_TOP_TO_BOTTOM;
+		else
+			return READ_BOTTOM_TO_TOP;
+	}
+	return READ_LEFT_TO_RIGHT;
+}
+
 /**
  * blend - blend the pixels from all planes and compute crc
  * @wb: The writeback frame buffer metadata
diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index 3ead8b39af4a..985e7a92b7bc 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -69,6 +69,17 @@  struct vkms_writeback_job {
 	pixel_write_t pixel_write;
 };
 
+/**
+ * enum pixel_read_direction - Enum used internaly by VKMS to represent a reading direction in a
+ * plane.
+ */
+enum pixel_read_direction {
+	READ_BOTTOM_TO_TOP,
+	READ_TOP_TO_BOTTOM,
+	READ_RIGHT_TO_LEFT,
+	READ_LEFT_TO_RIGHT
+};
+
 /**
  * typedef pixel_read_t - These functions are used to read a pixel in the source frame,
  * convert it to `struct pixel_argb_u16` and write it to @out_pixel.
diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c
index 649d75d05b1f..743b6fd06db5 100644
--- a/drivers/gpu/drm/vkms/vkms_formats.c
+++ b/drivers/gpu/drm/vkms/vkms_formats.c
@@ -75,6 +75,36 @@  static void packed_pixels_addr(const struct vkms_frame_info *frame_info,
 	*addr = (u8 *)frame_info->map[0].vaddr + offset;
 }
 
+/**
+ * get_step_next_block() - Common helper to compute the correct step value between each pixel block
+ * to read in a certain direction.
+ *
+ * As the returned offset is the number of bytes between two consecutive blocks in a direction,
+ * the caller may have to read multiple pixel before using the next one (for example, to read from
+ * left to right in a DRM_FORMAT_R1 plane, each block contains 8 pixels, so the step must be used
+ * only every 8 pixels.
+ *
+ * @fb: Framebuffer to iter on
+ * @direction: Direction of the reading
+ * @plane_index: Plane to get the step from
+ */
+static int get_step_next_block(struct drm_framebuffer *fb, enum pixel_read_direction direction,
+			       int plane_index)
+{
+	switch (direction) {
+	case READ_LEFT_TO_RIGHT:
+		return fb->format->char_per_block[plane_index];
+	case READ_RIGHT_TO_LEFT:
+		return -fb->format->char_per_block[plane_index];
+	case READ_TOP_TO_BOTTOM:
+		return (int)fb->pitches[plane_index];
+	case READ_BOTTOM_TO_TOP:
+		return -(int)fb->pitches[plane_index];
+	}
+
+	return 0;
+}
+
 static void *get_packed_src_addr(const struct vkms_frame_info *frame_info, int y,
 				 int plane_index)
 {