| Message ID | 1344512989-4071-8-git-send-email-archit@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On Thu, 2012-08-09 at 17:19 +0530, Archit Taneja wrote: > Add function omapdss_hdmi_display_get_timing() which returns the timings > maintained by the HDMI interface driver in it's hdmi_config field. This > prevents the need for the panel driver to configure default timings in it's > probe. > > This function is just intended to be used once during the panel driver's probe. > It makes sense for those interfaces which can be configured to a default timing. I'm not sure about this patch. So I think the basic idea here is that HDMI will use VGA video mode if, for whatever reason, no better mode is found out. After this change, the panel driver doesn't seem to do that. Instead it uses whatever mode was used previously by the hdmi output driver. Is the only reason for this patch to clean up the hdmi_panel driver? We could have a dss helper function which returns the timings for VGA (well, just a public const variable would be enough), and the panel driver could use that to initialize the timings struct. Tomi
On Tuesday 14 August 2012 06:32 PM, Tomi Valkeinen wrote: > Hi, > > On Thu, 2012-08-09 at 17:19 +0530, Archit Taneja wrote: >> Add function omapdss_hdmi_display_get_timing() which returns the timings >> maintained by the HDMI interface driver in it's hdmi_config field. This >> prevents the need for the panel driver to configure default timings in it's >> probe. >> >> This function is just intended to be used once during the panel driver's probe. >> It makes sense for those interfaces which can be configured to a default timing. > > I'm not sure about this patch. So I think the basic idea here is that > HDMI will use VGA video mode if, for whatever reason, no better mode is > found out. > > After this change, the panel driver doesn't seem to do that. Instead it > uses whatever mode was used previously by the hdmi output driver. > > Is the only reason for this patch to clean up the hdmi_panel driver? We > could have a dss helper function which returns the timings for VGA > (well, just a public const variable would be enough), and the panel > driver could use that to initialize the timings struct. Yes, that's the only reason, basically, to sync the panel with the timings to which the hdmi output driver configured itself during it's probe. We don't use hdmi_get_timing anywhere apart from here. I thought it would be clean to retrieve the timings by taking whatever is stored in the hdmi output driver, but we could leave it to a hardcoded VGA as before. I have done the same for venc, let me know if you think these should be removed. Archit -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2012-08-14 at 18:45 +0530, Archit Taneja wrote: > On Tuesday 14 August 2012 06:32 PM, Tomi Valkeinen wrote: > > Hi, > > > > On Thu, 2012-08-09 at 17:19 +0530, Archit Taneja wrote: > >> Add function omapdss_hdmi_display_get_timing() which returns the timings > >> maintained by the HDMI interface driver in it's hdmi_config field. This > >> prevents the need for the panel driver to configure default timings in it's > >> probe. > >> > >> This function is just intended to be used once during the panel driver's probe. > >> It makes sense for those interfaces which can be configured to a default timing. > > > > I'm not sure about this patch. So I think the basic idea here is that > > HDMI will use VGA video mode if, for whatever reason, no better mode is > > found out. > > > > After this change, the panel driver doesn't seem to do that. Instead it > > uses whatever mode was used previously by the hdmi output driver. > > > > Is the only reason for this patch to clean up the hdmi_panel driver? We > > could have a dss helper function which returns the timings for VGA > > (well, just a public const variable would be enough), and the panel > > driver could use that to initialize the timings struct. > > Yes, that's the only reason, basically, to sync the panel with the > timings to which the hdmi output driver configured itself during it's > probe. We don't use hdmi_get_timing anywhere apart from here. Does the hdmi output driver even need to configure any defaults? Wouldn't it be ok to presume that the panel driver configures the timings? I don't think it's sensible to think about default values in the output driver generally (well, at least for things like timings). Although in HDMI's case VGA is quite sensible default, but that's not the case for any other output. So I think generally we should just trust the panel driver to tell the output driver what configuration should be used before the panel driver enables the output. That said, it doesn't hurt that the output drivers initialize their own datastructures to something relatively sane to avoid any BUG() or WARN() calls in the omapdss. Although we could also somehow track if the timings has been set, and return an error when enabling the display if timings hasn't been set. But that requires an extra flag, so perhaps it's simpler to have some initial values in the output driver also. > I thought it would be clean to retrieve the timings by taking whatever > is stored in the hdmi output driver, but we could leave it to a > hardcoded VGA as before. My main worry is that it's not easily clear what is going on if you look at the panel code. It looks that the panel just uses whatever is in the output driver. It's not clear that it is always VGA. What if the previous mode was something else? I think it's much clearer if the panel driver sets the timings explicitly before enabling the output. It doesn't have to be in panel's probe, although that's perhaps the easiest place for it. > I have done the same for venc, let me know if you think these should be > removed. Well, I agree that the initial timings code in hdmi and venc is a bit ugly. I don't think it's bad as such, just that the timings are standard ones, and instead of having all the timings numbers there, we should have a common place for them. Tomi
On Tuesday 14 August 2012 07:40 PM, Tomi Valkeinen wrote: > On Tue, 2012-08-14 at 18:45 +0530, Archit Taneja wrote: >> On Tuesday 14 August 2012 06:32 PM, Tomi Valkeinen wrote: >>> Hi, >>> >>> On Thu, 2012-08-09 at 17:19 +0530, Archit Taneja wrote: >>>> Add function omapdss_hdmi_display_get_timing() which returns the timings >>>> maintained by the HDMI interface driver in it's hdmi_config field. This >>>> prevents the need for the panel driver to configure default timings in it's >>>> probe. >>>> >>>> This function is just intended to be used once during the panel driver's probe. >>>> It makes sense for those interfaces which can be configured to a default timing. >>> >>> I'm not sure about this patch. So I think the basic idea here is that >>> HDMI will use VGA video mode if, for whatever reason, no better mode is >>> found out. >>> >>> After this change, the panel driver doesn't seem to do that. Instead it >>> uses whatever mode was used previously by the hdmi output driver. >>> >>> Is the only reason for this patch to clean up the hdmi_panel driver? We >>> could have a dss helper function which returns the timings for VGA >>> (well, just a public const variable would be enough), and the panel >>> driver could use that to initialize the timings struct. >> >> Yes, that's the only reason, basically, to sync the panel with the >> timings to which the hdmi output driver configured itself during it's >> probe. We don't use hdmi_get_timing anywhere apart from here. > > Does the hdmi output driver even need to configure any defaults? > Wouldn't it be ok to presume that the panel driver configures the > timings? > > I don't think it's sensible to think about default values in the output > driver generally (well, at least for things like timings). Although in > HDMI's case VGA is quite sensible default, but that's not the case for > any other output. > > So I think generally we should just trust the panel driver to tell the > output driver what configuration should be used before the panel driver > enables the output. > > That said, it doesn't hurt that the output drivers initialize their own > datastructures to something relatively sane to avoid any BUG() or WARN() > calls in the omapdss. Although we could also somehow track if the > timings has been set, and return an error when enabling the display if > timings hasn't been set. But that requires an extra flag, so perhaps > it's simpler to have some initial values in the output driver also. > >> I thought it would be clean to retrieve the timings by taking whatever >> is stored in the hdmi output driver, but we could leave it to a >> hardcoded VGA as before. > > My main worry is that it's not easily clear what is going on if you look > at the panel code. It looks that the panel just uses whatever is in the > output driver. It's not clear that it is always VGA. What if the > previous mode was something else? > > I think it's much clearer if the panel driver sets the timings > explicitly before enabling the output. It doesn't have to be in panel's > probe, although that's perhaps the easiest place for it. > >> I have done the same for venc, let me know if you think these should be >> removed. > > Well, I agree that the initial timings code in hdmi and venc is a bit > ugly. I don't think it's bad as such, just that the timings are standard > ones, and instead of having all the timings numbers there, we should > have a common place for them. Okay, I'll remove the get_timing ops, keep defaults in the panel driver's probe, have a const variable for the defaults to make it look less ugly, and make sure that there is a set_timings for the output in the hdmi and venc panel drivers before they are enabled, it sort of okay for hdmi and venc panel drivers as there is going to be only one of them and be in our control. Archit -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/video/omap2/dss/dss.h b/drivers/video/omap2/dss/dss.h index 7e38ffd..df49c5d 100644 --- a/drivers/video/omap2/dss/dss.h +++ b/drivers/video/omap2/dss/dss.h @@ -485,6 +485,8 @@ static inline unsigned long hdmi_get_pixel_clock(void) #endif int omapdss_hdmi_display_enable(struct omap_dss_device *dssdev); void omapdss_hdmi_display_disable(struct omap_dss_device *dssdev); +void omapdss_hdmi_display_get_timing(struct omap_dss_device *dssdev, + struct omap_video_timings *timings); void omapdss_hdmi_display_set_timing(struct omap_dss_device *dssdev, struct omap_video_timings *timings); int omapdss_hdmi_display_check_timing(struct omap_dss_device *dssdev, diff --git a/drivers/video/omap2/dss/hdmi.c b/drivers/video/omap2/dss/hdmi.c index bdf1c33..f577c8e 100644 --- a/drivers/video/omap2/dss/hdmi.c +++ b/drivers/video/omap2/dss/hdmi.c @@ -544,6 +544,12 @@ static void hdmi_power_off(struct omap_dss_device *dssdev) hdmi_runtime_put(); } +void omapdss_hdmi_display_get_timing(struct omap_dss_device *dssdev, + struct omap_video_timings *timings) +{ + *timings = hdmi.ip_data.cfg.timings; +} + int omapdss_hdmi_display_check_timing(struct omap_dss_device *dssdev, struct omap_video_timings *timings) { diff --git a/drivers/video/omap2/dss/hdmi_panel.c b/drivers/video/omap2/dss/hdmi_panel.c index 8dce206..8473193 100644 --- a/drivers/video/omap2/dss/hdmi_panel.c +++ b/drivers/video/omap2/dss/hdmi_panel.c @@ -41,13 +41,13 @@ static struct { static int hdmi_panel_probe(struct omap_dss_device *dssdev) { + struct omap_video_timings timings; + DSSDBG("ENTER hdmi_panel_probe\n"); - dssdev->panel.timings = (struct omap_video_timings) - { 640, 480, 25175, 96, 16, 48, 2, 11, 31, - OMAPDSS_SIG_ACTIVE_LOW, OMAPDSS_SIG_ACTIVE_LOW, - false, - }; + omapdss_hdmi_display_get_timing(dssdev, &timings); + + dssdev->panel.timings = timings; DSSDBG("hdmi_panel_probe x_res= %d y_res = %d\n", dssdev->panel.timings.x_res,
Add function omapdss_hdmi_display_get_timing() which returns the timings maintained by the HDMI interface driver in it's hdmi_config field. This prevents the need for the panel driver to configure default timings in it's probe. This function is just intended to be used once during the panel driver's probe. It makes sense for those interfaces which can be configured to a default timing. Signed-off-by: Archit Taneja <archit@ti.com> --- drivers/video/omap2/dss/dss.h | 2 ++ drivers/video/omap2/dss/hdmi.c | 6 ++++++ drivers/video/omap2/dss/hdmi_panel.c | 10 +++++----- 3 files changed, 13 insertions(+), 5 deletions(-)