On Mon, Oct 31, 2011 at 11:15 AM, Thomas Reim <reimth@xxxxxxxxxxxxxx> wrote:
> 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:
Yes, the extended probe check will prevent the spewing of probe
failures, so why do we need to fetch all the edids again at load? It
just adds latency.
>
> 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.
Ok, I see what you are saying. I'm not sure how important it is that
we print that out. If we have an improperly terminated i2c line,
you'll either get an edid or garbage and we already cover that with
the extended edid probe check.
>
> 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.
It's confusing because on systems with eDP, DP, or DP bridge chips
(VGA and LVDS bridges) users get messages saying that the connectors
are disconnected or have a bad edid and then they file bugs that the
driver can''t detect their monitor when the monitor is detected just
fine, it's just on aux rather than i2c. It also adds latency to boot
since you now have two rounds of connector probing; once for the
ddc_dump, and once for the fb code. If we only dump information for
certain connectors it is also confusing since users will not see some
of the connectors checked at boot while others will be. We could add
additional logic for the DP cases, but you'd need to drag in most of
the logic from the dp detect functions in order to determine whether
the DP port is in DP or legacy mode and then decide whether or not to
use aux or i2c; all of which just adds more boot latency considering
we already have to do this for the fb setup.
>
>> 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?
yes, via drm_fb_helper_initial_config()
>
>
>> > 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?
You could add logic to the fb probe code.
>
>>
>> >
>> > 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,
>> >
>> >
>
>
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
