Re: [Intel-gfx] [PATCH] drm/i915/backlight: fix CPU mode backlight takeover on LPT
On Mon, 11 Jan 2021, Jani Nikula wrote: > On Fri, 08 Jan 2021, Lyude Paul wrote: >> Reviewed-by: Lyude Paul >> >> Let me know when you've pushed this upstream and I'll go ahead and send out a >> rebased version of my backlight series. > > Pushed, thanks for the review. > > I'm hoping to do more review of the series today, so please hold off on > actually sending the rebased version for a bit longer. Done, go ahead! BR, Jani. -- Jani Nikula, Intel Open Source Graphics Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/backlight: fix CPU mode backlight takeover on LPT
On Fri, 08 Jan 2021, Lyude Paul wrote: > Reviewed-by: Lyude Paul > > Let me know when you've pushed this upstream and I'll go ahead and send out a > rebased version of my backlight series. Pushed, thanks for the review. I'm hoping to do more review of the series today, so please hold off on actually sending the rebased version for a bit longer. BR, Jani. > > 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 >> Cc: Ville Syrjälä >> Cc: Lyude Paul >> Cc: # v5.1+ >> Signed-off-by: Jani Nikula >> --- >> 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; >> } >> -- Jani Nikula, Intel Open Source Graphics Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/backlight: fix CPU mode backlight takeover on LPT
Reviewed-by: Lyude Paul 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 > Cc: Ville Syrjälä > Cc: Lyude Paul > Cc: # v5.1+ > Signed-off-by: Jani Nikula > --- > 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
[Intel-gfx] [PATCH] drm/i915/backlight: fix CPU mode backlight takeover on LPT
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 Cc: Ville Syrjälä Cc: Lyude Paul Cc: # v5.1+ Signed-off-by: Jani Nikula --- 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; } -- 2.20.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx