Thierry, On Wed, Apr 23, 2014 at 1:12 PM, Thierry Reding <thierry.reding@xxxxxxxxx> 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@xxxxxxxxx> >> > 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 _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
