[RFC V3 2/3] drm/bridge: add a dummy panel driver to support lvds bridges

[Ajay kumar]

On Thu, May 15, 2014 at 1:43 PM, Thierry Reding
<thierry.reding at gmail.com> wrote:
> On Wed, May 14, 2014 at 11:39:30PM +0530, Ajay kumar wrote:
> [...]
>> >> >> diff --git a/Documentation/devicetree/bindings/drm/bridge/bridge_panel.txt b/Documentation/devicetree/bindings/drm/bridge/bridge_panel.txt
>> >> > [...]
>> >> >> +     -led-en-gpio:
>> >> >> +             eDP panel LED enable GPIO.
>> >> >> +                     Indicates which GPIO needs to be powered up as output
>> >> >> +                     to enable the backlight.
>> >> >
>> >> > Since this is used to control a backlight, then this should really be a
>> >> > separate node to describe the backlight device (and use the
>> >> > corresponding backlight driver) rather than duplicating a subset of that
>> >> > functionality.
>> >> I am stressing this point again!
>> >> Backlight should ideally be enabled only after:
>> >> 1) Panel is powered up (LCD_VDD)
>> >> 2) HPD is thrown.
>> >> 3) Valid data starts coming from the encoder(DP in our case)
>> >> All the above 3 are controlled inside drm, but pwm-backlight is
>> >> independent of drm. So, we let the pwm_config happen in pwm-backlight driver
>> >> and enable backlight GPIO(BL_EN) inside drm, after valid video data starts
>> >> coming out of the encoder.
>> >
>> > But that's completely the wrong way around. Why can't you simply control
>> > the backlight from within DRM and only enable it at the right time?
>> I am trying to reuse existing code as much as possible. pwm-backlight driver
>> takes care of generating proper pwm signals for the given backlight value,
>> so I am reusing that part of it. Even though it also provides a way to handle
>> "backlight enable" pin(which is ofcourse optional) I wish to control it in the
>> panel drvier, because generally all panel datasheets say you should control
>> backlight enable along with panel controls.
>
> I still don't see how controlling the enable GPIO from the panel will be
> in any way better, or different for that matter, from simply enabling
> the backlight and let the backlight driver handle the enable GPIO. Have
> you tried to do that and found that it doesn't work?
It works, but it gives me glitch when I try to configure video.
This is because backlight_on(pwm-backlight) happens much before
I configure video(inside drm), and while configuring video there would
be a glitch,
and that glitch would be visible to the user since backlight is already enabled.

On the other hand, if I choose to delay enabling backlight till I am sure enough
that video is properly configured, then even though the glitch comes up, the
user cannot make it out since backlight if off - a better user experience!

>> >> As you said, we can get this GPIO from a backlight device node, but since
>> >> this GPIO is related to panel also, I think conceptually its not wrong
>> >> to have this
>> >> GPIO binding defined in a panel node.
>> >
>> > It's not related to the panel. It's an enable for the backlight.
>> > Backlight could be considered part of the panel, therefore it makes
>> > sense to control the backlight from the panel driver.
>> Ok. This GPIO goes from my AP to the backlight unit of the panel.
>> Basically, a board related GPIO which exists on all the boards.
>> Where should I be handling this?
>
> In the driver that handles the backlight unit. That is: pwm-backlight.
>
>> >> > What are panel_VCC or video_enable?
>> >> It is the time taken for the panel to throw a valid HPD from the point
>> >> we enabled LCD_VCC.
>> >> After HPD is thrown, we can start sending video data.
>> >
>> > What does "throw a valid HPD" mean? Is it an actual signal that can be
>> > captured via software (GPIO, interrupt, ...)? If so then it would be
>> > preferable to not use a delay at all but rather rely on that mechanism
>> > to determine when it's safe to send a video signal.
>> Right, your explanation holds good, but only for the case of eDP panels.
>> But, when we use eDP-LVDS bridges(ps8622), its a different case!
>> the bridge takes care of sending HPD, so the encoder gets HPD interrupt
>> and tries to read edid from the panel, but the panel might not have powered up
>> enough to read edid. In such cases we would need delay.
>> So, having a delay field here would be really useful.
>
> Okay.
>
>> May be, while describing this particular DT binding I should elaborate more.
>
> Yes, something like the above explanation would be good to have in the
> binding.
Ok, will do that.

>> >> >> diff --git a/drivers/gpu/drm/bridge/bridge_panel.c b/drivers/gpu/drm/bridge/bridge_panel.c
>> >> > [...]
>> >> >
>> >> > This duplicates much of the functionality that panels should provide. I
>> >> > think a better solution would be to properly integrate panels with
>> >> > bridges.
>> >> True, ideally I should be calling drm_panel functions from a simple dummy
>> >> bridge which sits at the end of the bridge chain. But, since you are not
>> >> convinced with having pre_enable and post_disable calls for panels, I had
>> >> no other way to do this, than stuffing these panel controls inside
>> >> bridge! :(
>> >
>> > What makes you think that I would suddenly be any more convinced by this
>> > solution than by your prior proposal? I didn't say outright no to what
>> > you proposed earlier. What I said was that I didn't like it and that I
>> > thought we could come up with a better solution. Part of getting to a
>> > better solution is trying to understand the problems involved. You don't
>> > solve a problem by simply moving code into a different driver.
>> Ok. What is better solution according to you?
>> Handling the backlight enable GPIO in pwm-backlight driver is not
>> a better soultion "ALWAYS". Sometimes, we may just need to control it
>> along with video encoders.
>
> You keep saying that, but you haven't said *why* you need to do that.
I have explained above. Also see chapter 6.5 in [1]

>> And, even the panel datasheets mention that  in "Power On/off sequence"
>> chapter.
>
> Can you point me to such a datasheet?
[1] : http://www.yslcd.com.tw/docs/product/B116XW03%20V.0.pdf
chapter 6.5

>> And, I have explained the problems and feasible solutions.
>
> You have explained the solutions that you've found to the problem. I'm
> trying to understand the problem fully so that perhaps we can find a
> more generic solution.
>
>> If drm_panel can support pre_enable/post_disable, we can easily
>> implement a generic bridge which sits at the end of bridge chain, and
>> calls corresponding drm_panel functions.
>
> The problem with blindly adding .pre_enable() and .post_disable()
> without thinking this through is that it can easily introduce
> incompatibilities between various drivers and panels. There is a risk
> that some of the code that goes into a panel's .pre_enable() will be
> specific to a given setup (and perhaps not even related to the panel
> at all). If that panel is reused on a different board where the
> restrictions may be different the panel may not work.
Ok, I understand. That's why I am pointing out to the LVDS datasheet above.
I have a similar datasheet for eDP panel B133HTN01, but its private and I am
not supposed to share it! :(

> So before we go and add any new functions to DRM panel I'd like to see
> them properly documented. It needs to be defined precisely what the
> effect of these functions should be so that both panel driver writers
> know what to implement and display driver writers need to know when to
> call which function.
Agreed, we should put in precise explanation in bindings/comments.

> Also, please let's not call them .pre_enable() and .post_disable(). The
> names should be something more specific to reflect what they are meant
> to do. Even something like .prepare() and .unprepare() would be better
> choices.
.prepare() and .unprepare() also sound good and meaningful.
powering up the LCD unit goes into prepare()
powering up the LED unit goes into enable()

Regards,
Ajay