Re: [PATCH] drm/radeon/kms: remove useless radeon_ddc_dump()

[Thomas Reim <reimth@googlemail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 6693 bytes --]

Dear Alex, 

your reply confuses me:

> On Sat, Oct 29, 2011 at 8:55 AM, Thomas Reim <reimth@googlemail.com> wrote:
> > Dear Alex,
> >
> > but we use DDC probing e. g. to identify connectors with improperly
> > terminated i2c bus. Instead of flooding logs and terminals with EDID dumps,
> > we decided some months ago to use this function during module loading to
> > inform the user once (and only once!), which connector has a monitor
> > connected with valid EDID and which connector has not.
> 
> There's nothing in that function that actually prevents the printing
> of bad EDIDs.  

That's true. 

> All it does is call drm_get_edid() and report whether
> it found an EDID or not.  radeon_dvi_detect() already takes into
> account the requires_extended_probe flag. 

The extended probe flag will prevent the radeon driver from further
useless printing of bad EDIDs every ten seconds:

in radeon_connectors.c:
if (radeon_connector->ddc_bus)
                dret = radeon_ddc_probe(radeon_connector,
                                        radeon_connector->requires_extended_probe);
        if (dret) {
                if (radeon_connector->edid) {
                        kfree(radeon_connector->edid);
                        radeon_connector->edid = NULL;
                }
                radeon_connector->edid = drm_get_edid(&radeon_connector->base, &radeon_connector->ddc_bus->adapter);




But the now to be removed function radeon_ddc_dump() took care during
connector setup that at least one (!) dump was in the logs, accompagnied
by the info, that no monitor is connected, or there is a wrong/buggy
EDID. 

in radeon_display.c:
radeon_print_display_setup(dev);
                list_for_each_entry(drm_connector, &dev->mode_config.connector_list, head)
                        radeon_ddc_dump(drm_connector);


I cannot imagine why this should confuse users. Having retrieved and
configured connectors plus info plus EDID dump in case of problems
logged next to each other was perfect.

> The connectors are probed by
> the fb code for the console as well so it just adds to the module load
> time.  If we want to print what connectors are connected, it should be
> done from the fb code so we only have to do it once.
> 

I briefly checked the code, but couldn't find a promising connector
probing function in the fb code. Did you mean function
drm_fb_helper_probe_connector_modes?


> > Graphic solutions in general have several connectors integrated, and it's
> > sometimes really difficult to identify, which one of the does not work as
> > expected, based on the logs. From above log we see, that a monitor is
> > connected to DVI connector, nothing is connected to the VGA connector, and
> > we have a problem with the HDMI connector. Without this fuinction, nothing
> > helpful from radeon module would be in the logs.
> 
> We should print the connector name in the generic drm error code then
> if we want to print this info at boot time.

Is there a place that is not called every ten seconds?

> 
> >
> > Maybe we can keep this function, but call it only for connectors, for which
> > it works, i. e. not for DP, eDP and DP bridge connectors.
> 
> That's just as bad.  Users won't understand why only certain
> connectors are probed.
> 
> >
> > Best regards
> >
> > Thomas Reim
> >
> > Am Dienstag, den 25.10.2011, 19:24 -0400 schrieb alexdeucher@gmail.com:
> >
> > From: Alex Deucher <alexander.deucher@amd.com>
> >
> > The function didn't work with DP, eDP, or DP bridge
> > connectors and thus confused users as it lead them to
> > believe nothing was connected or the EDID was invalid
> > when in fact is was, just on the aux bus rather an i2c.
> >
> > It should also speed up module loading as it avoids a
> > bunch of extra DDC probing.
> >
> > Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
> > ---
> >  drivers/gpu/drm/radeon/radeon_display.c |   33
> > -------------------------------
> >  1 files changed, 0 insertions(+), 33 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/radeon/radeon_display.c
> > b/drivers/gpu/drm/radeon/radeon_display.c
> > index 6adb3e5..4998736 100644
> > --- a/drivers/gpu/drm/radeon/radeon_display.c
> > +++ b/drivers/gpu/drm/radeon/radeon_display.c
> > @@ -33,8 +33,6 @@
> >  #include "drm_crtc_helper.h"
> >  #include "drm_edid.h"
> >
> > -static int radeon_ddc_dump(struct drm_connector *connector);
> > -
> >  static void avivo_crtc_load_lut(struct drm_crtc *crtc)
> >  {
> >  	struct radeon_crtc *radeon_crtc = to_radeon_crtc(crtc);
> > @@ -669,7 +667,6 @@ static void radeon_print_display_setup(struct drm_device
> > *dev)
> >  static bool radeon_setup_enc_conn(struct drm_device *dev)
> >  {
> >  	struct radeon_device *rdev = dev->dev_private;
> > -	struct drm_connector *drm_connector;
> >  	bool ret = false;
> >
> >  	if (rdev->bios) {
> > @@ -689,8 +686,6 @@ static bool radeon_setup_enc_conn(struct drm_device
> > *dev)
> >  	if (ret) {
> >  		radeon_setup_encoder_clones(dev);
> >  		radeon_print_display_setup(dev);
> > -		list_for_each_entry(drm_connector, &dev->mode_config.connector_list,
> > head)
> > -			radeon_ddc_dump(drm_connector);
> >  	}
> >
> >  	return ret;
> > @@ -743,34 +738,6 @@ int radeon_ddc_get_modes(struct radeon_connector
> > *radeon_connector)
> >  	return 0;
> >  }
> >
> > -static int radeon_ddc_dump(struct drm_connector *connector)
> > -{
> > -	struct edid *edid;
> > -	struct radeon_connector *radeon_connector =
> > to_radeon_connector(connector);
> > -	int ret = 0;
> > -
> > -	/* on hw with routers, select right port */
> > -	if (radeon_connector->router.ddc_valid)
> > -		radeon_router_select_ddc_port(radeon_connector);
> > -
> > -	if (!radeon_connector->ddc_bus)
> > -		return -1;
> > -	edid = drm_get_edid(connector, &radeon_connector->ddc_bus->adapter);
> > -	/* Log EDID retrieval status here. In particular with regard to
> > -	 * connectors with requires_extended_probe flag set, that will prevent
> > -	 * function radeon_dvi_detect() to fetch EDID on this connector,
> > -	 * as long as there is no valid EDID header found */
> > -	if (edid) {
> > -		DRM_INFO("Radeon display connector %s: Found valid EDID",
> > -				drm_get_connector_name(connector));
> > -		kfree(edid);
> > -	} else {
> > -		DRM_INFO("Radeon display connector %s: No monitor connected or invalid
> > EDID",
> > -				drm_get_connector_name(connector));
> > -	}
> > -	return ret;
> > -}
> > -
> >  /* avivo */
> >  static void avivo_get_fb_div(struct radeon_pll *pll,
> >  			     u32 target_clock,
> >
> >


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel