Hi Thomas,

On Mon, May 30, 2022 at 10:34 AM Geert Uytterhoeven
[off-list ref] wrote:

On Mon, May 30, 2022 at 9:47 AM Thomas Zimmermann [off-list ref] wrote:

quoted

Am 28.05.22 um 22:17 schrieb Geert Uytterhoeven:

quoted

On Mon, Oct 14, 2019 at 4:05 PM Thomas Zimmermann [off-list ref] wrote:

quoted

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>

quoted

--- 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.

quoted

quoted

quoted

+
+       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.

quoted

quoted

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!

Getting the format from the new plane state of pipe->plane doesn't
work, as pipe->plane.state->fb = NULL.
But it is indeed available in the drm_simple_display_pipe_funcs.check()
callback, so that seems to work...

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