Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <cover.1657294931.git.geert@linux-m68k.org> <1cae5ebc28513ec1c91c66b00647ce3ca23bfba7.1657294931.git.geert@linux-m68k.org>
 <YvPVxy4kYKdzWgT8@phenom.ffwll.local>
In-Reply-To: <YvPVxy4kYKdzWgT8@phenom.ffwll.local>
From:   Geert Uytterhoeven <geert@linux-m68k.org>
Date:   Thu, 11 Aug 2022 09:59:39 +0200
Message-ID: <CAMuHMdVMuuXgYW-AkyB+G77Wsjkm715u1ifDvaY=5DufXjryRA@mail.gmail.com>
Subject: Re: [PATCH v3 01/10] drm/fourcc: Add drm_format_info_bpp() helper
To:     Daniel Vetter <daniel@ffwll.ch>
Cc:     Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
        Maxime Ripard <mripard@kernel.org>,
        Thomas Zimmermann <tzimmermann@suse.de>,
        David Airlie <airlied@linux.ie>,
        DRI Development <dri-devel@lists.freedesktop.org>,
        Linux Fbdev development list <linux-fbdev@vger.kernel.org>,
        "Linux/m68k" <linux-m68k@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Javier Martinez Canillas <javierm@redhat.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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