drm/i915: Add DPI FIFO empty status check

Submitted by Deepak M on Dec. 9, 2015, 11:35 a.m.

Details

Message ID 1449660952-25222-1-git-send-email-m.deepak@intel.com
State New
Headers show
Series "Series without cover letter" ( rev: 3 ) in Intel GFX

Not browsing as part of any series.

Commit Message

Deepak M Dec. 9, 2015, 11:35 a.m.
From: Gaurav K Singh <gaurav.k.singh@intel.com>

Before sending TURN ON packet,check the DPI
FIFO empty status.

v2: Change in commit message
    Checking for FIFO empty  only during TURN ON packet.
v3: Adding a new function for DPI FIFO empty check

Signed-off-by: Gaurav K Singh <gaurav.k.singh@intel.com>
Signed-off-by: Deepak M <m.deepak@intel.com>
---
 drivers/gpu/drm/i915/intel_dsi.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
index 170ae6f..eff982b 100644
--- a/drivers/gpu/drm/i915/intel_dsi.c
+++ b/drivers/gpu/drm/i915/intel_dsi.c
@@ -60,6 +60,17 @@  static void wait_for_dsi_fifo_empty(struct intel_dsi *intel_dsi, enum port port)
 		DRM_ERROR("DPI FIFOs are not empty\n");
 }
 
