Re: [PATCH 04/13] drm/i915/cnp: Backlight support for CNP.

["Vivi, Rodrigo" <rodrigo.vivi@intel.com>]

On Thu, 2017-06-01 at 02:15 +0000, Pandiyan, Dhinakaran wrote:
> On Tue, 2017-05-30 at 15:42 -0700, Rodrigo Vivi wrote:
> > Split out BXT and CNP's setup_backlight(),enable_backlight(),
> > disable_backlight() and hz_to_pwm() into
> > two separate functions instead of reusing BXT function.
> > 
> > Reuse set_backlight() and get_backlight() since they have
> > no reference to the utility pin.
> > 
> > v2: Reuse BXT functions with controller 0 instead of
> >     redefining it. (Jani).
> >     Use dev_priv->rawclk_freq instead of getting the value
> >     from SFUSE_STRAP.
> > v3: Avoid setup backligh controller along with hooks and
> >     fully reuse hooks setup as suggested by Jani.
> > v4: Clean up commit message.
> > v5: Implement per PCH instead per platform.
> > 
> > v6: Introduce a new function for CNP.(Jani and Ville)
> > 
> > v7: Squash the all CNP Backlight support patches into a
> > single patch. (Jani)
> > 
> > v8: Correct indentation, remove unneeded blank lines and
> > correct mail address (Jani).
> > 
> > v9: Remove unused enum pipe. (by CI)
> > 
> > Reviewed-by: Jani Nikula <jani.nikula@intel.com>
> > Suggested-by: Jani Nikula <jani.nikula@intel.com>
> > Suggested-by: Ville Syrjala <ville.syrjala@intel.com>
> > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> > Cc: Jani Nikula <jani.nikula@intel.com>
> > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_panel.c | 93 ++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 93 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> > index c8103f8..7e34a1b 100644
> > --- a/drivers/gpu/drm/i915/intel_panel.c
> > +++ b/drivers/gpu/drm/i915/intel_panel.c
> > @@ -796,6 +796,19 @@ static void bxt_disable_backlight(struct intel_connector *connector)
> >  	}
> >  }
> >  
> > +static void cnp_disable_backlight(struct intel_connector *connector)
> > +{
> > +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> > +	struct intel_panel *panel = &connector->panel;
> > +	u32 tmp;
> > +
> > +	intel_panel_actually_set_backlight(connector, 0);
> > +
> > +	tmp = I915_READ(BXT_BLC_PWM_CTL(panel->backlight.controller));
> > +	I915_WRITE(BXT_BLC_PWM_CTL(panel->backlight.controller),
> > +		   tmp & ~BXT_BLC_PWM_ENABLE);
> > +}
> > +
> >  static void pwm_disable_backlight(struct intel_connector *connector)
> >  {
> >  	struct intel_panel *panel = &connector->panel;
> > @@ -1086,6 +1099,35 @@ static void bxt_enable_backlight(struct intel_connector *connector)
> >  			pwm_ctl | BXT_BLC_PWM_ENABLE);
> >  }
> >  
> > +static void cnp_enable_backlight(struct intel_connector *connector)
> > +{
> > +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> > +	struct intel_panel *panel = &connector->panel;
> > +	u32 pwm_ctl;
> > +
> > +	pwm_ctl = I915_READ(BXT_BLC_PWM_CTL(panel->backlight.controller));
> 
> Shouldn't this be BLC_PWM_PCH_CTL1?

Not sure. Are we going to
1) redefine everything? including all registers and bit definitions that
are identical?
2) reuse part of CPU and part of old PCH?

> 
> I think reusing CPU register definitions for PCH is confusing. Even more
> so, when we already have separate definitions for PCH.

Actually the BXT backlight implementation was used on CNP. So all of
this was moved from CPU to PCH.

> BSpec specifically refers to these registers as SBLC_PWM_CTL1,
> SBLC_PWM_FREQ and SBLC_PWM_DUTY.

I believe we traditionally try to reuse registers definitions that are
there already instead of redefine everytime that spec changes the name.

For me all of this implementation is more like BXT so we should proceed
with this from this point and on and reusing as much as we can.

However during the review it was decided to not reuse directly the bxt
functions... So now I'm not sure in which point we should stop
duplicating code anymore... 

