[PATCHv2,03/22] drm/bridge: tc358767: fix ansi 8b10b use

Message ID 20190326103146.24795-4-tomi.valkeinen@ti.com
State Superseded
Headers show
Series
  • drm/bridge: tc358767: DP support
Related show

Commit Message

Tomi Valkeinen March 26, 2019, 10:31 a.m.
DP always uses ANSI 8B10B encoding. Some monitors (old?) may not have
the ANSI 8B10B bit set in DPCD, even if it should always be set.

The tc358767 driver currently respects that flag, and turns the encoding
off if the monitor does not have the bit set, which then results in the
monitor not working.

This patch makes the driver to always use ANSI 8B10B encoding, and drops
the 'coding8b10b' field which is no longer used.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/bridge/tc358767.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

Comments

Andrzej Hajda April 15, 2019, 7:29 a.m. | #1
On 26.03.2019 11:31, Tomi Valkeinen wrote:
> DP always uses ANSI 8B10B encoding. Some monitors (old?) may not have
> the ANSI 8B10B bit set in DPCD, even if it should always be set.
>
> The tc358767 driver currently respects that flag, and turns the encoding
> off if the monitor does not have the bit set, which then results in the
> monitor not working.
>
> This patch makes the driver to always use ANSI 8B10B encoding, and drops
> the 'coding8b10b' field which is no longer used.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>

 --
Regards
Andrzej
Laurent Pinchart April 20, 2019, 9:13 p.m. | #2
Hi Tomi,

Thank you for the patch.

On Tue, Mar 26, 2019 at 12:31:27PM +0200, Tomi Valkeinen wrote:
> DP always uses ANSI 8B10B encoding. Some monitors (old?) may not have
> the ANSI 8B10B bit set in DPCD, even if it should always be set.

Makes you wonder why the bit is present :-) I've checked the DP v1.0
specification, and even though the bit isn't documented as being always
1, 8B/10B encoding is mandatory, so this should be safe from a DP point
of view.

Without access to the TC358767 datasheet I can't tell what use cases
were intended for disabling 8B/10B encoding. Could it be related to
video sources that already provide X3.230-1994 encoded data ? In any
case this shouldn't be driven by the sink but by the source, so

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Andrey, as this feature was present in the initial driver version that
you authored, do you have more information about its intended use cases
?

> The tc358767 driver currently respects that flag, and turns the encoding
> off if the monitor does not have the bit set, which then results in the
> monitor not working.
> 
> This patch makes the driver to always use ANSI 8B10B encoding, and drops
> the 'coding8b10b' field which is no longer used.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/gpu/drm/bridge/tc358767.c | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index 11a50f7bb4be..163c594fa6ac 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -188,7 +188,6 @@ struct tc_edp_link {
>  	u8			assr;
>  	int			scrambler_dis;
>  	int			spread;
> -	int			coding8b10b;
>  	u8			swing;
>  	u8			preemp;
>  };
> @@ -390,13 +389,10 @@ static u32 tc_srcctrl(struct tc_data *tc)
>  	 * No training pattern, skew lane 1 data by two LSCLK cycles with
>  	 * respect to lane 0 data, AutoCorrect Mode = 0
>  	 */
> -	u32 reg = DP0_SRCCTRL_NOTP | DP0_SRCCTRL_LANESKEW;
> +	u32 reg = DP0_SRCCTRL_NOTP | DP0_SRCCTRL_LANESKEW | DP0_SRCCTRL_EN810B;
>  
>  	if (tc->link.scrambler_dis)
>  		reg |= DP0_SRCCTRL_SCRMBLDIS;	/* Scrambler Disabled */
> -	if (tc->link.coding8b10b)
> -		/* Enable 8/10B Encoder (TxData[19:16] not used) */
> -		reg |= DP0_SRCCTRL_EN810B;
>  	if (tc->link.spread)
>  		reg |= DP0_SRCCTRL_SSCG;	/* Spread Spectrum Enable */
>  	if (tc->link.base.num_lanes == 2)
> @@ -635,7 +631,7 @@ static int tc_get_display_props(struct tc_data *tc)
>  	ret = drm_dp_dpcd_readb(&tc->aux, DP_MAIN_LINK_CHANNEL_CODING, tmp);
>  	if (ret < 0)
>  		goto err_dpcd_read;
> -	tc->link.coding8b10b = tmp[0] & BIT(0);
> +
>  	tc->link.scrambler_dis = 0;
>  	/* read assr */
>  	ret = drm_dp_dpcd_readb(&tc->aux, DP_EDP_CONFIGURATION_SET, tmp);
> @@ -649,7 +645,6 @@ static int tc_get_display_props(struct tc_data *tc)
>  		tc->link.base.num_lanes,
>  		(tc->link.base.capabilities & DP_LINK_CAP_ENHANCED_FRAMING) ?
>  		"enhanced" : "non-enhanced");
> -	dev_dbg(tc->dev, "ANSI 8B/10B: %d\n", tc->link.coding8b10b);
>  	dev_dbg(tc->dev, "Display ASSR: %d, TC358767 ASSR: %d\n",
>  		tc->link.assr, tc->assr);
>  
> @@ -951,7 +946,7 @@ static int tc_main_link_setup(struct tc_data *tc)
>  	/* DOWNSPREAD_CTRL */
>  	tmp[0] = tc->link.spread ? DP_SPREAD_AMP_0_5 : 0x00;
>  	/* MAIN_LINK_CHANNEL_CODING_SET */
> -	tmp[1] =  tc->link.coding8b10b ? DP_SET_ANSI_8B10B : 0x00;
> +	tmp[1] =  DP_SET_ANSI_8B10B;
>  	ret = drm_dp_dpcd_write(aux, DP_DOWNSPREAD_CTRL, tmp, 2);
>  	if (ret < 0)
>  		goto err_dpcd_write;
Andrey Gusakov April 23, 2019, 8:19 a.m. | #3
Hi Laurent,

