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

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

[Harish Chegondi]

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

✓ Fi.CI.BAT: success for drm: Do not call drm_probe_ddc() when connector force isn't specified

[Patchwork]

== 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

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

[Jani Nikula]

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

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

[Daniel Vetter]

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

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

[Jani Nikula]

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

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

[Harish Chegondi]

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

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

[Jani Nikula]

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

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

[Daniel Vetter]

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

✗ Fi.CI.IGT: failure for drm: Do not call drm_probe_ddc() when connector force isn't specified

[Patchwork]

== 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