diff mbox series

[v5,13/13] drm/bridge: cdns-dsi: Use pre_enable/post_disable to enable/disable

Message ID 20241019200530.270738-6-aradhya.bhatia@linux.dev (mailing list archive)
State New, archived
Headers show
Series drm/bridge: cdns-dsi: Fix the color-shift issue | expand

Commit Message

Aradhya Bhatia Oct. 19, 2024, 8:05 p.m. UTC 
From: Aradhya Bhatia <a-bhatia1@ti.com>

The cdns-dsi controller requires that it be turned on completely before
the input DPI's source has begun streaming[0]. Not having that, allows
for a small window before cdns-dsi enable and after cdns-dsi disable
where the previous entity (in this case tidss's videoport) to continue
streaming DPI video signals. This small window where cdns-dsi is
disabled but is still receiving signals causes the input FIFO of
cdns-dsi to get corrupted. This causes the colors to shift on the output
display. The colors can either shift by one color component (R->G, G->B,
B->R), or by two color components (R->B, G->R, B->G).

Since tidss's videoport starts streaming via crtc enable hooks, we need
cdns-dsi to be up and running before that. Now that the bridges are
pre_enabled before crtc is enabled, and post_disabled after crtc is
disabled, use the pre_enable and post_disable hooks to get cdns-dsi
ready and running before the tidss videoport to get pass the color shift
issues.

[0]: See section 12.6.5.7.3 "Start-up Procedure" in J721E SoC TRM
     TRM Link: http://www.ti.com/lit/pdf/spruil1

Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
Signed-off-by: Aradhya Bhatia <aradhya.bhatia@linux.dev>
---
 .../gpu/drm/bridge/cadence/cdns-dsi-core.c    | 62 ++++++++++---------
 1 file changed, 34 insertions(+), 28 deletions(-)

Comments

Dmitry Baryshkov Oct. 20, 2024, 11:57 a.m. UTC | #1 
On Sun, Oct 20, 2024 at 01:35:30AM +0530, Aradhya Bhatia wrote:
> From: Aradhya Bhatia <a-bhatia1@ti.com>
> 
> The cdns-dsi controller requires that it be turned on completely before
> the input DPI's source has begun streaming[0]. Not having that, allows
> for a small window before cdns-dsi enable and after cdns-dsi disable
> where the previous entity (in this case tidss's videoport) to continue
> streaming DPI video signals. This small window where cdns-dsi is
> disabled but is still receiving signals causes the input FIFO of
> cdns-dsi to get corrupted. This causes the colors to shift on the output
> display. The colors can either shift by one color component (R->G, G->B,
> B->R), or by two color components (R->B, G->R, B->G).
> 
> Since tidss's videoport starts streaming via crtc enable hooks, we need
> cdns-dsi to be up and running before that. Now that the bridges are
> pre_enabled before crtc is enabled, and post_disabled after crtc is
> disabled, use the pre_enable and post_disable hooks to get cdns-dsi
> ready and running before the tidss videoport to get pass the color shift
> issues.
> 

Not being an expert in the TI DSS driver, would it be more proper to
handle that in the TI driver instead? I mean, sending out DPI signals
isn't a part of the CRTC setup, it's a job of the encoder.

> [0]: See section 12.6.5.7.3 "Start-up Procedure" in J721E SoC TRM
>      TRM Link: http://www.ti.com/lit/pdf/spruil1
> 
> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
> Signed-off-by: Aradhya Bhatia <aradhya.bhatia@linux.dev>
> ---
>  .../gpu/drm/bridge/cadence/cdns-dsi-core.c    | 62 ++++++++++---------
>  1 file changed, 34 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> index 79d8c2264c14..dfeb53841ebc 100644
> --- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> +++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> @@ -658,13 +658,28 @@ cdns_dsi_bridge_mode_valid(struct drm_bridge *bridge,
>  	return MODE_OK;
>  }
>  
> -static void cdns_dsi_bridge_atomic_disable(struct drm_bridge *bridge,
> -					   struct drm_bridge_state *old_bridge_state)
> +static void cdns_dsi_bridge_atomic_post_disable(struct drm_bridge *bridge,
> +						struct drm_bridge_state *old_bridge_state)
>  {
>  	struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
>  	struct cdns_dsi *dsi = input_to_dsi(input);
>  	u32 val;
>  
> +	/*
> +	 * The cdns-dsi controller needs to be disabled after it's DPI source
> +	 * has stopped streaming. If this is not followed, there is a brief
> +	 * window before DPI source is disabled and after cdns-dsi controller
> +	 * has been disabled where the DPI stream is still on, but the cdns-dsi
> +	 * controller is not ready anymore to accept the incoming signals. This
> +	 * is one of the reasons why a shift in pixel colors is observed on
> +	 * displays that have cdns-dsi as one of the bridges.
> +	 *
> +	 * To mitigate this, disable this bridge from the bridge post_disable()
> +	 * hook, instead of the bridge _disable() hook. The bridge post_disable()
> +	 * hook gets called after the CRTC disable, where often many DPI sources
> +	 * disable their streams.
> +	 */
> +
>  	val = readl(dsi->regs + MCTL_MAIN_DATA_CTL);
>  	val &= ~(IF_VID_SELECT_MASK | IF_VID_MODE | VID_EN | HOST_EOT_GEN |
>  		 DISP_EOT_GEN);
> @@ -683,15 +698,6 @@ static void cdns_dsi_bridge_atomic_disable(struct drm_bridge *bridge,
>  	pm_runtime_put(dsi->base.dev);
>  }
>  
> -static void cdns_dsi_bridge_atomic_post_disable(struct drm_bridge *bridge,
> -						struct drm_bridge_state *old_bridge_state)
> -{
> -	struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
> -	struct cdns_dsi *dsi = input_to_dsi(input);
> -
> -	pm_runtime_put(dsi->base.dev);
> -}
> -
>  static void cdns_dsi_hs_init(struct cdns_dsi *dsi)
>  {
>  	struct cdns_dsi_output *output = &dsi->output;
> @@ -760,8 +766,8 @@ static void cdns_dsi_init_link(struct cdns_dsi *dsi)
>  	dsi->link_initialized = true;
>  }
>  
> -static void cdns_dsi_bridge_atomic_enable(struct drm_bridge *bridge,
> -					  struct drm_bridge_state *old_bridge_state)
> +static void cdns_dsi_bridge_atomic_pre_enable(struct drm_bridge *bridge,
> +					      struct drm_bridge_state *old_bridge_state)
>  {
>  	struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
>  	struct cdns_dsi *dsi = input_to_dsi(input);
> @@ -776,6 +782,21 @@ static void cdns_dsi_bridge_atomic_enable(struct drm_bridge *bridge,
>  	if (WARN_ON(pm_runtime_get_sync(dsi->base.dev) < 0))
>  		return;
>  
> +	/*
> +	 * The cdns-dsi controller needs to be enabled before it's DPI source
> +	 * has begun streaming. If this is not followed, there is a brief window
> +	 * after DPI source enable and before cdns-dsi controller enable where
> +	 * the DPI stream is on, but the cdns-dsi controller is not ready to
> +	 * accept the incoming signals. This is one of the reasons why a shift
> +	 * in pixel colors is observed on displays that have cdns-dsi as one of
> +	 * the bridges.
> +	 *
> +	 * To mitigate this, enable this bridge from the bridge pre_enable()
> +	 * hook, instead of the bridge _enable() hook. The bridge pre_enable()
> +	 * hook gets called before the CRTC enable, where often many DPI sources
> +	 * enable their streams.
> +	 */
> +
>  	if (dsi->platform_ops && dsi->platform_ops->enable)
>  		dsi->platform_ops->enable(dsi);
>  
> @@ -912,19 +933,6 @@ static void cdns_dsi_bridge_atomic_enable(struct drm_bridge *bridge,
>  	writel(tmp, dsi->regs + MCTL_MAIN_EN);
>  }
>  
> -static void cdns_dsi_bridge_atomic_pre_enable(struct drm_bridge *bridge,
> -					      struct drm_bridge_state *old_bridge_state)
> -{
> -	struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
> -	struct cdns_dsi *dsi = input_to_dsi(input);
> -
> -	if (WARN_ON(pm_runtime_get_sync(dsi->base.dev) < 0))
> -		return;
> -
> -	cdns_dsi_init_link(dsi);
> -	cdns_dsi_hs_init(dsi);
> -}
> -
>  static u32 *cdns_dsi_bridge_get_input_bus_fmts(struct drm_bridge *bridge,
>  					       struct drm_bridge_state *bridge_state,
>  					       struct drm_crtc_state *crtc_state,
> @@ -968,9 +976,7 @@ static int cdns_dsi_bridge_atomic_check(struct drm_bridge *bridge,
>  static const struct drm_bridge_funcs cdns_dsi_bridge_funcs = {
>  	.attach = cdns_dsi_bridge_attach,
>  	.mode_valid = cdns_dsi_bridge_mode_valid,
> -	.atomic_disable = cdns_dsi_bridge_atomic_disable,
>  	.atomic_pre_enable = cdns_dsi_bridge_atomic_pre_enable,
> -	.atomic_enable = cdns_dsi_bridge_atomic_enable,
>  	.atomic_post_disable = cdns_dsi_bridge_atomic_post_disable,
>  	.atomic_check = cdns_dsi_bridge_atomic_check,
>  	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
> -- 
> 2.34.1
>
Aradhya Bhatia Oct. 21, 2024, 5:07 p.m. UTC | #2 
Hi Dmitry,

Thank you for reviewing the patches!

On 10/20/24 17:27, Dmitry Baryshkov wrote:
> On Sun, Oct 20, 2024 at 01:35:30AM +0530, Aradhya Bhatia wrote:
>> From: Aradhya Bhatia <a-bhatia1@ti.com>
>>
>> The cdns-dsi controller requires that it be turned on completely before
>> the input DPI's source has begun streaming[0]. Not having that, allows
>> for a small window before cdns-dsi enable and after cdns-dsi disable
>> where the previous entity (in this case tidss's videoport) to continue
>> streaming DPI video signals. This small window where cdns-dsi is
>> disabled but is still receiving signals causes the input FIFO of
>> cdns-dsi to get corrupted. This causes the colors to shift on the output
>> display. The colors can either shift by one color component (R->G, G->B,
>> B->R), or by two color components (R->B, G->R, B->G).
>>
>> Since tidss's videoport starts streaming via crtc enable hooks, we need
>> cdns-dsi to be up and running before that. Now that the bridges are
>> pre_enabled before crtc is enabled, and post_disabled after crtc is
>> disabled, use the pre_enable and post_disable hooks to get cdns-dsi
>> ready and running before the tidss videoport to get pass the color shift
>> issues.
>>
> 
> Not being an expert in the TI DSS driver, would it be more proper to
> handle that in the TI driver instead? I mean, sending out DPI signals
> isn't a part of the CRTC setup, it's a job of the encoder.

I haven't done a feasibility analysis of moving the CRTC bits of TIDSS
into the encoder, but even if it were possible, it wouldn't solve the
issue.

The bridge_enable() sequence gets called _after_ the encoder has been
enabled - causing the TIDSS's DPI (enabled from encoder) to still be
up and running before the DSI has had a chance to be ready.

Regards
Aradhya


> 
>> [0]: See section 12.6.5.7.3 "Start-up Procedure" in J721E SoC TRM
>>      TRM Link: http://www.ti.com/lit/pdf/spruil1
>>
>> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
>> Signed-off-by: Aradhya Bhatia <aradhya.bhatia@linux.dev>
>> ---
>>  .../gpu/drm/bridge/cadence/cdns-dsi-core.c    | 62 ++++++++++---------
>>  1 file changed, 34 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
>> index 79d8c2264c14..dfeb53841ebc 100644
>> --- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
>> +++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
>> @@ -658,13 +658,28 @@ cdns_dsi_bridge_mode_valid(struct drm_bridge *bridge,
>>  	return MODE_OK;
>>  }
>>  
>> -static void cdns_dsi_bridge_atomic_disable(struct drm_bridge *bridge,
>> -					   struct drm_bridge_state *old_bridge_state)
>> +static void cdns_dsi_bridge_atomic_post_disable(struct drm_bridge *bridge,
>> +						struct drm_bridge_state *old_bridge_state)
>>  {
>>  	struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
>>  	struct cdns_dsi *dsi = input_to_dsi(input);
>>  	u32 val;
>>  
>> +	/*
>> +	 * The cdns-dsi controller needs to be disabled after it's DPI source
>> +	 * has stopped streaming. If this is not followed, there is a brief
>> +	 * window before DPI source is disabled and after cdns-dsi controller
>> +	 * has been disabled where the DPI stream is still on, but the cdns-dsi
>> +	 * controller is not ready anymore to accept the incoming signals. This
>> +	 * is one of the reasons why a shift in pixel colors is observed on
>> +	 * displays that have cdns-dsi as one of the bridges.
>> +	 *
>> +	 * To mitigate this, disable this bridge from the bridge post_disable()
>> +	 * hook, instead of the bridge _disable() hook. The bridge post_disable()
>> +	 * hook gets called after the CRTC disable, where often many DPI sources
>> +	 * disable their streams.
>> +	 */
>> +
>>  	val = readl(dsi->regs + MCTL_MAIN_DATA_CTL);
>>  	val &= ~(IF_VID_SELECT_MASK | IF_VID_MODE | VID_EN | HOST_EOT_GEN |
>>  		 DISP_EOT_GEN);
>> @@ -683,15 +698,6 @@ static void cdns_dsi_bridge_atomic_disable(struct drm_bridge *bridge,
>>  	pm_runtime_put(dsi->base.dev);
>>  }
>>  
>> -static void cdns_dsi_bridge_atomic_post_disable(struct drm_bridge *bridge,
>> -						struct drm_bridge_state *old_bridge_state)
>> -{
>> -	struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
>> -	struct cdns_dsi *dsi = input_to_dsi(input);
>> -
>> -	pm_runtime_put(dsi->base.dev);
>> -}
>> -
>>  static void cdns_dsi_hs_init(struct cdns_dsi *dsi)
>>  {
>>  	struct cdns_dsi_output *output = &dsi->output;
>> @@ -760,8 +766,8 @@ static void cdns_dsi_init_link(struct cdns_dsi *dsi)
>>  	dsi->link_initialized = true;
>>  }
>>  
>> -static void cdns_dsi_bridge_atomic_enable(struct drm_bridge *bridge,
>> -					  struct drm_bridge_state *old_bridge_state)
>> +static void cdns_dsi_bridge_atomic_pre_enable(struct drm_bridge *bridge,
>> +					      struct drm_bridge_state *old_bridge_state)
>>  {
>>  	struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
>>  	struct cdns_dsi *dsi = input_to_dsi(input);
>> @@ -776,6 +782,21 @@ static void cdns_dsi_bridge_atomic_enable(struct drm_bridge *bridge,
>>  	if (WARN_ON(pm_runtime_get_sync(dsi->base.dev) < 0))
>>  		return;
>>  
>> +	/*
>> +	 * The cdns-dsi controller needs to be enabled before it's DPI source
>> +	 * has begun streaming. If this is not followed, there is a brief window
>> +	 * after DPI source enable and before cdns-dsi controller enable where
>> +	 * the DPI stream is on, but the cdns-dsi controller is not ready to
>> +	 * accept the incoming signals. This is one of the reasons why a shift
>> +	 * in pixel colors is observed on displays that have cdns-dsi as one of
>> +	 * the bridges.
>> +	 *
>> +	 * To mitigate this, enable this bridge from the bridge pre_enable()
>> +	 * hook, instead of the bridge _enable() hook. The bridge pre_enable()
>> +	 * hook gets called before the CRTC enable, where often many DPI sources
>> +	 * enable their streams.
>> +	 */
>> +
>>  	if (dsi->platform_ops && dsi->platform_ops->enable)
>>  		dsi->platform_ops->enable(dsi);
>>  
>> @@ -912,19 +933,6 @@ static void cdns_dsi_bridge_atomic_enable(struct drm_bridge *bridge,
>>  	writel(tmp, dsi->regs + MCTL_MAIN_EN);
>>  }
>>  
>> -static void cdns_dsi_bridge_atomic_pre_enable(struct drm_bridge *bridge,
>> -					      struct drm_bridge_state *old_bridge_state)
>> -{
>> -	struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
>> -	struct cdns_dsi *dsi = input_to_dsi(input);
>> -
>> -	if (WARN_ON(pm_runtime_get_sync(dsi->base.dev) < 0))
>> -		return;
>> -
>> -	cdns_dsi_init_link(dsi);
>> -	cdns_dsi_hs_init(dsi);
>> -}
>> -
>>  static u32 *cdns_dsi_bridge_get_input_bus_fmts(struct drm_bridge *bridge,
>>  					       struct drm_bridge_state *bridge_state,
>>  					       struct drm_crtc_state *crtc_state,
>> @@ -968,9 +976,7 @@ static int cdns_dsi_bridge_atomic_check(struct drm_bridge *bridge,
>>  static const struct drm_bridge_funcs cdns_dsi_bridge_funcs = {
>>  	.attach = cdns_dsi_bridge_attach,
>>  	.mode_valid = cdns_dsi_bridge_mode_valid,
>> -	.atomic_disable = cdns_dsi_bridge_atomic_disable,
>>  	.atomic_pre_enable = cdns_dsi_bridge_atomic_pre_enable,
>> -	.atomic_enable = cdns_dsi_bridge_atomic_enable,
>>  	.atomic_post_disable = cdns_dsi_bridge_atomic_post_disable,
>>  	.atomic_check = cdns_dsi_bridge_atomic_check,
>>  	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
>> -- 
>> 2.34.1
>>
>
Dmitry Baryshkov Oct. 21, 2024, 6:39 p.m. UTC | #3 
On Mon, 21 Oct 2024 at 20:07, Aradhya Bhatia <aradhya.bhatia@linux.dev> wrote:
>
> Hi Dmitry,
>
> Thank you for reviewing the patches!
>
> On 10/20/24 17:27, Dmitry Baryshkov wrote:
> > On Sun, Oct 20, 2024 at 01:35:30AM +0530, Aradhya Bhatia wrote:
> >> From: Aradhya Bhatia <a-bhatia1@ti.com>
> >>
> >> The cdns-dsi controller requires that it be turned on completely before
> >> the input DPI's source has begun streaming[0]. Not having that, allows
> >> for a small window before cdns-dsi enable and after cdns-dsi disable
> >> where the previous entity (in this case tidss's videoport) to continue
> >> streaming DPI video signals. This small window where cdns-dsi is
> >> disabled but is still receiving signals causes the input FIFO of
> >> cdns-dsi to get corrupted. This causes the colors to shift on the output
> >> display. The colors can either shift by one color component (R->G, G->B,
> >> B->R), or by two color components (R->B, G->R, B->G).
> >>
> >> Since tidss's videoport starts streaming via crtc enable hooks, we need
> >> cdns-dsi to be up and running before that. Now that the bridges are
> >> pre_enabled before crtc is enabled, and post_disabled after crtc is
> >> disabled, use the pre_enable and post_disable hooks to get cdns-dsi
> >> ready and running before the tidss videoport to get pass the color shift
> >> issues.
> >>
> >
> > Not being an expert in the TI DSS driver, would it be more proper to
> > handle that in the TI driver instead? I mean, sending out DPI signals
> > isn't a part of the CRTC setup, it's a job of the encoder.
>
> I haven't done a feasibility analysis of moving the CRTC bits of TIDSS
> into the encoder, but even if it were possible, it wouldn't solve the
> issue.
>
> The bridge_enable() sequence gets called _after_ the encoder has been
> enabled - causing the TIDSS's DPI (enabled from encoder) to still be
> up and running before the DSI has had a chance to be ready.

But then you can move CDSI setup to pre_enable, so that all setup
happens before encoder's (= DPI) being enabled.
Aradhya Bhatia Oct. 22, 2024, 5:19 p.m. UTC | #4 
On 10/22/24 00:09, Dmitry Baryshkov wrote:
> On Mon, 21 Oct 2024 at 20:07, Aradhya Bhatia <aradhya.bhatia@linux.dev> wrote:
>>
>> Hi Dmitry,
>>
>> Thank you for reviewing the patches!
>>
>> On 10/20/24 17:27, Dmitry Baryshkov wrote:
>>> On Sun, Oct 20, 2024 at 01:35:30AM +0530, Aradhya Bhatia wrote:
>>>> From: Aradhya Bhatia <a-bhatia1@ti.com>
>>>>
>>>> The cdns-dsi controller requires that it be turned on completely before
>>>> the input DPI's source has begun streaming[0]. Not having that, allows
>>>> for a small window before cdns-dsi enable and after cdns-dsi disable
>>>> where the previous entity (in this case tidss's videoport) to continue
>>>> streaming DPI video signals. This small window where cdns-dsi is
>>>> disabled but is still receiving signals causes the input FIFO of
>>>> cdns-dsi to get corrupted. This causes the colors to shift on the output
>>>> display. The colors can either shift by one color component (R->G, G->B,
>>>> B->R), or by two color components (R->B, G->R, B->G).
>>>>
>>>> Since tidss's videoport starts streaming via crtc enable hooks, we need
>>>> cdns-dsi to be up and running before that. Now that the bridges are
>>>> pre_enabled before crtc is enabled, and post_disabled after crtc is
>>>> disabled, use the pre_enable and post_disable hooks to get cdns-dsi
>>>> ready and running before the tidss videoport to get pass the color shift
>>>> issues.
>>>>
>>>
>>> Not being an expert in the TI DSS driver, would it be more proper to
>>> handle that in the TI driver instead? I mean, sending out DPI signals
>>> isn't a part of the CRTC setup, it's a job of the encoder.
>>
>> I haven't done a feasibility analysis of moving the CRTC bits of TIDSS
>> into the encoder, but even if it were possible, it wouldn't solve the
>> issue.
>>
>> The bridge_enable() sequence gets called _after_ the encoder has been
>> enabled - causing the TIDSS's DPI (enabled from encoder) to still be
>> up and running before the DSI has had a chance to be ready.
> 
> But then you can move CDSI setup to pre_enable, so that all setup
> happens before encoder's (= DPI) being enabled.
> 
> 

Ah! I did not realize that you were suggesting against moving
_bridge_pre_enable() to before crtc_enable().

I think, despite its initial complexity, it is a good idea to move the
_bridge_pre_enable() before the crtc_enable().

The boundary between an encoder and a CRTC in the modern display
hardware seems to have blurred a bit. Maybe the tidss / cdns-dsi is a
unique case (only for now), but of course, tidss is not
the only one generating the DPI signal from the CRTC.

And bridges should be allowed an option to get _some_ configuration done
before the CRTC and encoder are up and running, and I think that's where
the re-ordering is of the essence.

My initial version did suggest creating a new API[0], "_early_enable()"
that allowed _pre_enable() to remain where it is, all the while allowing
the bridges to have the option of configuring before the signals start
getting generated in the pipeline. That idea was then translated into
the current one.


Regards
Aradhya


[0]: https://lore.kernel.org/all/20240511153051.1355825-7-a-bhatia1@ti.com/
diff mbox series

Patch

diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
index 79d8c2264c14..dfeb53841ebc 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
@@ -658,13 +658,28 @@  cdns_dsi_bridge_mode_valid(struct drm_bridge *bridge,
 	return MODE_OK;
 }
 
-static void cdns_dsi_bridge_atomic_disable(struct drm_bridge *bridge,
-					   struct drm_bridge_state *old_bridge_state)
+static void cdns_dsi_bridge_atomic_post_disable(struct drm_bridge *bridge,
+						struct drm_bridge_state *old_bridge_state)
 {
 	struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
 	struct cdns_dsi *dsi = input_to_dsi(input);
 	u32 val;
 
+	/*
+	 * The cdns-dsi controller needs to be disabled after it's DPI source
+	 * has stopped streaming. If this is not followed, there is a brief
+	 * window before DPI source is disabled and after cdns-dsi controller
+	 * has been disabled where the DPI stream is still on, but the cdns-dsi
+	 * controller is not ready anymore to accept the incoming signals. This
+	 * is one of the reasons why a shift in pixel colors is observed on
+	 * displays that have cdns-dsi as one of the bridges.
+	 *
+	 * To mitigate this, disable this bridge from the bridge post_disable()
+	 * hook, instead of the bridge _disable() hook. The bridge post_disable()
+	 * hook gets called after the CRTC disable, where often many DPI sources
+	 * disable their streams.
+	 */
+
 	val = readl(dsi->regs + MCTL_MAIN_DATA_CTL);
 	val &= ~(IF_VID_SELECT_MASK | IF_VID_MODE | VID_EN | HOST_EOT_GEN |
 		 DISP_EOT_GEN);
@@ -683,15 +698,6 @@  static void cdns_dsi_bridge_atomic_disable(struct drm_bridge *bridge,
 	pm_runtime_put(dsi->base.dev);
 }
 
-static void cdns_dsi_bridge_atomic_post_disable(struct drm_bridge *bridge,
-						struct drm_bridge_state *old_bridge_state)
-{
-	struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
-	struct cdns_dsi *dsi = input_to_dsi(input);
-
-	pm_runtime_put(dsi->base.dev);
-}
-
 static void cdns_dsi_hs_init(struct cdns_dsi *dsi)
 {
 	struct cdns_dsi_output *output = &dsi->output;
@@ -760,8 +766,8 @@  static void cdns_dsi_init_link(struct cdns_dsi *dsi)
 	dsi->link_initialized = true;
 }
 
-static void cdns_dsi_bridge_atomic_enable(struct drm_bridge *bridge,
-					  struct drm_bridge_state *old_bridge_state)
+static void cdns_dsi_bridge_atomic_pre_enable(struct drm_bridge *bridge,
+					      struct drm_bridge_state *old_bridge_state)
 {
 	struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
 	struct cdns_dsi *dsi = input_to_dsi(input);
@@ -776,6 +782,21 @@  static void cdns_dsi_bridge_atomic_enable(struct drm_bridge *bridge,
 	if (WARN_ON(pm_runtime_get_sync(dsi->base.dev) < 0))
 		return;
 
+	/*
+	 * The cdns-dsi controller needs to be enabled before it's DPI source
+	 * has begun streaming. If this is not followed, there is a brief window
+	 * after DPI source enable and before cdns-dsi controller enable where
+	 * the DPI stream is on, but the cdns-dsi controller is not ready to
+	 * accept the incoming signals. This is one of the reasons why a shift
+	 * in pixel colors is observed on displays that have cdns-dsi as one of
+	 * the bridges.
+	 *
+	 * To mitigate this, enable this bridge from the bridge pre_enable()
+	 * hook, instead of the bridge _enable() hook. The bridge pre_enable()
+	 * hook gets called before the CRTC enable, where often many DPI sources
+	 * enable their streams.
+	 */
+
 	if (dsi->platform_ops && dsi->platform_ops->enable)
 		dsi->platform_ops->enable(dsi);
 
@@ -912,19 +933,6 @@  static void cdns_dsi_bridge_atomic_enable(struct drm_bridge *bridge,
 	writel(tmp, dsi->regs + MCTL_MAIN_EN);
 }
 
-static void cdns_dsi_bridge_atomic_pre_enable(struct drm_bridge *bridge,
-					      struct drm_bridge_state *old_bridge_state)
-{
-	struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
-	struct cdns_dsi *dsi = input_to_dsi(input);
-
-	if (WARN_ON(pm_runtime_get_sync(dsi->base.dev) < 0))
-		return;
-
-	cdns_dsi_init_link(dsi);
-	cdns_dsi_hs_init(dsi);
-}
-
 static u32 *cdns_dsi_bridge_get_input_bus_fmts(struct drm_bridge *bridge,
 					       struct drm_bridge_state *bridge_state,
 					       struct drm_crtc_state *crtc_state,
@@ -968,9 +976,7 @@  static int cdns_dsi_bridge_atomic_check(struct drm_bridge *bridge,
 static const struct drm_bridge_funcs cdns_dsi_bridge_funcs = {
 	.attach = cdns_dsi_bridge_attach,
 	.mode_valid = cdns_dsi_bridge_mode_valid,
-	.atomic_disable = cdns_dsi_bridge_atomic_disable,
 	.atomic_pre_enable = cdns_dsi_bridge_atomic_pre_enable,
-	.atomic_enable = cdns_dsi_bridge_atomic_enable,
 	.atomic_post_disable = cdns_dsi_bridge_atomic_post_disable,
 	.atomic_check = cdns_dsi_bridge_atomic_check,
 	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,