diff mbox

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

Message ID 1432895827-5185-12-git-send-email-gaurav.k.singh@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Gaurav K Singh May 29, 2015, 10:37 a.m. UTC 
During enable sequence for MIPI encoder in command mode, enable
MIPI display self-refresh mode bit in Pipe Ctrl reg.

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 |   15 +++++++++++++++
 1 file changed, 15 insertions(+)

Comments

Daniel Vetter May 29, 2015, 5:21 p.m. UTC | #1 
On Fri, May 29, 2015 at 04:07:03PM +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.
> 
> 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 |   15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index cab2ac8..fc84313 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -44,6 +44,7 @@
>  #include <drm/drm_plane_helper.h>
>  #include <drm/drm_rect.h>
>  #include <linux/dma_remapping.h>
> +#include "intel_dsi.h"
>  
>  /* Primary plane formats supported by all gen */
>  #define COMMON_PRIMARY_FORMATS \
> @@ -2110,6 +2111,8 @@ static void intel_enable_pipe(struct intel_crtc *crtc)
>  {
>  	struct drm_device *dev = crtc->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_encoder *encoder;
> +	struct intel_dsi *intel_dsi;
>  	enum pipe pipe = crtc->pipe;
>  	enum transcoder cpu_transcoder = intel_pipe_to_cpu_transcoder(dev_priv,
>  								      pipe);
> @@ -2154,6 +2157,18 @@ static void intel_enable_pipe(struct intel_crtc *crtc)
>  		return;
>  	}
>  
> +	for_each_encoder_on_crtc(dev, &crtc->base, encoder) {
> +		if (encoder->type == INTEL_OUTPUT_DSI) {
> +			intel_dsi = enc_to_intel_dsi(&encoder->base);
> +			if (intel_dsi && (intel_dsi->operation_mode ==
> +			    INTEL_DSI_COMMAND_MODE)) {
> +				val = val | PIPECONF_MIPI_DSR_ENABLE;
> +				I915_WRITE(reg, val);
> +			}
> +			break;
> +		}
> +	}

When we have these kind of encoder/crtc state depencies we resolve them by
adding a bit of state to intel_crtc_state which is set as needed in the
encoder's compute_config callback. Then all you need here is

	if (intel_state->dsi_self_refresh)
		val |= PIPECONF_MIPI_DSR_ENABLE;

Also is that additional write really required?
-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
Mohan Marimuthu, Yogesh June 13, 2015, 6:54 a.m. UTC | #2 
On 5/29/2015 10:51 PM, Daniel Vetter wrote:
> On Fri, May 29, 2015 at 04:07:03PM +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.
>>
>> 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 |   15 +++++++++++++++
>>   1 file changed, 15 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index cab2ac8..fc84313 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -44,6 +44,7 @@
>>   #include <drm/drm_plane_helper.h>
>>   #include <drm/drm_rect.h>
>>   #include <linux/dma_remapping.h>
>> +#include "intel_dsi.h"
>>   
>>   /* Primary plane formats supported by all gen */
>>   #define COMMON_PRIMARY_FORMATS \
>> @@ -2110,6 +2111,8 @@ static void intel_enable_pipe(struct intel_crtc *crtc)
>>   {
>>   	struct drm_device *dev = crtc->base.dev;
>>   	struct drm_i915_private *dev_priv = dev->dev_private;
>> +	struct intel_encoder *encoder;
>> +	struct intel_dsi *intel_dsi;
>>   	enum pipe pipe = crtc->pipe;
>>   	enum transcoder cpu_transcoder = intel_pipe_to_cpu_transcoder(dev_priv,
>>   								      pipe);
>> @@ -2154,6 +2157,18 @@ static void intel_enable_pipe(struct intel_crtc *crtc)
>>   		return;
>>   	}
>>   
>> +	for_each_encoder_on_crtc(dev, &crtc->base, encoder) {
>> +		if (encoder->type == INTEL_OUTPUT_DSI) {
>> +			intel_dsi = enc_to_intel_dsi(&encoder->base);
>> +			if (intel_dsi && (intel_dsi->operation_mode ==
>> +			    INTEL_DSI_COMMAND_MODE)) {
>> +				val = val | PIPECONF_MIPI_DSR_ENABLE;
>> +				I915_WRITE(reg, val);
>> +			}
>> +			break;
>> +		}
>> +	}
> When we have these kind of encoder/crtc state depencies we resolve them by
> adding a bit of state to intel_crtc_state which is set as needed in the
> encoder's compute_config callback. Then all you need here is
>
> 	if (intel_state->dsi_self_refresh)
> 		val |= PIPECONF_MIPI_DSR_ENABLE;
>
> Also is that additional write really required?
> -Daniel
Yes additional write is required. MIPI_DSR_ENABLE has to be written 
first then followed
by pipe enable. if MIPI_DSR_ENABLE and pipe enable is done in same 
write, observed
that the image from pipe is not sent to panel when issued mem write command.

