DRM/AST regression (likely 4.14 -> 4.19+), providing EDID manually fails

[Harish Chegondi]

On Tue, Jun 04, 2019 at 11:18:06AM +0300, Jani Nikula wrote:
> On Mon, 03 Jun 2019, Harish Chegondi <harish.chegondi at intel.com> wrote:
> > On Wed, May 29, 2019 at 01:22:30PM +0300, Jani Nikula wrote:
> >> On Wed, 29 May 2019, Ilpo Järvinen <ilpo.jarvinen at cs.helsinki.fi> wrote:
> >> > On Tue, 28 May 2019, Jani Nikula wrote:
> >> >
> >> >> On Mon, 27 May 2019, Ashutosh Dixit <ashutosh.dixit at intel.com> wrote:
> >> >> > On Sun, 26 May 2019 12:50:51 -0700, Ilpo Järvinen wrote:
> >> >> >>
> >> >> >> Hi all,
> >> >> >>
> >> >> >> I've a workstation which has internal VGA that is detected as AST 2400 and
> >> >> >> with it EDID has been always quite flaky (except for some time it worked
> >> >> >> with 4.14 long enough that I thought the problems would be past until the
> >> >> >> problems reappeared also with 4.14). Thus, I've provided manually the EDID
> >> >> >> that I extracted from the monitor using other computer (the monitor itself
> >> >> >> worked just fine on the earlier computer so it is likely fine).
> >> >> >>
> >> >> >> I setup the manual EDID using drm_kms_helper.edid_firmware, however,
> >> >> >> after upgrading to 4.19.45 it stopped working (no "Got external EDID base
> >> >> >> block" appears in dmesg, the text mode is kept in the lower res mode, and
> >> >> >> Xorg logs no longer dumps the EDID info like it did with 4.14). So I guess
> >> >> >> the EDID I provided manually on the command line is not correctly put into
> >> >> >> use with 4.19+ kernels.
> >> >> >>
> >> >> >> The 4.19 dmesg indicated that drm_kms_helper.edid_firmware is deprecated
> >> >> >> so I also tested with drm.edid_firmware it suggested as the replacement
> >> >> >> but with no luck (but I believe also the drm_kms_helper one should have
> >> >> >> worked as it was only "deprecated").
> >> >> >>
> >> >> >> I also tried 5.1.2 but it did not work any better (and with it also tried
> >> >> >> removing all the manual *.edid_firmware from the command line so I still
> >> >> >> need to provide one manually to have it reliable working it seems).
> >> >> >
> >> >> > I believe there is a bug already tracking this, here:
> >> >> >
> >> >> > https://bugs.freedesktop.org/show_bug.cgi?id=107583
> >> >> 
> >> >> Ilpo, does video=VGA-1:e command-line option work around the problem for
> >> >> you?
> >> >
> >> > Yes it does; together with the modeline stuff for Xorg (after reading the 
> >> > referenced bug report I realized I can fix the X side with it). So I now 
> >> > have the desired modes/resolutions in use. Thank you all!
> >> 
> >> Great! It should be enough to just have the replacement firmware EDID
> >> for the modes, as long as you have an EDID that represents the
> >> capabilities of the display. The modelines for Xorg should not be
> >> needed.
> >> 
> >> BR,
> >> Jani.
> >> 
> >
> > Ilpo,
> >
> > I have a patch which I think will fix this problem without needing
> > the video=VGA-1:e command-line option work around. Will you be able to
> > help me with testing it on your system?
> 
> Alas the patch you provided in the referenced bugzilla will regress in
> other ways; specifically losing the ability to use DDC probe for
> detecting displays. This is very old happy day scenario functionality
> that should not be messed with.
> 
> BR,
> Jani.

Hi Jani,

Please allow me to explain the reason behind my patch which I came up
after analyzing the code in drm_get_edid() and drm_do_get_edid().

I see the DDC probe function drm_probe_ddc() only calls
drm_do_probe_ddc_edid(adapter, &out, 0, 1). So, it is probing by reading
1 byte of EDID data via I2C. With my patch which I pasted below for
reference, drm_probe_ddc() is not called but drm_do_get_edid() is called
with drm_do_probe_ddc_edid() passed as a function pointer argument.
drm_do_get_edid() function first handles the EDID override case and then
calls drm_do_probe_ddc_edid(data, edid, 0, EDID_LENGTH) via function
pointer argument get_edid_block. So, even with my patch, isn't the code
probing the display by calling drm_do_probe_ddc_edid(, , , EDID_LENGTH)
which reads EDID_LENGTH bytes of EDID data via I2C? The only difference
is that drm_probe_ddc() probes the display by reading just 1 byte of
EDID data. Please let me know your thoughts on my analysis.

Thank You!
Harish.

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);

> 
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center