On Sun, Apr 21, 2019 at 12:14 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Tomi,
>
> Thank you for the patch.
>
> On Tue, Mar 26, 2019 at 12:31:27PM +0200, Tomi Valkeinen wrote:
> > DP always uses ANSI 8B10B encoding. Some monitors (old?) may not have
> > the ANSI 8B10B bit set in DPCD, even if it should always be set.
>
> Makes you wonder why the bit is present :-) I've checked the DP v1.0
> specification, and even though the bit isn't documented as being always
> 1, 8B/10B encoding is mandatory, so this should be safe from a DP point
> of view.
>
> Without access to the TC358767 datasheet I can't tell what use cases
> were intended for disabling 8B/10B encoding. Could it be related to
> video sources that already provide X3.230-1994 encoded data ? In any
> case this shouldn't be driven by the sink but by the source, so
Datasheet only describes this bit in register, without any additional
information how it should be handled.

>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> Andrey, as this feature was present in the initial driver version that
> you authored, do you have more information about its intended use cases
> ?
During initial driver development I had one eDP display that reports 0 in Bit 0
(ANSI 8B/10B) of DPCD reg 0x0006 (MAIN_LINK_CHANNEL_CODING).
Also it does not react on setting Bit 0 (SET_ANSI 8B10B) in 0x0108
(MAIN_LINK_CHANNEL_CODING_SET) - after reading back it was 0 again.
So I had to disable 8B10 encoding on tc358767 side to make this display
work.

On other hand if there are displays that report zero bit 0 in
MAIN_LINK_CHANNEL_CODING while needing 8b10b then this patch looks
reasonable.

May be driver should read back MAIN_LINK_CHANNEL_CODING_SET
register after setting it and check if 8b10b actually enabled?

Andrey.

>
> > The tc358767 driver currently respects that flag, and turns the encoding
> > off if the monitor does not have the bit set, which then results in the
> > monitor not working.
> >
> > This patch makes the driver to always use ANSI 8B10B encoding, and drops
> > the 'coding8b10b' field which is no longer used.
> >
> > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> > ---
> >  drivers/gpu/drm/bridge/tc358767.c | 11 +++--------
> >  1 file changed, 3 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> > index 11a50f7bb4be..163c594fa6ac 100644
> > --- a/drivers/gpu/drm/bridge/tc358767.c
> > +++ b/drivers/gpu/drm/bridge/tc358767.c
> > @@ -188,7 +188,6 @@ struct tc_edp_link {
> >       u8                      assr;
> >       int                     scrambler_dis;
> >       int                     spread;
> > -     int                     coding8b10b;
> >       u8                      swing;
> >       u8                      preemp;
> >  };
> > @@ -390,13 +389,10 @@ static u32 tc_srcctrl(struct tc_data *tc)
> >        * No training pattern, skew lane 1 data by two LSCLK cycles with
> >        * respect to lane 0 data, AutoCorrect Mode = 0
> >        */
> > -     u32 reg = DP0_SRCCTRL_NOTP | DP0_SRCCTRL_LANESKEW;
> > +     u32 reg = DP0_SRCCTRL_NOTP | DP0_SRCCTRL_LANESKEW | DP0_SRCCTRL_EN810B;
> >
> >       if (tc->link.scrambler_dis)
> >               reg |= DP0_SRCCTRL_SCRMBLDIS;   /* Scrambler Disabled */
> > -     if (tc->link.coding8b10b)
> > -             /* Enable 8/10B Encoder (TxData[19:16] not used) */
> > -             reg |= DP0_SRCCTRL_EN810B;
> >       if (tc->link.spread)
> >               reg |= DP0_SRCCTRL_SSCG;        /* Spread Spectrum Enable */
> >       if (tc->link.base.num_lanes == 2)
> > @@ -635,7 +631,7 @@ static int tc_get_display_props(struct tc_data *tc)
> >       ret = drm_dp_dpcd_readb(&tc->aux, DP_MAIN_LINK_CHANNEL_CODING, tmp);
> >       if (ret < 0)
> >               goto err_dpcd_read;
> > -     tc->link.coding8b10b = tmp[0] & BIT(0);
> > +
> >       tc->link.scrambler_dis = 0;
> >       /* read assr */
> >       ret = drm_dp_dpcd_readb(&tc->aux, DP_EDP_CONFIGURATION_SET, tmp);
> > @@ -649,7 +645,6 @@ static int tc_get_display_props(struct tc_data *tc)
> >               tc->link.base.num_lanes,
> >               (tc->link.base.capabilities & DP_LINK_CAP_ENHANCED_FRAMING) ?
> >               "enhanced" : "non-enhanced");
> > -     dev_dbg(tc->dev, "ANSI 8B/10B: %d\n", tc->link.coding8b10b);
> >       dev_dbg(tc->dev, "Display ASSR: %d, TC358767 ASSR: %d\n",
> >               tc->link.assr, tc->assr);
> >
> > @@ -951,7 +946,7 @@ static int tc_main_link_setup(struct tc_data *tc)
> >       /* DOWNSPREAD_CTRL */
> >       tmp[0] = tc->link.spread ? DP_SPREAD_AMP_0_5 : 0x00;
> >       /* MAIN_LINK_CHANNEL_CODING_SET */
> > -     tmp[1] =  tc->link.coding8b10b ? DP_SET_ANSI_8B10B : 0x00;
> > +     tmp[1] =  DP_SET_ANSI_8B10B;
> >       ret = drm_dp_dpcd_write(aux, DP_DOWNSPREAD_CTRL, tmp, 2);
> >       if (ret < 0)
> >               goto err_dpcd_write;
>
> --
> Regards,
>
> Laurent Pinchart
Laurent Pinchart April 23, 2019, 2:56 p.m. | #4
Hi Andrey,

