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 <[email protected]>
---
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 <[email protected]>
As the function uses non-trivial bit operations, add a little paragraph
to describe what is the expected behavior.
Signed-off-by: Louis Chauvet <[email protected]>
---
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
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 <[email protected]>
---
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
On Tue, 09 Apr 2024, Louis Chauvet <[email protected]> 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 <[email protected]>
> ---
> 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
Le 09/04/24 - 13:18, Jani Nikula a ?crit :
> On Tue, 09 Apr 2024, Louis Chauvet <[email protected]> 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 <[email protected]>
> > ---
> > 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
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
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 <[email protected]>
--
An old man doll... just what I always wanted! - Clara
Le 15/04/24 - 15:00, Pekka Paalanen a ?crit :
> On Tue, 09 Apr 2024 12:04:07 +0200
> Louis Chauvet <[email protected]> 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 <[email protected]>
> > ---
> > 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
On Wed, 17 Apr 2024 00:30:58 +0200
Louis Chauvet <[email protected]> wrote:
> Le 15/04/24 - 15:00, Pekka Paalanen a écrit :
> > On Tue, 09 Apr 2024 12:04:07 +0200
> > Louis Chauvet <[email protected]> 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 <[email protected]>
> > > ---
> > > 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