[PATCH] drm: edid: enable probing and listing of non rb modes

[Shirish S]

Hi,

On Sat, Nov 16, 2013 at 12:29 AM, Adam Jackson <ajax at redhat.com> wrote:
> On Fri, 2013-11-15 at 10:38 +0530, Shirish S wrote:
>> The current solution checks for the existing RB mode,
>> if available in the edid block returns by adding it,
>> but does not populate the connector with the modes
>> of same resolution but which are non-rb modes.
>>
>> As a result the probing and listing of non-rb modes can't
>> be made, in case the rb mode's pixel clock is not
>> supported but non-rb mode is supported.
>
> This is... almost okay.
>
> So the EDID 1.4 spec says that for modes in standard descriptors:
>
> a) if there's a match in DMT, use DMT
> b) if there's a range descriptor and it says GTF, use GTF
> c) if there's a range descriptor and it says CVT, use CVT
> d) if there's no range descriptor, use CVT
>
> But case d) is clearly insane if the sink is EDID 1.3, CVT wasn't a
> standard when 1.3 was defined, so the sink may not in fact support CVT
> timings.  Hence the logic in standard_timing_level().
>
> The other thing the spec says is that if you're using CVT for standard
> descriptors that you should use normal blanking instead of reduced.
> This is... problematic.  If we see 1920x1200 at 60 in a standard descriptor
> we almost certainly _don't_ mean normal blanking, because that won't fit
> on single-link DVI.  And if both the source and the sink support reduced
> blanking we should probably prefer it, it's marginally less power
> consumption on digital links and marginally better picture quality on
> analog links.
>
> So probably what we should do instead is:
>
> - if drm_monitor_supports_rb(), add both normal and reduced blanking
> timings for either the DMT or CVT path
> - extend drm_connector to also have rb-allowed to match interlace and
> doublescan
> - fix up all the drivers to indicate rb-allowed (except whatever broken
> thing it is you're trying to work around here)
> - fix drm_mode_validate_flag and callers to filter out rb modes as
> appropriate
>
> Also: CVT dates to 2003.  EDID 1.4 was 2006.  Any hardware being made in
> 2013 that can't generate RB timings is going out of its way to be
> broken.  You should really demand better from your hardware.
>
True, but we have to deal with such hardware also, so with this patch
can we add up to the mode collection logic, to accomodate such hardwares also?
>> 1. adds "rb" suffix to rb modes.
>
> No.  The userspace convention is:
>
> dmt:~% cvt -r 1920 1080
> # 1920x1080 59.93 Hz (CVT 2.07M9-R) hsync: 66.59 kHz; pclk: 138.50 MHz
> Modeline "1920x1080R"  138.50  1920 1968 2000 2080  1080 1083 1088 1111 +hsync -vsync
>
> drm_mode_parse_command_line_for_connector() expects the same thing.
> Let's be consistent.
>
I found that there is a suffix for interlaced modes, which got added
recently, hence thought of adding this,
with the addition of 'rb" suffix my system shows up more modes that is
supports and there is no need to
actually set the particular mode to know if its RB or non-RB.
Without this change it never showed 1920x1080R kind of mode, am i
missing anything?
>> @@ -1621,19 +1621,20 @@ drm_mode_std(struct drm_connector *connector, struct edid *edid,
>>               mode->hdisplay = 1366;
>>               mode->hsync_start = mode->hsync_start - 1;
>>               mode->hsync_end = mode->hsync_end - 1;
>> -             return mode;
>> +             goto done;
>>       }
>>
>>       /* check whether it can be found in default mode table */
>>       if (drm_monitor_supports_rb(edid)) {
>>               mode = drm_mode_find_dmt(dev, hsize, vsize, vrefresh_rate,
>>                                        true);
>> -             if (mode)
>> -                     return mode;
>> +             if (mode) {
>> +                     drm_mode_probed_add(connector, mode);
>> +                     modes++;
>> +             }
>>       }
>>       mode = drm_mode_find_dmt(dev, hsize, vsize, vrefresh_rate, false);
>> -     if (mode)
>> -             return mode;
>> +     goto done;
>>
>>       /* okay, generate it */
>>       switch (timing_level) {
>
> Unconditional "goto done" that skips all the logic for generating modes
> not in the dmt list?  This breaks everything _but_ case a) above, so eg.
> 1600x900 at 60 would no longer be generated.
>
My bad ,missed it, will correct it in the next patch set, if you are
ok with this logic of mine.
Kindly confirm if you want me to send next patch set with updated
changes or what/way i am working on
is not required.
> - ajax
>