ping!
On Fri, Apr 25, 2014 at 11:46 PM, Ajay kumar <ajaynumb at gmail.com> wrote: > On Fri, Apr 25, 2014 at 11:34 AM, Ajay kumar <ajaynumb at gmail.com> wrote: >> On Friday, April 25, 2014, Thierry Reding <thierry.reding at 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 at 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
