| Message ID | 20190606073800.64781-1-harish.chegondi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] drm: Do not call drm_probe_ddc() when connector force isn't specified | expand |
On Thu, 06 Jun 2019, 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 > --- > 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; > - > edid = drm_do_get_edid(connector, drm_do_probe_ddc_edid, adapter); > if (edid) > drm_get_displayid(connector, edid); We've had the drm_probe_ddc() since 61e57a8d72f2 ("drm/edid: Fix secondary block fetch.") in 2010. The commit message does not shed any light on why this was added, though I assume early and fast bail out when the DDC isn't there. In any case, this patch solves one problem, but creates another. It loses the ability to use DDC probe to detect display presence when using firmware/override EDID, i.e. the firmware/override EDID effectively leads to connector forcing. In the referenced bug, the problem is that hotplug detect works, but DDC does not. Using connector forcing to work around the issue leads to losing hotplug detect. IMO the override/firmware EDID should be orthogonal to hotplug detect, regardless of whether the detect is achieved via source hardware detect or DDC. If you need to allow for cases where one or the other hotplug detect methods is bust, the only viable solution is to add another level of connector force to bypass drm_probe_ddc() above, yet rely on hardware hotplug detect. Say, DRM_FORCE_ON_DDC. It does mean the folks affected by the referenced bug would need to work around their experienced regression by adding a connector force, *but* we are talking about folks with broken displays. BR, Jani.
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> > --- > 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
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... 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
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... > > BR, > Jani. Hi Jani, Can you please give more details on which regression this patch may cause. Any specific test setup and IGT test would be helpful. I will re-work my patch to make sure it doesn't cause any regression. The CI BAT report didn't indicate any regressions for this patch. Thank You Harish. > > > > > > > >> --- > >> 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
On Thu, 06 Jun 2019, Harish Chegondi <harish.chegondi@intel.com> wrote: > 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... >> >> BR, >> Jani. > > Hi Jani, > > Can you please give more details on which regression this patch may > cause. Any specific test setup and IGT test would be helpful. > I will re-work my patch to make sure it doesn't cause any regression. > The CI BAT report didn't indicate any regressions for this patch. drm_get_edid() is used all over the place to detect if there's a display. If you remove drm_probe_ddc(), you push down the detection to drm_do_get_edid(). If you use override or firmware EDID (debugfs or drm.edid_firmware parameter respectively) there is no detection in drm_do_get_edid(). It will always return non-NULL due to the override/firmware EDID. Effectively this conflates override/firmware EDID and connector forcing, and loses the ability to detect displays using DDC with override/firmware EDID. Something that has worked for eons. Imagine you have a display with DDC communications working but returning corrupted EDID. You want to provide the EDID via drm.edid_firmware, but you also want detection via DDC to work. Otherwise, you'd tell the rest of the stack you have a display connected even when it's unplugged. Incidentally, that's one aspect of the bug you're trying to fix. There, DDC does not work at all, but hotplug works for detection. I proposed using connector forcing as a reasonable workaround, but unfortunately it also leads to display being considered always connected. IGT won't help you here because you'd need something emulating broken DDC and hotplug. BR, Jani. > > Thank You > Harish. > >> >> >> > >> > >> >> --- >> >> 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
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
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; - edid = drm_do_get_edid(connector, drm_do_probe_ddc_edid, adapter); if (edid) drm_get_displayid(connector, edid);
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 --- drivers/gpu/drm/drm_edid.c | 3 --- 1 file changed, 3 deletions(-)