| Message ID | 1398119958-32005-3-git-send-email-ajaykumar.rs@samsung.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
On Tue, Apr 22, 2014 at 04:09:11AM +0530, Ajay Kumar wrote: > Most of the panels need an init sequence as mentioned below: > -- poweron LCD unit/LCD_EN > -- start video data > -- poweron LED unit/BL_EN > And, a de-init sequence as mentioned below: > -- poweroff LED unit/BL_EN > -- stop video data > -- poweroff LCD unit/LCD_EN > With existing callbacks for drm panel, we cannot accomodate such panels, > since only two callbacks, i.e "panel_enable" and panel_disable are supported. > > This patch adds: > -- "pre_enable" callback which can be called before > the actual video data is on, and then call the "enable" > callback after the video data is available. > > -- "post_disable" callback which can be called after > the video data is off, and use "disable" callback > to do something before switching off the video data. > > Now, we can easily map the above scenario as shown below: > poweron LCD unit/LCD_EN = "pre_enable" callback > poweron LED unit/BL_EN = "enable" callback > poweroff LED unit/BL_EN = "disable" callback > poweroff LCD unit/LCD_EN = "post_disable" callback I don't like this. What happens when the next panel comes around that has a yet slightly different requirement? Will we introduce a new pre_pre_enable() and post_post_disable() function then? There's got to be a better way to solve this. Thierry
Hi Thierry, On Tue, Apr 22, 2014 at 1:49 PM, Thierry Reding <thierry.reding@gmail.com> wrote: > On Tue, Apr 22, 2014 at 04:09:11AM +0530, Ajay Kumar wrote: >> Most of the panels need an init sequence as mentioned below: >> -- poweron LCD unit/LCD_EN >> -- start video data >> -- poweron LED unit/BL_EN >> And, a de-init sequence as mentioned below: >> -- poweroff LED unit/BL_EN >> -- stop video data >> -- poweroff LCD unit/LCD_EN >> With existing callbacks for drm panel, we cannot accomodate such panels, >> since only two callbacks, i.e "panel_enable" and panel_disable are supported. >> >> This patch adds: >> -- "pre_enable" callback which can be called before >> the actual video data is on, and then call the "enable" >> callback after the video data is available. >> >> -- "post_disable" callback which can be called after >> the video data is off, and use "disable" callback >> to do something before switching off the video data. >> >> Now, we can easily map the above scenario as shown below: >> poweron LCD unit/LCD_EN = "pre_enable" callback >> poweron LED unit/BL_EN = "enable" callback >> poweroff LED unit/BL_EN = "disable" callback >> poweroff LCD unit/LCD_EN = "post_disable" callback > > I don't like this. What happens when the next panel comes around that > has a yet slightly different requirement? Will we introduce a new > pre_pre_enable() and post_post_disable() function then? > As I have already explained, these 2 callbacks are sufficient enough to take care the power up/down sequence for LVDS and eDP panels. And, definitely having just 2 callbacks "enable" and "disable" is not at all sufficient. I initially thought of using panel_simple_enable from panel-simple driver. But it doesn't go well with case wherein there are 2 regulators(one for LCD and one for LED) a BL_EN signal etc. And, often(Believe me, I have referred to both eDP panel datasheets and LVDS panel datasheets) proper powerup sequence for such panels is as mentioned below: powerup: -- power up the supply (LCD_VCC). -- start video data. -- enable backlight. powerdown -- disable backlight. -- stop video data. -- power off the supply (LCD VCC) For the above cases, if I have to somehow fit in all the required settings, it breaks the sequence and I end up getting glitches during bootup/DPMS. Also, when the drm_bridge can support pre_enable and post_disable, why not drm_panel provide that both should work in tandem? > There's got to be a better way to solve this. > > Thierry Thanks and Regards, Ajay Kumar
On Tue, Apr 22, 2014 at 08:06:19PM +0530, Ajay kumar wrote: > Hi Thierry, > > > On Tue, Apr 22, 2014 at 1:49 PM, Thierry Reding <thierry.reding@gmail.com> > wrote: > > On Tue, Apr 22, 2014 at 04:09:11AM +0530, Ajay Kumar wrote: > >> Most of the panels need an init sequence as mentioned below: > >> -- poweron LCD unit/LCD_EN > >> -- start video data > >> -- poweron LED unit/BL_EN > >> And, a de-init sequence as mentioned below: > >> -- poweroff LED unit/BL_EN > >> -- stop video data > >> -- poweroff LCD unit/LCD_EN > >> With existing callbacks for drm panel, we cannot accomodate such panels, > >> since only two callbacks, i.e "panel_enable" and panel_disable are > supported. > >> > >> This patch adds: > >> -- "pre_enable" callback which can be called before > >> the actual video data is on, and then call the "enable" > >> callback after the video data is available. > >> > >> -- "post_disable" callback which can be called after > >> the video data is off, and use "disable" callback > >> to do something before switching off the video data. > >> > >> Now, we can easily map the above scenario as shown below: > >> poweron LCD unit/LCD_EN = "pre_enable" callback > >> poweron LED unit/BL_EN = "enable" callback > >> poweroff LED unit/BL_EN = "disable" callback > >> poweroff LCD unit/LCD_EN = "post_disable" callback > > > > I don't like this. What happens when the next panel comes around that > > has a yet slightly different requirement? Will we introduce a new > > pre_pre_enable() and post_post_disable() function then? > > > As I have already explained, these 2 callbacks are sufficient enough to > take care > the power up/down sequence for LVDS and eDP panels. And, definitely having > just 2 callbacks "enable" and "disable" is not at all sufficient. > > I initially thought of using panel_simple_enable from panel-simple driver. > But it doesn't go well with case wherein there are 2 regulators(one for LCD > and one for LED) > a BL_EN signal etc. And, often(Believe me, I have referred to both eDP > panel datasheets > and LVDS panel datasheets) proper powerup sequence for such panels is as > mentioned below: > > powerup: > -- power up the supply (LCD_VCC). > -- start video data. > -- enable backlight. > > powerdown > -- disable backlight. > -- stop video data. > -- power off the supply (LCD VCC) > > For the above cases, if I have to somehow fit in all the required settings, > it breaks the sequence and I end up getting glitches during bootup/DPMS. > > Also, when the drm_bridge can support pre_enable and post_disable, why not > drm_panel provide that both should work in tandem? Imo this makes sense, especially if we go through with the idea talked about yesterday of creating a drm_bridge to wrap-up a drm_panel with sufficient isolation between all components. -Daniel
On Wed, Apr 23, 2014 at 09:29:15AM +0200, Daniel Vetter wrote: > On Tue, Apr 22, 2014 at 08:06:19PM +0530, Ajay kumar wrote: > > Hi Thierry, > > > > > > On Tue, Apr 22, 2014 at 1:49 PM, Thierry Reding <thierry.reding@gmail.com> > > wrote: > > > On Tue, Apr 22, 2014 at 04:09:11AM +0530, Ajay Kumar wrote: > > >> Most of the panels need an init sequence as mentioned below: > > >> -- poweron LCD unit/LCD_EN > > >> -- start video data > > >> -- poweron LED unit/BL_EN > > >> And, a de-init sequence as mentioned below: > > >> -- poweroff LED unit/BL_EN > > >> -- stop video data > > >> -- poweroff LCD unit/LCD_EN > > >> With existing callbacks for drm panel, we cannot accomodate such panels, > > >> since only two callbacks, i.e "panel_enable" and panel_disable are > > supported. > > >> > > >> This patch adds: > > >> -- "pre_enable" callback which can be called before > > >> the actual video data is on, and then call the "enable" > > >> callback after the video data is available. > > >> > > >> -- "post_disable" callback which can be called after > > >> the video data is off, and use "disable" callback > > >> to do something before switching off the video data. > > >> > > >> Now, we can easily map the above scenario as shown below: > > >> poweron LCD unit/LCD_EN = "pre_enable" callback > > >> poweron LED unit/BL_EN = "enable" callback > > >> poweroff LED unit/BL_EN = "disable" callback > > >> poweroff LCD unit/LCD_EN = "post_disable" callback > > > > > > I don't like this. What happens when the next panel comes around that > > > has a yet slightly different requirement? Will we introduce a new > > > pre_pre_enable() and post_post_disable() function then? > > > > > As I have already explained, these 2 callbacks are sufficient enough to > > take care > > the power up/down sequence for LVDS and eDP panels. And, definitely having > > just 2 callbacks "enable" and "disable" is not at all sufficient. > > > > I initially thought of using panel_simple_enable from panel-simple driver. > > But it doesn't go well with case wherein there are 2 regulators(one for LCD > > and one for LED) > > a BL_EN signal etc. And, often(Believe me, I have referred to both eDP > > panel datasheets > > and LVDS panel datasheets) proper powerup sequence for such panels is as > > mentioned below: > > > > powerup: > > -- power up the supply (LCD_VCC). > > -- start video data. > > -- enable backlight. > > > > powerdown > > -- disable backlight. > > -- stop video data. > > -- power off the supply (LCD VCC) > > > > For the above cases, if I have to somehow fit in all the required settings, > > it breaks the sequence and I end up getting glitches during bootup/DPMS. > > > > Also, when the drm_bridge can support pre_enable and post_disable, why not > > drm_panel provide that both should work in tandem? > > Imo this makes sense, especially if we go through with the idea talked > about yesterday of creating a drm_bridge to wrap-up a drm_panel with > sufficient isolation between all components. I'm not at all comfortable with these. The names are totally confusing. Next somebody will need to do something after the panel has been enabled and we'll have to introduce .post_enable() and .pre_disable() functions. And worse, what if somebody needs something to be done between .pre_enable() and .enable()? .post_pre_enable()? Where does it end? According to the above description the only reason why we need this is to avoid visible glitches when the panel is already on, but the video stream hasn't started yet. If that's the only reason why we need this, then perhaps adding a way for a panel to expose the associated backlight would be better? Thierry
Daniel, On Wed, Apr 23, 2014 at 12:59 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Tue, Apr 22, 2014 at 08:06:19PM +0530, Ajay kumar wrote: >> Hi Thierry, >> >> >> On Tue, Apr 22, 2014 at 1:49 PM, Thierry Reding <thierry.reding@gmail.com> >> wrote: >> > On Tue, Apr 22, 2014 at 04:09:11AM +0530, Ajay Kumar wrote: >> >> Most of the panels need an init sequence as mentioned below: >> >> -- poweron LCD unit/LCD_EN >> >> -- start video data >> >> -- poweron LED unit/BL_EN >> >> And, a de-init sequence as mentioned below: >> >> -- poweroff LED unit/BL_EN >> >> -- stop video data >> >> -- poweroff LCD unit/LCD_EN >> >> With existing callbacks for drm panel, we cannot accomodate such panels, >> >> since only two callbacks, i.e "panel_enable" and panel_disable are >> supported. >> >> >> >> This patch adds: >> >> -- "pre_enable" callback which can be called before >> >> the actual video data is on, and then call the "enable" >> >> callback after the video data is available. >> >> >> >> -- "post_disable" callback which can be called after >> >> the video data is off, and use "disable" callback >> >> to do something before switching off the video data. >> >> >> >> Now, we can easily map the above scenario as shown below: >> >> poweron LCD unit/LCD_EN = "pre_enable" callback >> >> poweron LED unit/BL_EN = "enable" callback >> >> poweroff LED unit/BL_EN = "disable" callback >> >> poweroff LCD unit/LCD_EN = "post_disable" callback >> > >> > I don't like this. What happens when the next panel comes around that >> > has a yet slightly different requirement? Will we introduce a new >> > pre_pre_enable() and post_post_disable() function then? >> > >> As I have already explained, these 2 callbacks are sufficient enough to >> take care >> the power up/down sequence for LVDS and eDP panels. And, definitely having >> just 2 callbacks "enable" and "disable" is not at all sufficient. >> >> I initially thought of using panel_simple_enable from panel-simple driver. >> But it doesn't go well with case wherein there are 2 regulators(one for LCD >> and one for LED) >> a BL_EN signal etc. And, often(Believe me, I have referred to both eDP >> panel datasheets >> and LVDS panel datasheets) proper powerup sequence for such panels is as >> mentioned below: >> >> powerup: >> -- power up the supply (LCD_VCC). >> -- start video data. >> -- enable backlight. >> >> powerdown >> -- disable backlight. >> -- stop video data. >> -- power off the supply (LCD VCC) >> >> For the above cases, if I have to somehow fit in all the required settings, >> it breaks the sequence and I end up getting glitches during bootup/DPMS. >> >> Also, when the drm_bridge can support pre_enable and post_disable, why not >> drm_panel provide that both should work in tandem? > > Imo this makes sense, especially if we go through with the idea talked > about yesterday of creating a drm_bridge to wrap-up a drm_panel with > sufficient isolation between all components. > -Daniel Actually, I tried implementing this for ptn3460. But, it breaks the working. As explained in the other patch(reply for Rob), we cannot truly ISOLATE the panel controls from bridge controls and keep them as seperate. They should be kept together, in the individual bridge chip drivers, so that the bridge chip driver can decide which panel control to call at what point. So, I think combining bridge chip and panel controls was really a bad idea. I will implement the basic panel controls required by the bridge as optional bridge properties via DT. By that way, at least the driver remains robust. Thanks and regards, Ajay Kumar
Thierry, On Wed, Apr 23, 2014 at 1:12 PM, Thierry Reding <thierry.reding@gmail.com> wrote: > On Wed, Apr 23, 2014 at 09:29:15AM +0200, Daniel Vetter wrote: >> On Tue, Apr 22, 2014 at 08:06:19PM +0530, Ajay kumar wrote: >> > Hi Thierry, >> > >> > >> > On Tue, Apr 22, 2014 at 1:49 PM, Thierry Reding <thierry.reding@gmail.com> >> > wrote: >> > > On Tue, Apr 22, 2014 at 04:09:11AM +0530, Ajay Kumar wrote: >> > >> Most of the panels need an init sequence as mentioned below: >> > >> -- poweron LCD unit/LCD_EN >> > >> -- start video data >> > >> -- poweron LED unit/BL_EN >> > >> And, a de-init sequence as mentioned below: >> > >> -- poweroff LED unit/BL_EN >> > >> -- stop video data >> > >> -- poweroff LCD unit/LCD_EN >> > >> With existing callbacks for drm panel, we cannot accomodate such panels, >> > >> since only two callbacks, i.e "panel_enable" and panel_disable are >> > supported. >> > >> >> > >> This patch adds: >> > >> -- "pre_enable" callback which can be called before >> > >> the actual video data is on, and then call the "enable" >> > >> callback after the video data is available. >> > >> >> > >> -- "post_disable" callback which can be called after >> > >> the video data is off, and use "disable" callback >> > >> to do something before switching off the video data. >> > >> >> > >> Now, we can easily map the above scenario as shown below: >> > >> poweron LCD unit/LCD_EN = "pre_enable" callback >> > >> poweron LED unit/BL_EN = "enable" callback >> > >> poweroff LED unit/BL_EN = "disable" callback >> > >> poweroff LCD unit/LCD_EN = "post_disable" callback >> > > >> > > I don't like this. What happens when the next panel comes around that >> > > has a yet slightly different requirement? Will we introduce a new >> > > pre_pre_enable() and post_post_disable() function then? >> > > >> > As I have already explained, these 2 callbacks are sufficient enough to >> > take care >> > the power up/down sequence for LVDS and eDP panels. And, definitely having >> > just 2 callbacks "enable" and "disable" is not at all sufficient. >> > >> > I initially thought of using panel_simple_enable from panel-simple driver. >> > But it doesn't go well with case wherein there are 2 regulators(one for LCD >> > and one for LED) >> > a BL_EN signal etc. And, often(Believe me, I have referred to both eDP >> > panel datasheets >> > and LVDS panel datasheets) proper powerup sequence for such panels is as >> > mentioned below: >> > >> > powerup: >> > -- power up the supply (LCD_VCC). >> > -- start video data. >> > -- enable backlight. >> > >> > powerdown >> > -- disable backlight. >> > -- stop video data. >> > -- power off the supply (LCD VCC) >> > >> > For the above cases, if I have to somehow fit in all the required settings, >> > it breaks the sequence and I end up getting glitches during bootup/DPMS. >> > >> > Also, when the drm_bridge can support pre_enable and post_disable, why not >> > drm_panel provide that both should work in tandem? >> >> Imo this makes sense, especially if we go through with the idea talked >> about yesterday of creating a drm_bridge to wrap-up a drm_panel with >> sufficient isolation between all components. > > I'm not at all comfortable with these. The names are totally confusing. > Next somebody will need to do something after the panel has been enabled > and we'll have to introduce .post_enable() and .pre_disable() functions. > And worse, what if somebody needs something to be done between > .pre_enable() and .enable()? .post_pre_enable()? Where does it end? > > According to the above description the only reason why we need this is > to avoid visible glitches when the panel is already on, but the video > stream hasn't started yet. If that's the only reason why we need this, > then perhaps adding a way for a panel to expose the associated backlight > would be better? Actually, we need not expose the entire backlight device. AFAIK, the glitch is caused when you enable BL_EN before there is valid video data. So, one way to mask off the glitch is to follow this sequence: -- power up the panel. -- start video data, (start PWM here or) -- (start PWM here), enable backlight The problem is that the above scenario cannot be mapped to panel-simple driver. IMO, panel_simple should provide enable/disable controls both for LCD and backlight. something like panel_simple_lcd_enable/panel_simple_led_enable, and panel_simple_lcd_disable/panel_simple_led_disable. Then we can call panel_simple_lcd_enable before video stream is present, And call panel_simple_led_enable after the video stream is present. In that way we can accomodate majority of LVDS and eDP panels inside panel_simple driver without having to worry about the glitch :) Thanks and regards, Ajay Kumar
On Thu, Apr 24, 2014 at 12:56:02AM +0530, Ajay kumar wrote: > Thierry, > > On Wed, Apr 23, 2014 at 1:12 PM, Thierry Reding > <thierry.reding@gmail.com> wrote: > > On Wed, Apr 23, 2014 at 09:29:15AM +0200, Daniel Vetter wrote: [...] > >> Imo this makes sense, especially if we go through with the idea talked > >> about yesterday of creating a drm_bridge to wrap-up a drm_panel with > >> sufficient isolation between all components. > > > > I'm not at all comfortable with these. The names are totally confusing. > > Next somebody will need to do something after the panel has been enabled > > and we'll have to introduce .post_enable() and .pre_disable() functions. > > And worse, what if somebody needs something to be done between > > .pre_enable() and .enable()? .post_pre_enable()? Where does it end? > > > > According to the above description the only reason why we need this is > > to avoid visible glitches when the panel is already on, but the video > > stream hasn't started yet. If that's the only reason why we need this, > > then perhaps adding a way for a panel to expose the associated backlight > > would be better? > Actually, we need not expose the entire backlight device. > AFAIK, the glitch is caused when you enable BL_EN before > there is valid video data. So, one way to mask off the glitch is to > follow this sequence: > -- power up the panel. > -- start video data, (start PWM here or) > -- (start PWM here), enable backlight That's very difficult to get right, isn't it? Even if you have fine- grained control over what to enable you still need a way to determine _when_ it's safe to enable the backlight. Typically I guess that would be the duration of one frame (or perhaps 2, depending on when the panel syncs to the video signal). Perhaps it could even by sync'ed to the VBLANK? > The problem is that the above scenario cannot be mapped to panel-simple driver. > IMO, panel_simple should provide enable/disable controls both for LCD > and backlight. > something like panel_simple_lcd_enable/panel_simple_led_enable, and > panel_simple_lcd_disable/panel_simple_led_disable. That's not what the simple panel driver can do. If we want this it needs to be solved in a generic way for all panels since they all need to use the drm_panel_*() functions to abstract away the details from drivers that use the panels. Thierry
On Friday, April 25, 2014, Thierry Reding <thierry.reding@gmail.com> wrote: > > On Thu, Apr 24, 2014 at 12:56:02AM +0530, Ajay kumar wrote: > > Thierry, > > > > On Wed, Apr 23, 2014 at 1:12 PM, Thierry Reding > > <thierry.reding@gmail.com> wrote: > > > On Wed, Apr 23, 2014 at 09:29:15AM +0200, Daniel Vetter wrote: > [...] > > >> Imo this makes sense, especially if we go through with the idea talked > > >> about yesterday of creating a drm_bridge to wrap-up a drm_panel with > > >> sufficient isolation between all components. > > > > > > I'm not at all comfortable with these. The names are totally confusing. > > > Next somebody will need to do something after the panel has been enabled > > > and we'll have to introduce .post_enable() and .pre_disable() functions. > > > And worse, what if somebody needs something to be done between > > > .pre_enable() and .enable()? .post_pre_enable()? Where does it end? > > > > > > According to the above description the only reason why we need this is > > > to avoid visible glitches when the panel is already on, but the video > > > stream hasn't started yet. If that's the only reason why we need this, > > > then perhaps adding a way for a panel to expose the associated backlight > > > would be better? > > Actually, we need not expose the entire backlight device. > > AFAIK, the glitch is caused when you enable BL_EN before > > there is valid video data. So, one way to mask off the glitch is to > > follow this sequence: > > -- power up the panel. > > -- start video data, (start PWM here or) > > -- (start PWM here), enable backlight > > That's very difficult to get right, isn't it? Even if you have fine- > grained control over what to enable you still need a way to determine > _when_ it's safe to enable the backlight. Typically I guess that would > be the duration of one frame (or perhaps 2, depending on when the panel > syncs to the video signal). We need not "determine", its already present in LVDS datasheet. The LVDS datasheet says at least 200ms delay is needed from "Valid data" to "BL on". > Perhaps it could even by sync'ed to the VBLANK? No. vblanks are related to crtc. And the bridge/panel driver should be independent of vblank. > > The problem is that the above scenario cannot be mapped to panel-simple driver. > > IMO, panel_simple should provide enable/disable controls both for LCD > > and backlight. > > something like panel_simple_lcd_enable/panel_simple_led_enable, and > > panel_simple_lcd_disable/panel_simple_led_disable. > > That's not what the simple panel driver can do. If we want this it needs > to be solved in a generic way for all panels since they all need to use > the drm_panel_*() functions to abstract away the details from drivers > that use the panels. Right. So only I have added pre_enable and post_disable callbacks. Using that name won't harm existing panel drivers and still addresses our requirement. Regards, Ajay
On Fri, Apr 25, 2014 at 11:34 AM, Ajay kumar <ajaynumb@gmail.com> wrote: > On Friday, April 25, 2014, Thierry Reding <thierry.reding@gmail.com> wrote: >> >> On Thu, Apr 24, 2014 at 12:56:02AM +0530, Ajay kumar wrote: >> > Thierry, >> > >> > On Wed, Apr 23, 2014 at 1:12 PM, Thierry Reding >> > <thierry.reding@gmail.com> wrote: >> > > On Wed, Apr 23, 2014 at 09:29:15AM +0200, Daniel Vetter wrote: >> [...] >> > >> Imo this makes sense, especially if we go through with the idea talked >> > >> about yesterday of creating a drm_bridge to wrap-up a drm_panel with >> > >> sufficient isolation between all components. >> > > >> > > I'm not at all comfortable with these. The names are totally confusing. >> > > Next somebody will need to do something after the panel has been enabled >> > > and we'll have to introduce .post_enable() and .pre_disable() functions. >> > > And worse, what if somebody needs something to be done between >> > > .pre_enable() and .enable()? .post_pre_enable()? Where does it end? >> > > >> > > According to the above description the only reason why we need this is >> > > to avoid visible glitches when the panel is already on, but the video >> > > stream hasn't started yet. If that's the only reason why we need this, >> > > then perhaps adding a way for a panel to expose the associated backlight >> > > would be better? >> > Actually, we need not expose the entire backlight device. >> > AFAIK, the glitch is caused when you enable BL_EN before >> > there is valid video data. So, one way to mask off the glitch is to >> > follow this sequence: >> > -- power up the panel. >> > -- start video data, (start PWM here or) >> > -- (start PWM here), enable backlight >> >> That's very difficult to get right, isn't it? Even if you have fine- >> grained control over what to enable you still need a way to determine >> _when_ it's safe to enable the backlight. Typically I guess that would >> be the duration of one frame (or perhaps 2, depending on when the panel >> syncs to the video signal). > We need not "determine", its already present in LVDS datasheet. > The LVDS datasheet says at least 200ms delay is needed from "Valid > data" to "BL on". > >> Perhaps it could even by sync'ed to the VBLANK? > No. vblanks are related to crtc. And the bridge/panel driver should be > independent of vblank. > >> > The problem is that the above scenario cannot be mapped to panel-simple driver. >> > IMO, panel_simple should provide enable/disable controls both for LCD >> > and backlight. >> > something like panel_simple_lcd_enable/panel_simple_led_enable, and >> > panel_simple_lcd_disable/panel_simple_led_disable. >> >> That's not what the simple panel driver can do. If we want this it needs >> to be solved in a generic way for all panels since they all need to use >> the drm_panel_*() functions to abstract away the details from drivers >> that use the panels. > Right. So only I have added pre_enable and post_disable callbacks. > Using that name won't harm existing panel drivers and still addresses > our requirement. > > > Regards, > Ajay Thierry : Are you really ok with the new callback names? like pre_enable and post_disable? Adding those new callbacks really won't harm the existing panels anyhow. Daniel, Rob : I think I have given sufficient amount of information as to why the concept of drm_panel_bridge fails and why we cannot have callbacks for drm_panel inside crtc helpers or any other generic place. Please let me know your final opinion so that I can start reworking on this series. Thanks and regards, Ajay Kumar
ping! On Fri, Apr 25, 2014 at 11:46 PM, Ajay kumar <ajaynumb@gmail.com> wrote: > On Fri, Apr 25, 2014 at 11:34 AM, Ajay kumar <ajaynumb@gmail.com> wrote: >> On Friday, April 25, 2014, Thierry Reding <thierry.reding@gmail.com> wrote: >>> >>> On Thu, Apr 24, 2014 at 12:56:02AM +0530, Ajay kumar wrote: >>> > Thierry, >>> > >>> > On Wed, Apr 23, 2014 at 1:12 PM, Thierry Reding >>> > <thierry.reding@gmail.com> wrote: >>> > > On Wed, Apr 23, 2014 at 09:29:15AM +0200, Daniel Vetter wrote: >>> [...] >>> > >> Imo this makes sense, especially if we go through with the idea talked >>> > >> about yesterday of creating a drm_bridge to wrap-up a drm_panel with >>> > >> sufficient isolation between all components. >>> > > >>> > > I'm not at all comfortable with these. The names are totally confusing. >>> > > Next somebody will need to do something after the panel has been enabled >>> > > and we'll have to introduce .post_enable() and .pre_disable() functions. >>> > > And worse, what if somebody needs something to be done between >>> > > .pre_enable() and .enable()? .post_pre_enable()? Where does it end? >>> > > >>> > > According to the above description the only reason why we need this is >>> > > to avoid visible glitches when the panel is already on, but the video >>> > > stream hasn't started yet. If that's the only reason why we need this, >>> > > then perhaps adding a way for a panel to expose the associated backlight >>> > > would be better? >>> > Actually, we need not expose the entire backlight device. >>> > AFAIK, the glitch is caused when you enable BL_EN before >>> > there is valid video data. So, one way to mask off the glitch is to >>> > follow this sequence: >>> > -- power up the panel. >>> > -- start video data, (start PWM here or) >>> > -- (start PWM here), enable backlight >>> >>> That's very difficult to get right, isn't it? Even if you have fine- >>> grained control over what to enable you still need a way to determine >>> _when_ it's safe to enable the backlight. Typically I guess that would >>> be the duration of one frame (or perhaps 2, depending on when the panel >>> syncs to the video signal). >> We need not "determine", its already present in LVDS datasheet. >> The LVDS datasheet says at least 200ms delay is needed from "Valid >> data" to "BL on". >> >>> Perhaps it could even by sync'ed to the VBLANK? >> No. vblanks are related to crtc. And the bridge/panel driver should be >> independent of vblank. >> >>> > The problem is that the above scenario cannot be mapped to panel-simple driver. >>> > IMO, panel_simple should provide enable/disable controls both for LCD >>> > and backlight. >>> > something like panel_simple_lcd_enable/panel_simple_led_enable, and >>> > panel_simple_lcd_disable/panel_simple_led_disable. >>> >>> That's not what the simple panel driver can do. If we want this it needs >>> to be solved in a generic way for all panels since they all need to use >>> the drm_panel_*() functions to abstract away the details from drivers >>> that use the panels. >> Right. So only I have added pre_enable and post_disable callbacks. >> Using that name won't harm existing panel drivers and still addresses >> our requirement. >> >> >> Regards, >> Ajay > > > Thierry : > Are you really ok with the new callback names? like pre_enable and post_disable? > Adding those new callbacks really won't harm the existing panels anyhow. > > Daniel, Rob : > I think I have given sufficient amount of information as to why the concept of > drm_panel_bridge fails and why we cannot have callbacks for drm_panel > inside crtc helpers > or any other generic place. > > Please let me know your final opinion so that I can start reworking on > this series. > > Thanks and regards, > Ajay Kumar
diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h index c2ab77a..bf191df 100644 --- a/include/drm/drm_panel.h +++ b/include/drm/drm_panel.h @@ -31,7 +31,9 @@ struct drm_device; struct drm_panel; struct drm_panel_funcs { + int (*post_disable)(struct drm_panel *panel); int (*disable)(struct drm_panel *panel); + int (*pre_enable)(struct drm_panel *panel); int (*enable)(struct drm_panel *panel); int (*get_modes)(struct drm_panel *panel); }; @@ -46,6 +48,14 @@ struct drm_panel { struct list_head list; }; +static inline int drm_panel_post_disable(struct drm_panel *panel) +{ + if (panel && panel->funcs && panel->funcs->post_disable) + return panel->funcs->post_disable(panel); + + return panel ? -ENOSYS : -EINVAL; +} + static inline int drm_panel_disable(struct drm_panel *panel) { if (panel && panel->funcs && panel->funcs->disable) @@ -54,6 +64,14 @@ static inline int drm_panel_disable(struct drm_panel *panel) return panel ? -ENOSYS : -EINVAL; } +static inline int drm_panel_pre_enable(struct drm_panel *panel) +{ + if (panel && panel->funcs && panel->funcs->pre_enable) + return panel->funcs->pre_enable(panel); + + return panel ? -ENOSYS : -EINVAL; +} + static inline int drm_panel_enable(struct drm_panel *panel) { if (panel && panel->funcs && panel->funcs->enable)
Most of the panels need an init sequence as mentioned below: -- poweron LCD unit/LCD_EN -- start video data -- poweron LED unit/BL_EN And, a de-init sequence as mentioned below: -- poweroff LED unit/BL_EN -- stop video data -- poweroff LCD unit/LCD_EN With existing callbacks for drm panel, we cannot accomodate such panels, since only two callbacks, i.e "panel_enable" and panel_disable are supported. This patch adds: -- "pre_enable" callback which can be called before the actual video data is on, and then call the "enable" callback after the video data is available. -- "post_disable" callback which can be called after the video data is off, and use "disable" callback to do something before switching off the video data. Now, we can easily map the above scenario as shown below: poweron LCD unit/LCD_EN = "pre_enable" callback poweron LED unit/BL_EN = "enable" callback poweroff LED unit/BL_EN = "disable" callback poweroff LCD unit/LCD_EN = "post_disable" callback Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com> --- Changes since V1: Added post_disable callback include/drm/drm_panel.h | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)