On Tue, Apr 23, 2019 at 11:19:17AM +0300, Andrey Gusakov wrote:
> On Sun, Apr 21, 2019 at 12:14 AM Laurent Pinchart wrote:
> > On Tue, Mar 26, 2019 at 12:31:27PM +0200, Tomi Valkeinen wrote:
> >> DP always uses ANSI 8B10B encoding. Some monitors (old?) may not have
> >> the ANSI 8B10B bit set in DPCD, even if it should always be set.
> >
> > Makes you wonder why the bit is present :-) I've checked the DP v1.0
> > specification, and even though the bit isn't documented as being always
> > 1, 8B/10B encoding is mandatory, so this should be safe from a DP point
> > of view.
> >
> > Without access to the TC358767 datasheet I can't tell what use cases
> > were intended for disabling 8B/10B encoding. Could it be related to
> > video sources that already provide X3.230-1994 encoded data ? In any
> > case this shouldn't be driven by the sink but by the source, so
> 
> Datasheet only describes this bit in register, without any additional
> information how it should be handled.
> 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >
> > Andrey, as this feature was present in the initial driver version that
> > you authored, do you have more information about its intended use cases
> > ?
> 
> During initial driver development I had one eDP display that reports 0 in Bit 0
> (ANSI 8B/10B) of DPCD reg 0x0006 (MAIN_LINK_CHANNEL_CODING).
> Also it does not react on setting Bit 0 (SET_ANSI 8B10B) in 0x0108
> (MAIN_LINK_CHANNEL_CODING_SET) - after reading back it was 0 again.
> So I had to disable 8B10 encoding on tc358767 side to make this display
> work.

Out of curiosity, how does the eDP display recover the clock without
8B/10B encoding ?

> On other hand if there are displays that report zero bit 0 in
> MAIN_LINK_CHANNEL_CODING while needing 8b10b then this patch looks
> reasonable.
> 
> May be driver should read back MAIN_LINK_CHANNEL_CODING_SET
> register after setting it and check if 8b10b actually enabled?

This sounds like a reasonable thing to try. Tomi, do you still have
accesss to those faulty monitors ?

