Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <34c2e9c8-9e3d-129c-8295-18ff440f1f84@ideasonboard.com> <20221130054551.112944-1-eddy.zhang@rock-chips.com>
In-Reply-To: <20221130054551.112944-1-eddy.zhang@rock-chips.com>
From:   Doug Anderson <dianders@chromium.org>
Date:   Wed, 30 Nov 2022 06:58:06 -0800
Message-ID: <CAD=FV=WRbg=VyNHfteBBuijC-uhx5Oyc2TX40BEQ2SHv6s=puQ@mail.gmail.com>
Subject: Re: [PATCH] drm/bridge: ti-sn65dsi86: Fix output polarity setting bug
To:     Qiqi Zhang <eddy.zhang@rock-chips.com>
Cc:     tomi.valkeinen@ideasonboard.com, Laurent.pinchart@ideasonboard.com,
        airlied@gmail.com, andrzej.hajda@intel.com, daniel@ffwll.ch,
        dri-devel@lists.freedesktop.org, jernej.skrabec@gmail.com,
        jonas@kwiboo.se, linux-kernel@vger.kernel.org,
        neil.armstrong@linaro.org, robert.foss@linaro.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi,

On Tue, Nov 29, 2022 at 9:46 PM Qiqi Zhang <eddy.zhang@rock-chips.com> 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@rock-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@rock-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@chromium.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.
>
> 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