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