> >> The tc358767 driver currently respects that flag, and turns the encoding
> >> off if the monitor does not have the bit set, which then results in the
> >> monitor not working.
> >>
> >> This patch makes the driver to always use ANSI 8B10B encoding, and drops
> >> the 'coding8b10b' field which is no longer used.
> >>
> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> >> ---
> >>  drivers/gpu/drm/bridge/tc358767.c | 11 +++--------
> >>  1 file changed, 3 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> >> index 11a50f7bb4be..163c594fa6ac 100644
> >> --- a/drivers/gpu/drm/bridge/tc358767.c
> >> +++ b/drivers/gpu/drm/bridge/tc358767.c
> >> @@ -188,7 +188,6 @@ struct tc_edp_link {
> >>       u8                      assr;
> >>       int                     scrambler_dis;
> >>       int                     spread;
> >> -     int                     coding8b10b;
> >>       u8                      swing;
> >>       u8                      preemp;
> >>  };
> >> @@ -390,13 +389,10 @@ static u32 tc_srcctrl(struct tc_data *tc)
> >>        * No training pattern, skew lane 1 data by two LSCLK cycles with
> >>        * respect to lane 0 data, AutoCorrect Mode = 0
> >>        */
> >> -     u32 reg = DP0_SRCCTRL_NOTP | DP0_SRCCTRL_LANESKEW;
> >> +     u32 reg = DP0_SRCCTRL_NOTP | DP0_SRCCTRL_LANESKEW | DP0_SRCCTRL_EN810B;
> >>
> >>       if (tc->link.scrambler_dis)
> >>               reg |= DP0_SRCCTRL_SCRMBLDIS;   /* Scrambler Disabled */
> >> -     if (tc->link.coding8b10b)
> >> -             /* Enable 8/10B Encoder (TxData[19:16] not used) */
> >> -             reg |= DP0_SRCCTRL_EN810B;
> >>       if (tc->link.spread)
> >>               reg |= DP0_SRCCTRL_SSCG;        /* Spread Spectrum Enable */
> >>       if (tc->link.base.num_lanes == 2)
> >> @@ -635,7 +631,7 @@ static int tc_get_display_props(struct tc_data *tc)
> >>       ret = drm_dp_dpcd_readb(&tc->aux, DP_MAIN_LINK_CHANNEL_CODING, tmp);
> >>       if (ret < 0)
> >>               goto err_dpcd_read;
> >> -     tc->link.coding8b10b = tmp[0] & BIT(0);
> >> +
> >>       tc->link.scrambler_dis = 0;
> >>       /* read assr */
> >>       ret = drm_dp_dpcd_readb(&tc->aux, DP_EDP_CONFIGURATION_SET, tmp);
> >> @@ -649,7 +645,6 @@ static int tc_get_display_props(struct tc_data *tc)
> >>               tc->link.base.num_lanes,
> >>               (tc->link.base.capabilities & DP_LINK_CAP_ENHANCED_FRAMING) ?
> >>               "enhanced" : "non-enhanced");
> >> -     dev_dbg(tc->dev, "ANSI 8B/10B: %d\n", tc->link.coding8b10b);
> >>       dev_dbg(tc->dev, "Display ASSR: %d, TC358767 ASSR: %d\n",
> >>               tc->link.assr, tc->assr);
> >>
> >> @@ -951,7 +946,7 @@ static int tc_main_link_setup(struct tc_data *tc)
> >>       /* DOWNSPREAD_CTRL */
> >>       tmp[0] = tc->link.spread ? DP_SPREAD_AMP_0_5 : 0x00;
> >>       /* MAIN_LINK_CHANNEL_CODING_SET */
> >> -     tmp[1] =  tc->link.coding8b10b ? DP_SET_ANSI_8B10B : 0x00;
> >> +     tmp[1] =  DP_SET_ANSI_8B10B;
> >>       ret = drm_dp_dpcd_write(aux, DP_DOWNSPREAD_CTRL, tmp, 2);
> >>       if (ret < 0)
> >>               goto err_dpcd_write;
Andrey Gusakov April 24, 2019, 1:52 p.m. | #5
Hi Laurent,

On Tue, Apr 23, 2019 at 5:56 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Andrey,
>
> On Tue, Apr 23, 2019 at 11:19:17AM +0300, Andrey Gusakov wrote:
> > On Sun, Apr 21, 2019 at 12:14 AM Laurent Pinchart wrote:
> > > On Tue, Mar 26, 2019 at 12:31:27PM +0200, Tomi Valkeinen wrote:
> > >> DP always uses ANSI 8B10B encoding. Some monitors (old?) may not have
> > >> the ANSI 8B10B bit set in DPCD, even if it should always be set.
> > >
> > > Makes you wonder why the bit is present :-) I've checked the DP v1.0
> > > specification, and even though the bit isn't documented as being always
> > > 1, 8B/10B encoding is mandatory, so this should be safe from a DP point
> > > of view.
> > >
> > > Without access to the TC358767 datasheet I can't tell what use cases
> > > were intended for disabling 8B/10B encoding. Could it be related to
> > > video sources that already provide X3.230-1994 encoded data ? In any
> > > case this shouldn't be driven by the sink but by the source, so
> >
> > Datasheet only describes this bit in register, without any additional
> > information how it should be handled.
> >
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > >
> > > Andrey, as this feature was present in the initial driver version that
> > > you authored, do you have more information about its intended use cases
> > > ?
> >
> > During initial driver development I had one eDP display that reports 0 in Bit 0
> > (ANSI 8B/10B) of DPCD reg 0x0006 (MAIN_LINK_CHANNEL_CODING).
> > Also it does not react on setting Bit 0 (SET_ANSI 8B10B) in 0x0108
> > (MAIN_LINK_CHANNEL_CODING_SET) - after reading back it was 0 again.
> > So I had to disable 8B10 encoding on tc358767 side to make this display
> > work.
>
> Out of curiosity, how does the eDP display recover the clock without
> 8B/10B encoding ?
Sorry, I have no much knowledge how it works on phy level.
I thought additional 2 bit are used for DC balancing only.

