From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jesse Barnes Subject: Re: [PATCH 1/3] drm/i915: add PWM and BLC assertion checks Date: Tue, 1 Apr 2014 09:13:52 -0700 Message-ID: <20140401091352.3b579029@jbarnes-desktop> References: <1396289637-1013-1-git-send-email-jbarnes@virtuousgeek.org> <878urpecu6.fsf@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from gproxy5-pub.mail.unifiedlayer.com (gproxy5-pub.mail.unifiedlayer.com [67.222.38.55]) by gabe.freedesktop.org (Postfix) with SMTP id 68F4C6E770 for ; Tue, 1 Apr 2014 09:12:48 -0700 (PDT) In-Reply-To: <878urpecu6.fsf@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Jani Nikula Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Tue, 01 Apr 2014 10:19:29 +0300 Jani Nikula wrote: > On Mon, 31 Mar 2014, Jesse Barnes wrote: > > To make sure we properly follow the enable/disable sequences. > > > > Signed-off-by: Jesse Barnes > > --- > > 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