Having a state variable instead of looping through the encoders 
definitely looks good.
Need to find a place to update the state variable. I will get back on this.
>> +
>>   	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 15, 2015, 10:33 a.m. UTC | #3 
On Sat, Jun 13, 2015 at 12:24:57PM +0530, Mohan Marimuthu, Yogesh wrote:
> 
> 
> On 5/29/2015 10:51 PM, Daniel Vetter wrote:
> >On Fri, May 29, 2015 at 04:07:03PM +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.
> >>
> >>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 |   15 +++++++++++++++
> >>  1 file changed, 15 insertions(+)
> >>
> >>diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >>index cab2ac8..fc84313 100644
> >>--- a/drivers/gpu/drm/i915/intel_display.c
> >>+++ b/drivers/gpu/drm/i915/intel_display.c
> >>@@ -44,6 +44,7 @@
> >>  #include <drm/drm_plane_helper.h>
> >>  #include <drm/drm_rect.h>
> >>  #include <linux/dma_remapping.h>
> >>+#include "intel_dsi.h"
> >>  /* Primary plane formats supported by all gen */
> >>  #define COMMON_PRIMARY_FORMATS \
> >>@@ -2110,6 +2111,8 @@ static void intel_enable_pipe(struct intel_crtc *crtc)
> >>  {
> >>  	struct drm_device *dev = crtc->base.dev;
> >>  	struct drm_i915_private *dev_priv = dev->dev_private;
> >>+	struct intel_encoder *encoder;
> >>+	struct intel_dsi *intel_dsi;
> >>  	enum pipe pipe = crtc->pipe;
> >>  	enum transcoder cpu_transcoder = intel_pipe_to_cpu_transcoder(dev_priv,
> >>  								      pipe);
> >>@@ -2154,6 +2157,18 @@ static void intel_enable_pipe(struct intel_crtc *crtc)
> >>  		return;
> >>  	}
> >>+	for_each_encoder_on_crtc(dev, &crtc->base, encoder) {
> >>+		if (encoder->type == INTEL_OUTPUT_DSI) {
> >>+			intel_dsi = enc_to_intel_dsi(&encoder->base);
> >>+			if (intel_dsi && (intel_dsi->operation_mode ==
> >>+			    INTEL_DSI_COMMAND_MODE)) {
> >>+				val = val | PIPECONF_MIPI_DSR_ENABLE;
> >>+				I915_WRITE(reg, val);
> >>+			}
> >>+			break;
> >>+		}
> >>+	}
> >When we have these kind of encoder/crtc state depencies we resolve them by
> >adding a bit of state to intel_crtc_state which is set as needed in the
> >encoder's compute_config callback. Then all you need here is
> >
> >	if (intel_state->dsi_self_refresh)
> >		val |= PIPECONF_MIPI_DSR_ENABLE;
> >
> >Also is that additional write really required?
> >-Daniel
> Yes additional write is required. MIPI_DSR_ENABLE has to be written first
> then followed
> by pipe enable. if MIPI_DSR_ENABLE and pipe enable is done in same write,
> observed
> that the image from pipe is not sent to panel when issued mem write command.
> 
> Having a state variable instead of looping through the encoders definitely
> looks good.
> Need to find a place to update the state variable. I will get back on this.

Like I said such state is precomputed in the encoder callbacks, in this
case intel_dsi_compute_config.

Cheers, Daniel
Gaurav K Singh June 16, 2015, 5:03 p.m. UTC | #4 
On 6/15/2015 4:03 PM, Daniel Vetter wrote:
> On Sat, Jun 13, 2015 at 12:24:57PM +0530, Mohan Marimuthu, Yogesh wrote:
>>
>> On 5/29/2015 10:51 PM, Daniel Vetter wrote:
>>> On Fri, May 29, 2015 at 04:07:03PM +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.
>>>>
>>>> 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 |   15 +++++++++++++++
>>>>   1 file changed, 15 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>>> index cab2ac8..fc84313 100644
>>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>>> @@ -44,6 +44,7 @@
>>>>   #include <drm/drm_plane_helper.h>
>>>>   #include <drm/drm_rect.h>
>>>>   #include <linux/dma_remapping.h>
>>>> +#include "intel_dsi.h"
>>>>   /* Primary plane formats supported by all gen */
>>>>   #define COMMON_PRIMARY_FORMATS \
>>>> @@ -2110,6 +2111,8 @@ static void intel_enable_pipe(struct intel_crtc *crtc)
>>>>   {
>>>>   	struct drm_device *dev = crtc->base.dev;
>>>>   	struct drm_i915_private *dev_priv = dev->dev_private;
>>>> +	struct intel_encoder *encoder;
>>>> +	struct intel_dsi *intel_dsi;
>>>>   	enum pipe pipe = crtc->pipe;
>>>>   	enum transcoder cpu_transcoder = intel_pipe_to_cpu_transcoder(dev_priv,
>>>>   								      pipe);
>>>> @@ -2154,6 +2157,18 @@ static void intel_enable_pipe(struct intel_crtc *crtc)
>>>>   		return;
>>>>   	}
>>>> +	for_each_encoder_on_crtc(dev, &crtc->base, encoder) {
>>>> +		if (encoder->type == INTEL_OUTPUT_DSI) {
>>>> +			intel_dsi = enc_to_intel_dsi(&encoder->base);
>>>> +			if (intel_dsi && (intel_dsi->operation_mode ==
>>>> +			    INTEL_DSI_COMMAND_MODE)) {
>>>> +				val = val | PIPECONF_MIPI_DSR_ENABLE;
>>>> +				I915_WRITE(reg, val);
>>>> +			}
>>>> +			break;
>>>> +		}
>>>> +	}
>>> When we have these kind of encoder/crtc state depencies we resolve them by
>>> adding a bit of state to intel_crtc_state which is set as needed in the
>>> encoder's compute_config callback. Then all you need here is
>>>
>>> 	if (intel_state->dsi_self_refresh)
>>> 		val |= PIPECONF_MIPI_DSR_ENABLE;
>>>
>>> Also is that additional write really required?
>>> -Daniel
>> Yes additional write is required. MIPI_DSR_ENABLE has to be written first
>> then followed
>> by pipe enable. if MIPI_DSR_ENABLE and pipe enable is done in same write,
>> observed
>> that the image from pipe is not sent to panel when issued mem write command.
>>
>> Having a state variable instead of looping through the encoders definitely
>> looks good.
>> Need to find a place to update the state variable. I will get back on this.
> Like I said such state is precomputed in the encoder callbacks, in this
> case intel_dsi_compute_config.
>
> Cheers, Daniel
Agree with you daniel regarding state flag. Updated patch is ready, will 
upload shortly.

Regarding additional write, as Yogesh confirmed, both the writes are 
required.