Andrey.
>
> > On other hand if there are displays that report zero bit 0 in
> > MAIN_LINK_CHANNEL_CODING while needing 8b10b then this patch looks
> > reasonable.
> >
> > May be driver should read back MAIN_LINK_CHANNEL_CODING_SET
> > register after setting it and check if 8b10b actually enabled?
>
> This sounds like a reasonable thing to try. Tomi, do you still have
> accesss to those faulty monitors ?
>
> > >> The tc358767 driver currently respects that flag, and turns the encoding
> > >> off if the monitor does not have the bit set, which then results in the
> > >> monitor not working.
> > >>
> > >> This patch makes the driver to always use ANSI 8B10B encoding, and drops
> > >> the 'coding8b10b' field which is no longer used.
> > >>
> > >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> > >> ---
> > >>  drivers/gpu/drm/bridge/tc358767.c | 11 +++--------
> > >>  1 file changed, 3 insertions(+), 8 deletions(-)
> > >>
> > >> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> > >> index 11a50f7bb4be..163c594fa6ac 100644
> > >> --- a/drivers/gpu/drm/bridge/tc358767.c
> > >> +++ b/drivers/gpu/drm/bridge/tc358767.c
> > >> @@ -188,7 +188,6 @@ struct tc_edp_link {
> > >>       u8                      assr;
> > >>       int                     scrambler_dis;
> > >>       int                     spread;
> > >> -     int                     coding8b10b;
> > >>       u8                      swing;
> > >>       u8                      preemp;
> > >>  };
> > >> @@ -390,13 +389,10 @@ static u32 tc_srcctrl(struct tc_data *tc)
> > >>        * No training pattern, skew lane 1 data by two LSCLK cycles with
> > >>        * respect to lane 0 data, AutoCorrect Mode = 0
> > >>        */
> > >> -     u32 reg = DP0_SRCCTRL_NOTP | DP0_SRCCTRL_LANESKEW;
> > >> +     u32 reg = DP0_SRCCTRL_NOTP | DP0_SRCCTRL_LANESKEW | DP0_SRCCTRL_EN810B;
> > >>
> > >>       if (tc->link.scrambler_dis)
> > >>               reg |= DP0_SRCCTRL_SCRMBLDIS;   /* Scrambler Disabled */
> > >> -     if (tc->link.coding8b10b)
> > >> -             /* Enable 8/10B Encoder (TxData[19:16] not used) */
> > >> -             reg |= DP0_SRCCTRL_EN810B;
> > >>       if (tc->link.spread)
> > >>               reg |= DP0_SRCCTRL_SSCG;        /* Spread Spectrum Enable */
> > >>       if (tc->link.base.num_lanes == 2)
> > >> @@ -635,7 +631,7 @@ static int tc_get_display_props(struct tc_data *tc)
> > >>       ret = drm_dp_dpcd_readb(&tc->aux, DP_MAIN_LINK_CHANNEL_CODING, tmp);
> > >>       if (ret < 0)
> > >>               goto err_dpcd_read;
> > >> -     tc->link.coding8b10b = tmp[0] & BIT(0);
> > >> +
> > >>       tc->link.scrambler_dis = 0;
> > >>       /* read assr */
> > >>       ret = drm_dp_dpcd_readb(&tc->aux, DP_EDP_CONFIGURATION_SET, tmp);
> > >> @@ -649,7 +645,6 @@ static int tc_get_display_props(struct tc_data *tc)
> > >>               tc->link.base.num_lanes,
> > >>               (tc->link.base.capabilities & DP_LINK_CAP_ENHANCED_FRAMING) ?
> > >>               "enhanced" : "non-enhanced");
> > >> -     dev_dbg(tc->dev, "ANSI 8B/10B: %d\n", tc->link.coding8b10b);
> > >>       dev_dbg(tc->dev, "Display ASSR: %d, TC358767 ASSR: %d\n",
> > >>               tc->link.assr, tc->assr);
> > >>
> > >> @@ -951,7 +946,7 @@ static int tc_main_link_setup(struct tc_data *tc)
> > >>       /* DOWNSPREAD_CTRL */
> > >>       tmp[0] = tc->link.spread ? DP_SPREAD_AMP_0_5 : 0x00;
> > >>       /* MAIN_LINK_CHANNEL_CODING_SET */
> > >> -     tmp[1] =  tc->link.coding8b10b ? DP_SET_ANSI_8B10B : 0x00;
> > >> +     tmp[1] =  DP_SET_ANSI_8B10B;
> > >>       ret = drm_dp_dpcd_write(aux, DP_DOWNSPREAD_CTRL, tmp, 2);
> > >>       if (ret < 0)
> > >>               goto err_dpcd_write;
>
> --
> Regards,
>
> Laurent Pinchart
Tomi Valkeinen May 3, 2019, 11:43 a.m. | #6
On 23/04/2019 17:56, Laurent Pinchart wrote:

>> During initial driver development I had one eDP display that reports 0 in Bit 0
>> (ANSI 8B/10B) of DPCD reg 0x0006 (MAIN_LINK_CHANNEL_CODING).
>> Also it does not react on setting Bit 0 (SET_ANSI 8B10B) in 0x0108
>> (MAIN_LINK_CHANNEL_CODING_SET) - after reading back it was 0 again.
>> So I had to disable 8B10 encoding on tc358767 side to make this display
>> work.
> 
> Out of curiosity, how does the eDP display recover the clock without
> 8B/10B encoding ?
> 
>> On other hand if there are displays that report zero bit 0 in
>> MAIN_LINK_CHANNEL_CODING while needing 8b10b then this patch looks
>> reasonable.
>>
>> May be driver should read back MAIN_LINK_CHANNEL_CODING_SET
>> register after setting it and check if 8b10b actually enabled?
> 
> This sounds like a reasonable thing to try. Tomi, do you still have
> accesss to those faulty monitors ?

