diff mbox series

drm/i915/display: Fixed a screen flickering when turning on display from off

Message ID 20240306031348.1344034-1-gareth.yu@intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915/display: Fixed a screen flickering when turning on display from off | expand

Commit Message

Yu, Gareth March 6, 2024, 3:13 a.m. UTC 
From: Gareth Yu <gareth.yu@intel.com>

Turn on the panel from zero brightness of the last state, the panel was set
a maximum PWM in the flow. Once the panel initialization is completed, the
backlight is restored to zero brightness. There is a flckering generated.

Set the brightness to the minimum value when the brightness is less or equal
to the minimum value to fix this flickering

Cc : Tejas Upadhyay <tejaskumarx.surendrakumar.upadhyay@intel.com>
Cc : Matt Roper <matthew.d.roper@intel.com>
Cc : Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Gareth Yu <gareth.yu@intel.com>
---
 drivers/gpu/drm/i915/display/intel_backlight.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Jani Nikula March 6, 2024, 10:19 a.m. UTC | #1 
On Wed, 06 Mar 2024, gareth.yu@intel.com wrote:
> From: Gareth Yu <gareth.yu@intel.com>
>
> Turn on the panel from zero brightness of the last state, the panel was set
> a maximum PWM in the flow. Once the panel initialization is completed, the
> backlight is restored to zero brightness. There is a flckering generated.

Please be more precise in describing what exactly happens and
when. Driver probe? Modeset? What restores backlight to zero brightness?

Better yet, please file a bug at fdo gitlab, attach full dmesg with
debugs, etc.

Before we had the concept of minimum brightness, the minimum was always
0. And the check was:

	if (level == 0)
		level = max;

Historically, the point was, if you're enabling the display and
backlight, you don't want it to be at 0 brightness, because for most
displays that means a black screen.

BR,
Jani.


> Set the brightness to the minimum value when the brightness is less or equal
> to the minimum value to fix this flickering
>
> Cc : Tejas Upadhyay <tejaskumarx.surendrakumar.upadhyay@intel.com>
> Cc : Matt Roper <matthew.d.roper@intel.com>
> Cc : Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Gareth Yu <gareth.yu@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_backlight.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_backlight.c b/drivers/gpu/drm/i915/display/intel_backlight.c
> index 3f3cd944a1c5..855d6ead905c 100644
> --- a/drivers/gpu/drm/i915/display/intel_backlight.c
> +++ b/drivers/gpu/drm/i915/display/intel_backlight.c
> @@ -762,7 +762,7 @@ static void __intel_backlight_enable(const struct intel_crtc_state *crtc_state,
>  	WARN_ON(panel->backlight.max == 0);
>  
>  	if (panel->backlight.level <= panel->backlight.min) {
> -		panel->backlight.level = panel->backlight.max;
> +		panel->backlight.level = panel->backlight.min;
>  		if (panel->backlight.device)
>  			panel->backlight.device->props.brightness =
>  				scale_hw_to_user(connector,
Ville Syrjälä March 15, 2024, 11:02 a.m. UTC | #2 
On Wed, Mar 06, 2024 at 12:19:42PM +0200, Jani Nikula wrote:
> On Wed, 06 Mar 2024, gareth.yu@intel.com wrote:
> > From: Gareth Yu <gareth.yu@intel.com>
> >
> > Turn on the panel from zero brightness of the last state, the panel was set
> > a maximum PWM in the flow. Once the panel initialization is completed, the
> > backlight is restored to zero brightness. There is a flckering generated.
> 
> Please be more precise in describing what exactly happens and
> when. Driver probe? Modeset? What restores backlight to zero brightness?
> 
> Better yet, please file a bug at fdo gitlab, attach full dmesg with
> debugs, etc.
> 
> Before we had the concept of minimum brightness, the minimum was always
> 0. And the check was:
> 
> 	if (level == 0)
> 		level = max;
> 
> Historically, the point was, if you're enabling the display and
> backlight, you don't want it to be at 0 brightness, because for most
> displays that means a black screen.

I think that hack was originally added becaue some silly
userspace thingy was setting the backlight level to 0 on
suspend/etc. and then forgetting to restore it back to a
sane value afterwards. Dunno if that nonsense behaviour
still persists to this day.
Yu, Gareth March 18, 2024, 2:20 a.m. UTC | #3 
The original implementation was 47356eb67285 in Sep 17, 2001. The condition can't be replicated for now. 

The condition for invalid brightness level includes the minimum available value. It does not make sense. I've added the new condition in rev #5. Please consider if the condition is workable.

-       if (panel->backlight.level <= panel->backlight.min) {
+       if (panel->backlight.level < panel->backlight.min) {

Thanks,
Gareth

-----Original Message-----
From: Ville Syrjälä <ville.syrjala@linux.intel.com> 
Sent: Friday, March 15, 2024 7:03 PM
To: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Yu, Gareth <gareth.yu@intel.com>; intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/i915/display: Fixed a screen flickering when turning on display from off

On Wed, Mar 06, 2024 at 12:19:42PM +0200, Jani Nikula wrote:
> On Wed, 06 Mar 2024, gareth.yu@intel.com wrote:
> > From: Gareth Yu <gareth.yu@intel.com>
> >
> > Turn on the panel from zero brightness of the last state, the panel 
> > was set a maximum PWM in the flow. Once the panel initialization is 
> > completed, the backlight is restored to zero brightness. There is a flckering generated.
> 
> Please be more precise in describing what exactly happens and when. 
> Driver probe? Modeset? What restores backlight to zero brightness?
> 
> Better yet, please file a bug at fdo gitlab, attach full dmesg with 
> debugs, etc.
> 
> Before we had the concept of minimum brightness, the minimum was 
> always 0. And the check was:
> 
> 	if (level == 0)
> 		level = max;
> 
> Historically, the point was, if you're enabling the display and 
> backlight, you don't want it to be at 0 brightness, because for most 
> displays that means a black screen.

I think that hack was originally added becaue some silly userspace thingy was setting the backlight level to 0 on suspend/etc. and then forgetting to restore it back to a sane value afterwards. Dunno if that nonsense behaviour still persists to this day.

--
Ville Syrjälä
Intel
Dolan Liu March 18, 2024, 7:58 a.m. UTC | #4 
this case have been there so many years, it's time to fix it if 
possible. And user-space software may improved by themselves in we 
didn't realize place.

even if not,  for the proof user-space setting  0, it's better to change to


    if (level < min || level == 0 )

         level =max;


Intel default FSP will set the default min is 2% (6/255). if someone 
missed the setting, it will be keep the default and level.min will be 
larger than 0.

if someone changed the default min in VBT or coreboot, the user-space 
lowest level set as 0, still can go though to this logic.


whatever, we think this one should correct back, otherwise it will keep 
occurring in each new kernel release on all Intel device, this is not 
very friendly to all developers.

and the only fix way is  hack patch to remove "level=max".


On 3/15/24 19:02, Ville Syrjälä wrote:
> On Wed, Mar 06, 2024 at 12:19:42PM +0200, Jani Nikula wrote:
>> On Wed, 06 Mar 2024, gareth.yu@intel.com wrote:
>>> From: Gareth Yu <gareth.yu@intel.com>
>>>
>>> Turn on the panel from zero brightness of the last state, the panel was set
>>> a maximum PWM in the flow. Once the panel initialization is completed, the
>>> backlight is restored to zero brightness. There is a flckering generated.
>> Please be more precise in describing what exactly happens and
>> when. Driver probe? Modeset? What restores backlight to zero brightness?
>>
>> Better yet, please file a bug at fdo gitlab, attach full dmesg with
>> debugs, etc.
>>
>> Before we had the concept of minimum brightness, the minimum was always
>> 0. And the check was:
>>
>> 	if (level == 0)
>> 		level = max;
>>
>> Historically, the point was, if you're enabling the display and
>> backlight, you don't want it to be at 0 brightness, because for most
>> displays that means a black screen.
> I think that hack was originally added becaue some silly
> userspace thingy was setting the backlight level to 0 on
> suspend/etc. and then forgetting to restore it back to a
> sane value afterwards. Dunno if that nonsense behaviour
> still persists to this day.
>
Jani Nikula March 18, 2024, 1:45 p.m. UTC | #5 
On Mon, 18 Mar 2024, Dolan Liu <liuyong5@huaqin.corp-partner.google.com> wrote:
> this case have been there so many years, it's time to fix it if 
> possible. And user-space software may improved by themselves in we 
> didn't realize place.
>
> even if not,  for the proof user-space setting  0, it's better to change to
>
>
>     if (level < min || level == 0 )
>
>          level =max;
>
>
> Intel default FSP will set the default min is 2% (6/255). if someone 
> missed the setting, it will be keep the default and level.min will be 
> larger than 0.
>
> if someone changed the default min in VBT or coreboot, the user-space 
> lowest level set as 0, still can go though to this logic.
>
>
> whatever, we think this one should correct back, otherwise it will keep 
> occurring in each new kernel release on all Intel device, this is not 
> very friendly to all developers.
>
> and the only fix way is  hack patch to remove "level=max".

As far as the change goes, the original patch is pretty much the only
one we should consider. I only ever asked for 1) an issue to be reported
at fdo gitlab, and 2) a commit message properly detailing the issue.

I never asked for functional changes in the patch. Frankly, all the
alternative versions are nonsense.

I think we can try to go with the patch, but please understand that if
it regresses some silly userspace, the change will be reverted instead
of fixing that silly userspace, because that's the rule in kernel
development.


BR,
Jani.


PS. Please try to reply inline instead of top-posting in public mailing
lists.



>
>
> On 3/15/24 19:02, Ville Syrjälä wrote:
>> On Wed, Mar 06, 2024 at 12:19:42PM +0200, Jani Nikula wrote:
>>> On Wed, 06 Mar 2024, gareth.yu@intel.com wrote:
>>>> From: Gareth Yu <gareth.yu@intel.com>
>>>>
>>>> Turn on the panel from zero brightness of the last state, the panel was set
>>>> a maximum PWM in the flow. Once the panel initialization is completed, the
>>>> backlight is restored to zero brightness. There is a flckering generated.
>>> Please be more precise in describing what exactly happens and
>>> when. Driver probe? Modeset? What restores backlight to zero brightness?
>>>
>>> Better yet, please file a bug at fdo gitlab, attach full dmesg with
>>> debugs, etc.
>>>
>>> Before we had the concept of minimum brightness, the minimum was always
>>> 0. And the check was:
>>>
>>> 	if (level == 0)
>>> 		level = max;
>>>
>>> Historically, the point was, if you're enabling the display and
>>> backlight, you don't want it to be at 0 brightness, because for most
>>> displays that means a black screen.
>> I think that hack was originally added becaue some silly
>> userspace thingy was setting the backlight level to 0 on
>> suspend/etc. and then forgetting to restore it back to a
>> sane value afterwards. Dunno if that nonsense behaviour
>> still persists to this day.
>>
Yu, Gareth March 19, 2024, 2:04 a.m. UTC | #6 
https://patchwork.freedesktop.org/patch/582411/?series=130780&rev=2 provides the scenario and the details of DMESG in the commit message on rev#1. I'll assume we'll go with rev#2.

Thanks,
Gareth

-----Original Message-----
From: Jani Nikula <jani.nikula@linux.intel.com> 
Sent: Monday, March 18, 2024 9:45 PM
To: Dolan Liu <liuyong5@huaqin.corp-partner.google.com>; Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Yu, Gareth <gareth.yu@intel.com>; intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/i915/display: Fixed a screen flickering when turning on display from off

...

As far as the change goes, the original patch is pretty much the only one we should consider. I only ever asked for 1) an issue to be reported at fdo gitlab, and 2) a commit message properly detailing the issue.

I never asked for functional changes in the patch. Frankly, all the alternative versions are nonsense.

I think we can try to go with the patch, but please understand that if it regresses some silly userspace, the change will be reverted instead of fixing that silly userspace, because that's the rule in kernel development.


BR,
Jani.


PS. Please try to reply inline instead of top-posting in public mailing lists.
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_backlight.c b/drivers/gpu/drm/i915/display/intel_backlight.c
index 3f3cd944a1c5..855d6ead905c 100644
--- a/drivers/gpu/drm/i915/display/intel_backlight.c
+++ b/drivers/gpu/drm/i915/display/intel_backlight.c
@@ -762,7 +762,7 @@  static void __intel_backlight_enable(const struct intel_crtc_state *crtc_state,
 	WARN_ON(panel->backlight.max == 0);
 
 	if (panel->backlight.level <= panel->backlight.min) {
-		panel->backlight.level = panel->backlight.max;
+		panel->backlight.level = panel->backlight.min;
 		if (panel->backlight.device)
 			panel->backlight.device->props.brightness =
 				scale_hw_to_user(connector,