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

[Jesse Barnes <jbarnes@virtuousgeek.org>]

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.

> >  
> > +	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