On my monitor it does not seem to matter whether I write 0 or 1 to
MAIN_LINK_CHANNEL_CODING_SET, as long as I have 8b10b enabled on
TC358767 side. The writes do go through, and I can read the written bit
back.

So... I guess when we talk about eDP panels, there may be all kinds of
oddities, as they're usually meant to be used with a certain configuration.

But if everyone agrees that 8B10B is a normal, required feature of DP,
and there is an eDP panel that needs 8B10B disabled, maybe that panel
has to be handled separately as a special case? A dts entry to disable
8B10B? Or something. But it does not sound like a good idea for the
driver to try to guess this.

 Tomi
Laurent Pinchart May 3, 2019, 12:48 p.m. | #7
Hi Tomi,

On Fri, May 03, 2019 at 02:43:51PM +0300, Tomi Valkeinen wrote:
> On 23/04/2019 17:56, Laurent Pinchart wrote:
> 
> >> During initial driver development I had one eDP display that reports 0 in Bit 0
> >> (ANSI 8B/10B) of DPCD reg 0x0006 (MAIN_LINK_CHANNEL_CODING).
> >> Also it does not react on setting Bit 0 (SET_ANSI 8B10B) in 0x0108
> >> (MAIN_LINK_CHANNEL_CODING_SET) - after reading back it was 0 again.
> >> So I had to disable 8B10 encoding on tc358767 side to make this display
> >> work.
> > 
> > Out of curiosity, how does the eDP display recover the clock without
> > 8B/10B encoding ?
> > 
> >> On other hand if there are displays that report zero bit 0 in
> >> MAIN_LINK_CHANNEL_CODING while needing 8b10b then this patch looks
> >> reasonable.
> >>
> >> May be driver should read back MAIN_LINK_CHANNEL_CODING_SET
> >> register after setting it and check if 8b10b actually enabled?
> > 
> > This sounds like a reasonable thing to try. Tomi, do you still have
> > accesss to those faulty monitors ?
> 
> On my monitor it does not seem to matter whether I write 0 or 1 to
> MAIN_LINK_CHANNEL_CODING_SET, as long as I have 8b10b enabled on
> TC358767 side. The writes do go through, and I can read the written bit
> back.
> 
> So... I guess when we talk about eDP panels, there may be all kinds of
> oddities, as they're usually meant to be used with a certain configuration.
> 
> But if everyone agrees that 8B10B is a normal, required feature of DP,
> and there is an eDP panel that needs 8B10B disabled, maybe that panel
> has to be handled separately as a special case? A dts entry to disable
> 8B10B? Or something. But it does not sound like a good idea for the
> driver to try to guess this.

As reported by Andrey, there is at least one panel that would break with
this patch. I agree 8b10b should be the default, but if the above
procedure works for all the panels we know about, is there an issue
implementing it ?
Tomi Valkeinen May 3, 2019, 1:17 p.m. | #8
On 03/05/2019 15:48, Laurent Pinchart wrote:
> Hi Tomi,
> 
> On Fri, May 03, 2019 at 02:43:51PM +0300, Tomi Valkeinen wrote:
>> On 23/04/2019 17:56, Laurent Pinchart wrote:
>>
>>>> During initial driver development I had one eDP display that reports 0 in Bit 0
>>>> (ANSI 8B/10B) of DPCD reg 0x0006 (MAIN_LINK_CHANNEL_CODING).
>>>> Also it does not react on setting Bit 0 (SET_ANSI 8B10B) in 0x0108
>>>> (MAIN_LINK_CHANNEL_CODING_SET) - after reading back it was 0 again.
>>>> So I had to disable 8B10 encoding on tc358767 side to make this display
>>>> work.
>>>
>>> Out of curiosity, how does the eDP display recover the clock without
>>> 8B/10B encoding ?
>>>
>>>> On other hand if there are displays that report zero bit 0 in
>>>> MAIN_LINK_CHANNEL_CODING while needing 8b10b then this patch looks
>>>> reasonable.
>>>>
>>>> May be driver should read back MAIN_LINK_CHANNEL_CODING_SET
>>>> register after setting it and check if 8b10b actually enabled?
>>>
>>> This sounds like a reasonable thing to try. Tomi, do you still have
>>> accesss to those faulty monitors ?
>>
>> On my monitor it does not seem to matter whether I write 0 or 1 to
>> MAIN_LINK_CHANNEL_CODING_SET, as long as I have 8b10b enabled on
>> TC358767 side. The writes do go through, and I can read the written bit
>> back.
>>
>> So... I guess when we talk about eDP panels, there may be all kinds of
>> oddities, as they're usually meant to be used with a certain configuration.
>>
>> But if everyone agrees that 8B10B is a normal, required feature of DP,
>> and there is an eDP panel that needs 8B10B disabled, maybe that panel
>> has to be handled separately as a special case? A dts entry to disable
>> 8B10B? Or something. But it does not sound like a good idea for the
>> driver to try to guess this.
> 
> As reported by Andrey, there is at least one panel that would break with
> this patch. I agree 8b10b should be the default, but if the above
> procedure works for all the panels we know about, is there an issue
> implementing it ?