> 
> 
> 
> 
> > +	if (pwm_ctl & BXT_BLC_PWM_ENABLE) {
> > +		DRM_DEBUG_KMS("backlight already enabled\n");
> > +		pwm_ctl &= ~BXT_BLC_PWM_ENABLE;
> > +		I915_WRITE(BXT_BLC_PWM_CTL(panel->backlight.controller),
> > +			   pwm_ctl);
> > +	}
> > +
> > +	I915_WRITE(BXT_BLC_PWM_FREQ(panel->backlight.controller),
> > +		   panel->backlight.max);
> > +
> > +	intel_panel_actually_set_backlight(connector, panel->backlight.level);
> > +
> > +	pwm_ctl = 0;
> > +	if (panel->backlight.active_low_pwm)
> > +		pwm_ctl |= BXT_BLC_PWM_POLARITY;
> > +
> > +	I915_WRITE(BXT_BLC_PWM_CTL(panel->backlight.controller), pwm_ctl);
> > +	POSTING_READ(BXT_BLC_PWM_CTL(panel->backlight.controller));
> > +	I915_WRITE(BXT_BLC_PWM_CTL(panel->backlight.controller),
> > +		   pwm_ctl | BXT_BLC_PWM_ENABLE);
> > +}
> > +
> >  static void pwm_enable_backlight(struct intel_connector *connector)
> >  {
> >  	struct intel_panel *panel = &connector->panel;
> > @@ -1250,6 +1292,18 @@ void intel_backlight_device_unregister(struct intel_connector *connector)
> >  #endif /* CONFIG_BACKLIGHT_CLASS_DEVICE */
> >  
> >  /*
> > + * CNP: PWM clock frequency is 19.2 MHz or 24 MHz.
> > + *      Value is found in SFUSE_STRAP.
> > + *      PWM increment = 1
> > + */
> > +static u32 cnp_hz_to_pwm(struct intel_connector *connector, u32 pwm_freq_hz)
> > +{
> > +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> > +
> > +	return DIV_ROUND_CLOSEST(KHz(dev_priv->rawclk_freq), pwm_freq_hz);
> > +}
> > +
> > +/*
> >   * BXT: PWM clock frequency = 19.2 MHz.
> >   */
> >  static u32 bxt_hz_to_pwm(struct intel_connector *connector, u32 pwm_freq_hz)
> > @@ -1644,6 +1698,37 @@ static int vlv_setup_backlight(struct intel_connector *connector, enum pipe pipe
> >  	return 0;
> >  }
> >  
> > +static int
> > +cnp_setup_backlight(struct intel_connector *connector, enum pipe unused)
> > +{
> > +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> > +	struct intel_panel *panel = &connector->panel;
> > +	u32 pwm_ctl, val;
> > +
> > +	panel->backlight.controller = dev_priv->vbt.backlight.controller;
> 
> Are there are two controllers in CNP? I did not find any references in
> BSpec, can you please confirm?

We only have one controller on CNP. We might have more coming soon, but
for the current CNP platforms there is only one controller and I do
believe we could just ignore VBT for now. But question now is:

1. Leave as is.

2. set this panel->backlight.controller = 0; and move on.

3. Use BLC_PWM_PCH_CTL1 directly and  BXT_BLC_PWM_FREQ(0) and _DUTY(0)

4. Use BLC_PWM_PCH_CTL1 directly and reimplement SBLC_PWM_FREQ and
SBLC_PWM_DUTY.

5. Reimplement SBLC_PWM_CTL1, SBLC_PWM_FREQ and SBLC_PWM_DUTY.

Although 4 or 5 seems to be the right way to go now I'd like to
highlight that maybe next PCH can be more like BXT than like CNP. So we
would need to reimplement everything again.

I honestly don't mind which option we go. My position was to fully reuse
BXT as I had initially done. But that was nacked. So please just let me
know the direction that I provide the patch ;)

Thanks,
Rodrigo.

> 
> 
> > +
> > +	pwm_ctl = I915_READ(BXT_BLC_PWM_CTL(panel->backlight.controller));
> > +
> > +	panel->backlight.active_low_pwm = pwm_ctl & BXT_BLC_PWM_POLARITY;
> > +	panel->backlight.max =
> > +		I915_READ(BXT_BLC_PWM_FREQ(panel->backlight.controller));
> > +
> > +	if (!panel->backlight.max)
> > +		panel->backlight.max = get_backlight_max_vbt(connector);
> > +
> > +	if (!panel->backlight.max)
> > +		return -ENODEV;
> > +
> > +	val = bxt_get_backlight(connector);
> > +	val = intel_panel_compute_brightness(connector, val);
> > +	panel->backlight.level = clamp(val, panel->backlight.min,
> > +				       panel->backlight.max);
> > +
> > +	panel->backlight.enabled = pwm_ctl & BXT_BLC_PWM_ENABLE;
> > +
> > +	return 0;
> > +}
> > +
> >  static int pwm_setup_backlight(struct intel_connector *connector,
> >  			       enum pipe pipe)
> >  {
> > @@ -1760,6 +1845,14 @@ void intel_panel_destroy_backlight(struct drm_connector *connector)
> >  		panel->backlight.set = bxt_set_backlight;
> >  		panel->backlight.get = bxt_get_backlight;
> >  		panel->backlight.hz_to_pwm = bxt_hz_to_pwm;
> > +		panel->backlight.hz_to_pwm = bxt_hz_to_pwm;
> ^Spurious line.
> 
> > +	} else if (HAS_PCH_CNP(dev_priv)) {
> > +		panel->backlight.setup = cnp_setup_backlight;
> > +		panel->backlight.enable = cnp_enable_backlight;
> > +		panel->backlight.disable = cnp_disable_backlight;
> > +		panel->backlight.set = bxt_set_backlight;
> > +		panel->backlight.get = bxt_get_backlight;
> > +		panel->backlight.hz_to_pwm = cnp_hz_to_pwm;
> >  	} else if (HAS_PCH_LPT(dev_priv) || HAS_PCH_SPT(dev_priv) ||
> >  		   HAS_PCH_KBP(dev_priv)) {
> >  		panel->backlight.setup = lpt_setup_backlight;
> 

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx