[Freedreno] [DPU PATCH v2 2/2] drm/panel: add backlight control support for truly panel

Wed Apr 18 22:08:50 UTC 2018

Thanks Daniel and Sean for your comments.

Yes, the magic and algorithm is in userspace.

After this discussion, it seems like the complexity of the userspace 
having to
figure out which device node to use and to scale the backlight 
accordingly is not
a strong enough reason to handle this in the driver as it seems like 
there are other
opensource userspace clients doing the same thing.

I will submit another patchset to use the method suggested by Bjorne.

Thanks

Abhinav
On 2018-04-18 06:42, Sean Paul wrote:
> On Wed, Apr 18, 2018 at 11:52:18AM +0100, Daniel Thompson wrote:
>> On Tue, Apr 17, 2018 at 05:42:04PM -0700, abhinavk at codeaurora.org 
>> wrote:
>> > Adding another point.
>> >
>> > On 2018-04-17 17:04, abhinavk at codeaurora.org wrote:
>> > > Hi Bjorn
>> > >
>> > > Apologies if the prev reply wasnt clear.
>> > >
>> > > Hope this one is.
>> > >
>> > > reply inline.
>> > >
>> > > On 2018-04-17 14:29, Bjorn Andersson wrote:
>> > > > On Tue 17 Apr 11:21 PDT 2018, abhinavk at codeaurora.org wrote:
>> > > > > On 2018-04-16 23:13, Bjorn Andersson wrote:
>> > > > [..]
>> > > > > > If the panel isn't actually a piece of backlight hardware then it should
>> > > > > > not register a backlight device. Why do you need your own sysfs?
>> > > > > >
>> > > > > > Regards,
>> > > > > > Bjorn
>> > > > > [Abhinav] This particular panel isnt a piece of backlight hardware.
>> > > > > But, we want to have our own sysfs to give flexibility to our
>> > > > > userspace
>> > > > > to write and read stuff for its proprietary uses.
>> > > >
>> > > > Please be more specific in your replies, no one will accept code that
>> > > > "does stuff" and expecting a reviewer to spend time guessing or
>> > > > pulling
>> > > > the information out of you is a sure way to get your patches ignored.
>> > > >
>> > > > Exactly what kind of stuff are you talking about here and exactly
>> > > > which
>> > > > problem are you solving.
>> > > >
>> > > > > Thats how our downstream has been working so far and hence to
>> > > > > maintain
>> > > > > the compatibility would like to have it.
>> > > >
>> > > > Make your proprietary code work with the upstream kernel and you
>> > > > shouldn't ever have to modify it.
>> > > >
>> > > > Regards,
>> > > > Bjorn
>> > >
>> > > [Abhinav] We have a few userspace clients today for the backlight sysfs
>> > > node
>> > > which read/write directly to
>> > > "/sys/class/backlight/panel0-backlight/brightness"
>> > > When I said "stuff" I was only referring to the brightness value.
>> > > This separate sysfs node allows us to validate those userspace features
>> > > of ours
>> > > which directly edit the backlight value on our reference platform.
>> > > Since our reference platform uses this panel,hence having our own
>> > > sysfs alias helps.
>> > > Otherwise, whenever we try to use this panel with upstream code we
>> > > will have to end up
>> > > changing all those places in our userspace/framework to change the sysfs
>> > > path.
>> > > Hence we wanted to preserve that sysfs node name.
>> > > The wled device is not created under /sys/class/backlight but under
>> > > /sys/class/leds/wled.
>> > > So we will have to change the name of this node across all our
>> > > userspace.
>> > >
>> > > If this isnt a convincing reason enough to have its own sysfs node
>> > > path, I will use
>> > > your approach.
>> > >
>> > > Let me know your comments or if this is still not clear.
>> > >
>> > [Abhinav] We also might not be using the brightness value "as-it-is".
>> >
>> > We will potentially scale it up/down based on some requirements.
>> >
>> > If we do not have our own sysfs alias for this, how do we account for
>> > providing this interface for our chipset specific backlight dependent
>> > feature.
>> >
>> > Can you please comment on this?
>> 
>> Not easily. It's rather unclear what this chipset specific backlight
>> dependent feature you have alluded to is so how can we suggest how to
>> control or model it in the upstream kernel?
>> 
> 
> The code is here:
> 
> https://gitlab.freedesktop.org/seanpaul/dpu-staging/blob/mtp-squashed/drivers/gpu/drm/msm/dsi-staging/dsi_display.c#L76
> 
> AFAICT, there's nothing fancy in the kernel aside from scaling the 
> brightness
> level down twice. I assume the magic is in userspace. My initial 
> reaction was
> that the scaling factor should just be applied in userspace. Especially 
> since
> the scaling factor reduces the resolution of the backlight, and that's 
> not
> immediately obvious by looking at "brightness".
> 
> Sean
> 
> 
>> I can make a guess that is might be to do with brightness curves... 
>> but
>> I'd really prefer not to have to guess.
>> 
>> There are some problems with the current interface because it is not
>> well defined with the brightness control is linear or
>> logarithmic/perceptual (patches welcome) but for other common embedded
>> backlights (pwm_bl particularly) we expect calibration of the
>> brightness curve to be a job for the device tree (because it is a
>> property of the hardware it can be described in the DT) and there are
>> patches pending to improve this.
>> 
>> 
>> Daniel.
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel