On Thu, 06 Jun 2019, Harish Chegondi <harish.chegondi@xxxxxxxxx> wrote: > On Thu, Jun 06, 2019 at 02:56:53PM +0300, Jani Nikula wrote: >> On Thu, 06 Jun 2019, Daniel Vetter <daniel@xxxxxxxx> wrote: >> > On Thu, Jun 6, 2019 at 9:38 AM Harish Chegondi >> > <harish.chegondi@xxxxxxxxx> 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@xxxxxxxxx> >> >> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> >> >> Signed-off-by: Harish Chegondi <harish.chegondi@xxxxxxxxx> >> >> 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@xxxxxxxxxxxxxxx> # 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@xxxxxxxx> >> >> 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@xxxxxxxxxxxxxxxxxxxxx >> >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx >> >> -- >> Jani Nikula, Intel Open Source Graphics Center -- Jani Nikula, Intel Open Source Graphics Center _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
