[PATCH] drm/i915: dsi: fix pipe-off timeout due to port vs. pipe disable ordering

[PATCH] drm/i915: dsi: fix pipe-off timeout due to port vs. pipe disable ordering

[Imre Deak]

If we disable first the port (by disabling DPI) and only then the
display pipe the pipe-off flag will never be set, possibly leading to a
hanged pipe state at the next modeset-enable.

Note that according to the VLV2 display cluster HAS, we should disable
the port before the pipe. This doesn't seem to match reality based on
the above and it's also asymmetric with the enabling sequence, where we
first enable the port and then the pipe.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_dsi.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
index e73bec6..3756af1 100644
--- a/drivers/gpu/drm/i915/intel_dsi.c
+++ b/drivers/gpu/drm/i915/intel_dsi.c
@@ -206,6 +206,11 @@ static void intel_dsi_enable_nop(struct intel_encoder *encoder)
 	 */
 }
 
+static void intel_dsi_disable_nop(struct intel_encoder *encoder)
+{
+	DRM_DEBUG_KMS("\n");
+}
+
 static void intel_dsi_disable(struct intel_encoder *encoder)
 {
 	struct drm_device *dev = encoder->base.dev;
@@ -293,6 +298,8 @@ static void intel_dsi_post_disable(struct intel_encoder *encoder)
 
 	DRM_DEBUG_KMS("\n");
 
+	intel_dsi_disable(encoder);
+
 	intel_dsi_clear_device_ready(encoder);
 
 	val = I915_READ(DSPCLK_GATE_D);
@@ -691,7 +698,7 @@ bool intel_dsi_init(struct drm_device *dev)
 	intel_encoder->pre_pll_enable = intel_dsi_pre_pll_enable;
 	intel_encoder->pre_enable = intel_dsi_pre_enable;
 	intel_encoder->enable = intel_dsi_enable_nop;
-	intel_encoder->disable = intel_dsi_disable;
+	intel_encoder->disable = intel_dsi_disable_nop;
 	intel_encoder->post_disable = intel_dsi_post_disable;
 	intel_encoder->get_hw_state = intel_dsi_get_hw_state;
 	intel_encoder->get_config = intel_dsi_get_config;
-- 
1.8.4

Re: [PATCH] drm/i915: dsi: fix pipe-off timeout due to port vs. pipe disable ordering

[Kumar, Shobhit]

On 5/27/2014 6:45 PM, Imre Deak wrote:
> If we disable first the port (by disabling DPI) and only then the
> display pipe the pipe-off flag will never be set, possibly leading to a
> hanged pipe state at the next modeset-enable.
>
> Note that according to the VLV2 display cluster HAS, we should disable
> the port before the pipe. This doesn't seem to match reality based on
> the above and it's also asymmetric with the enabling sequence, where we
> first enable the port and then the pipe.
>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_dsi.c | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> index e73bec6..3756af1 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.c
> +++ b/drivers/gpu/drm/i915/intel_dsi.c
> @@ -206,6 +206,11 @@ static void intel_dsi_enable_nop(struct intel_encoder *encoder)
>   	 */
>   }
>
> +static void intel_dsi_disable_nop(struct intel_encoder *encoder)
> +{
> +	DRM_DEBUG_KMS("\n");
> +}
> +

Make this as *_pre_disable and the SHUTDOWN command should be sent in 
this function. That is the recommended sequence in disable path. First 
thing is to shutdown the panel. Remove that bit from the intel_dsi_disable.

