| Message ID | 1346833555-31258-16-git-send-email-tomi.valkeinen@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wednesday 05 September 2012 01:55 PM, Tomi Valkeinen wrote: > Now that fifo merge has been removed, nobody uses the extra_info related > completion code, which can be removed. I think this might come into use when we fix the usage of channel out field. That is, since channel out is an immediate write field, when we set a new manager for an overlay, we may need to wait till the overlay disable kicks in, only then we should change channel out. For this, we would need some wait for extra_info, right? Archit > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> > --- > drivers/video/omap2/dss/apply.c | 69 --------------------------------------- > 1 file changed, 69 deletions(-) > > diff --git a/drivers/video/omap2/dss/apply.c b/drivers/video/omap2/dss/apply.c > index 32b5379..2579d15 100644 > --- a/drivers/video/omap2/dss/apply.c > +++ b/drivers/video/omap2/dss/apply.c > @@ -118,7 +118,6 @@ static struct { > static spinlock_t data_lock; > /* lock for blocking functions */ > static DEFINE_MUTEX(apply_lock); > -static DECLARE_COMPLETION(extra_updated_completion); > > static void dss_register_vsync_isr(void); > > @@ -355,69 +354,6 @@ static bool need_go(struct omap_overlay_manager *mgr) > return false; > } > > -/* returns true if an extra_info field is currently being updated */ > -static bool extra_info_update_ongoing(void) > -{ > - const int num_mgrs = dss_feat_get_num_mgrs(); > - int i; > - > - for (i = 0; i < num_mgrs; ++i) { > - struct omap_overlay_manager *mgr; > - struct omap_overlay *ovl; > - struct mgr_priv_data *mp; > - > - mgr = omap_dss_get_overlay_manager(i); > - mp = get_mgr_priv(mgr); > - > - if (!mp->enabled) > - continue; > - > - if (!mp->updating) > - continue; > - > - if (mp->extra_info_dirty || mp->shadow_extra_info_dirty) > - return true; > - > - list_for_each_entry(ovl, &mgr->overlays, list) { > - struct ovl_priv_data *op = get_ovl_priv(ovl); > - > - if (op->extra_info_dirty || op->shadow_extra_info_dirty) > - return true; > - } > - } > - > - return false; > -} > - > -/* wait until no extra_info updates are pending */ > -static void wait_pending_extra_info_updates(void) > -{ > - bool updating; > - unsigned long flags; > - unsigned long t; > - int r; > - > - spin_lock_irqsave(&data_lock, flags); > - > - updating = extra_info_update_ongoing(); > - > - if (!updating) { > - spin_unlock_irqrestore(&data_lock, flags); > - return; > - } > - > - init_completion(&extra_updated_completion); > - > - spin_unlock_irqrestore(&data_lock, flags); > - > - t = msecs_to_jiffies(500); > - r = wait_for_completion_timeout(&extra_updated_completion, t); > - if (r == 0) > - DSSWARN("timeout in wait_pending_extra_info_updates\n"); > - else if (r < 0) > - DSSERR("wait_pending_extra_info_updates failed: %d\n", r); > -} > - > int dss_mgr_wait_for_go(struct omap_overlay_manager *mgr) > { > unsigned long timeout = msecs_to_jiffies(500); > @@ -823,7 +759,6 @@ static void dss_apply_irq_handler(void *data, u32 mask) > { > const int num_mgrs = dss_feat_get_num_mgrs(); > int i; > - bool extra_updating; > > spin_lock(&data_lock); > > @@ -854,10 +789,6 @@ static void dss_apply_irq_handler(void *data, u32 mask) > dss_write_regs(); > dss_set_go_bits(); > > - extra_updating = extra_info_update_ongoing(); > - if (!extra_updating) > - complete_all(&extra_updated_completion); > - > if (!need_isr()) > dss_unregister_vsync_isr(); > > -- 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 Wed, 2012-09-05 at 19:01 +0530, Archit Taneja wrote: > On Wednesday 05 September 2012 01:55 PM, Tomi Valkeinen wrote: > > Now that fifo merge has been removed, nobody uses the extra_info related > > completion code, which can be removed. > > I think this might come into use when we fix the usage of channel out > field. That is, since channel out is an immediate write field, when we > set a new manager for an overlay, we may need to wait till the overlay > disable kicks in, only then we should change channel out. > > For this, we would need some wait for extra_info, right? Hmm, yes, I think you are right. Previously the ovl_disable waited until the overlay is not used anymore, but now it doesn't. So I think I need to add wait_pending_extra_info_updates() call to the beginning of dss_ovl_set_manager(). Or, should it be in unset_manager... I think unset is better, then a "free" overlay always disabled also in the HW level. Tomi
On Thursday 06 September 2012 06:34 PM, Tomi Valkeinen wrote: > On Wed, 2012-09-05 at 19:01 +0530, Archit Taneja wrote: >> On Wednesday 05 September 2012 01:55 PM, Tomi Valkeinen wrote: >>> Now that fifo merge has been removed, nobody uses the extra_info related >>> completion code, which can be removed. >> >> I think this might come into use when we fix the usage of channel out >> field. That is, since channel out is an immediate write field, when we >> set a new manager for an overlay, we may need to wait till the overlay >> disable kicks in, only then we should change channel out. >> >> For this, we would need some wait for extra_info, right? > > Hmm, yes, I think you are right. Previously the ovl_disable waited until > the overlay is not used anymore, but now it doesn't. > > So I think I need to add wait_pending_extra_info_updates() call to the > beginning of dss_ovl_set_manager(). Or, should it be in unset_manager... > I think unset is better, then a "free" overlay always disabled also in > the HW level. Yes, I also think it should be in unset_manager. One option could be to leave the wait_pending_extra_info_updates() in ovl_disable itself, as it was before. But that would force us to use mutexes there, and we'd rather have overlay enabling and disabling as a non blocking thing. 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 Thu, 2012-09-06 at 19:05 +0530, Archit Taneja wrote: > On Thursday 06 September 2012 06:34 PM, Tomi Valkeinen wrote: > > On Wed, 2012-09-05 at 19:01 +0530, Archit Taneja wrote: > >> On Wednesday 05 September 2012 01:55 PM, Tomi Valkeinen wrote: > >>> Now that fifo merge has been removed, nobody uses the extra_info related > >>> completion code, which can be removed. > >> > >> I think this might come into use when we fix the usage of channel out > >> field. That is, since channel out is an immediate write field, when we > >> set a new manager for an overlay, we may need to wait till the overlay > >> disable kicks in, only then we should change channel out. > >> > >> For this, we would need some wait for extra_info, right? > > > > Hmm, yes, I think you are right. Previously the ovl_disable waited until > > the overlay is not used anymore, but now it doesn't. > > > > So I think I need to add wait_pending_extra_info_updates() call to the > > beginning of dss_ovl_set_manager(). Or, should it be in unset_manager... > > I think unset is better, then a "free" overlay always disabled also in > > the HW level. > > Yes, I also think it should be in unset_manager. One option could be to > leave the wait_pending_extra_info_updates() in ovl_disable itself, as it > was before. But that would force us to use mutexes there, and we'd > rather have overlay enabling and disabling as a non blocking thing. Actually, we do have mutexes there. You are thinking about the prototype API I have. (I also thought we didn't have mutex there =). So, in fact, we can have the wait at ovl_disable like it was before. The prototype API, which cannot block, will not have the wait, but there the caller (i.e. omapdrm) will have to manage the proper wait. Tomi
On Thursday 06 September 2012 07:12 PM, Tomi Valkeinen wrote: > On Thu, 2012-09-06 at 19:05 +0530, Archit Taneja wrote: >> On Thursday 06 September 2012 06:34 PM, Tomi Valkeinen wrote: >>> On Wed, 2012-09-05 at 19:01 +0530, Archit Taneja wrote: >>>> On Wednesday 05 September 2012 01:55 PM, Tomi Valkeinen wrote: >>>>> Now that fifo merge has been removed, nobody uses the extra_info related >>>>> completion code, which can be removed. >>>> >>>> I think this might come into use when we fix the usage of channel out >>>> field. That is, since channel out is an immediate write field, when we >>>> set a new manager for an overlay, we may need to wait till the overlay >>>> disable kicks in, only then we should change channel out. >>>> >>>> For this, we would need some wait for extra_info, right? >>> >>> Hmm, yes, I think you are right. Previously the ovl_disable waited until >>> the overlay is not used anymore, but now it doesn't. >>> >>> So I think I need to add wait_pending_extra_info_updates() call to the >>> beginning of dss_ovl_set_manager(). Or, should it be in unset_manager... >>> I think unset is better, then a "free" overlay always disabled also in >>> the HW level. >> >> Yes, I also think it should be in unset_manager. One option could be to >> leave the wait_pending_extra_info_updates() in ovl_disable itself, as it >> was before. But that would force us to use mutexes there, and we'd >> rather have overlay enabling and disabling as a non blocking thing. > > Actually, we do have mutexes there. You are thinking about the prototype > API I have. (I also thought we didn't have mutex there =). Ah, I missed looking at that :) > > So, in fact, we can have the wait at ovl_disable like it was before. The > prototype API, which cannot block, will not have the wait, but there the > caller (i.e. omapdrm) will have to manage the proper wait. I'm more inclined towards waiting in the unset_manager() now, we have a choice between "wait in ovl_disable, ensure the overlay is actually disabled in hw, and then get out" and "wait only when you know you need to wait (i.e, in unset_manager)". The second choice seems more efficient. This wait would could last for a 1 VSYNC if we do it in ovl_disable. If the next task of the user of DSS is to enable another overlay, this wait would unnecessarily delay the enabling of the second overlay by a VSYNC. We could have done these tasks in the same VSYNC (since we aren't supporting fifomerge). So, I feel that we should rather wait in unset_manager, where we know an immediate write can mess things up. Maybe, we could delay it set_manager too. But yeah, we won't know whether we are aligned with hw or not. So even with the prototype API, where omapdrm is responsible for doing the waiting, it should probably wait when switching the manager, rather than when disabling the overlay. 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 Thu, 2012-09-06 at 19:31 +0530, Archit Taneja wrote: > On Thursday 06 September 2012 07:12 PM, Tomi Valkeinen wrote: > > On Thu, 2012-09-06 at 19:05 +0530, Archit Taneja wrote: > >> On Thursday 06 September 2012 06:34 PM, Tomi Valkeinen wrote: > >>> On Wed, 2012-09-05 at 19:01 +0530, Archit Taneja wrote: > >>>> On Wednesday 05 September 2012 01:55 PM, Tomi Valkeinen wrote: > >>>>> Now that fifo merge has been removed, nobody uses the extra_info related > >>>>> completion code, which can be removed. > >>>> > >>>> I think this might come into use when we fix the usage of channel out > >>>> field. That is, since channel out is an immediate write field, when we > >>>> set a new manager for an overlay, we may need to wait till the overlay > >>>> disable kicks in, only then we should change channel out. > >>>> > >>>> For this, we would need some wait for extra_info, right? > >>> > >>> Hmm, yes, I think you are right. Previously the ovl_disable waited until > >>> the overlay is not used anymore, but now it doesn't. > >>> > >>> So I think I need to add wait_pending_extra_info_updates() call to the > >>> beginning of dss_ovl_set_manager(). Or, should it be in unset_manager... > >>> I think unset is better, then a "free" overlay always disabled also in > >>> the HW level. > >> > >> Yes, I also think it should be in unset_manager. One option could be to > >> leave the wait_pending_extra_info_updates() in ovl_disable itself, as it > >> was before. But that would force us to use mutexes there, and we'd > >> rather have overlay enabling and disabling as a non blocking thing. > > > > Actually, we do have mutexes there. You are thinking about the prototype > > API I have. (I also thought we didn't have mutex there =). > > Ah, I missed looking at that :) > > > > > So, in fact, we can have the wait at ovl_disable like it was before. The > > prototype API, which cannot block, will not have the wait, but there the > > caller (i.e. omapdrm) will have to manage the proper wait. > > I'm more inclined towards waiting in the unset_manager() now, we have a > choice between "wait in ovl_disable, ensure the overlay is actually > disabled in hw, and then get out" and "wait only when you know you need > to wait (i.e, in unset_manager)". The second choice seems more efficient. > > This wait would could last for a 1 VSYNC if we do it in ovl_disable. If > the next task of the user of DSS is to enable another overlay, this wait > would unnecessarily delay the enabling of the second overlay by a VSYNC. > We could have done these tasks in the same VSYNC (since we aren't > supporting fifomerge). > > So, I feel that we should rather wait in unset_manager, where we know an > immediate write can mess things up. Maybe, we could delay it set_manager > too. But yeah, we won't know whether we are aligned with hw or not. Good points. Then again, the wait function doesn't wait for the ovl to be disabled, it waits for all extra_info changes to be done. So we could be waiting unnecessarily in unset/set_manager. Which makes me think that we should remove the current wait function and implement a specialized wait for ovl disable. But that can be looked at later if seen necessary. Also, it feels safer to ensure the ovl is disabled at ovl_disable call. However, I was going through the code and I didn't come up with a case where it would cause problems, except the set_manager part. So, I guess having the wait in unset_manager is still best overall, we don't unnecessarily spend time waiting at ovl_disable. > So even with the prototype API, where omapdrm is responsible for doing > the waiting, it should probably wait when switching the manager, rather > than when disabling the overlay. Yep. Tomi
diff --git a/drivers/video/omap2/dss/apply.c b/drivers/video/omap2/dss/apply.c index 32b5379..2579d15 100644 --- a/drivers/video/omap2/dss/apply.c +++ b/drivers/video/omap2/dss/apply.c @@ -118,7 +118,6 @@ static struct { static spinlock_t data_lock; /* lock for blocking functions */ static DEFINE_MUTEX(apply_lock); -static DECLARE_COMPLETION(extra_updated_completion); static void dss_register_vsync_isr(void); @@ -355,69 +354,6 @@ static bool need_go(struct omap_overlay_manager *mgr) return false; } -/* returns true if an extra_info field is currently being updated */ -static bool extra_info_update_ongoing(void) -{ - const int num_mgrs = dss_feat_get_num_mgrs(); - int i; - - for (i = 0; i < num_mgrs; ++i) { - struct omap_overlay_manager *mgr; - struct omap_overlay *ovl; - struct mgr_priv_data *mp; - - mgr = omap_dss_get_overlay_manager(i); - mp = get_mgr_priv(mgr); - - if (!mp->enabled) - continue; - - if (!mp->updating) - continue; - - if (mp->extra_info_dirty || mp->shadow_extra_info_dirty) - return true; - - list_for_each_entry(ovl, &mgr->overlays, list) { - struct ovl_priv_data *op = get_ovl_priv(ovl); - - if (op->extra_info_dirty || op->shadow_extra_info_dirty) - return true; - } - } - - return false; -} - -/* wait until no extra_info updates are pending */ -static void wait_pending_extra_info_updates(void) -{ - bool updating; - unsigned long flags; - unsigned long t; - int r; - - spin_lock_irqsave(&data_lock, flags); - - updating = extra_info_update_ongoing(); - - if (!updating) { - spin_unlock_irqrestore(&data_lock, flags); - return; - } - - init_completion(&extra_updated_completion); - - spin_unlock_irqrestore(&data_lock, flags); - - t = msecs_to_jiffies(500); - r = wait_for_completion_timeout(&extra_updated_completion, t); - if (r == 0) - DSSWARN("timeout in wait_pending_extra_info_updates\n"); - else if (r < 0) - DSSERR("wait_pending_extra_info_updates failed: %d\n", r); -} - int dss_mgr_wait_for_go(struct omap_overlay_manager *mgr) { unsigned long timeout = msecs_to_jiffies(500); @@ -823,7 +759,6 @@ static void dss_apply_irq_handler(void *data, u32 mask) { const int num_mgrs = dss_feat_get_num_mgrs(); int i; - bool extra_updating; spin_lock(&data_lock); @@ -854,10 +789,6 @@ static void dss_apply_irq_handler(void *data, u32 mask) dss_write_regs(); dss_set_go_bits(); - extra_updating = extra_info_update_ongoing(); - if (!extra_updating) - complete_all(&extra_updated_completion); - if (!need_isr()) dss_unregister_vsync_isr();
Now that fifo merge has been removed, nobody uses the extra_info related completion code, which can be removed. Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> --- drivers/video/omap2/dss/apply.c | 69 --------------------------------------- 1 file changed, 69 deletions(-)