Re: [PATCH v2 5/7] drm/i915: Reorganize the DSI enable/disable sequence

[Shobhit Kumar <shobhit.kumar@intel.com>]

On Friday 15 November 2013 01:57 PM, Jani Nikula wrote:
> On Sat, 09 Nov 2013, Shobhit Kumar <shobhit.kumar@intel.com> wrote:
>> Basically ULPS handling during enable/disable has been moved to
>> pre_enable and post_disable phases. PLL and panel power disable
>> also has been moved to post_disable phase. The ULPS entry/exit
>> sequneces as suggested by HW team is as follows -
>>
>> During enable time -
>> set DEVICE_READY --> Clear DEVICE_READY --> set DEVICE_READY
>>
>> And during disable time to flush all FIFOs -
>> set ENTER_SLEEP --> EXIT_SLEEP --> ENTER_SLEEP
>>
>> Also during disbale sequnece sub-encoder disable is moved to the end
>> after port is disabled.
>>
>> v2: Based on comments from Ville
>>      - Detailed epxlaination in the commit messgae
>>      - Moved parameter changes out into another patch
>>      - Backlight enabling will be a new patch
>>
>> Signed-off-by: Yogesh Mohan Marimuthu <yogesh.mohan.marimuthu@intel.com>
>> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_drv.h  |   11 ++++
>>   drivers/gpu/drm/i915/intel_dsi.c |  111 ++++++++++++++++++++++++++------------
>>   drivers/gpu/drm/i915/intel_dsi.h |    2 +
>>   3 files changed, 91 insertions(+), 33 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index a2bbff9..55c16cb 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -2433,6 +2433,17 @@ int vlv_freq_opcode(int ddr_freq, int val);
>>   #define POSTING_READ(reg)	(void)I915_READ_NOTRACE(reg)
>>   #define POSTING_READ16(reg)	(void)I915_READ16_NOTRACE(reg)
>>
>> +#define I915_WRITE_BITS(reg, val, mask) \
>> +do { \
>> +	u32 tmp, data; \
>> +	tmp = I915_READ((reg)); \
>> +	tmp &= ~(mask); \
>> +	data = (val) & (mask); \
>> +	data = data | tmp; \
>> +	I915_WRITE((reg), data); \
>> +} while(0)
>
> I would still prefer the explicit read, modify, and write in the code
> instead of this, but it's a matter of taste I'll leave for Daniel to
> call the shots on.
>
> One reason for my dislike is how easy it will be to accidentally get the
> val and mask parameters mixed up. I would instinctively put the mask
> before the value (common convention of context before the rest), which
> would be wrong here. I am not saying changing the order would make me
> like this.

As mentioned in another mail, this is not required and I will remove it

