linux-kernel - Re: [PATCH] drm/bridge: ti-sn65dsi86: Fix output polarity setting bug

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <34c2e9c8-9e3d-129c-8295-18ff440f1f84@ideasonboard.com>
Date:   Tue, 29 Nov 2022 13:45:58 +0200
From:   Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
To:     Doug Anderson <dianders@...omium.org>,
        Qiqi Zhang <eddy.zhang@...k-chips.com>
Cc:     andrzej.hajda@...el.com, neil.armstrong@...aro.org,
        robert.foss@...aro.org, Laurent.pinchart@...asonboard.com,
        jonas@...boo.se, jernej.skrabec@...il.com, airlied@...il.com,
        daniel@...ll.ch, dri-devel@...ts.freedesktop.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] drm/bridge: ti-sn65dsi86: Fix output polarity setting bug

On 29/11/2022 03:13, Doug Anderson wrote:
> Hi,
> 
> On Fri, Nov 25, 2022 at 2:54 AM Qiqi Zhang <eddy.zhang@...k-chips.com> wrote:
>>
>> According to the description in ti-sn65dsi86's datasheet:
>>
>> CHA_HSYNC_POLARITY:
>> 0 = Active High Pulse. Synchronization signal is high for the sync
>> pulse width. (default)
>> 1 = Active Low Pulse. Synchronization signal is low for the sync
>> pulse width.
>>
>> CHA_VSYNC_POLARITY:
>> 0 = Active High Pulse. Synchronization signal is high for the sync
>> pulse width. (Default)
>> 1 = Active Low Pulse. Synchronization signal is low for the sync
>> pulse width.
>>
>> We should only set these bits when the polarity is negative.
>> Signed-off-by: Qiqi Zhang <eddy.zhang@...k-chips.com>
>> ---
>>   drivers/gpu/drm/bridge/ti-sn65dsi86.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
>> index 3c3561942eb6..eb24322df721 100644
>> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
>> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
>> @@ -931,9 +931,9 @@ static void ti_sn_bridge_set_video_timings(struct ti_sn65dsi86 *pdata)
>>                  &pdata->bridge.encoder->crtc->state->adjusted_mode;
>>          u8 hsync_polarity = 0, vsync_polarity = 0;
>>
>> -       if (mode->flags & DRM_MODE_FLAG_PHSYNC)
>> +       if (mode->flags & DRM_MODE_FLAG_NHSYNC)
>>                  hsync_polarity = CHA_HSYNC_POLARITY;
>> -       if (mode->flags & DRM_MODE_FLAG_PVSYNC)
>> +       if (mode->flags & DRM_MODE_FLAG_NVSYNC)
>>                  vsync_polarity = CHA_VSYNC_POLARITY;
> 
> Looks right to me.
> 
> Reviewed-by: Douglas Anderson <dianders@...omium.org>
> 
> I've never seen the polarity matter for any eDP panels I've worked
> with, which presumably explains why this was wrong for so long. As far

Afaik, DP doesn't have sync polarity as such (neither does DSI), and the 
sync polarity is just "metadata". So if you're in full-DP domain, I 
don't see why it would matter. I guess it becomes relevant when you 
convert from DP to some other bus format.

> as I can tell, it's been wrong since the start. Probably you should
> have:
> 
> Fixes: a095f15c00e2 ("drm/bridge: add support for sn65dsi86 bridge driver")
> 
> I put this on a sc7180-trogdor-lazor device and it didn't make
> anything worse. Since the sync polarity never mattered to begin with,
> I guess this isn't a surprise. ...so I guess that's a weak tested-by:
> 
> Tested-by: Douglas Anderson <dianders@...omium.org>
> 
> I'm happy to land this patch, but sounds like we're hoping to get
> extra testing so I'll hold off for now.

Looks fine to me and works for me with my DP monitor.

Reviewed-by: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>

  Tomi