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

[Laurent Pinchart]

Hi Tomi,

On Thursday, 5 April 2018 13:21:30 EEST Tomi Valkeinen wrote:
> On 04/04/18 17:23, Laurent Pinchart wrote:
> >>>> +	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.
> 
> That's true. And we should check them in the plane atomic check (but do
> we?).

We don't, that should be added.

> In that case I don't mind a warn there, but you should still return an
> error if it happens, instead of continuing with bad config.

But this should really not happen if we add a check to the CRTC 
atomic_check() handler. Do you distrust the DRM core that much ? :-)

> >>>> +	/* 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 ?
> 
> Yes, but I don't so. If our planes can support up to, say, 1000. Then we
> plug in a monitor with native width of 1100, which omapdrm would accept
> happily and try to use it by default. But we can't show fbdev or any
> normal setup there, because the planes won't support it. How would we
> manage that?
> 
> While not strictly correct, I think it's fine to reject videomodes which
> can't be shown with a normal full-screen plane.

It could be argued that such modes would still be useful even if planes can't 
be shown full-screen, or that two planes could be used side by side to achieve 
a larger full-screen display than what would be possible with a single plane. 
I'll leave it up to you to decide whether we should support such use cases.

-- 
Regards,

Laurent Pinchart