>
>> +
>> +
>>   /* "Broadcast RGB" property */
>>   #define INTEL_BROADCAST_RGB_AUTO 0
>>   #define INTEL_BROADCAST_RGB_FULL 1
>> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
>> index 8dc9a38..9e67f78 100644
>> --- a/drivers/gpu/drm/i915/intel_dsi.c
>> +++ b/drivers/gpu/drm/i915/intel_dsi.c
>> @@ -101,46 +101,59 @@ static void intel_dsi_pre_pll_enable(struct intel_encoder *encoder)
>>   	vlv_enable_dsi_pll(encoder);
>>   }
>>
>> -static void intel_dsi_pre_enable(struct intel_encoder *encoder)
>> -{
>> -	DRM_DEBUG_KMS("\n");
>> -}
>> -
>> -static void intel_dsi_enable(struct intel_encoder *encoder)
>> +void intel_dsi_device_ready(struct intel_encoder *encoder)
>>   {
>>   	struct drm_i915_private *dev_priv = encoder->base.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 (intel_dsi->dev.dev_ops->panel_reset)
>>   		intel_dsi->dev.dev_ops->panel_reset(&intel_dsi->dev);
>>
>> -	temp = I915_READ(MIPI_DEVICE_READY(pipe));
>> -	if ((temp & DEVICE_READY) == 0) {
>> -		temp &= ~ULPS_STATE_MASK;
>> -		I915_WRITE(MIPI_DEVICE_READY(pipe), temp | DEVICE_READY);
>> -	} else if (temp & ULPS_STATE_MASK) {
>> -		temp &= ~ULPS_STATE_MASK;
>> -		I915_WRITE(MIPI_DEVICE_READY(pipe), temp | ULPS_STATE_EXIT);
>> -		/*
>> -		 * We need to ensure that there is a minimum of 1 ms time
>> -		 * available before clearing the UPLS exit state.
>> -		 */
>> -		msleep(2);
>> -		I915_WRITE(MIPI_DEVICE_READY(pipe), temp);
>> -	}
>> +	I915_WRITE_BITS(MIPI_PORT_CTRL(pipe), LP_OUTPUT_HOLD, LP_OUTPUT_HOLD);
>> +	usleep_range(1000, 1500);
>> +	I915_WRITE_BITS(MIPI_DEVICE_READY(pipe), DEVICE_READY |
>> +			ULPS_STATE_EXIT, DEVICE_READY | ULPS_STATE_MASK);
>> +	usleep_range(2000, 2500);
>> +	I915_WRITE_BITS(MIPI_DEVICE_READY(pipe), DEVICE_READY,
>> +			DEVICE_READY | ULPS_STATE_MASK);
>> +	usleep_range(2000, 2500);
>> +	I915_WRITE_BITS(MIPI_DEVICE_READY(pipe), 0x00,
>> +			DEVICE_READY | ULPS_STATE_MASK);
>> +	usleep_range(2000, 2500);
>> +	I915_WRITE_BITS(MIPI_DEVICE_READY(pipe), DEVICE_READY,
>> +			DEVICE_READY | ULPS_STATE_MASK);
>
> It seems like an odd dance, but if that's what the hw folks say, I guess
> we'll just have to take their word for it...

Yeah, but I reconfirmed from HW team and this is also now updated in 
documents

>
>> +	usleep_range(2000, 2500);
>>
>>   	if (intel_dsi->dev.dev_ops->send_otp_cmds)
>>   		intel_dsi->dev.dev_ops->send_otp_cmds(&intel_dsi->dev);
>
> Maybe the hooks would better belong in intel_dsi_pre_enable, not
> here. The panel_reset hook could be renamed pre_enable while at it.
>

Ok, moved the hooks into pre_enable, but I am not too sure on changing 
the names as the current name suggests what exactly we are doing with 
the panel. If you agree I will leave that bit out but do other changes.

>>
>> +}
>> +static void intel_dsi_pre_enable(struct intel_encoder *encoder)
>> +{
>> +	DRM_DEBUG_KMS("\n");
>> +
>> +	/* put device in ready state */
>> +	intel_dsi_device_ready(encoder);
>> +}
>> +
>> +static void intel_dsi_enable(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_cmd_mode(intel_dsi))
>>   		I915_WRITE(MIPI_MAX_RETURN_PKT_SIZE(pipe), 8 * 4);
>> -
>> -	if (is_vid_mode(intel_dsi)) {
>> +	else {
>>   		msleep(20); /* XXX */
>>   		dpi_send_cmd(intel_dsi, TURN_ON);
>>   		msleep(100);
>> @@ -157,7 +170,8 @@ static void intel_dsi_enable(struct intel_encoder *encoder)
>>
>>   static void intel_dsi_disable(struct intel_encoder *encoder)
>>   {
>> -	struct drm_i915_private *dev_priv = encoder->base.dev->dev_private;
>> +	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;
>> @@ -165,8 +179,6 @@ static void intel_dsi_disable(struct intel_encoder *encoder)
>>
>>   	DRM_DEBUG_KMS("\n");
>>
>> -	intel_dsi->dev.dev_ops->disable(&intel_dsi->dev);
>> -
>>   	if (is_vid_mode(intel_dsi)) {
>>   		dpi_send_cmd(intel_dsi, SHUTDOWN);
>>   		msleep(10);
>> @@ -179,19 +191,52 @@ static void intel_dsi_disable(struct intel_encoder *encoder)
>>   		msleep(2);
>>   	}
>>
>> -	temp = I915_READ(MIPI_DEVICE_READY(pipe));
>> -	if (temp & DEVICE_READY) {
>> -		temp &= ~DEVICE_READY;
>> -		temp &= ~ULPS_STATE_MASK;
>> -		I915_WRITE(MIPI_DEVICE_READY(pipe), temp);
>> -	}
>> +	/* if disable packets are sent before sending shutdown packet then in
>> +	 * some next enable sequence send turn on packet error is observed */
>> +	if (intel_dsi->dev.dev_ops->disable)
>> +		intel_dsi->dev.dev_ops->disable(&intel_dsi->dev);
>>   }
>>
>> -static void intel_dsi_post_disable(struct intel_encoder *encoder)
>> +void intel_dsi_clear_device_ready(struct intel_encoder *encoder)
>>   {
>> +	struct drm_i915_private *dev_priv = encoder->base.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;
>> +
>>   	DRM_DEBUG_KMS("\n");
>>
>> +	I915_WRITE_BITS(MIPI_DEVICE_READY(pipe), ULPS_STATE_ENTER,
>> +							ULPS_STATE_MASK);
>> +	usleep_range(2000, 2500);
>> +
>> +	I915_WRITE_BITS(MIPI_DEVICE_READY(pipe), ULPS_STATE_EXIT,
>> +							ULPS_STATE_MASK);
>> +	usleep_range(2000, 2500);
>> +
>> +	I915_WRITE_BITS(MIPI_DEVICE_READY(pipe), ULPS_STATE_ENTER,
>> +							ULPS_STATE_MASK);
>> +	usleep_range(2000, 2500);
>> +
>> +	I915_WRITE_BITS(MIPI_PORT_CTRL(pipe), 0, LP_OUTPUT_HOLD);
>> +	usleep_range(1000, 1500);
>> +
>> +	if (wait_for(((I915_READ(MIPI_PORT_CTRL(pipe)) & 0x20000)
>> +					== 0x00000), 30))
>> +		DRM_ERROR("DSI LP not going Low\n");
>
> AFE_LATCHOUT?

Yeah, fixed it. Rap on me hardcodings :)

>
>> +
>> +	I915_WRITE_BITS(MIPI_DEVICE_READY(pipe), 0x00, DEVICE_READY);
>> +	usleep_range(2000, 2500);
>> +
>>   	vlv_disable_dsi_pll(encoder);
>> +
>> +	if (intel_dsi->dev.dev_ops->disable_panel_power)
>> +		intel_dsi->dev.dev_ops->disable_panel_power(&intel_dsi->dev);
>
> Here too, hook better suited at intel_dsi_post_disable. And call it
> post_disable.

Will move the hook but not too sure on changing the name as above

You can expect updated patches by Monday. Sorry it took me some time to 
get back to this.

Regards
Shobiht