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: <20220215165226.2738568-1-geert@linux-m68k.org>
 <20220215165226.2738568-3-geert@linux-m68k.org> <4fff0c08-adab-c1d5-4a7e-1513cb2bf7ca@suse.de>
 <CAMuHMdVK7eWfod73JQAntO=7BAMEcS-ktH4NJmDjna3zUn7giw@mail.gmail.com> <4ed13232-bee3-0bea-9abb-51bf9d0bf84b@suse.de>
In-Reply-To: <4ed13232-bee3-0bea-9abb-51bf9d0bf84b@suse.de>
From:   Geert Uytterhoeven <geert@linux-m68k.org>
Date:   Fri, 18 Feb 2022 09:53:20 +0100
Message-ID: <CAMuHMdXjXdfNSvy4tvTgxe16EHoZ1CnXKNXRoP4SVpHnSFLvAQ@mail.gmail.com>
Subject: Re: [PATCH 2/8] drm/fb-helper: Add support for DRM_FORMAT_C[124]
To:     Thomas Zimmermann <tzimmermann@suse.de>
Cc:     Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
        Maxime Ripard <mripard@kernel.org>,
        David Airlie <airlied@linux.ie>,
        Daniel Vetter <daniel@ffwll.ch>, Helge Deller <deller@gmx.de>,
        Javier Martinez Canillas <javierm@redhat.com>,
        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>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Thomas,

On Fri, Feb 18, 2022 at 9:14 AM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> Am 17.02.22 um 17:12 schrieb Geert Uytterhoeven:
> > On Thu, Feb 17, 2022 at 3:57 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> >> Am 15.02.22 um 17:52 schrieb Geert Uytterhoeven:
> >>> Add support for color-indexed frame buffer formats with two, four, and
> >>> sixteen colors to the DRM framebuffer helper functions:
> >>>     1. Add support for depths 1/2/4 to the damage helper,
> >>>     2. For color-indexed modes, the length of the color bitfields must be
> >>>        set to the color depth, else the logo code may pick a logo with too
> >>>        many colors.  Drop the incorrect DAC width comment, which
> >>>        originates from the i915 driver.
> >>>     3. Accept C[124] modes when validating or filling in struct
> >>>        fb_var_screeninfo, and  use the correct number of bits per pixel.
> >>>     4. Set the visual to FB_VISUAL_PSEUDOCOLOR for all supported
> >>>        color-indexed modes.
> >>>
> >>> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> >
> >>> --- a/drivers/gpu/drm/drm_fb_helper.c
> >>> +++ b/drivers/gpu/drm/drm_fb_helper.c
> >>> @@ -376,12 +376,34 @@ static void drm_fb_helper_damage_blit_real(struct drm_fb_helper *fb_helper,
> >>>                                           struct iosys_map *dst)
> >>>    {
> >>>        struct drm_framebuffer *fb = fb_helper->fb;
> >>> -     unsigned int cpp = fb->format->cpp[0];
> >>> -     size_t offset = clip->y1 * fb->pitches[0] + clip->x1 * cpp;
> >>> -     void *src = fb_helper->fbdev->screen_buffer + offset;
> >>> -     size_t len = (clip->x2 - clip->x1) * cpp;
> >>> +     size_t offset = clip->y1 * fb->pitches[0];
> >>> +     size_t len = clip->x2 - clip->x1;
> >>>        unsigned int y;
> >>> +     void *src;
> >>>
> >>> +     switch (fb->format->depth) {
> >>
> >> The depth field is deprecated. It's probably better to use
> >> fb->format->format and test against 4CC codes.
> >
> > The reason I checked for depth instead of a 4CC code is that the only
> > thing that matters here is the number of bits per pixel.  Hence this
> > function won't need any changes to support R1, R2, R4, and D1 later.
> > When we get here, we already know that we are using a format that
> > is supported by the fbdev helper code, and thus passed the 4CC
> > checks elsewhere.
>
> At some point, we will probably have to change several of these tests to
> 4cc. C8 and RGB332 both have 8-bit depth/bpp; same for C4 and RGB121; or
> whatever low-color formats we also want to add.
>
> It's not a blocker now, but maybe something to keep in mind.
>
> >
> > Alternatively, we could introduce drm_format_info_bpp() earlier in
> > the series, and use that?
>
> Having a helper for this might indeed be useful. We use depth for the
> number of color bits and bpp for the number of bits in he pixel.  That's
> important for XRGB8888, where depth is 24, or XRGB555 where depth is 15.
>
> If that makes sense, maybe have a helper for depth and one for bpp, even
> if they return the same value in most of the cases.

The helper for bpp is introduced in "[PATCH 3/8] drm/fourcc: Add
drm_format_info_bpp() helper".
I don't think we need a helper for depth, there's already the .depth
field.  It might be deprecated, but it's still used?
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