On Thu, 01 Jun 2017, "Pandiyan, Dhinakaran" <dhinakaran.pandi...@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.pandi...@intel.com>
> Cc: intel-gfx@lists.freedesktop.org; Nikula, Jani <jani.nik...@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.nik...@intel.com>
>> > Suggested-by: Jani Nikula <jani.nik...@intel.com>
>> > Suggested-by: Ville Syrjala <ville.syrj...@intel.com>
>> > Cc: Ville Syrjala <ville.syrj...@linux.intel.com>
>> > Cc: Jani Nikula <jani.nik...@intel.com>
>> > Signed-off-by: Anusha Srivatsa <anusha.sriva...@intel.com>
>> > Signed-off-by: Rodrigo Vivi <rodrigo.v...@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
