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

Tue Nov 1 07:01:43 PDT 2011

On Tue, Nov 1, 2011 at 7:23 AM, Thomas Reim <reimth at googlemail.com> wrote:
> Dear Alex,
>
> I think we've got the point. The users with improperly terminated i2c
> busses suffered a long time from flooded logs and terminals. We solved
> the problem by introducing the extended DDC probe check, which will be
> according to your other patch the default for all implementations. We
> reduced the log entry flood to one entry that shows that the connector
> cannot be used (due to invalid EDID) plus four EDID dumps. Now the patch
> here will also remove this one log entry. And we come to the point,
> where there's no such information at all, as for most other users
> (except of those that still use monitors with wrong EDIDs).
>
> Believe me, we still need to get used to this "normal" driver
> behaviour.

User's will still realize they can't use the monitor since it will
listed as disconnected.  Is there really a lot of value in printing a
garbage EDID dump due to an improperly terminated i2c line?  Now that
you fixed the probe function, it becomes a moot point I think.
Monitors with bad EDID checksums, etc. will still get logged.  I'd
argue that we should still use those too as in most cases the EDID is
still valid, but that is another discussion.

>
> Nevertheless, I checked drm_fb_helper_initial_config() in more detail.
> Whereas drm_get_edid() logs on kernel error level in case of (EDID)
> problems, the to be removed function radeon_ddc_dump() adds information
> on info level, the functions called by drm_fb_helper_initial_config()
> stay on debug level. I switched on drm.debug and checked the logs.
> You're right, most of the required information is there, but requires
> drm.debug to be enabled.
>
> The question that you also asked in your previous mail and that remains
> is, how important is it to inform the users about the connector status
> during driver load?

Most users never look at their kernel log unless there is a problem in
which case they'll file a bug report and we can request debug output.

>
> In the following you find a proposal how the (new) log could look like
> after adding and modifying some logic in the
> drm_fb_helper_initial_config() function:
>
>> [   14.912386] [drm] Radeon Display Connectors
>> [   14.912389] [drm] Connector 0:
>> [   14.912391] [drm]   VGA
>> [   14.912394] [drm]   DDC: 0x7e50 0x7e40 0x7e54 0x7e44 0x7e58 0x7e48 0x7e5c 0x7e4c
>> [   14.912397] [drm]   Encoders:
>> [   14.912398] [drm]     CRT1: INTERNAL_KLDSCP_DAC1
>> [   14.912401] [drm] Connector 1:
>> [   14.912402] [drm]   S-video
>> [   14.912404] [drm]   Encoders:
>> [   14.912405] [drm]     TV1: INTERNAL_KLDSCP_DAC1
>> [   14.912407] [drm] Connector 2:
>> [   14.912409] [drm]   HDMI-A
>> [   14.912410] [drm]   HPD2
>> [   14.912413] [drm]   DDC: 0x7e40 0x7e60 0x7e44 0x7e64 0x7e48 0x7e68 0x7e4c 0x7e6c
>> [   14.912415] [drm]   Encoders:
>> [   14.912417] [drm]     DFP2: INTERNAL_DDI
>> [   14.912419] [drm] Connector 3:
>> [   14.912421] [drm]   DVI-D
>> [   14.912423] [drm]   DDC: 0x7e40 0x7e50 0x7e44 0x7e54 0x7e48 0x7e58 0x7e4c 0x7e5c
>> [   14.912425] [drm]   Encoders:
>> [   14.912427] [drm]     DFP3: INTERNAL_LVTM1
>>
>> ...
> // Information that is already on debug log level changed to kernel info level
>> [   15.088976] [drm] Probing connector VGA-1 ...
>> [   15.100040] [drm] VGA-1 is disconnected
>> [   15.200048] [drm] Probing connector SVIDEO-1 ...
>> [   15.390040] [drm] SVIDEO-1 is disconnected
>> [   15.390048] [drm] Probing connector HDMI-A-1 ...
> // NEW: drm_get_edid output in case of EDID problems:
>> [   15.470734] Raw EDID:
>> [   15.470745] <3>00 00 00 00 00 00 00 00 00 00 00 07 00 00 00 00  ................
>> [   15.470748] <3>00 00 00 00 00 00 00 00 00 00 07 00 00 00 00 00  ................
>> [   15.470751] <3>00 00 00 00 00 00 00 00 00 00 7f 00 00 00 00 00  ................
>> [   15.470754] <3>00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>> [   15.470757] <3>00 00 00 00 1f 00 00 00 00 00 00 00 00 00 00 00  ................
>> [   15.470760] <3>00 00 00 00 00 07 00 00 00 00 00 7f 00 00 00 00  ................
>> [   15.470762] <3>00 00 00 00 00 00 07 00 00 00 00 00 00 00 00 00  ................
>> [   15.470765] <3>00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 01  ................
>> [   15.470767]
>> [   15.779568] Raw EDID:
>> [   15.779578] <3>00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>> [   15.779581] <3>00 00 00 00 00 7f 00 00 00 00 03 00 00 00 00 00  ................
>> [   15.779584] <3>00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>> [   15.779587] <3>00 00 00 00 ff 00 00 00 00 00 00 00 00 00 00 00  ................
>> [   15.779590] <3>00 00 00 00 00 00 00 00 ff 00 00 00 00 00 00 00  ................
>> [   15.779593] <3>00 00 00 00 00 00 00 00 00 01 00 00 00 00 00 00  ................
>> [   15.779596] <3>00 00 00 7f 00 00 00 00 00 03 07 00 00 00 00 00  ................
>> [   15.779598] <3>00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>> [   15.779600]
>> [   16.151817] Raw EDID:
>> [   16.151827] <3>00 00 00 00 00 00 1f 00 00 00 00 00 00 00 00 00  ................
>> [   16.151830] <3>00 00 00 00 00 00 00 00 00 01 00 00 00 00 00 00  ................
>> [   16.151833] <3>00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>> [   16.151836] <3>00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>> [   16.151839] <3>00 00 1f 00 00 00 00 00 00 00 00 00 00 00 0f 00  ................
>> [   16.151842] <3>01 00 00 00 00 00 00 00 01 00 00 00 00 07 00 ff  ................
>> [   16.151844] <3>00 00 00 00 00 00 ff 00 00 00 00 00 00 00 7f 00  ................
>> [   16.151847] <3>00 0f 00 00 00 00 00 00 00 3f 00 00 00 00 00 00  .........?......
>> [   16.151849]
>> [   16.444209] Raw EDID:
>> [   16.444220] <3>00 07 00 00 00 00 00 00 ff 00 00 00 00 ff 00 00  ................
>> [   16.444223] <3>00 00 3f 00 00 00 00 00 00 00 00 00 00 ff 00 00  ..?.............
>> [   16.444226] <3>00 00 00 00 00 00 00 00 00 07 00 00 00 01 0f 00  ................
>> [   16.444229] <3>00 07 00 00 00 00 00 00 00 00 01 07 00 00 00 00  ................
>> [   16.444231] <3>00 00 00 00 00 00 00 00 7f 00 00 00 00 1f 00 00  ................
>> [   16.444234] <3>00 00 03 00 00 00 00 3f 00 03 00 00 00 00 00 00  .......?........
>> [   16.444237] <3>7f 00 00 1f 00 00 00 00 00 00 00 00 0f 07 00 00  ................
>> [   16.444240] <3>00 00 00 00 00 00 00 00 00 00 03 00 00 00 00 00  ................
>> [   16.444248] radeon 0000:01:05.0: HDMI-A-1: EDID block 0 invalid.
> // Existing information on debug level, shifted to info level
>> [   16.694378] [drm] HDMI-A-1 is disconnected
>> [   16.694382] [drm] Probing connector DVI-D-1 ...
>> [   16.750763] [drm] Probed modes for DVI-D-1
> // If drm.debug is enabled, mode information would follow here
>> [   16.750769] [drm:drm_mode_debug_printmodeline], Modeline 17:"1280x1024" 60 108000 1280 1328 1440 1688 1024 1025 1028 1066 0x48 0x5
>> [   16.750776] [drm:drm_mode_debug_printmodeline], Modeline 26:"1280x1024" 75 135000 1280 1296 1440 1688 1024 1025 1028 1066 0x40 0x5
>>
>> ...
>>
>> [   17.570156] [drm] fb mappable at 0xF0040000
>>
>
> Continuing from here we would have nothing in the logs, except for the
> case, that we retrieve a wrong EDID with correct EDID header (0x00 0xFF
> 0xFF 0xFF 0xFF 0xFF 0xFF 0x00).
>

If you want to add better debugging output, that'd be great.  While
you are at it, it would be nice to print which modes are rejected by
the driver's mode_valid functions.

> Would this work also for other users, especially the users with eDP, DP,
> or DP bridge chips?

Yes, it should.

Alex