Re: [PATCH] drm/i915/bxt: Save/Restore Backlight registers when PG0 is gated

[Jani Nikula <jani.nikula@linux.intel.com>]

On Sat, 02 Jan 2016, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> On Thu, 31 Dec 2015, Vidya Srinivas <vidya.srinivas@intel.com> wrote:
>> Currently Backlight registers which are associated with Power Well 0
>> are not being saved before gating the power well for S0ix. Hence,
>> upon resume from S0ix, these registers are not being restored. Due to
>> this, the display has resumed and since there is no backlight, nothing is
>> seen. Patch fixes this issue by saving/restoring BLC registers for S0ix.
>
> No, this is not the approach.
>
> The backlight disable/enable hooks should handle this by always writing
> the full state of the registers on enable. Doing blind register copies
> (and read-modify-writes) is what we've been trying to get rid of across
> the driver.

To be more specific, we *already* write the full state to the three
registers in bxt_enable_backlight(). Your conclusions in the commit
message are wrong. If you're seeing issues, it must be about the
sequence in which the registers are written. Please look into that.

BR,
Jani.

>
> BR,
> Jani.
>
>
>>
>> Signed-off-by: A.Sunil Kamath <sunil.kamath@intel.com>
>> Signed-off-by: Vidya Srinivas <vidya.srinivas@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_drv.h   |  4 ++++
>>  drivers/gpu/drm/i915/intel_panel.c | 18 ++++++++++++++++++
>>  2 files changed, 22 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 50f83d2..752fb58 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -193,6 +193,10 @@ struct intel_panel {
>>  				      uint32_t hz);
>>  		void (*power)(struct intel_connector *, bool enable);
>>  	} backlight;
>> +
>> +	u32 blc_pwm_ctl;
>> +	u32 blc_pwm_freq;
>> +	u32 blc_pwm_duty;
>>  };
>>  
>>  struct intel_connector {
>> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
>> index ae808b6..421cd3a 100644
>> --- a/drivers/gpu/drm/i915/intel_panel.c
>> +++ b/drivers/gpu/drm/i915/intel_panel.c
>> @@ -816,6 +816,16 @@ static void bxt_disable_backlight(struct intel_connector *connector)
>>  		val &= ~UTIL_PIN_ENABLE;
>>  		I915_WRITE(UTIL_PIN_CTL, val);
>>  	}
>> +
>> +	/* Saving BLC registers for PG0 gating */
>> +	panel->blc_pwm_ctl =
>> +		I915_READ(BXT_BLC_PWM_CTL(panel->backlight.controller));
>> +	panel->blc_pwm_freq =
>> +		I915_READ(BXT_BLC_PWM_FREQ(panel->backlight.controller));
>> +	panel->blc_pwm_duty =
>> +		I915_READ(BXT_BLC_PWM_DUTY(panel->backlight.controller));
>> +
>> +
>>  }
>>  
>>  static void pwm_disable_backlight(struct intel_connector *connector)
>> @@ -1050,6 +1060,14 @@ static void bxt_enable_backlight(struct intel_connector *connector)
>>  	enum pipe pipe = intel_get_pipe_from_connector(connector);
>>  	u32 pwm_ctl, val;
>>  
>> +	/* Restore BLC registers if PG0 was gated */
>> +	I915_WRITE(BXT_BLC_PWM_CTL(panel->backlight.controller),
>> +				panel->blc_pwm_ctl);
>> +	I915_WRITE(BXT_BLC_PWM_FREQ(panel->backlight.controller),
>> +				panel->blc_pwm_freq);
>> +	I915_WRITE(BXT_BLC_PWM_DUTY(panel->backlight.controller),
>> +				panel->blc_pwm_duty);
>> +
>>  	/* To use 2nd set of backlight registers, utility pin has to be
>>  	 * enabled with PWM mode.
>>  	 * The field should only be changed when the utility pin is disabled

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx