From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jani Nikula Subject: Re: [PATCH 1/3] drm/i915: add PWM and BLC assertion checks Date: Wed, 25 Jun 2014 15:45:03 +0300 Message-ID: <87k3856tv4.fsf@intel.com> References: <1396289637-1013-1-git-send-email-jbarnes@virtuousgeek.org> <878urpecu6.fsf@intel.com> <20140401091352.3b579029@jbarnes-desktop> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by gabe.freedesktop.org (Postfix) with ESMTP id 37ED06E685 for ; Wed, 25 Jun 2014 05:45:20 -0700 (PDT) In-Reply-To: <20140401091352.3b579029@jbarnes-desktop> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Jesse Barnes Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Tue, 01 Apr 2014, Jesse Barnes wrote: > 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. 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