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

[abhinavk-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org]

Hi Bjorn

Thanks for the comments.

I will upload a new patch for this which will be dependent on 
https://patchwork.kernel.org/patch/10377537/ series.

This series registers the backlight device we need.

Thanks

Abhinav

On 2018-04-18 21:42, Bjorn Andersson wrote:
> On Wed 18 Apr 21:23 PDT 2018, abhinavk@codeaurora.org wrote:
> 
>> Hi Bjorn
>> 
> 
> Hi Abhinav,
> 
>> Thanks very much for the detailed response.
>> 
> 
> You're welcome.
> 
>> Yes, we decided that userspace hardcoding this node name is not a
>> strong enough reason to register as another backlight device.
>> 
>> Had one follow up question though.
>> 
>> The QC WLED driver, drivers/leds/leds-qpnp-wled.c is not registering 
>> itself
>> as a backlight device.
>> 
>> It only registers as a led device.
>> 
>> In that case, we cannot invoke of_find_backlight_by_node to get a 
>> handle on
>> it.
>> 
>> One question we have is , is it required that every WLED driver 
>> register
>> itself as a backlight device too?
>> 
>> In this case since it is not doing so, but we need to trigger it for 
>> the
>> backlight.
>> 
>> Is there any way we can do this?
>> 
> 
> It seems I answered this in private, so let me summarize my answer for
> the record.
> 
> The downstream driver for the Qualcomm WLED registers a LED, but the
> WLED driver should register a backlight device. There is a driver
> (drivers/video/backlight/pm8941-wled.c) that does that for the WLED
> version found in PM8941, so the appropriate solution to this problem is
> to extend that to support the PMIC you're looking at.
> 
>> OR shall we just abandon the backlight control out of this driver 
>> entirely.
>> 
> 
> The backlight handling in the panel driver serves the purpose of
> blanking and unblanking the associated backlight device, given the
> status of the panel. And this is still considered valuable.
> 
> It does sounds like a reasonable idea to extend this to also cover
> brightness management, but as you might guess this becomes a larger and
> completely generic issue.
> 
> Regards,
> Bjorn
> 
>> Thanks
>> 
>> Abhinav
>> 
>> On 2018-04-18 21:00, Bjorn Andersson wrote:
>> > On Tue 17 Apr 17:04 PDT 2018, abhinavk@codeaurora.org wrote:
>> >
>> > > Hi Bjorn
>> > >
>> > > Apologies if the prev reply wasnt clear.
>> > >
>> > > Hope this one is.
>> > >
>> >
>> > Much better, now we can discuss the actual issues :)
>> >
>> > > reply inline.
>> > >
>> > > On 2018-04-17 14:29, Bjorn Andersson wrote:
>> > > > On Tue 17 Apr 11:21 PDT 2018, abhinavk@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.
>> >
>> > That's nice, but you're enforcing that either all panel drivers must
>> > implement this backlight wrapper or that your customers must modify
>> > their user space to match their backlight implementation.
>> >
>> > > 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.
>> >
>> > The actual problem comes down to "how does user space know the name of
>> > the backlight instance associated with the panel" and this is a valid
>> > question to pursue.
>> >
>> > But given your current design you could just scan for the one and only
>> > backlight device available in the system; as your use of the static name
>> > "panel0-backlight" doesn't allow multiple backlights anyway.
>> >
>> >
>> > If your goal is simply to ship your reference with something that you
>> > can show work, then just replace the hard coded panel0-backlight with
>> > the name of the wled backlight device. Customers can change panels as
>> > they wish, but in the event that they plug in a different backlight
>> > controller they would need to modify the code.
>> >
>> > > 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.
>> > >
>> >
>> > Hard coding /sys/class/backlight/panel0-backlight in your user space
>> > instead of /sys/class/leds/wled is hardly a gain, in particular since
>> > the cost is 94 insertions - per panel driver.
>> >
>> > Regards,
>> > Bjorn
>> > _______________________________________________
>> > Freedreno mailing list
>> > Freedreno@lists.freedesktop.org
>> > https://lists.freedesktop.org/mailman/listinfo/freedreno
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno