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

[Rob Clark]

On Tue, Feb 14, 2017 at 4:49 PM, Daniel Vetter <daniel@xxxxxxxx> wrote:
> 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).

sorry, I was probably not expressing my idea to John very well on IRC,
but yeah, the idea was for this to only reject modes that are
impossible for all CRTCs (so a bit different than the case that the
atomic_check callbacks would be validating)

and btw, yeah, this is specifically about fixing things for bridges or
situations where the connector is shared across multiple drivers.  It
isn't really something we can solve in-driver.  Maybe driver provided
callbacks to the bridge would do the trick, but that seemed a bit
weird.  The simple idea was to give the bridge a way to figure out
things that were completely unpossible and let the driver figure out
how to make the things that are possible work somehow.

BR,
-R