| Message ID | 1346833555-31258-13-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: > dss_mgr_set_timings() can only be called when the output is not active. > This means that most of the code in the function is extra, as there's no > need to write the values to registers, etc, because that will be handled > when the output will be enabled. We need to fix dpi_set_timings() before we can make this change. DPI still tries to change timings on the fly, i.e, with manager enabled. We need to disable and enable the DPI output like we do for other outputs. Archit > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> > --- > drivers/video/omap2/dss/apply.c | 18 ++++++++---------- > 1 file changed, 8 insertions(+), 10 deletions(-) > > diff --git a/drivers/video/omap2/dss/apply.c b/drivers/video/omap2/dss/apply.c > index 53629dd..1b49019 100644 > --- a/drivers/video/omap2/dss/apply.c > +++ b/drivers/video/omap2/dss/apply.c > @@ -1314,21 +1314,19 @@ void dss_mgr_set_timings(struct omap_overlay_manager *mgr, > const struct omap_video_timings *timings) > { > unsigned long flags; > - > - mutex_lock(&apply_lock); > + struct mgr_priv_data *mp = get_mgr_priv(mgr); > > spin_lock_irqsave(&data_lock, flags); > > - dss_apply_mgr_timings(mgr, timings); > - > - dss_write_regs(); > - dss_set_go_bits(); > + if (mp->enabled) { > + DSSERR("cannot set timings for %s: manager needs to be disabled\n", > + mgr->name); > + goto out; > + } > > + dss_apply_mgr_timings(mgr, timings); > +out: > spin_unlock_irqrestore(&data_lock, flags); > - > - wait_pending_extra_info_updates(); > - > - mutex_unlock(&apply_lock); > } > > static void dss_apply_mgr_lcd_config(struct omap_overlay_manager *mgr, > -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" 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 14:45 +0530, Archit Taneja wrote: > On Wednesday 05 September 2012 01:55 PM, Tomi Valkeinen wrote: > > dss_mgr_set_timings() can only be called when the output is not active. > > This means that most of the code in the function is extra, as there's no > > need to write the values to registers, etc, because that will be handled > > when the output will be enabled. > > We need to fix dpi_set_timings() before we can make this change. DPI > still tries to change timings on the fly, i.e, with manager enabled. We > need to disable and enable the DPI output like we do for other outputs. Yep, and for HDMI also (I didn't check the others yet). I think the simplest way to handle this is to only write the dpi.timings in omapdss_dpi_set_timings, and remove the call to dss_mgr_set_timings. This is not perfect, as a call to omapdss_dpi_set_timings when the display is enabled would result in changing the dpi.timings, but the changes wouldn't be actually in use. However, there are only a few places where set_timings is called, and we can make sure in those places to first disable the output and then enable it again. Tomi
On Wednesday 05 September 2012 04:11 PM, Tomi Valkeinen wrote: > On Wed, 2012-09-05 at 14:45 +0530, Archit Taneja wrote: >> On Wednesday 05 September 2012 01:55 PM, Tomi Valkeinen wrote: >>> dss_mgr_set_timings() can only be called when the output is not active. >>> This means that most of the code in the function is extra, as there's no >>> need to write the values to registers, etc, because that will be handled >>> when the output will be enabled. >> >> We need to fix dpi_set_timings() before we can make this change. DPI >> still tries to change timings on the fly, i.e, with manager enabled. We >> need to disable and enable the DPI output like we do for other outputs. > > Yep, and for HDMI also (I didn't check the others yet). I don't think HDMI is impacted, we do the full power off and power on for HDMI, so the manager would be disabled when we set the timings. > > I think the simplest way to handle this is to only write the dpi.timings > in omapdss_dpi_set_timings, and remove the call to dss_mgr_set_timings. > This is not perfect, as a call to omapdss_dpi_set_timings when the > display is enabled would result in changing the dpi.timings, but the > changes wouldn't be actually in use. The simplest way would be to do what other outputs do, disable the output and re-enable the output with the new timings value, if the panel is enabled. Archit -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" 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 17:04 +0530, Archit Taneja wrote: > On Wednesday 05 September 2012 04:11 PM, Tomi Valkeinen wrote: > > On Wed, 2012-09-05 at 14:45 +0530, Archit Taneja wrote: > >> On Wednesday 05 September 2012 01:55 PM, Tomi Valkeinen wrote: > >>> dss_mgr_set_timings() can only be called when the output is not active. > >>> This means that most of the code in the function is extra, as there's no > >>> need to write the values to registers, etc, because that will be handled > >>> when the output will be enabled. > >> > >> We need to fix dpi_set_timings() before we can make this change. DPI > >> still tries to change timings on the fly, i.e, with manager enabled. We > >> need to disable and enable the DPI output like we do for other outputs. > > > > Yep, and for HDMI also (I didn't check the others yet). > > I don't think HDMI is impacted, we do the full power off and power on > for HDMI, so the manager would be disabled when we set the timings. Ah right. > > I think the simplest way to handle this is to only write the dpi.timings > > in omapdss_dpi_set_timings, and remove the call to dss_mgr_set_timings. > > This is not perfect, as a call to omapdss_dpi_set_timings when the > > display is enabled would result in changing the dpi.timings, but the > > changes wouldn't be actually in use. > > The simplest way would be to do what other outputs do, disable the > output and re-enable the output with the new timings value, if the panel > is enabled. Not quite, as there's the mutex in dpi so we can't call enable/disable from set_timings. I could create separate non-locked internal functions for enable and disable, but that feels more complex than just removing the enable & disable from set_timings. In the end we'll anyway only allow changing timings when the output is disabled. The only change I had to do, in addition to removing code from set_timings functions, was to add display disable & enable calls to the "timings" sysfs write. omapfb already only calls set_timings when the output is disabled, and omapdrm does the same. Tomi
On Wednesday 05 September 2012 05:11 PM, Tomi Valkeinen wrote: > On Wed, 2012-09-05 at 17:04 +0530, Archit Taneja wrote: >> On Wednesday 05 September 2012 04:11 PM, Tomi Valkeinen wrote: >>> On Wed, 2012-09-05 at 14:45 +0530, Archit Taneja wrote: >>>> On Wednesday 05 September 2012 01:55 PM, Tomi Valkeinen wrote: >>>>> dss_mgr_set_timings() can only be called when the output is not active. >>>>> This means that most of the code in the function is extra, as there's no >>>>> need to write the values to registers, etc, because that will be handled >>>>> when the output will be enabled. >>>> >>>> We need to fix dpi_set_timings() before we can make this change. DPI >>>> still tries to change timings on the fly, i.e, with manager enabled. We >>>> need to disable and enable the DPI output like we do for other outputs. >>> >>> Yep, and for HDMI also (I didn't check the others yet). >> >> I don't think HDMI is impacted, we do the full power off and power on >> for HDMI, so the manager would be disabled when we set the timings. > > Ah right. > >>> I think the simplest way to handle this is to only write the dpi.timings >>> in omapdss_dpi_set_timings, and remove the call to dss_mgr_set_timings. >>> This is not perfect, as a call to omapdss_dpi_set_timings when the >>> display is enabled would result in changing the dpi.timings, but the >>> changes wouldn't be actually in use. >> >> The simplest way would be to do what other outputs do, disable the >> output and re-enable the output with the new timings value, if the panel >> is enabled. > > Not quite, as there's the mutex in dpi so we can't call enable/disable > from set_timings. I could create separate non-locked internal functions > for enable and disable, but that feels more complex than just removing > the enable & disable from set_timings. Okay, right. > > In the end we'll anyway only allow changing timings when the output is > disabled. > > The only change I had to do, in addition to removing code from > set_timings functions, was to add display disable & enable calls to the > "timings" sysfs write. omapfb already only calls set_timings when the > output is disabled, and omapdrm does the same. Okay, that sounds good then. Archit -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" 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 11:25 +0300, Tomi Valkeinen wrote: > dss_mgr_set_timings() can only be called when the output is not active. > This means that most of the code in the function is extra, as there's no > need to write the values to registers, etc, because that will be handled > when the output will be enabled. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> > --- > drivers/video/omap2/dss/apply.c | 18 ++++++++---------- > 1 file changed, 8 insertions(+), 10 deletions(-) > > diff --git a/drivers/video/omap2/dss/apply.c b/drivers/video/omap2/dss/apply.c > index 53629dd..1b49019 100644 > --- a/drivers/video/omap2/dss/apply.c > +++ b/drivers/video/omap2/dss/apply.c > @@ -1314,21 +1314,19 @@ void dss_mgr_set_timings(struct omap_overlay_manager *mgr, > const struct omap_video_timings *timings) > { > unsigned long flags; > - > - mutex_lock(&apply_lock); > + struct mgr_priv_data *mp = get_mgr_priv(mgr); > > spin_lock_irqsave(&data_lock, flags); > > - dss_apply_mgr_timings(mgr, timings); > - > - dss_write_regs(); > - dss_set_go_bits(); > + if (mp->enabled) { > + DSSERR("cannot set timings for %s: manager needs to be disabled\n", > + mgr->name); > + goto out; > + } There was a problem with this one. When using manual update display, we call set_timings before each update, and the mgr is enabled at that time. I'll fix this by changing the check from mp->enabled to mp->updating. That flag tells if the DISPC channel is actually enabled or not. Enabled flag just tells that the channel is being reserved, although for auto update displays that also implies "updating". But do you see any reason to call set_timings before each update? It was required when we have partial update support, but now we support only full screen updates, so isn't it enough to set the timings just once when configuring? Tomi
On Friday 07 September 2012 03:41 PM, Tomi Valkeinen wrote: > On Wed, 2012-09-05 at 11:25 +0300, Tomi Valkeinen wrote: >> dss_mgr_set_timings() can only be called when the output is not active. >> This means that most of the code in the function is extra, as there's no >> need to write the values to registers, etc, because that will be handled >> when the output will be enabled. >> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> >> --- >> drivers/video/omap2/dss/apply.c | 18 ++++++++---------- >> 1 file changed, 8 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/video/omap2/dss/apply.c b/drivers/video/omap2/dss/apply.c >> index 53629dd..1b49019 100644 >> --- a/drivers/video/omap2/dss/apply.c >> +++ b/drivers/video/omap2/dss/apply.c >> @@ -1314,21 +1314,19 @@ void dss_mgr_set_timings(struct omap_overlay_manager *mgr, >> const struct omap_video_timings *timings) >> { >> unsigned long flags; >> - >> - mutex_lock(&apply_lock); >> + struct mgr_priv_data *mp = get_mgr_priv(mgr); >> >> spin_lock_irqsave(&data_lock, flags); >> >> - dss_apply_mgr_timings(mgr, timings); >> - >> - dss_write_regs(); >> - dss_set_go_bits(); >> + if (mp->enabled) { >> + DSSERR("cannot set timings for %s: manager needs to be disabled\n", >> + mgr->name); >> + goto out; >> + } > > There was a problem with this one. When using manual update display, we > call set_timings before each update, and the mgr is enabled at that > time. > > I'll fix this by changing the check from mp->enabled to mp->updating. > That flag tells if the DISPC channel is actually enabled or not. Enabled > flag just tells that the channel is being reserved, although for auto > update displays that also implies "updating". > > But do you see any reason to call set_timings before each update? It was > required when we have partial update support, but now we support only > full screen updates, so isn't it enough to set the timings just once > when configuring? I think we put it there for rotation. We may need to swap manager width and height before an update. Archit -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" 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/apply.c b/drivers/video/omap2/dss/apply.c index 53629dd..1b49019 100644 --- a/drivers/video/omap2/dss/apply.c +++ b/drivers/video/omap2/dss/apply.c @@ -1314,21 +1314,19 @@ void dss_mgr_set_timings(struct omap_overlay_manager *mgr, const struct omap_video_timings *timings) { unsigned long flags; - - mutex_lock(&apply_lock); + struct mgr_priv_data *mp = get_mgr_priv(mgr); spin_lock_irqsave(&data_lock, flags); - dss_apply_mgr_timings(mgr, timings); - - dss_write_regs(); - dss_set_go_bits(); + if (mp->enabled) { + DSSERR("cannot set timings for %s: manager needs to be disabled\n", + mgr->name); + goto out; + } + dss_apply_mgr_timings(mgr, timings); +out: spin_unlock_irqrestore(&data_lock, flags); - - wait_pending_extra_info_updates(); - - mutex_unlock(&apply_lock); } static void dss_apply_mgr_lcd_config(struct omap_overlay_manager *mgr,
dss_mgr_set_timings() can only be called when the output is not active. This means that most of the code in the function is extra, as there's no need to write the values to registers, etc, because that will be handled when the output will be enabled. Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> --- drivers/video/omap2/dss/apply.c | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-)