| Message ID | 1344512989-4071-10-git-send-email-archit@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 2012-08-09 at 17:19 +0530, Archit Taneja wrote: > Create function omapdss_sdi_set_timings(). Configuring new timings is done the > same way as before, SDI is disabled, and re-enabled with the new timings in > dssdev. This just moves the code from the panel drivers to the SDI driver. > > The panel drivers shouldn't be aware of how SDI manages to configure a new set > of timings. This should be taken care of by the SDI driver itself. I'm not sure about this one. Although I see that dpi.c does currently the same thing as you're doing in your patch. One thing is that we should try to remove dssdev uses from the output drivers, including use of dssdev->state. The other thing is that I don't think the output driver should disable & enable the output during set timings. I think sdi's set_timings should return EBUSY if the output is enabled. The same way as other configuration functions should (like dpi_set_data_lines or such). I'm actually not sure if even the panel driver should disable & enable the output during set_timings. Perhaps it should be the caller's (omapdrm or such) responsibility.... My reasoning here is that disabling & enabling the video output is not invisible to the upper layers, so doing it "in secret" may be bad. Then again, perhaps timings can be changed freely on some other platforms, and then it'd be nice if the panel driver wouldn't disable & enable the output. So I'm again not quite sure what's the best way to handle this... (of the dssdev->state I'm sure, its use should be removed from omapdss). Any thoughts? Tomi
On Tuesday 14 August 2012 07:14 PM, Tomi Valkeinen wrote: > On Thu, 2012-08-09 at 17:19 +0530, Archit Taneja wrote: >> Create function omapdss_sdi_set_timings(). Configuring new timings is done the >> same way as before, SDI is disabled, and re-enabled with the new timings in >> dssdev. This just moves the code from the panel drivers to the SDI driver. >> >> The panel drivers shouldn't be aware of how SDI manages to configure a new set >> of timings. This should be taken care of by the SDI driver itself. > > I'm not sure about this one. Although I see that dpi.c does currently > the same thing as you're doing in your patch. Even HDMI does the same thing. > > One thing is that we should try to remove dssdev uses from the output > drivers, including use of dssdev->state. Yes, we could do that by keeping a state of the output(and also checking state of the manager) > > The other thing is that I don't think the output driver should disable & > enable the output during set timings. I think sdi's set_timings should > return EBUSY if the output is enabled. The same way as other > configuration functions should (like dpi_set_data_lines or such). > > I'm actually not sure if even the panel driver should disable & enable > the output during set_timings. Perhaps it should be the caller's > (omapdrm or such) responsibility.... > > My reasoning here is that disabling & enabling the video output is not > invisible to the upper layers, so doing it "in secret" may be bad. > > Then again, perhaps timings can be changed freely on some other > platforms, and then it'd be nice if the panel driver wouldn't disable & > enable the output. > > So I'm again not quite sure what's the best way to handle this... (of > the dssdev->state I'm sure, its use should be removed from omapdss). Any > thoughts? I guess it depends on how drm/fb want to use it. I guess an output should have a set_timings() kind of op if it can do it seamlessly. I guess we can do that easily in DPI, for example, we could reduce the fps from 60 to 30 without causing an artefacts(I think). For outputs which can't do it, we could remove the set_timings totally. However, it'll be kind of inconsistent for some outputs to set timings, and for others to not, and if in the future drm/fb gets exposed to ops too, we may have dirty checks to see if set_timings is populated or not. The easiest way would be to make all set_timings just update the copy of the timings output has, and expect drm/fb to disable and re enable the panel. We may end up doing unnecessary gpio resets and configuration of the panels though. 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 22:26 +0530, Archit Taneja wrote: > On Tuesday 14 August 2012 07:14 PM, Tomi Valkeinen wrote: > I guess it depends on how drm/fb want to use it. I guess an output > should have a set_timings() kind of op if it can do it seamlessly. I > guess we can do that easily in DPI, for example, we could reduce the fps > from 60 to 30 without causing an artefacts(I think). For outputs which Yes, that kind of thing is easy to do by just changing the pck divider, which is in a shadow register. I mean, "easy" in theory, at least. Our clock calculation doesn't work like that currently, though, so it could end up changing DSS fck. > can't do it, we could remove the set_timings totally. But we do need set_timings for other outputs also (like sdi). We just can't change them just like that. Only outputs that do not need timings are DSI command mode and rfbi. > However, it'll be kind of inconsistent for some outputs to set timings, > and for others to not, and if in the future drm/fb gets exposed to ops > too, we may have dirty checks to see if set_timings is populated or not. > > The easiest way would be to make all set_timings just update the copy of > the timings output has, and expect drm/fb to disable and re enable the > panel. We may end up doing unnecessary gpio resets and configuration of > the panels though. I think changing things like timings is quite a rare operation. The only case it'd be necessary to change timings often, with speed, and without artifacts would be the fps drop you mentioned, for lower power use with panels that don't mind the fps drop. If I understood correctly, Rob said that drm already disables the output when changing the mode, when I asked if it's ok for the apply's set_timings to require the output to be off. In any case this is not a big issue, I mean, it's not causing any problems. Somebody is going to disable the output anyway when changing the timings. Perhaps even these patches are good, because they make the set_timings consistent across the output drivers (don't they?). Tomi
On Tuesday 14 August 2012 11:03 PM, Tomi Valkeinen wrote: > On Tue, 2012-08-14 at 22:26 +0530, Archit Taneja wrote: >> On Tuesday 14 August 2012 07:14 PM, Tomi Valkeinen wrote: > >> I guess it depends on how drm/fb want to use it. I guess an output >> should have a set_timings() kind of op if it can do it seamlessly. I >> guess we can do that easily in DPI, for example, we could reduce the fps >> from 60 to 30 without causing an artefacts(I think). For outputs which > > Yes, that kind of thing is easy to do by just changing the pck divider, > which is in a shadow register. I mean, "easy" in theory, at least. Our > clock calculation doesn't work like that currently, though, so it could > end up changing DSS fck. > >> can't do it, we could remove the set_timings totally. > > But we do need set_timings for other outputs also (like sdi). We just > can't change them just like that. Only outputs that do not need timings > are DSI command mode and rfbi. > >> However, it'll be kind of inconsistent for some outputs to set timings, >> and for others to not, and if in the future drm/fb gets exposed to ops >> too, we may have dirty checks to see if set_timings is populated or not. >> >> The easiest way would be to make all set_timings just update the copy of >> the timings output has, and expect drm/fb to disable and re enable the >> panel. We may end up doing unnecessary gpio resets and configuration of >> the panels though. > > I think changing things like timings is quite a rare operation. The only > case it'd be necessary to change timings often, with speed, and without > artifacts would be the fps drop you mentioned, for lower power use with > panels that don't mind the fps drop. > > If I understood correctly, Rob said that drm already disables the output > when changing the mode, when I asked if it's ok for the apply's > set_timings to require the output to be off. > > In any case this is not a big issue, I mean, it's not causing any > problems. Somebody is going to disable the output anyway when changing > the timings. Perhaps even these patches are good, because they make the > set_timings consistent across the output drivers (don't they?). Yes, they do, there isn't a set_timings for RFBI though, only a set_size, and DSI has set_timings for video mode and a set_size for command mode, I haven't put checks in the ops for a panel driver to wrongly call set_timings in commmand mode, and set_size in video mode. I haven't done that yet because a future patch of mine will have a DSI specific op called set_operation_mode() to make us independent of dssdev->panel.dsi_mode, there is no guarantee that the panel driver to first call set_operation_mode(), and then set_timings(), so I'm not sure yet how to deal with that. Probably having a mode/state which says that a field is unintialized might help, but that would overcomplicate things. 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, Aug 14, 2012 at 11:56 AM, Archit Taneja <archit@ti.com> wrote: > On Tuesday 14 August 2012 07:14 PM, Tomi Valkeinen wrote: >> >> On Thu, 2012-08-09 at 17:19 +0530, Archit Taneja wrote: >>> >>> Create function omapdss_sdi_set_timings(). Configuring new timings is >>> done the >>> same way as before, SDI is disabled, and re-enabled with the new timings >>> in >>> dssdev. This just moves the code from the panel drivers to the SDI >>> driver. >>> >>> The panel drivers shouldn't be aware of how SDI manages to configure a >>> new set >>> of timings. This should be taken care of by the SDI driver itself. >> >> >> I'm not sure about this one. Although I see that dpi.c does currently >> the same thing as you're doing in your patch. > > > Even HDMI does the same thing. > > >> >> One thing is that we should try to remove dssdev uses from the output >> drivers, including use of dssdev->state. > > > Yes, we could do that by keeping a state of the output(and also checking > state of the manager) > > >> >> The other thing is that I don't think the output driver should disable & >> enable the output during set timings. I think sdi's set_timings should >> return EBUSY if the output is enabled. The same way as other >> configuration functions should (like dpi_set_data_lines or such). >> >> I'm actually not sure if even the panel driver should disable & enable >> the output during set_timings. Perhaps it should be the caller's >> (omapdrm or such) responsibility.... >> >> My reasoning here is that disabling & enabling the video output is not >> invisible to the upper layers, so doing it "in secret" may be bad. >> >> Then again, perhaps timings can be changed freely on some other >> platforms, and then it'd be nice if the panel driver wouldn't disable & >> enable the output. >> >> So I'm again not quite sure what's the best way to handle this... (of >> the dssdev->state I'm sure, its use should be removed from omapdss). Any >> thoughts? > > > I guess it depends on how drm/fb want to use it. I guess an output should > have a set_timings() kind of op if it can do it seamlessly. I guess we can > do that easily in DPI, for example, we could reduce the fps from 60 to 30 > without causing an artefacts(I think). For outputs which can't do it, we > could remove the set_timings totally. fwiw, drm wouldn't try to change timings on the fly.. or at least it is bracketed by a call to the driver's crtc->prepare() and crtc->commit() fxns (which in our case disable/enable output). I haven't seen much userspace that tries to do things like this, except maybe some apps like xbmc which seem to have some options to attempt to change timings to align w/ video playback framerate. I am a bit curious how many tv's and drivers could support this in a glitch-free way. BR, -R > However, it'll be kind of inconsistent for some outputs to set timings, and > for others to not, and if in the future drm/fb gets exposed to ops too, we > may have dirty checks to see if set_timings is populated or not. > > The easiest way would be to make all set_timings just update the copy of the > timings output has, and expect drm/fb to disable and re enable the panel. We > may end up doing unnecessary gpio resets and configuration of the panels > though. > > 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 -- 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-08-15 at 00:38 +0530, Archit Taneja wrote: > On Tuesday 14 August 2012 11:03 PM, Tomi Valkeinen wrote: > > On Tue, 2012-08-14 at 22:26 +0530, Archit Taneja wrote: > >> On Tuesday 14 August 2012 07:14 PM, Tomi Valkeinen wrote: > > > >> I guess it depends on how drm/fb want to use it. I guess an output > >> should have a set_timings() kind of op if it can do it seamlessly. I > >> guess we can do that easily in DPI, for example, we could reduce the fps > >> from 60 to 30 without causing an artefacts(I think). For outputs which > > > > Yes, that kind of thing is easy to do by just changing the pck divider, > > which is in a shadow register. I mean, "easy" in theory, at least. Our > > clock calculation doesn't work like that currently, though, so it could > > end up changing DSS fck. > > > >> can't do it, we could remove the set_timings totally. > > > > But we do need set_timings for other outputs also (like sdi). We just > > can't change them just like that. Only outputs that do not need timings > > are DSI command mode and rfbi. > > > >> However, it'll be kind of inconsistent for some outputs to set timings, > >> and for others to not, and if in the future drm/fb gets exposed to ops > >> too, we may have dirty checks to see if set_timings is populated or not. > >> > >> The easiest way would be to make all set_timings just update the copy of > >> the timings output has, and expect drm/fb to disable and re enable the > >> panel. We may end up doing unnecessary gpio resets and configuration of > >> the panels though. > > > > I think changing things like timings is quite a rare operation. The only > > case it'd be necessary to change timings often, with speed, and without > > artifacts would be the fps drop you mentioned, for lower power use with > > panels that don't mind the fps drop. > > > > If I understood correctly, Rob said that drm already disables the output > > when changing the mode, when I asked if it's ok for the apply's > > set_timings to require the output to be off. > > > > In any case this is not a big issue, I mean, it's not causing any > > problems. Somebody is going to disable the output anyway when changing > > the timings. Perhaps even these patches are good, because they make the > > set_timings consistent across the output drivers (don't they?). > > Yes, they do, there isn't a set_timings for RFBI though, only a > set_size, and DSI has set_timings for video mode and a set_size for > command mode, I haven't put checks in the ops for a panel driver to > wrongly call set_timings in commmand mode, and set_size in video mode. Ok. Well, perhaps we should go forward with these patches then. They make things consistent, and we don't really know which would be the best way to handle this, so the method in your patches is as good as some other. The dssdev->state needs to be removed at some point, but that can be a separate task. > I haven't done that yet because a future patch of mine will have a DSI > specific op called set_operation_mode() to make us independent of > dssdev->panel.dsi_mode, there is no guarantee that the panel driver to > first call set_operation_mode(), and then set_timings(), so I'm not sure > yet how to deal with that. Probably having a mode/state which says that > a field is unintialized might help, but that would overcomplicate things. Well, we can define that set_operation_mode needs to be called before any other dsi functions. They are kernel drivers, we can presume they act correctly (although we should of course try to handle error cases anyway). If they don't, we need to fix them. If you want to be extra safe there, you could add third value to the dsi_mode enum: UNDEFINED or such (I guess this is what you meant also). By default dsi's mode would be undefined, and functions could check it. But I agree it'd add lots of checks all around. Tomi
diff --git a/drivers/video/omap2/displays/panel-acx565akm.c b/drivers/video/omap2/displays/panel-acx565akm.c index eaeed43..11bdc88 100644 --- a/drivers/video/omap2/displays/panel-acx565akm.c +++ b/drivers/video/omap2/displays/panel-acx565akm.c @@ -731,18 +731,7 @@ static int acx_panel_resume(struct omap_dss_device *dssdev) static void acx_panel_set_timings(struct omap_dss_device *dssdev, struct omap_video_timings *timings) { - int r; - - if (dssdev->state == OMAP_DSS_DISPLAY_ACTIVE) - omapdss_sdi_display_disable(dssdev); - - dssdev->panel.timings = *timings; - - if (dssdev->state == OMAP_DSS_DISPLAY_ACTIVE) { - r = omapdss_sdi_display_enable(dssdev); - if (r) - dev_err(&dssdev->dev, "%s enable failed\n", __func__); - } + omapdss_sdi_set_timings(dssdev, timings); } static int acx_panel_check_timings(struct omap_dss_device *dssdev, diff --git a/drivers/video/omap2/dss/sdi.c b/drivers/video/omap2/dss/sdi.c index 5d31699..0474962 100644 --- a/drivers/video/omap2/dss/sdi.c +++ b/drivers/video/omap2/dss/sdi.c @@ -146,6 +146,23 @@ void omapdss_sdi_display_disable(struct omap_dss_device *dssdev) } EXPORT_SYMBOL(omapdss_sdi_display_disable); +void omapdss_sdi_set_timings(struct omap_dss_device *dssdev, + struct omap_video_timings *timings) +{ + int r; + + dssdev->panel.timings = *timings; + + if (dssdev->state == OMAP_DSS_DISPLAY_ACTIVE) { + omapdss_sdi_display_disable(dssdev); + + r = omapdss_sdi_display_enable(dssdev); + if (r) + DSSERR("failed to set new timings\n"); + } +} +EXPORT_SYMBOL(omapdss_sdi_set_timings); + static int __init sdi_init_display(struct omap_dss_device *dssdev) { DSSDBG("SDI init\n"); diff --git a/include/video/omapdss.h b/include/video/omapdss.h index a85b3fb..c5c013e 100644 --- a/include/video/omapdss.h +++ b/include/video/omapdss.h @@ -761,6 +761,8 @@ int dpi_check_timings(struct omap_dss_device *dssdev, int omapdss_sdi_display_enable(struct omap_dss_device *dssdev); void omapdss_sdi_display_disable(struct omap_dss_device *dssdev); +void omapdss_sdi_set_timings(struct omap_dss_device *dssdev, + struct omap_video_timings *timings); int omapdss_rfbi_display_enable(struct omap_dss_device *dssdev); void omapdss_rfbi_display_disable(struct omap_dss_device *dssdev);
Create function omapdss_sdi_set_timings(). Configuring new timings is done the same way as before, SDI is disabled, and re-enabled with the new timings in dssdev. This just moves the code from the panel drivers to the SDI driver. The panel drivers shouldn't be aware of how SDI manages to configure a new set of timings. This should be taken care of by the SDI driver itself. Signed-off-by: Archit Taneja <archit@ti.com> --- drivers/video/omap2/displays/panel-acx565akm.c | 13 +------------ drivers/video/omap2/dss/sdi.c | 17 +++++++++++++++++ include/video/omapdss.h | 2 ++ 3 files changed, 20 insertions(+), 12 deletions(-)