Well, we don't have data for a big set of panels. I'm worried that such
a change, which, in my opinion, makes the driver guess whether to enable
or disable 8b10b, can break other panels or monitors if the guess
doesn't go right. Especially as disabling 8b10b does not sound like a
valid setup "officially".

I agree that if the panel Andrey mentioned is still used, we need to
handle it somehow. But I think explicitly handling such a case is better
than guessing.

And isn't this something that's not really TC358767 specific? If that
panel is used with other bridges, we need to deal with this case with
those bridges too? Or is TC358767 the only bridge that allows disabling
8b10b?

 Tomi
Laurent Pinchart May 3, 2019, 5:11 p.m. | #9
Hi Tomi,

On Fri, May 03, 2019 at 04:17:41PM +0300, Tomi Valkeinen wrote:
> On 03/05/2019 15:48, Laurent Pinchart wrote:
> > On Fri, May 03, 2019 at 02:43:51PM +0300, Tomi Valkeinen wrote:
> >> On 23/04/2019 17:56, Laurent Pinchart wrote:
> >>
> >>>> During initial driver development I had one eDP display that reports 0 in Bit 0
> >>>> (ANSI 8B/10B) of DPCD reg 0x0006 (MAIN_LINK_CHANNEL_CODING).
> >>>> Also it does not react on setting Bit 0 (SET_ANSI 8B10B) in 0x0108
> >>>> (MAIN_LINK_CHANNEL_CODING_SET) - after reading back it was 0 again.
> >>>> So I had to disable 8B10 encoding on tc358767 side to make this display
> >>>> work.
> >>>
> >>> Out of curiosity, how does the eDP display recover the clock without
> >>> 8B/10B encoding ?
> >>>
> >>>> On other hand if there are displays that report zero bit 0 in
> >>>> MAIN_LINK_CHANNEL_CODING while needing 8b10b then this patch looks
> >>>> reasonable.
> >>>>
> >>>> May be driver should read back MAIN_LINK_CHANNEL_CODING_SET
> >>>> register after setting it and check if 8b10b actually enabled?
> >>>
> >>> This sounds like a reasonable thing to try. Tomi, do you still have
> >>> accesss to those faulty monitors ?
> >>
> >> On my monitor it does not seem to matter whether I write 0 or 1 to
> >> MAIN_LINK_CHANNEL_CODING_SET, as long as I have 8b10b enabled on
> >> TC358767 side. The writes do go through, and I can read the written bit
> >> back.
> >>
> >> So... I guess when we talk about eDP panels, there may be all kinds of
> >> oddities, as they're usually meant to be used with a certain configuration.
> >>
> >> But if everyone agrees that 8B10B is a normal, required feature of DP,
> >> and there is an eDP panel that needs 8B10B disabled, maybe that panel
> >> has to be handled separately as a special case? A dts entry to disable
> >> 8B10B? Or something. But it does not sound like a good idea for the
> >> driver to try to guess this.
> > 
> > As reported by Andrey, there is at least one panel that would break with
> > this patch. I agree 8b10b should be the default, but if the above
> > procedure works for all the panels we know about, is there an issue
> > implementing it ?
> 
> Well, we don't have data for a big set of panels. I'm worried that such
> a change, which, in my opinion, makes the driver guess whether to enable
> or disable 8b10b, can break other panels or monitors if the guess
> doesn't go right. Especially as disabling 8b10b does not sound like a
> valid setup "officially".
> 
> I agree that if the panel Andrey mentioned is still used, we need to
> handle it somehow. But I think explicitly handling such a case is better
> than guessing.

The risk may not be worth it, I agree. I would explain this in details
in the commit message though, and add a comment to the code as well, to
ease future debugging.

> And isn't this something that's not really TC358767 specific? If that
> panel is used with other bridges, we need to deal with this case with
> those bridges too? Or is TC358767 the only bridge that allows disabling
> 8b10b?

I don't know about other bridges, but I think it would need to be
handled globally, yes.
Tomi Valkeinen May 6, 2019, 9:58 a.m. | #10
Hi Laurent, Andrey,

On 03/05/2019 20:11, Laurent Pinchart wrote:
>> I agree that if the panel Andrey mentioned is still used, we need to
>> handle it somehow. But I think explicitly handling such a case is better
>> than guessing.
> 
> The risk may not be worth it, I agree. I would explain this in details
> in the commit message though, and add a comment to the code as well, to
> ease future debugging.

Andrey, do you still have the panel that doesn't work with 8b10b? Is it
used in real life (i.e. it was not just a temporary development phase
panel)? What's the model, and is there a public datasheet?

Everywhere I look, I always see 8b10b as mandatory for all DP versions
for the main link. If the panel in question requires 8b10b to be
disabled, I would imagine that mentioned in its datasheet, as it's kind
of a big thing. I would guess that the panel doesn't work with many eDP
sources.

>> And isn't this something that's not really TC358767 specific? If that
>> panel is used with other bridges, we need to deal with this case with
>> those bridges too? Or is TC358767 the only bridge that allows disabling
>> 8b10b?
> 
> I don't know about other bridges, but I think it would need to be
> handled globally, yes.

Ok. This does sound like a relatively big work, adding a new field to
simple panel, or maybe a new DT property to the panels, and making the
bridges work use that data (even if we'd add the support only to
tc358767 for now).

I don't want to break Andrey's panel, but I have to say I'm not very
enthusiastic about this work either =).

The DP 1.0 spec does say that PRBS7 test pattern is not 8b10b encoded. I
understand this meaning that 8b10b is not used for some particular
tests, which would explain why 8b10b can be turned off in tc358767 (and
maybe that also means it can be turned off in all/most other DP sources).

 Tomi
Andrey Smirnov May 21, 2019, 7:21 a.m. | #11
On Mon, May 6, 2019 at 2:59 AM Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>
> Hi Laurent, Andrey,
>
> On 03/05/2019 20:11, Laurent Pinchart wrote:
> >> I agree that if the panel Andrey mentioned is still used, we need to
> >> handle it somehow. But I think explicitly handling such a case is better
> >> than guessing.
> >
> > The risk may not be worth it, I agree. I would explain this in details
> > in the commit message though, and add a comment to the code as well, to
> > ease future debugging.
>
> Andrey, do you still have the panel that doesn't work with 8b10b? Is it
> used in real life (i.e. it was not just a temporary development phase
> panel)? What's the model, and is there a public datasheet?

Note that I am a different Andrey, and I can't speak about the
original panel that caused the issue. However, production units are
shipped with this panel:

https://www.data-modul.com/productfinder/sites/default/files/products/NL192108AC13-02D_specification_12023727.pdf

which seems to do pretty standard DP stuff. In all of my testing of
Tomi's patches, I haven't seen any problems so far (I still have to
test v3 though), so I think we can carefully proceed assuming that
that weird panel was just a fluke.

Thanks,
Andrey Smirnov

Patch

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 11a50f7bb4be..163c594fa6ac 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -188,7 +188,6 @@  struct tc_edp_link {
 	u8			assr;
 	int			scrambler_dis;
 	int			spread;
-	int			coding8b10b;
 	u8			swing;
 	u8			preemp;
 };
@@ -390,13 +389,10 @@  static u32 tc_srcctrl(struct tc_data *tc)
 	 * No training pattern, skew lane 1 data by two LSCLK cycles with
 	 * respect to lane 0 data, AutoCorrect Mode = 0
 	 */
-	u32 reg = DP0_SRCCTRL_NOTP | DP0_SRCCTRL_LANESKEW;
+	u32 reg = DP0_SRCCTRL_NOTP | DP0_SRCCTRL_LANESKEW | DP0_SRCCTRL_EN810B;
 
 	if (tc->link.scrambler_dis)
 		reg |= DP0_SRCCTRL_SCRMBLDIS;	/* Scrambler Disabled */
-	if (tc->link.coding8b10b)
-		/* Enable 8/10B Encoder (TxData[19:16] not used) */
-		reg |= DP0_SRCCTRL_EN810B;
 	if (tc->link.spread)
 		reg |= DP0_SRCCTRL_SSCG;	/* Spread Spectrum Enable */
 	if (tc->link.base.num_lanes == 2)
@@ -635,7 +631,7 @@  static int tc_get_display_props(struct tc_data *tc)
 	ret = drm_dp_dpcd_readb(&tc->aux, DP_MAIN_LINK_CHANNEL_CODING, tmp);
 	if (ret < 0)
 		goto err_dpcd_read;
-	tc->link.coding8b10b = tmp[0] & BIT(0);
+
 	tc->link.scrambler_dis = 0;
 	/* read assr */
 	ret = drm_dp_dpcd_readb(&tc->aux, DP_EDP_CONFIGURATION_SET, tmp);
@@ -649,7 +645,6 @@  static int tc_get_display_props(struct tc_data *tc)
 		tc->link.base.num_lanes,
 		(tc->link.base.capabilities & DP_LINK_CAP_ENHANCED_FRAMING) ?
 		"enhanced" : "non-enhanced");
-	dev_dbg(tc->dev, "ANSI 8B/10B: %d\n", tc->link.coding8b10b);
 	dev_dbg(tc->dev, "Display ASSR: %d, TC358767 ASSR: %d\n",
 		tc->link.assr, tc->assr);
 
@@ -951,7 +946,7 @@  static int tc_main_link_setup(struct tc_data *tc)
 	/* DOWNSPREAD_CTRL */
 	tmp[0] = tc->link.spread ? DP_SPREAD_AMP_0_5 : 0x00;
 	/* MAIN_LINK_CHANNEL_CODING_SET */
-	tmp[1] =  tc->link.coding8b10b ? DP_SET_ANSI_8B10B : 0x00;
+	tmp[1] =  DP_SET_ANSI_8B10B;
 	ret = drm_dp_dpcd_write(aux, DP_DOWNSPREAD_CTRL, tmp, 2);
 	if (ret < 0)
 		goto err_dpcd_write;