>   static void intel_dsi_disable(struct intel_encoder *encoder)
>   {
>   	struct drm_device *dev = encoder->base.dev;
> @@ -293,6 +298,8 @@ static void intel_dsi_post_disable(struct intel_encoder *encoder)
>
>   	DRM_DEBUG_KMS("\n");
>
> +	intel_dsi_disable(encoder);
> +
>   	intel_dsi_clear_device_ready(encoder);
>
>   	val = I915_READ(DSPCLK_GATE_D);
> @@ -691,7 +698,7 @@ bool intel_dsi_init(struct drm_device *dev)
>   	intel_encoder->pre_pll_enable = intel_dsi_pre_pll_enable;
>   	intel_encoder->pre_enable = intel_dsi_pre_enable;
>   	intel_encoder->enable = intel_dsi_enable_nop;
> -	intel_encoder->disable = intel_dsi_disable;
> +	intel_encoder->disable = intel_dsi_disable_nop;

intel_encoder->disable = intel_dsi_pre_disable
as suggested above

>   	intel_encoder->post_disable = intel_dsi_post_disable;
>   	intel_encoder->get_hw_state = intel_dsi_get_hw_state;
>   	intel_encoder->get_config = intel_dsi_get_config;
>

Regards
Shobhit

Re: [PATCH] drm/i915: dsi: fix pipe-off timeout due to port vs. pipe disable ordering

[Imre Deak]

[-- Attachment #1.1: Type: text/plain, Size: 2827 bytes --]

On Tue, 2014-05-27 at 19:45 +0530, Kumar, Shobhit wrote:
> On 5/27/2014 6:45 PM, Imre Deak wrote:
> > If we disable first the port (by disabling DPI) and only then the
> > display pipe the pipe-off flag will never be set, possibly leading to a
> > hanged pipe state at the next modeset-enable.
> >
> > Note that according to the VLV2 display cluster HAS, we should disable
> > the port before the pipe. This doesn't seem to match reality based on
> > the above and it's also asymmetric with the enabling sequence, where we
> > first enable the port and then the pipe.
> >
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >   drivers/gpu/drm/i915/intel_dsi.c | 9 ++++++++-
> >   1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> > index e73bec6..3756af1 100644
> > --- a/drivers/gpu/drm/i915/intel_dsi.c
> > +++ b/drivers/gpu/drm/i915/intel_dsi.c
> > @@ -206,6 +206,11 @@ static void intel_dsi_enable_nop(struct intel_encoder *encoder)
> >   	 */
> >   }
> >
> > +static void intel_dsi_disable_nop(struct intel_encoder *encoder)
> > +{
> > +	DRM_DEBUG_KMS("\n");
> > +}
> > +
> 
> Make this as *_pre_disable and the SHUTDOWN command should be sent in 
> this function. That is the recommended sequence in disable path. First 
> thing is to shutdown the panel. Remove that bit from the intel_dsi_disable.

I tried this, but it results in a "Timeout waiting for DPI FIFO empty"
error when sending the SHUTDOWN command. This order would be also
asymmetric with the enabling sequence where we first turn on the panel
and only then enable the pipe. So I would need more explanation for the
above before moving the SHUTDOWN command earlier.

--Imre 

> >   static void intel_dsi_disable(struct intel_encoder *encoder)
> >   {
> >   	struct drm_device *dev = encoder->base.dev;
> > @@ -293,6 +298,8 @@ static void intel_dsi_post_disable(struct intel_encoder *encoder)
> >
> >   	DRM_DEBUG_KMS("\n");
> >
> > +	intel_dsi_disable(encoder);
> > +
> >   	intel_dsi_clear_device_ready(encoder);
> >
> >   	val = I915_READ(DSPCLK_GATE_D);
> > @@ -691,7 +698,7 @@ bool intel_dsi_init(struct drm_device *dev)
> >   	intel_encoder->pre_pll_enable = intel_dsi_pre_pll_enable;
> >   	intel_encoder->pre_enable = intel_dsi_pre_enable;
> >   	intel_encoder->enable = intel_dsi_enable_nop;
> > -	intel_encoder->disable = intel_dsi_disable;
> > +	intel_encoder->disable = intel_dsi_disable_nop;
> 
> intel_encoder->disable = intel_dsi_pre_disable
> as suggested above
> 
> >   	intel_encoder->post_disable = intel_dsi_post_disable;
> >   	intel_encoder->get_hw_state = intel_dsi_get_hw_state;
> >   	intel_encoder->get_config = intel_dsi_get_config;
> >
> 
> Regards
> Shobhit


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: dsi: fix pipe-off timeout due to port vs. pipe disable ordering

[Kumar, Shobhit]

On 5/27/2014 8:42 PM, Imre Deak wrote:
> On Tue, 2014-05-27 at 19:45 +0530, Kumar, Shobhit wrote:
>> On 5/27/2014 6:45 PM, Imre Deak wrote:
>>> If we disable first the port (by disabling DPI) and only then the
>>> display pipe the pipe-off flag will never be set, possibly leading to a
>>> hanged pipe state at the next modeset-enable.
>>>
>>> Note that according to the VLV2 display cluster HAS, we should disable
>>> the port before the pipe. This doesn't seem to match reality based on
>>> the above and it's also asymmetric with the enabling sequence, where we
>>> first enable the port and then the pipe.
>>>
>>> Signed-off-by: Imre Deak <imre.deak@intel.com>
>>> ---
>>>    drivers/gpu/drm/i915/intel_dsi.c | 9 ++++++++-
>>>    1 file changed, 8 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
>>> index e73bec6..3756af1 100644
>>> --- a/drivers/gpu/drm/i915/intel_dsi.c
>>> +++ b/drivers/gpu/drm/i915/intel_dsi.c
>>> @@ -206,6 +206,11 @@ static void intel_dsi_enable_nop(struct intel_encoder *encoder)
>>>    	 */
>>>    }
>>>
>>> +static void intel_dsi_disable_nop(struct intel_encoder *encoder)
>>> +{
>>> +	DRM_DEBUG_KMS("\n");
>>> +}
>>> +
>>
>> Make this as *_pre_disable and the SHUTDOWN command should be sent in
>> this function. That is the recommended sequence in disable path. First
>> thing is to shutdown the panel. Remove that bit from the intel_dsi_disable.
>
> I tried this, but it results in a "Timeout waiting for DPI FIFO empty"
> error when sending the SHUTDOWN command. This order would be also
> asymmetric with the enabling sequence where we first turn on the panel
> and only then enable the pipe. So I would need more explanation for the
> above before moving the SHUTDOWN command earlier.
>

I think checking DPI FIFO empty before sending DPI commands is wrong. 
Unless pipe is stopped DPI FIFO will not be empty. Basically we should 
test for DPI FIFO full in case of error. I was planning to look into 
that correction.

Regards
Shobhit

Re: [PATCH] drm/i915: dsi: fix pipe-off timeout due to port vs. pipe disable ordering

[Imre Deak]

[-- Attachment #1.1: Type: text/plain, Size: 2272 bytes --]

On Tue, 2014-05-27 at 21:04 +0530, Kumar, Shobhit wrote:
> On 5/27/2014 8:42 PM, Imre Deak wrote:
> > On Tue, 2014-05-27 at 19:45 +0530, Kumar, Shobhit wrote:
> >> On 5/27/2014 6:45 PM, Imre Deak wrote:
> >>> If we disable first the port (by disabling DPI) and only then the
> >>> display pipe the pipe-off flag will never be set, possibly leading to a
> >>> hanged pipe state at the next modeset-enable.
> >>>
> >>> Note that according to the VLV2 display cluster HAS, we should disable
> >>> the port before the pipe. This doesn't seem to match reality based on
> >>> the above and it's also asymmetric with the enabling sequence, where we
> >>> first enable the port and then the pipe.
> >>>
> >>> Signed-off-by: Imre Deak <imre.deak@intel.com>
> >>> ---
> >>>    drivers/gpu/drm/i915/intel_dsi.c | 9 ++++++++-
> >>>    1 file changed, 8 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> >>> index e73bec6..3756af1 100644
> >>> --- a/drivers/gpu/drm/i915/intel_dsi.c
> >>> +++ b/drivers/gpu/drm/i915/intel_dsi.c
> >>> @@ -206,6 +206,11 @@ static void intel_dsi_enable_nop(struct intel_encoder *encoder)
> >>>    	 */
> >>>    }
> >>>
> >>> +static void intel_dsi_disable_nop(struct intel_encoder *encoder)
> >>> +{
> >>> +	DRM_DEBUG_KMS("\n");
> >>> +}
> >>> +
> >>
> >> Make this as *_pre_disable and the SHUTDOWN command should be sent in
> >> this function. That is the recommended sequence in disable path. First
> >> thing is to shutdown the panel. Remove that bit from the intel_dsi_disable.
> >
> > I tried this, but it results in a "Timeout waiting for DPI FIFO empty"
> > error when sending the SHUTDOWN command. This order would be also
> > asymmetric with the enabling sequence where we first turn on the panel
> > and only then enable the pipe. So I would need more explanation for the
> > above before moving the SHUTDOWN command earlier.
> >
> I think checking DPI FIFO empty before sending DPI commands is wrong. 
> Unless pipe is stopped DPI FIFO will not be empty. Basically we should 
> test for DPI FIFO full in case of error. I was planning to look into 
> that correction.

Ok, makes sense, I'll update the patch.

--Imre


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/i915: dsi: fix pipe-off timeout due to port vs. pipe disable ordering

[Imre Deak]

If we disable first the port (by disabling DPI) and only then the
display pipe the pipe-off flag will never be set, possibly leading to a
hanged pipe state at the next modeset-enable.

Note that according to the VLV2 display cluster HAS, we should disable
the port before the pipe. This doesn't seem to match reality based on
the above and it's also asymmetric with the enabling sequence, where we
first enable the port and then the pipe.

v2:
- send the panel shutdown command before stopping the pipe, since this
  is the recommended sequence (Shobhit)

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_dsi.c | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
index 3ea3839..b397465 100644
--- a/drivers/gpu/drm/i915/intel_dsi.c
+++ b/drivers/gpu/drm/i915/intel_dsi.c
@@ -207,14 +207,9 @@ static void intel_dsi_enable_nop(struct intel_encoder *encoder)
 	 */
 }
 
