| Message ID | 1cae5ebc28513ec1c91c66b00647ce3ca23bfba7.1657294931.git.geert@linux-m68k.org |
|---|---|
| State | New |
| Headers | show |
| Series | drm: Add support for low-color frame buffer formats | expand |
On Fri, Jul 08, 2022 at 08:20:46PM +0200, Geert Uytterhoeven wrote: > Add a helper to retrieve the actual number of bits per pixel for a > plane, taking into account the number of characters and pixels per > block for tiled formats. > > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org> > Reviewed-by: Javier Martinez Canillas <javierm@redhat.com> > --- > v3: > - Add Reviewed-by, > > v2: > - Move up. > --- > drivers/gpu/drm/drm_fourcc.c | 19 +++++++++++++++++++ > include/drm/drm_fourcc.h | 1 + > 2 files changed, 20 insertions(+) > > diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c > index 07741b678798b0f1..cf48ea0b2cb70ba8 100644 > --- a/drivers/gpu/drm/drm_fourcc.c > +++ b/drivers/gpu/drm/drm_fourcc.c > @@ -370,6 +370,25 @@ unsigned int drm_format_info_block_height(const struct drm_format_info *info, > } > EXPORT_SYMBOL(drm_format_info_block_height); > > +/** > + * drm_format_info_bpp - number of bits per pixel > + * @info: pixel format info > + * @plane: plane index > + * > + * Returns: > + * The actual number of bits per pixel, depending on the plane index. > + */ > +unsigned int drm_format_info_bpp(const struct drm_format_info *info, int plane) > +{ > + if (!info || plane < 0 || plane >= info->num_planes) > + return 0; > + > + return info->char_per_block[plane] * 8 / > + (drm_format_info_block_width(info, plane) * > + drm_format_info_block_height(info, plane)); Do we really needs this for blocky formats where this is potentially ill-defined? I think if there's no need then this should also return 0 when block_width/height != 1, it doesn't make much sense to compute bpp when it's not really bits per _pixel_. Minimally this needs to check whether the division actually makes sense or whether there's a reminder, and if there's reminder, then fail. But that feels like a bad hack and I think we should avoid it if it's not absolutely necessary. Otherwise lgtm. -Daniel > +} > +EXPORT_SYMBOL(drm_format_info_bpp); > + > /** > * drm_format_info_min_pitch - computes the minimum required pitch in bytes > * @info: pixel format info > diff --git a/include/drm/drm_fourcc.h b/include/drm/drm_fourcc.h > index 22aa64d07c7905e2..3800a7ad7f0cda7a 100644 > --- a/include/drm/drm_fourcc.h > +++ b/include/drm/drm_fourcc.h > @@ -313,6 +313,7 @@ unsigned int drm_format_info_block_width(const struct drm_format_info *info, > int plane); > unsigned int drm_format_info_block_height(const struct drm_format_info *info, > int plane); > +unsigned int drm_format_info_bpp(const struct drm_format_info *info, int plane); > uint64_t drm_format_info_min_pitch(const struct drm_format_info *info, > int plane, unsigned int buffer_width); > > -- > 2.25.1 >
Hi Daniel, On Wed, Aug 10, 2022 at 5:59 PM Daniel Vetter <daniel@ffwll.ch> wrote: > On Fri, Jul 08, 2022 at 08:20:46PM +0200, Geert Uytterhoeven wrote: > > Add a helper to retrieve the actual number of bits per pixel for a > > plane, taking into account the number of characters and pixels per > > block for tiled formats. > > > > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org> > > Reviewed-by: Javier Martinez Canillas <javierm@redhat.com> > > --- a/drivers/gpu/drm/drm_fourcc.c > > +++ b/drivers/gpu/drm/drm_fourcc.c > > @@ -370,6 +370,25 @@ unsigned int drm_format_info_block_height(const struct drm_format_info *info, > > } > > EXPORT_SYMBOL(drm_format_info_block_height); > > > > +/** > > + * drm_format_info_bpp - number of bits per pixel > > + * @info: pixel format info > > + * @plane: plane index > > + * > > + * Returns: > > + * The actual number of bits per pixel, depending on the plane index. > > + */ > > +unsigned int drm_format_info_bpp(const struct drm_format_info *info, int plane) > > +{ > > + if (!info || plane < 0 || plane >= info->num_planes) > > + return 0; > > + > > + return info->char_per_block[plane] * 8 / > > + (drm_format_info_block_width(info, plane) * > > + drm_format_info_block_height(info, plane)); > > Do we really needs this for blocky formats where this is potentially > ill-defined? I think if there's no need then this should also return 0 > when block_width/height != 1, it doesn't make much sense to compute bpp > when it's not really bits per _pixel_. Yes, we do need this. For low-color formats, the number of bits per pixel is less than eight, and block_width is larger than one. That is actually the point of this patch. > Minimally this needs to check whether the division actually makes sense or > whether there's a reminder, and if there's reminder, then fail. But that > feels like a bad hack and I think we should avoid it if it's not > absolutely necessary. Looking at drivers/gpu/drm/drm_fourcc.c, the only supported format where there can be a remainder is P030, which has 2 spare bits per 32-bit word, and thus is special anyway. Still, 4 * 8 / 3 = 10, so you get the correct numbers of bits for the first plane. For the second plane, you get 8 * 8 / 3 = 21, but as .is_yuv = true, you have to divide that result by two again, so you get 10 again. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Thu, Aug 11, 2022 at 09:59:39AM +0200, Geert Uytterhoeven wrote: > Hi Daniel, > > On Wed, Aug 10, 2022 at 5:59 PM Daniel Vetter <daniel@ffwll.ch> wrote: > > On Fri, Jul 08, 2022 at 08:20:46PM +0200, Geert Uytterhoeven wrote: > > > Add a helper to retrieve the actual number of bits per pixel for a > > > plane, taking into account the number of characters and pixels per > > > block for tiled formats. > > > > > > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org> > > > Reviewed-by: Javier Martinez Canillas <javierm@redhat.com> > > > > --- a/drivers/gpu/drm/drm_fourcc.c > > > +++ b/drivers/gpu/drm/drm_fourcc.c > > > @@ -370,6 +370,25 @@ unsigned int drm_format_info_block_height(const struct drm_format_info *info, > > > } > > > EXPORT_SYMBOL(drm_format_info_block_height); > > > > > > +/** > > > + * drm_format_info_bpp - number of bits per pixel > > > + * @info: pixel format info > > > + * @plane: plane index > > > + * > > > + * Returns: > > > + * The actual number of bits per pixel, depending on the plane index. > > > + */ > > > +unsigned int drm_format_info_bpp(const struct drm_format_info *info, int plane) > > > +{ > > > + if (!info || plane < 0 || plane >= info->num_planes) > > > + return 0; > > > + > > > + return info->char_per_block[plane] * 8 / > > > + (drm_format_info_block_width(info, plane) * > > > + drm_format_info_block_height(info, plane)); > > > > Do we really needs this for blocky formats where this is potentially > > ill-defined? I think if there's no need then this should also return 0 > > when block_width/height != 1, it doesn't make much sense to compute bpp > > when it's not really bits per _pixel_. > > Yes, we do need this. For low-color formats, the number of bits > per pixel is less than eight, and block_width is larger than one. > That is actually the point of this patch. Hm right, I didn't realize that this is how we have to describe the formats with less than 8 bpp. I think we can include them easily with a check for char_per_block == 1 and then making sure that the division does not have a reminder (just in case someone does something really funny, it could e.g. be a 332 layout or something like that for 3 pixels). > > Minimally this needs to check whether the division actually makes sense or > > whether there's a reminder, and if there's reminder, then fail. But that > > feels like a bad hack and I think we should avoid it if it's not > > absolutely necessary. > > Looking at drivers/gpu/drm/drm_fourcc.c, the only supported format > where there can be a remainder is P030, which has 2 spare bits per > 32-bit word, and thus is special anyway. > Still, 4 * 8 / 3 = 10, so you get the correct numbers of bits for > the first plane. For the second plane, you get 8 * 8 / 3 = 21, > but as .is_yuv = true, you have to divide that result by two again, > so you get 10 again. Yeah I don't think we should describe these with bpp or cpp or anything like that. bpp < 8 makes sense since that's how this has been done since decades, but trying to extend these to funny new formats is a bad idea. This is also why cpp and depth refuse to compute these (or at least should). -Daniel
Hi Geert, Daniel. On Thu, Aug 11, 2022 at 06:11:40PM +0200, Daniel Vetter wrote: > On Thu, Aug 11, 2022 at 09:59:39AM +0200, Geert Uytterhoeven wrote: > > Hi Daniel, > > > > On Wed, Aug 10, 2022 at 5:59 PM Daniel Vetter <daniel@ffwll.ch> wrote: > > > On Fri, Jul 08, 2022 at 08:20:46PM +0200, Geert Uytterhoeven wrote: > > > > Add a helper to retrieve the actual number of bits per pixel for a > > > > plane, taking into account the number of characters and pixels per > > > > block for tiled formats. > > > > > > > > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org> > > > > Reviewed-by: Javier Martinez Canillas <javierm@redhat.com> > > > > > > --- a/drivers/gpu/drm/drm_fourcc.c > > > > +++ b/drivers/gpu/drm/drm_fourcc.c > > > > @@ -370,6 +370,25 @@ unsigned int drm_format_info_block_height(const struct drm_format_info *info, > > > > } > > > > EXPORT_SYMBOL(drm_format_info_block_height); > > > > > > > > +/** > > > > + * drm_format_info_bpp - number of bits per pixel > > > > + * @info: pixel format info > > > > + * @plane: plane index > > > > + * > > > > + * Returns: > > > > + * The actual number of bits per pixel, depending on the plane index. > > > > + */ > > > > +unsigned int drm_format_info_bpp(const struct drm_format_info *info, int plane) > > > > +{ > > > > + if (!info || plane < 0 || plane >= info->num_planes) > > > > + return 0; > > > > + > > > > + return info->char_per_block[plane] * 8 / > > > > + (drm_format_info_block_width(info, plane) * > > > > + drm_format_info_block_height(info, plane)); > > > > > > Do we really needs this for blocky formats where this is potentially > > > ill-defined? I think if there's no need then this should also return 0 > > > when block_width/height != 1, it doesn't make much sense to compute bpp > > > when it's not really bits per _pixel_. > > > > Yes, we do need this. For low-color formats, the number of bits > > per pixel is less than eight, and block_width is larger than one. > > That is actually the point of this patch. > > Hm right, I didn't realize that this is how we have to describe the > formats with less than 8 bpp. > > I think we can include them easily with a check for char_per_block == 1 > and then making sure that the division does not have a reminder (just in > case someone does something really funny, it could e.g. be a 332 layout or > something like that for 3 pixels). > > > > Minimally this needs to check whether the division actually makes sense or > > > whether there's a reminder, and if there's reminder, then fail. But that > > > feels like a bad hack and I think we should avoid it if it's not > > > absolutely necessary. > > > > Looking at drivers/gpu/drm/drm_fourcc.c, the only supported format > > where there can be a remainder is P030, which has 2 spare bits per > > 32-bit word, and thus is special anyway. > > Still, 4 * 8 / 3 = 10, so you get the correct numbers of bits for > > the first plane. For the second plane, you get 8 * 8 / 3 = 21, > > but as .is_yuv = true, you have to divide that result by two again, > > so you get 10 again. > > Yeah I don't think we should describe these with bpp or cpp or anything > like that. bpp < 8 makes sense since that's how this has been done since > decades, but trying to extend these to funny new formats is a bad idea. > This is also why cpp and depth refuse to compute these (or at least > should). Daniel and I discussed this on irc. Let me try to recap here. Using the bits per pixel info from drm_format_info is something we shall try to avoid as this is often a sign of the wrong abstraction/design (my words based on the irc talk). So we shall limit the use of drm_format_info_bpp() to what we need now, thus blocky formats should not be supported - to try to avoid seeing this used more than necessary. Daniel suggested a rename to drm_format_info_legacy_bpp() to make it obvious that this is often/always the wrong solution. I did not jump on doing the rename as I do not know stuff good enough to tell people what to use when this is not the right solution. The rename is simple, it is the follow-up that keep me away. On top of this there is a few formats in drm_drourcc that has a depth field set which should be dropped. .depth is only for the few legacy formats where it is used today. We would also like to convert the fbdev helpers to drm_format_info, and doing so will likely teach us a bit more what we need and what we can drop. Geert - can you give drm_format_info_bpp() a spin so it is limited to the formats used now (not the blocky ones). Sam
Hi Sam, On Thu, Aug 11, 2022 at 8:29 PM Sam Ravnborg <sam@ravnborg.org> wrote: > On Thu, Aug 11, 2022 at 06:11:40PM +0200, Daniel Vetter wrote: > > On Thu, Aug 11, 2022 at 09:59:39AM +0200, Geert Uytterhoeven wrote: > > > On Wed, Aug 10, 2022 at 5:59 PM Daniel Vetter <daniel@ffwll.ch> wrote: > > > > On Fri, Jul 08, 2022 at 08:20:46PM +0200, Geert Uytterhoeven wrote: > > > > > Add a helper to retrieve the actual number of bits per pixel for a > > > > > plane, taking into account the number of characters and pixels per > > > > > block for tiled formats. > > > > > > > > > > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org> > > > > > Reviewed-by: Javier Martinez Canillas <javierm@redhat.com> > > > > > > > > --- a/drivers/gpu/drm/drm_fourcc.c > > > > > +++ b/drivers/gpu/drm/drm_fourcc.c > > > > > @@ -370,6 +370,25 @@ unsigned int drm_format_info_block_height(const struct drm_format_info *info, > > > > > } > > > > > EXPORT_SYMBOL(drm_format_info_block_height); > > > > > > > > > > +/** > > > > > + * drm_format_info_bpp - number of bits per pixel > > > > > + * @info: pixel format info > > > > > + * @plane: plane index > > > > > + * > > > > > + * Returns: > > > > > + * The actual number of bits per pixel, depending on the plane index. > > > > > + */ > > > > > +unsigned int drm_format_info_bpp(const struct drm_format_info *info, int plane) > > > > > +{ > > > > > + if (!info || plane < 0 || plane >= info->num_planes) > > > > > + return 0; > > > > > + > > > > > + return info->char_per_block[plane] * 8 / > > > > > + (drm_format_info_block_width(info, plane) * > > > > > + drm_format_info_block_height(info, plane)); > > > > > > > > Do we really needs this for blocky formats where this is potentially > > > > ill-defined? I think if there's no need then this should also return 0 > > > > when block_width/height != 1, it doesn't make much sense to compute bpp > > > > when it's not really bits per _pixel_. > > > > > > Yes, we do need this. For low-color formats, the number of bits > > > per pixel is less than eight, and block_width is larger than one. > > > That is actually the point of this patch. > > > > Hm right, I didn't realize that this is how we have to describe the > > formats with less than 8 bpp. > > > > I think we can include them easily with a check for char_per_block == 1 > > and then making sure that the division does not have a reminder (just in > > case someone does something really funny, it could e.g. be a 332 layout or > > something like that for 3 pixels). > > > > > > Minimally this needs to check whether the division actually makes sense or > > > > whether there's a reminder, and if there's reminder, then fail. But that > > > > feels like a bad hack and I think we should avoid it if it's not > > > > absolutely necessary. > > > > > > Looking at drivers/gpu/drm/drm_fourcc.c, the only supported format > > > where there can be a remainder is P030, which has 2 spare bits per > > > 32-bit word, and thus is special anyway. > > > Still, 4 * 8 / 3 = 10, so you get the correct numbers of bits for > > > the first plane. For the second plane, you get 8 * 8 / 3 = 21, > > > but as .is_yuv = true, you have to divide that result by two again, > > > so you get 10 again. > > > > Yeah I don't think we should describe these with bpp or cpp or anything > > like that. bpp < 8 makes sense since that's how this has been done since > > decades, but trying to extend these to funny new formats is a bad idea. > > This is also why cpp and depth refuse to compute these (or at least > > should). > > Daniel and I discussed this on irc. Let me try to recap here. > Using the bits per pixel info from drm_format_info is something we shall > try to avoid as this is often a sign of the wrong abstraction/design (my > words based on the irc talk). > So we shall limit the use of drm_format_info_bpp() to what we need now, > thus blocky formats should not be supported - to try to avoid seeing > this used more than necessary. > > Daniel suggested a rename to drm_format_info_legacy_bpp() to make it > obvious that this is often/always the wrong solution. I did not jump on > doing the rename as I do not know stuff good enough to tell people what > to use when this is not the right solution. The rename is simple, it is > the follow-up that keep me away. > > On top of this there is a few formats in drm_drourcc that has a depth > field set which should be dropped. .depth is only for the few legacy > formats where it is used today. > > We would also like to convert the fbdev helpers to drm_format_info, > and doing so will likely teach us a bit more what we need and what we > can drop. > > Geert - can you give drm_format_info_bpp() a spin so it is limited to > the formats used now (not the blocky ones). You mean return 0 if char_per_block[] > 1? I'm not sure it's actually safe to do so (and make this change this late in the development cycle), as this is used in drm_client_buffer_create(), drm_client_buffer_addfb(), and drm_mode_getfb(). Some of them do rely on bpp to be non-zero. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hi Geert. > > > > > > Yeah I don't think we should describe these with bpp or cpp or anything > > > like that. bpp < 8 makes sense since that's how this has been done since > > > decades, but trying to extend these to funny new formats is a bad idea. > > > This is also why cpp and depth refuse to compute these (or at least > > > should). > > > > Daniel and I discussed this on irc. Let me try to recap here. > > Using the bits per pixel info from drm_format_info is something we shall > > try to avoid as this is often a sign of the wrong abstraction/design (my > > words based on the irc talk). > > So we shall limit the use of drm_format_info_bpp() to what we need now, > > thus blocky formats should not be supported - to try to avoid seeing > > this used more than necessary. > > > > Daniel suggested a rename to drm_format_info_legacy_bpp() to make it > > obvious that this is often/always the wrong solution. I did not jump on > > doing the rename as I do not know stuff good enough to tell people what > > to use when this is not the right solution. The rename is simple, it is > > the follow-up that keep me away. > > > > On top of this there is a few formats in drm_drourcc that has a depth > > field set which should be dropped. .depth is only for the few legacy > > formats where it is used today. > > > > We would also like to convert the fbdev helpers to drm_format_info, > > and doing so will likely teach us a bit more what we need and what we > > can drop. > > > > Geert - can you give drm_format_info_bpp() a spin so it is limited to > > the formats used now (not the blocky ones). > > You mean return 0 if char_per_block[] > 1? if char_per_block[] > 1 AND block_w[] > 0 AND block_h[] > 0 should be enough. > I'm not sure it's actually safe to do so (and make this change this late > in the development cycle), as this is used in drm_client_buffer_create(), > drm_client_buffer_addfb(), and drm_mode_getfb(). drm_client_buffer_create() and drm_client_buffer_addfb() both get their format from drm_mode_legacy_fb_format() which do not produce any blocky formats - so they are good. drm_mode_getfb() looks up a framebuffer originally created using one of the above (I think), so here it should also be fine. I do not see the need to push this to fixes, so it has a full cycle to mature if it causes issues. Sam
diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c index 07741b678798b0f1..cf48ea0b2cb70ba8 100644 --- a/drivers/gpu/drm/drm_fourcc.c +++ b/drivers/gpu/drm/drm_fourcc.c @@ -370,6 +370,25 @@ unsigned int drm_format_info_block_height(const struct drm_format_info *info, } EXPORT_SYMBOL(drm_format_info_block_height); +/** + * drm_format_info_bpp - number of bits per pixel + * @info: pixel format info + * @plane: plane index + * + * Returns: + * The actual number of bits per pixel, depending on the plane index. + */ +unsigned int drm_format_info_bpp(const struct drm_format_info *info, int plane) +{ + if (!info || plane < 0 || plane >= info->num_planes) + return 0; + + return info->char_per_block[plane] * 8 / + (drm_format_info_block_width(info, plane) * + drm_format_info_block_height(info, plane)); +} +EXPORT_SYMBOL(drm_format_info_bpp); + /** * drm_format_info_min_pitch - computes the minimum required pitch in bytes * @info: pixel format info diff --git a/include/drm/drm_fourcc.h b/include/drm/drm_fourcc.h index 22aa64d07c7905e2..3800a7ad7f0cda7a 100644 --- a/include/drm/drm_fourcc.h +++ b/include/drm/drm_fourcc.h @@ -313,6 +313,7 @@ unsigned int drm_format_info_block_width(const struct drm_format_info *info, int plane); unsigned int drm_format_info_block_height(const struct drm_format_info *info, int plane); +unsigned int drm_format_info_bpp(const struct drm_format_info *info, int plane); uint64_t drm_format_info_min_pitch(const struct drm_format_info *info, int plane, unsigned int buffer_width);