* [PATCH 0/2] OMAPDSS: DISPC: Improvements to DIGIT sync signal selection @ 2012-07-28 0:21 Ricardo Neri 2012-07-28 0:21 ` [PATCH 1/2] OMAPDSS: DISPC: Improve logic of selection of external sync signal Ricardo Neri ` (2 more replies) 0 siblings, 3 replies; 6+ messages in thread From: Ricardo Neri @ 2012-07-28 0:21 UTC (permalink / raw) To: tomi.valkeinen; +Cc: archit, s-guiriec, linux-omap, Ricardo Neri Hello, DSS code wrongly assumes that VENC is always available as source for the external sync signal for the display controller DIGIT channel. One cannot blindly rely only on the value of DSS_CONTROL register as certain processors may not have VENC (e.g., OMAP5). These patches add logic to correctly set/get the sync signal based on the available displays in the DIGIT channel. For the HDMI driver, handle the error in the selection of the source. BR, Ricardo Ricardo Neri (2): OMAPDSS: DISPC: Improve logic of selection of external sync signal OMAPDSS:HDMI: Improve error handling at power on drivers/video/omap2/dss/dss.c | 19 +++++++++++++++++-- drivers/video/omap2/dss/dss.h | 2 +- drivers/video/omap2/dss/hdmi.c | 10 ++++++++-- 3 files changed, 26 insertions(+), 5 deletions(-) -- 1.7.5.4 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] OMAPDSS: DISPC: Improve logic of selection of external sync signal 2012-07-28 0:21 [PATCH 0/2] OMAPDSS: DISPC: Improvements to DIGIT sync signal selection Ricardo Neri @ 2012-07-28 0:21 ` Ricardo Neri 2012-07-28 0:21 ` [PATCH 2/2] OMAPDSS:HDMI: Improve error handling at power on Ricardo Neri 2012-07-30 11:11 ` [PATCH 0/2] OMAPDSS: DISPC: Improvements to DIGIT sync signal selection Tomi Valkeinen 2 siblings, 0 replies; 6+ messages in thread From: Ricardo Neri @ 2012-07-28 0:21 UTC (permalink / raw) To: tomi.valkeinen; +Cc: archit, s-guiriec, linux-omap, Ricardo Neri The DIGIT channel of the display controller can take the external sync signal from VENC or HDMI. The code was wrongly assuming that VENC was always available, which is not necessarily true (e.g., OMAP5). Now, first verify if the requested external source sync signal is available before getting/setting the source. Signed-off-by: Ricardo Neri <ricardo.neri@ti.com> --- drivers/video/omap2/dss/dss.c | 19 +++++++++++++++++-- drivers/video/omap2/dss/dss.h | 2 +- 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/drivers/video/omap2/dss/dss.c b/drivers/video/omap2/dss/dss.c index 04b4586..e399b4f 100644 --- a/drivers/video/omap2/dss/dss.c +++ b/drivers/video/omap2/dss/dss.c @@ -648,9 +648,21 @@ void dss_set_dac_pwrdn_bgz(bool enable) REG_FLD_MOD(DSS_CONTROL, enable, 5, 5); /* DAC Power-Down Control */ } -void dss_select_hdmi_venc_clk_source(enum dss_hdmi_venc_clk_source_select hdmi) +int dss_select_hdmi_venc_clk_source(enum dss_hdmi_venc_clk_source_select src) { - REG_FLD_MOD(DSS_CONTROL, hdmi, 15, 15); /* VENC_HDMI_SWITCH */ + enum omap_display_type displays; + + displays = dss_feat_get_supported_displays(OMAP_DSS_CHANNEL_DIGIT); + + /* discard invalid cases */ + if (src == DSS_VENC_TV_CLK && !(displays & OMAP_DISPLAY_TYPE_VENC)) + return -EINVAL; + + if (src == DSS_HDMI_M_PCLK && !(displays & OMAP_DISPLAY_TYPE_HDMI)) + return -EINVAL; + + REG_FLD_MOD(DSS_CONTROL, src, 15, 15); /* VENC_HDMI_SWITCH */ + return 0; } enum dss_hdmi_venc_clk_source_select dss_get_hdmi_venc_clk_source(void) @@ -661,6 +673,9 @@ enum dss_hdmi_venc_clk_source_select dss_get_hdmi_venc_clk_source(void) if ((displays & OMAP_DISPLAY_TYPE_HDMI) == 0) return DSS_VENC_TV_CLK; + if ((displays & OMAP_DISPLAY_TYPE_VENC) == 0) + return DSS_HDMI_M_PCLK; + return REG_GET(DSS_CONTROL, 15, 15); } diff --git a/drivers/video/omap2/dss/dss.h b/drivers/video/omap2/dss/dss.h index f67afe7..7ae763c 100644 --- a/drivers/video/omap2/dss/dss.h +++ b/drivers/video/omap2/dss/dss.h @@ -270,7 +270,7 @@ bool dss_ovl_use_replication(struct dss_lcd_mgr_config config, int dss_init_platform_driver(void) __init; void dss_uninit_platform_driver(void); -void dss_select_hdmi_venc_clk_source(enum dss_hdmi_venc_clk_source_select); +int dss_select_hdmi_venc_clk_source(enum dss_hdmi_venc_clk_source_select); enum dss_hdmi_venc_clk_source_select dss_get_hdmi_venc_clk_source(void); const char *dss_get_generic_clk_source_name(enum omap_dss_clk_source clk_src); void dss_dump_clocks(struct seq_file *s); -- 1.7.5.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] OMAPDSS:HDMI: Improve error handling at power on 2012-07-28 0:21 [PATCH 0/2] OMAPDSS: DISPC: Improvements to DIGIT sync signal selection Ricardo Neri 2012-07-28 0:21 ` [PATCH 1/2] OMAPDSS: DISPC: Improve logic of selection of external sync signal Ricardo Neri @ 2012-07-28 0:21 ` Ricardo Neri 2012-07-30 11:11 ` [PATCH 0/2] OMAPDSS: DISPC: Improvements to DIGIT sync signal selection Tomi Valkeinen 2 siblings, 0 replies; 6+ messages in thread From: Ricardo Neri @ 2012-07-28 0:21 UTC (permalink / raw) To: tomi.valkeinen; +Cc: archit, s-guiriec, linux-omap, Ricardo Neri Handle the errors that may occur when selecting the source of the sync signal for the DIGIT channel of the display controller and when enabling the PHY. Signed-off-by: Ricardo Neri <ricardo.neri@ti.com> --- drivers/video/omap2/dss/hdmi.c | 10 ++++++++-- 1 files changed, 8 insertions(+), 2 deletions(-) diff --git a/drivers/video/omap2/dss/hdmi.c b/drivers/video/omap2/dss/hdmi.c index 060216f..3caac2a 100644 --- a/drivers/video/omap2/dss/hdmi.c +++ b/drivers/video/omap2/dss/hdmi.c @@ -501,13 +501,17 @@ static int hdmi_power_on(struct omap_dss_device *dssdev) r = hdmi.ip_data.ops->phy_enable(&hdmi.ip_data); if (r) { DSSDBG("Failed to start PHY\n"); - goto err; + goto err_phy_enable; } hdmi.ip_data.ops->video_configure(&hdmi.ip_data); /* Make selection of HDMI in DSS */ - dss_select_hdmi_venc_clk_source(DSS_HDMI_M_PCLK); + r = dss_select_hdmi_venc_clk_source(DSS_HDMI_M_PCLK); + if (r) { + DSSDBG("Failed to start PHY\n"); + goto err_src_hdmi_venc; + } /* Select the dispc clock source as PRCM clock, to ensure that it is not * DSI PLL source as the clock selected by DSI PLL might not be @@ -536,7 +540,9 @@ static int hdmi_power_on(struct omap_dss_device *dssdev) err_mgr_enable: hdmi.ip_data.ops->video_disable(&hdmi.ip_data); err_vid_enable: +err_src_hdmi_venc: hdmi.ip_data.ops->phy_disable(&hdmi.ip_data); +err_phy_enable: hdmi.ip_data.ops->pll_disable(&hdmi.ip_data); err: hdmi_runtime_put(); -- 1.7.5.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 0/2] OMAPDSS: DISPC: Improvements to DIGIT sync signal selection 2012-07-28 0:21 [PATCH 0/2] OMAPDSS: DISPC: Improvements to DIGIT sync signal selection Ricardo Neri 2012-07-28 0:21 ` [PATCH 1/2] OMAPDSS: DISPC: Improve logic of selection of external sync signal Ricardo Neri 2012-07-28 0:21 ` [PATCH 2/2] OMAPDSS:HDMI: Improve error handling at power on Ricardo Neri @ 2012-07-30 11:11 ` Tomi Valkeinen 2012-07-30 11:48 ` Archit Taneja 2 siblings, 1 reply; 6+ messages in thread From: Tomi Valkeinen @ 2012-07-30 11:11 UTC (permalink / raw) To: Ricardo Neri; +Cc: archit, s-guiriec, linux-omap [-- Attachment #1: Type: text/plain, Size: 1510 bytes --] On Fri, 2012-07-27 at 19:21 -0500, Ricardo Neri wrote: > Hello, > > DSS code wrongly assumes that VENC is always available as source for the external > sync signal for the display controller DIGIT channel. One cannot blindly > rely only on the value of DSS_CONTROL register as certain processors may not > have VENC (e.g., OMAP5). > > These patches add logic to correctly set/get the sync signal based on the > available displays in the DIGIT channel. > > For the HDMI driver, handle the error in the selection of the source. Is there an actual problem seen that this resolves? If so, It'd be nice to have a brief description of the problem. Does the bit 15 in DSS_CONTROL still exist on OMAP5? What does it return on OMAP5? I think it's clear that dss_get_hdmi_venc_clk_source() needs to return a correct value so that DSS operates correctly. But I'm not sure we need the extra code in dss_select_hdmi_venc_clk_source(). dss_select_hdmi_venc_clk_source() should be called by venc or hdmi, and there should be any possibility of using it in the wrong way. For extra sanity checking there could be a BUG_ON() check in the dss_select_hdmi_venc_clk_source() to point out if there's a bad bug in venc or hdmi. I'd like to keep the lowest level dss-internal functions as simple as possible, and written in the manner that they presume they are called correctly. Otherwise we'll end up with lots of new code, checking for errors that can never happen in practice. Tomi [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 0/2] OMAPDSS: DISPC: Improvements to DIGIT sync signal selection 2012-07-30 11:11 ` [PATCH 0/2] OMAPDSS: DISPC: Improvements to DIGIT sync signal selection Tomi Valkeinen @ 2012-07-30 11:48 ` Archit Taneja 2012-07-30 19:21 ` Ricardo Neri 0 siblings, 1 reply; 6+ messages in thread From: Archit Taneja @ 2012-07-30 11:48 UTC (permalink / raw) To: Tomi Valkeinen; +Cc: Ricardo Neri, s-guiriec, linux-omap On Monday 30 July 2012 04:41 PM, Tomi Valkeinen wrote: > On Fri, 2012-07-27 at 19:21 -0500, Ricardo Neri wrote: >> Hello, >> >> DSS code wrongly assumes that VENC is always available as source for the external >> sync signal for the display controller DIGIT channel. One cannot blindly >> rely only on the value of DSS_CONTROL register as certain processors may not >> have VENC (e.g., OMAP5). >> >> These patches add logic to correctly set/get the sync signal based on the >> available displays in the DIGIT channel. >> >> For the HDMI driver, handle the error in the selection of the source. > > Is there an actual problem seen that this resolves? If so, It'd be nice > to have a brief description of the problem. The issue is seen on OMAP5 with a 3.5 kernel. In 3.5 kernel, we had a hack where we used to divide the TV manager's height by 2 if the connected interface was VENC. On OMAP5, the bit 15 returns VENC (a zero) irrespective of what we write. This won't happen in 3.6 as we have included interlace as a part of the omap_video_timings struct, and we don't try to check the bit to see whether we are VENC or HDMI. I guess we should still put a check to not set the bit on OMAP5 as it may cause unknown HW behavior, i.e, make the select function like: void dss_select_hdmi_venc_clk_source() { ... displays = dss_feat_get_supported_displays(OMAP_DSS_CHANNEL_DIGIT); if ((displays & OMAP_DISPLAY_TYPE_VENC) == 0) return; REG_FLD_MOD(DSS_CONTROL, src, 15, 15); /* VENC_HDMI_SWITCH */ } Archit > > Does the bit 15 in DSS_CONTROL still exist on OMAP5? What does it return > on OMAP5? > > I think it's clear that dss_get_hdmi_venc_clk_source() needs to return a > correct value so that DSS operates correctly. But I'm not sure we need > the extra code in dss_select_hdmi_venc_clk_source(). > > dss_select_hdmi_venc_clk_source() should be called by venc or hdmi, and > there should be any possibility of using it in the wrong way. For extra > sanity checking there could be a BUG_ON() check in the > dss_select_hdmi_venc_clk_source() to point out if there's a bad bug in > venc or hdmi. > > I'd like to keep the lowest level dss-internal functions as simple as > possible, and written in the manner that they presume they are called > correctly. Otherwise we'll end up with lots of new code, checking for > errors that can never happen in practice. > > Tomi > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 0/2] OMAPDSS: DISPC: Improvements to DIGIT sync signal selection 2012-07-30 11:48 ` Archit Taneja @ 2012-07-30 19:21 ` Ricardo Neri 0 siblings, 0 replies; 6+ messages in thread From: Ricardo Neri @ 2012-07-30 19:21 UTC (permalink / raw) To: Archit Taneja; +Cc: Tomi Valkeinen, s-guiriec, linux-omap Hi Tomi, Archit, Thanks for your feedback. On 07/30/2012 06:48 AM, Archit Taneja wrote: > On Monday 30 July 2012 04:41 PM, Tomi Valkeinen wrote: >> On Fri, 2012-07-27 at 19:21 -0500, Ricardo Neri wrote: >>> Hello, >>> >>> DSS code wrongly assumes that VENC is always available as source for >>> the external >>> sync signal for the display controller DIGIT channel. One cannot blindly >>> rely only on the value of DSS_CONTROL register as certain processors >>> may not >>> have VENC (e.g., OMAP5). >>> >>> These patches add logic to correctly set/get the sync signal based on >>> the >>> available displays in the DIGIT channel. >>> >>> For the HDMI driver, handle the error in the selection of the source. >> >> Is there an actual problem seen that this resolves? If so, It'd be nice >> to have a brief description of the problem. > > The issue is seen on OMAP5 with a 3.5 kernel. In 3.5 kernel, we had a > hack where we used to divide the TV manager's height by 2 if the > connected interface was VENC. On OMAP5, the bit 15 returns VENC (a zero) > irrespective of what we write. > > This won't happen in 3.6 as we have included interlace as a part of the > omap_video_timings struct, and we don't try to check the bit to see > whether we are VENC or HDMI. > > I guess we should still put a check to not set the bit on OMAP5 as it > may cause unknown HW behavior, i.e, make the select function like: > > void dss_select_hdmi_venc_clk_source() > { > ... > > displays = > dss_feat_get_supported_displays(OMAP_DSS_CHANNEL_DIGIT); > > if ((displays & OMAP_DISPLAY_TYPE_VENC) == 0) > return; > > REG_FLD_MOD(DSS_CONTROL, src, 15, 15); /* VENC_HDMI_SWITCH */ > } As Tomi mentions, perhaps this could be complemented with a BUG_ON could be used if someone tries to use VENC as source when this is not available. > > Archit > >> >> Does the bit 15 in DSS_CONTROL still exist on OMAP5? What does it return >> on OMAP5? >> >> I think it's clear that dss_get_hdmi_venc_clk_source() needs to return a >> correct value so that DSS operates correctly. But I'm not sure we need >> the extra code in dss_select_hdmi_venc_clk_source(). Yes, the main purpose of these patch was to avoid blindly reading DSS_CONTROL[15]. The additions to selection were just for completeness. >> >> dss_select_hdmi_venc_clk_source() should be called by venc or hdmi, and >> there should be any possibility of using it in the wrong way. For extra >> sanity checking there could be a BUG_ON() check in the >> dss_select_hdmi_venc_clk_source() to point out if there's a bad bug in >> venc or hdmi. >> >> I'd like to keep the lowest level dss-internal functions as simple as >> possible, and written in the manner that they presume they are called >> correctly. Otherwise we'll end up with lots of new code, checking for >> errors that can never happen in practice. Well, this was an error that happened in practice :). I will resubmit based on these comments. Thanks, Ricardo >> >> Tomi >> > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-07-30 19:22 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-07-28 0:21 [PATCH 0/2] OMAPDSS: DISPC: Improvements to DIGIT sync signal selection Ricardo Neri 2012-07-28 0:21 ` [PATCH 1/2] OMAPDSS: DISPC: Improve logic of selection of external sync signal Ricardo Neri 2012-07-28 0:21 ` [PATCH 2/2] OMAPDSS:HDMI: Improve error handling at power on Ricardo Neri 2012-07-30 11:11 ` [PATCH 0/2] OMAPDSS: DISPC: Improvements to DIGIT sync signal selection Tomi Valkeinen 2012-07-30 11:48 ` Archit Taneja 2012-07-30 19:21 ` Ricardo Neri
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.