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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Geert,

first of all, thanks for looking at the patch.

Am 28.05.22 um 22:17 schrieb Geert Uytterhoeven:
Hi Thomas,

On Mon, Oct 14, 2019 at 4:05 PM Thomas Zimmermann <tzimmermann@xxxxxxx> 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@xxxxxxx>

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


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


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.

Best regards
Thomas

Thanks!

+       if (ret < 0)
+               return false;
+
+       drm_mode_update_from_fb_var_screeninfo(adjusted_mode, &fb_var);
+
         return true;
  }
  EXPORT_SYMBOL(drm_fbconv_simple_display_pipe_mode_fixup);

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

--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

Attachment: OpenPGP_signature
Description: OpenPGP digital signature


[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux