diff mbox

Revert "drm/i915/bxt: Disable device ready before shutdown command"

Message ID 1504604671-17237-1-git-send-email-vidya.srinivas@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Vidya Srinivas Sept. 5, 2017, 9:44 a.m. UTC
From: Uma Shankar <uma.shankar@intel.com>

This reverts commit bbdf0b2ff32aa75c7bd167569130e9391d2e6282.

Disable device ready before shutdown command was added previously to avoid a
split screen issue seen on dual link DSI panels. As of now, dual link is not
supported and will need some rework in the upstream code. For single link DSI
panels, the change is not required. This will cause failure in sending SHUTDOWN
packet during disable. Hence reverting the change. Will handle the change
as part of dual link enabling in upstream.

Signed-off-by: Uma Shankar <uma.shankar@intel.com>
Signed-off-by: Vidya Srinivas <vidya.srinivas@intel.com>
---
 drivers/gpu/drm/i915/intel_dsi.c | 11 -----------
 1 file changed, 11 deletions(-)

Comments

Mika Kahola Sept. 5, 2017, 10:43 a.m. UTC | #1
The revert patch solved the issue

*ERROR* Video mode command 0x00000041 send failed.

on my setup with APL+MIPI/DSI single link combo.

Tested-by: Mika Kahola <mika.kahola@intel.com>


On Tue, 2017-09-05 at 15:14 +0530, Vidya Srinivas wrote:
> From: Uma Shankar <uma.shankar@intel.com>
> 
> This reverts commit bbdf0b2ff32aa75c7bd167569130e9391d2e6282.
> 
> Disable device ready before shutdown command was added previously to
> avoid a
> split screen issue seen on dual link DSI panels. As of now, dual link
> is not
> supported and will need some rework in the upstream code. For single
> link DSI
> panels, the change is not required. This will cause failure in
> sending SHUTDOWN
> packet during disable. Hence reverting the change. Will handle the
> change
> as part of dual link enabling in upstream.
> 
> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> Signed-off-by: Vidya Srinivas <vidya.srinivas@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dsi.c | 11 -----------
>  1 file changed, 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dsi.c
> b/drivers/gpu/drm/i915/intel_dsi.c
> index 2a0f5d3..fc25d7d 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.c
> +++ b/drivers/gpu/drm/i915/intel_dsi.c
> @@ -892,8 +892,6 @@ static void intel_dsi_disable(struct
> intel_encoder *encoder,
>  			      const struct intel_crtc_state
> *old_crtc_state,
>  			      const struct drm_connector_state
> *old_conn_state)
>  {
> -	struct drm_device *dev = encoder->base.dev;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder-
> >base);
>  	enum port port;
>  
> @@ -903,15 +901,6 @@ static void intel_dsi_disable(struct
> intel_encoder *encoder,
>  	intel_panel_disable_backlight(old_conn_state);
>  
>  	/*
> -	 * Disable Device ready before the port shutdown in order
> -	 * to avoid split screen
> -	 */
> -	if (IS_BROXTON(dev_priv)) {
> -		for_each_dsi_port(port, intel_dsi->ports)
> -			I915_WRITE(MIPI_DEVICE_READY(port), 0);
> -	}
> -
> -	/*
>  	 * According to the spec we should send SHUTDOWN before
>  	 * MIPI_SEQ_DISPLAY_OFF only for v3+ VBTs, but field testing
>  	 * has shown that the v3 sequence works for v2 VBTs too
Chauhan, Madhav Sept. 12, 2017, 10:02 a.m. UTC | #2
> -----Original Message-----

> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of

> Vidya Srinivas

> Sent: Tuesday, September 5, 2017 3:15 PM

> To: intel-gfx@lists.freedesktop.org

> Cc: Srinivas, Vidya <vidya.srinivas@intel.com>

> Subject: [Intel-gfx] [PATCH] Revert "drm/i915/bxt: Disable device ready

> before shutdown command"

> 

> From: Uma Shankar <uma.shankar@intel.com>

> 

> This reverts commit bbdf0b2ff32aa75c7bd167569130e9391d2e6282.

> 

> Disable device ready before shutdown command was added previously to

> avoid a split screen issue seen on dual link DSI panels. As of now, dual link is

> not supported and will need some rework in the upstream code. For single

> link DSI panels, the change is not required. This will cause failure in sending

> SHUTDOWN packet during disable. Hence reverting the change. Will handle

> the change as part of dual link enabling in upstream.


Already there are lot of changes  wrt  to DSI dual link inside intel_dsi.c.
Can't we keep this change as well under the condition if (dual_link && Broxton)??
Might help to fetch dual link changes in future instead of losing it.

Regards,
Madhav

> 

> Signed-off-by: Uma Shankar <uma.shankar@intel.com>

> Signed-off-by: Vidya Srinivas <vidya.srinivas@intel.com>

> ---

>  drivers/gpu/drm/i915/intel_dsi.c | 11 -----------

>  1 file changed, 11 deletions(-)

> 

> diff --git a/drivers/gpu/drm/i915/intel_dsi.c

> b/drivers/gpu/drm/i915/intel_dsi.c

> index 2a0f5d3..fc25d7d 100644

> --- a/drivers/gpu/drm/i915/intel_dsi.c

> +++ b/drivers/gpu/drm/i915/intel_dsi.c

> @@ -892,8 +892,6 @@ static void intel_dsi_disable(struct intel_encoder

> *encoder,

>  			      const struct intel_crtc_state *old_crtc_state,

>  			      const struct drm_connector_state

> *old_conn_state)  {

> -	struct drm_device *dev = encoder->base.dev;

> -	struct drm_i915_private *dev_priv = dev->dev_private;

>  	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);

>  	enum port port;

> 

> @@ -903,15 +901,6 @@ static void intel_dsi_disable(struct intel_encoder

> *encoder,

>  	intel_panel_disable_backlight(old_conn_state);

> 

>  	/*

> -	 * Disable Device ready before the port shutdown in order

> -	 * to avoid split screen

> -	 */

> -	if (IS_BROXTON(dev_priv)) {

> -		for_each_dsi_port(port, intel_dsi->ports)

> -			I915_WRITE(MIPI_DEVICE_READY(port), 0);

> -	}

> -

> -	/*

>  	 * According to the spec we should send SHUTDOWN before

>  	 * MIPI_SEQ_DISPLAY_OFF only for v3+ VBTs, but field testing

>  	 * has shown that the v3 sequence works for v2 VBTs too

> --

> 1.9.1

> 

> _______________________________________________

> Intel-gfx mailing list

> Intel-gfx@lists.freedesktop.org

> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Jani Nikula Sept. 13, 2017, 8:05 a.m. UTC | #3
On Tue, 05 Sep 2017, Mika Kahola <mika.kahola@intel.com> wrote:
> The revert patch solved the issue
>
> *ERROR* Video mode command 0x00000041 send failed.
>
> on my setup with APL+MIPI/DSI single link combo.
>
> Tested-by: Mika Kahola <mika.kahola@intel.com>

Pushed to dinq, thanks for the patch and testing. Alas, I failed to add
your Tested-by tag, apologies.

BR,
Jani.


>
>
> On Tue, 2017-09-05 at 15:14 +0530, Vidya Srinivas wrote:
>> From: Uma Shankar <uma.shankar@intel.com>
>> 
>> This reverts commit bbdf0b2ff32aa75c7bd167569130e9391d2e6282.
>> 
>> Disable device ready before shutdown command was added previously to
>> avoid a
>> split screen issue seen on dual link DSI panels. As of now, dual link
>> is not
>> supported and will need some rework in the upstream code. For single
>> link DSI
>> panels, the change is not required. This will cause failure in
>> sending SHUTDOWN
>> packet during disable. Hence reverting the change. Will handle the
>> change
>> as part of dual link enabling in upstream.
>> 
>> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
>> Signed-off-by: Vidya Srinivas <vidya.srinivas@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_dsi.c | 11 -----------
>>  1 file changed, 11 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/intel_dsi.c
>> b/drivers/gpu/drm/i915/intel_dsi.c
>> index 2a0f5d3..fc25d7d 100644
>> --- a/drivers/gpu/drm/i915/intel_dsi.c
>> +++ b/drivers/gpu/drm/i915/intel_dsi.c
>> @@ -892,8 +892,6 @@ static void intel_dsi_disable(struct
>> intel_encoder *encoder,
>>  			      const struct intel_crtc_state
>> *old_crtc_state,
>>  			      const struct drm_connector_state
>> *old_conn_state)
>>  {
>> -	struct drm_device *dev = encoder->base.dev;
>> -	struct drm_i915_private *dev_priv = dev->dev_private;
>>  	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder-
>> >base);
>>  	enum port port;
>>  
>> @@ -903,15 +901,6 @@ static void intel_dsi_disable(struct
>> intel_encoder *encoder,
>>  	intel_panel_disable_backlight(old_conn_state);
>>  
>>  	/*
>> -	 * Disable Device ready before the port shutdown in order
>> -	 * to avoid split screen
>> -	 */
>> -	if (IS_BROXTON(dev_priv)) {
>> -		for_each_dsi_port(port, intel_dsi->ports)
>> -			I915_WRITE(MIPI_DEVICE_READY(port), 0);
>> -	}
>> -
>> -	/*
>>  	 * According to the spec we should send SHUTDOWN before
>>  	 * MIPI_SEQ_DISPLAY_OFF only for v3+ VBTs, but field testing
>>  	 * has shown that the v3 sequence works for v2 VBTs too
Jani Nikula Sept. 13, 2017, 8:09 a.m. UTC | #4
On Tue, 12 Sep 2017, "Chauhan, Madhav" <madhav.chauhan@intel.com> wrote:
>> -----Original Message-----
>> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of
>> Vidya Srinivas
>> Sent: Tuesday, September 5, 2017 3:15 PM
>> To: intel-gfx@lists.freedesktop.org
>> Cc: Srinivas, Vidya <vidya.srinivas@intel.com>
>> Subject: [Intel-gfx] [PATCH] Revert "drm/i915/bxt: Disable device ready
>> before shutdown command"
>> 
>> From: Uma Shankar <uma.shankar@intel.com>
>> 
>> This reverts commit bbdf0b2ff32aa75c7bd167569130e9391d2e6282.
>> 
>> Disable device ready before shutdown command was added previously to
>> avoid a split screen issue seen on dual link DSI panels. As of now, dual link is
>> not supported and will need some rework in the upstream code. For single
>> link DSI panels, the change is not required. This will cause failure in sending
>> SHUTDOWN packet during disable. Hence reverting the change. Will handle
>> the change as part of dual link enabling in upstream.
>
> Already there are lot of changes  wrt  to DSI dual link inside intel_dsi.c.
> Can't we keep this change as well under the condition if (dual_link && Broxton)??
> Might help to fetch dual link changes in future instead of losing it.

I've pushed the patch as-is. I don't think there are grounds to keep it
even with dual_link. As Uma says, these things may be panel specific
too.

BR,
Jani.
Chauhan, Madhav Sept. 13, 2017, 8:13 a.m. UTC | #5
> -----Original Message-----
> From: Nikula, Jani
> Sent: Wednesday, September 13, 2017 1:39 PM
> To: Chauhan, Madhav <madhav.chauhan@intel.com>; Srinivas, Vidya
> <vidya.srinivas@intel.com>; intel-gfx@lists.freedesktop.org
> Subject: RE: [Intel-gfx] [PATCH] Revert "drm/i915/bxt: Disable device ready
> before shutdown command"
> 
> On Tue, 12 Sep 2017, "Chauhan, Madhav" <madhav.chauhan@intel.com>
> wrote:
> >> -----Original Message-----
> >> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On
> >> Behalf Of Vidya Srinivas
> >> Sent: Tuesday, September 5, 2017 3:15 PM
> >> To: intel-gfx@lists.freedesktop.org
> >> Cc: Srinivas, Vidya <vidya.srinivas@intel.com>
> >> Subject: [Intel-gfx] [PATCH] Revert "drm/i915/bxt: Disable device
> >> ready before shutdown command"
> >>
> >> From: Uma Shankar <uma.shankar@intel.com>
> >>
> >> This reverts commit bbdf0b2ff32aa75c7bd167569130e9391d2e6282.
> >>
> >> Disable device ready before shutdown command was added previously to
> >> avoid a split screen issue seen on dual link DSI panels. As of now,
> >> dual link is not supported and will need some rework in the upstream
> >> code. For single link DSI panels, the change is not required. This
> >> will cause failure in sending SHUTDOWN packet during disable. Hence
> >> reverting the change. Will handle the change as part of dual link enabling
> in upstream.
> >
> > Already there are lot of changes  wrt  to DSI dual link inside intel_dsi.c.
> > Can't we keep this change as well under the condition if (dual_link &&
> Broxton)??
> > Might help to fetch dual link changes in future instead of losing it.
> 
> I've pushed the patch as-is. I don't think there are grounds to keep it even
> with dual_link. As Uma says, these things may be panel specific too.

Hmm..Ok Thanks for clarification.

Regards,
Madhav

> 
> BR,
> Jani.
> 
> 
> --
> Jani Nikula, Intel Open Source Technology Center
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
index 2a0f5d3..fc25d7d 100644
--- a/drivers/gpu/drm/i915/intel_dsi.c
+++ b/drivers/gpu/drm/i915/intel_dsi.c
@@ -892,8 +892,6 @@  static void intel_dsi_disable(struct intel_encoder *encoder,
 			      const struct intel_crtc_state *old_crtc_state,
 			      const struct drm_connector_state *old_conn_state)
 {
-	struct drm_device *dev = encoder->base.dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
 	enum port port;
 
@@ -903,15 +901,6 @@  static void intel_dsi_disable(struct intel_encoder *encoder,
 	intel_panel_disable_backlight(old_conn_state);
 
 	/*
-	 * Disable Device ready before the port shutdown in order
-	 * to avoid split screen
-	 */
-	if (IS_BROXTON(dev_priv)) {
-		for_each_dsi_port(port, intel_dsi->ports)
-			I915_WRITE(MIPI_DEVICE_READY(port), 0);
-	}
-
-	/*
 	 * According to the spec we should send SHUTDOWN before
 	 * MIPI_SEQ_DISPLAY_OFF only for v3+ VBTs, but field testing
 	 * has shown that the v3 sequence works for v2 VBTs too