Dear Alex,
your reply confuses me:
> On Sat, Oct 29, 2011 at 8:55 AM, Thomas Reim <reimth@xxxxxxxxxxxxxx> 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@xxxxxxxxx:
> >
> > From: Alex Deucher <alexander.deucher@xxxxxxx>
> >
> > 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@xxxxxxx>
> > ---
> > 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:
signature.asc
Description: This is a digitally signed message part
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
