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

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

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