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

[Doug Anderson]

Hi,

On Tue, Nov 29, 2022 at 9:46 PM Qiqi Zhang <eddy.zhang@xxxxxxxxxxxxxx> wrote:
>
> Hi,
>
> on Nov. 29, 2022, 11:45 a.m. Tomi wrote:
> >On 29/11/2022 03:13, Doug Anderson wrote:
> >> Hi,
> >>
> >> On Fri, Nov 25, 2022 at 2:54 AM Qiqi Zhang <eddy.zhang@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
> >>> ---
> >>>   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@xxxxxxxxxxxx>
> >>
> >> 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.
>
> Just like Tomi said, the wrong polarity worked fine on my eDP panel(LP079QX1)
> and standard DP monitor, I didn't notice the polarity configuration problem
> here until my customer used the following solution and got a abnormal display:
> GPU->mipi->eDP->DP->lvds->panel.

Wow, that's convoluted, but makes sense. I think this fully explains
why this is a problem for you but wasn't in the past.


> >> 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")
>
> Doug you mean I need to update my commit message? It's my first time using
> kernel list and I'm a little confused about this.

Nah, I'll add it in and land it. OK, pushed to drm-misc-fixes:

8c115864501f drm/bridge: ti-sn65dsi86: Fix output polarity setting bug