Re: [PATCH] drm: add fb max width/height fields to drm_mode_config

[Jeykumar Sankaran]

On 2019-09-30 03:39, Ville SyrjÃlà wrote:
> On Fri, Sep 27, 2019 at 06:28:51PM -0700, Jeykumar Sankaran wrote:
> > The mode_config max width/height values determine the maximum
> > resolution the pixel reader can handle.
>
> Not according to the docs I "fixed" a while ago.
>
> > But the same values are
> > used to restrict the size of the framebuffer creation. Hardware's
> > with scaling blocks can operate on framebuffers larger/smaller than
> > that of the pixel reader resolutions by scaling them down/up before
> > rendering.
> >
> > This changes adds a separate framebuffer max width/height fields
> > in drm_mode_config to allow vendors to set if they are different
> > than that of the default max resolution values.
>
> If you're going to change the meaning of the old values you need
> to fix the drivers too.
>
> Personally I don't see too much point in this since you most likely
> want to validate all the other timings as well, and so likely need
> some kind of mode_valid implementation anyway. Hence to validate
> modes there's not much benefit of having global min/max values.
https://patchwork.kernel.org/patch/10467155/

I believe you are referring to this patch.

I am primarily interested in the scaling scenario mentioned here. MSM
and a few other hardware have scaling block that are used both ways:

1) Where FB limits are larger than the display limits. Scalar blocks are 
used to
   downscale the framebuffers and render within display limits.

In this scenario, with your patch, are you suggesting the drivers 
maintain the
display limits locally and use those values in fill_modes() / 
mode_valid() to filter
out invalid modes explicitly instead of mode_config.max_width/height?

2) Where FB limits are smaller than display limits. Enforced for 
performance reasons on low tier hardware.
It reduces the fetch bandwidth and uses post blending scalar block to 
scale up the pixel stream
to match the display resolution.

Any suggestions on how this topology can be handled with a single set of 
max/min values?

Thanks and Regards,
Jeykumar S.
> > Vendors setting these fields should fix their mode_set paths too
> > by filtering and validating the modes against the appropriate max
> > fields in their mode_valid() implementations.
> >
> > Signed-off-by: Neil Armstrong <narmstrong@xxxxxxxxxxxx>
> > Signed-off-by: Jeykumar Sankaran <jsanka@xxxxxxxxxxxxxx>
> > ---
> >  drivers/gpu/drm/drm_framebuffer.c | 15 +++++++++++----
> >  include/drm/drm_mode_config.h     |  3 +++
> >  2 files changed, 14 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_framebuffer.c
> b/drivers/gpu/drm/drm_framebuffer.c
> > index 5756431..2083168 100644
> > --- a/drivers/gpu/drm/drm_framebuffer.c
> > +++ b/drivers/gpu/drm/drm_framebuffer.c
> > @@ -300,14 +300,21 @@ struct drm_framebuffer *
> >  		return ERR_PTR(-EINVAL);
> >  	}
> >
> > -	if ((config->min_width > r->width) || (r->width >
> config->max_width)) {
> > +	if ((config->min_width > r->width) ||
> > +	    (!config->max_fb_width && r->width > config->max_width) ||
> > +	    (config->max_fb_width && r->width > config->max_fb_width)) {
> >  		DRM_DEBUG_KMS("bad framebuffer width %d, should be >= %d
> && <= %d\n",
> > -			  r->width, config->min_width, config->max_width);
> > +			r->width, config->min_width, config->max_fb_width
> ?
> > +			config->max_fb_width : config->max_width);
> >  		return ERR_PTR(-EINVAL);
> >  	}
> > -	if ((config->min_height > r->height) || (r->height >
> config->max_height)) {
> > +
> > +	if ((config->min_height > r->height) ||
> > +	    (!config->max_fb_height && r->height > config->max_height) ||
> > +	    (config->max_fb_height && r->height > config->max_fb_height))
> {
> >  		DRM_DEBUG_KMS("bad framebuffer height %d, should be >= %d
> && <= %d\n",
> > -			  r->height, config->min_height,
> config->max_height);
> > +			r->height, config->min_height,
> config->max_fb_width ?
> > +			config->max_fb_height : config->max_height);
> >  		return ERR_PTR(-EINVAL);
> >  	}
> >
> > diff --git a/include/drm/drm_mode_config.h
> b/include/drm/drm_mode_config.h
> > index 3bcbe30..c6394ed 100644
> > --- a/include/drm/drm_mode_config.h
> > +++ b/include/drm/drm_mode_config.h
> > @@ -339,6 +339,8 @@ struct drm_mode_config_funcs {
> >   * @min_height: minimum fb pixel height on this device
> >   * @max_width: maximum fb pixel width on this device
> >   * @max_height: maximum fb pixel height on this device
> > + * @max_fb_width: maximum fb buffer width if differs from max_width
> > + * @max_fb_height: maximum fb buffer height if differs from  
> > max_height
> >   * @funcs: core driver provided mode setting functions
> >   * @fb_base: base address of the framebuffer
> >   * @poll_enabled: track polling support for this device
> > @@ -523,6 +525,7 @@ struct drm_mode_config {
> >
> >  	int min_width, min_height;
> >  	int max_width, max_height;
> > +	int max_fb_width, max_fb_height;
> >  	const struct drm_mode_config_funcs *funcs;
> >  	resource_size_t fb_base;
> >
> > --
> > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
> Forum,
> > a Linux Foundation Collaborative Project
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Ville SyrjÃlÃ
> Intel

--
Jeykumar S