[PATCH 0/3] drm: Multiple documentation update

[PATCH 0/3] drm: Multiple documentation update

[Louis Chauvet]

PATCH 1 and PATCH 2 focus on the rotation property. The rotation property 
can be challenging to understand, especially when it is combined with 
reflections. These patches aim to provide clearer explanations and 
examples to aid in comprehension.

Patch 3 relates to the fourcc property. It includes additional details 
about block and char_per_block to provide a more comprehensive 
understanding of this feature.

Regarding PATCH 1, I would appreciate some feedback on the expected 
behavior. During a recent VKMS refactor, I used drm_rect_rotate as a 
reference for the rotation. However, during my testing phase, I noticed 
that the original VKMS implementation interpreted the rotation 
differently. Therefore, I kindly request that someone validate or 
invalidate my interpretation before proceeding with the merge.

Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
---
Louis Chauvet (3):
      drm: drm_blend.c: Add precision in drm_rotation_simplify kernel doc
      drm: drm_blend.c: Improve drm_plane_create_rotation_property kernel doc
      drm/fourcc: Add documentation around drm_format_info

 drivers/gpu/drm/drm_blend.c | 57 ++++++++++++++++++++++++++++++++++-----------
 include/drm/drm_fourcc.h    | 45 +++++++++++++++++++++++++++++------
 2 files changed, 81 insertions(+), 21 deletions(-)
---
base-commit: e495e523b888a6155f82c767d34c8d712a41ee54
change-id: 20240327-google-drm-doc-cd275291792f

Best regards,
-- 
Louis Chauvet <louis.chauvet@bootlin.com>

[PATCH 1/3] drm: drm_blend.c: Add precision in drm_rotation_simplify kernel doc

[Louis Chauvet]

As the function uses non-trivial bit operations, add a little paragraph
to describe what is the expected behavior.

Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
---
 drivers/gpu/drm/drm_blend.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
index 6e74de833466..8d4b317eb9d7 100644
--- a/drivers/gpu/drm/drm_blend.c
+++ b/drivers/gpu/drm/drm_blend.c
@@ -321,6 +321,11 @@ EXPORT_SYMBOL(drm_plane_create_rotation_property);
  * transforms the hardware supports, this function may not
  * be able to produce a supported transform, so the caller should
  * check the result afterwards.
+ *
+ * If the rotation is not fully supported, this function will add a rotation of 180
+ * (ROTATE_90 would become ROTATE_270) and add a reflection on X and Y.
+ * The resulting transformation is the same (REFLECT_X | REFLECT_Y | ROTATE_180
+ * is a no-op), but some unsupported flags are removed.
  */
 unsigned int drm_rotation_simplify(unsigned int rotation,
 				   unsigned int supported_rotations)

-- 
2.43.0

[PATCH 2/3] drm: drm_blend.c: Improve drm_plane_create_rotation_property kernel doc

[Louis Chauvet]

The expected behavior of the rotation property was not very clear. Add
more examples to explain what is the expected result.

Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
---
 drivers/gpu/drm/drm_blend.c | 52 +++++++++++++++++++++++++++++++++------------
 1 file changed, 38 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
index 8d4b317eb9d7..6fbb8730d8b0 100644
--- a/drivers/gpu/drm/drm_blend.c
+++ b/drivers/gpu/drm/drm_blend.c
@@ -104,6 +104,9 @@
  *	Without this property the rectangle is only scaled, but not rotated or
  *	reflected.
  *
+ *	See drm_plane_create_rotation_property() for details about the expected rotation and
+ *	reflection behavior.
+ *
  *	Possbile values:
  *
  *	"rotate-<degrees>":
@@ -114,18 +117,6 @@
  *		Signals that the contents of a drm plane is reflected along the
  *		<axis> axis, in the same way as mirroring.
  *
- *	reflect-x::
- *
- *			|o |    | o|
- *			|  | -> |  |
- *			| v|    |v |
- *
- *	reflect-y::
- *
- *			|o |    | ^|
- *			|  | -> |  |
- *			| v|    |o |
- *
  * zpos:
  *	Z position is set up with drm_plane_create_zpos_immutable_property() and
  *	drm_plane_create_zpos_property(). It controls the visibility of overlapping
@@ -266,8 +257,41 @@ EXPORT_SYMBOL(drm_plane_create_alpha_property);
  *
  * Rotation is the specified amount in degrees in counter clockwise direction,
  * the X and Y axis are within the source rectangle, i.e.  the X/Y axis before
