[Patch v2 1/6] drm/omap: Add ability to filter out modes which can't be supported

[Laurent Pinchart]

Hi Benoit,

On Wednesday, 4 April 2018 16:15:11 EEST Benoit Parrot wrote:
> Tomi Valkeinen wrote on Wed [2018-Apr-04 14:12:13 +0300]:
> > On 26/03/18 19:21, Benoit Parrot wrote:
> >> Currently available display mode from a connector are filtered out
> >> based only on pixel clock capability. However we also need to filter
> >> out wider mode if we cannot handle them based on available pipeline
> >> capabilities.
> >> 
> >> Signed-off-by: Benoit Parrot <bparrot at ti.com>
> >> ---
> >> 
> >>  drivers/gpu/drm/omapdrm/dss/dispc.c      | 27 +++++++++++++++++++++++++
> >>  drivers/gpu/drm/omapdrm/dss/omapdss.h    |  1 +
> >>  drivers/gpu/drm/omapdrm/omap_connector.c | 10 ++++++++++
> >>  3 files changed, 38 insertions(+)
> >> 
> >> diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c
> >> b/drivers/gpu/drm/omapdrm/dss/dispc.c index 624dee22f46b..35541d4441df
> >> 100644
> >--- a/drivers/gpu/drm/omapdrm/dss/dispc.c
> >> +++ b/drivers/gpu/drm/omapdrm/dss/dispc.c
> >> @@ -100,6 +100,8 @@ struct dispc_features {
> >>  	u8 mgr_height_start;
> >>  	u16 mgr_width_max;
> >>  	u16 mgr_height_max;
> >> +	u16 ovl_width_max;
> >> +	u16 ovl_height_max;
> >>  	unsigned long max_lcd_pclk;
> >>  	unsigned long max_tv_pclk;
> >>  	unsigned int max_downscale;
> >> @@ -2465,6 +2467,12 @@ static int dispc_ovl_calc_scaling(unsigned long
> >> pclk, unsigned long lclk,
> >>  	return 0;
> >>  }
> >> 
> >> +static void dispc_ovl_get_max_size(u16 *width, u16 *height)
> >> +{
> >> +	*width = dispc.feat->ovl_width_max;
> >> +	*height = dispc.feat->ovl_height_max;
> >> +}
> >> +
> >>  static int dispc_ovl_setup_common(enum omap_plane_id plane,
> >>  		enum omap_overlay_caps caps, u32 paddr, u32 p_uv_addr,
> >>  		u16 screen_width, int pos_x, int pos_y, u16 width, u16 height,
> >> @@ -2500,6 +2508,10 @@ static int dispc_ovl_setup_common(enum
> >> omap_plane_id plane,
> >>  	out_width = out_width == 0 ? width : out_width;
> >>  	out_height = out_height == 0 ? height : out_height;
> >> 
> >> +	WARN(out_width > dispc.feat->ovl_width_max,
> >> +	     "Requested OVL width (%d) is larger than can be supported
> >> (%d).\n",
> >> +	     out_width, dispc.feat->ovl_width_max);
> > 
> > Why don't you return an error here? I don't see a need for WARN here.
> 
> So here you mean replace the WARN with something like this:
> 
> 	if (out_width > dispc.feat->ovl_width_max) {
> 		DSSERR("Requested OVL width (%d) is larger than can be supported (%d).
\n",
> out_width, dispc.feat->ovl_width_max);
>                 return -EINVAL;
> 	}

Can this happen ? If we reject invalid settings in omapdrm we should never get 
them here.

> >>  void dispc_set_ops(const struct dispc_ops *o);
> >> 
> >> diff --git a/drivers/gpu/drm/omapdrm/omap_connector.c
> >> b/drivers/gpu/drm/omapdrm/omap_connector.c index
> >> a0d7b1d905e8..d5e059abb555 100644
> >> --- a/drivers/gpu/drm/omapdrm/omap_connector.c
> >> +++ b/drivers/gpu/drm/omapdrm/omap_connector.c
> >> @@ -197,6 +197,16 @@ static int omap_connector_mode_valid(struct
> >> drm_connector *connector,
> >>  			r = 0;
> >>  	}
> >> 
> >> +	/* Check if the advertised width exceed what the pipeline can do */
> >> +	if (!r) {
> >> +		struct omap_drm_private *priv = dev->dev_private;
> >> +		u16 width, height;
> >> +
> >> +		priv->dispc_ops->ovl_get_max_size(&width, &height);
> >> +		if (mode->hdisplay > width)
> >> +			r = -EINVAL;
> > 
> > You should check the height also.
> 
> Yeah, I'll fix that.

Unless I'm mistaken the restriction doesn't come from the output side of the 
display controller but from the overlays (planes), right ? Shouldn't it then 
be implemented in the drm_plane_helper_funcs.atomic_check operation ?

-- 
Regards,

Laurent Pinchart