[Intel-gfx] [PATCH 04/13] drm/i915/cnp: Backlight support for CNP.
Jani Nikula
jani.nikula at intel.com
Fri Jun 2 07:02:37 UTC 2017
On Thu, 01 Jun 2017, "Pandiyan, Dhinakaran" <dhinakaran.pandiyan at intel.com> wrote:
> Based on your clarification the second option feels like the right
> choice, with a relevant comment in code. Like you said, we get to
> retain BXT register definitions and clarify that the register is on a
> PCH for CNP.
Ack. We can also clarify/unify the definitions later on as needed.
BR,
Jani.
>
> -DK
>
> -----Original Message-----
> From: Vivi, Rodrigo
> Sent: Thursday, June 1, 2017 9:29 AM
> To: Pandiyan, Dhinakaran <dhinakaran.pandiyan at intel.com>
> Cc: intel-gfx at lists.freedesktop.org; Nikula, Jani <jani.nikula at intel.com>
> Subject: Re: [Intel-gfx] [PATCH 04/13] drm/i915/cnp: Backlight support for CNP.
>
> 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 at intel.com>
>> > Suggested-by: Jani Nikula <jani.nikula at intel.com>
>> > Suggested-by: Ville Syrjala <ville.syrjala at intel.com>
>> > Cc: Ville Syrjala <ville.syrjala at linux.intel.com>
>> > Cc: Jani Nikula <jani.nikula at intel.com>
>> > Signed-off-by: Anusha Srivatsa <anusha.srivatsa at intel.com>
>> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi at 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;
>>
>
--
Jani Nikula, Intel Open Source Technology Center
More information about the Intel-gfx
mailing list