[RFC,11/14] drm/i915: Enable MIPI display self refresh mode
diff mbox

Message ID 1434664605-20419-1-git-send-email-gaurav.k.singh@intel.com
State New
Headers show

Commit Message

Singh, Gaurav K June 18, 2015, 9:56 p.m. UTC
During enable sequence for MIPI encoder in command mode, enable
MIPI display self-refresh mode bit in Pipe Ctrl reg.

v2: Use crtc state flag instead of loop over encoders (Daniel)

Signed-off-by: Gaurav K Singh <gaurav.k.singh@intel.com>
Signed-off-by: Yogesh Mohan Marimuthu <yogesh.mohan.marimuthu@intel.com>
Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |    5 +++++
 1 file changed, 5 insertions(+)

Comments

Daniel Vetter June 22, 2015, 12:05 p.m. UTC | #1
On Fri, Jun 19, 2015 at 03:26:45AM +0530, Gaurav K Singh wrote:
> During enable sequence for MIPI encoder in command mode, enable
> MIPI display self-refresh mode bit in Pipe Ctrl reg.
> 
> v2: Use crtc state flag instead of loop over encoders (Daniel)
> 
> Signed-off-by: Gaurav K Singh <gaurav.k.singh@intel.com>
> Signed-off-by: Yogesh Mohan Marimuthu <yogesh.mohan.marimuthu@intel.com>
> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c |    5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index dd518d6..c53f66d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2158,6 +2158,11 @@ static void intel_enable_pipe(struct intel_crtc *crtc)
>  		return;
>  	}
>  
> +	if (crtc->config->dsi_self_refresh) {
> +		val = val | PIPECONF_MIPI_DSR_ENABLE;
> +		I915_WRITE(reg, val);
> +	}

Ah here it is. Please squash this patch with patch 7 so that you introduce
the state tracking and the user for the new dsi_self_refresh bit in one
patch. Makes reviewing a lot easier. Also please add a comment here that
the additional write is required, and enforce ordering with a
POSTING_READ.

Thanks, Daniel

> +
>  	I915_WRITE(reg, val | PIPECONF_ENABLE);
>  	POSTING_READ(reg);
>  }
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Daniel Vetter June 22, 2015, 12:08 p.m. UTC | #2
On Mon, Jun 22, 2015 at 02:05:51PM +0200, Daniel Vetter wrote:
> On Fri, Jun 19, 2015 at 03:26:45AM +0530, Gaurav K Singh wrote:
> > During enable sequence for MIPI encoder in command mode, enable
> > MIPI display self-refresh mode bit in Pipe Ctrl reg.
> > 
> > v2: Use crtc state flag instead of loop over encoders (Daniel)
> > 
> > Signed-off-by: Gaurav K Singh <gaurav.k.singh@intel.com>
> > Signed-off-by: Yogesh Mohan Marimuthu <yogesh.mohan.marimuthu@intel.com>
> > Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c |    5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index dd518d6..c53f66d 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -2158,6 +2158,11 @@ static void intel_enable_pipe(struct intel_crtc *crtc)
> >  		return;
> >  	}
> >  
> > +	if (crtc->config->dsi_self_refresh) {
> > +		val = val | PIPECONF_MIPI_DSR_ENABLE;
> > +		I915_WRITE(reg, val);
> > +	}
> 
> Ah here it is. Please squash this patch with patch 7 so that you introduce
> the state tracking and the user for the new dsi_self_refresh bit in one
> patch. Makes reviewing a lot easier. Also please add a comment here that
> the additional write is required, and enforce ordering with a
> POSTING_READ.

And I just realized that I've written the same review in the discussion of
the previous patch. Please make sure next time around you address all the
outstanding review, since if you don't the reviewer has to paintstakingly
check _everything_ every time around, which is a massive waste of
everyone's time.

Also can you pls change the subject to include DSI (and drop the MIPI
part, that's just the standard's group and not the standard itself)?
Applies to a few other patches in this series too.

Thanks, Daniel

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index dd518d6..c53f66d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2158,6 +2158,11 @@  static void intel_enable_pipe(struct intel_crtc *crtc)
 		return;
 	}
 
+	if (crtc->config->dsi_self_refresh) {
+		val = val | PIPECONF_MIPI_DSR_ENABLE;
+		I915_WRITE(reg, val);
+	}
+
 	I915_WRITE(reg, val | PIPECONF_ENABLE);
 	POSTING_READ(reg);
 }