Re: [Intel-gfx] [PATCH] drm/i915/backlight: fix CPU mode backlight takeover on LPT

[Lyude Paul <lyude@redhat.com>]

Reviewed-by: Lyude Paul <lyude@redhat.com>

Let me know when you've pushed this upstream and I'll go ahead and send out a
rebased version of my backlight series.

On Fri, 2021-01-08 at 17:28 +0200, Jani Nikula wrote:
> The pch_get_backlight(), lpt_get_backlight(), and lpt_set_backlight()
> functions operate directly on the hardware registers. If inverting the
> value is needed, using intel_panel_compute_brightness(), it should only
> be done in the interface between hardware registers and
> panel->backlight.level.
> 
> The CPU mode takeover code added in commit 5b1ec9ac7ab5
> ("drm/i915/backlight: Fix backlight takeover on LPT, v3.") reads the
> hardware register and converts to panel->backlight.level correctly,
> however the value written back should remain in the hardware register
> "domain".
> 
> This hasn't been an issue, because GM45 machines are the only known
> users of i915.invert_brightness and the brightness invert quirk, and
> without one of them no conversion is made. It's likely nobody's ever hit
> the problem.
> 
> Fixes: 5b1ec9ac7ab5 ("drm/i915/backlight: Fix backlight takeover on LPT, v3.")
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Lyude Paul <lyude@redhat.com>
> Cc: <stable@vger.kernel.org> # v5.1+
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_panel.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_panel.c
> b/drivers/gpu/drm/i915/display/intel_panel.c
> index 67f81ae995c4..7a4239d1c241 100644
> --- a/drivers/gpu/drm/i915/display/intel_panel.c
> +++ b/drivers/gpu/drm/i915/display/intel_panel.c
> @@ -1649,16 +1649,13 @@ static int lpt_setup_backlight(struct intel_connector
> *connector, enum pipe unus
>                 val = pch_get_backlight(connector);
>         else
>                 val = lpt_get_backlight(connector);
> -       val = intel_panel_compute_brightness(connector, val);
> -       panel->backlight.level = clamp(val, panel->backlight.min,
> -                                      panel->backlight.max);
>  
>         if (cpu_mode) {
>                 drm_dbg_kms(&dev_priv->drm,
>                             "CPU backlight register was enabled, switching to
> PCH override\n");
>  
>                 /* Write converted CPU PWM value to PCH override register */
> -               lpt_set_backlight(connector->base.state, panel-
> >backlight.level);
> +               lpt_set_backlight(connector->base.state, val);
>                 intel_de_write(dev_priv, BLC_PWM_PCH_CTL1,
>                                pch_ctl1 | BLM_PCH_OVERRIDE_ENABLE);
>  
> @@ -1666,6 +1663,10 @@ static int lpt_setup_backlight(struct intel_connector
> *connector, enum pipe unus
>                                cpu_ctl2 & ~BLM_PWM_ENABLE);
>         }
>  
> +       val = intel_panel_compute_brightness(connector, val);
> +       panel->backlight.level = clamp(val, panel->backlight.min,
> +                                      panel->backlight.max);
> +
>         return 0;
>  }
>  

-- 
Sincerely,
   Lyude Paul (she/her)
   Software Engineer at Red Hat
   
Note: I deal with a lot of emails and have a lot of bugs on my plate. If you've
asked me a question, are waiting for a review/merge on a patch, etc. and I
haven't responded in a while, please feel free to send me another email to check
on my status. I don't bite!

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