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

[Ajay kumar]

ping.

On Thu, May 15, 2014 at 5:10 PM, Ajay kumar <ajaynumb at gmail.com> wrote:
> 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