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 <[email protected]>
---
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;
ti_sn65dsi86_write_u16(pdata, SN_CHA_ACTIVE_LINE_LENGTH_LOW_REG,
--
2.25.1
Adding Tomi to the CC list. Tomi, would you be able to test this ?
On Fri, Nov 25, 2022 at 06:45:58PM +0800, Qiqi Zhang 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 <[email protected]>
> ---
> 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;
>
> ti_sn65dsi86_write_u16(pdata, SN_CHA_ACTIVE_LINE_LENGTH_LOW_REG,
--
Regards,
Laurent Pinchart
Hi,
On Fri, Nov 25, 2022 at 2:54 AM Qiqi Zhang <[email protected]> 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 <[email protected]>
> ---
> 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 <[email protected]>
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
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 <[email protected]>
I'm happy to land this patch, but sounds like we're hoping to get
extra testing so I'll hold off for now.
-Doug
On 29/11/2022 03:13, Doug Anderson wrote:
> Hi,
>
> On Fri, Nov 25, 2022 at 2:54 AM Qiqi Zhang <[email protected]> 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 <[email protected]>
>> ---
>> 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 <[email protected]>
>
> 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 <[email protected]>
>
> 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 <[email protected]>
Tomi
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 <[email protected]> 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 <[email protected]>
>>> ---
>>> 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 <[email protected]>
>>
>> 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.
>> 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.
>>
>> 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 <[email protected]>
>>
>> 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 <[email protected]>
-Eddy
Hi,
On Tue, Nov 29, 2022 at 9:46 PM Qiqi Zhang <[email protected]> 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 <[email protected]> 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 <[email protected]>
> >>> ---
> >>> 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 <[email protected]>
> >>
> >> 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