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

[Jani Nikula <jani.nikula@intel.com>]

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