| Message ID | 20240409-google-drm-doc-v1-3-033d55cc8250@bootlin.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: Multiple documentation update | expand |
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
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.
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
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
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
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(-)