Re: [PATCH 1/3] drm/i915: add PWM and BLC assertion checks

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

On Tue, 01 Apr 2014, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> On Tue, 01 Apr 2014 10:19:29 +0300
> Jani Nikula <jani.nikula@linux.intel.com> wrote:
>
>> On Mon, 31 Mar 2014, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
>> > To make sure we properly follow the enable/disable sequences.
>> >
>> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
>> > ---
>> >  drivers/gpu/drm/i915/intel_dp.c    | 62 ++++++++++++++++++++++++++++++++++++--
>> >  drivers/gpu/drm/i915/intel_drv.h   |  1 +
>> >  drivers/gpu/drm/i915/intel_panel.c |  5 ++-
>> >  3 files changed, 65 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> > index bf73771..b6f7087 100644
>> > --- a/drivers/gpu/drm/i915/intel_dp.c
>> > +++ b/drivers/gpu/drm/i915/intel_dp.c
>> > @@ -301,6 +301,20 @@ static u32 _pp_stat_reg(struct intel_dp *intel_dp)
>> >  		return VLV_PIPE_PP_STATUS(vlv_power_sequencer_pipe(intel_dp));
>> >  }
>> >  
>> > +static void assert_pwm(struct intel_connector *connector,
>> > +		       bool expected_state)
>> > +{
>> > +	bool state;
>> > +
>> > +	state = intel_panel_get_backlight(connector);
>> 
>> If the duty cycle is regarded as a binary on/off, I'd rather add an
>> additional "is enabled" call to intel_panel.c. Especially so because the
>> duty cycle value returned by intel_panel_get_backlight is meaningless
>> without the max value.
>
> Hm I guess that would be cleaner; for my purposes I thought any
> non-zero PWM duty cycle would be sufficient, but of course other checks
> are needed as well, like whether the PWM enable bit is on, and checks
> against the BLC_EN bit in the PP regs, but those are logically
> separate.  is_enabled might better map back to the PWM_EN bit rather
> than a non-zero duty cycle though.

We could add intel_panel_backlight_enabled() call that returns
connector->panel.backlight.enabled.

BR,
Jani.






>
>> >  
>> > +	if (I915_READ(VLV_BLC_PWM_CTL2(pipe) & BLM_PWM_ENABLE))
>> > +		return 0;
>> > +
>> 
>> If our internal state is consistent, I don't think this should be
>> necessary. And if our internal state isn't consistent, we should fix
>> that and maybe add internal asserts within intel_panel.c.
>
> Yeah this could be covered with other asserts as long as we have them
> in all the right places.
>
> -- 
> Jesse Barnes, Intel Open Source Technology Center

-- 
Jani Nikula, Intel Open Source Technology Center