Hi Thomas,
On Fri, Feb 18, 2022 at 9:14 AM Thomas Zimmermann <tzimmermann@xxxxxxx> wrote:
> Am 17.02.22 um 17:12 schrieb Geert Uytterhoeven:
> > On Thu, Feb 17, 2022 at 3:57 PM Thomas Zimmermann <tzimmermann@xxxxxxx> 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@xxxxxxxxxxxxxx>
> >
> >>> --- 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@xxxxxxxxxxxxxx
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
