Re: [PATCH 1/2] disp: activate dual link TMDS links only when possible

[Ben Skeggs <skeggsb-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

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

On 11/04/2015 10:37 AM, Ilia Mirkin wrote:
> On Tue, Nov 3, 2015 at 7:02 PM, Ben Skeggs <skeggsb-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> On 11/04/2015 08:41 AM, Ilia Mirkin wrote:
>>> From: Hauke Mehrtens <hauke-5/S+JYg5SzeELgA04lAiVw@public.gmane.org>
>>>
>>> Without this patch a pixel clock rate above 165 MHz on a TMDS link is
>>> assumed to be dual link. This is true for DVI, but not for HDMI. HDMI
>>> supports no dual link, but it supports pixel clock rates above 165 MHz.
>>> Only activate Dual Link mode when it is actual possible.
>>>
>>> Signed-off-by: Hauke Mehrtens <hauke-5/S+JYg5SzeELgA04lAiVw@public.gmane.org>
>>> Signed-off-by: Ilia Mirkin <imirkin-FrUbXkNCsVf2fBVCVOL8/A@public.gmane.org>
>>> ---
>>>  drm/nouveau/nv50_display.c           | 8 ++++----
>>>  drm/nouveau/nvkm/engine/disp/gf119.c | 2 +-
>>>  drm/nouveau/nvkm/engine/disp/nv50.c  | 2 +-
>>>  3 files changed, 6 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drm/nouveau/nv50_display.c b/drm/nouveau/nv50_display.c
>>> index c053c50..93bcfdf 100644
>>> --- a/drm/nouveau/nv50_display.c
>>> +++ b/drm/nouveau/nv50_display.c
>>> @@ -1961,10 +1961,10 @@ nv50_sor_mode_set(struct drm_encoder *encoder, struct drm_display_mode *umode,
>>>       switch (nv_encoder->dcb->type) {
>>>       case DCB_OUTPUT_TMDS:
>>>               if (nv_encoder->dcb->sorconf.link & 1) {
>>> -                     if (mode->clock < 165000)
>>> -                             proto = 0x1;
>>> -                     else
>>> -                             proto = 0x5;
>>> +                     proto = 0x1;
>>> +                     if (mode->clock >= 165000 &&
>>> +                         nv_encoder->dcb->duallink_possible)
>>> +                             proto |= 0x4;
>> This is a somewhat flaky condition, given that one could plug a
>> single-link HDMI monitor into a duallink-capable TMDS connector.
>>
>> Still, it's an improvement :)
> 
> Yeah, FWIW I thought of that (for the second patch too). All this
> stuff is pretty fragile. But... what are you gonna do. Is there some
> other way of telling whether we're on HDMI or DVI?
drm_detect_hdmi_monitor() should do the trick :)

> 
>>
>>>               } else {
>>>                       proto = 0x2;
>>>               }
>>> diff --git a/drm/nouveau/nvkm/engine/disp/gf119.c b/drm/nouveau/nvkm/engine/disp/gf119.c
>>> index 186fd3a..8691b68 100644
>>> --- a/drm/nouveau/nvkm/engine/disp/gf119.c
>>> +++ b/drm/nouveau/nvkm/engine/disp/gf119.c
>>> @@ -158,7 +158,7 @@ exec_clkcmp(struct nv50_disp *disp, int head, int id, u32 pclk, u32 *conf)
>>>       switch (outp->info.type) {
>>>       case DCB_OUTPUT_TMDS:
>>>               *conf = (ctrl & 0x00000f00) >> 8;
>>> -             if (pclk >= 165000)
>>> +             if (pclk >= 165000 && outp->info.duallink_possible)
>>>                       *conf |= 0x0100;
>> I think it might be more robust to key this off the SOR protocol, rather
>> than duplicating the condition above.
> 
> You mean disp->sor.lvdsconf? What do I do with that? Or did you have
> something else in mind?
No, not that.  The "proto" field you're setting set "5" in the previous
hunk when dual-link is requested is actually
NV907D_SOR_SET_CONTROL_PROTOCOL_DUAL_TMDS - which is what is parsed here
with (ctrl & 0x00000f00).

So, instead of "if (pclk >= 165000)" here, you should just be able to do
"(*conf == 5)".

I have no idea why the BIOS tables identify the dual-link data with
0x0105 instead of 0x0005, when "5" already indicates DUAL_TMDS - but anyway.

> 
>>
>>>               break;
>>>       case DCB_OUTPUT_LVDS:
>>> diff --git a/drm/nouveau/nvkm/engine/disp/nv50.c b/drm/nouveau/nvkm/engine/disp/nv50.c
>>> index 32e73a9..ceecd0e 100644
>>> --- a/drm/nouveau/nvkm/engine/disp/nv50.c
>>> +++ b/drm/nouveau/nvkm/engine/disp/nv50.c
>>> @@ -391,7 +391,7 @@ exec_clkcmp(struct nv50_disp *disp, int head, int id, u32 pclk, u32 *conf)
>>>               switch (outp->info.type) {
>>>               case DCB_OUTPUT_TMDS:
>>>                       *conf = (ctrl & 0x00000f00) >> 8;
>>> -                     if (pclk >= 165000)
>>> +                     if (pclk >= 165000 && outp->info.duallink_possible)
>>>                               *conf |= 0x0100;
>> Same here.
>>
>>>                       break;
>>>               case DCB_OUTPUT_LVDS:
>>>
>>


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau