| Message ID | 20190326103146.24795-4-tomi.valkeinen@ti.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | drm/bridge: tc358767: DP support | expand |
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
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;
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
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;
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
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
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 ?
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
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.
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
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
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;
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(-)