| Message ID | 1399647182-20951-3-git-send-email-ajaykumar.rs@samsung.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Fri, May 09, 2014 at 08:23:01PM +0530, Ajay Kumar wrote: > implement basic panel controls as a drm_bridge so that > the existing bridges can make use of it. > > The driver assumes that it is the last entity in the bridge chain. > > Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com> > --- > .../bindings/drm/bridge/bridge_panel.txt | 45 ++++ Can we please stop adding files to this directory? Device tree bindings are supposed to be OS agnostic, but DRM is specific to Linux and should not be used when describing hardware. > 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. > + -panel-pre-enable-delay: > + delay value in ms required for panel_pre_enable process > + Delay in ms needed for the eDP panel LCD unit to > + powerup, and delay needed between panel_VCC and > + video_enable. What are panel_VCC or video_enable? > + -panel-enable-delay: > + delay value in ms required for panel_enable process > + Delay in ms needed for the eDP panel backlight/LED unit > + to powerup, and delay needed between video_enable and > + BL_EN. Similarily, what does BL_EN stand for? > + bridge-panel { > + compatible = "drm-bridge,panel"; Again, drm- doesn't mean anything outside of Linux (and maybe BSD), therefore shouldn't be used to describe hardware in device tree. > 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. Thierry
On Tue, May 13, 2014 at 1:35 PM, Thierry Reding <thierry.reding@gmail.com> wrote: > On Fri, May 09, 2014 at 08:23:01PM +0530, Ajay Kumar wrote: >> implement basic panel controls as a drm_bridge so that >> the existing bridges can make use of it. >> >> The driver assumes that it is the last entity in the bridge chain. >> >> Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com> >> --- >> .../bindings/drm/bridge/bridge_panel.txt | 45 ++++ > > Can we please stop adding files to this directory? Device tree bindings > are supposed to be OS agnostic, but DRM is specific to Linux and should > not be used when describing hardware. One should not be adding a devicetree node if it is not describing the actual hardware? >> 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. 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. >> + -panel-pre-enable-delay: >> + delay value in ms required for panel_pre_enable process >> + Delay in ms needed for the eDP panel LCD unit to >> + powerup, and delay needed between panel_VCC and >> + video_enable. > > 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. >> + -panel-enable-delay: >> + delay value in ms required for panel_enable process >> + Delay in ms needed for the eDP panel backlight/LED unit >> + to powerup, and delay needed between video_enable and >> + BL_EN. > > Similarily, what does BL_EN stand for? Backlight Enable, same as "enable-gpios" in pwm-backlight driver. >> + bridge-panel { >> + compatible = "drm-bridge,panel"; > > Again, drm- doesn't mean anything outside of Linux (and maybe BSD), > therefore shouldn't be used to describe hardware in device tree. Agreed. :) >> 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! :( See the discussion here. I have already explained this! http://www.spinics.net/lists/dri-devel/msg57973.html Ajay
On Tue, May 13, 2014 at 10:19:33PM +0530, Ajay kumar wrote: > On Tue, May 13, 2014 at 1:35 PM, Thierry Reding > <thierry.reding@gmail.com> wrote: > > On Fri, May 09, 2014 at 08:23:01PM +0530, Ajay Kumar wrote: > >> implement basic panel controls as a drm_bridge so that > >> the existing bridges can make use of it. > >> > >> The driver assumes that it is the last entity in the bridge chain. > >> > >> Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com> > >> --- > >> .../bindings/drm/bridge/bridge_panel.txt | 45 ++++ > > > > Can we please stop adding files to this directory? Device tree bindings > > are supposed to be OS agnostic, but DRM is specific to Linux and should > > not be used when describing hardware. > One should not be adding a devicetree node if it is not describing the > actual hardware? Correct. But my point is that even if you describe actual hardware, then the bindings don't belong in a drm subdirectory, because DRM does not make any sense in a hardware context. Something like video/bridge may be a better name for a directory. > >> 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? > 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. > >> + -panel-pre-enable-delay: > >> + delay value in ms required for panel_pre_enable process > >> + Delay in ms needed for the eDP panel LCD unit to > >> + powerup, and delay needed between panel_VCC and > >> + video_enable. > > > > 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. > >> + -panel-enable-delay: > >> + delay value in ms required for panel_enable process > >> + Delay in ms needed for the eDP panel backlight/LED unit > >> + to powerup, and delay needed between video_enable and > >> + BL_EN. > > > > Similarily, what does BL_EN stand for? > Backlight Enable, same as "enable-gpios" in pwm-backlight driver. When writing a binding it's useful to describe these things without referring to signal names or abbreviations, since they may be something else for different people. > >> 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. Thierry
On Wed, May 14, 2014 at 8:24 PM, Thierry Reding <thierry.reding@gmail.com> wrote: > On Tue, May 13, 2014 at 10:19:33PM +0530, Ajay kumar wrote: >> On Tue, May 13, 2014 at 1:35 PM, Thierry Reding >> <thierry.reding@gmail.com> wrote: >> > On Fri, May 09, 2014 at 08:23:01PM +0530, Ajay Kumar wrote: >> >> implement basic panel controls as a drm_bridge so that >> >> the existing bridges can make use of it. >> >> >> >> The driver assumes that it is the last entity in the bridge chain. >> >> >> >> Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com> >> >> --- >> >> .../bindings/drm/bridge/bridge_panel.txt | 45 ++++ >> > >> > Can we please stop adding files to this directory? Device tree bindings >> > are supposed to be OS agnostic, but DRM is specific to Linux and should >> > not be used when describing hardware. >> One should not be adding a devicetree node if it is not describing the >> actual hardware? > > Correct. But my point is that even if you describe actual hardware, then > the bindings don't belong in a drm subdirectory, because DRM does not > make any sense in a hardware context. > > Something like video/bridge may be a better name for a directory. Ok, this is just about the name. >> >> 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. >> 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? >> >> + -panel-pre-enable-delay: >> >> + delay value in ms required for panel_pre_enable process >> >> + Delay in ms needed for the eDP panel LCD unit to >> >> + powerup, and delay needed between panel_VCC and >> >> + video_enable. >> > >> > 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. May be, while describing this particular DT binding I should elaborate more. >> >> + -panel-enable-delay: >> >> + delay value in ms required for panel_enable process >> >> + Delay in ms needed for the eDP panel backlight/LED unit >> >> + to powerup, and delay needed between video_enable and >> >> + BL_EN. >> > >> > Similarily, what does BL_EN stand for? >> Backlight Enable, same as "enable-gpios" in pwm-backlight driver. > > When writing a binding it's useful to describe these things without > referring to signal names or abbreviations, since they may be something > else for different people. Ok. Will take care of this from now on :) >> >> 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. And, even the panel datasheets mention that in "Power On/off sequence" chapter. And, I have explained the problems and feasible solutions. 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. Please see the discussion happening here: https://www.mail-archive.com/linux-samsung-soc@vger.kernel.org/msg30586.html Ajay
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? > >> 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. > >> >> 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. > And, even the panel datasheets mention that in "Power On/off sequence" > chapter. Can you point me to such a datasheet? > 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. 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. 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. Thierry
On Thu, May 15, 2014 at 1:43 PM, Thierry Reding <thierry.reding@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
ping. On Thu, May 15, 2014 at 5:10 PM, Ajay kumar <ajaynumb@gmail.com> wrote: > On Thu, May 15, 2014 at 1:43 PM, Thierry Reding > <thierry.reding@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
On Thu, May 15, 2014 at 05:10:16PM +0530, Ajay kumar wrote: > On Thu, May 15, 2014 at 1:43 PM, Thierry Reding <thierry.reding@gmail.com> wrote: [...] > > 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. Okay, so this means that your backlight is turned on too early. Instead of working around that problem by moving control of the backlight enable GPIO from the backlight driver into the panel driver, the correct way to fix it is to make sure the backlight stays off until video is ready. Thierry
On Sun, May 18, 2014 at 2:44 AM, Thierry Reding <thierry.reding@gmail.com> wrote: > On Thu, May 15, 2014 at 05:10:16PM +0530, Ajay kumar wrote: >> On Thu, May 15, 2014 at 1:43 PM, Thierry Reding <thierry.reding@gmail.com> wrote: > [...] >> > 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. > > Okay, so this means that your backlight is turned on too early. Instead > of working around that problem by moving control of the backlight enable > GPIO from the backlight driver into the panel driver, the correct way to > fix it is to make sure the backlight stays off until video is ready. Ok. Please suggest an idea how to do the same! I have already suggested a simple idea which conforms to a valid spec. Just because enable_gpio is already a part of pwm_bl.c, I somewhat feel like we are forcing people to handle enable_gpio inside pwm_bl.c. Note that, pwm_bl can still work properly without enabling the backlight GPIO. And, I did point out to a valid datasheet from AUO, which clearly indicates why backlight enable GPIO should be a part of panel driver and not pwm_bl driver. I am not trying to say we should remove enable_gpio from pwm_bl. Provided that its already an optional property, and if someone wants to control it in a panel driver, what is wrong in doing so? Regards, Ajay
On Sun, May 18, 2014 at 01:50:36PM +0530, Ajay kumar wrote: > On Sun, May 18, 2014 at 2:44 AM, Thierry Reding > <thierry.reding@gmail.com> wrote: > > On Thu, May 15, 2014 at 05:10:16PM +0530, Ajay kumar wrote: > >> On Thu, May 15, 2014 at 1:43 PM, Thierry Reding <thierry.reding@gmail.com> wrote: > > [...] > >> > 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. > > > > Okay, so this means that your backlight is turned on too early. Instead > > of working around that problem by moving control of the backlight enable > > GPIO from the backlight driver into the panel driver, the correct way to > > fix it is to make sure the backlight stays off until video is ready. > Ok. Please suggest an idea how to do the same! I did post a patch[0] a long time ago that added a way to fix this for pwm-backlight at least. > I have already suggested a simple idea which conforms to a valid spec. > Just because enable_gpio is already a part of pwm_bl.c, I somewhat feel > like we are forcing people to handle enable_gpio inside pwm_bl.c. And that's a good thing. That's what people will expect. Backlights are exposed via sysfs, which is currently also the only way to control the backlight from userspace. If the driver for that doesn't have everything required to control the backlight how can userspace control it? > Note that, pwm_bl can still work properly without enabling the backlight GPIO. > And, I did point out to a valid datasheet from AUO, which clearly indicates why > backlight enable GPIO should be a part of panel driver and not pwm_bl driver. Just because some spec mentions the backlight enable pin in some panel power up sequence diagram that doesn't mean we have to implement it as part of the panel driver. > I am not trying to say we should remove enable_gpio from pwm_bl. > Provided that its already an optional property, and if someone wants > to control it in a panel driver, what is wrong in doing so? See above. Thierry [0]: https://lkml.org/lkml/2013/10/7/188
On 5/19/14, Thierry Reding <thierry.reding@gmail.com> wrote: > On Sun, May 18, 2014 at 01:50:36PM +0530, Ajay kumar wrote: >> On Sun, May 18, 2014 at 2:44 AM, Thierry Reding >> <thierry.reding@gmail.com> wrote: >> > On Thu, May 15, 2014 at 05:10:16PM +0530, Ajay kumar wrote: >> >> On Thu, May 15, 2014 at 1:43 PM, Thierry Reding >> >> <thierry.reding@gmail.com> wrote: >> > [...] >> >> > 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. >> > >> > Okay, so this means that your backlight is turned on too early. Instead >> > of working around that problem by moving control of the backlight >> > enable >> > GPIO from the backlight driver into the panel driver, the correct way >> > to >> > fix it is to make sure the backlight stays off until video is ready. >> Ok. Please suggest an idea how to do the same! > > I did post a patch[0] a long time ago that added a way to fix this for > pwm-backlight at least. I have tried this. We have to wait till the userspace to switch the backlight on again :( >> I have already suggested a simple idea which conforms to a valid spec. >> Just because enable_gpio is already a part of pwm_bl.c, I somewhat feel >> like we are forcing people to handle enable_gpio inside pwm_bl.c. > > And that's a good thing. That's what people will expect. Backlights are > exposed via sysfs, which is currently also the only way to control the > backlight from userspace. If the driver for that doesn't have everything > required to control the backlight how can userspace control it? This is a valid point, only at the hardware level. But if you consider a user experience perspective, user still will not be able to see the backlight even though enable_gpio is controlled elsewhere, since pwm is disabled anyhow. Now lets talk about backlight_on case. Even though user tries to turn on the backlight, and the driver turns on backlight supply, pwm and enable_gpio, the driver cannot always guarantee him that backlight is "really on" since it also depends on panel power. Such ambiguities exist for few panels. And, for such panels why can't we keep enable_gpio in another driver, and let the pwm_bl control only backlight levels? >> Note that, pwm_bl can still work properly without enabling the backlight >> GPIO. >> And, I did point out to a valid datasheet from AUO, which clearly >> indicates why >> backlight enable GPIO should be a part of panel driver and not pwm_bl >> driver. > > Just because some spec mentions the backlight enable pin in some panel > power up sequence diagram that doesn't mean we have to implement it as > part of the panel driver. That is not "some spec". I believe all the AUO panels share the same sequence! >> I am not trying to say we should remove enable_gpio from pwm_bl. >> Provided that its already an optional property, and if someone wants >> to control it in a panel driver, what is wrong in doing so? > > See above. > > Thierry > > [0]: https://lkml.org/lkml/2013/10/7/188 >
On Tue, May 20, 2014 at 12:37:38AM +0530, Ajay kumar wrote: > On 5/19/14, Thierry Reding <thierry.reding@gmail.com> wrote: > > On Sun, May 18, 2014 at 01:50:36PM +0530, Ajay kumar wrote: > >> On Sun, May 18, 2014 at 2:44 AM, Thierry Reding > >> <thierry.reding@gmail.com> wrote: > >> > On Thu, May 15, 2014 at 05:10:16PM +0530, Ajay kumar wrote: > >> >> On Thu, May 15, 2014 at 1:43 PM, Thierry Reding > >> >> <thierry.reding@gmail.com> wrote: > >> > [...] > >> >> > 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. > >> > > >> > Okay, so this means that your backlight is turned on too early. Instead > >> > of working around that problem by moving control of the backlight > >> > enable > >> > GPIO from the backlight driver into the panel driver, the correct way > >> > to > >> > fix it is to make sure the backlight stays off until video is ready. > >> Ok. Please suggest an idea how to do the same! > > > > I did post a patch[0] a long time ago that added a way to fix this for > > pwm-backlight at least. > I have tried this. We have to wait till the userspace to switch the > backlight on again :( Erm... why? > >> I have already suggested a simple idea which conforms to a valid spec. > >> Just because enable_gpio is already a part of pwm_bl.c, I somewhat feel > >> like we are forcing people to handle enable_gpio inside pwm_bl.c. > > > > And that's a good thing. That's what people will expect. Backlights are > > exposed via sysfs, which is currently also the only way to control the > > backlight from userspace. If the driver for that doesn't have everything > > required to control the backlight how can userspace control it? > This is a valid point, only at the hardware level. > But if you consider a user experience perspective, > user still will not be able to see the backlight even > though enable_gpio is controlled elsewhere, since > pwm is disabled anyhow. But that's precisely my point. If the enable_gpio is controlled elsewhere there will be no way for the userspace interface to enable the backlight. > Now lets talk about backlight_on case. Even though user tries to turn > on the backlight, and the driver turns on backlight supply, pwm and > enable_gpio, the driver cannot always guarantee him that backlight is > "really on" since it also depends on panel power. No. If the backlight is properly hooked up to all resources, then turning it on will *really turn it on*. > >> Note that, pwm_bl can still work properly without enabling the backlight > >> GPIO. > >> And, I did point out to a valid datasheet from AUO, which clearly > >> indicates why > >> backlight enable GPIO should be a part of panel driver and not pwm_bl > >> driver. > > > > Just because some spec mentions the backlight enable pin in some panel > > power up sequence diagram that doesn't mean we have to implement it as > > part of the panel driver. > That is not "some spec". I believe all the AUO panels share the same > sequence! Just because some specs mention the backlight enable pin in some panel power up sequence diagram that doesn't mean we have to implement it as part of the panel driver. Thierry
On Tue, May 20, 2014 at 1:40 AM, Thierry Reding <thierry.reding@gmail.com> wrote: > On Tue, May 20, 2014 at 12:37:38AM +0530, Ajay kumar wrote: >> On 5/19/14, Thierry Reding <thierry.reding@gmail.com> wrote: >> > On Sun, May 18, 2014 at 01:50:36PM +0530, Ajay kumar wrote: >> >> On Sun, May 18, 2014 at 2:44 AM, Thierry Reding >> >> <thierry.reding@gmail.com> wrote: >> >> > On Thu, May 15, 2014 at 05:10:16PM +0530, Ajay kumar wrote: >> >> >> On Thu, May 15, 2014 at 1:43 PM, Thierry Reding >> >> >> <thierry.reding@gmail.com> wrote: >> >> > [...] >> >> >> > 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. >> >> > >> >> > Okay, so this means that your backlight is turned on too early. Instead >> >> > of working around that problem by moving control of the backlight >> >> > enable >> >> > GPIO from the backlight driver into the panel driver, the correct way >> >> > to >> >> > fix it is to make sure the backlight stays off until video is ready. >> >> Ok. Please suggest an idea how to do the same! >> > >> > I did post a patch[0] a long time ago that added a way to fix this for >> > pwm-backlight at least. >> I have tried this. We have to wait till the userspace to switch the >> backlight on again :( > > Erm... why? Because, backlight remains in powerdown state till the userspace startup scripts turn it back on! >> >> I have already suggested a simple idea which conforms to a valid spec. >> >> Just because enable_gpio is already a part of pwm_bl.c, I somewhat feel >> >> like we are forcing people to handle enable_gpio inside pwm_bl.c. >> > >> > And that's a good thing. That's what people will expect. Backlights are >> > exposed via sysfs, which is currently also the only way to control the >> > backlight from userspace. If the driver for that doesn't have everything >> > required to control the backlight how can userspace control it? >> This is a valid point, only at the hardware level. >> But if you consider a user experience perspective, >> user still will not be able to see the backlight even >> though enable_gpio is controlled elsewhere, since >> pwm is disabled anyhow. > > But that's precisely my point. If the enable_gpio is controlled > elsewhere there will be no way for the userspace interface to enable the > backlight. Hmm, this is a valid point. Still, see my explanation below.[1] >> Now lets talk about backlight_on case. Even though user tries to turn >> on the backlight, and the driver turns on backlight supply, pwm and >> enable_gpio, the driver cannot always guarantee him that backlight is >> "really on" since it also depends on panel power. > > No. If the backlight is properly hooked up to all resources, then > turning it on will *really turn it on*. [1] : This is what I am trying to elaborate. Backlight really does depend on panel power also, and you cannot tie LCD resources to backlight driver. Now, consider that panel/LCD is off and user wishes to control backlight, it will have no effect. So, ideally backlight drivers are not "completely independent" in generating backlight in most of the cases. LCD driver and backlight driver should both co ordinate at user/kernel level to make things work properly. Lets consider this also as a similar case - "backlight enable being handled in panel driver." Here also, things will work if LCD and backlight drivers work in tandem, at user space. >> >> Note that, pwm_bl can still work properly without enabling the backlight >> >> GPIO. >> >> And, I did point out to a valid datasheet from AUO, which clearly >> >> indicates why >> >> backlight enable GPIO should be a part of panel driver and not pwm_bl >> >> driver. >> > >> > Just because some spec mentions the backlight enable pin in some panel >> > power up sequence diagram that doesn't mean we have to implement it as >> > part of the panel driver. >> That is not "some spec". I believe all the AUO panels share the same >> sequence! > > Just because some specs mention the backlight enable pin in some panel > power up sequence diagram that doesn't mean we have to implement it as > part of the panel driver. > Thierry
On 06/03/2014 11:58 AM, Ajay kumar wrote: > On Tue, May 20, 2014 at 1:40 AM, Thierry Reding > <thierry.reding@gmail.com> wrote: >> On Tue, May 20, 2014 at 12:37:38AM +0530, Ajay kumar wrote: >>> On 5/19/14, Thierry Reding <thierry.reding@gmail.com> wrote: >>>> On Sun, May 18, 2014 at 01:50:36PM +0530, Ajay kumar wrote: >>>>> On Sun, May 18, 2014 at 2:44 AM, Thierry Reding >>>>> <thierry.reding@gmail.com> wrote: >>>>>> On Thu, May 15, 2014 at 05:10:16PM +0530, Ajay kumar wrote: >>>>>>> On Thu, May 15, 2014 at 1:43 PM, Thierry Reding >>>>>>> <thierry.reding@gmail.com> wrote: >>>>>> [...] >>>>>>>> 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. >>>>>> Okay, so this means that your backlight is turned on too early. Instead >>>>>> of working around that problem by moving control of the backlight >>>>>> enable >>>>>> GPIO from the backlight driver into the panel driver, the correct way >>>>>> to >>>>>> fix it is to make sure the backlight stays off until video is ready. >>>>> Ok. Please suggest an idea how to do the same! >>>> I did post a patch[0] a long time ago that added a way to fix this for >>>> pwm-backlight at least. >>> I have tried this. We have to wait till the userspace to switch the >>> backlight on again :( >> Erm... why? > Because, backlight remains in powerdown state till the userspace startup scripts > turn it back on! > >>>>> I have already suggested a simple idea which conforms to a valid spec. >>>>> Just because enable_gpio is already a part of pwm_bl.c, I somewhat feel >>>>> like we are forcing people to handle enable_gpio inside pwm_bl.c. >>>> And that's a good thing. That's what people will expect. Backlights are >>>> exposed via sysfs, which is currently also the only way to control the >>>> backlight from userspace. If the driver for that doesn't have everything >>>> required to control the backlight how can userspace control it? >>> This is a valid point, only at the hardware level. >>> But if you consider a user experience perspective, >>> user still will not be able to see the backlight even >>> though enable_gpio is controlled elsewhere, since >>> pwm is disabled anyhow. >> But that's precisely my point. If the enable_gpio is controlled >> elsewhere there will be no way for the userspace interface to enable the >> backlight. > Hmm, this is a valid point. Still, see my explanation below.[1] > >>> Now lets talk about backlight_on case. Even though user tries to turn >>> on the backlight, and the driver turns on backlight supply, pwm and >>> enable_gpio, the driver cannot always guarantee him that backlight is >>> "really on" since it also depends on panel power. >> No. If the backlight is properly hooked up to all resources, then >> turning it on will *really turn it on*. > [1] : This is what I am trying to elaborate. > Backlight really does depend on panel power also, and > you cannot tie LCD resources to backlight driver. > Now, consider that panel/LCD is off and user wishes to control backlight, > it will have no effect. > So, ideally backlight drivers are not "completely independent" in generating > backlight in most of the cases. > LCD driver and backlight driver should both co ordinate at user/kernel level > to make things work properly. > > Lets consider this also as a similar case - "backlight enable being handled in > panel driver." > Here also, things will work if LCD and backlight drivers work in tandem, > at user space. > >>>>> Note that, pwm_bl can still work properly without enabling the backlight >>>>> GPIO. >>>>> And, I did point out to a valid datasheet from AUO, which clearly >>>>> indicates why >>>>> backlight enable GPIO should be a part of panel driver and not pwm_bl >>>>> driver. >>>> Just because some spec mentions the backlight enable pin in some panel >>>> power up sequence diagram that doesn't mean we have to implement it as >>>> part of the panel driver. >>> That is not "some spec". I believe all the AUO panels share the same >>> sequence! >> Just because some specs mention the backlight enable pin in some panel >> power up sequence diagram that doesn't mean we have to implement it as >> part of the panel driver. >> Thierry My two cents. I have took some random panel specs with separate backlight: - LD070WX3 - implemented in Linux as simple panel [1], - LP129QE1 - again implemented as simple panel [2]. Both panels requires that backlight should be turned on after 200ms of valid video data and turned off 200ms before end of video data. [1]: http://www.displayalliance.com/storage/1-spec-sheets/LD070WX3-SL01.pdf [2]: http://www.revointeractive.com/prodimages/LCD-Display-Panel/LG-12.9-LP129QE1-SPA1-LVDS-2560-1600-400-NITS.pdf If I understand correctly currently there is no communication between video data provider (encoder) and backlight device driver, so it is possible to violate panel specifications. I guess usually it is harmless but I think it would be good thing to solve it somehow. IMHO it could be done this way: 1. Panel should be notified about starting of video data from its video source, for example by some drm_panel callback. 2. Panel should have possibility to activate/deactivate backlight device, as I understand there is no such mechanism at the moment, unless we try to play with backlight registration/unregistration. Any opinions? Regards Andrzej
diff --git a/Documentation/devicetree/bindings/drm/bridge/bridge_panel.txt b/Documentation/devicetree/bindings/drm/bridge/bridge_panel.txt new file mode 100644 index 0000000..0f916b0 --- /dev/null +++ b/Documentation/devicetree/bindings/drm/bridge/bridge_panel.txt @@ -0,0 +1,45 @@ +Simple panel interface for chaining along with bridges + +Required properties: + - compatible: "drm-bridge,panel" + +Optional properties: + -lcd-en-gpio: + eDP panel LCD poweron GPIO. + Indicates which GPIO needs to be powered up as output + to powerup/enable the switch to the LCD panel. + -led-en-gpio: + eDP panel LED enable GPIO. + Indicates which GPIO needs to be powered up as output + to enable the backlight. + -panel-pre-enable-delay: + delay value in ms required for panel_pre_enable process + Delay in ms needed for the eDP panel LCD unit to + powerup, and delay needed between panel_VCC and + video_enable. + -panel-enable-delay: + delay value in ms required for panel_enable process + Delay in ms needed for the eDP panel backlight/LED unit + to powerup, and delay needed between video_enable and + BL_EN. + -panel-disable-delay: + delay value in ms required for panel_disable process + Delay in ms needed for the eDP panel backlight/LED unit + powerdown, and delay needed between BL_DISABLE and + video_disable. + -panel-post-disable-delay: + delay value in ms required for panel_post_disable process + Delay in ms needed for the eDP panel LCD unit to + to powerdown, and delay between video_disable and + panel_VCC going down. + +Example: + + bridge-panel { + compatible = "drm-bridge,panel"; + led-en-gpio = <&gpx3 0 1>; + panel-pre-enable-delay = <40>; + panel-enable-delay = <20>; + panel-disable-delay = <20>; + panel-post-disable-delay = <30>; + }; diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig index 884923f..654c5ea 100644 --- a/drivers/gpu/drm/bridge/Kconfig +++ b/drivers/gpu/drm/bridge/Kconfig @@ -3,3 +3,9 @@ config DRM_PTN3460 depends on DRM select DRM_KMS_HELPER ---help--- + +config DRM_BRIDGE_PANEL + tristate "dummy bridge panel" + depends on DRM + select DRM_KMS_HELPER + ---help--- diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile index b4733e1..bf433cf 100644 --- a/drivers/gpu/drm/bridge/Makefile +++ b/drivers/gpu/drm/bridge/Makefile @@ -1,3 +1,4 @@ ccflags-y := -Iinclude/drm obj-$(CONFIG_DRM_PTN3460) += ptn3460.o +obj-$(CONFIG_DRM_BRIDGE_PANEL) += bridge_panel.o diff --git a/drivers/gpu/drm/bridge/bridge_panel.c b/drivers/gpu/drm/bridge/bridge_panel.c new file mode 100644 index 0000000..c629e93 --- /dev/null +++ b/drivers/gpu/drm/bridge/bridge_panel.c @@ -0,0 +1,217 @@ +/* + * Copyright (C) 2014 Samsung Electronics Co., Ltd. + * + * This software is licensed under the terms of the GNU General Public + * License version 2, as published by the Free Software Foundation, and + * may be copied, distributed, and modified under those terms. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <linux/module.h> +#include <linux/of.h> +#include <linux/of_gpio.h> +#include <linux/gpio.h> +#include <linux/delay.h> +#include <linux/regulator/consumer.h> + +#include "drmP.h" + +#include "bridge/bridge_panel.h" + +struct bridge_panel { + struct drm_connector connector; + struct i2c_client *client; + struct drm_encoder *encoder; + struct drm_bridge *bridge; + struct regulator *backlight_fet; + struct regulator *lcd_fet; + bool backlight_fet_enabled; + bool lcd_fet_enabled; + int led_en_gpio; + int lcd_en_gpio; + int panel_pre_enable_delay; + int panel_enable_delay; + int panel_disable_delay; + int panel_post_disable_delay; +}; + +static void bridge_panel_pre_enable(struct drm_bridge *bridge) +{ + struct bridge_panel *panel = bridge->driver_private; + + if (!IS_ERR_OR_NULL(panel->lcd_fet)) + if (!panel->lcd_fet_enabled) { + if (regulator_enable(panel->lcd_fet)) + DRM_ERROR("Failed to enable LCD fet\n"); + panel->lcd_fet_enabled = true; + } + + if (gpio_is_valid(panel->lcd_en_gpio)) + gpio_set_value(panel->lcd_en_gpio, 1); + + msleep(panel->panel_pre_enable_delay); +} + +static void bridge_panel_enable(struct drm_bridge *bridge) +{ + struct bridge_panel *panel = bridge->driver_private; + + if (!IS_ERR_OR_NULL(panel->backlight_fet)) + if (!panel->backlight_fet_enabled) { + if (regulator_enable(panel->backlight_fet)) + DRM_ERROR("Failed to enable LED fet\n"); + panel->backlight_fet_enabled = true; + } + + msleep(panel->panel_enable_delay); + + if (gpio_is_valid(panel->led_en_gpio)) + gpio_set_value(panel->led_en_gpio, 1); +} + +static void bridge_panel_disable(struct drm_bridge *bridge) +{ + struct bridge_panel *panel = bridge->driver_private; + + if (gpio_is_valid(panel->led_en_gpio)) + gpio_set_value(panel->led_en_gpio, 0); + + if (!IS_ERR_OR_NULL(panel->backlight_fet)) + if (panel->backlight_fet_enabled) { + regulator_disable(panel->backlight_fet); + panel->backlight_fet_enabled = false; + } + + msleep(panel->panel_disable_delay); +} + +static void bridge_panel_post_disable(struct drm_bridge *bridge) +{ + struct bridge_panel *panel = bridge->driver_private; + + if (gpio_is_valid(panel->lcd_en_gpio)) + gpio_set_value(panel->lcd_en_gpio, 0); + + if (!IS_ERR_OR_NULL(panel->lcd_fet)) + if (panel->lcd_fet_enabled) { + regulator_disable(panel->lcd_fet); + panel->lcd_fet_enabled = false; + } + + msleep(panel->panel_post_disable_delay); +} + +void bridge_panel_destroy(struct drm_bridge *bridge) +{ + struct bridge_panel *panel = bridge->driver_private; + + drm_bridge_cleanup(bridge); + + if (gpio_is_valid(panel->lcd_en_gpio)) + gpio_free(panel->lcd_en_gpio); + if (gpio_is_valid(panel->led_en_gpio)) + gpio_free(panel->led_en_gpio); + /* Nothing else to free, we've got devm allocated memory */ +} + +struct drm_bridge_funcs bridge_panel_funcs = { + .pre_enable = bridge_panel_pre_enable, + .enable = bridge_panel_enable, + .disable = bridge_panel_disable, + .post_disable = bridge_panel_post_disable, + .destroy = bridge_panel_destroy, +}; + +struct drm_bridge *bridge_panel_init(struct drm_device *dev, + struct drm_encoder *encoder, + struct i2c_client *client, + struct device_node *node) +{ + int ret; + struct drm_bridge *bridge; + struct bridge_panel *panel; + + bridge = devm_kzalloc(dev->dev, sizeof(*bridge), GFP_KERNEL); + if (!bridge) { + DRM_ERROR("Failed to allocate drm bridge\n"); + return NULL; + } + + panel = devm_kzalloc(dev->dev, sizeof(*panel), GFP_KERNEL); + if (!panel) { + DRM_ERROR("Failed to allocate bridge panel\n"); + return NULL; + } + + panel->client = client; + panel->encoder = encoder; + panel->bridge = bridge; + + panel->lcd_en_gpio = of_get_named_gpio(node, "lcd-en-gpio", 0); + panel->lcd_en_gpio = of_get_named_gpio(node, "led-en-gpio", 0); + + of_property_read_u32(node, "panel-pre-enable-delay", + &panel->panel_pre_enable_delay); + of_property_read_u32(node, "panel-enable-delay", + &panel->panel_enable_delay); + of_property_read_u32(node, "panel-disable-delay", + &panel->panel_disable_delay); + of_property_read_u32(node, "panel-post-disable-delay", + &panel->panel_post_disable_delay); + + panel->lcd_fet = devm_regulator_get(dev->dev, "lcd_vdd"); + if (IS_ERR(panel->lcd_fet)) + return NULL; + + panel->backlight_fet = devm_regulator_get(dev->dev, "vcd_led"); + if (IS_ERR(panel->backlight_fet)) + return NULL; + + if (gpio_is_valid(panel->lcd_en_gpio)) { + ret = devm_gpio_request_one(dev->dev, panel->lcd_en_gpio, + GPIOF_OUT_INIT_LOW, "lcd_en_gpio"); + if (ret) { + DRM_ERROR("failed to get lcd-en gpio [%d]\n", ret); + return NULL; + } + } else { + panel->lcd_en_gpio = -ENODEV; + } + + if (gpio_is_valid(panel->led_en_gpio)) { + ret = devm_gpio_request_one(dev->dev, panel->led_en_gpio, + GPIOF_OUT_INIT_LOW, "led_en_gpio"); + if (ret) { + DRM_ERROR("failed to get led-en gpio [%d]\n", ret); + return NULL; + } + } else { + panel->led_en_gpio = -ENODEV; + } + + ret = drm_bridge_init(dev, bridge, &bridge_panel_funcs); + if (ret) { + DRM_ERROR("Failed to initialize bridge with drm\n"); + goto err; + } + + bridge->driver_private = panel; + + if (!encoder->bridge) + /* First entry in the bridge chain */ + encoder->bridge = bridge; + + return bridge; + +err: + if (gpio_is_valid(panel->lcd_en_gpio)) + gpio_free(panel->lcd_en_gpio); + if (gpio_is_valid(panel->led_en_gpio)) + gpio_free(panel->led_en_gpio); + return NULL; +} +EXPORT_SYMBOL(bridge_panel_init); diff --git a/include/drm/bridge/bridge_panel.h b/include/drm/bridge/bridge_panel.h new file mode 100644 index 0000000..7f0d662 --- /dev/null +++ b/include/drm/bridge/bridge_panel.h @@ -0,0 +1,40 @@ +/* + * Copyright (C) 2014 Samsung Electronics Co., Ltd. + * + * This software is licensed under the terms of the GNU General Public + * License version 2, as published by the Free Software Foundation, and + * may be copied, distributed, and modified under those terms. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#ifndef _DRM_BRIDGE_PANEL_H_ +#define _DRM_BRIDGE_PANEL_H_ + +struct drm_device; +struct drm_encoder; +struct i2c_client; +struct device_node; + +#if defined(CONFIG_DRM_BRIDGE_PANEL) + +struct drm_bridge *bridge_panel_init(struct drm_device *dev, + struct drm_encoder *encoder, + struct i2c_client *client, + struct device_node *node); +#else + +static inline struct drm_bridge *bridge_panel_init(struct drm_device *dev, + struct drm_encoder *encoder, + struct i2c_client *client, + struct device_node *node) +{ + return 0; +} + +#endif + +#endif
implement basic panel controls as a drm_bridge so that the existing bridges can make use of it. The driver assumes that it is the last entity in the bridge chain. Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com> --- .../bindings/drm/bridge/bridge_panel.txt | 45 ++++ drivers/gpu/drm/bridge/Kconfig | 6 + drivers/gpu/drm/bridge/Makefile | 1 + drivers/gpu/drm/bridge/bridge_panel.c | 217 ++++++++++++++++++++ include/drm/bridge/bridge_panel.h | 40 ++++ 5 files changed, 309 insertions(+) create mode 100644 Documentation/devicetree/bindings/drm/bridge/bridge_panel.txt create mode 100644 drivers/gpu/drm/bridge/bridge_panel.c create mode 100644 include/drm/bridge/bridge_panel.h