-static void intel_dsi_disable(struct intel_encoder *encoder)
+static void intel_dsi_pre_disable(struct intel_encoder *encoder)
 {
-	struct drm_device *dev = encoder->base.dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
 	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
-	int pipe = intel_crtc->pipe;
-	u32 temp;
 
 	DRM_DEBUG_KMS("\n");
 
@@ -222,7 +217,21 @@ static void intel_dsi_disable(struct intel_encoder *encoder)
 		/* Send Shutdown command to the panel in LP mode */
 		dpi_send_cmd(intel_dsi, SHUTDOWN, DPI_LP_MODE_EN);
 		msleep(10);
+	}
+}
 
+static void intel_dsi_disable(struct intel_encoder *encoder)
+{
+	struct drm_device *dev = encoder->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
+	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
+	int pipe = intel_crtc->pipe;
+	u32 temp;
+
+	DRM_DEBUG_KMS("\n");
+
+	if (is_vid_mode(intel_dsi)) {
 		/* de-assert ip_tg_enable signal */
 		temp = I915_READ(MIPI_PORT_CTRL(pipe));
 		I915_WRITE(MIPI_PORT_CTRL(pipe), temp & ~DPI_ENABLE);
@@ -294,6 +303,8 @@ static void intel_dsi_post_disable(struct intel_encoder *encoder)
 
 	DRM_DEBUG_KMS("\n");
 
+	intel_dsi_disable(encoder);
+
 	intel_dsi_clear_device_ready(encoder);
 
 	val = I915_READ(DSPCLK_GATE_D);
@@ -692,7 +703,7 @@ bool intel_dsi_init(struct drm_device *dev)
 	intel_encoder->pre_pll_enable = intel_dsi_pre_pll_enable;
 	intel_encoder->pre_enable = intel_dsi_pre_enable;
 	intel_encoder->enable = intel_dsi_enable_nop;
-	intel_encoder->disable = intel_dsi_disable;
+	intel_encoder->disable = intel_dsi_pre_disable;
 	intel_encoder->post_disable = intel_dsi_post_disable;
 	intel_encoder->get_hw_state = intel_dsi_get_hw_state;
 	intel_encoder->get_config = intel_dsi_get_config;
-- 
1.8.4

Re: [PATCH] drm/i915: dsi: fix pipe-off timeout due to port vs. pipe disable ordering

[Kumar, Shobhit]

On 5/27/2014 9:30 PM, Imre Deak wrote:
> If we disable first the port (by disabling DPI) and only then the
> display pipe the pipe-off flag will never be set, possibly leading to a
> hanged pipe state at the next modeset-enable.
>
> Note that according to the VLV2 display cluster HAS, we should disable
> the port before the pipe. This doesn't seem to match reality based on
> the above and it's also asymmetric with the enabling sequence, where we
> first enable the port and then the pipe.
>
> v2:
> - send the panel shutdown command before stopping the pipe, since this
>    is the recommended sequence (Shobhit)
>
> Signed-off-by: Imre Deak <imre.deak@intel.com>

Reviewed-by: Shobhit Kumar <shobhit.kumar@intel.com>

Regards
Shobhit

> ---
>   drivers/gpu/drm/i915/intel_dsi.c | 25 ++++++++++++++++++-------
>   1 file changed, 18 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> index 3ea3839..b397465 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.c
> +++ b/drivers/gpu/drm/i915/intel_dsi.c
> @@ -207,14 +207,9 @@ static void intel_dsi_enable_nop(struct intel_encoder *encoder)
>   	 */
>   }
>
> -static void intel_dsi_disable(struct intel_encoder *encoder)
> +static void intel_dsi_pre_disable(struct intel_encoder *encoder)
>   {
> -	struct drm_device *dev = encoder->base.dev;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
>   	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
> -	int pipe = intel_crtc->pipe;
> -	u32 temp;
>
>   	DRM_DEBUG_KMS("\n");
>
> @@ -222,7 +217,21 @@ static void intel_dsi_disable(struct intel_encoder *encoder)
>   		/* Send Shutdown command to the panel in LP mode */
>   		dpi_send_cmd(intel_dsi, SHUTDOWN, DPI_LP_MODE_EN);
>   		msleep(10);
> +	}
> +}
>
> +static void intel_dsi_disable(struct intel_encoder *encoder)
> +{
> +	struct drm_device *dev = encoder->base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
> +	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
> +	int pipe = intel_crtc->pipe;
> +	u32 temp;
> +
> +	DRM_DEBUG_KMS("\n");
> +
> +	if (is_vid_mode(intel_dsi)) {
>   		/* de-assert ip_tg_enable signal */
>   		temp = I915_READ(MIPI_PORT_CTRL(pipe));
>   		I915_WRITE(MIPI_PORT_CTRL(pipe), temp & ~DPI_ENABLE);
> @@ -294,6 +303,8 @@ static void intel_dsi_post_disable(struct intel_encoder *encoder)
>
>   	DRM_DEBUG_KMS("\n");
>
> +	intel_dsi_disable(encoder);
> +
>   	intel_dsi_clear_device_ready(encoder);
>
>   	val = I915_READ(DSPCLK_GATE_D);
> @@ -692,7 +703,7 @@ bool intel_dsi_init(struct drm_device *dev)
>   	intel_encoder->pre_pll_enable = intel_dsi_pre_pll_enable;
>   	intel_encoder->pre_enable = intel_dsi_pre_enable;
>   	intel_encoder->enable = intel_dsi_enable_nop;
> -	intel_encoder->disable = intel_dsi_disable;
> +	intel_encoder->disable = intel_dsi_pre_disable;
>   	intel_encoder->post_disable = intel_dsi_post_disable;
>   	intel_encoder->get_hw_state = intel_dsi_get_hw_state;
>   	intel_encoder->get_config = intel_dsi_get_config;
>