* [RFC] drm: Do not call drm_probe_ddc() when connector force isn't specified @ 2019-06-06 7:38 Harish Chegondi 2019-06-06 8:47 ` ✓ Fi.CI.BAT: success for " Patchwork ` (3 more replies) 0 siblings, 4 replies; 9+ messages in thread From: Harish Chegondi @ 2019-06-06 7:38 UTC (permalink / raw) To: intel-gfx; +Cc: jani.nikula 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); -- 2.21.0 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 9+ messages in thread
* ✓ Fi.CI.BAT: success for drm: Do not call drm_probe_ddc() when connector force isn't specified 2019-06-06 7:38 [RFC] drm: Do not call drm_probe_ddc() when connector force isn't specified Harish Chegondi @ 2019-06-06 8:47 ` Patchwork 2019-06-06 11:22 ` [RFC] " Jani Nikula ` (2 subsequent siblings) 3 siblings, 0 replies; 9+ messages in thread From: Patchwork @ 2019-06-06 8:47 UTC (permalink / raw) To: Harish Chegondi; +Cc: intel-gfx == Series Details == Series: drm: Do not call drm_probe_ddc() when connector force isn't specified URL : https://patchwork.freedesktop.org/series/61702/ State : success == Summary == CI Bug Log - changes from CI_DRM_6203 -> Patchwork_13189 ==================================================== Summary ------- **SUCCESS** No regressions found. External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13189/ Known issues ------------ Here are the changes found in Patchwork_13189 that come from known issues: ### IGT changes ### #### Issues hit #### * igt@gem_basic@bad-close: - fi-icl-u3: [PASS][1] -> [DMESG-WARN][2] ([fdo#107724]) +1 similar issue [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6203/fi-icl-u3/igt@gem_basic@bad-close.html [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13189/fi-icl-u3/igt@gem_basic@bad-close.html * igt@kms_chamelium@dp-crc-fast: - fi-cml-u2: [PASS][3] -> [FAIL][4] ([fdo#110627]) [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6203/fi-cml-u2/igt@kms_chamelium@dp-crc-fast.html [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13189/fi-cml-u2/igt@kms_chamelium@dp-crc-fast.html #### Possible fixes #### * igt@gem_exec_create@basic: - {fi-icl-guc}: [INCOMPLETE][5] ([fdo#107713]) -> [PASS][6] [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6203/fi-icl-guc/igt@gem_exec_create@basic.html [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13189/fi-icl-guc/igt@gem_exec_create@basic.html * igt@gem_flink_basic@bad-open: - fi-icl-u3: [DMESG-WARN][7] ([fdo#107724]) -> [PASS][8] +2 similar issues [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6203/fi-icl-u3/igt@gem_flink_basic@bad-open.html [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13189/fi-icl-u3/igt@gem_flink_basic@bad-open.html {name}: This element is suppressed. This means it is ignored when computing the status of the difference (SUCCESS, WARNING, or FAILURE). [fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713 [fdo#107724]: https://bugs.freedesktop.org/show_bug.cgi?id=107724 [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271 [fdo#110627]: https://bugs.freedesktop.org/show_bug.cgi?id=110627 [fdo#110829]: https://bugs.freedesktop.org/show_bug.cgi?id=110829 Participating hosts (53 -> 46) ------------------------------ Additional (1): fi-apl-guc Missing (8): fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-byt-clapper fi-kbl-7560u fi-icl-dsi fi-bdw-samus Build changes ------------- * Linux: CI_DRM_6203 -> Patchwork_13189 CI_DRM_6203: 04e951ec46d7d5b65cfefa46789cc6610d24bebd @ git://anongit.freedesktop.org/gfx-ci/linux IGT_5042: f354607cee4baef103f209cc1cc13bbab7def386 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools Patchwork_13189: 902a7a203184b7cf9fa9447f0be3a8e04067fdb7 @ git://anongit.freedesktop.org/gfx-ci/linux == Linux commits == 902a7a203184 drm: Do not call drm_probe_ddc() when connector force isn't specified == Logs == For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13189/ _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC] drm: Do not call drm_probe_ddc() when connector force isn't specified 2019-06-06 7:38 [RFC] drm: Do not call drm_probe_ddc() when connector force isn't specified Harish Chegondi 2019-06-06 8:47 ` ✓ Fi.CI.BAT: success for " Patchwork @ 2019-06-06 11:22 ` Jani Nikula 2019-06-06 11:40 ` Daniel Vetter 2019-06-08 13:17 ` ✗ Fi.CI.IGT: failure for " Patchwork 3 siblings, 0 replies; 9+ messages in thread From: Jani Nikula @ 2019-06-06 11:22 UTC (permalink / raw) To: intel-gfx; +Cc: harish.chegondi, dri-devel 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. -- Jani Nikula, Intel Open Source Graphics Center _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC] drm: Do not call drm_probe_ddc() when connector force isn't specified 2019-06-06 7:38 [RFC] drm: Do not call drm_probe_ddc() when connector force isn't specified Harish Chegondi 2019-06-06 8:47 ` ✓ Fi.CI.BAT: success for " Patchwork 2019-06-06 11:22 ` [RFC] " Jani Nikula @ 2019-06-06 11:40 ` Daniel Vetter 2019-06-06 11:56 ` Jani Nikula 2019-06-08 13:17 ` ✗ Fi.CI.IGT: failure for " Patchwork 3 siblings, 1 reply; 9+ messages in thread From: Daniel Vetter @ 2019-06-06 11:40 UTC (permalink / raw) To: Harish Chegondi; +Cc: Jani Nikula, intel-gfx 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 -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC] drm: Do not call drm_probe_ddc() when connector force isn't specified 2019-06-06 11:40 ` Daniel Vetter @ 2019-06-06 11:56 ` Jani Nikula 2019-06-06 21:38 ` Harish Chegondi 2019-06-07 8:23 ` Daniel Vetter 0 siblings, 2 replies; 9+ messages in thread From: Jani Nikula @ 2019-06-06 11:56 UTC (permalink / raw) To: Daniel Vetter, Harish Chegondi; +Cc: 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 -- Jani Nikula, Intel Open Source Graphics Center _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC] drm: Do not call drm_probe_ddc() when connector force isn't specified 2019-06-06 11:56 ` Jani Nikula @ 2019-06-06 21:38 ` Harish Chegondi 2019-06-07 7:37 ` Jani Nikula 2019-06-07 8:23 ` Daniel Vetter 1 sibling, 1 reply; 9+ messages in thread From: Harish Chegondi @ 2019-06-06 21:38 UTC (permalink / raw) To: Jani Nikula; +Cc: 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC] drm: Do not call drm_probe_ddc() when connector force isn't specified 2019-06-06 21:38 ` Harish Chegondi @ 2019-06-07 7:37 ` Jani Nikula 0 siblings, 0 replies; 9+ messages in thread From: Jani Nikula @ 2019-06-07 7:37 UTC (permalink / raw) To: Harish Chegondi; +Cc: intel-gfx 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 -- Jani Nikula, Intel Open Source Graphics Center _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC] drm: Do not call drm_probe_ddc() when connector force isn't specified 2019-06-06 11:56 ` Jani Nikula 2019-06-06 21:38 ` Harish Chegondi @ 2019-06-07 8:23 ` Daniel Vetter 1 sibling, 0 replies; 9+ messages in thread From: Daniel Vetter @ 2019-06-07 8:23 UTC (permalink / raw) To: Jani Nikula; +Cc: 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... 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* ✗ Fi.CI.IGT: failure for drm: Do not call drm_probe_ddc() when connector force isn't specified 2019-06-06 7:38 [RFC] drm: Do not call drm_probe_ddc() when connector force isn't specified Harish Chegondi ` (2 preceding siblings ...) 2019-06-06 11:40 ` Daniel Vetter @ 2019-06-08 13:17 ` Patchwork 3 siblings, 0 replies; 9+ messages in thread From: Patchwork @ 2019-06-08 13:17 UTC (permalink / raw) To: Harish Chegondi; +Cc: intel-gfx == Series Details == Series: drm: Do not call drm_probe_ddc() when connector force isn't specified URL : https://patchwork.freedesktop.org/series/61702/ State : failure == Summary == CI Bug Log - changes from CI_DRM_6203_full -> Patchwork_13189_full ==================================================== Summary ------- **FAILURE** Serious unknown changes coming with Patchwork_13189_full absolutely need to be verified manually. If you think the reported changes have nothing to do with the changes introduced in Patchwork_13189_full, please notify your bug team to allow them to document this new failure mode, which will reduce false positives in CI. Possible new issues ------------------- Here are the unknown changes that may have been introduced in Patchwork_13189_full: ### IGT changes ### #### Possible regressions #### * igt@kms_big_fb@yf-tiled-8bpp-rotate-0: - shard-iclb: NOTRUN -> [SKIP][1] [1]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13189/shard-iclb4/igt@kms_big_fb@yf-tiled-8bpp-rotate-0.html Known issues ------------ Here are the changes found in Patchwork_13189_full that come from known issues: ### IGT changes ### #### Issues hit #### * igt@kms_flip@modeset-vs-vblank-race-interruptible: - shard-glk: [PASS][2] -> [FAIL][3] ([fdo#103060]) [2]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6203/shard-glk6/igt@kms_flip@modeset-vs-vblank-race-interruptible.html [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13189/shard-glk7/igt@kms_flip@modeset-vs-vblank-race-interruptible.html * igt@kms_frontbuffer_tracking@fbc-suspend: - shard-apl: [PASS][4] -> [DMESG-WARN][5] ([fdo#108566]) +2 similar issues [4]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6203/shard-apl3/igt@kms_frontbuffer_tracking@fbc-suspend.html [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13189/shard-apl2/igt@kms_frontbuffer_tracking@fbc-suspend.html * igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-cur-indfb-draw-render: - shard-iclb: [PASS][6] -> [FAIL][7] ([fdo#103167]) +5 similar issues [6]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6203/shard-iclb4/igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-cur-indfb-draw-render.html [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13189/shard-iclb8/igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-cur-indfb-draw-render.html * igt@kms_plane_alpha_blend@pipe-b-coverage-7efc: - shard-skl: [PASS][8] -> [FAIL][9] ([fdo#108145] / [fdo#110403]) [8]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6203/shard-skl3/igt@kms_plane_alpha_blend@pipe-b-coverage-7efc.html [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13189/shard-skl6/igt@kms_plane_alpha_blend@pipe-b-coverage-7efc.html * igt@kms_plane_lowres@pipe-a-tiling-x: - shard-iclb: [PASS][10] -> [FAIL][11] ([fdo#103166]) +1 similar issue [10]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6203/shard-iclb2/igt@kms_plane_lowres@pipe-a-tiling-x.html [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13189/shard-iclb5/igt@kms_plane_lowres@pipe-a-tiling-x.html * igt@kms_psr@psr2_cursor_plane_onoff: - shard-iclb: [PASS][12] -> [SKIP][13] ([fdo#109441]) +3 similar issues [12]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6203/shard-iclb2/igt@kms_psr@psr2_cursor_plane_onoff.html [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13189/shard-iclb3/igt@kms_psr@psr2_cursor_plane_onoff.html * igt@kms_sysfs_edid_timing: - shard-glk: [PASS][14] -> [FAIL][15] ([fdo#100047]) [14]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6203/shard-glk8/igt@kms_sysfs_edid_timing.html [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13189/shard-glk3/igt@kms_sysfs_edid_timing.html #### Possible fixes #### * igt@i915_pm_rpm@sysfs-read: - shard-iclb: [INCOMPLETE][16] ([fdo#107713] / [fdo#108840]) -> [PASS][17] [16]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6203/shard-iclb2/igt@i915_pm_rpm@sysfs-read.html [17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13189/shard-iclb1/igt@i915_pm_rpm@sysfs-read.html * igt@kms_cursor_legacy@pipe-b-forked-bo: - shard-apl: [INCOMPLETE][18] ([fdo#103927]) -> [PASS][19] +1 similar issue [18]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6203/shard-apl2/igt@kms_cursor_legacy@pipe-b-forked-bo.html [19]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13189/shard-apl8/igt@kms_cursor_legacy@pipe-b-forked-bo.html - shard-iclb: [INCOMPLETE][20] ([fdo#107713]) -> [PASS][21] [20]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6203/shard-iclb7/igt@kms_cursor_legacy@pipe-b-forked-bo.html [21]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13189/shard-iclb4/igt@kms_cursor_legacy@pipe-b-forked-bo.html * igt@kms_flip@plain-flip-fb-recreate: - shard-glk: [FAIL][22] ([fdo#100368]) -> [PASS][23] [22]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6203/shard-glk8/igt@kms_flip@plain-flip-fb-recreate.html [23]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13189/shard-glk3/igt@kms_flip@plain-flip-fb-recreate.html * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-pri-indfb-draw-render: - shard-iclb: [FAIL][24] ([fdo#103167]) -> [PASS][25] +3 similar issues [24]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6203/shard-iclb7/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-pri-indfb-draw-render.html [25]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13189/shard-iclb3/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-pri-indfb-draw-render.html * igt@kms_frontbuffer_tracking@psr-suspend: - shard-skl: [INCOMPLETE][26] ([fdo#104108] / [fdo#106978]) -> [PASS][27] [26]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6203/shard-skl10/igt@kms_frontbuffer_tracking@psr-suspend.html [27]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13189/shard-skl5/igt@kms_frontbuffer_tracking@psr-suspend.html * igt@kms_plane@plane-panning-bottom-right-suspend-pipe-b-planes: - shard-skl: [INCOMPLETE][28] ([fdo#104108]) -> [PASS][29] [28]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6203/shard-skl2/igt@kms_plane@plane-panning-bottom-right-suspend-pipe-b-planes.html [29]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13189/shard-skl2/igt@kms_plane@plane-panning-bottom-right-suspend-pipe-b-planes.html - shard-apl: [DMESG-WARN][30] ([fdo#108566]) -> [PASS][31] +2 similar issues [30]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6203/shard-apl3/igt@kms_plane@plane-panning-bottom-right-suspend-pipe-b-planes.html [31]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13189/shard-apl2/igt@kms_plane@plane-panning-bottom-right-suspend-pipe-b-planes.html * igt@kms_psr2_su@frontbuffer: - shard-iclb: [SKIP][32] ([fdo#109642]) -> [PASS][33] [32]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6203/shard-iclb7/igt@kms_psr2_su@frontbuffer.html [33]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13189/shard-iclb2/igt@kms_psr2_su@frontbuffer.html * igt@kms_psr@psr2_dpms: - shard-iclb: [SKIP][34] ([fdo#109441]) -> [PASS][35] [34]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6203/shard-iclb4/igt@kms_psr@psr2_dpms.html [35]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13189/shard-iclb2/igt@kms_psr@psr2_dpms.html #### Warnings #### * igt@gem_mmap_gtt@forked-big-copy-odd: - shard-iclb: [INCOMPLETE][36] ([fdo#107713] / [fdo#109100]) -> [TIMEOUT][37] ([fdo#109673]) [36]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6203/shard-iclb3/igt@gem_mmap_gtt@forked-big-copy-odd.html [37]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13189/shard-iclb4/igt@gem_mmap_gtt@forked-big-copy-odd.html * igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-spr-indfb-onoff: - shard-skl: [FAIL][38] ([fdo#108040]) -> [FAIL][39] ([fdo#103167]) [38]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6203/shard-skl6/igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-spr-indfb-onoff.html [39]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13189/shard-skl3/igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-spr-indfb-onoff.html [fdo#100047]: https://bugs.freedesktop.org/show_bug.cgi?id=100047 [fdo#100368]: https://bugs.freedesktop.org/show_bug.cgi?id=100368 [fdo#103060]: https://bugs.freedesktop.org/show_bug.cgi?id=103060 [fdo#103166]: https://bugs.freedesktop.org/show_bug.cgi?id=103166 [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167 [fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927 [fdo#104108]: https://bugs.freedesktop.org/show_bug.cgi?id=104108 [fdo#106978]: https://bugs.freedesktop.org/show_bug.cgi?id=106978 [fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713 [fdo#108040]: https://bugs.freedesktop.org/show_bug.cgi?id=108040 [fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145 [fdo#108566]: https://bugs.freedesktop.org/show_bug.cgi?id=108566 [fdo#108840]: https://bugs.freedesktop.org/show_bug.cgi?id=108840 [fdo#109100]: https://bugs.freedesktop.org/show_bug.cgi?id=109100 [fdo#109441]: https://bugs.freedesktop.org/show_bug.cgi?id=109441 [fdo#109642]: https://bugs.freedesktop.org/show_bug.cgi?id=109642 [fdo#109673]: https://bugs.freedesktop.org/show_bug.cgi?id=109673 [fdo#110403]: https://bugs.freedesktop.org/show_bug.cgi?id=110403 Participating hosts (10 -> 9) ------------------------------ Missing (1): shard-hsw Build changes ------------- * Linux: CI_DRM_6203 -> Patchwork_13189 CI_DRM_6203: 04e951ec46d7d5b65cfefa46789cc6610d24bebd @ git://anongit.freedesktop.org/gfx-ci/linux IGT_5042: f354607cee4baef103f209cc1cc13bbab7def386 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools Patchwork_13189: 902a7a203184b7cf9fa9447f0be3a8e04067fdb7 @ git://anongit.freedesktop.org/gfx-ci/linux piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit == Logs == For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13189/ _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-06-08 13:17 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-06-06 7:38 [RFC] drm: Do not call drm_probe_ddc() when connector force isn't specified Harish Chegondi 2019-06-06 8:47 ` ✓ Fi.CI.BAT: success for " Patchwork 2019-06-06 11:22 ` [RFC] " Jani Nikula 2019-06-06 11:40 ` Daniel Vetter 2019-06-06 11:56 ` Jani Nikula 2019-06-06 21:38 ` Harish Chegondi 2019-06-07 7:37 ` Jani Nikula 2019-06-07 8:23 ` Daniel Vetter 2019-06-08 13:17 ` ✗ Fi.CI.IGT: failure for " Patchwork
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.