- * rotation. After reflection, the rotation is applied to the image sampled from
- * the source rectangle, before scaling it to fit the destination rectangle.
+ * rotation.
+ *
+ * Here are some examples of rotation and reflections:
+ *
+ * |o  +|  REFLECT_X  |+  o|
+ * |    |  ========>  |    |
+ * |    |             |    |
+ *
+ * |o   |  REFLECT_Y  |+   |
+ * |    |  ========>  |    |
+ * |+   |             |o   |
+ *
+ * |o  +|  ROTATE_90  |+   |
+ * |    |  ========>  |    |
+ * |    |             |o   |
+ *
+ * |o   |  ROTATE_180 |   +|
+ * |    |  ========>  |    |
+ * |+   |             |   o|
+ *
+ * |o   |  ROTATE_270 |+  o|
+ * |    |  ========>  |    |
+ * |+   |             |    |
+ *
+ * Rotation and reflection can be combined to handle more situations. In this condition, the
+ * reflection is applied first and the rotation in second.
+ *
+ * For example the expected result for DRM_MODE_ROTATE_90 | DRM_MODE_REFLECT_X is:
+ *
+ * |o  +|  REFLECT_X  |+  o|  ROTATE_90  |o   |
+ * |    |  ========>  |    |  ========>  |    |
+ * |    |             |    |             |+   |
+ *
+ * It is not possible to pass multiple rotation at the same time. (i.e ROTATE_90 | ROTATE_180 is
+ * not the same as ROTATE_270 and is not accepted).
  */
 int drm_plane_create_rotation_property(struct drm_plane *plane,
 				       unsigned int rotation,

-- 
2.43.0

[PATCH 3/3] drm/fourcc: Add documentation around drm_format_info

[Louis Chauvet]

Let's provide more details about the drm_format_info structure because
its content may not be straightforward for someone not used to video
formats and drm internals.

Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
---
 include/drm/drm_fourcc.h | 45 ++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 38 insertions(+), 7 deletions(-)

diff --git a/include/drm/drm_fourcc.h b/include/drm/drm_fourcc.h
index ccf91daa4307..66cc30e28f79 100644
--- a/include/drm/drm_fourcc.h
+++ b/include/drm/drm_fourcc.h
@@ -58,6 +58,44 @@ struct drm_mode_fb_cmd2;
 
 /**
  * struct drm_format_info - information about a DRM format
+ *
+ * A drm_format_info describes how planes and pixels are stored in memory.
+ *
+ * Some format like YUV can have multiple planes, counted in @num_planes. It
+ * means that a full pixel can be stored in multiple non-continuous buffers.
+ * For example, NV12 is a YUV format using two planes: one for the Y values and
+ * one for the UV values.
+ *
+ * On each plane, the "pixel" unit can be different in case of subsampling. For
+ * example with the NV12 format, a pixel in the UV plane is used for four pixels
+ * in the Y plane.
+ * The fields @hsub and @vsub are the relation between the size of the main
+ * plane and the size of the subsampled planes in pixels:
+ *	plane[0] width = hsub * plane[1] width
+ *	plane[0] height = vsub * plane[1] height
+ *
+ * In some formats, pixels are not independent in memory. It can be a packed
+ * representation to store more pixels per byte (for example P030 uses 4 bytes
+ * for three 10 bit pixels). It can also be used to represent tiled formats,
+ * where a continuous buffer in memory can represent a rectangle of pixels (for
+ * example, in DRM_FORMAT_Y0L0, a buffer of 8 bytes represents a 2x2 pixel
+ * region of the picture).
+ *	The field @char_per_block is the size of a block on a specific plane, in
+ *	bytes.
+ *	The fields @block_w and @block_h are the size of a block in pixels.
+ *
+ * The older format representation (which only uses @cpp, kept for historical
+ * reasons because there are a lot of places in drivers where it's used) is
+ * assuming that a block is always 1x1 pixel.
+ *
+ * To keep the compatibility with older format representations and treat block
+ * and non-block formats in the same way one should use:
+ *	- @char_per_block to access the size of a block on a specific plane, in
+ *	bytes.
+ *	- drm_format_info_block_width() to access the width of a block of a
+ *	specific plane, in pixels.
+ *	- drm_format_info_block_height() to access the height of a block of a
+ *	specific plane, in pixels.
  */
 struct drm_format_info {
 	/** @format: 4CC format identifier (DRM_FORMAT_*) */
@@ -97,13 +135,6 @@ struct drm_format_info {
 		 * formats for which the memory needed for a single pixel is not
 		 * byte aligned.
 		 *
-		 * @cpp has been kept for historical reasons because there are
-		 * a lot of places in drivers where it's used. In drm core for
-		 * generic code paths the preferred way is to use
-		 * @char_per_block, drm_format_info_block_width() and
-		 * drm_format_info_block_height() which allows handling both
-		 * block and non-block formats in the same way.
-		 *
 		 * For formats that are intended to be used only with non-linear
 		 * modifiers both @cpp and @char_per_block must be 0 in the
 		 * generic format table. Drivers could supply accurate

-- 
2.43.0

Re: [PATCH 0/3] drm: Multiple documentation update

[Jani Nikula]

On Tue, 09 Apr 2024, Louis Chauvet <louis.chauvet@bootlin.com> wrote:
> PATCH 1 and PATCH 2 focus on the rotation property. The rotation property 
> can be challenging to understand, especially when it is combined with 
> reflections. These patches aim to provide clearer explanations and 
> examples to aid in comprehension.
>
> Patch 3 relates to the fourcc property. It includes additional details 
> about block and char_per_block to provide a more comprehensive 
> understanding of this feature.
>
> Regarding PATCH 1, I would appreciate some feedback on the expected 
> behavior. During a recent VKMS refactor, I used drm_rect_rotate as a 
> reference for the rotation. However, during my testing phase, I noticed 
> that the original VKMS implementation interpreted the rotation 
> differently. Therefore, I kindly request that someone validate or 
> invalidate my interpretation before proceeding with the merge.

Did you run 'make htmldocs' and check the results after your changes?
I'm almost certain this botches up the layout.

BR,
Jani.

>
> Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
> ---
> Louis Chauvet (3):
>       drm: drm_blend.c: Add precision in drm_rotation_simplify kernel doc
>       drm: drm_blend.c: Improve drm_plane_create_rotation_property kernel doc
>       drm/fourcc: Add documentation around drm_format_info
>
>  drivers/gpu/drm/drm_blend.c | 57 ++++++++++++++++++++++++++++++++++-----------
>  include/drm/drm_fourcc.h    | 45 +++++++++++++++++++++++++++++------
>  2 files changed, 81 insertions(+), 21 deletions(-)
> ---
> base-commit: e495e523b888a6155f82c767d34c8d712a41ee54
> change-id: 20240327-google-drm-doc-cd275291792f
>
> Best regards,

-- 
Jani Nikula, Intel

Re: [PATCH 0/3] drm: Multiple documentation update

[Louis Chauvet]

Le 09/04/24 - 13:18, Jani Nikula a écrit :
> On Tue, 09 Apr 2024, Louis Chauvet <louis.chauvet@bootlin.com> wrote:
> > PATCH 1 and PATCH 2 focus on the rotation property. The rotation property 
> > can be challenging to understand, especially when it is combined with 
> > reflections. These patches aim to provide clearer explanations and 
> > examples to aid in comprehension.
> >
> > Patch 3 relates to the fourcc property. It includes additional details 
> > about block and char_per_block to provide a more comprehensive 
> > understanding of this feature.
> >
> > Regarding PATCH 1, I would appreciate some feedback on the expected 
> > behavior. During a recent VKMS refactor, I used drm_rect_rotate as a 
> > reference for the rotation. However, during my testing phase, I noticed 
> > that the original VKMS implementation interpreted the rotation 
> > differently. Therefore, I kindly request that someone validate or 
> > invalidate my interpretation before proceeding with the merge.
> 
> Did you run 'make htmldocs' and check the results after your changes?
> I'm almost certain this botches up the layout.

I did not think about htmldocs. I have a new version ready where the 
layout is fixed. Sorry about that.

I'll wait for more reviews before submitting it.

Thanks,
Louis Chauvet
 
> BR,
> Jani.
> 
> >
> > Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
> > ---
> > Louis Chauvet (3):
> >       drm: drm_blend.c: Add precision in drm_rotation_simplify kernel doc
> >       drm: drm_blend.c: Improve drm_plane_create_rotation_property kernel doc
> >       drm/fourcc: Add documentation around drm_format_info
> >
> >  drivers/gpu/drm/drm_blend.c | 57 ++++++++++++++++++++++++++++++++++-----------
> >  include/drm/drm_fourcc.h    | 45 +++++++++++++++++++++++++++++------
> >  2 files changed, 81 insertions(+), 21 deletions(-)
> > ---
> > base-commit: e495e523b888a6155f82c767d34c8d712a41ee54
> > change-id: 20240327-google-drm-doc-cd275291792f
> >
> > Best regards,
> 
> -- 
> Jani Nikula, Intel

-- 
Louis Chauvet, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 2/3] drm: drm_blend.c: Improve drm_plane_create_rotation_property kernel doc

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 3742 bytes --]

On Tue, 09 Apr 2024 12:04:06 +0200
Louis Chauvet <louis.chauvet@bootlin.com> wrote:

> The expected behavior of the rotation property was not very clear. Add
> more examples to explain what is the expected result.
> 
> Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
> ---
>  drivers/gpu/drm/drm_blend.c | 52 +++++++++++++++++++++++++++++++++------------
>  1 file changed, 38 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
> index 8d4b317eb9d7..6fbb8730d8b0 100644
> --- a/drivers/gpu/drm/drm_blend.c
> +++ b/drivers/gpu/drm/drm_blend.c
> @@ -104,6 +104,9 @@
>   *	Without this property the rectangle is only scaled, but not rotated or
>   *	reflected.
>   *
> + *	See drm_plane_create_rotation_property() for details about the expected rotation and
> + *	reflection behavior.

I think internal function docs should be referring to UAPI docs, and
not vice versa. Internal functions can change, but UAPI cannot.

> + *
>   *	Possbile values:
>   *
>   *	"rotate-<degrees>":
> @@ -114,18 +117,6 @@
>   *		Signals that the contents of a drm plane is reflected along the
>   *		<axis> axis, in the same way as mirroring.
>   *
> - *	reflect-x::
> - *
> - *			|o |    | o|
> - *			|  | -> |  |
> - *			| v|    |v |
> - *
> - *	reflect-y::
> - *
> - *			|o |    | ^|
> - *			|  | -> |  |
> - *			| v|    |o |
> - *
>   * zpos:
>   *	Z position is set up with drm_plane_create_zpos_immutable_property() and
>   *	drm_plane_create_zpos_property(). It controls the visibility of overlapping
> @@ -266,8 +257,41 @@ EXPORT_SYMBOL(drm_plane_create_alpha_property);
>   *
>   * Rotation is the specified amount in degrees in counter clockwise direction,
>   * the X and Y axis are within the source rectangle, i.e.  the X/Y axis before
> - * rotation. After reflection, the rotation is applied to the image sampled from
> - * the source rectangle, before scaling it to fit the destination rectangle.
> + * rotation.
> + *
> + * Here are some examples of rotation and reflections:
> + *
> + * |o  +|  REFLECT_X  |+  o|
> + * |    |  ========>  |    |
> + * |    |             |    |
> + *
> + * |o   |  REFLECT_Y  |+   |
> + * |    |  ========>  |    |
> + * |+   |             |o   |
> + *
> + * |o  +|  ROTATE_90  |+   |
> + * |    |  ========>  |    |
> + * |    |             |o   |
> + *
> + * |o   |  ROTATE_180 |   +|
> + * |    |  ========>  |    |
> + * |+   |             |   o|
> + *
> + * |o   |  ROTATE_270 |+  o|
> + * |    |  ========>  |    |
> + * |+   |             |    |
> + *
> + * Rotation and reflection can be combined to handle more situations. In this condition, the
> + * reflection is applied first and the rotation in second.

When going in which direction? Is the first image the FB source
rectangle contents, and the second image what the plane looks like in
CRTC frame of reference?

> + *
> + * For example the expected result for DRM_MODE_ROTATE_90 | DRM_MODE_REFLECT_X is:
> + *
> + * |o  +|  REFLECT_X  |+  o|  ROTATE_90  |o   |
> + * |    |  ========>  |    |  ========>  |    |
> + * |    |             |    |             |+   |
> + *
> + * It is not possible to pass multiple rotation at the same time. (i.e ROTATE_90 | ROTATE_180 is
> + * not the same as ROTATE_270 and is not accepted).
>   */
>  int drm_plane_create_rotation_property(struct drm_plane *plane,
>  				       unsigned int rotation,
> 

These are definitely improvements. I think they should just be in the
UAPI section rather than implementation details.

Disclaimer again to everyone else: I cannot tell if this is the correct
documentation or its inverse.


Thanks,
pq

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 3/3] drm/fourcc: Add documentation around drm_format_info

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 5148 bytes --]

