Re: [RFC][PATCH 1/2] drm/probe-helper: Add mode_valid check to drm_crtc_helper_funcs

[Daniel Vetter]

On Tue, Feb 14, 2017 at 01:07:21PM -0800, John Stultz wrote:
> On Tue, Feb 14, 2017 at 12:32 PM, Daniel Stone <daniel@xxxxxxxxxxxxx> wrote:
> > Hi John,
> >
> > On 14 February 2017 at 19:25, John Stultz <john.stultz@xxxxxxxxxx> wrote:
> >> +static enum drm_mode_status
> >> +drm_connector_check_crtc_modes(struct drm_connector *connector,
> >> +                              struct drm_display_mode *mode)
> >> +{
> >> +       struct drm_device *dev = connector->dev;
> >> +       const struct drm_crtc_helper_funcs *crtc_funcs;
> >> +       struct drm_crtc *c;
> >> +
> >> +       if (mode->status != MODE_OK)
> >> +               return mode->status;
> >> +
> >> +       /* Check all the crtcs on a connector to make sure the mode is valid */
> >> +       drm_for_each_crtc(c, dev) {
> >> +               crtc_funcs = c->helper_private;
> >> +               if (crtc_funcs && crtc_funcs->mode_valid)
> >> +                       mode->status = crtc_funcs->mode_valid(c, mode);
> >> +               if (mode->status != MODE_OK)
> >> +                       break;
> >> +       }
> >> +       return mode->status;
> >> +}
> >
> > Hm, that's unfortunate: it limits the mode list for every connector,
> > to those which are supported by every single CRTC. So if you have one
> > CRTC serving low-res LVDS, and another serving higher-res HDMI,
> > suddenly you can't get bigger modes on HDMI. The idea seems sound
> > enough, but a little more nuance might be good ...
> 
> Yea. That is not my intent at all I'm just trying to get the drm_crtc
> attached to the connector that we're getting the EDID mode lines from.
> I had tried going connector->encoder->crtc, but at the time this is
> called, the encoder is null. So Rob suggested the for_each_crtc(), and
> I guess I mistook that for being each crtc on the connector.
> 
> Thanks for pointing out this issue. From Daniel's feedback it looks
> like I need to start over from scratch though, so little worry this
> implementation will go much further.

Well your idea was somewhat right, but logic inverted. In ->mode_valid we
need to check whether any encoder/crtc combo could support the mode. Which
means you need to reject it only when there's no encoder/crtc combo that
could support the mode (you reject it if there's only one crtc which can't
handle it).

On the ->mode_fixup/atomic_check callbacks otoh we need to reject the mode
when it's not suitable for the current chain (as described in the atomic
states). That little difference is why this is not an entirely trivial
problem, and yes there's lots of hw out there where this matters.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch