Re: [PATCH] backlight: pwm_bl: configure pwm only once per backlight toggle

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Oct 17, 2019 at 8:05 AM Thierry Reding <thierry.reding@xxxxxxxxx> wrote:
>
> On Thu, Oct 17, 2019 at 07:40:47AM -0500, Adam Ford wrote:
> > On Thu, Oct 17, 2019 at 7:34 AM Adam Ford <aford173@xxxxxxxxx> wrote:
> > >
> > > On Thu, Oct 17, 2019 at 7:19 AM Uwe Kleine-König
> > > <u.kleine-koenig@xxxxxxxxxxxxxx> wrote:
> > > >
> > > > On Thu, Oct 17, 2019 at 12:47:27PM +0100, Daniel Thompson wrote:
> > > > > On Thu, Oct 17, 2019 at 10:10:59AM +0200, Uwe Kleine-König wrote:
> > > > > > A previous change in the pwm core (namely 01ccf903edd6 ("pwm: Let
> > > > > > pwm_get_state() return the last implemented state")) changed the
> > > > > > semantic of pwm_get_state() and disclosed an (as it seems) common
> > > > > > problem in lowlevel PWM drivers. By not relying on the period and duty
> > > > > > cycle being retrievable from a disabled PWM this type of problem is
> > > > > > worked around.
> > > > > >
> > > > > > Apart from this issue only calling the pwm_get_state/pwm_apply_state
> > > > > > combo once is also more effective.
> > > > >
> > > > > I'm only interested in the second paragraph here.
> > > > >
> > > > > There seems to be a reasonable consensus that the i.MX27 and cros-ec
> > > > > PWM drivers should be fixed for the benefit of other PWM clients.
> > > > > So we make this change because it makes the pwm-bl better... not to
> > > > > work around bugs ;-).
> > > >
> > > > That's fine, still I think it's fair to explain the motivation of
> > > > creating this patch.
> > > >
> > > > > > diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
> > > > > > index 746eebc411df..ddebd62b3978 100644
> > > > > > --- a/drivers/video/backlight/pwm_bl.c
> > > > > > +++ b/drivers/video/backlight/pwm_bl.c
> > > > > > @@ -67,40 +62,27 @@ static void pwm_backlight_power_on(struct pwm_bl_data *pb)
> > > > > >
> > > > > >  static void pwm_backlight_power_off(struct pwm_bl_data *pb)
> > > > > >  {
> > > > > > -   struct pwm_state state;
> > > > > > -
> > > > > > -   pwm_get_state(pb->pwm, &state);
> > > > > > -   if (!pb->enabled)
> > > > > > -           return;
> > > > > > -
> > > > >
> > > > > Why remove the pb->enabled check? I thought that was there to ensure we
> > > > > don't mess up the regular reference counts.
> > > >
> > > > I havn't looked yet, but I guess I have to respin. Expect a v2 later
> > > > today.
> > >
> > > I would agree that a high-level fix is better than a series of low
> > > level driver fixes.  For what its worth, your V1 patch worked fine on
> > > my i.MX6Q.  I can test the V2 patch when its ready.
> >
> > I may have spoken too soon.  The patch fixes the display in that it
> > comes on when it previously did not, but changes to brightness do not
> > appear to do anything anymore.  I don't have an oscilloscope where I
> > am now, so I cannot verify whether or not the PWM duty cycle changes.
> >
> > To my eye, the following do not appear to change the brightness of the screen:
> >      echo 7 > /sys/devices/soc0/backlight-lvds/backlight/backlight-lvds/brightness
> >      echo 2 > /sys/devices/soc0/backlight-lvds/backlight/backlight-lvds/brightness
> >      echo 5 > /sys/devices/soc0/backlight-lvds/backlight/backlight-lvds/brightness
>
> Hi Adam,
>
> can you try the i.MX PWM patch that I posted earlier instead? I really
> think we need to fix this in the PWM drivers because they are broken.
> pwm-backlight is not. -rc3 is really not a time to work around breakage
> in consumers.

I did try your patch, but it did not appear to make any difference on my i.MX6Q.

>
> If my patch doesn't help, can you try reverting the offending patch? If
> we can't come up with a good fix for the drivers, reverting is the next
> best option.

I am able to get an image after reverting the offending patch.  I did
not try playing with brightness levels after reverting.


>
> Thierry
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux