Quoting Doug Anderson (2022-03-07 23:22:00) > Hi, > > On Mon, Mar 7, 2022 at 10:00 AM Kieran Bingham > <kieran.bingham+renesas@xxxxxxxxxxxxxxxx> wrote: > > > > From: Laurent Pinchart <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx> > > > > 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 detected as a DisplayPort > > connector, and disable eDP features in that case. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx> > > Reworked to set bridge type based on the next bridge/connector. > > Signed-off-by: Kieran Bingham <kieran.bingham+renesas@xxxxxxxxxxxxxxxx> > > -- > > Changes since v1/RFC: > > - Rebased on top of "drm/bridge: ti-sn65dsi86: switch to > > devm_drm_of_get_bridge" > > - eDP/DP mode determined from the next bridge connector type. > > > > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 16 +++++++++++++++- > > 1 file changed, 15 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > > index 29f5f7123ed9..461f963faa0b 100644 > > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > > @@ -60,6 +60,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 > > @@ -91,6 +92,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) > > @@ -1005,6 +1008,11 @@ static int ti_sn_link_training(struct ti_sn65dsi86 *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 thought it was agreed that this hunk wasn't doing anything and > should be removed? I did some research previously [1] and the manual > said that this bit is "read only" unless "TEST2" is pulled high. In > the previous discussion Laurent said that it wasn't. I also pointed > out that this conflicts with the bit of code in ti_sn_bridge_enable() > which tells the sink to enable the alternate scrambler. Sorry - I had misremembered indeed, and looking back I confirmed that this hunk was not required. I had incorrectly remembered that the second part was required (below) and assumed that had meant both were. Of course if we're disabling the scrambling mode, then it likely doesn't matter what the seed is!. Looking at the datasheet though, register 0x5a, clearly says the default is 01 (ASSR) which we're not using. So the datasheet implies we want the DP scrambler seed (if it were enabled?) Reading the 0x5a register here shows we have 0x05. Indeed, re-reading after attempting to write '0' to it still shows 0x05. So it's read-only and not relevant regardless. I've removed this hunk now. > > /* enable DP PLL */ > > regmap_write(pdata->regmap, SN_PLL_ENABLE_REG, 1); > > > > @@ -1016,6 +1024,11 @@ static int ti_sn_link_training(struct ti_sn65dsi86 *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); > > In my previous review I asked for some comments to include the "why" > we disabling scrambling mode for DP. Can you please add that? > > I also presume that for DP it's probably a good idea to avoid the code > in ti_sn_bridge_enable() that tells the sink to use the alternate > scrambler. looking at it yes. I've added a check to avoid that in my DP connector case, and there doesn't seem to be any effect on the output. But I'll add it to the patch for the next version. >> 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. Is this better: /* * Only eDP pannels which support Alternate Scrambler Seed Reset (ASSR) * are supported by the SN65DSI86. For DisplayPort, disable scrambling * mode. */ I don't know if it answers your 'why' ... and I don't think adding "We think it is for DRM protection" really suits the comment. > [1] https://lore.kernel.org/r/CAD=FV=Wwayx1Y-xv=RPuJbG+Q1wHrUWgh4P7wuzy_bAL=_FN0g@xxxxxxxxxxxxxx > > -Doug
