| Message ID | 1346326845-16583-10-git-send-email-archit@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 2012-08-30 at 17:10 +0530, Archit Taneja wrote: > Links between DSS entities are made in dss_recheck_connections when a new panel > is probed. Rewrite the code in dss_recheck_connections to link managers to > outputs, and outputs to devices. > > The fields in omap_dss_device struct gives information on which output and > manager to connect to. The desired manager and output pointers are retrieved and > prepared(existing outputs/devices unset, if default display)) to form the > desired links. The output is linked to the device, and then the manager to the > output. If a probed device's required manager isn't free, the required output > is still connected to the device so that it's easier to use the panel in the > future. I've been pondering this one, and I can't come to a conclusion. The recheck_connections is just so ugly that I'd like to get rid of it =). I guess this patch doesn't make it any more ugly, so we can include it in the patch series. And as I mentioned earlier, I think we should get rid of the OMAP_DISPLAY_TYPE_* enum, as it's kinda extra now. But doing that would require changing all board files. That's not out of the question, but I think we should first make sure we know what we are doing with the board files so we don't go back and forth there. Just gathering my thoughts: When we define a panel in DT/board file, we should also define the output instance, because that's part of the hardware design. But there are no hardcoded links between mgrs and outputs, so that doesn't belong to the DT/board file. For example, omap4460's DSI1 can take input from LCD1 or LCD2. So who could/should make the decision which mgr to use... Well, I don't know on what basis the decision can be made, but I still think omapdss can't make good decisions on that, so probably the whole "chain" should be configured in the omapfb/omapdrm level. Tomi
On Friday 31 August 2012 07:40 PM, Tomi Valkeinen wrote: > On Thu, 2012-08-30 at 17:10 +0530, Archit Taneja wrote: >> Links between DSS entities are made in dss_recheck_connections when a new panel >> is probed. Rewrite the code in dss_recheck_connections to link managers to >> outputs, and outputs to devices. >> >> The fields in omap_dss_device struct gives information on which output and >> manager to connect to. The desired manager and output pointers are retrieved and >> prepared(existing outputs/devices unset, if default display)) to form the >> desired links. The output is linked to the device, and then the manager to the >> output. If a probed device's required manager isn't free, the required output >> is still connected to the device so that it's easier to use the panel in the >> future. > > I've been pondering this one, and I can't come to a conclusion. The > recheck_connections is just so ugly that I'd like to get rid of it =). I > guess this patch doesn't make it any more ugly, so we can include it in > the patch series. > > And as I mentioned earlier, I think we should get rid of the > OMAP_DISPLAY_TYPE_* enum, as it's kinda extra now. But doing that would > require changing all board files. That's not out of the question, but I > think we should first make sure we know what we are doing with the board > files so we don't go back and forth there. Yes, I didn't remove it for the same reason. > > Just gathering my thoughts: > > When we define a panel in DT/board file, we should also define the > output instance, because that's part of the hardware design. But there It's a part of hardware design if the panel can't be detached. But yes, even for detachable panels, we can assume that the panel would eventually connect to that output. > are no hardcoded links between mgrs and outputs, so that doesn't belong > to the DT/board file. For example, omap4460's DSI1 can take input from > LCD1 or LCD2. Right, so we don't need an equivalent of the dssdev->channel field in DT info. As you said, we need the output instance, is that why you were sceptical about it being defined as an enum in previous patches? Probably we could define output instances in DT as a pair of instance number and instance type {number, type}. It would be sort of hard to play around with those within OMAPDSS though. > > So who could/should make the decision which mgr to use... Well, I don't > know on what basis the decision can be made, but I still think omapdss > can't make good decisions on that, so probably the whole "chain" should > be configured in the omapfb/omapdrm level. Which manager to chose could be simply picking up the next free manager which can connect to that output. omapfb/drm can take care of 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
On Fri, 2012-08-31 at 19:54 +0530, Archit Taneja wrote: > On Friday 31 August 2012 07:40 PM, Tomi Valkeinen wrote: > > On Thu, 2012-08-30 at 17:10 +0530, Archit Taneja wrote: > >> Links between DSS entities are made in dss_recheck_connections when a new panel > >> is probed. Rewrite the code in dss_recheck_connections to link managers to > >> outputs, and outputs to devices. > >> > >> The fields in omap_dss_device struct gives information on which output and > >> manager to connect to. The desired manager and output pointers are retrieved and > >> prepared(existing outputs/devices unset, if default display)) to form the > >> desired links. The output is linked to the device, and then the manager to the > >> output. If a probed device's required manager isn't free, the required output > >> is still connected to the device so that it's easier to use the panel in the > >> future. > > > > I've been pondering this one, and I can't come to a conclusion. The > > recheck_connections is just so ugly that I'd like to get rid of it =). I > > guess this patch doesn't make it any more ugly, so we can include it in > > the patch series. > > > > And as I mentioned earlier, I think we should get rid of the > > OMAP_DISPLAY_TYPE_* enum, as it's kinda extra now. But doing that would > > require changing all board files. That's not out of the question, but I > > think we should first make sure we know what we are doing with the board > > files so we don't go back and forth there. > > Yes, I didn't remove it for the same reason. > > > > > Just gathering my thoughts: > > > > When we define a panel in DT/board file, we should also define the > > output instance, because that's part of the hardware design. But there > > It's a part of hardware design if the panel can't be detached. But yes, > even for detachable panels, we can assume that the panel would > eventually connect to that output. > > > are no hardcoded links between mgrs and outputs, so that doesn't belong > > to the DT/board file. For example, omap4460's DSI1 can take input from > > LCD1 or LCD2. > > Right, so we don't need an equivalent of the dssdev->channel field in DT > info. As you said, we need the output instance, is that why you were What I'm planning for DT is a direct link to the output instance. Something like this (not correct, just to give the idea): dss { dpi { }; }; i2c { /* an i2c controlled panel */ my-panel { video-bus = <&dpi>; }; }; Then my-panel can get the handle from video-bus, and it'll get the output device directly, without need for any IDs or such. > sceptical about it being defined as an enum in previous patches? > Probably we could define output instances in DT as a pair of instance > number and instance type {number, type}. It would be sort of hard to > play around with those within OMAPDSS though. Well, optimally we wouldn't need to know about display types or output instances, at least in most of the cases. We'd just have a bunch of managers, and a bunch of outputs, and rules how these can be connected. And the panel devices would have a link to the output it's connect in HW level. There are probably some cases where we need some kind of ID, so that we can handle the DSI PLL's and such. And we need to setup the rules above somewhere, and perhaps that code needs IDs to set it up. I'm not sure. And, well, if we don't have IDs, the rules above need to be some kind of lists of pointers. Perhaps having a bit mask is just simpler, as we'll never have too many output instances. So I'm not really against having the enum. It just would've been neat to have the output type and instance number encoded into this enum, so that it'd be easy to extract either one. But that kinda ruins the possibility to use it in a bit mask. > > So who could/should make the decision which mgr to use... Well, I don't > > know on what basis the decision can be made, but I still think omapdss > > can't make good decisions on that, so probably the whole "chain" should > > be configured in the omapfb/omapdrm level. > > Which manager to chose could be simply picking up the next free manager > which can connect to that output. omapfb/drm can take care of that. That's a good default implementation, but not quite perfect. If there are two displays, often the other display (well, output) can only be connected to one particular manager, whereas the other one could use two managers. And if both try to use the same manager, especially if the one with two options is selected first, the other one is left without a mgr. Tomi
On Fri, 2012-08-31 at 17:45 +0300, Tomi Valkeinen wrote: > So I'm not really against having the enum. It just would've been neat to > have the output type and instance number encoded into this enum, so that > it'd be easy to extract either one. But that kinda ruins the possibility > to use it in a bit mask. Hmm, this is just a non-finished idea (and it's late Friday... =) that came to my mind: I had a brief look at TRMs for different OMAPs (omap4/5/6), and it looks like the maximum output options for one manager is 4 (LCD2 on omap4460). Usually there are just 2 or 3. So, we could trust that the above holds and do this so that we use a u32 field to hold four 8 bit output options. In 8 bits we can combine two values from 0 to 15. 15 should be more than enough for output display types and output instances. Then with helpers like these: /* second DSI instance */ #define DSI1 ((1 << 4) | DISPLAY_TYPE_DSI) /* first DPI instance */ #define DPI0 ((0 << 4) | DISPLAY_TYPE_DPI) an example output options for a manager that can be connected to DSI1 and DPI0 could be: (DSI1 << 8) | (DPI0) This could be parsed with a for loop, going though the 4 bytes of the u32 value. This would allow us to avoid managing a real list, and we could extract the instance and the type from the value. Of course we could also extend the field to u64 to hold 8 outputs if need be. Whether this would be worth it compared to simple bitmask, I'm not sure =). Depends how much we need to extract the ID and type. Tomi
On Friday 31 August 2012 08:38 PM, Tomi Valkeinen wrote: > On Fri, 2012-08-31 at 17:45 +0300, Tomi Valkeinen wrote: > >> So I'm not really against having the enum. It just would've been neat to >> have the output type and instance number encoded into this enum, so that >> it'd be easy to extract either one. But that kinda ruins the possibility >> to use it in a bit mask. > > Hmm, this is just a non-finished idea (and it's late Friday... =) that > came to my mind: > > I had a brief look at TRMs for different OMAPs (omap4/5/6), and it looks > like the maximum output options for one manager is 4 (LCD2 on omap4460). > Usually there are just 2 or 3. > > So, we could trust that the above holds and do this so that we use a u32 > field to hold four 8 bit output options. In 8 bits we can combine two > values from 0 to 15. 15 should be more than enough for output display > types and output instances. > > Then with helpers like these: > > /* second DSI instance */ > #define DSI1 ((1 << 4) | DISPLAY_TYPE_DSI) > /* first DPI instance */ > #define DPI0 ((0 << 4) | DISPLAY_TYPE_DPI) Okay, so DISPLAY_TYPE_DSI / DPI can't be a number left shifted any more(that would limit us to have only 4 types of output display types), like we have right now, they would need to be 0, 1, 2, 3 and so on. So with this approach, we could extract the instance number quickly, but we may not be able to check if the manager supports the output display type in constant time. > > an example output options for a manager that can be connected to DSI1 > and DPI0 could be: > > (DSI1 << 8) | (DPI0) > > This could be parsed with a for loop, going though the 4 bytes of the > u32 value. This would allow us to avoid managing a real list, and we > could extract the instance and the type from the value. Of course we > could also extend the field to u64 to hold 8 outputs if need be. Probably we could have a u64, and 2 bytes for each output, and a bit mask for both output instance and output display type. We could have display type enums as: DISPLAY_TYPE_DPI 1 << 5 DISPLAY_TYPE_DSI 1 << 6 DISPLAY_TYPE_VENC 1 << 7 DISPLAY_TYPE_HDMI 1 << 8 DISPLAY_TYPE_SDI 1 << 9 DISPLAY_TYPE_RFBI 1 << 10 ... > > Whether this would be worth it compared to simple bitmask, I'm not sure > =). Depends how much we need to extract the ID and type. Anyway, I don't think I'll try to implement this in this series. I'll stick to the output instance enums for now. 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 Mon, 2012-09-03 at 14:56 +0530, Archit Taneja wrote: > On Friday 31 August 2012 08:38 PM, Tomi Valkeinen wrote: > > On Fri, 2012-08-31 at 17:45 +0300, Tomi Valkeinen wrote: > > > >> So I'm not really against having the enum. It just would've been neat to > >> have the output type and instance number encoded into this enum, so that > >> it'd be easy to extract either one. But that kinda ruins the possibility > >> to use it in a bit mask. > > > > Hmm, this is just a non-finished idea (and it's late Friday... =) that > > came to my mind: > > > > I had a brief look at TRMs for different OMAPs (omap4/5/6), and it looks > > like the maximum output options for one manager is 4 (LCD2 on omap4460). > > Usually there are just 2 or 3. > > > > So, we could trust that the above holds and do this so that we use a u32 > > field to hold four 8 bit output options. In 8 bits we can combine two > > values from 0 to 15. 15 should be more than enough for output display > > types and output instances. > > > > Then with helpers like these: > > > > /* second DSI instance */ > > #define DSI1 ((1 << 4) | DISPLAY_TYPE_DSI) > > /* first DPI instance */ > > #define DPI0 ((0 << 4) | DISPLAY_TYPE_DPI) > > Okay, so DISPLAY_TYPE_DSI / DPI can't be a number left shifted any > more(that would limit us to have only 4 types of output display types), > like we have right now, they would need to be 0, 1, 2, 3 and so on. So > with this approach, we could extract the instance number quickly, but we > may not be able to check if the manager supports the output display type > in constant time. True, it wouldn't be constant. But it'd be checking between one to four bytes, so I think it's quite fast =). > > an example output options for a manager that can be connected to DSI1 > > and DPI0 could be: > > > > (DSI1 << 8) | (DPI0) > > > > This could be parsed with a for loop, going though the 4 bytes of the > > u32 value. This would allow us to avoid managing a real list, and we > > could extract the instance and the type from the value. Of course we > > could also extend the field to u64 to hold 8 outputs if need be. > > Probably we could have a u64, and 2 bytes for each output, and a bit > mask for both output instance and output display type. We could have > display type enums as: > > DISPLAY_TYPE_DPI 1 << 5 > DISPLAY_TYPE_DSI 1 << 6 > DISPLAY_TYPE_VENC 1 << 7 > DISPLAY_TYPE_HDMI 1 << 8 > DISPLAY_TYPE_SDI 1 << 9 > DISPLAY_TYPE_RFBI 1 << 10 > ... > > > > > Whether this would be worth it compared to simple bitmask, I'm not sure > > =). Depends how much we need to extract the ID and type. > > Anyway, I don't think I'll try to implement this in this series. I'll > stick to the output instance enums for now. Sure. And as I said, I'm not sure if it's worth it. We currently have multiple instances only for DSI, and only two instances there. And I think we need to handle the instance number only in a few places. It's not too much to do: if (type == DSI1) ... else if (type == DSI2) ... Tomi
diff --git a/drivers/video/omap2/dss/overlay.c b/drivers/video/omap2/dss/overlay.c index 952c6fa..07605f1 100644 --- a/drivers/video/omap2/dss/overlay.c +++ b/drivers/video/omap2/dss/overlay.c @@ -525,51 +525,67 @@ void dss_init_overlays(struct platform_device *pdev) void dss_recheck_connections(struct omap_dss_device *dssdev, bool force) { int i; - struct omap_overlay_manager *lcd_mgr; - struct omap_overlay_manager *tv_mgr; - struct omap_overlay_manager *lcd2_mgr = NULL; - struct omap_overlay_manager *lcd3_mgr = NULL; struct omap_overlay_manager *mgr = NULL; + struct omap_dss_output *out = NULL; + enum omap_dss_output_id id; + + switch (dssdev->type) { + case OMAP_DISPLAY_TYPE_DPI: + out = omap_dss_get_output(OMAP_DSS_OUTPUT_DPI); + break; + case OMAP_DISPLAY_TYPE_DBI: + out = omap_dss_get_output(OMAP_DSS_OUTPUT_DBI); + break; + case OMAP_DISPLAY_TYPE_SDI: + out = omap_dss_get_output(OMAP_DSS_OUTPUT_SDI); + break; + case OMAP_DISPLAY_TYPE_VENC: + out = omap_dss_get_output(OMAP_DSS_OUTPUT_VENC); + break; + case OMAP_DISPLAY_TYPE_HDMI: + out = omap_dss_get_output(OMAP_DSS_OUTPUT_HDMI); + break; + case OMAP_DISPLAY_TYPE_DSI: + id = dssdev->phy.dsi.module == 0 ? OMAP_DSS_OUTPUT_DSI1 : + OMAP_DSS_OUTPUT_DSI2; + out = omap_dss_get_output(id); + break; + default: + break; + } - lcd_mgr = omap_dss_get_overlay_manager(OMAP_DSS_CHANNEL_LCD); - tv_mgr = omap_dss_get_overlay_manager(OMAP_DSS_CHANNEL_DIGIT); - if (dss_has_feature(FEAT_MGR_LCD3)) - lcd3_mgr = omap_dss_get_overlay_manager(OMAP_DSS_CHANNEL_LCD3); - if (dss_has_feature(FEAT_MGR_LCD2)) - lcd2_mgr = omap_dss_get_overlay_manager(OMAP_DSS_CHANNEL_LCD2); - - if (dssdev->channel == OMAP_DSS_CHANNEL_LCD3) { - if (!lcd3_mgr->device || force) { - if (lcd3_mgr->device) - lcd3_mgr->unset_device(lcd3_mgr); - lcd3_mgr->set_device(lcd3_mgr, dssdev); - mgr = lcd3_mgr; - } - } else if (dssdev->channel == OMAP_DSS_CHANNEL_LCD2) { - if (!lcd2_mgr->device || force) { - if (lcd2_mgr->device) - lcd2_mgr->unset_device(lcd2_mgr); - lcd2_mgr->set_device(lcd2_mgr, dssdev); - mgr = lcd2_mgr; - } - } else if (dssdev->type != OMAP_DISPLAY_TYPE_VENC - && dssdev->type != OMAP_DISPLAY_TYPE_HDMI) { - if (!lcd_mgr->device || force) { - if (lcd_mgr->device) - lcd_mgr->unset_device(lcd_mgr); - lcd_mgr->set_device(lcd_mgr, dssdev); - mgr = lcd_mgr; - } + /* + * We don't want to touch board files and mention channel for VENC + * devices. Force the channel as DIGIT for HDMI and VENC devices + */ + if (dssdev->type == OMAP_DISPLAY_TYPE_VENC || + dssdev->type == OMAP_DISPLAY_TYPE_HDMI) + dssdev->channel = OMAP_DSS_CHANNEL_DIGIT; + + mgr = omap_dss_get_overlay_manager(dssdev->channel); + + if (!mgr || !out) { + DSSERR("Incorrect manager or output\n"); + return; } - if (dssdev->type == OMAP_DISPLAY_TYPE_VENC - || dssdev->type == OMAP_DISPLAY_TYPE_HDMI) { - if (!tv_mgr->device || force) { - if (tv_mgr->device) - tv_mgr->unset_device(tv_mgr); - tv_mgr->set_device(tv_mgr, dssdev); - mgr = tv_mgr; + if (!mgr->output || force) { + struct omap_dss_output *curr_out = mgr->output; + + if (curr_out) { + if (curr_out->device) + curr_out->unset_device(curr_out); + mgr->unset_output(mgr); } + out->set_device(out, dssdev); + mgr->set_output(mgr, out); + } else { + /* + * connect a floating output to the device even if the desired + * manager is in use + */ + if (!out->manager && !out->device) + out->set_device(out, dssdev); } if (mgr) {
Links between DSS entities are made in dss_recheck_connections when a new panel is probed. Rewrite the code in dss_recheck_connections to link managers to outputs, and outputs to devices. The fields in omap_dss_device struct gives information on which output and manager to connect to. The desired manager and output pointers are retrieved and prepared(existing outputs/devices unset, if default display)) to form the desired links. The output is linked to the device, and then the manager to the output. If a probed device's required manager isn't free, the required output is still connected to the device so that it's easier to use the panel in the future. Signed-off-by: Archit Taneja <archit@ti.com> --- drivers/video/omap2/dss/overlay.c | 96 +++++++++++++++++++++---------------- 1 file changed, 56 insertions(+), 40 deletions(-)