| Message ID | 20210322030128.2283-11-laurent.pinchart+renesas@ideasonboard.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | drm/bridge: ti-sn65dsi86: Support DisplayPort mode | expand |
Hi, On Sun, Mar 21, 2021 at 8:02 PM Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> wrote: > > Despite the SN65DSI86 being an eDP bridge, on some systems its output is > routed to a DisplayPort connector. Enable DisplayPort mode when the next > component in the display pipeline is not a panel, and disable eDP > features in that case. > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > --- > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 32 ++++++++++++++++++++------- > 1 file changed, 24 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > index e2527d597ccb..f792227142a7 100644 > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > @@ -55,6 +55,7 @@ > #define SN_LN_ASSIGN_REG 0x59 > #define LN_ASSIGN_WIDTH 2 > #define SN_ENH_FRAME_REG 0x5A > +#define ASSR_CONTROL BIT(0) > #define VSTREAM_ENABLE BIT(3) > #define LN_POLRS_OFFSET 4 > #define LN_POLRS_MASK 0xf0 > @@ -86,6 +87,8 @@ > #define SN_DATARATE_CONFIG_REG 0x94 > #define DP_DATARATE_MASK GENMASK(7, 5) > #define DP_DATARATE(x) ((x) << 5) > +#define SN_TRAINING_SETTING_REG 0x95 > +#define SCRAMBLE_DISABLE BIT(4) > #define SN_ML_TX_MODE_REG 0x96 > #define ML_TX_MAIN_LINK_OFF 0 > #define ML_TX_NORMAL_MODE BIT(0) > @@ -723,6 +726,11 @@ static int ti_sn_link_training(struct ti_sn_bridge *pdata, int dp_rate_idx, > regmap_update_bits(pdata->regmap, SN_DATARATE_CONFIG_REG, > DP_DATARATE_MASK, DP_DATARATE(dp_rate_idx)); > > + /* For DisplayPort, use the standard DP scrambler seed. */ > + if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort) > + regmap_update_bits(pdata->regmap, SN_ENH_FRAME_REG, > + ASSR_CONTROL, 0); I don't actually know anything about DP scrambler seeds. However: 1. From reading the docs, this field seems to be documented to be "read only" unless: 1a) The "TEST2" pin is pulled high when you power on the bridge. 1b) You set "ASSR_OVERRIDE" (page select to page 7, write to register 0x16, page select back to page 0). I don't know if TEST2 is being pulled high in your hardware, but at least I can see that 1b) isn't done. So I'm guessing that this line is a no-op? If I had to guess from all the hoops they're making you jump through there's some sort of errata around standard scrambling on this bridge chip. Are you sure it works OK? 2. The docs I see claim that this field is 2 bits big. It seems like it would be nice to honor. Yeah, it's silly because 0x11 and 0x10 are "reserved" so it's really more like a 1-bit field, but still seems like it would be better to set both bits, or at least add a comment explaining why you're not matching the datasheet. 3. Your patch doesn't seem to touch the bit of code in ti_sn_bridge_enable() that says this: /** * The SN65DSI86 only supports ASSR Display Authentication method and * this method is enabled by default. An eDP panel must support this * authentication method. We need to enable this method in the eDP panel * at DisplayPort address 0x0010A prior to link training. */ drm_dp_dpcd_writeb(&pdata->aux, DP_EDP_CONFIGURATION_SET, DP_ALTERNATE_SCRAMBLER_RESET_ENABLE); Won't that be a problem? > + > /* enable DP PLL */ > regmap_write(pdata->regmap, SN_PLL_ENABLE_REG, 1); > > @@ -734,6 +742,11 @@ static int ti_sn_link_training(struct ti_sn_bridge *pdata, int dp_rate_idx, > goto exit; > } > > + /* For DisplayPort, disable scrambling mode. */ > + if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort) > + regmap_update_bits(pdata->regmap, SN_TRAINING_SETTING_REG, > + SCRAMBLE_DISABLE, SCRAMBLE_DISABLE); I'm assuming that this is the important part of your patch? Would be sorta nice to include the "why" in your comment. Why do you want to disable scrambling mode for DP but not for eDP? Maybe you care about compatibility but not EMI if you're hooking up to random DP things? -Doug
Hi Doug, On Wed, Mar 24, 2021 at 03:47:07PM -0700, Doug Anderson wrote: > On Sun, Mar 21, 2021 at 8:02 PM Laurent Pinchart wrote: > > > > Despite the SN65DSI86 being an eDP bridge, on some systems its output is > > routed to a DisplayPort connector. Enable DisplayPort mode when the next > > component in the display pipeline is not a panel, and disable eDP > > features in that case. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > > --- > > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 32 ++++++++++++++++++++------- > > 1 file changed, 24 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > > index e2527d597ccb..f792227142a7 100644 > > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > > @@ -55,6 +55,7 @@ > > #define SN_LN_ASSIGN_REG 0x59 > > #define LN_ASSIGN_WIDTH 2 > > #define SN_ENH_FRAME_REG 0x5A > > +#define ASSR_CONTROL BIT(0) > > #define VSTREAM_ENABLE BIT(3) > > #define LN_POLRS_OFFSET 4 > > #define LN_POLRS_MASK 0xf0 > > @@ -86,6 +87,8 @@ > > #define SN_DATARATE_CONFIG_REG 0x94 > > #define DP_DATARATE_MASK GENMASK(7, 5) > > #define DP_DATARATE(x) ((x) << 5) > > +#define SN_TRAINING_SETTING_REG 0x95 > > +#define SCRAMBLE_DISABLE BIT(4) > > #define SN_ML_TX_MODE_REG 0x96 > > #define ML_TX_MAIN_LINK_OFF 0 > > #define ML_TX_NORMAL_MODE BIT(0) > > @@ -723,6 +726,11 @@ static int ti_sn_link_training(struct ti_sn_bridge *pdata, int dp_rate_idx, > > regmap_update_bits(pdata->regmap, SN_DATARATE_CONFIG_REG, > > DP_DATARATE_MASK, DP_DATARATE(dp_rate_idx)); > > > > + /* For DisplayPort, use the standard DP scrambler seed. */ > > + if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort) > > + regmap_update_bits(pdata->regmap, SN_ENH_FRAME_REG, > > + ASSR_CONTROL, 0); > > I don't actually know anything about DP scrambler seeds. However: > > 1. From reading the docs, this field seems to be documented to be > "read only" unless: > > 1a) The "TEST2" pin is pulled high when you power on the bridge. > 1b) You set "ASSR_OVERRIDE" (page select to page 7, write to register > 0x16, page select back to page 0). > > I don't know if TEST2 is being pulled high in your hardware, but at > least I can see that 1b) isn't done. So I'm guessing that this line is > a no-op? If I had to guess from all the hoops they're making you jump > through there's some sort of errata around standard scrambling on this > bridge chip. Are you sure it works OK? Good question :-) We managed to get the SN65DSI86 to work with an external DP monitor yesterday, so it's possible (some modes don't operate correctly yet, but I assume that to be an issue with the DSI encoder). The TEST2 pin is strapped to ground on the board. According to the DisplayPort specification, eDP and DP use different scrambler seeds to prevent interoperability between an eDP source and a DP sink. I'll check what happens without this change. > 2. The docs I see claim that this field is 2 bits big. It seems like > it would be nice to honor. Yeah, it's silly because 0x11 and 0x10 are > "reserved" so it's really more like a 1-bit field, but still seems > like it would be better to set both bits, or at least add a comment > explaining why you're not matching the datasheet. Sure. > 3. Your patch doesn't seem to touch the bit of code in > ti_sn_bridge_enable() that says this: > > /** > * The SN65DSI86 only supports ASSR Display Authentication method and > * this method is enabled by default. An eDP panel must support this > * authentication method. We need to enable this method in the eDP panel > * at DisplayPort address 0x0010A prior to link training. > */ > drm_dp_dpcd_writeb(&pdata->aux, DP_EDP_CONFIGURATION_SET, > DP_ALTERNATE_SCRAMBLER_RESET_ENABLE); > > Won't that be a problem? I'll have a look. > > + > > /* enable DP PLL */ > > regmap_write(pdata->regmap, SN_PLL_ENABLE_REG, 1); > > > > @@ -734,6 +742,11 @@ static int ti_sn_link_training(struct ti_sn_bridge *pdata, int dp_rate_idx, > > goto exit; > > } > > > > + /* For DisplayPort, disable scrambling mode. */ > > + if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort) > > + regmap_update_bits(pdata->regmap, SN_TRAINING_SETTING_REG, > > + SCRAMBLE_DISABLE, SCRAMBLE_DISABLE); > > I'm assuming that this is the important part of your patch? Would be > sorta nice to include the "why" in your comment. Why do you want to > disable scrambling mode for DP but not for eDP? Maybe you care about > compatibility but not EMI if you're hooking up to random DP things? I'll investigate and include proper documentation in v2 (or drop the change altogether if it's not required).
Hi Doug, Laurent, Quoting Laurent Pinchart (2021-06-23 14:59:00) > Hi Doug, > > On Wed, Mar 24, 2021 at 03:47:07PM -0700, Doug Anderson wrote: > > On Sun, Mar 21, 2021 at 8:02 PM Laurent Pinchart wrote: > > > > > > Despite the SN65DSI86 being an eDP bridge, on some systems its output is > > > routed to a DisplayPort connector. Enable DisplayPort mode when the next > > > component in the display pipeline is not a panel, and disable eDP > > > features in that case. > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > > > --- > > > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 32 ++++++++++++++++++++------- > > > 1 file changed, 24 insertions(+), 8 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > > > index e2527d597ccb..f792227142a7 100644 > > > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > > > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > > > @@ -55,6 +55,7 @@ > > > #define SN_LN_ASSIGN_REG 0x59 > > > #define LN_ASSIGN_WIDTH 2 > > > #define SN_ENH_FRAME_REG 0x5A > > > +#define ASSR_CONTROL BIT(0) > > > #define VSTREAM_ENABLE BIT(3) > > > #define LN_POLRS_OFFSET 4 > > > #define LN_POLRS_MASK 0xf0 > > > @@ -86,6 +87,8 @@ > > > #define SN_DATARATE_CONFIG_REG 0x94 > > > #define DP_DATARATE_MASK GENMASK(7, 5) > > > #define DP_DATARATE(x) ((x) << 5) > > > +#define SN_TRAINING_SETTING_REG 0x95 > > > +#define SCRAMBLE_DISABLE BIT(4) > > > #define SN_ML_TX_MODE_REG 0x96 > > > #define ML_TX_MAIN_LINK_OFF 0 > > > #define ML_TX_NORMAL_MODE BIT(0) > > > @@ -723,6 +726,11 @@ static int ti_sn_link_training(struct ti_sn_bridge *pdata, int dp_rate_idx, > > > regmap_update_bits(pdata->regmap, SN_DATARATE_CONFIG_REG, > > > DP_DATARATE_MASK, DP_DATARATE(dp_rate_idx)); > > > > > > + /* For DisplayPort, use the standard DP scrambler seed. */ > > > + if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort) > > > + regmap_update_bits(pdata->regmap, SN_ENH_FRAME_REG, > > > + ASSR_CONTROL, 0); > > > > I don't actually know anything about DP scrambler seeds. However: > > > > 1. From reading the docs, this field seems to be documented to be > > "read only" unless: > > > > 1a) The "TEST2" pin is pulled high when you power on the bridge. > > 1b) You set "ASSR_OVERRIDE" (page select to page 7, write to register > > 0x16, page select back to page 0). > > > > I don't know if TEST2 is being pulled high in your hardware, but at > > least I can see that 1b) isn't done. So I'm guessing that this line is > > a no-op? If I had to guess from all the hoops they're making you jump > > through there's some sort of errata around standard scrambling on this > > bridge chip. Are you sure it works OK? > > Good question :-) We managed to get the SN65DSI86 to work with an > external DP monitor yesterday, so it's possible (some modes don't > operate correctly yet, but I assume that to be an issue with the DSI > encoder). > > The TEST2 pin is strapped to ground on the board. > > According to the DisplayPort specification, eDP and DP use different > scrambler seeds to prevent interoperability between an eDP source and a > DP sink. I'll check what happens without this change. Without this change, the display still works... > > > 2. The docs I see claim that this field is 2 bits big. It seems like > > it would be nice to honor. Yeah, it's silly because 0x11 and 0x10 are > > "reserved" so it's really more like a 1-bit field, but still seems > > like it would be better to set both bits, or at least add a comment > > explaining why you're not matching the datasheet. > > Sure. > > > 3. Your patch doesn't seem to touch the bit of code in > > ti_sn_bridge_enable() that says this: > > > > /** > > * The SN65DSI86 only supports ASSR Display Authentication method and > > * this method is enabled by default. An eDP panel must support this > > * authentication method. We need to enable this method in the eDP panel > > * at DisplayPort address 0x0010A prior to link training. > > */ > > drm_dp_dpcd_writeb(&pdata->aux, DP_EDP_CONFIGURATION_SET, > > DP_ALTERNATE_SCRAMBLER_RESET_ENABLE); > > > > Won't that be a problem? > > I'll have a look. I'm not sure I yet fully understand the requirements here, but could it be that only supporting ASSR is why the scrambling is disabled below? Commenting out that write does not affect the bring up of my DP monitor. > > > > + > > > /* enable DP PLL */ > > > regmap_write(pdata->regmap, SN_PLL_ENABLE_REG, 1); > > > > > > @@ -734,6 +742,11 @@ static int ti_sn_link_training(struct ti_sn_bridge *pdata, int dp_rate_idx, > > > goto exit; > > > } > > > > > > + /* For DisplayPort, disable scrambling mode. */ > > > + if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort) > > > + regmap_update_bits(pdata->regmap, SN_TRAINING_SETTING_REG, > > > + SCRAMBLE_DISABLE, SCRAMBLE_DISABLE); > > > > I'm assuming that this is the important part of your patch? Would be > > sorta nice to include the "why" in your comment. Why do you want to > > disable scrambling mode for DP but not for eDP? Maybe you care about > > compatibility but not EMI if you're hooking up to random DP things? > > I'll investigate and include proper documentation in v2 (or drop the > change altogether if it's not required). And indeed, this part is important. If I drop this hunk - then I get no display output. I'm afraid I don't (yet) know the reasons 'why' to extend the comment, beyond "Scrambling is not supported for DP". If anyone already does, please feel free to provide the text, and I'll include it in the next revision, or I'll try to do some more digging into this part. -- Kieran > > -- > Regards, > > Laurent Pinchart
Hi, On Wed, Feb 23, 2022 at 10:05 AM Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > > > > > + /* For DisplayPort, disable scrambling mode. */ > > > > + if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort) > > > > + regmap_update_bits(pdata->regmap, SN_TRAINING_SETTING_REG, > > > > + SCRAMBLE_DISABLE, SCRAMBLE_DISABLE); > > > > > > I'm assuming that this is the important part of your patch? Would be > > > sorta nice to include the "why" in your comment. Why do you want to > > > disable scrambling mode for DP but not for eDP? Maybe you care about > > > compatibility but not EMI if you're hooking up to random DP things? > > > > I'll investigate and include proper documentation in v2 (or drop the > > change altogether if it's not required). > > And indeed, this part is important. If I drop this hunk - then I get no > display output. > > I'm afraid I don't (yet) know the reasons 'why' to extend the comment, > beyond "Scrambling is not supported for DP". > > If anyone already does, please feel free to provide the text, and I'll > include it in the next revision, or I'll try to do some more digging > into this part. I don't know _tons_ about it, but I later learned that the "alternate" scrambler is used for eDP and the normal scrambler is used for DP. I don't have any background about why they are different other than what looks to be intentionally making the two things incompatible. ...so I guess that would make it pretty clear why you can't use the alternate scrambler for DP. I haven't personally done the research to know if you can be officially DP compliant with the scrambler disabled. I also don't know why the ti-sn65dsi86 makes it so difficult to switch to the standard scrambler or if it works at all... ;-) -Doug
On Wed, Feb 23, 2022 at 10:20:18AM -0800, Doug Anderson wrote: > On Wed, Feb 23, 2022 at 10:05 AM Kieran Bingham wrote: > > > > > > > + /* For DisplayPort, disable scrambling mode. */ > > > > > + if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort) > > > > > + regmap_update_bits(pdata->regmap, SN_TRAINING_SETTING_REG, > > > > > + SCRAMBLE_DISABLE, SCRAMBLE_DISABLE); > > > > > > > > I'm assuming that this is the important part of your patch? Would be > > > > sorta nice to include the "why" in your comment. Why do you want to > > > > disable scrambling mode for DP but not for eDP? Maybe you care about > > > > compatibility but not EMI if you're hooking up to random DP things? > > > > > > I'll investigate and include proper documentation in v2 (or drop the > > > change altogether if it's not required). > > > > And indeed, this part is important. If I drop this hunk - then I get no > > display output. > > > > I'm afraid I don't (yet) know the reasons 'why' to extend the comment, > > beyond "Scrambling is not supported for DP". > > > > If anyone already does, please feel free to provide the text, and I'll > > include it in the next revision, or I'll try to do some more digging > > into this part. > > I don't know _tons_ about it, but I later learned that the "alternate" > scrambler is used for eDP and the normal scrambler is used for DP. I > don't have any background about why they are different other than what > looks to be intentionally making the two things incompatible. I think it was done for DRM purpose, to prevent signals meant for a panel to be connected to a device that could capture video from a DP source. > ...so I guess that would make it pretty clear why you can't use the > alternate scrambler for DP. I haven't personally done the research to > know if you can be officially DP compliant with the scrambler > disabled. I also don't know why the ti-sn65dsi86 makes it so difficult > to switch to the standard scrambler or if it works at all... ;-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index e2527d597ccb..f792227142a7 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -55,6 +55,7 @@ #define SN_LN_ASSIGN_REG 0x59 #define LN_ASSIGN_WIDTH 2 #define SN_ENH_FRAME_REG 0x5A +#define ASSR_CONTROL BIT(0) #define VSTREAM_ENABLE BIT(3) #define LN_POLRS_OFFSET 4 #define LN_POLRS_MASK 0xf0 @@ -86,6 +87,8 @@ #define SN_DATARATE_CONFIG_REG 0x94 #define DP_DATARATE_MASK GENMASK(7, 5) #define DP_DATARATE(x) ((x) << 5) +#define SN_TRAINING_SETTING_REG 0x95 +#define SCRAMBLE_DISABLE BIT(4) #define SN_ML_TX_MODE_REG 0x96 #define ML_TX_MAIN_LINK_OFF 0 #define ML_TX_NORMAL_MODE BIT(0) @@ -723,6 +726,11 @@ static int ti_sn_link_training(struct ti_sn_bridge *pdata, int dp_rate_idx, regmap_update_bits(pdata->regmap, SN_DATARATE_CONFIG_REG, DP_DATARATE_MASK, DP_DATARATE(dp_rate_idx)); + /* For DisplayPort, use the standard DP scrambler seed. */ + if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort) + regmap_update_bits(pdata->regmap, SN_ENH_FRAME_REG, + ASSR_CONTROL, 0); + /* enable DP PLL */ regmap_write(pdata->regmap, SN_PLL_ENABLE_REG, 1); @@ -734,6 +742,11 @@ static int ti_sn_link_training(struct ti_sn_bridge *pdata, int dp_rate_idx, goto exit; } + /* For DisplayPort, disable scrambling mode. */ + if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort) + regmap_update_bits(pdata->regmap, SN_TRAINING_SETTING_REG, + SCRAMBLE_DISABLE, SCRAMBLE_DISABLE); + /* * We'll try to link train several times. As part of link training * the bridge chip will write DP_SET_POWER_D0 to DP_SET_POWER. If @@ -1288,18 +1301,20 @@ static int ti_sn_bridge_probe(struct i2c_client *client, pdata->dev = &client->dev; ret = drm_of_find_panel_or_bridge(pdata->dev->of_node, 1, 0, - &pdata->panel, NULL); + &pdata->panel, &pdata->next_bridge); if (ret) { DRM_ERROR("could not find any panel node\n"); return ret; } - pdata->next_bridge = devm_drm_panel_bridge_add(pdata->dev, - pdata->panel); - if (IS_ERR(pdata->next_bridge)) { - DRM_ERROR("failed to create panel bridge\n"); - ret = PTR_ERR(pdata->next_bridge); - return ret; + if (!pdata->next_bridge) { + pdata->next_bridge = devm_drm_panel_bridge_add(pdata->dev, + pdata->panel); + if (IS_ERR(pdata->next_bridge)) { + DRM_ERROR("failed to create panel bridge\n"); + ret = PTR_ERR(pdata->next_bridge); + return ret; + } } dev_set_drvdata(&client->dev, pdata); @@ -1351,7 +1366,8 @@ static int ti_sn_bridge_probe(struct i2c_client *client, pdata->bridge.funcs = &ti_sn_bridge_funcs; pdata->bridge.of_node = client->dev.of_node; pdata->bridge.ops = DRM_BRIDGE_OP_EDID; - pdata->bridge.type = DRM_MODE_CONNECTOR_eDP; + pdata->bridge.type = pdata->panel ? DRM_MODE_CONNECTOR_eDP + : DRM_MODE_CONNECTOR_DisplayPort; drm_bridge_add(&pdata->bridge);
Despite the SN65DSI86 being an eDP bridge, on some systems its output is routed to a DisplayPort connector. Enable DisplayPort mode when the next component in the display pipeline is not a panel, and disable eDP features in that case. Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> --- drivers/gpu/drm/bridge/ti-sn65dsi86.c | 32 ++++++++++++++++++++------- 1 file changed, 24 insertions(+), 8 deletions(-)