| Message ID | 1345200551-28712-3-git-send-email-archit@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 2012-08-17 at 16:19 +0530, Archit Taneja wrote: > The functions dss_mgr_wait_for_go() and dss_mgr_wait_for_go_ovl() check if there > is an enabled display connected to the manager before trying to see the state of > the GO bit. > > The checks related to the display can be replaced by checking the state of the > manager, i.e, whether the manager is enabled or not. This makes more sense than > checking with the connected display as the GO bit behaviour is more connected > with the manager state rather than the display state. A GO bit can only be set > if the manager is enabled. If a manager isn't enabled, we can safely assume that > the GO bit is not set. > > Signed-off-by: Archit Taneja <archit@ti.com> > --- > drivers/video/omap2/dss/apply.c | 32 +++++++++++++++++++++----------- > 1 file changed, 21 insertions(+), 11 deletions(-) > > diff --git a/drivers/video/omap2/dss/apply.c b/drivers/video/omap2/dss/apply.c > index 52a5940..74f1a58 100644 > --- a/drivers/video/omap2/dss/apply.c > +++ b/drivers/video/omap2/dss/apply.c > @@ -424,17 +424,23 @@ static void wait_pending_extra_info_updates(void) > int dss_mgr_wait_for_go(struct omap_overlay_manager *mgr) > { > unsigned long timeout = msecs_to_jiffies(500); > - struct mgr_priv_data *mp; > + struct mgr_priv_data *mp = get_mgr_priv(mgr); > u32 irq; > + unsigned long flags; > int r; > int i; > - struct omap_dss_device *dssdev = mgr->device; > > - if (!dssdev || dssdev->state != OMAP_DSS_DISPLAY_ACTIVE) > + if (mgr_manual_update(mgr)) This needs to be inside the spinlock also. > return 0; > > - if (mgr_manual_update(mgr)) > + spin_lock_irqsave(&data_lock, flags); > + > + if (!mp->enabled) { > + spin_unlock_irqrestore(&data_lock, flags); > return 0; > + } > + > + spin_unlock_irqrestore(&data_lock, flags); > > r = dispc_runtime_get(); > if (r) > @@ -442,10 +448,8 @@ int dss_mgr_wait_for_go(struct omap_overlay_manager *mgr) > > irq = dispc_mgr_get_vsync_irq(mgr->id); > > - mp = get_mgr_priv(mgr); > i = 0; > while (1) { > - unsigned long flags; > bool shadow_dirty, dirty; > > spin_lock_irqsave(&data_lock, flags); > @@ -489,21 +493,28 @@ int dss_mgr_wait_for_go_ovl(struct omap_overlay *ovl) > { > unsigned long timeout = msecs_to_jiffies(500); > struct ovl_priv_data *op; > - struct omap_dss_device *dssdev; > + struct mgr_priv_data *mp; > u32 irq; > + unsigned long flags; > int r; > int i; > > if (!ovl->manager) > return 0; And this should be inside spinlock (yes, you didn't change that, but now that you're changing these... =) > - dssdev = ovl->manager->device; > + mp = get_mgr_priv(ovl->manager); > > - if (!dssdev || dssdev->state != OMAP_DSS_DISPLAY_ACTIVE) > + if (ovl_manual_update(ovl)) Inside spinlock here too. Actually, shouldn't the whole wait_for functions be locked with the apply mutex? Otherwise the output can be disabled/changed while waiting. On the other hand, that could be quite a long lock, and I don't see anything in the code that could really break if the output is disabled or similar. Perhaps it's fine to just hit the timeout in case something has been changed. If we add a mutex, we risk breaking something that currently works =). Tomi
diff --git a/drivers/video/omap2/dss/apply.c b/drivers/video/omap2/dss/apply.c index 52a5940..74f1a58 100644 --- a/drivers/video/omap2/dss/apply.c +++ b/drivers/video/omap2/dss/apply.c @@ -424,17 +424,23 @@ static void wait_pending_extra_info_updates(void) int dss_mgr_wait_for_go(struct omap_overlay_manager *mgr) { unsigned long timeout = msecs_to_jiffies(500); - struct mgr_priv_data *mp; + struct mgr_priv_data *mp = get_mgr_priv(mgr); u32 irq; + unsigned long flags; int r; int i; - struct omap_dss_device *dssdev = mgr->device; - if (!dssdev || dssdev->state != OMAP_DSS_DISPLAY_ACTIVE) + if (mgr_manual_update(mgr)) return 0; - if (mgr_manual_update(mgr)) + spin_lock_irqsave(&data_lock, flags); + + if (!mp->enabled) { + spin_unlock_irqrestore(&data_lock, flags); return 0; + } + + spin_unlock_irqrestore(&data_lock, flags); r = dispc_runtime_get(); if (r) @@ -442,10 +448,8 @@ int dss_mgr_wait_for_go(struct omap_overlay_manager *mgr) irq = dispc_mgr_get_vsync_irq(mgr->id); - mp = get_mgr_priv(mgr); i = 0; while (1) { - unsigned long flags; bool shadow_dirty, dirty; spin_lock_irqsave(&data_lock, flags); @@ -489,21 +493,28 @@ int dss_mgr_wait_for_go_ovl(struct omap_overlay *ovl) { unsigned long timeout = msecs_to_jiffies(500); struct ovl_priv_data *op; - struct omap_dss_device *dssdev; + struct mgr_priv_data *mp; u32 irq; + unsigned long flags; int r; int i; if (!ovl->manager) return 0; - dssdev = ovl->manager->device; + mp = get_mgr_priv(ovl->manager); - if (!dssdev || dssdev->state != OMAP_DSS_DISPLAY_ACTIVE) + if (ovl_manual_update(ovl)) return 0; - if (ovl_manual_update(ovl)) + spin_lock_irqsave(&data_lock, flags); + + if (!mp->enabled) { + spin_unlock_irqrestore(&data_lock, flags); return 0; + } + + spin_unlock_irqrestore(&data_lock, flags); r = dispc_runtime_get(); if (r) @@ -514,7 +525,6 @@ int dss_mgr_wait_for_go_ovl(struct omap_overlay *ovl) op = get_ovl_priv(ovl); i = 0; while (1) { - unsigned long flags; bool shadow_dirty, dirty; spin_lock_irqsave(&data_lock, flags);
The functions dss_mgr_wait_for_go() and dss_mgr_wait_for_go_ovl() check if there is an enabled display connected to the manager before trying to see the state of the GO bit. The checks related to the display can be replaced by checking the state of the manager, i.e, whether the manager is enabled or not. This makes more sense than checking with the connected display as the GO bit behaviour is more connected with the manager state rather than the display state. A GO bit can only be set if the manager is enabled. If a manager isn't enabled, we can safely assume that the GO bit is not set. Signed-off-by: Archit Taneja <archit@ti.com> --- drivers/video/omap2/dss/apply.c | 32 +++++++++++++++++++++----------- 1 file changed, 21 insertions(+), 11 deletions(-)