+static void wait_for_dpi_fifo_empty(struct intel_dsi *intel_dsi, enum port port)
+{
+	struct drm_encoder *encoder = &intel_dsi->base.base;
+	struct drm_device *dev = encoder->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	if (wait_for((I915_READ(MIPI_GEN_FIFO_STAT(port)) & DPI_FIFO_EMPTY)
+					== DPI_FIFO_EMPTY, 50))
+		DRM_ERROR("Timeout waiting for DPI FIFO empty\n");
+}
+
 static void write_data(struct drm_i915_private *dev_priv, u32 reg,
 		       const u8 *data, u32 len)
 {
@@ -443,8 +454,10 @@  static void intel_dsi_enable(struct intel_encoder *encoder)
 			I915_WRITE(MIPI_MAX_RETURN_PKT_SIZE(port), 8 * 4);
 	} else {
 		msleep(20); /* XXX */
-		for_each_dsi_port(port, intel_dsi->ports)
+		for_each_dsi_port(port, intel_dsi->ports) {
+			wait_for_dpi_fifo_empty(intel_dsi, port);
 			dpi_send_cmd(intel_dsi, TURN_ON, false, port);
+		}
 		msleep(100);
 
 		drm_panel_enable(intel_dsi->panel);

Comments

On Wed, Dec 09, 2015 at 05:05:52PM +0530, Deepak M wrote:
> From: Gaurav K Singh <gaurav.k.singh@intel.com>
> 
> Before sending TURN ON packet,check the DPI
> FIFO empty status.
> 
> v2: Change in commit message
>     Checking for FIFO empty  only during TURN ON packet.
> v3: Adding a new function for DPI FIFO empty check
> 
> Signed-off-by: Gaurav K Singh <gaurav.k.singh@intel.com>
> Signed-off-by: Deepak M <m.deepak@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dsi.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> index 170ae6f..eff982b 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.c
> +++ b/drivers/gpu/drm/i915/intel_dsi.c
> @@ -60,6 +60,17 @@ static void wait_for_dsi_fifo_empty(struct intel_dsi *intel_dsi, enum port port)
>  		DRM_ERROR("DPI FIFOs are not empty\n");
>  }
>  
> +static void wait_for_dpi_fifo_empty(struct intel_dsi *intel_dsi, enum port port)
> +{
> +	struct drm_encoder *encoder = &intel_dsi->base.base;
> +	struct drm_device *dev = encoder->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;

Passing in intel_dsi just to dig out the dev_priv again seems a bit
pointless. Could just pass dev_priv directly. But I guess it matches the
already existing functions. 

BTW for future reference, we would rather see to_i915(<whatever>)
instead of <whatever>->dev_private.

Anyway, the patch seems to do what it says. I can't really judge
whether it's the right thing to do though.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> +
> +	if (wait_for((I915_READ(MIPI_GEN_FIFO_STAT(port)) & DPI_FIFO_EMPTY)
> +					== DPI_FIFO_EMPTY, 50))
> +		DRM_ERROR("Timeout waiting for DPI FIFO empty\n");
> +}
> +
>  static void write_data(struct drm_i915_private *dev_priv, u32 reg,
>  		       const u8 *data, u32 len)
>  {
> @@ -443,8 +454,10 @@ static void intel_dsi_enable(struct intel_encoder *encoder)
>  			I915_WRITE(MIPI_MAX_RETURN_PKT_SIZE(port), 8 * 4);
>  	} else {
>  		msleep(20); /* XXX */
> -		for_each_dsi_port(port, intel_dsi->ports)
> +		for_each_dsi_port(port, intel_dsi->ports) {
> +			wait_for_dpi_fifo_empty(intel_dsi, port);
>  			dpi_send_cmd(intel_dsi, TURN_ON, false, port);
> +		}
>  		msleep(100);
>  
>  		drm_panel_enable(intel_dsi->panel);
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, Dec 09, 2015 at 04:34:54PM +0200, Ville Syrjälä wrote:
> On Wed, Dec 09, 2015 at 05:05:52PM +0530, Deepak M wrote:
> > From: Gaurav K Singh <gaurav.k.singh@intel.com>
> > 
> > Before sending TURN ON packet,check the DPI
> > FIFO empty status.
> > 
> > v2: Change in commit message
> >     Checking for FIFO empty  only during TURN ON packet.
> > v3: Adding a new function for DPI FIFO empty check
> > 
> > Signed-off-by: Gaurav K Singh <gaurav.k.singh@intel.com>
> > Signed-off-by: Deepak M <m.deepak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dsi.c | 15 ++++++++++++++-
> >  1 file changed, 14 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> > index 170ae6f..eff982b 100644
> > --- a/drivers/gpu/drm/i915/intel_dsi.c
> > +++ b/drivers/gpu/drm/i915/intel_dsi.c
> > @@ -60,6 +60,17 @@ static void wait_for_dsi_fifo_empty(struct intel_dsi *intel_dsi, enum port port)
> >  		DRM_ERROR("DPI FIFOs are not empty\n");
> >  }
> >  
> > +static void wait_for_dpi_fifo_empty(struct intel_dsi *intel_dsi, enum port port)
> > +{
> > +	struct drm_encoder *encoder = &intel_dsi->base.base;
> > +	struct drm_device *dev = encoder->dev;
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> 
> Passing in intel_dsi just to dig out the dev_priv again seems a bit
> pointless. Could just pass dev_priv directly. But I guess it matches the
> already existing functions. 
> 
> BTW for future reference, we would rather see to_i915(<whatever>)
> instead of <whatever>->dev_private.

Yeah I think fixing up the above would be good. We're pretty close to
being able to embed our device structure.
-Daniel

> 
> Anyway, the patch seems to do what it says. I can't really judge
> whether it's the right thing to do though.
> 
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> > +
> > +	if (wait_for((I915_READ(MIPI_GEN_FIFO_STAT(port)) & DPI_FIFO_EMPTY)
> > +					== DPI_FIFO_EMPTY, 50))
> > +		DRM_ERROR("Timeout waiting for DPI FIFO empty\n");
> > +}
> > +
> >  static void write_data(struct drm_i915_private *dev_priv, u32 reg,
> >  		       const u8 *data, u32 len)
> >  {
> > @@ -443,8 +454,10 @@ static void intel_dsi_enable(struct intel_encoder *encoder)
> >  			I915_WRITE(MIPI_MAX_RETURN_PKT_SIZE(port), 8 * 4);
> >  	} else {
> >  		msleep(20); /* XXX */
> > -		for_each_dsi_port(port, intel_dsi->ports)
> > +		for_each_dsi_port(port, intel_dsi->ports) {
> > +			wait_for_dpi_fifo_empty(intel_dsi, port);
> >  			dpi_send_cmd(intel_dsi, TURN_ON, false, port);
> > +		}
> >  		msleep(100);
> >  
> >  		drm_panel_enable(intel_dsi->panel);
> > -- 
> > 1.9.1
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, 09 Dec 2015, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Wed, Dec 09, 2015 at 05:05:52PM +0530, Deepak M wrote:
>> From: Gaurav K Singh <gaurav.k.singh@intel.com>
>> 
>> Before sending TURN ON packet,check the DPI
>> FIFO empty status.
>> 
>> v2: Change in commit message
>>     Checking for FIFO empty  only during TURN ON packet.
>> v3: Adding a new function for DPI FIFO empty check
>> 
>> Signed-off-by: Gaurav K Singh <gaurav.k.singh@intel.com>
>> Signed-off-by: Deepak M <m.deepak@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_dsi.c | 15 ++++++++++++++-
>>  1 file changed, 14 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
>> index 170ae6f..eff982b 100644
>> --- a/drivers/gpu/drm/i915/intel_dsi.c
>> +++ b/drivers/gpu/drm/i915/intel_dsi.c
>> @@ -60,6 +60,17 @@ static void wait_for_dsi_fifo_empty(struct intel_dsi *intel_dsi, enum port port)
>>  		DRM_ERROR("DPI FIFOs are not empty\n");
>>  }
>>  
>> +static void wait_for_dpi_fifo_empty(struct intel_dsi *intel_dsi, enum port port)
>> +{
>> +	struct drm_encoder *encoder = &intel_dsi->base.base;
>> +	struct drm_device *dev = encoder->dev;
>> +	struct drm_i915_private *dev_priv = dev->dev_private;
>
> Passing in intel_dsi just to dig out the dev_priv again seems a bit
> pointless. Could just pass dev_priv directly. But I guess it matches the
> already existing functions. 
>
> BTW for future reference, we would rather see to_i915(<whatever>)
> instead of <whatever>->dev_private.
>
> Anyway, the patch seems to do what it says. I can't really judge
> whether it's the right thing to do though.
>
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

OTOH,

commit a799a9780eb5c874d9d7ca0bbee66401ca98c013
Author: Shobhit Kumar <shobhit.kumar@intel.com>
Date:   Thu Jul 3 16:35:40 2014 +0530

    drm/i915/vlv: DPI FIFO empty check is not needed
    
    While sending DPI SHUTDOWN command, we cannot wait for FIFO empty as
    pipes are not disabled at that time. In case of MIPI we disable port
    first and send SHUTDOWN command while pipe is still running and FIFOs
    will not be empty, causing spurious error log
    
    Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
    Tested-by: Chris Wilson <chris@chris-wilson.co.uk>
    Signed-off-by: Jani Nikula <jani.nikula@intel.com>


>
>> +
>> +	if (wait_for((I915_READ(MIPI_GEN_FIFO_STAT(port)) & DPI_FIFO_EMPTY)
>> +					== DPI_FIFO_EMPTY, 50))
>> +		DRM_ERROR("Timeout waiting for DPI FIFO empty\n");
>> +}
>> +
>>  static void write_data(struct drm_i915_private *dev_priv, u32 reg,
>>  		       const u8 *data, u32 len)
>>  {
>> @@ -443,8 +454,10 @@ static void intel_dsi_enable(struct intel_encoder *encoder)
>>  			I915_WRITE(MIPI_MAX_RETURN_PKT_SIZE(port), 8 * 4);
>>  	} else {
>>  		msleep(20); /* XXX */
>> -		for_each_dsi_port(port, intel_dsi->ports)
>> +		for_each_dsi_port(port, intel_dsi->ports) {
>> +			wait_for_dpi_fifo_empty(intel_dsi, port);
>>  			dpi_send_cmd(intel_dsi, TURN_ON, false, port);
>> +		}
>>  		msleep(100);
>>  
>>  		drm_panel_enable(intel_dsi->panel);
>> -- 
>> 1.9.1
>> 
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx