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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