On Tue, 09 Apr 2024 12:04:07 +0200
Louis Chauvet <louis.chauvet@bootlin.com> wrote:

> Let's provide more details about the drm_format_info structure because
> its content may not be straightforward for someone not used to video
> formats and drm internals.
> 
> Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
> ---
>  include/drm/drm_fourcc.h | 45 ++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 38 insertions(+), 7 deletions(-)
> 
> diff --git a/include/drm/drm_fourcc.h b/include/drm/drm_fourcc.h
> index ccf91daa4307..66cc30e28f79 100644
> --- a/include/drm/drm_fourcc.h
> +++ b/include/drm/drm_fourcc.h
> @@ -58,6 +58,44 @@ struct drm_mode_fb_cmd2;
>  
>  /**
>   * struct drm_format_info - information about a DRM format
> + *
> + * A drm_format_info describes how planes and pixels are stored in memory.
> + *
> + * Some format like YUV can have multiple planes, counted in @num_planes. It
> + * means that a full pixel can be stored in multiple non-continuous buffers.
> + * For example, NV12 is a YUV format using two planes: one for the Y values and
> + * one for the UV values.
> + *
> + * On each plane, the "pixel" unit can be different in case of subsampling. For
> + * example with the NV12 format, a pixel in the UV plane is used for four pixels
> + * in the Y plane.
> + * The fields @hsub and @vsub are the relation between the size of the main
> + * plane and the size of the subsampled planes in pixels:
> + *	plane[0] width = hsub * plane[1] width
> + *	plane[0] height = vsub * plane[1] height

This makes it sound like plane[1] would be the one determining the
image size. It is plane[0] that determines the image size (I don't know
of a format that would have it otherwise), and vsub and hsub are used
as divisors. It's in their name, too: horizontal/vertical sub-sampling.

This is important for images with odd dimensions. If plane[1]
determined the image size, it would be impossible to have odd sized
NV12 images, for instance.

Odd dimensions also imply something about rounding the size of the
sub-sampled planes. I guess the rounding is up, not down?

> + *
> + * In some formats, pixels are not independent in memory. It can be a packed

"Independent in memory" sounds to me like it describes sub-sampling:
some pixel components are shared between multiple pixels. Here you seem
to refer to just packing: one pixel's data may take a fractional number
of bytes.

> + * representation to store more pixels per byte (for example P030 uses 4 bytes
> + * for three 10 bit pixels). It can also be used to represent tiled formats,

s/tiled/block/

Tiling is given by format modifiers rather than formats.

> + * where a continuous buffer in memory can represent a rectangle of pixels (for
> + * example, in DRM_FORMAT_Y0L0, a buffer of 8 bytes represents a 2x2 pixel
> + * region of the picture).
> + *	The field @char_per_block is the size of a block on a specific plane, in
> + *	bytes.
> + *	The fields @block_w and @block_h are the size of a block in pixels.
> + *
> + * The older format representation (which only uses @cpp, kept for historical

Move the paren to: representation which only uses @cpp (kept

so that the sentence is still understandable if one skips the
parenthesised part.

> + * reasons because there are a lot of places in drivers where it's used) is
> + * assuming that a block is always 1x1 pixel.
> + *
> + * To keep the compatibility with older format representations and treat block
> + * and non-block formats in the same way one should use:
> + *	- @char_per_block to access the size of a block on a specific plane, in
> + *	bytes.
> + *	- drm_format_info_block_width() to access the width of a block of a
> + *	specific plane, in pixels.
> + *	- drm_format_info_block_height() to access the height of a block of a
> + *	specific plane, in pixels.
>   */
>  struct drm_format_info {
>  	/** @format: 4CC format identifier (DRM_FORMAT_*) */
> @@ -97,13 +135,6 @@ struct drm_format_info {
>  		 * formats for which the memory needed for a single pixel is not
>  		 * byte aligned.
>  		 *
> -		 * @cpp has been kept for historical reasons because there are
> -		 * a lot of places in drivers where it's used. In drm core for
> -		 * generic code paths the preferred way is to use
> -		 * @char_per_block, drm_format_info_block_width() and
> -		 * drm_format_info_block_height() which allows handling both
> -		 * block and non-block formats in the same way.
> -		 *
>  		 * For formats that are intended to be used only with non-linear
>  		 * modifiers both @cpp and @char_per_block must be 0 in the
>  		 * generic format table. Drivers could supply accurate
> 

Other than that, sounds fine to me.

Perhaps one thing to clarify is that chroma sub-sampling and blocks are
two different things. Chroma sub-sampling is about the resolution of
the chroma (image). Blocks are about packing multiple pixels' components
into a contiguous addressable block of memory. Blocks could appear
inside a separate sub-sampled UV plane, for example.


Thanks,
pq

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 3/3] drm/fourcc: Add documentation around drm_format_info

[Bagas Sanjaya]

[-- Attachment #1: Type: text/plain, Size: 5775 bytes --]

On Tue, Apr 09, 2024 at 12:04:07PM +0200, Louis Chauvet wrote:
>  /**
>   * struct drm_format_info - information about a DRM format
> + *
> + * A drm_format_info describes how planes and pixels are stored in memory.
> + *
> + * Some format like YUV can have multiple planes, counted in @num_planes. It
> + * means that a full pixel can be stored in multiple non-continuous buffers.
> + * For example, NV12 is a YUV format using two planes: one for the Y values and
> + * one for the UV values.
> + *
> + * On each plane, the "pixel" unit can be different in case of subsampling. For
> + * example with the NV12 format, a pixel in the UV plane is used for four pixels
> + * in the Y plane.
> + * The fields @hsub and @vsub are the relation between the size of the main
> + * plane and the size of the subsampled planes in pixels:
> + *	plane[0] width = hsub * plane[1] width
> + *	plane[0] height = vsub * plane[1] height
> + *
> + * In some formats, pixels are not independent in memory. It can be a packed
> + * representation to store more pixels per byte (for example P030 uses 4 bytes
> + * for three 10 bit pixels). It can also be used to represent tiled formats,
> + * where a continuous buffer in memory can represent a rectangle of pixels (for
> + * example, in DRM_FORMAT_Y0L0, a buffer of 8 bytes represents a 2x2 pixel
> + * region of the picture).
> + *	The field @char_per_block is the size of a block on a specific plane, in
> + *	bytes.
> + *	The fields @block_w and @block_h are the size of a block in pixels.
> + *
> + * The older format representation (which only uses @cpp, kept for historical
> + * reasons because there are a lot of places in drivers where it's used) is
> + * assuming that a block is always 1x1 pixel.
> + *
> + * To keep the compatibility with older format representations and treat block
> + * and non-block formats in the same way one should use:
> + *	- @char_per_block to access the size of a block on a specific plane, in
> + *	bytes.
> + *	- drm_format_info_block_width() to access the width of a block of a
> + *	specific plane, in pixels.
> + *	- drm_format_info_block_height() to access the height of a block of a
> + *	specific plane, in pixels.
>   */
>  struct drm_format_info {
>  	/** @format: 4CC format identifier (DRM_FORMAT_*) */
> @@ -97,13 +135,6 @@ struct drm_format_info {
>  		 * formats for which the memory needed for a single pixel is not
>  		 * byte aligned.
>  		 *
> -		 * @cpp has been kept for historical reasons because there are
> -		 * a lot of places in drivers where it's used. In drm core for
> -		 * generic code paths the preferred way is to use
> -		 * @char_per_block, drm_format_info_block_width() and
> -		 * drm_format_info_block_height() which allows handling both
> -		 * block and non-block formats in the same way.
> -		 *
>  		 * For formats that are intended to be used only with non-linear
>  		 * modifiers both @cpp and @char_per_block must be 0 in the
>  		 * generic format table. Drivers could supply accurate
> 

Sphinx reports htmldocs warnings:

Documentation/gpu/drm-kms:357: ./include/drm/drm_fourcc.h:74: ERROR: Unexpected indentation.
Documentation/gpu/drm-kms:357: ./include/drm/drm_fourcc.h:83: ERROR: Unexpected indentation.
Documentation/gpu/drm-kms:357: ./include/drm/drm_fourcc.h:93: ERROR: Unexpected indentation.
Documentation/gpu/drm-kms:357: ./include/drm/drm_fourcc.h:94: WARNING: Bullet list ends without a blank line; unexpected unindent.

I have to fix up the lists:

---- >8 ----
diff --git a/include/drm/drm_fourcc.h b/include/drm/drm_fourcc.h
index 66cc30e28f794a..10ee74fa46d21e 100644
--- a/include/drm/drm_fourcc.h
+++ b/include/drm/drm_fourcc.h
@@ -71,8 +71,9 @@ struct drm_mode_fb_cmd2;
  * in the Y plane.
  * The fields @hsub and @vsub are the relation between the size of the main
  * plane and the size of the subsampled planes in pixels:
- *	plane[0] width = hsub * plane[1] width
- *	plane[0] height = vsub * plane[1] height
+ *
+ *	- plane[0] width = hsub * plane[1] width
+ *	- plane[0] height = vsub * plane[1] height
  *
  * In some formats, pixels are not independent in memory. It can be a packed
  * representation to store more pixels per byte (for example P030 uses 4 bytes
@@ -80,9 +81,10 @@ struct drm_mode_fb_cmd2;
  * where a continuous buffer in memory can represent a rectangle of pixels (for
  * example, in DRM_FORMAT_Y0L0, a buffer of 8 bytes represents a 2x2 pixel
  * region of the picture).
- *	The field @char_per_block is the size of a block on a specific plane, in
- *	bytes.
- *	The fields @block_w and @block_h are the size of a block in pixels.
+ *
+ *	- The field @char_per_block is the size of a block on a specific plane,
+ *	  in bytes.
+ *	- The fields @block_w and @block_h are the size of a block in pixels.
  *
  * The older format representation (which only uses @cpp, kept for historical
  * reasons because there are a lot of places in drivers where it's used) is
@@ -90,12 +92,13 @@ struct drm_mode_fb_cmd2;
  *
  * To keep the compatibility with older format representations and treat block
  * and non-block formats in the same way one should use:
+ *
  *	- @char_per_block to access the size of a block on a specific plane, in
- *	bytes.
+ *	  bytes.
  *	- drm_format_info_block_width() to access the width of a block of a
- *	specific plane, in pixels.
+ *	  specific plane, in pixels.
  *	- drm_format_info_block_height() to access the height of a block of a
- *	specific plane, in pixels.
+ *	  specific plane, in pixels.
  */
 struct drm_format_info {
 	/** @format: 4CC format identifier (DRM_FORMAT_*) */

Thanks.

-- 
An old man doll... just what I always wanted! - Clara

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 2/3] drm: drm_blend.c: Improve drm_plane_create_rotation_property kernel doc

[Bagas Sanjaya]

[-- Attachment #1: Type: text/plain, Size: 5071 bytes --]

On Tue, Apr 09, 2024 at 12:04:06PM +0200, Louis Chauvet wrote:
> @@ -266,8 +257,41 @@ EXPORT_SYMBOL(drm_plane_create_alpha_property);
>   *
>   * Rotation is the specified amount in degrees in counter clockwise direction,
>   * the X and Y axis are within the source rectangle, i.e.  the X/Y axis before
> - * rotation. After reflection, the rotation is applied to the image sampled from
> - * the source rectangle, before scaling it to fit the destination rectangle.
> + * rotation.
> + *
> + * Here are some examples of rotation and reflections:
> + *
> + * |o  +|  REFLECT_X  |+  o|
> + * |    |  ========>  |    |
> + * |    |             |    |
> + *
> + * |o   |  REFLECT_Y  |+   |
> + * |    |  ========>  |    |
> + * |+   |             |o   |
> + *
> + * |o  +|  ROTATE_90  |+   |
> + * |    |  ========>  |    |
> + * |    |             |o   |
> + *
> + * |o   |  ROTATE_180 |   +|
> + * |    |  ========>  |    |
> + * |+   |             |   o|
> + *
> + * |o   |  ROTATE_270 |+  o|
> + * |    |  ========>  |    |
> + * |+   |             |    |
> + *
> + * Rotation and reflection can be combined to handle more situations. In this condition, the
> + * reflection is applied first and the rotation in second.
> + *
> + * For example the expected result for DRM_MODE_ROTATE_90 | DRM_MODE_REFLECT_X is:
> + *
> + * |o  +|  REFLECT_X  |+  o|  ROTATE_90  |o   |
> + * |    |  ========>  |    |  ========>  |    |
> + * |    |             |    |             |+   |
> + *
> + * It is not possible to pass multiple rotation at the same time. (i.e ROTATE_90 | ROTATE_180 is
> + * not the same as ROTATE_270 and is not accepted).

Sphinx reports htmldocs warnings on these transformation diagrams:

Documentation/gpu/drm-kms:389: ./drivers/gpu/drm/drm_blend.c:265: ERROR: Undefined substitution referenced: "o +".
Documentation/gpu/drm-kms:389: ./drivers/gpu/drm/drm_blend.c:265: ERROR: Undefined substitution referenced: "+ o".
Documentation/gpu/drm-kms:389: ./drivers/gpu/drm/drm_blend.c:273: ERROR: Undefined substitution referenced: "o +".
Documentation/gpu/drm-kms:389: ./drivers/gpu/drm/drm_blend.c:277: ERROR: Undefined substitution referenced: "o | ROTATE_180 | +".
Documentation/gpu/drm-kms:389: ./drivers/gpu/drm/drm_blend.c:277: ERROR: Undefined substitution referenced: "+ | | o".
Documentation/gpu/drm-kms:389: ./drivers/gpu/drm/drm_blend.c:281: ERROR: Undefined substitution referenced: "o | ROTATE_270 |+ o".
Documentation/gpu/drm-kms:389: ./drivers/gpu/drm/drm_blend.c:290: ERROR: Undefined substitution referenced: "o +".
Documentation/gpu/drm-kms:389: ./drivers/gpu/drm/drm_blend.c:290: ERROR: Undefined substitution referenced: "+ o".

I have to wrap them in literal blocks:

---- >8 ----
diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
index 6fbb8730d8b022..f2cbf8d8efbbbc 100644
--- a/drivers/gpu/drm/drm_blend.c
+++ b/drivers/gpu/drm/drm_blend.c
@@ -259,36 +259,36 @@ EXPORT_SYMBOL(drm_plane_create_alpha_property);
  * the X and Y axis are within the source rectangle, i.e.  the X/Y axis before
  * rotation.
  *
- * Here are some examples of rotation and reflections:
+ * Here are some examples of rotation and reflections::
  *
- * |o  +|  REFLECT_X  |+  o|
- * |    |  ========>  |    |
- * |    |             |    |
+ *	|o  +|  REFLECT_X  |+  o|
+ *	|    |  ========>  |    |
+ *	|    |             |    |
  *
- * |o   |  REFLECT_Y  |+   |
- * |    |  ========>  |    |
- * |+   |             |o   |
+ *	|o   |  REFLECT_Y  |+   |
+ *	|    |  ========>  |    |
+ *	|+   |             |o   |
  *
- * |o  +|  ROTATE_90  |+   |
- * |    |  ========>  |    |
- * |    |             |o   |
+ *	|o  +|  ROTATE_90  |+   |
+ *	|    |  ========>  |    |
+ *	|    |             |o   |
  *
- * |o   |  ROTATE_180 |   +|
- * |    |  ========>  |    |
- * |+   |             |   o|
+ *	|o   |  ROTATE_180 |   +|
+ *	|    |  ========>  |    |
+ *	|+   |             |   o|
  *
- * |o   |  ROTATE_270 |+  o|
- * |    |  ========>  |    |
- * |+   |             |    |
+ *	|o   |  ROTATE_270 |+  o|
+ *	|    |  ========>  |    |
+ *	|+   |             |    |
  *
  * Rotation and reflection can be combined to handle more situations. In this condition, the
  * reflection is applied first and the rotation in second.
  *
- * For example the expected result for DRM_MODE_ROTATE_90 | DRM_MODE_REFLECT_X is:
+ * For example the expected result for DRM_MODE_ROTATE_90 | DRM_MODE_REFLECT_X is::
  *
- * |o  +|  REFLECT_X  |+  o|  ROTATE_90  |o   |
- * |    |  ========>  |    |  ========>  |    |
- * |    |             |    |             |+   |
+ *	|o  +|  REFLECT_X  |+  o|  ROTATE_90  |o   |
+ *	|    |  ========>  |    |  ========>  |    |
+ *	|    |             |    |             |+   |
  *
  * It is not possible to pass multiple rotation at the same time. (i.e ROTATE_90 | ROTATE_180 is
  * not the same as ROTATE_270 and is not accepted).

Thanks.

-- 
An old man doll... just what I always wanted! - Clara

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 1/3] drm: drm_blend.c: Add precision in drm_rotation_simplify kernel doc

[Bagas Sanjaya]

[-- Attachment #1: Type: text/plain, Size: 1077 bytes --]

On Tue, Apr 09, 2024 at 12:04:05PM +0200, Louis Chauvet wrote:
> diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
> index 6e74de833466..8d4b317eb9d7 100644
> --- a/drivers/gpu/drm/drm_blend.c
> +++ b/drivers/gpu/drm/drm_blend.c
> @@ -321,6 +321,11 @@ EXPORT_SYMBOL(drm_plane_create_rotation_property);
>   * transforms the hardware supports, this function may not
>   * be able to produce a supported transform, so the caller should
>   * check the result afterwards.
> + *
> + * If the rotation is not fully supported, this function will add a rotation of 180
> + * (ROTATE_90 would become ROTATE_270) and add a reflection on X and Y.
> + * The resulting transformation is the same (REFLECT_X | REFLECT_Y | ROTATE_180
> + * is a no-op), but some unsupported flags are removed.
>   */
>  unsigned int drm_rotation_simplify(unsigned int rotation,
>  				   unsigned int supported_rotations)
> 

The wording LGTM, thanks!

Reviewed-by: Bagas Sanjaya <bagasdotme@gmail.com>

-- 
An old man doll... just what I always wanted! - Clara

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 2/3] drm: drm_blend.c: Improve drm_plane_create_rotation_property kernel doc

[Louis Chauvet]

Le 15/04/24 - 14:36, Pekka Paalanen a écrit :
> On Tue, 09 Apr 2024 12:04:06 +0200
> Louis Chauvet <louis.chauvet@bootlin.com> wrote:
> 
> > The expected behavior of the rotation property was not very clear. Add
> > more examples to explain what is the expected result.
> > 
> > Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
> > ---
> >  drivers/gpu/drm/drm_blend.c | 52 +++++++++++++++++++++++++++++++++------------
> >  1 file changed, 38 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
> > index 8d4b317eb9d7..6fbb8730d8b0 100644
> > --- a/drivers/gpu/drm/drm_blend.c
> > +++ b/drivers/gpu/drm/drm_blend.c
> > @@ -104,6 +104,9 @@
> >   *	Without this property the rectangle is only scaled, but not rotated or
> >   *	reflected.
> >   *
> > + *	See drm_plane_create_rotation_property() for details about the expected rotation and
> > + *	reflection behavior.
> 
> I think internal function docs should be referring to UAPI docs, and
> not vice versa. Internal functions can change, but UAPI cannot.
> 
> > + *
> >   *	Possbile values:
> >   *
> >   *	"rotate-<degrees>":
> > @@ -114,18 +117,6 @@
> >   *		Signals that the contents of a drm plane is reflected along the
> >   *		<axis> axis, in the same way as mirroring.
> >   *
> > - *	reflect-x::
> > - *
> > - *			|o |    | o|
> > - *			|  | -> |  |
> > - *			| v|    |v |
> > - *
> > - *	reflect-y::
> > - *
> > - *			|o |    | ^|
> > - *			|  | -> |  |
> > - *			| v|    |o |
> > - *
> >   * zpos:
> >   *	Z position is set up with drm_plane_create_zpos_immutable_property() and
> >   *	drm_plane_create_zpos_property(). It controls the visibility of overlapping
> > @@ -266,8 +257,41 @@ EXPORT_SYMBOL(drm_plane_create_alpha_property);
> >   *
> >   * Rotation is the specified amount in degrees in counter clockwise direction,
> >   * the X and Y axis are within the source rectangle, i.e.  the X/Y axis before
> > - * rotation. After reflection, the rotation is applied to the image sampled from
> > - * the source rectangle, before scaling it to fit the destination rectangle.
> > + * rotation.
> > + *
> > + * Here are some examples of rotation and reflections:
> > + *
> > + * |o  +|  REFLECT_X  |+  o|
> > + * |    |  ========>  |    |
> > + * |    |             |    |
> > + *
> > + * |o   |  REFLECT_Y  |+   |
> > + * |    |  ========>  |    |
> > + * |+   |             |o   |
> > + *
> > + * |o  +|  ROTATE_90  |+   |
> > + * |    |  ========>  |    |
> > + * |    |             |o   |
> > + *
> > + * |o   |  ROTATE_180 |   +|
> > + * |    |  ========>  |    |
> > + * |+   |             |   o|
> > + *
> > + * |o   |  ROTATE_270 |+  o|
> > + * |    |  ========>  |    |
> > + * |+   |             |    |
> > + *
> > + * Rotation and reflection can be combined to handle more situations. In this condition, the
> > + * reflection is applied first and the rotation in second.
> 
> When going in which direction? Is the first image the FB source
> rectangle contents, and the second image what the plane looks like in
> CRTC frame of reference?

The first is the FB source, the second is the expected result on the CRTC 
output.

I will add a sentence before the schemas:

 * Here are some examples of rotation and reflections, on the left it is 
 * the content of the source frame buffer, on the right is the expected 
 * result on the CRTC output.

> 
> > + *
> > + * For example the expected result for DRM_MODE_ROTATE_90 | DRM_MODE_REFLECT_X is:
> > + *
> > + * |o  +|  REFLECT_X  |+  o|  ROTATE_90  |o   |
> > + * |    |  ========>  |    |  ========>  |    |
> > + * |    |             |    |             |+   |
> > + *
> > + * It is not possible to pass multiple rotation at the same time. (i.e ROTATE_90 | ROTATE_180 is
> > + * not the same as ROTATE_270 and is not accepted).
> >   */
> >  int drm_plane_create_rotation_property(struct drm_plane *plane,
> >  				       unsigned int rotation,
> > 
> 
> These are definitely improvements. I think they should just be in the
> UAPI section rather than implementation details.

So, somewhere in [1]? It feel strange because this is in the `GPU Driver 
Developer’s Guide` section, not a `UAPI interfaces`.

[1]: https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html

Thanks,
Louis Chauvet

> Disclaimer again to everyone else: I cannot tell if this is the correct
> documentation or its inverse.
> 
> 
> Thanks,
> pq



-- 
Louis Chauvet, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 3/3] drm/fourcc: Add documentation around drm_format_info

[Louis Chauvet]

Le 15/04/24 - 15:00, Pekka Paalanen a écrit :
> On Tue, 09 Apr 2024 12:04:07 +0200
> Louis Chauvet <louis.chauvet@bootlin.com> wrote:
> 
> > Let's provide more details about the drm_format_info structure because
> > its content may not be straightforward for someone not used to video
> > formats and drm internals.
> > 
> > Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
> > ---
> >  include/drm/drm_fourcc.h | 45 ++++++++++++++++++++++++++++++++++++++-------
> >  1 file changed, 38 insertions(+), 7 deletions(-)
> > 
> > diff --git a/include/drm/drm_fourcc.h b/include/drm/drm_fourcc.h
> > index ccf91daa4307..66cc30e28f79 100644
> > --- a/include/drm/drm_fourcc.h
> > +++ b/include/drm/drm_fourcc.h
> > @@ -58,6 +58,44 @@ struct drm_mode_fb_cmd2;
> >  
> >  /**
> >   * struct drm_format_info - information about a DRM format
> > + *
> > + * A drm_format_info describes how planes and pixels are stored in memory.
> > + *
> > + * Some format like YUV can have multiple planes, counted in @num_planes. It
> > + * means that a full pixel can be stored in multiple non-continuous buffers.
> > + * For example, NV12 is a YUV format using two planes: one for the Y values and
> > + * one for the UV values.
> > + *
> > + * On each plane, the "pixel" unit can be different in case of subsampling. For
> > + * example with the NV12 format, a pixel in the UV plane is used for four pixels
> > + * in the Y plane.
> > + * The fields @hsub and @vsub are the relation between the size of the main
> > + * plane and the size of the subsampled planes in pixels:
> > + *	plane[0] width = hsub * plane[1] width
> > + *	plane[0] height = vsub * plane[1] height
> 
> This makes it sound like plane[1] would be the one determining the
> image size. It is plane[0] that determines the image size (I don't know
> of a format that would have it otherwise), and vsub and hsub are used
> as divisors. It's in their name, too: horizontal/vertical sub-sampling.
> 
> This is important for images with odd dimensions. If plane[1]
> determined the image size, it would be impossible to have odd sized
> NV12 images, for instance.
> 
> Odd dimensions also imply something about rounding the size of the
> sub-sampled planes. I guess the rounding is up, not down?

I will change the equation to:

plane[1] = plane[0] / hsub (round up)

Can a DRM maintainer confirm the rounding up?
 
> > + *
> > + * In some formats, pixels are not independent in memory. It can be a packed
> 
> "Independent in memory" sounds to me like it describes sub-sampling:
> some pixel components are shared between multiple pixels. Here you seem
> to refer to just packing: one pixel's data may take a fractional number
> of bytes.

 * In some formats, pixels are not individually addressable. It ...

> > + * representation to store more pixels per byte (for example P030 uses 4 bytes
> > + * for three 10 bit pixels). It can also be used to represent tiled formats,
> 
> s/tiled/block/
> 
> Tiling is given by format modifiers rather than formats.

Fixed in the v2.

> > + * where a continuous buffer in memory can represent a rectangle of pixels (for
> > + * example, in DRM_FORMAT_Y0L0, a buffer of 8 bytes represents a 2x2 pixel
> > + * region of the picture).
> > + *	The field @char_per_block is the size of a block on a specific plane, in
> > + *	bytes.
> > + *	The fields @block_w and @block_h are the size of a block in pixels.
> > + *
> > + * The older format representation (which only uses @cpp, kept for historical
> 
> Move the paren to: representation which only uses @cpp (kept
> 
> so that the sentence is still understandable if one skips the
> parenthesised part.

Fixed in v2.

> > + * reasons because there are a lot of places in drivers where it's used) is
> > + * assuming that a block is always 1x1 pixel.
> > + *
> > + * To keep the compatibility with older format representations and treat block
> > + * and non-block formats in the same way one should use:
> > + *	- @char_per_block to access the size of a block on a specific plane, in
> > + *	bytes.
> > + *	- drm_format_info_block_width() to access the width of a block of a
> > + *	specific plane, in pixels.
> > + *	- drm_format_info_block_height() to access the height of a block of a
> > + *	specific plane, in pixels.
> >   */
> >  struct drm_format_info {
> >  	/** @format: 4CC format identifier (DRM_FORMAT_*) */
> > @@ -97,13 +135,6 @@ struct drm_format_info {
> >  		 * formats for which the memory needed for a single pixel is not
> >  		 * byte aligned.
> >  		 *
> > -		 * @cpp has been kept for historical reasons because there are
> > -		 * a lot of places in drivers where it's used. In drm core for
> > -		 * generic code paths the preferred way is to use
> > -		 * @char_per_block, drm_format_info_block_width() and
> > -		 * drm_format_info_block_height() which allows handling both
> > -		 * block and non-block formats in the same way.
> > -		 *
> >  		 * For formats that are intended to be used only with non-linear
> >  		 * modifiers both @cpp and @char_per_block must be 0 in the
> >  		 * generic format table. Drivers could supply accurate
> > 
> 
> Other than that, sounds fine to me.
> 
> Perhaps one thing to clarify is that chroma sub-sampling and blocks are
> two different things. Chroma sub-sampling is about the resolution of
> the chroma (image). Blocks are about packing multiple pixels' components
> into a contiguous addressable block of memory. Blocks could appear
> inside a separate sub-sampled UV plane, for example.

Is this clear? i will add it just before "In some formats, 
pixels...

 * Chroma subsamping (hsub/vsub) must not be confused with pixel blocks. The
 * first describe the relation between the resolution of each color components
 * (for YUV format, the relation between the "y" resolution and the "uv"
 * resolution), the second describe the way to pack multiple pixels into one
 * contiguous block of memory (for example, DRM_FORMAT_Y0L0, one block is 2x2
 * pixels).

Thanks,
Louis Chauvet
 
> Thanks,
> pq


-- 
Louis Chauvet, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 2/3] drm: drm_blend.c: Improve drm_plane_create_rotation_property kernel doc

[Louis Chauvet]

Le 15/04/24 - 21:28, Bagas Sanjaya a écrit :
> On Tue, Apr 09, 2024 at 12:04:06PM +0200, Louis Chauvet wrote:
> > @@ -266,8 +257,41 @@ EXPORT_SYMBOL(drm_plane_create_alpha_property);
> >   *
> >   * Rotation is the specified amount in degrees in counter clockwise direction,
> >   * the X and Y axis are within the source rectangle, i.e.  the X/Y axis before
> > - * rotation. After reflection, the rotation is applied to the image sampled from
> > - * the source rectangle, before scaling it to fit the destination rectangle.
> > + * rotation.
> > + *
> > + * Here are some examples of rotation and reflections:
> > + *
> > + * |o  +|  REFLECT_X  |+  o|
> > + * |    |  ========>  |    |
> > + * |    |             |    |
> > + *
> > + * |o   |  REFLECT_Y  |+   |
> > + * |    |  ========>  |    |
> > + * |+   |             |o   |
> > + *
> > + * |o  +|  ROTATE_90  |+   |
> > + * |    |  ========>  |    |
> > + * |    |             |o   |
> > + *
> > + * |o   |  ROTATE_180 |   +|
> > + * |    |  ========>  |    |
> > + * |+   |             |   o|
> > + *
> > + * |o   |  ROTATE_270 |+  o|
> > + * |    |  ========>  |    |
> > + * |+   |             |    |
> > + *
> > + * Rotation and reflection can be combined to handle more situations. In this condition, the
> > + * reflection is applied first and the rotation in second.
> > + *
> > + * For example the expected result for DRM_MODE_ROTATE_90 | DRM_MODE_REFLECT_X is:
> > + *
> > + * |o  +|  REFLECT_X  |+  o|  ROTATE_90  |o   |
> > + * |    |  ========>  |    |  ========>  |    |
> > + * |    |             |    |             |+   |
> > + *
> > + * It is not possible to pass multiple rotation at the same time. (i.e ROTATE_90 | ROTATE_180 is
> > + * not the same as ROTATE_270 and is not accepted).
> 
> Sphinx reports htmldocs warnings on these transformation diagrams:
> 
> Documentation/gpu/drm-kms:389: ./drivers/gpu/drm/drm_blend.c:265: ERROR: Undefined substitution referenced: "o +".
> Documentation/gpu/drm-kms:389: ./drivers/gpu/drm/drm_blend.c:265: ERROR: Undefined substitution referenced: "+ o".
> Documentation/gpu/drm-kms:389: ./drivers/gpu/drm/drm_blend.c:273: ERROR: Undefined substitution referenced: "o +".
> Documentation/gpu/drm-kms:389: ./drivers/gpu/drm/drm_blend.c:277: ERROR: Undefined substitution referenced: "o | ROTATE_180 | +".
> Documentation/gpu/drm-kms:389: ./drivers/gpu/drm/drm_blend.c:277: ERROR: Undefined substitution referenced: "+ | | o".
> Documentation/gpu/drm-kms:389: ./drivers/gpu/drm/drm_blend.c:281: ERROR: Undefined substitution referenced: "o | ROTATE_270 |+ o".
> Documentation/gpu/drm-kms:389: ./drivers/gpu/drm/drm_blend.c:290: ERROR: Undefined substitution referenced: "o +".
> Documentation/gpu/drm-kms:389: ./drivers/gpu/drm/drm_blend.c:290: ERROR: Undefined substitution referenced: "+ o".
> 
> I have to wrap them in literal blocks:
> 
> ---- >8 ----
> diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
> index 6fbb8730d8b022..f2cbf8d8efbbbc 100644
> --- a/drivers/gpu/drm/drm_blend.c
> +++ b/drivers/gpu/drm/drm_blend.c
> @@ -259,36 +259,36 @@ EXPORT_SYMBOL(drm_plane_create_alpha_property);
>   * the X and Y axis are within the source rectangle, i.e.  the X/Y axis before
>   * rotation.
>   *
> - * Here are some examples of rotation and reflections:
> + * Here are some examples of rotation and reflections::
>   *
> - * |o  +|  REFLECT_X  |+  o|
> - * |    |  ========>  |    |
> - * |    |             |    |
> + *	|o  +|  REFLECT_X  |+  o|
> + *	|    |  ========>  |    |
> + *	|    |             |    |
>   *
> - * |o   |  REFLECT_Y  |+   |
> - * |    |  ========>  |    |
> - * |+   |             |o   |
> + *	|o   |  REFLECT_Y  |+   |
> + *	|    |  ========>  |    |
> + *	|+   |             |o   |
>   *
> - * |o  +|  ROTATE_90  |+   |
> - * |    |  ========>  |    |
> - * |    |             |o   |
> + *	|o  +|  ROTATE_90  |+   |
> + *	|    |  ========>  |    |
> + *	|    |             |o   |
>   *
> - * |o   |  ROTATE_180 |   +|
> - * |    |  ========>  |    |
> - * |+   |             |   o|
> + *	|o   |  ROTATE_180 |   +|
> + *	|    |  ========>  |    |
> + *	|+   |             |   o|
>   *
> - * |o   |  ROTATE_270 |+  o|
> - * |    |  ========>  |    |
> - * |+   |             |    |
> + *	|o   |  ROTATE_270 |+  o|
> + *	|    |  ========>  |    |
> + *	|+   |             |    |
>   *
>   * Rotation and reflection can be combined to handle more situations. In this condition, the
>   * reflection is applied first and the rotation in second.
>   *
> - * For example the expected result for DRM_MODE_ROTATE_90 | DRM_MODE_REFLECT_X is:
> + * For example the expected result for DRM_MODE_ROTATE_90 | DRM_MODE_REFLECT_X is::
>   *
> - * |o  +|  REFLECT_X  |+  o|  ROTATE_90  |o   |
> - * |    |  ========>  |    |  ========>  |    |
> - * |    |             |    |             |+   |
> + *	|o  +|  REFLECT_X  |+  o|  ROTATE_90  |o   |
> + *	|    |  ========>  |    |  ========>  |    |
> + *	|    |             |    |             |+   |
>   *
>   * It is not possible to pass multiple rotation at the same time. (i.e ROTATE_90 | ROTATE_180 is
>   * not the same as ROTATE_270 and is not accepted).
> 
> Thanks.
> 
> -- 
> An old man doll... just what I always wanted! - Clara

It will be fixed in the next version, thanks!

Louis Chauvet

-- 
Louis Chauvet, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 2/3] drm: drm_blend.c: Improve drm_plane_create_rotation_property kernel doc

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 5748 bytes --]

On Wed, 17 Apr 2024 00:30:58 +0200
Louis Chauvet <louis.chauvet@bootlin.com> wrote:

> Le 15/04/24 - 14:36, Pekka Paalanen a écrit :
> > On Tue, 09 Apr 2024 12:04:06 +0200
> > Louis Chauvet <louis.chauvet@bootlin.com> wrote:
> >   
> > > The expected behavior of the rotation property was not very clear. Add
> > > more examples to explain what is the expected result.
> > > 
> > > Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
> > > ---
> > >  drivers/gpu/drm/drm_blend.c | 52 +++++++++++++++++++++++++++++++++------------
> > >  1 file changed, 38 insertions(+), 14 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
> > > index 8d4b317eb9d7..6fbb8730d8b0 100644
> > > --- a/drivers/gpu/drm/drm_blend.c
> > > +++ b/drivers/gpu/drm/drm_blend.c
> > > @@ -104,6 +104,9 @@
> > >   *	Without this property the rectangle is only scaled, but not rotated or
> > >   *	reflected.
> > >   *
> > > + *	See drm_plane_create_rotation_property() for details about the expected rotation and
> > > + *	reflection behavior.  
> > 
> > I think internal function docs should be referring to UAPI docs, and
> > not vice versa. Internal functions can change, but UAPI cannot.
> >   
> > > + *
> > >   *	Possbile values:
> > >   *
> > >   *	"rotate-<degrees>":
> > > @@ -114,18 +117,6 @@
> > >   *		Signals that the contents of a drm plane is reflected along the
> > >   *		<axis> axis, in the same way as mirroring.
> > >   *
> > > - *	reflect-x::
> > > - *
> > > - *			|o |    | o|
> > > - *			|  | -> |  |
> > > - *			| v|    |v |
> > > - *
> > > - *	reflect-y::
> > > - *
> > > - *			|o |    | ^|
> > > - *			|  | -> |  |
> > > - *			| v|    |o |
> > > - *
> > >   * zpos:
> > >   *	Z position is set up with drm_plane_create_zpos_immutable_property() and
> > >   *	drm_plane_create_zpos_property(). It controls the visibility of overlapping
> > > @@ -266,8 +257,41 @@ EXPORT_SYMBOL(drm_plane_create_alpha_property);
> > >   *
> > >   * Rotation is the specified amount in degrees in counter clockwise direction,
> > >   * the X and Y axis are within the source rectangle, i.e.  the X/Y axis before
> > > - * rotation. After reflection, the rotation is applied to the image sampled from
> > > - * the source rectangle, before scaling it to fit the destination rectangle.
> > > + * rotation.
> > > + *
> > > + * Here are some examples of rotation and reflections:
> > > + *
> > > + * |o  +|  REFLECT_X  |+  o|
> > > + * |    |  ========>  |    |
> > > + * |    |             |    |
> > > + *
> > > + * |o   |  REFLECT_Y  |+   |
> > > + * |    |  ========>  |    |
> > > + * |+   |             |o   |
> > > + *
> > > + * |o  +|  ROTATE_90  |+   |
> > > + * |    |  ========>  |    |
> > > + * |    |             |o   |
> > > + *
> > > + * |o   |  ROTATE_180 |   +|
> > > + * |    |  ========>  |    |
> > > + * |+   |             |   o|
> > > + *
> > > + * |o   |  ROTATE_270 |+  o|
> > > + * |    |  ========>  |    |
> > > + * |+   |             |    |
> > > + *
> > > + * Rotation and reflection can be combined to handle more situations. In this condition, the
> > > + * reflection is applied first and the rotation in second.  
> > 
> > When going in which direction? Is the first image the FB source
> > rectangle contents, and the second image what the plane looks like in
> > CRTC frame of reference?  
> 
> The first is the FB source, the second is the expected result on the CRTC 
> output.
> 
> I will add a sentence before the schemas:
> 
>  * Here are some examples of rotation and reflections, on the left it is 
>  * the content of the source frame buffer, on the right is the expected 
>  * result on the CRTC output.
> 
> >   
> > > + *
> > > + * For example the expected result for DRM_MODE_ROTATE_90 | DRM_MODE_REFLECT_X is:
> > > + *
> > > + * |o  +|  REFLECT_X  |+  o|  ROTATE_90  |o   |
> > > + * |    |  ========>  |    |  ========>  |    |
> > > + * |    |             |    |             |+   |
> > > + *
> > > + * It is not possible to pass multiple rotation at the same time. (i.e ROTATE_90 | ROTATE_180 is
> > > + * not the same as ROTATE_270 and is not accepted).
> > >   */
> > >  int drm_plane_create_rotation_property(struct drm_plane *plane,
> > >  				       unsigned int rotation,
> > >   
> > 
> > These are definitely improvements. I think they should just be in the
> > UAPI section rather than implementation details.  
> 
> So, somewhere in [1]? It feel strange because this is in the `GPU Driver 
> Developer’s Guide` section, not a `UAPI interfaces`.

The whole kernel documentation layout is a big mess. I *still*
spend ages trying to find the pages I know exist.

https://docs.kernel.org/gpu/drm-kms.html#plane-composition-properties
is where properties are documented for userspace developers to look at.

Let's see... I'm cheating and looking what hierarchy I need to follow
to find the place I am at:

The Linux Kernel
-> Subsystems
  -> Human Interfaces: GPU Driver Developer's Guide
    -> Kernel Mode Setting (KMS)
    <- oops, don't click that, take a step back
    -> Kernel Mode Setting (KMS): KMS properties
    <- oops, don't click that, take a step back
    -> Kernel Mode Setting (KMS): KMS properties: Plane Composition Properties

So yeah, UAPI docs are under graphics driver developer's guide, inside
human interface subsystems.


Thanks,
pq

> [1]: https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html
> 
> Thanks,
> Louis Chauvet
> 
> > Disclaimer again to everyone else: I cannot tell if this is the correct
> > documentation or its inverse.
> > 
> > 
> > Thanks,
> > pq  
> 
> 
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 3/3] drm/fourcc: Add documentation around drm_format_info

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 6737 bytes --]

On Wed, 17 Apr 2024 00:30:58 +0200
Louis Chauvet <louis.chauvet@bootlin.com> wrote:

> Le 15/04/24 - 15:00, Pekka Paalanen a écrit :
> > On Tue, 09 Apr 2024 12:04:07 +0200
> > Louis Chauvet <louis.chauvet@bootlin.com> wrote:
> >   
> > > Let's provide more details about the drm_format_info structure because
> > > its content may not be straightforward for someone not used to video
> > > formats and drm internals.
> > > 
> > > Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
> > > ---
> > >  include/drm/drm_fourcc.h | 45 ++++++++++++++++++++++++++++++++++++++-------
> > >  1 file changed, 38 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/include/drm/drm_fourcc.h b/include/drm/drm_fourcc.h
> > > index ccf91daa4307..66cc30e28f79 100644
> > > --- a/include/drm/drm_fourcc.h
> > > +++ b/include/drm/drm_fourcc.h
> > > @@ -58,6 +58,44 @@ struct drm_mode_fb_cmd2;
> > >  
> > >  /**
> > >   * struct drm_format_info - information about a DRM format
> > > + *
> > > + * A drm_format_info describes how planes and pixels are stored in memory.
> > > + *
> > > + * Some format like YUV can have multiple planes, counted in @num_planes. It
> > > + * means that a full pixel can be stored in multiple non-continuous buffers.
> > > + * For example, NV12 is a YUV format using two planes: one for the Y values and
> > > + * one for the UV values.
> > > + *
> > > + * On each plane, the "pixel" unit can be different in case of subsampling. For
> > > + * example with the NV12 format, a pixel in the UV plane is used for four pixels
> > > + * in the Y plane.
> > > + * The fields @hsub and @vsub are the relation between the size of the main
> > > + * plane and the size of the subsampled planes in pixels:
> > > + *	plane[0] width = hsub * plane[1] width
> > > + *	plane[0] height = vsub * plane[1] height  
> > 
> > This makes it sound like plane[1] would be the one determining the
> > image size. It is plane[0] that determines the image size (I don't know
> > of a format that would have it otherwise), and vsub and hsub are used
> > as divisors. It's in their name, too: horizontal/vertical sub-sampling.
> > 
> > This is important for images with odd dimensions. If plane[1]
> > determined the image size, it would be impossible to have odd sized
> > NV12 images, for instance.
> > 
> > Odd dimensions also imply something about rounding the size of the
> > sub-sampled planes. I guess the rounding is up, not down?  
> 
> I will change the equation to:
> 
> plane[1] = plane[0] / hsub (round up)
> 
> Can a DRM maintainer confirm the rounding up?
>  
> > > + *
> > > + * In some formats, pixels are not independent in memory. It can be a packed  
> > 
> > "Independent in memory" sounds to me like it describes sub-sampling:
> > some pixel components are shared between multiple pixels. Here you seem
> > to refer to just packing: one pixel's data may take a fractional number
> > of bytes.  
> 
>  * In some formats, pixels are not individually addressable. It ...
> 
> > > + * representation to store more pixels per byte (for example P030 uses 4 bytes
> > > + * for three 10 bit pixels). It can also be used to represent tiled formats,  
> > 
> > s/tiled/block/
> > 
> > Tiling is given by format modifiers rather than formats.  
> 
> Fixed in the v2.
> 
> > > + * where a continuous buffer in memory can represent a rectangle of pixels (for
> > > + * example, in DRM_FORMAT_Y0L0, a buffer of 8 bytes represents a 2x2 pixel
> > > + * region of the picture).
> > > + *	The field @char_per_block is the size of a block on a specific plane, in
> > > + *	bytes.
> > > + *	The fields @block_w and @block_h are the size of a block in pixels.
> > > + *
> > > + * The older format representation (which only uses @cpp, kept for historical  
> > 
> > Move the paren to: representation which only uses @cpp (kept
> > 
> > so that the sentence is still understandable if one skips the
> > parenthesised part.  
> 
> Fixed in v2.
> 
> > > + * reasons because there are a lot of places in drivers where it's used) is
> > > + * assuming that a block is always 1x1 pixel.
> > > + *
> > > + * To keep the compatibility with older format representations and treat block
> > > + * and non-block formats in the same way one should use:
> > > + *	- @char_per_block to access the size of a block on a specific plane, in
> > > + *	bytes.
> > > + *	- drm_format_info_block_width() to access the width of a block of a
> > > + *	specific plane, in pixels.
> > > + *	- drm_format_info_block_height() to access the height of a block of a
> > > + *	specific plane, in pixels.
> > >   */
> > >  struct drm_format_info {
> > >  	/** @format: 4CC format identifier (DRM_FORMAT_*) */
> > > @@ -97,13 +135,6 @@ struct drm_format_info {
> > >  		 * formats for which the memory needed for a single pixel is not
> > >  		 * byte aligned.
> > >  		 *
> > > -		 * @cpp has been kept for historical reasons because there are
> > > -		 * a lot of places in drivers where it's used. In drm core for
> > > -		 * generic code paths the preferred way is to use
> > > -		 * @char_per_block, drm_format_info_block_width() and
> > > -		 * drm_format_info_block_height() which allows handling both
> > > -		 * block and non-block formats in the same way.
> > > -		 *
> > >  		 * For formats that are intended to be used only with non-linear
> > >  		 * modifiers both @cpp and @char_per_block must be 0 in the
> > >  		 * generic format table. Drivers could supply accurate
> > >   
> > 
> > Other than that, sounds fine to me.
> > 
> > Perhaps one thing to clarify is that chroma sub-sampling and blocks are
> > two different things. Chroma sub-sampling is about the resolution of
> > the chroma (image). Blocks are about packing multiple pixels' components
> > into a contiguous addressable block of memory. Blocks could appear
> > inside a separate sub-sampled UV plane, for example.  
> 
> Is this clear? i will add it just before "In some formats, 
> pixels...
> 
>  * Chroma subsamping (hsub/vsub) must not be confused with pixel blocks. The
>  * first describe the relation between the resolution of each color components
>  * (for YUV format, the relation between the "y" resolution and the "uv"
>  * resolution), the second describe the way to pack multiple pixels into one
>  * contiguous block of memory (for example, DRM_FORMAT_Y0L0, one block is 2x2
>  * pixels).

A different example would be better, e.g. DRM_FORMAT_R2, because chroma
sub-sampling too can share U and V for a 2x2 set of pixels. R2 is in
the RGB family, so chroma sub-sampling does not even apply.

Yes, sounds fine.


Thanks,
pq

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]