| Message ID | 1343817088-29645-9-git-send-email-archit@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 2012-08-01 at 16:01 +0530, Archit Taneja wrote: > The DSI driver currently relies on the timings in omap_dss_device struct to > configure the DISPC and DSI blocks accordingly. This makes the DSI interface > driver dependent on the omap_dss_device struct. > > Make the DPI driver data maintain it's own timings field. The panel driver is ^^^ DSI > expected to call omapdss_dsi_set_timings() to set these timings before the panel > is enabled. > > Signed-off-by: Archit Taneja <archit@ti.com>d > --- > drivers/video/omap2/displays/panel-taal.c | 2 ++ > drivers/video/omap2/dss/dsi.c | 27 ++++++++++++++++++++++----- > include/video/omapdss.h | 2 ++ > 3 files changed, 26 insertions(+), 5 deletions(-) > > diff --git a/drivers/video/omap2/displays/panel-taal.c b/drivers/video/omap2/displays/panel-taal.c > index 3f5acc7..4775580 100644 > --- a/drivers/video/omap2/displays/panel-taal.c > +++ b/drivers/video/omap2/displays/panel-taal.c > @@ -1060,6 +1060,8 @@ static int taal_power_on(struct omap_dss_device *dssdev) > goto err0; > }; > > + omapdss_dsi_set_timings(dssdev, &td->panel_config->timings); > + > r = omapdss_dsi_display_enable(dssdev); > if (r) { > dev_err(&dssdev->dev, "failed to enable DSI\n"); Video timings for command mode panel are meaningless. If we need to pass the resolution of the panel, perhaps we should have a separate function for that. However, with a quick glance at dsi.c, we don't even use the dssdev->panel.timings for cmd mode panel. But we do use dssdev->get_resolution() in a few places. Those calls could be replaced by storing the panel size in dsi.c, given with omapdss_dsi_set_size() or such. We could use the timings field in dsi.c to store them, though. Tomi
On Tuesday 07 August 2012 07:37 PM, Tomi Valkeinen wrote: > On Wed, 2012-08-01 at 16:01 +0530, Archit Taneja wrote: >> The DSI driver currently relies on the timings in omap_dss_device struct to >> configure the DISPC and DSI blocks accordingly. This makes the DSI interface >> driver dependent on the omap_dss_device struct. >> >> Make the DPI driver data maintain it's own timings field. The panel driver is > ^^^ > DSI > >> expected to call omapdss_dsi_set_timings() to set these timings before the panel >> is enabled. >> >> Signed-off-by: Archit Taneja <archit@ti.com>d >> --- >> drivers/video/omap2/displays/panel-taal.c | 2 ++ >> drivers/video/omap2/dss/dsi.c | 27 ++++++++++++++++++++++----- >> include/video/omapdss.h | 2 ++ >> 3 files changed, 26 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/video/omap2/displays/panel-taal.c b/drivers/video/omap2/displays/panel-taal.c >> index 3f5acc7..4775580 100644 >> --- a/drivers/video/omap2/displays/panel-taal.c >> +++ b/drivers/video/omap2/displays/panel-taal.c >> @@ -1060,6 +1060,8 @@ static int taal_power_on(struct omap_dss_device *dssdev) >> goto err0; >> }; >> >> + omapdss_dsi_set_timings(dssdev, &td->panel_config->timings); >> + >> r = omapdss_dsi_display_enable(dssdev); >> if (r) { >> dev_err(&dssdev->dev, "failed to enable DSI\n"); > > Video timings for command mode panel are meaningless. If we need to pass > the resolution of the panel, perhaps we should have a separate function > for that. > > However, with a quick glance at dsi.c, we don't even use the > dssdev->panel.timings for cmd mode panel. But we do use > dssdev->get_resolution() in a few places. Those calls could be replaced > by storing the panel size in dsi.c, given with omapdss_dsi_set_size() or > such. We could use the timings field in dsi.c to store them, though. I am a bit unclear about resolution when it comes to command mode panels. For command mode panels, we can perform rotation at the panel side. That is, the panel refreshes itself by fetching pixels from it's buffer in a rotated way. Is that right? If the original resolution is 864x480, and we set rotation at panel side to make the rotation 480x864, the DISPC manager size should also be configured at 480x864 right? We seem to be setting the manager timings only once when DSI is enabled. After that, setting rotation doesn't impact manager size. I am asking this to understand if we need to keep resolution as a separate parameter than timings. That is, timings represents the initial width and height of the panel, and resolution represents the current width and height of the panel. 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 Wed, 2012-08-08 at 11:27 +0530, Archit Taneja wrote: > I am a bit unclear about resolution when it comes to command mode panels. Right, it's a bit confusing. And I'm not 100% sure how to manage the rotation. > For command mode panels, we can perform rotation at the panel side. That > is, the panel refreshes itself by fetching pixels from it's buffer in a > rotated way. Is that right? Yes. Well, actually I think the panel stores the pixels in rotated manner when it receives them from OMAP, but it's practically the same. One thing to realize is that this kind of rotation is a bit limited: because there's only one buffer, OMAP will write pixels to the buffers at the same time as the panel shows them. When rotating, this leads to tearing. If the panel has double buffer, that solves the problem, but I haven't seen such panels. Another option is to update the panel in two parts, like N9 does, but that's timing sensitive and a bit tricky. > If the original resolution is 864x480, and we set rotation at panel side > to make the rotation 480x864, the DISPC manager size should also be > configured at 480x864 right? Yep. When we use the panel rotation, from OMAP's point of view the panel resolution has changed. > We seem to be setting the manager timings only once when DSI is enabled. > After that, setting rotation doesn't impact manager size. Hmm, previously the mgr size was set before each update. I wonder if that code has been dropped, probably because we removed the support for partial updates at one point. Without partial updates, the size stays the same, except obviously with rotation. I think I just forgot about rotation at that time. > I am asking this to understand if we need to keep resolution as a > separate parameter than timings. That is, timings represents the initial > width and height of the panel, and resolution represents the current > width and height of the panel. I'm not sure. I think that OMAP doesn't really need to know about the initial resolution. It doesn't really matter from OMAP's point of view. I think I originally kept timings and resolution separately, and the idea was that timings represent the panel's timings, i.e. how it updates the screen from its own memory. And resolution represents the usable resolution, from OMAP's point of view. While I haven't seen such a cmd mode panel, there could be a command sent to the panel to configure its timings. For this we need real timings, not the rotated resolution. However, even in that case the DISPC doesn't need to know about those timings, they would be handled by the panel driver (which could, perhaps, reconfigure the DSI bus speed to match the new timings). So I think that inside omapdss, we don't need separate timings and resolution for DSI cmd mode panels. Tomi
On Wednesday 08 August 2012 11:45 AM, Tomi Valkeinen wrote: > On Wed, 2012-08-08 at 11:27 +0530, Archit Taneja wrote: > >> I am a bit unclear about resolution when it comes to command mode panels. > > Right, it's a bit confusing. And I'm not 100% sure how to manage the > rotation. > >> For command mode panels, we can perform rotation at the panel side. That >> is, the panel refreshes itself by fetching pixels from it's buffer in a >> rotated way. Is that right? > > Yes. Well, actually I think the panel stores the pixels in rotated > manner when it receives them from OMAP, but it's practically the same. > > One thing to realize is that this kind of rotation is a bit limited: > because there's only one buffer, OMAP will write pixels to the buffers > at the same time as the panel shows them. When rotating, this leads to > tearing. > > If the panel has double buffer, that solves the problem, but I haven't > seen such panels. Another option is to update the panel in two parts, > like N9 does, but that's timing sensitive and a bit tricky. > >> If the original resolution is 864x480, and we set rotation at panel side >> to make the rotation 480x864, the DISPC manager size should also be >> configured at 480x864 right? > > Yep. When we use the panel rotation, from OMAP's point of view the panel > resolution has changed. > >> We seem to be setting the manager timings only once when DSI is enabled. >> After that, setting rotation doesn't impact manager size. > > Hmm, previously the mgr size was set before each update. I wonder if > that code has been dropped, probably because we removed the support for > partial updates at one point. Without partial updates, the size stays > the same, except obviously with rotation. I think I just forgot about > rotation at that time. I tried out rotation on Taal, and it only works for 180 degrees(and 0 of course), 90 and 270 result in no output. I'll add a dss_mgr_set_timings() in omap_dsi_update, that should sort of fix it, but someone would need to reconfigure the connected overlays too before trying out an update. > >> I am asking this to understand if we need to keep resolution as a >> separate parameter than timings. That is, timings represents the initial >> width and height of the panel, and resolution represents the current >> width and height of the panel. > > I'm not sure. I think that OMAP doesn't really need to know about the > initial resolution. It doesn't really matter from OMAP's point of view. > > I think I originally kept timings and resolution separately, and the > idea was that timings represent the panel's timings, i.e. how it updates > the screen from its own memory. And resolution represents the usable > resolution, from OMAP's point of view. > > While I haven't seen such a cmd mode panel, there could be a command > sent to the panel to configure its timings. For this we need real > timings, not the rotated resolution. > > However, even in that case the DISPC doesn't need to know about those > timings, they would be handled by the panel driver (which could, > perhaps, reconfigure the DSI bus speed to match the new timings). So I > think that inside omapdss, we don't need separate timings and resolution > for DSI cmd mode panels. Ok. 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 Wed, 2012-08-08 at 11:59 +0530, Archit Taneja wrote: > I tried out rotation on Taal, and it only works for 180 degrees(and 0 of > course), 90 and 270 result in no output. I'll add a > dss_mgr_set_timings() in omap_dsi_update, that should sort of fix it, > but someone would need to reconfigure the connected overlays too before > trying out an update. Right, but that's something omapdss/panel cannot do, it must be done by the user. The same problem is there with changing, say, DPI mode also. Btw, can you separate smaller cleanups/fixes to another patch series, to make this series even slightly smaller? I think at least the first patches in this series are quite separate, and the rotation fix is also. Tomi
On Wednesday 08 August 2012 12:40 PM, Tomi Valkeinen wrote: > On Wed, 2012-08-08 at 11:59 +0530, Archit Taneja wrote: > >> I tried out rotation on Taal, and it only works for 180 degrees(and 0 of >> course), 90 and 270 result in no output. I'll add a >> dss_mgr_set_timings() in omap_dsi_update, that should sort of fix it, >> but someone would need to reconfigure the connected overlays too before >> trying out an update. > > Right, but that's something omapdss/panel cannot do, it must be done by > the user. The same problem is there with changing, say, DPI mode also. > > Btw, can you separate smaller cleanups/fixes to another patch series, to > make this series even slightly smaller? I think at least the first > patches in this series are quite separate, and the rotation fix is also. Right, I'll do that. 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
diff --git a/drivers/video/omap2/displays/panel-taal.c b/drivers/video/omap2/displays/panel-taal.c index 3f5acc7..4775580 100644 --- a/drivers/video/omap2/displays/panel-taal.c +++ b/drivers/video/omap2/displays/panel-taal.c @@ -1060,6 +1060,8 @@ static int taal_power_on(struct omap_dss_device *dssdev) goto err0; }; + omapdss_dsi_set_timings(dssdev, &td->panel_config->timings); + r = omapdss_dsi_display_enable(dssdev); if (r) { dev_err(&dssdev->dev, "failed to enable DSI\n"); diff --git a/drivers/video/omap2/dss/dsi.c b/drivers/video/omap2/dss/dsi.c index 27c27c4..598d965 100644 --- a/drivers/video/omap2/dss/dsi.c +++ b/drivers/video/omap2/dss/dsi.c @@ -333,6 +333,7 @@ struct dsi_data { unsigned scp_clk_refcount; struct dss_lcd_mgr_config mgr_config; + struct omap_video_timings timings; }; struct dsi_packet_sent_handler_data { @@ -3607,9 +3608,10 @@ static void dsi_config_vp_num_line_buffers(struct omap_dss_device *dssdev) int num_line_buffers; if (dssdev->panel.dsi_mode == OMAP_DSS_DSI_VIDEO_MODE) { + struct dsi_data *dsi = dsi_get_dsidrv_data(dsidev); int bpp = dsi_get_pixel_size(dssdev->panel.dsi_pix_fmt); unsigned line_buf_size = dsi_get_line_buf_size(dsidev); - struct omap_video_timings *timings = &dssdev->panel.timings; + struct omap_video_timings *timings = &dsi->timings; /* * Don't use line buffers if width is greater than the video * port's line buffer size @@ -3738,7 +3740,7 @@ static void dsi_config_cmd_mode_interleaving(struct omap_dss_device *dssdev) int ddr_clk_pre, ddr_clk_post, enter_hs_mode_lat, exit_hs_mode_lat; int tclk_trail, ths_exit, exiths_clk; bool ddr_alwon; - struct omap_video_timings *timings = &dssdev->panel.timings; + struct omap_video_timings *timings = &dsi->timings; int bpp = dsi_get_pixel_size(dssdev->panel.dsi_pix_fmt); int ndl = dsi->num_lanes_used - 1; const struct omapdss_clock_config *clks; @@ -3996,7 +3998,7 @@ static void dsi_proto_timings(struct omap_dss_device *dssdev) int vbp = dssdev->panel.dsi_vm_data.vbp; int window_sync = dssdev->panel.dsi_vm_data.window_sync; bool hsync_end = dssdev->panel.dsi_vm_data.vp_hsync_end; - struct omap_video_timings *timings = &dssdev->panel.timings; + struct omap_video_timings *timings = &dsi->timings; int bpp = dsi_get_pixel_size(dssdev->panel.dsi_pix_fmt); int tl, t_he, width_bytes; @@ -4105,6 +4107,7 @@ EXPORT_SYMBOL(omapdss_dsi_configure_pins); int dsi_enable_video_output(struct omap_dss_device *dssdev, int channel) { struct platform_device *dsidev = dsi_get_dsidev_from_dssdev(dssdev); + struct dsi_data *dsi = dsi_get_dsidrv_data(dsidev); int bpp = dsi_get_pixel_size(dssdev->panel.dsi_pix_fmt); u8 data_type; u16 word_count; @@ -4135,7 +4138,7 @@ int dsi_enable_video_output(struct omap_dss_device *dssdev, int channel) /* MODE, 1 = video mode */ REG_FLD_MOD(dsidev, DSI_VC_CTRL(channel), 1, 4, 4); - word_count = DIV_ROUND_UP(dssdev->panel.timings.x_res * bpp, 8); + word_count = DIV_ROUND_UP(dsi->timings.x_res * bpp, 8); dsi_vc_write_long_header(dsidev, channel, data_type, word_count, 0); @@ -4401,7 +4404,7 @@ static int dsi_display_init_dispc(struct omap_dss_device *dssdev) dsi->mgr_config.stallmode = true; dsi->mgr_config.fifohandcheck = true; } else { - timings = dssdev->panel.timings; + timings = dsi->timings; dsi->mgr_config.stallmode = false; dsi->mgr_config.fifohandcheck = false; @@ -4688,6 +4691,20 @@ int omapdss_dsi_enable_te(struct omap_dss_device *dssdev, bool enable) } EXPORT_SYMBOL(omapdss_dsi_enable_te); +void omapdss_dsi_set_timings(struct omap_dss_device *dssdev, + struct omap_video_timings *timings) +{ + struct platform_device *dsidev = dsi_get_dsidev_from_dssdev(dssdev); + struct dsi_data *dsi = dsi_get_dsidrv_data(dsidev); + + mutex_lock(&dsi->lock); + + dsi->timings = *timings; + + mutex_unlock(&dsi->lock); +} +EXPORT_SYMBOL(omapdss_dsi_set_timings); + static int __init dsi_init_display(struct omap_dss_device *dssdev) { struct platform_device *dsidev = dsi_get_dsidev_from_dssdev(dssdev); diff --git a/include/video/omapdss.h b/include/video/omapdss.h index ebf2ebd..a2cd133 100644 --- a/include/video/omapdss.h +++ b/include/video/omapdss.h @@ -736,6 +736,8 @@ int omap_dispc_wait_for_irq_interruptible_timeout(u32 irqmask, void omapdss_dsi_vc_enable_hs(struct omap_dss_device *dssdev, int channel, bool enable); int omapdss_dsi_enable_te(struct omap_dss_device *dssdev, bool enable); +void omapdss_dsi_set_timings(struct omap_dss_device *dssdev, + struct omap_video_timings *timings); int omap_dsi_update(struct omap_dss_device *dssdev, int channel, void (*callback)(int, void *), void *data);
The DSI driver currently relies on the timings in omap_dss_device struct to configure the DISPC and DSI blocks accordingly. This makes the DSI interface driver dependent on the omap_dss_device struct. Make the DPI driver data maintain it's own timings field. The panel driver is expected to call omapdss_dsi_set_timings() to set these timings before the panel is enabled. Signed-off-by: Archit Taneja <archit@ti.com>d --- drivers/video/omap2/displays/panel-taal.c | 2 ++ drivers/video/omap2/dss/dsi.c | 27 ++++++++++++++++++++++----- include/video/omapdss.h | 2 ++ 3 files changed, 26 insertions(+), 5 deletions(-)