diff mbox series

[v3,01/10] drm/fourcc: Add drm_format_info_bpp() helper

Message ID 1cae5ebc28513ec1c91c66b00647ce3ca23bfba7.1657294931.git.geert@linux-m68k.org (mailing list archive)
State New, archived
Headers show
Series drm: Add support for low-color frame buffer formats | expand

Commit Message

Geert Uytterhoeven July 8, 2022, 6:20 p.m. UTC
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(+)

Comments

Daniel Vetter Aug. 10, 2022, 3:59 p.m. UTC | #1
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
>
Geert Uytterhoeven Aug. 11, 2022, 7:59 a.m. UTC | #2
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
Daniel Vetter Aug. 11, 2022, 4:11 p.m. UTC | #3
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
Sam Ravnborg Aug. 11, 2022, 6:29 p.m. UTC | #4
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
Geert Uytterhoeven Aug. 11, 2022, 6:49 p.m. UTC | #5
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
Sam Ravnborg Aug. 11, 2022, 7:30 p.m. UTC | #6
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 mbox series

Patch

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