Re: [PATCH v2 09/15] drm/fbconv: Mode-setting pipeline enable / disable

[Geert Uytterhoeven <geert@linux-m68k.org>]

Hi Thomas,

On Mon, May 30, 2022 at 9:47 AM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> first of all, thanks for looking at the patch.

Thank you, your patch series helped a lot.

> Am 28.05.22 um 22:17 schrieb Geert Uytterhoeven:
> > On Mon, Oct 14, 2019 at 4:05 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> >> The display mode is set by converting the DRM display mode to an
> >> fb_info state and handling it to the fbdev driver's fb_setvar()
> >> function. This also requires a color depth, which we take from the
> >> value of struct drm_mode_config.preferred_depth
> >>
> >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> >
> >> --- a/drivers/gpu/drm/drm_fbconv_helper.c
> >> +++ b/drivers/gpu/drm/drm_fbconv_helper.c
> >> @@ -919,6 +919,24 @@ static int drm_fbconv_update_fb_var_screeninfo_from_framebuffer(
> >>          return 0;
> >>   }
> >>
> >> +static int drm_fbconv_update_fb_var_screeninfo_from_simple_display_pipe(
> >> +       struct fb_var_screeninfo *fb_var, struct drm_simple_display_pipe *pipe)
> >> +{
> >> +       struct drm_plane *primary = pipe->crtc.primary;
> >> +       struct drm_fbconv_modeset *modeset = drm_fbconv_modeset_of_pipe(pipe);
> >> +
> >> +       if (primary && primary->state && primary->state->fb)
> >> +               return drm_fbconv_update_fb_var_screeninfo_from_framebuffer(
> >> +                       fb_var, primary->state->fb,
> >> +                       modeset->fb_info->fix.smem_len);
> >> +
> >> +       fb_var->xres_virtual = fb_var->xres;
> >> +       fb_var->yres_virtual = fb_var->yres;
> >> +       fb_var->bits_per_pixel = modeset->dev->mode_config.preferred_depth;
> >
> > This looks wrong to me: IMHO bits_per_pixel should be derived from
> > the fourcc format of the _new_ mode to be set...
>
> Indeed, this appears to be wrong.

OK.

>
> >
> >> +
> >> +       return 0;
> >> +}
> >> +
> >>   /**
> >>    * drm_fbconv_simple_display_pipe_mode_valid - default implementation for
> >>    *     struct drm_simple_display_pipe_funcs.mode_valid
> >> @@ -950,6 +968,28 @@ bool drm_fbconv_simple_display_pipe_mode_fixup(
> >>          struct drm_crtc *crtc, const struct drm_display_mode *mode,
> >>          struct drm_display_mode *adjusted_mode)
> >>   {
> >> +       struct drm_simple_display_pipe *pipe =
> >> +               container_of(crtc, struct drm_simple_display_pipe, crtc);
> >> +       struct drm_fbconv_modeset *modeset = drm_fbconv_modeset_of_pipe(pipe);
> >> +       struct fb_var_screeninfo fb_var;
> >> +       int ret;
> >> +
> >> +       if (!modeset->fb_info->fbops->fb_check_var)
> >> +               return true;
> >> +
> >> +       drm_fbconv_init_fb_var_screeninfo_from_mode(&fb_var, mode);
> >> +
> >> +       ret = drm_fbconv_update_fb_var_screeninfo_from_simple_display_pipe(
> >> +               &fb_var, &modeset->pipe);
> >> +       if (ret)
> >> +               return true;
> >> +
> >> +       ret = modeset->fb_info->fbops->fb_check_var(&fb_var, modeset->fb_info);
> >
> > ... hence this fails if the requested mode is valid with the new
> > fourcc format, but invalid with the old (but preferred) depth.
> > E.g. due to bandwidth limitations, a high-resolution mode is valid
> > with a low color depth, while a high color depth is limited to lower
> > resolutions.
>
> I tested the helpers with various fbdev drivers and modified them until
> all of these drivers produced at least some output. I'm not surprised
> that there are still bugs.

As usual, the devil is in the details ;-)

The other issue I was facing are the non-rounding KHZ2PICOS() and
PICOS2KHZ() macros, and the numerous back-and-forth conversions:
a valid pixclock in kHz is converted to a valid pixclock in ps,
and accepted.  The returned pixclock in ps is slightly different,
and converted to an invalid pixclock in kHz, hence rejected in the
next iteration (remember: fb_ops.fb_check_var() should only round up
values to match, never round down)...

> > Unfortunately we do not know the new fourcc format here, as both
> > drm_simple_display_pipe_funcs.mode_{valid,fixup}() are passed
> > the mode (from drm_mode_set.mode), but not the new format (from
> > drm_mode_set.fb->format).
> >
> > Am I missing something? Is the new format available in some other way?
>
> We can always get the format from the new plane state of
> modeset->pipe->plane. We'd have this in the atomic_check call. And it
> appears that drm_fbconv_simple_display_pipe_check() is a better place
> for this code anyway.

Thanks, I'll give that a try!

Anyway, I finally made some progress with KMS-style mode-setting inside
my ataridrm driver. Before, I relied solely on initial fbdev-style
mode-setting in the probe function.

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