| Message ID | 1340805944-28805-1-git-send-email-jaswinder.singh@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Wed, 2012-06-27 at 19:35 +0530, jaswinder.singh@linaro.org wrote: > From: Jassi Brar <jaswinder.singh@linaro.org> > > We can easily keep track of latest EDID from the display and hence avoid > expensive EDID re-reads over I2C. > This could also help some cheapo displays that provide EDID reliably only > immediately after asserting HPD and not later. > Even with good displays, there is something in OMAPDSS that apparantly > messes up DDC occasionally while EDID is being read, giving the > "operation stopped when reading edid" error. Btw, this is in nitpicking area, but what editor do you use? I find it difficult to read text that is not formatted properly =). At least vim formats text nicely with its formating commands. > Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org> > --- > drivers/video/omap2/dss/hdmi.c | 1 + > drivers/video/omap2/dss/ti_hdmi.h | 2 ++ > drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c | 15 +++++++++++++-- > 3 files changed, 16 insertions(+), 2 deletions(-) > > diff --git a/drivers/video/omap2/dss/hdmi.c b/drivers/video/omap2/dss/hdmi.c > index 0738090..9853621 100644 > --- a/drivers/video/omap2/dss/hdmi.c > +++ b/drivers/video/omap2/dss/hdmi.c > @@ -758,6 +758,7 @@ static int __init omapdss_hdmihw_probe(struct platform_device *pdev) > hdmi.ip_data.core_av_offset = HDMI_CORE_AV; > hdmi.ip_data.pll_offset = HDMI_PLLCTRL; > hdmi.ip_data.phy_offset = HDMI_PHY; > + hdmi.ip_data.edid_len = 0; /* Invalidate EDID Cache */ > mutex_init(&hdmi.ip_data.lock); > > hdmi_panel_init(); > diff --git a/drivers/video/omap2/dss/ti_hdmi.h b/drivers/video/omap2/dss/ti_hdmi.h > index cc292b8..4735860 100644 > --- a/drivers/video/omap2/dss/ti_hdmi.h > +++ b/drivers/video/omap2/dss/ti_hdmi.h > @@ -178,6 +178,8 @@ struct hdmi_ip_data { > /* ti_hdmi_4xxx_ip private data. These should be in a separate struct */ > int hpd_gpio; > struct mutex lock; > + u8 edid_cached[256]; > + unsigned edid_len; > }; > int ti_hdmi_4xxx_phy_enable(struct hdmi_ip_data *ip_data); > void ti_hdmi_4xxx_phy_disable(struct hdmi_ip_data *ip_data); > diff --git a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c > index 04acca9..b5c3dc4 100644 > --- a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c > +++ b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c > @@ -243,10 +243,13 @@ static int hdmi_check_hpd_state(struct hdmi_ip_data *ip_data) > > hpd = gpio_get_value(ip_data->hpd_gpio); > > - if (hpd) > + if (hpd) { > r = hdmi_set_phy_pwr(ip_data, HDMI_PHYPWRCMD_TXON); > - else > + } else { > + /* Invalidate EDID Cache */ > + ip_data->edid_len = 0; > r = hdmi_set_phy_pwr(ip_data, HDMI_PHYPWRCMD_LDOON); > + } There's a problem with this patch, which leaves a wrong EDID in the cache: if you first have the cable connected and hdmi is enabled, you then turn off the HDMI display device via sysfs, we do not go to hdmi_check_hpd_state at all. The next time hdmi is enabled, we only get the plug-in event, and thus EDID cache is never invalidated. Perhaps the EDID cache should be invalidated always in hdmi_check_hpd_state. In that case I think there's a chance (theoretical, perhaps), that we get two hpd interrupts, and for both the hpd gpio reads 1. I'm not quite sure what that would imply (perhaps we just missed the hpd gpio = 0 case), but even in that case invalidating the cache sounds ok. Tomi
On 28 June 2012 13:18, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote: > On Wed, 2012-06-27 at 19:35 +0530, jaswinder.singh@linaro.org wrote: >> From: Jassi Brar <jaswinder.singh@linaro.org> >> >> We can easily keep track of latest EDID from the display and hence avoid >> expensive EDID re-reads over I2C. >> This could also help some cheapo displays that provide EDID reliably only >> immediately after asserting HPD and not later. >> Even with good displays, there is something in OMAPDSS that apparantly >> messes up DDC occasionally while EDID is being read, giving the >> "operation stopped when reading edid" error. > > Btw, this is in nitpicking area, but what editor do you use? I find it > difficult to read text that is not formatted properly =). At least vim > formats text nicely with its formating commands. > Indeed a nitpick :) I use vim and, iirc, checkpatch.pl gave 0 warning. Perhaps my poor cmoposition. Please do tell how I could I make it more appealing to you ? >> --- a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c >> +++ b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c >> @@ -243,10 +243,13 @@ static int hdmi_check_hpd_state(struct hdmi_ip_data *ip_data) >> >> hpd = gpio_get_value(ip_data->hpd_gpio); >> >> - if (hpd) >> + if (hpd) { >> r = hdmi_set_phy_pwr(ip_data, HDMI_PHYPWRCMD_TXON); >> - else >> + } else { >> + /* Invalidate EDID Cache */ >> + ip_data->edid_len = 0; >> r = hdmi_set_phy_pwr(ip_data, HDMI_PHYPWRCMD_LDOON); >> + } > > There's a problem with this patch, which leaves a wrong EDID in the > cache: if you first have the cable connected and hdmi is enabled, you > then turn off the HDMI display device via sysfs, we do not go to > hdmi_check_hpd_state at all. The next time hdmi is enabled, we only get > the plug-in event, and thus EDID cache is never invalidated. > If the hdmi cable is not replugged during that period, I don't see why would you want the EDID invalidated ?
On Thu, 2012-06-28 at 15:18 +0530, Jassi Brar wrote: > On 28 June 2012 13:18, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote: > > On Wed, 2012-06-27 at 19:35 +0530, jaswinder.singh@linaro.org wrote: > >> From: Jassi Brar <jaswinder.singh@linaro.org> > >> > >> We can easily keep track of latest EDID from the display and hence avoid > >> expensive EDID re-reads over I2C. > >> This could also help some cheapo displays that provide EDID reliably only > >> immediately after asserting HPD and not later. > >> Even with good displays, there is something in OMAPDSS that apparantly > >> messes up DDC occasionally while EDID is being read, giving the > >> "operation stopped when reading edid" error. > > > > Btw, this is in nitpicking area, but what editor do you use? I find it > > difficult to read text that is not formatted properly =). At least vim > > formats text nicely with its formating commands. > > > Indeed a nitpick :) > I use vim and, iirc, checkpatch.pl gave 0 warning. Perhaps my poor > cmoposition. Please do tell how I could I make it more appealing to > you ? Like: --- We can easily keep track of latest EDID from the display and hence avoid expensive EDID re-reads over I2C. This could also help some cheapo displays that provide EDID reliably only immediately after asserting HPD and not later. Even with good displays, there is something in OMAPDSS that apparantly messes up DDC occasionally while EDID is being read, giving the "operation stopped when reading edid" error. --- So basically two things: properly formatted paragraphs and an empty line between paragraphs. With "properly formatted" I mean that that the text goes from the beginning of the line to the end, so that the text fills the whole line. This can be done easily in vim with first painting the paragraph (or multiple paragraphs), and then then press g and w. I think those simple rules make the text much easier to read. > >> --- a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c > >> +++ b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c > >> @@ -243,10 +243,13 @@ static int hdmi_check_hpd_state(struct hdmi_ip_data *ip_data) > >> > >> hpd = gpio_get_value(ip_data->hpd_gpio); > >> > >> - if (hpd) > >> + if (hpd) { > >> r = hdmi_set_phy_pwr(ip_data, HDMI_PHYPWRCMD_TXON); > >> - else > >> + } else { > >> + /* Invalidate EDID Cache */ > >> + ip_data->edid_len = 0; > >> r = hdmi_set_phy_pwr(ip_data, HDMI_PHYPWRCMD_LDOON); > >> + } > > > > There's a problem with this patch, which leaves a wrong EDID in the > > cache: if you first have the cable connected and hdmi is enabled, you > > then turn off the HDMI display device via sysfs, we do not go to > > hdmi_check_hpd_state at all. The next time hdmi is enabled, we only get > > the plug-in event, and thus EDID cache is never invalidated. > > > If the hdmi cable is not replugged during that period, I don't see why > would you want the EDID invalidated ? I wasn't very clear with my comment. When the display device is disabled, we're not listening to the hpd interrupt anymore. So when it's disabled, the cable can be replugged and the monitor changed, and the driver won't know about it. And so it'll return the old EDID for the new monitor. Tomi
On 28 June 2012 15:44, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote: > On Thu, 2012-06-28 at 15:18 +0530, Jassi Brar wrote: >> >> --- a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c >> >> +++ b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c >> >> @@ -243,10 +243,13 @@ static int hdmi_check_hpd_state(struct hdmi_ip_data *ip_data) >> >> >> >> hpd = gpio_get_value(ip_data->hpd_gpio); >> >> >> >> - if (hpd) >> >> + if (hpd) { >> >> r = hdmi_set_phy_pwr(ip_data, HDMI_PHYPWRCMD_TXON); >> >> - else >> >> + } else { >> >> + /* Invalidate EDID Cache */ >> >> + ip_data->edid_len = 0; >> >> r = hdmi_set_phy_pwr(ip_data, HDMI_PHYPWRCMD_LDOON); >> >> + } >> > >> > There's a problem with this patch, which leaves a wrong EDID in the >> > cache: if you first have the cable connected and hdmi is enabled, you >> > then turn off the HDMI display device via sysfs, we do not go to >> > hdmi_check_hpd_state at all. The next time hdmi is enabled, we only get >> > the plug-in event, and thus EDID cache is never invalidated. >> > >> If the hdmi cable is not replugged during that period, I don't see why >> would you want the EDID invalidated ? > > I wasn't very clear with my comment. > > When the display device is disabled, we're not listening to the hpd > interrupt anymore. So when it's disabled, the cable can be replugged and > the monitor changed, and the driver won't know about it. And so it'll > return the old EDID for the new monitor. > If that could be a problem, then we already have some problem with current omapdss. I think among the first things, while enabling HDMI, should be to see if there is really some display connected on the port i.e, HPD asserted. Only if ti_hdmi_4xxx_detect() returned true, should we proceed otherwise wait for HPQ irq. Unconditionally invalidating edid really seems like a regression - we impose atleast 50ms (edid read) as extra cost on hdmi_check_hpd_state(), which kills half the purpose of this patch.
On 28 June 2012 16:17, Jassi Brar <jaswinder.singh@linaro.org> wrote: > On 28 June 2012 15:44, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote: >> On Thu, 2012-06-28 at 15:18 +0530, Jassi Brar wrote: > >>> >> --- a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c >>> >> +++ b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c >>> >> @@ -243,10 +243,13 @@ static int hdmi_check_hpd_state(struct hdmi_ip_data *ip_data) >>> >> >>> >> hpd = gpio_get_value(ip_data->hpd_gpio); >>> >> >>> >> - if (hpd) >>> >> + if (hpd) { >>> >> r = hdmi_set_phy_pwr(ip_data, HDMI_PHYPWRCMD_TXON); >>> >> - else >>> >> + } else { >>> >> + /* Invalidate EDID Cache */ >>> >> + ip_data->edid_len = 0; >>> >> r = hdmi_set_phy_pwr(ip_data, HDMI_PHYPWRCMD_LDOON); >>> >> + } >>> > >>> > There's a problem with this patch, which leaves a wrong EDID in the >>> > cache: if you first have the cable connected and hdmi is enabled, you >>> > then turn off the HDMI display device via sysfs, we do not go to >>> > hdmi_check_hpd_state at all. The next time hdmi is enabled, we only get >>> > the plug-in event, and thus EDID cache is never invalidated. >>> > >>> If the hdmi cable is not replugged during that period, I don't see why >>> would you want the EDID invalidated ? >> >> I wasn't very clear with my comment. >> >> When the display device is disabled, we're not listening to the hpd >> interrupt anymore. So when it's disabled, the cable can be replugged and >> the monitor changed, and the driver won't know about it. And so it'll >> return the old EDID for the new monitor. >> > If that could be a problem, then we already have some problem with > current omapdss. > > I think among the first things, while enabling HDMI, should be to see > if there is really some display connected on the port i.e, HPD > asserted. Only if ti_hdmi_4_detect() returned true, should we > proceed otherwise wait for HPQ irq. > > Unconditionally invalidating edid really seems like a regression - we > impose atleast 50ms (edid read) as extra cost on > hdmi_check_hpd_state(), which kills half the purpose of this patch. > Sorry a correction. Reading detect() won't work. I suggest we keep HPD IRQ enabled for the lifetime of the driver.
On Thu, 2012-06-28 at 16:17 +0530, Jassi Brar wrote: > On 28 June 2012 15:44, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote: > > When the display device is disabled, we're not listening to the hpd > > interrupt anymore. So when it's disabled, the cable can be replugged and > > the monitor changed, and the driver won't know about it. And so it'll > > return the old EDID for the new monitor. > > > If that could be a problem, then we already have some problem with > current omapdss. > > I think among the first things, while enabling HDMI, should be to see > if there is really some display connected on the port i.e, HPD > asserted. Only if ti_hdmi_4xxx_detect() returned true, should we > proceed otherwise wait for HPQ irq. I'm not sure I understand. The HDMI driver does just that. It calls hdmi_check_hpd_state when it's being enabled to see if there's a display connected. > Unconditionally invalidating edid really seems like a regression - we > impose atleast 50ms (edid read) as extra cost on > hdmi_check_hpd_state(), which kills half the purpose of this patch. If the HDMI hardware is turned off, we should unconditionally invalidate the EDID. We have no way to keep track if the same monitor is kept plugged in or not. So what exactly is the purpose of this patch, what kind of scenario? I thought it's to cache the EDID, so that if it will be read multiple times while the same monitor is connected, we'll just return the cached value. Of course, I don't know why the upper layers would read the EDID multiple times, because I think they should read it only once. So perhaps I'm either not understanding something, or it's the omapdrm layer that should be fixed? Tomi
On Thu, 2012-06-28 at 16:28 +0530, Jassi Brar wrote: > On 28 June 2012 16:17, Jassi Brar <jaswinder.singh@linaro.org> wrote: > > On 28 June 2012 15:44, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote: > >> On Thu, 2012-06-28 at 15:18 +0530, Jassi Brar wrote: > > > >>> >> --- a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c > >>> >> +++ b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c > >>> >> @@ -243,10 +243,13 @@ static int hdmi_check_hpd_state(struct hdmi_ip_data *ip_data) > >>> >> > >>> >> hpd = gpio_get_value(ip_data->hpd_gpio); > >>> >> > >>> >> - if (hpd) > >>> >> + if (hpd) { > >>> >> r = hdmi_set_phy_pwr(ip_data, HDMI_PHYPWRCMD_TXON); > >>> >> - else > >>> >> + } else { > >>> >> + /* Invalidate EDID Cache */ > >>> >> + ip_data->edid_len = 0; > >>> >> r = hdmi_set_phy_pwr(ip_data, HDMI_PHYPWRCMD_LDOON); > >>> >> + } > >>> > > >>> > There's a problem with this patch, which leaves a wrong EDID in the > >>> > cache: if you first have the cable connected and hdmi is enabled, you > >>> > then turn off the HDMI display device via sysfs, we do not go to > >>> > hdmi_check_hpd_state at all. The next time hdmi is enabled, we only get > >>> > the plug-in event, and thus EDID cache is never invalidated. > >>> > > >>> If the hdmi cable is not replugged during that period, I don't see why > >>> would you want the EDID invalidated ? > >> > >> I wasn't very clear with my comment. > >> > >> When the display device is disabled, we're not listening to the hpd > >> interrupt anymore. So when it's disabled, the cable can be replugged and > >> the monitor changed, and the driver won't know about it. And so it'll > >> return the old EDID for the new monitor. > >> > > If that could be a problem, then we already have some problem with > > current omapdss. > > > > I think among the first things, while enabling HDMI, should be to see > > if there is really some display connected on the port i.e, HPD > > asserted. Only if ti_hdmi_4_detect() returned true, should we > > proceed otherwise wait for HPQ irq. > > > > Unconditionally invalidating edid really seems like a regression - we > > impose atleast 50ms (edid read) as extra cost on > > hdmi_check_hpd_state(), which kills half the purpose of this patch. > > > Sorry a correction. Reading detect() won't work. I suggest we keep HPD > IRQ enabled for the lifetime of the driver. Ok, I see. But that's not acceptable. It would require us to keep the TPD12S015 always powered and enabled. Even if you're not interested in using HDMI at all. For this to work like you want we need a bigger restructuring of HDMI and partly omapdss also. Currently we have just one big "enabled" or "disabled" state. We need multiple states. Probably something like: - disabled, everything totally off - low power, hotplug detection enabled - powered on, but no video - video enabled Been long in my mind, but I'm not very familiar with HDMI so I could get my simple prototypes to work when I tried something like this once. Tomi
On Thu, 2012-06-28 at 14:10 +0300, Tomi Valkeinen wrote: > On Thu, 2012-06-28 at 16:28 +0530, Jassi Brar wrote: > > Sorry a correction. Reading detect() won't work. I suggest we keep HPD > > IRQ enabled for the lifetime of the driver. > > Ok, I see. But that's not acceptable. It would require us to keep the > TPD12S015 always powered and enabled. Even if you're not interested in > using HDMI at all. Btw, a bigger problem that I see is how we have to do read_edid() (and detect(), if I recall correctly): we enable the whole video pipeline and output. We should only enable enough of the HW to be able to read the EDID or read the HPD GPIO. I've noticed that this leads to sync losts quite easily, as we switch the hdmi output on and off quickly multiple times. I couldn't figure out why the sync losts happen though, and I did try quite many different combinations how to handle it. Tomi
On 06/28/12 19:10, the mail apparently from Tomi Valkeinen included: > On Thu, 2012-06-28 at 16:28 +0530, Jassi Brar wrote: >> On 28 June 2012 16:17, Jassi Brar <jaswinder.singh@linaro.org> wrote: >>> On 28 June 2012 15:44, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote: >>>> On Thu, 2012-06-28 at 15:18 +0530, Jassi Brar wrote: >>> >>>>>>> --- a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c >>>>>>> +++ b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c >>>>>>> @@ -243,10 +243,13 @@ static int hdmi_check_hpd_state(struct hdmi_ip_data *ip_data) >>>>>>> >>>>>>> hpd = gpio_get_value(ip_data->hpd_gpio); >>>>>>> >>>>>>> - if (hpd) >>>>>>> + if (hpd) { >>>>>>> r = hdmi_set_phy_pwr(ip_data, HDMI_PHYPWRCMD_TXON); >>>>>>> - else >>>>>>> + } else { >>>>>>> + /* Invalidate EDID Cache */ >>>>>>> + ip_data->edid_len = 0; >>>>>>> r = hdmi_set_phy_pwr(ip_data, HDMI_PHYPWRCMD_LDOON); >>>>>>> + } >>>>>> >>>>>> There's a problem with this patch, which leaves a wrong EDID in the >>>>>> cache: if you first have the cable connected and hdmi is enabled, you >>>>>> then turn off the HDMI display device via sysfs, we do not go to >>>>>> hdmi_check_hpd_state at all. The next time hdmi is enabled, we only get >>>>>> the plug-in event, and thus EDID cache is never invalidated. >>>>>> >>>>> If the hdmi cable is not replugged during that period, I don't see why >>>>> would you want the EDID invalidated ? >>>> >>>> I wasn't very clear with my comment. >>>> >>>> When the display device is disabled, we're not listening to the hpd >>>> interrupt anymore. So when it's disabled, the cable can be replugged and >>>> the monitor changed, and the driver won't know about it. And so it'll >>>> return the old EDID for the new monitor. >>>> >>> If that could be a problem, then we already have some problem with >>> current omapdss. >>> >>> I think among the first things, while enabling HDMI, should be to see >>> if there is really some display connected on the port i.e, HPD >>> asserted. Only if ti_hdmi_4_detect() returned true, should we >>> proceed otherwise wait for HPQ irq. >>> >>> Unconditionally invalidating edid really seems like a regression - we >>> impose atleast 50ms (edid read) as extra cost on >>> hdmi_check_hpd_state(), which kills half the purpose of this patch. >>> >> Sorry a correction. Reading detect() won't work. I suggest we keep HPD >> IRQ enabled for the lifetime of the driver. > > Ok, I see. But that's not acceptable. It would require us to keep the > TPD12S015 always powered and enabled. Even if you're not interested in > using HDMI at all. > > For this to work like you want we need a bigger restructuring of HDMI > and partly omapdss also. Currently we have just one big "enabled" or > "disabled" state. We need multiple states. Probably something like: > > - disabled, everything totally off > - low power, hotplug detection enabled > - powered on, but no video > - video enabled > > Been long in my mind, but I'm not very familiar with HDMI so I could get > my simple prototypes to work when I tried something like this once. That doesn't sound too hard since the difference between the first three states at the HDMI chip is just whether the two gpio controlling it are 00, 10 or 11. If Jassi's alright with it we might have a go at implementing this, but can you define a bit more about how we logically tell DSS that we want to, eg, disable HDMI totally? -Andy
On 06/28/12 19:38, the mail apparently from Tomi Valkeinen included: > On Thu, 2012-06-28 at 14:10 +0300, Tomi Valkeinen wrote: >> On Thu, 2012-06-28 at 16:28 +0530, Jassi Brar wrote: > >>> Sorry a correction. Reading detect() won't work. I suggest we keep HPD >>> IRQ enabled for the lifetime of the driver. >> >> Ok, I see. But that's not acceptable. It would require us to keep the >> TPD12S015 always powered and enabled. Even if you're not interested in >> using HDMI at all. > > Btw, a bigger problem that I see is how we have to do read_edid() (and > detect(), if I recall correctly): we enable the whole video pipeline and > output. We should only enable enough of the HW to be able to read the > EDID or read the HPD GPIO. > > I've noticed that this leads to sync losts quite easily, as we switch > the hdmi output on and off quickly multiple times. I couldn't figure out > why the sync losts happen though, and I did try quite many different > combinations how to handle it. SYNC LOST is one evil lurking in there, the other evil is EDID fetch "operation stopped" error. We were unable to figure out what was trampling on what there without the SoC HDMI PHY IP data which we don't have. Also at the moment I think we depower / repower the internal SoC and external PHY chip more than we need. Each time we remove the HDMI link power, after a short time where the big capacitor at the charge pump output decays enough (a time dependent on exact details of load presented by the TV or monitor...), hpd from the monitor goes low and remains there until we power the charge pump again. In turn the new hpd recognition provokes a new edid fetch. Something about that sequence provokes the "operation stopped" on EDID fetch, with Jassi's patches we manage to avoid it. Removing that syndrome, and just not enabling and disabling stuff like SoC HDMI PHY willy nilly can maybe be something else targeted by this proposed 4-state power scheme. -Andy
On 28 June 2012 16:40, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote: > On Thu, 2012-06-28 at 16:28 +0530, Jassi Brar wrote: > > >> Sorry a correction. Reading detect() won't work. I suggest we keep HPD >> IRQ enabled for the lifetime of the driver. > > Ok, I see. But that's not acceptable. It would require us to keep the > TPD12S015 always powered and enabled. Even if you're not interested in > using HDMI at all. > I think we need to differentiate between HDMI PHY enable and HDMI 5V+,HPD enable [1]... currently they are clubbed together in omap_dss_device.platform_enable. AFAIK, at least with TPD12S015, they can be controlled independently and PHY enabling is actually the main source of power consumption if no display is connected. By 'lifetime' I mean when the end-user selects some option to the effect of "Automatically detect and configure display over HDMI" .... and then we simply enable the HDMI 5V+/HPD, HDMI-PHY would be enabled only when we actually detect HPD asserted. If a device doesn't have a port or the user doesn't have a display, neither would be ever enabled. I mean we should provide a way to make it platform dependent. [1] Thanks to Andy and his crappy TV, he found clubbing enabling PHY with 5V+ application comes in the way of detecting cheapo displays that take ~700ms before asserting HPD i.e, making EDID available. See how we don't leave it to a HDMI display to take it's own time before asserting HPD - omapdss_hdmi_display_enable/disable pairs don't care for that.
On Thu, 2012-06-28 at 20:03 +0800, Andy Green wrote: > On 06/28/12 19:10, the mail apparently from Tomi Valkeinen included: > > For this to work like you want we need a bigger restructuring of HDMI > > and partly omapdss also. Currently we have just one big "enabled" or > > "disabled" state. We need multiple states. Probably something like: > > > > - disabled, everything totally off > > - low power, hotplug detection enabled > > - powered on, but no video > > - video enabled > > > > Been long in my mind, but I'm not very familiar with HDMI so I could get > > my simple prototypes to work when I tried something like this once. > > That doesn't sound too hard since the difference between the first three > states at the HDMI chip is just whether the two gpio controlling it are > 00, 10 or 11. I don't think it's that simple. We should be able to do EDID read on one of the states, perhaps second or third. For that we need parts of the HDMI IP to be enabled. Also, the third state should be something where the IP is fully functional. For DSI this would mean that you can communicate over DSI bus etc. So I guess EDID read should be possible at this level. > If Jassi's alright with it we might have a go at implementing this, but > can you define a bit more about how we logically tell DSS that we want > to, eg, disable HDMI totally? Disabling HDMI is easy, it's already done by the disable call. And the same for the video enabled mode. The middle ones are the ones that need implementation. And for HDMI, there's currently no way to enable it partially. This is what I tried with my quick hack, but I couldn't figure out what parts need to be enabled (but I didn't spend much time on it). This is something that should be implemented for all outputs, obviously, but we could approach it bit by bit. For example, implement it first for HDMI, and all other outputs just consider anything else than "disabled" as full enable. Tomi
On 28 June 2012 17:33, Andy Green <andy.green@linaro.org> wrote: > On 06/28/12 19:10, the mail apparently from Tomi Valkeinen included: > >> On Thu, 2012-06-28 at 16:28 +0530, Jassi Brar wrote: >>> >>> On 28 June 2012 16:17, Jassi Brar <jaswinder.singh@linaro.org> wrote: >>>> >>>> On 28 June 2012 15:44, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote: >>>>> >>>>> On Thu, 2012-06-28 at 15:18 +0530, Jassi Brar wrote: >>>> >>>> >>>>>>>> --- a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c >>>>>>>> +++ b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c >>>>>>>> @@ -243,10 +243,13 @@ static int hdmi_check_hpd_state(struct >>>>>>>> hdmi_ip_data *ip_data) >>>>>>>> >>>>>>>> hpd = gpio_get_value(ip_data->hpd_gpio); >>>>>>>> >>>>>>>> - if (hpd) >>>>>>>> + if (hpd) { >>>>>>>> r = hdmi_set_phy_pwr(ip_data, HDMI_PHYPWRCMD_TXON); >>>>>>>> - else >>>>>>>> + } else { >>>>>>>> + /* Invalidate EDID Cache */ >>>>>>>> + ip_data->edid_len = 0; >>>>>>>> r = hdmi_set_phy_pwr(ip_data, HDMI_PHYPWRCMD_LDOON); >>>>>>>> + } >>>>>>> >>>>>>> >>>>>>> There's a problem with this patch, which leaves a wrong EDID in the >>>>>>> cache: if you first have the cable connected and hdmi is enabled, you >>>>>>> then turn off the HDMI display device via sysfs, we do not go to >>>>>>> hdmi_check_hpd_state at all. The next time hdmi is enabled, we only >>>>>>> get >>>>>>> the plug-in event, and thus EDID cache is never invalidated. >>>>>>> >>>>>> If the hdmi cable is not replugged during that period, I don't see why >>>>>> would you want the EDID invalidated ? >>>>> >>>>> >>>>> I wasn't very clear with my comment. >>>>> >>>>> When the display device is disabled, we're not listening to the hpd >>>>> interrupt anymore. So when it's disabled, the cable can be replugged >>>>> and >>>>> the monitor changed, and the driver won't know about it. And so it'll >>>>> return the old EDID for the new monitor. >>>>> >>>> If that could be a problem, then we already have some problem with >>>> current omapdss. >>>> >>>> I think among the first things, while enabling HDMI, should be to see >>>> if there is really some display connected on the port i.e, HPD >>>> asserted. Only if ti_hdmi_4_detect() returned true, should we >>>> proceed otherwise wait for HPQ irq. >>>> >>>> Unconditionally invalidating edid really seems like a regression - we >>>> impose atleast 50ms (edid read) as extra cost on >>>> hdmi_check_hpd_state(), which kills half the purpose of this patch. >>>> >>> Sorry a correction. Reading detect() won't work. I suggest we keep HPD >>> IRQ enabled for the lifetime of the driver. >> >> >> Ok, I see. But that's not acceptable. It would require us to keep the >> TPD12S015 always powered and enabled. Even if you're not interested in >> using HDMI at all. >> >> For this to work like you want we need a bigger restructuring of HDMI >> and partly omapdss also. Currently we have just one big "enabled" or >> "disabled" state. We need multiple states. Probably something like: >> >> - disabled, everything totally off >> - low power, hotplug detection enabled >> - powered on, but no video >> - video enabled >> >> Been long in my mind, but I'm not very familiar with HDMI so I could get >> my simple prototypes to work when I tried something like this once. > > > That doesn't sound too hard since the difference between the first three > states at the HDMI chip is just whether the two gpio controlling it are 00, > 10 or 11. > > If Jassi's alright with it we might have a go at implementing this, but can > you define a bit more about how we logically tell DSS that we want to, eg, > disable HDMI totally? > A quick reaction of my guts say, we simply enable 5V/HPD_IRQ during probe and disable during remove. HDMI enable/disable via /sysfs/ and HPD (de)assertion, switch only HDMI_PHY on/off. The user selecting "Autodetect and Configure" option would then equate to "(un)loading" of the HDMI driver. Not to mean a trivial job.
On Thu, 2012-06-28 at 18:43 +0530, Jassi Brar wrote: > On 28 June 2012 17:33, Andy Green <andy.green@linaro.org> wrote: > > If Jassi's alright with it we might have a go at implementing this, but can > > you define a bit more about how we logically tell DSS that we want to, eg, > > disable HDMI totally? > > > A quick reaction of my guts say, we simply enable 5V/HPD_IRQ during > probe and disable during remove. The problem with this is a feature of omapdss: we can have multiple displays for the same output, of which only one can be enabled at the same time. What this means is that you shouldn't (and in some cases can't) allocate or enable resources in probe that may be shared, because then the driver for both displays would try to allocate the same resource. Sure, this is not a problem for the HDMI configuration we are using now, but it's still against the panel model we have. Thus we should allocate resources only when the panel device is turned on, and release them when it's disabled. I do think the model is slightly broken, but that's what we have now. And I'm also not even sure how it should be fixed... And also, as I said earlier, if you keep it enabled all the time, it'll eat power even if the user is never going to use HDMI. On a desktop I guess the power consumption wouldn't be an issue, but I do feel a bit uneasy about it on an embedded device. > HDMI enable/disable via /sysfs/ and HPD (de)assertion, switch only > HDMI_PHY on/off. > The user selecting "Autodetect and Configure" option would then equate > to "(un)loading" of the HDMI driver. HDMI cannot be currently compiled as a separate module. Although I think you can detach a device and a driver, achieving the same. Is that what you meant with unloading? Tomi
On Thu, 2012-06-28 at 16:28 +0530, Jassi Brar wrote: > > I think among the first things, while enabling HDMI, should be to see > > if there is really some display connected on the port i.e, HPD > > asserted. Only if ti_hdmi_4_detect() returned true, should we > > proceed otherwise wait for HPQ irq. > > > > Unconditionally invalidating edid really seems like a regression - we > > impose atleast 50ms (edid read) as extra cost on > > hdmi_check_hpd_state(), which kills half the purpose of this patch. > > > Sorry a correction. Reading detect() won't work. I suggest we keep HPD > IRQ enabled for the lifetime of the driver. By the way, when the device is in system suspend, we surely won't detect the HPD even if we kept the HPD always enabled. So there we'll miss the HPD interrupt anyway, and the EDID cache would be invalid. Tomi
On Thu, 2012-06-28 at 18:43 +0530, Jassi Brar wrote: > A quick reaction of my guts say, we simply enable 5V/HPD_IRQ during > probe and disable during remove. > HDMI enable/disable via /sysfs/ and HPD (de)assertion, switch only > HDMI_PHY on/off. > The user selecting "Autodetect and Configure" option would then equate > to "(un)loading" of the HDMI driver. > Not to mean a trivial job. One more thing I realized while thinking about this: While it could be argued that the power draw from having the tpd12s015 always enabled is very small, I think it could matter. If you consider a phone with HDMI output, it's likely that the phone is locked 99% of the time. When the phone is locked, there's no need to keep the HDMI HPD enabled. So this could add to a considerable amount of power wasted, if the HPD was always enabled. At least I can't figure out a reason why one would want the HPD to work when the phone is locked. Also, I have never used the HDMI output on my phone, so I'd be glad if it was totally powered off if it gave me more standby hours =). Tomi
On 28 June 2012 19:01, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote: > On Thu, 2012-06-28 at 18:43 +0530, Jassi Brar wrote: >> On 28 June 2012 17:33, Andy Green <andy.green@linaro.org> wrote: > >> > If Jassi's alright with it we might have a go at implementing this, but can >> > you define a bit more about how we logically tell DSS that we want to, eg, >> > disable HDMI totally? >> > >> A quick reaction of my guts say, we simply enable 5V/HPD_IRQ during >> probe and disable during remove. > > The problem with this is a feature of omapdss: we can have multiple > displays for the same output, of which only one can be enabled at the > same time. What this means is that you shouldn't (and in some cases > can't) allocate or enable resources in probe that may be shared, because > then the driver for both displays would try to allocate the same > resource. > > Sure, this is not a problem for the HDMI configuration we are using now, > but it's still against the panel model we have. Thus we should allocate > resources only when the panel device is turned on, and release them when > it's disabled. > > I do think the model is slightly broken, but that's what we have now. > And I'm also not even sure how it should be fixed... > I won't press further with my Utopian ideas, but I think we need to segregate 5V/HPD enabling from PHY enabling somehow. Because that is already failing slow but otherwise perfectly legit displays (like Andy's "HPD taking 700ms" TV) > And also, as I said earlier, if you keep it enabled all the time, it'll > eat power even if the user is never going to use HDMI. > > On a desktop I guess the power consumption wouldn't be an issue, but I > do feel a bit uneasy about it on an embedded device. > As I said, it should be platform dependent. If a device doesn't have HDMI port, the board file would not even have platform_enable. And if it has, some user action should enable it while 'making the device ready for new display'. IOW, how do you envision an OMAP4 based tablet with HDMI port react to display connections ? >> HDMI enable/disable via /sysfs/ and HPD (de)assertion, switch only >> HDMI_PHY on/off. >> The user selecting "Autodetect and Configure" option would then equate >> to "(un)loading" of the HDMI driver. > > HDMI cannot be currently compiled as a separate module. Although I think > you can detach a device and a driver, achieving the same. Is that what > you meant with unloading? > Yeah, I meant something to the effect of bringing HDMI driver to life. > By the way, when the device is in system suspend, we surely won't detect > the HPD even if we kept the HPD always enabled. So there we'll miss the > HPD interrupt anyway, and the EDID cache would be invalid. > If omapdss already handles the possibility of display changed during suspend, I think we should be good :)
On 28 June 2012 20:44, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote: > On Thu, 2012-06-28 at 18:43 +0530, Jassi Brar wrote: > >> A quick reaction of my guts say, we simply enable 5V/HPD_IRQ during >> probe and disable during remove. >> HDMI enable/disable via /sysfs/ and HPD (de)assertion, switch only >> HDMI_PHY on/off. >> The user selecting "Autodetect and Configure" option would then equate >> to "(un)loading" of the HDMI driver. >> Not to mean a trivial job. > > One more thing I realized while thinking about this: > > While it could be argued that the power draw from having the tpd12s015 > always enabled is very small, I think it could matter. If you consider a > phone with HDMI output, it's likely that the phone is locked 99% of the > time. When the phone is locked, there's no need to keep the HDMI HPD > enabled. So this could add to a considerable amount of power wasted, if > the HPD was always enabled. > > At least I can't figure out a reason why one would want the HPD to work > when the phone is locked. Also, I have never used the HDMI output on my > phone, so I'd be glad if it was totally powered off if it gave me more > standby hours =). > Of course, I don't suggest imposing any hard rule here. All I suggest is make it platform dependent and provide a way from user-space too to enable/disable HPD.
On Thu, 2012-06-28 at 20:44 +0530, Jassi Brar wrote: > I won't press further with my Utopian ideas, but I think we need to > segregate 5V/HPD enabling from PHY enabling somehow. Because that is > already failing slow but otherwise perfectly legit displays (like > Andy's "HPD taking 700ms" TV) Yes, I agree. That's what the four power states I suggested in the other mail were about. The second power state would have HPD enabled, and the third would enable the PHY. Or at least enough to do i2c transfers, I'm not sure if the PHY is needed for that. > > And also, as I said earlier, if you keep it enabled all the time, it'll > > eat power even if the user is never going to use HDMI. > > > > On a desktop I guess the power consumption wouldn't be an issue, but I > > do feel a bit uneasy about it on an embedded device. > > > As I said, it should be platform dependent. If a device doesn't have > HDMI port, the board file would not even have platform_enable. And if Not that it's relevant here, but I have a patch series where I remove the platform_enable stuff for HDMI, and move the work to the hdmi driver. That needs to be done for device tree stuff, when we don't have board files. > it has, some user action should enable it while 'making the device > ready for new display'. > IOW, how do you envision an OMAP4 based tablet with HDMI port react to > display connections ? I guess this was covered in my mail about the phone's HDMI. If the tablet is unlocked, and I plug in a HDMI cable, I expect the device to do something. Either clone the display, or perhaps ask me what I want to do. So yes, HPD would be always enabled, when the tablet is active (unlocked). > >> HDMI enable/disable via /sysfs/ and HPD (de)assertion, switch only > >> HDMI_PHY on/off. > >> The user selecting "Autodetect and Configure" option would then equate > >> to "(un)loading" of the HDMI driver. > > > > HDMI cannot be currently compiled as a separate module. Although I think > > you can detach a device and a driver, achieving the same. Is that what > > you meant with unloading? > > > Yeah, I meant something to the effect of bringing HDMI driver to life. > > > > By the way, when the device is in system suspend, we surely won't detect > > the HPD even if we kept the HPD always enabled. So there we'll miss the > > HPD interrupt anyway, and the EDID cache would be invalid. > > > If omapdss already handles the possibility of display changed during > suspend, I think we should be good :) Hmm I'm not sure I understand what you mean. I was referring to your patch, which invalidated the EDID cache only on HPD interrupt when the cable is unplugged. And we'd miss that interrupt when the board is in system suspend, even if we otherwise kept the HPD interrupt always enabled. Tomi
My $.02 from a set top box designer perspective., HPD is only an advisory signal indicating that the connected display has changed while you are actively monitoring connection state. If HPD under-goes any state transition, or if you enter and exit any sleep or standby mode where you could potentially miss a cable change by the user, you always need to check for receiver presence and then re-read EDID. You can check for receiver-on a variety of ways before re-reading EDID. Most HDMI MACs have a status bit based on impedance sense you can read - usually requiring a powered phy. You could also do a DDC 0 byte write ack/nack check. It doesn't hurt to reread EDID. Even if it's just to verify a hash and keep the same TMDS timings. Ideally you would use an EDID hash anyway to look-up a given connected monitor profile in a persisted table of mode preferences when evaluating EDID data verses output capabilities. I generally reevaluate EDID data verses the user stored preferences for the EDID hash. Based on that, I generate all the programmable properties for the output sub-system - dot clock, color depth, color conversion, AVInfoFrames, and clock tree pad configuration for the sourcing device. If anything is different in that data set verses what is currently programmed into the devices, I'll apply a differential update to the output settings. For example if the user adjusts their EDID preferences rather than the data itself changing, it might only be necessary to switch to deep color output and not have to drop TMDS timing completely. Likewise it might not be necessary to reprogram the entire video sub-system for a digital output change - leaving the analog outputs stable and glitch free during HDMI hotplug. -Alan
I'm also a little bit confused why a PHY discussion applies to HPD or DDC signals - unless you mean powering up/down the MAC/tranceiver as well? HPS is typically just a level shifting FET, TVS diode and pull-up. The only reason HDMI MACs even have an input for it is to combine the event into the normal interrupt generation structure. If it's completely integrated on the OMAP, I would hope it could be used as a normal GPIO and/or system standby wake-up interrupt without the MAC or PHY being powered. DDC is the same way (FET+TVS+PU). -Alan
On 28 June 2012 20:57, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote: > On Thu, 2012-06-28 at 20:44 +0530, Jassi Brar wrote: >> it has, some user action should enable it while 'making the device >> ready for new display'. >> IOW, how do you envision an OMAP4 based tablet with HDMI port react to >> display connections ? > > I guess this was covered in my mail about the phone's HDMI. If the > tablet is unlocked, and I plug in a HDMI cable, I expect the device to > do something. Either clone the display, or perhaps ask me what I want to > do. > > So yes, HPD would be always enabled, when the tablet is active > (unlocked). > OK, somehow I was under impression you didn't wanna spare even the 5V+ floating. Though there could also be some option in settings to enable 5/HPD only when the user is about the connect a display... so that the activate window is even narrowed down. Anyway... I am glad we are in sync. >> > By the way, when the device is in system suspend, we surely won't detect >> > the HPD even if we kept the HPD always enabled. So there we'll miss the >> > HPD interrupt anyway, and the EDID cache would be invalid. >> > >> If omapdss already handles the possibility of display changed during >> suspend, I think we should be good :) > > Hmm I'm not sure I understand what you mean. I was referring to your > patch, which invalidated the EDID cache only on HPD interrupt when the > cable is unplugged. And we'd miss that interrupt when the board is in > system suspend, even if we otherwise kept the HPD interrupt always > enabled. > I meant before stale-edid, we face potential problem of omapdss behaving badly to the displays switched during suspend ?
On 28 June 2012 21:18, Jassi Brar <jaswinder.singh@linaro.org> wrote: > On 28 June 2012 20:57, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote: >>> > By the way, when the device is in system suspend, we surely won't detect >>> > the HPD even if we kept the HPD always enabled. So there we'll miss the >>> > HPD interrupt anyway, and the EDID cache would be invalid. >>> > >>> If omapdss already handles the possibility of display changed during >>> suspend, I think we should be good :) >> >> Hmm I'm not sure I understand what you mean. I was referring to your >> patch, which invalidated the EDID cache only on HPD interrupt when the >> cable is unplugged. And we'd miss that interrupt when the board is in >> system suspend, even if we otherwise kept the HPD interrupt always >> enabled. >> > I meant before stale-edid, we face potential problem of omapdss > behaving badly to the displays switched during suspend ? > OK, I think I get now what you mean. We do need to invalidate edid-cache in the suspend path, irrespective of how omapdss behaves. Thanks.
diff --git a/drivers/video/omap2/dss/hdmi.c b/drivers/video/omap2/dss/hdmi.c index 0738090..9853621 100644 --- a/drivers/video/omap2/dss/hdmi.c +++ b/drivers/video/omap2/dss/hdmi.c @@ -758,6 +758,7 @@ static int __init omapdss_hdmihw_probe(struct platform_device *pdev) hdmi.ip_data.core_av_offset = HDMI_CORE_AV; hdmi.ip_data.pll_offset = HDMI_PLLCTRL; hdmi.ip_data.phy_offset = HDMI_PHY; + hdmi.ip_data.edid_len = 0; /* Invalidate EDID Cache */ mutex_init(&hdmi.ip_data.lock); hdmi_panel_init(); diff --git a/drivers/video/omap2/dss/ti_hdmi.h b/drivers/video/omap2/dss/ti_hdmi.h index cc292b8..4735860 100644 --- a/drivers/video/omap2/dss/ti_hdmi.h +++ b/drivers/video/omap2/dss/ti_hdmi.h @@ -178,6 +178,8 @@ struct hdmi_ip_data { /* ti_hdmi_4xxx_ip private data. These should be in a separate struct */ int hpd_gpio; struct mutex lock; + u8 edid_cached[256]; + unsigned edid_len; }; int ti_hdmi_4xxx_phy_enable(struct hdmi_ip_data *ip_data); void ti_hdmi_4xxx_phy_disable(struct hdmi_ip_data *ip_data); diff --git a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c index 04acca9..b5c3dc4 100644 --- a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c +++ b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c @@ -243,10 +243,13 @@ static int hdmi_check_hpd_state(struct hdmi_ip_data *ip_data) hpd = gpio_get_value(ip_data->hpd_gpio); - if (hpd) + if (hpd) { r = hdmi_set_phy_pwr(ip_data, HDMI_PHYPWRCMD_TXON); - else + } else { + /* Invalidate EDID Cache */ + ip_data->edid_len = 0; r = hdmi_set_phy_pwr(ip_data, HDMI_PHYPWRCMD_LDOON); + } if (r) { DSSERR("Failed to %s PHY TX power\n", @@ -454,6 +457,11 @@ int ti_hdmi_4xxx_read_edid(struct hdmi_ip_data *ip_data, { int r, l; + if (ip_data->edid_len) { + memcpy(edid, ip_data->edid_cached, ip_data->edid_len); + return ip_data->edid_len; + } + if (len < 128) return -EINVAL; @@ -474,6 +482,9 @@ int ti_hdmi_4xxx_read_edid(struct hdmi_ip_data *ip_data, l += 128; } + ip_data->edid_len = l; + memcpy(ip_data->edid_cached, edid, l); + return l; }