| Message ID | 1350472835-28727-10-git-send-email-tomi.valkeinen@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On Wednesday 17 October 2012 04:50 PM, Tomi Valkeinen wrote: > We currently have a single function to enable and disable the manager > output for LCD and DIGIT. The functions are a bit complex, as handling > both enable and disable require some extra steps to ensure that the > output is enabled or disabled properly without errors before exiting the > function. > > The code can be made simpler to understand by splitting the functions > into separate enable and disable functions. We'll also clean up the > comments and some parameter names at the same time. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> > --- > drivers/video/omap2/dss/apply.c | 6 +- > drivers/video/omap2/dss/dispc.c | 178 ++++++++++++++++++++++++--------------- > drivers/video/omap2/dss/dss.h | 3 +- > 3 files changed, 116 insertions(+), 71 deletions(-) > > diff --git a/drivers/video/omap2/dss/apply.c b/drivers/video/omap2/dss/apply.c > index ae9f70d..4fff8ac 100644 > --- a/drivers/video/omap2/dss/apply.c > +++ b/drivers/video/omap2/dss/apply.c > @@ -772,7 +772,7 @@ void dss_mgr_start_update(struct omap_overlay_manager *mgr) > if (!dss_data.irq_enabled && need_isr()) > dss_register_vsync_isr(); > > - dispc_mgr_enable(mgr->id, true); > + dispc_mgr_enable(mgr->id); > > mgr_clear_shadow_dirty(mgr); > > @@ -1027,7 +1027,7 @@ int dss_mgr_enable(struct omap_overlay_manager *mgr) > spin_unlock_irqrestore(&data_lock, flags); > > if (!mgr_manual_update(mgr)) > - dispc_mgr_enable(mgr->id, true); > + dispc_mgr_enable(mgr->id); > > out: > mutex_unlock(&apply_lock); > @@ -1052,7 +1052,7 @@ void dss_mgr_disable(struct omap_overlay_manager *mgr) > goto out; > > if (!mgr_manual_update(mgr)) > - dispc_mgr_enable(mgr->id, false); > + dispc_mgr_disable(mgr->id); > > spin_lock_irqsave(&data_lock, flags); > > diff --git a/drivers/video/omap2/dss/dispc.c b/drivers/video/omap2/dss/dispc.c > index 82339ad..94f393ec 100644 > --- a/drivers/video/omap2/dss/dispc.c > +++ b/drivers/video/omap2/dss/dispc.c > @@ -2590,7 +2590,7 @@ int dispc_ovl_enable(enum omap_plane plane, bool enable) > return 0; > } > > -static void dispc_disable_isr(void *data, u32 mask) > +static void dispc_mgr_disable_isr(void *data, u32 mask) > { > struct completion *compl = data; > complete(compl); > @@ -2608,122 +2608,166 @@ bool dispc_mgr_is_enabled(enum omap_channel channel) > return !!mgr_fld_read(channel, DISPC_MGR_FLD_ENABLE); > } > > -static void dispc_mgr_enable_lcd_out(enum omap_channel channel, bool enable) > +static void dispc_mgr_enable_lcd_out(enum omap_channel channel) > { > - struct completion frame_done_completion; > - bool is_on; > + _enable_mgr_out(channel, true); > +} > + > +static void dispc_mgr_disable_lcd_out(enum omap_channel channel) > +{ > + DECLARE_COMPLETION_ONSTACK(framedone_compl); > int r; > u32 irq; > > - /* When we disable LCD output, we need to wait until frame is done. > - * Otherwise the DSS is still working, and turning off the clocks > - * prevents DSS from going to OFF mode */ > - is_on = dispc_mgr_is_enabled(channel); > + if (dispc_mgr_is_enabled(channel) == false) > + return; > + > + /* > + * When we disable LCD output, we need to wait for FRAMEDONE to know > + * that DISPC has finished with the LCD output. > + */ > > - irq = mgr_desc[channel].framedone_irq; > + irq = dispc_mgr_get_framedone_irq(channel); > > - if (!enable && is_on) { > - init_completion(&frame_done_completion); > + r = omap_dispc_register_isr(dispc_mgr_disable_isr, &framedone_compl, > + irq); > + if (r) > + DSSERR("failed to register FRAMEDONE isr\n"); > > - r = omap_dispc_register_isr(dispc_disable_isr, > - &frame_done_completion, irq); > + _enable_mgr_out(channel, false); > > - if (r) > - DSSERR("failed to register FRAMEDONE isr\n"); > + /* if we couldn't register for framedone, just sleep and exit */ > + if (r) { > + msleep(200); We sleep for 200 ms if we fail to register for framedone. But we just wait for 100ms for FRAMEDONE to occur. It seems a bit incorrect, both should be kept the same, shouldn't they? > + return; > } > > - _enable_mgr_out(channel, enable); > + if (!wait_for_completion_timeout(&framedone_compl, > + msecs_to_jiffies(100))) > + DSSERR("timeout waiting for FRAME DONE\n"); > > - if (!enable && is_on) { > - if (!wait_for_completion_timeout(&frame_done_completion, > - msecs_to_jiffies(100))) > - DSSERR("timeout waiting for FRAME DONE\n"); > + r = omap_dispc_unregister_isr(dispc_mgr_disable_isr, &framedone_compl, > + irq); > + if (r) > + DSSERR("failed to unregister FRAMEDONE isr\n"); > +} > > - r = omap_dispc_unregister_isr(dispc_disable_isr, > - &frame_done_completion, irq); > +static void dispc_digit_out_enable_isr(void *data, u32 mask) > +{ > + struct completion *compl = data; > > - if (r) > - DSSERR("failed to unregister FRAMEDONE isr\n"); > + /* ignore any sync lost interrupts */ > + if (mask & (DISPC_IRQ_EVSYNC_EVEN | DISPC_IRQ_EVSYNC_ODD)) > + complete(compl); > +} > + > +static void dispc_mgr_enable_digit_out(void) > +{ > + DECLARE_COMPLETION_ONSTACK(vsync_compl); > + int r; > + u32 irq_mask; > + > + if (dispc_mgr_is_enabled(OMAP_DSS_CHANNEL_DIGIT) == true) > + return; > + > + /* > + * Digit output produces some sync lost interrupts during the first > + * frame when enabling. Those need to be ignored, so we register for the > + * sync lost irq to prevent the error handler from triggering. > + */ > + > + irq_mask = dispc_mgr_get_vsync_irq(OMAP_DSS_CHANNEL_DIGIT) | > + dispc_mgr_get_sync_lost_irq(OMAP_DSS_CHANNEL_DIGIT); > + > + r = omap_dispc_register_isr(dispc_digit_out_enable_isr, &vsync_compl, > + irq_mask); > + if (r) { > + DSSERR("failed to register %x isr\n", irq_mask); > + return; > } > + > + _enable_mgr_out(OMAP_DSS_CHANNEL_DIGIT, true); > + > + /* wait for the first evsync */ > + if (!wait_for_completion_timeout(&vsync_compl, msecs_to_jiffies(100))) > + DSSERR("timeout waiting for digit out to start\n"); > + > + r = omap_dispc_unregister_isr(dispc_digit_out_enable_isr, &vsync_compl, > + irq_mask); > + if (r) > + DSSERR("failed to unregister %x isr\n", irq_mask); > } > > -static void dispc_mgr_enable_digit_out(bool enable) > +static void dispc_mgr_disable_digit_out(void) > { > - struct completion frame_done_completion; > + DECLARE_COMPLETION_ONSTACK(framedone_compl); > enum dss_hdmi_venc_clk_source_select src; > int r, i; > u32 irq_mask; > int num_irqs; > > - if (dispc_mgr_is_enabled(OMAP_DSS_CHANNEL_DIGIT) == enable) > + if (dispc_mgr_is_enabled(OMAP_DSS_CHANNEL_DIGIT) == false) > return; > > src = dss_get_hdmi_venc_clk_source(); > > - if (enable) { > - unsigned long flags; > - /* When we enable digit output, we'll get an extra digit > - * sync lost interrupt, that we need to ignore */ > - spin_lock_irqsave(&dispc.irq_lock, flags); > - dispc.irq_error_mask &= ~DISPC_IRQ_SYNC_LOST_DIGIT; > - _omap_dispc_set_irqs(); > - spin_unlock_irqrestore(&dispc.irq_lock, flags); > - } > - > - /* When we disable digit output, we need to wait until fields are done. > - * Otherwise the DSS is still working, and turning off the clocks > - * prevents DSS from going to OFF mode. And when enabling, we need to > - * wait for the extra sync losts */ > - init_completion(&frame_done_completion); > + /* > + * When we disable the digit output, we need to wait for FRAMEDONE to > + * know that DISPC has finished with the output. For analog tv out we'll > + * use vsync, as omap2/3 don't have framedone for TV. > + */ > > - if (src == DSS_HDMI_M_PCLK && enable == false) { > + if (src == DSS_HDMI_M_PCLK) { > irq_mask = DISPC_IRQ_FRAMEDONETV; > num_irqs = 1; > } else { > - irq_mask = DISPC_IRQ_EVSYNC_EVEN | DISPC_IRQ_EVSYNC_ODD; > - /* XXX I understand from TRM that we should only wait for the > - * current field to complete. But it seems we have to wait for > - * both fields */ > + irq_mask = dispc_mgr_get_vsync_irq(OMAP_DSS_CHANNEL_DIGIT); > + /* > + * We need to wait for both even and odd vsyncs. Note that this > + * is not totally reliable, as we could get a vsync interrupt > + * before we disable the output, which leads to timeout in the > + * wait_for_completion. > + */ > num_irqs = 2; > } > > - r = omap_dispc_register_isr(dispc_disable_isr, &frame_done_completion, > + r = omap_dispc_register_isr(dispc_mgr_disable_isr, &framedone_compl, > irq_mask); > if (r) > DSSERR("failed to register %x isr\n", irq_mask); We should probably sleep here too as we did for LCD above. 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 2012-10-17 17:38, Archit Taneja wrote: > Hi, > > On Wednesday 17 October 2012 04:50 PM, Tomi Valkeinen wrote: >> - if (r) >> - DSSERR("failed to register FRAMEDONE isr\n"); >> + /* if we couldn't register for framedone, just sleep and exit */ >> + if (r) { >> + msleep(200); > > We sleep for 200 ms if we fail to register for framedone. But we just > wait for 100ms for FRAMEDONE to occur. It seems a bit incorrect, both > should be kept the same, shouldn't they? They are just arbitrary numbers, but you're right, it makes more sense to have same numbers for both. >> - r = omap_dispc_register_isr(dispc_disable_isr, >> &frame_done_completion, >> + r = omap_dispc_register_isr(dispc_mgr_disable_isr, &framedone_compl, >> irq_mask); >> if (r) >> DSSERR("failed to register %x isr\n", irq_mask); > > We should probably sleep here too as we did for LCD above. Yep, good point. I'll add that. Tomi
diff --git a/drivers/video/omap2/dss/apply.c b/drivers/video/omap2/dss/apply.c index ae9f70d..4fff8ac 100644 --- a/drivers/video/omap2/dss/apply.c +++ b/drivers/video/omap2/dss/apply.c @@ -772,7 +772,7 @@ void dss_mgr_start_update(struct omap_overlay_manager *mgr) if (!dss_data.irq_enabled && need_isr()) dss_register_vsync_isr(); - dispc_mgr_enable(mgr->id, true); + dispc_mgr_enable(mgr->id); mgr_clear_shadow_dirty(mgr); @@ -1027,7 +1027,7 @@ int dss_mgr_enable(struct omap_overlay_manager *mgr) spin_unlock_irqrestore(&data_lock, flags); if (!mgr_manual_update(mgr)) - dispc_mgr_enable(mgr->id, true); + dispc_mgr_enable(mgr->id); out: mutex_unlock(&apply_lock); @@ -1052,7 +1052,7 @@ void dss_mgr_disable(struct omap_overlay_manager *mgr) goto out; if (!mgr_manual_update(mgr)) - dispc_mgr_enable(mgr->id, false); + dispc_mgr_disable(mgr->id); spin_lock_irqsave(&data_lock, flags); diff --git a/drivers/video/omap2/dss/dispc.c b/drivers/video/omap2/dss/dispc.c index 82339ad..94f393ec 100644 --- a/drivers/video/omap2/dss/dispc.c +++ b/drivers/video/omap2/dss/dispc.c @@ -2590,7 +2590,7 @@ int dispc_ovl_enable(enum omap_plane plane, bool enable) return 0; } -static void dispc_disable_isr(void *data, u32 mask) +static void dispc_mgr_disable_isr(void *data, u32 mask) { struct completion *compl = data; complete(compl); @@ -2608,122 +2608,166 @@ bool dispc_mgr_is_enabled(enum omap_channel channel) return !!mgr_fld_read(channel, DISPC_MGR_FLD_ENABLE); } -static void dispc_mgr_enable_lcd_out(enum omap_channel channel, bool enable) +static void dispc_mgr_enable_lcd_out(enum omap_channel channel) { - struct completion frame_done_completion; - bool is_on; + _enable_mgr_out(channel, true); +} + +static void dispc_mgr_disable_lcd_out(enum omap_channel channel) +{ + DECLARE_COMPLETION_ONSTACK(framedone_compl); int r; u32 irq; - /* When we disable LCD output, we need to wait until frame is done. - * Otherwise the DSS is still working, and turning off the clocks - * prevents DSS from going to OFF mode */ - is_on = dispc_mgr_is_enabled(channel); + if (dispc_mgr_is_enabled(channel) == false) + return; + + /* + * When we disable LCD output, we need to wait for FRAMEDONE to know + * that DISPC has finished with the LCD output. + */ - irq = mgr_desc[channel].framedone_irq; + irq = dispc_mgr_get_framedone_irq(channel); - if (!enable && is_on) { - init_completion(&frame_done_completion); + r = omap_dispc_register_isr(dispc_mgr_disable_isr, &framedone_compl, + irq); + if (r) + DSSERR("failed to register FRAMEDONE isr\n"); - r = omap_dispc_register_isr(dispc_disable_isr, - &frame_done_completion, irq); + _enable_mgr_out(channel, false); - if (r) - DSSERR("failed to register FRAMEDONE isr\n"); + /* if we couldn't register for framedone, just sleep and exit */ + if (r) { + msleep(200); + return; } - _enable_mgr_out(channel, enable); + if (!wait_for_completion_timeout(&framedone_compl, + msecs_to_jiffies(100))) + DSSERR("timeout waiting for FRAME DONE\n"); - if (!enable && is_on) { - if (!wait_for_completion_timeout(&frame_done_completion, - msecs_to_jiffies(100))) - DSSERR("timeout waiting for FRAME DONE\n"); + r = omap_dispc_unregister_isr(dispc_mgr_disable_isr, &framedone_compl, + irq); + if (r) + DSSERR("failed to unregister FRAMEDONE isr\n"); +} - r = omap_dispc_unregister_isr(dispc_disable_isr, - &frame_done_completion, irq); +static void dispc_digit_out_enable_isr(void *data, u32 mask) +{ + struct completion *compl = data; - if (r) - DSSERR("failed to unregister FRAMEDONE isr\n"); + /* ignore any sync lost interrupts */ + if (mask & (DISPC_IRQ_EVSYNC_EVEN | DISPC_IRQ_EVSYNC_ODD)) + complete(compl); +} + +static void dispc_mgr_enable_digit_out(void) +{ + DECLARE_COMPLETION_ONSTACK(vsync_compl); + int r; + u32 irq_mask; + + if (dispc_mgr_is_enabled(OMAP_DSS_CHANNEL_DIGIT) == true) + return; + + /* + * Digit output produces some sync lost interrupts during the first + * frame when enabling. Those need to be ignored, so we register for the + * sync lost irq to prevent the error handler from triggering. + */ + + irq_mask = dispc_mgr_get_vsync_irq(OMAP_DSS_CHANNEL_DIGIT) | + dispc_mgr_get_sync_lost_irq(OMAP_DSS_CHANNEL_DIGIT); + + r = omap_dispc_register_isr(dispc_digit_out_enable_isr, &vsync_compl, + irq_mask); + if (r) { + DSSERR("failed to register %x isr\n", irq_mask); + return; } + + _enable_mgr_out(OMAP_DSS_CHANNEL_DIGIT, true); + + /* wait for the first evsync */ + if (!wait_for_completion_timeout(&vsync_compl, msecs_to_jiffies(100))) + DSSERR("timeout waiting for digit out to start\n"); + + r = omap_dispc_unregister_isr(dispc_digit_out_enable_isr, &vsync_compl, + irq_mask); + if (r) + DSSERR("failed to unregister %x isr\n", irq_mask); } -static void dispc_mgr_enable_digit_out(bool enable) +static void dispc_mgr_disable_digit_out(void) { - struct completion frame_done_completion; + DECLARE_COMPLETION_ONSTACK(framedone_compl); enum dss_hdmi_venc_clk_source_select src; int r, i; u32 irq_mask; int num_irqs; - if (dispc_mgr_is_enabled(OMAP_DSS_CHANNEL_DIGIT) == enable) + if (dispc_mgr_is_enabled(OMAP_DSS_CHANNEL_DIGIT) == false) return; src = dss_get_hdmi_venc_clk_source(); - if (enable) { - unsigned long flags; - /* When we enable digit output, we'll get an extra digit - * sync lost interrupt, that we need to ignore */ - spin_lock_irqsave(&dispc.irq_lock, flags); - dispc.irq_error_mask &= ~DISPC_IRQ_SYNC_LOST_DIGIT; - _omap_dispc_set_irqs(); - spin_unlock_irqrestore(&dispc.irq_lock, flags); - } - - /* When we disable digit output, we need to wait until fields are done. - * Otherwise the DSS is still working, and turning off the clocks - * prevents DSS from going to OFF mode. And when enabling, we need to - * wait for the extra sync losts */ - init_completion(&frame_done_completion); + /* + * When we disable the digit output, we need to wait for FRAMEDONE to + * know that DISPC has finished with the output. For analog tv out we'll + * use vsync, as omap2/3 don't have framedone for TV. + */ - if (src == DSS_HDMI_M_PCLK && enable == false) { + if (src == DSS_HDMI_M_PCLK) { irq_mask = DISPC_IRQ_FRAMEDONETV; num_irqs = 1; } else { - irq_mask = DISPC_IRQ_EVSYNC_EVEN | DISPC_IRQ_EVSYNC_ODD; - /* XXX I understand from TRM that we should only wait for the - * current field to complete. But it seems we have to wait for - * both fields */ + irq_mask = dispc_mgr_get_vsync_irq(OMAP_DSS_CHANNEL_DIGIT); + /* + * We need to wait for both even and odd vsyncs. Note that this + * is not totally reliable, as we could get a vsync interrupt + * before we disable the output, which leads to timeout in the + * wait_for_completion. + */ num_irqs = 2; } - r = omap_dispc_register_isr(dispc_disable_isr, &frame_done_completion, + r = omap_dispc_register_isr(dispc_mgr_disable_isr, &framedone_compl, irq_mask); if (r) DSSERR("failed to register %x isr\n", irq_mask); - _enable_mgr_out(OMAP_DSS_CHANNEL_DIGIT, enable); + _enable_mgr_out(OMAP_DSS_CHANNEL_DIGIT, false); for (i = 0; i < num_irqs; ++i) { - if (!wait_for_completion_timeout(&frame_done_completion, + if (!wait_for_completion_timeout(&framedone_compl, msecs_to_jiffies(100))) - DSSERR("timeout waiting for digit out to %s\n", - enable ? "start" : "stop"); + DSSERR("timeout waiting for digit out to stop\n"); } - r = omap_dispc_unregister_isr(dispc_disable_isr, &frame_done_completion, + r = omap_dispc_unregister_isr(dispc_mgr_disable_isr, &framedone_compl, irq_mask); if (r) DSSERR("failed to unregister %x isr\n", irq_mask); +} - if (enable) { - unsigned long flags; - spin_lock_irqsave(&dispc.irq_lock, flags); - dispc.irq_error_mask |= DISPC_IRQ_SYNC_LOST_DIGIT; - dispc_write_reg(DISPC_IRQSTATUS, DISPC_IRQ_SYNC_LOST_DIGIT); - _omap_dispc_set_irqs(); - spin_unlock_irqrestore(&dispc.irq_lock, flags); - } +void dispc_mgr_enable(enum omap_channel channel) +{ + if (dss_mgr_is_lcd(channel)) + dispc_mgr_enable_lcd_out(channel); + else if (channel == OMAP_DSS_CHANNEL_DIGIT) + dispc_mgr_enable_digit_out(); + else + WARN_ON(1); } -void dispc_mgr_enable(enum omap_channel channel, bool enable) +void dispc_mgr_disable(enum omap_channel channel) { if (dss_mgr_is_lcd(channel)) - dispc_mgr_enable_lcd_out(channel, enable); + dispc_mgr_disable_lcd_out(channel); else if (channel == OMAP_DSS_CHANNEL_DIGIT) - dispc_mgr_enable_digit_out(enable); + dispc_mgr_disable_digit_out(); else - BUG(); + WARN_ON(1); } void dispc_wb_enable(bool enable) @@ -2740,7 +2784,7 @@ void dispc_wb_enable(bool enable) if (!enable && is_on) { init_completion(&frame_done_completion); - r = omap_dispc_register_isr(dispc_disable_isr, + r = omap_dispc_register_isr(dispc_mgr_disable_isr, &frame_done_completion, irq); if (r) DSSERR("failed to register FRAMEDONEWB isr\n"); @@ -2753,7 +2797,7 @@ void dispc_wb_enable(bool enable) msecs_to_jiffies(100))) DSSERR("timeout waiting for FRAMEDONEWB\n"); - r = omap_dispc_unregister_isr(dispc_disable_isr, + r = omap_dispc_unregister_isr(dispc_mgr_disable_isr, &frame_done_completion, irq); if (r) DSSERR("failed to unregister FRAMEDONEWB isr\n"); diff --git a/drivers/video/omap2/dss/dss.h b/drivers/video/omap2/dss/dss.h index b6e3a06..e501a77 100644 --- a/drivers/video/omap2/dss/dss.h +++ b/drivers/video/omap2/dss/dss.h @@ -464,7 +464,8 @@ u32 dispc_mgr_get_sync_lost_irq(enum omap_channel channel); bool dispc_mgr_go_busy(enum omap_channel channel); void dispc_mgr_go(enum omap_channel channel); bool dispc_mgr_is_enabled(enum omap_channel channel); -void dispc_mgr_enable(enum omap_channel channel, bool enable); +void dispc_mgr_enable(enum omap_channel channel); +void dispc_mgr_disable(enum omap_channel channel); bool dispc_mgr_is_channel_enabled(enum omap_channel channel); void dispc_mgr_set_lcd_config(enum omap_channel channel, const struct dss_lcd_mgr_config *config);
We currently have a single function to enable and disable the manager output for LCD and DIGIT. The functions are a bit complex, as handling both enable and disable require some extra steps to ensure that the output is enabled or disabled properly without errors before exiting the function. The code can be made simpler to understand by splitting the functions into separate enable and disable functions. We'll also clean up the comments and some parameter names at the same time. Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> --- drivers/video/omap2/dss/apply.c | 6 +- drivers/video/omap2/dss/dispc.c | 178 ++++++++++++++++++++++++--------------- drivers/video/omap2/dss/dss.h | 3 +- 3 files changed, 116 insertions(+), 71 deletions(-)