With regards,
Gaurav
Daniel Vetter June 17, 2015, 11:39 a.m. UTC | #5 
On Tue, Jun 16, 2015 at 10:33:35PM +0530, Singh, Gaurav K wrote:
> 
> 
> On 6/15/2015 4:03 PM, Daniel Vetter wrote:
> >On Sat, Jun 13, 2015 at 12:24:57PM +0530, Mohan Marimuthu, Yogesh wrote:
> >>
> >>On 5/29/2015 10:51 PM, Daniel Vetter wrote:
> >>>On Fri, May 29, 2015 at 04:07:03PM +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.
> >>>>
> >>>>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 |   15 +++++++++++++++
> >>>>  1 file changed, 15 insertions(+)
> >>>>
> >>>>diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >>>>index cab2ac8..fc84313 100644
> >>>>--- a/drivers/gpu/drm/i915/intel_display.c
> >>>>+++ b/drivers/gpu/drm/i915/intel_display.c
> >>>>@@ -44,6 +44,7 @@
> >>>>  #include <drm/drm_plane_helper.h>
> >>>>  #include <drm/drm_rect.h>
> >>>>  #include <linux/dma_remapping.h>
> >>>>+#include "intel_dsi.h"
> >>>>  /* Primary plane formats supported by all gen */
> >>>>  #define COMMON_PRIMARY_FORMATS \
> >>>>@@ -2110,6 +2111,8 @@ static void intel_enable_pipe(struct intel_crtc *crtc)
> >>>>  {
> >>>>  	struct drm_device *dev = crtc->base.dev;
> >>>>  	struct drm_i915_private *dev_priv = dev->dev_private;
> >>>>+	struct intel_encoder *encoder;
> >>>>+	struct intel_dsi *intel_dsi;
> >>>>  	enum pipe pipe = crtc->pipe;
> >>>>  	enum transcoder cpu_transcoder = intel_pipe_to_cpu_transcoder(dev_priv,
> >>>>  								      pipe);
> >>>>@@ -2154,6 +2157,18 @@ static void intel_enable_pipe(struct intel_crtc *crtc)
> >>>>  		return;
> >>>>  	}
> >>>>+	for_each_encoder_on_crtc(dev, &crtc->base, encoder) {
> >>>>+		if (encoder->type == INTEL_OUTPUT_DSI) {
> >>>>+			intel_dsi = enc_to_intel_dsi(&encoder->base);
> >>>>+			if (intel_dsi && (intel_dsi->operation_mode ==
> >>>>+			    INTEL_DSI_COMMAND_MODE)) {
> >>>>+				val = val | PIPECONF_MIPI_DSR_ENABLE;
> >>>>+				I915_WRITE(reg, val);
> >>>>+			}
> >>>>+			break;
> >>>>+		}
> >>>>+	}
> >>>When we have these kind of encoder/crtc state depencies we resolve them by
> >>>adding a bit of state to intel_crtc_state which is set as needed in the
> >>>encoder's compute_config callback. Then all you need here is
> >>>
> >>>	if (intel_state->dsi_self_refresh)
> >>>		val |= PIPECONF_MIPI_DSR_ENABLE;
> >>>
> >>>Also is that additional write really required?
> >>>-Daniel
> >>Yes additional write is required. MIPI_DSR_ENABLE has to be written first
> >>then followed
> >>by pipe enable. if MIPI_DSR_ENABLE and pipe enable is done in same write,
> >>observed
> >>that the image from pipe is not sent to panel when issued mem write command.
> >>
> >>Having a state variable instead of looping through the encoders definitely
> >>looks good.
> >>Need to find a place to update the state variable. I will get back on this.
> >Like I said such state is precomputed in the encoder callbacks, in this
> >case intel_dsi_compute_config.
> >
> >Cheers, Daniel
> Agree with you daniel regarding state flag. Updated patch is ready, will
> upload shortly.
> 
> Regarding additional write, as Yogesh confirmed, both the writes are
> required.

If we _must_ have that second write, then you need a POSTING_READ and a
comment to explain this. Otherwise this extra write will get refactoring
into just one eventually. Also if you really need a 2nd write I wonder why
we don't have to wait for hw somehow to process the first one?
-Daniel
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index cab2ac8..fc84313 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -44,6 +44,7 @@ 
 #include <drm/drm_plane_helper.h>
 #include <drm/drm_rect.h>
 #include <linux/dma_remapping.h>
+#include "intel_dsi.h"
 
 /* Primary plane formats supported by all gen */
 #define COMMON_PRIMARY_FORMATS \
@@ -2110,6 +2111,8 @@  static void intel_enable_pipe(struct intel_crtc *crtc)
 {
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_encoder *encoder;
+	struct intel_dsi *intel_dsi;
 	enum pipe pipe = crtc->pipe;
 	enum transcoder cpu_transcoder = intel_pipe_to_cpu_transcoder(dev_priv,
 								      pipe);
@@ -2154,6 +2157,18 @@  static void intel_enable_pipe(struct intel_crtc *crtc)
 		return;
 	}
 
+	for_each_encoder_on_crtc(dev, &crtc->base, encoder) {
+		if (encoder->type == INTEL_OUTPUT_DSI) {
+			intel_dsi = enc_to_intel_dsi(&encoder->base);
+			if (intel_dsi && (intel_dsi->operation_mode ==
+			    INTEL_DSI_COMMAND_MODE)) {
+				val = val | PIPECONF_MIPI_DSR_ENABLE;
+				I915_WRITE(reg, val);
+			}
+			break;
+		}
+	}
+
 	I915_WRITE(reg, val | PIPECONF_ENABLE);
 	POSTING_READ(reg);
 }