Re: [RFC] drm: Do not call drm_probe_ddc() when connector force isn't specified

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Jun 06, 2019 at 02:56:53PM +0300, Jani Nikula wrote:
> On Thu, 06 Jun 2019, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Thu, Jun 6, 2019 at 9:38 AM Harish Chegondi
> > <harish.chegondi@intel.com> wrote:
> >>
> >> This would allow the EDID override to be handled correctly in
> >> drm_do_get_edid() for cases where EDID data is missing or corrupt.
> >>
> >> All drm_probe_ddc() does is call drm_do_probe_ddc_edid( , , , 1)
> >> which probes the display by reading 1 byte of EDID data via I2C.
> >> This patch removes the call to drm_probe_ddc() from drm_get_edid()
> >> but drm_get_edid() calls drm_do_get_edid() which first handles
> >> the EDID override case and then calls
> >> drm_do_probe_ddc_edid( , , ,EDID_LENGTH) via function pointer
> >> argument get_edid_block. So, the display device is still being
> >> probed by reading EDID_LENGTH bytes of EDID data via I2C.
> >>
> >> Cc: Jani Nikula <jani.nikula@intel.com>
> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> Signed-off-by: Harish Chegondi <harish.chegondi@intel.com>
> >> References: https://bugs.freedesktop.org/show_bug.cgi?id=107583
> >
> > Since it's a regression we need to annotate this correctly, for the
> > next version please include:
> >
> > Fixes: 53fd40a90f3c ("drm: handle override and firmware EDID at
> > drm_do_get_edid() level")
> > Cc: <stable@vger.kernel.org> # v4.15+
> >
> > So there's a pile more drm_probe_ddc calls all around in drivers, but
> > I reviewed them all, and they're all in ->detect callbacks. So not
> > affecting the regression we're discussing here. Looking at
> > drm_do_get_edid this should also not result in more failures. The only
> > thing this changes is that drm_do_get_edid will retry a bunch more
> > times if nothing is connected (4 times, instead of just the one probe
> > that drm_probe_ddc does). I guess we can restore that if anyone cares,
> > should at least mention it in the commit message.
> >
> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> Like I explained in my reply, this essentially makes override/firmware
> EDID a connector force for the case where hotplug detect isn't used or
> reliable. That's a regression for another set of people...

So revert to the old thing if we can't figure this out? I kinda figured it
probably doesn't matter much, edid override people usually want that edid,
and don't expect it to not work.

Another option would be have the override edid as a fallback if
->get_modes returns nothing and we do have an edid present. That should be
closer to exactly matching the old semantics. But again, this here is
probably good enough.
-Daniel

> 
> BR,
> Jani.
> 
> 
> >
> >
> >> ---
> >>  drivers/gpu/drm/drm_edid.c | 3 ---
> >>  1 file changed, 3 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> >> index d87f574feeca..41c420706532 100644
> >> --- a/drivers/gpu/drm/drm_edid.c
> >> +++ b/drivers/gpu/drm/drm_edid.c
> >> @@ -1724,9 +1724,6 @@ struct edid *drm_get_edid(struct drm_connector *connector,
> >>         if (connector->force == DRM_FORCE_OFF)
> >>                 return NULL;
> >>
> >> -       if (connector->force == DRM_FORCE_UNSPECIFIED && !drm_probe_ddc(adapter))
> >> -               return NULL;
> >
> > Trouble is there's a lot more drm_probe_ddc calls all over, and a lot of these
> >> -
> >>         edid = drm_do_get_edid(connector, drm_do_probe_ddc_edid, adapter);
> >>         if (edid)
> >>                 drm_get_displayid(connector, edid);
> >> --
> >> 2.21.0
> >>
> >> _______________________________________________
> >> Intel-gfx mailing list
> >> Intel-gfx@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx