Messages in this thread Patch in this message | | | Date | Tue, 24 Jan 2023 12:10:21 +0530 | Subject | Re: [PATCH v6 3/5] drm/tidss: Add support to configure OLDI mode for am625-dss. | From | Aradhya Bhatia <> |
| |
Hi Tomi,
Thanks for reviewing the patch series. I have implemented the most of your suggestions, but for the others, I needed to clarify things. I have made some comments there.
On 20-Dec-22 18:22, Tomi Valkeinen wrote: > Hi, > > On 19/11/2022 19:30, Aradhya Bhatia wrote: >> The newer version of DSS (AM625-DSS) has 2 OLDI TXes at its disposal. >> These can be configured to support the following modes: >> >> 1. OLDI_SINGLE_LINK_SINGLE_MODE >> Single Output over OLDI 0. >> +------+ +---------+ +-------+ >> | | | | | | >> | CRTC +------->+ ENCODER +----->| PANEL | >> | | | | | | >> +------+ +---------+ +-------+ >> >> 2. OLDI_SINGLE_LINK_CLONE_MODE >> Duplicate Output over OLDI 0 and 1. >> +------+ +---------+ +-------+ >> | | | | | | >> | CRTC +---+--->| ENCODER +----->| PANEL | >> | | | | | | | >> +------+ | +---------+ +-------+ >> | >> | +---------+ +-------+ >> | | | | | >> +--->| ENCODER +----->| PANEL | >> | | | | >> +---------+ +-------+ >> >> 3. OLDI_DUAL_LINK_MODE >> Combined Output over OLDI 0 and 1. >> +------+ +---------+ +-------+ >> | | | +----->| | >> | CRTC +------->+ ENCODER | | PANEL | >> | | | +----->| | >> +------+ +---------+ +-------+ >> >> Following the above pathways for different modes, 2 encoder/panel-bridge >> pipes get created for clone mode, and 1 pipe in cases of single link and >> dual link mode. >> >> Add support for confguring the OLDI modes using OF and LVDS DRM helper >> functions. >> >> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com> >> --- >> drivers/gpu/drm/tidss/tidss_dispc.c | 12 ++ >> drivers/gpu/drm/tidss/tidss_dispc.h | 9 ++ >> drivers/gpu/drm/tidss/tidss_drv.h | 3 + >> drivers/gpu/drm/tidss/tidss_encoder.c | 4 +- >> drivers/gpu/drm/tidss/tidss_encoder.h | 3 +- >> drivers/gpu/drm/tidss/tidss_kms.c | 188 +++++++++++++++++++++++--- >> 6 files changed, 198 insertions(+), 21 deletions(-) >> >> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c >> b/drivers/gpu/drm/tidss/tidss_dispc.c >> index dbc6a5b617ca..472226a83251 100644 >> --- a/drivers/gpu/drm/tidss/tidss_dispc.c >> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c >> @@ -365,6 +365,8 @@ struct dispc_device { >> struct dss_vp_data vp_data[TIDSS_MAX_VPS]; >> + enum dispc_oldi_modes oldi_mode; >> + >> u32 *fourccs; >> u32 num_fourccs; >> @@ -1967,6 +1969,16 @@ const u32 *dispc_plane_formats(struct >> dispc_device *dispc, unsigned int *len) >> return dispc->fourccs; >> } >> +int dispc_set_oldi_mode(struct dispc_device *dispc, >> + enum dispc_oldi_modes oldi_mode) >> +{ >> + WARN_ON(!dispc); > > This feels unnecessary. Is there even a theoretical case where we could > get dispc == NULL? > >> + >> + dispc->oldi_mode = oldi_mode; >> + >> + return 0; > > This function could as well be void function. > >> +} >> + >> static s32 pixinc(int pixels, u8 ps) >> { >> if (pixels == 1) >> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.h >> b/drivers/gpu/drm/tidss/tidss_dispc.h >> index 51db500590ee..e76a7599b544 100644 >> --- a/drivers/gpu/drm/tidss/tidss_dispc.h >> +++ b/drivers/gpu/drm/tidss/tidss_dispc.h >> @@ -64,6 +64,14 @@ enum dispc_dss_subrevision { >> DISPC_AM625, >> }; >> +enum dispc_oldi_modes { >> + OLDI_MODE_OFF, /* OLDI turned off / tied off in IP. */ >> + OLDI_SINGLE_LINK_SINGLE_MODE, /* Single Output over OLDI 0. */ >> + OLDI_SINGLE_LINK_CLONE_MODE, /* Duplicate Output over OLDI 0 and 1. */ >> + OLDI_DUAL_LINK_MODE, /* Combined Output over OLDI 0 and 1. */ >> + OLDI_MODE_UNSUPPORTED, /* Unsupported OLDI Mode */ >> +}; > > What is the difference with MODE_OFF and MODE_UNSUPPORTED? Is > MODE_UNSUPPORTED for cases where, e.g., the DT setup is wrong and the > driver should return an error? The code doesn't quite do that, it prints > an error but then continues.
Yes, OLDI_MODE_UNSUPPORTED is for the cases where DT setup is wrong. I have not exited in such a cases with an error, because then the driver will never have a chance to setup the 2nd pipeline (DPI) even if all the DT requirements are met.
> >> struct dispc_features { >> int min_pclk_khz; >> int max_pclk_khz[DISPC_VP_MAX_BUS_TYPE]; >> @@ -133,6 +141,7 @@ int dispc_plane_setup(struct dispc_device *dispc, u32 hw_plane, >> u32 hw_videoport); >> int dispc_plane_enable(struct dispc_device *dispc, u32 hw_plane, bool enable); >> const u32 *dispc_plane_formats(struct dispc_device *dispc, unsigned int *len); >> +int dispc_set_oldi_mode(struct dispc_device *dispc, enum dispc_oldi_modes oldi_mode); >> int dispc_init(struct tidss_device *tidss); >> void dispc_remove(struct tidss_device *tidss); >> diff --git a/drivers/gpu/drm/tidss/tidss_drv.h >> b/drivers/gpu/drm/tidss/tidss_drv.h >> index 0ce7ee5ccd5b..58892f065c16 100644 >> --- a/drivers/gpu/drm/tidss/tidss_drv.h >> +++ b/drivers/gpu/drm/tidss/tidss_drv.h >> @@ -13,6 +13,9 @@ >> #define TIDSS_MAX_PLANES 4 >> #define TIDSS_MAX_OUTPUT_PORTS 4 >> +/* For AM625-DSS with 2 OLDI TXes */ >> +#define TIDSS_MAX_BRIDGES_PER_PIPE 2 >> + >> typedef u32 dispc_irq_t; >> struct tidss_device { >> diff --git a/drivers/gpu/drm/tidss/tidss_encoder.c >> b/drivers/gpu/drm/tidss/tidss_encoder.c >> index e278a9c89476..141383ec4045 100644 >> --- a/drivers/gpu/drm/tidss/tidss_encoder.c >> +++ b/drivers/gpu/drm/tidss/tidss_encoder.c >> @@ -70,7 +70,8 @@ static const struct drm_encoder_funcs encoder_funcs = { >> }; >> struct drm_encoder *tidss_encoder_create(struct tidss_device *tidss, >> - u32 encoder_type, u32 possible_crtcs) >> + u32 encoder_type, u32 possible_crtcs, >> + u32 possible_clones) >> { >> struct drm_encoder *enc; >> int ret; >> @@ -80,6 +81,7 @@ struct drm_encoder *tidss_encoder_create(struct >> tidss_device *tidss, >> return ERR_PTR(-ENOMEM); >> enc->possible_crtcs = possible_crtcs; >> + enc->possible_clones = possible_clones; >> ret = drm_encoder_init(&tidss->ddev, enc, &encoder_funcs, >> encoder_type, NULL); >> diff --git a/drivers/gpu/drm/tidss/tidss_encoder.h >> b/drivers/gpu/drm/tidss/tidss_encoder.h >> index ace877c0e0fd..01c62ba3ef16 100644 >> --- a/drivers/gpu/drm/tidss/tidss_encoder.h >> +++ b/drivers/gpu/drm/tidss/tidss_encoder.h >> @@ -12,6 +12,7 @@ >> struct tidss_device; >> struct drm_encoder *tidss_encoder_create(struct tidss_device *tidss, >> - u32 encoder_type, u32 possible_crtcs); >> + u32 encoder_type, u32 possible_crtcs, >> + u32 possible_clones); >> #endif >> diff --git a/drivers/gpu/drm/tidss/tidss_kms.c >> b/drivers/gpu/drm/tidss/tidss_kms.c >> index fc9c4eefd31d..8ae321f02197 100644 >> --- a/drivers/gpu/drm/tidss/tidss_kms.c >> +++ b/drivers/gpu/drm/tidss/tidss_kms.c >> @@ -106,30 +106,115 @@ static const struct drm_mode_config_funcs >> mode_config_funcs = { >> .atomic_commit = drm_atomic_helper_commit, >> }; >> +static enum dispc_oldi_modes tidss_get_oldi_mode(struct device_node >> *oldi0_port, >> + struct device_node *oldi1_port) >> +{ >> + int pixel_order; >> + >> + if (!(oldi0_port || oldi1_port)) { >> + /* Keep OLDI TXes off if neither OLDI port is present. */ >> + return OLDI_MODE_OFF; >> + } else if (oldi0_port && !oldi1_port) { >> + /* >> + * OLDI0 port found, but not OLDI1 port. Setting single >> + * link, single mode output. >> + */ >> + return OLDI_SINGLE_LINK_SINGLE_MODE; >> + } else if (!oldi0_port && oldi1_port) { >> + /* >> + * The 2nd OLDI TX cannot be operated alone. This use case is >> + * not supported in the HW. Since the pins for OLDIs 0 and 1 are >> + * separate, one could theoretically set a clone mode over OLDIs >> + * 0 and 1 and just simply not use the OLDI 0. This is a hacky >> + * way to enable only OLDI TX 1 and hence is not officially >> + * supported. >> + */ >> + return OLDI_MODE_UNSUPPORTED; > > If OLDI_MODE_UNSUPPORTED is supposed to result in an error, maybe you > could print the error here (and possibly in the default case below), and > then, in the caller, just return with an error code. > >> + } >> + >> + /* >> + * OLDI Ports found for both the OLDI TXes. The DSS is to be configured >> + * in either Dual Link or Clone Mode. >> + */ >> + pixel_order = drm_of_lvds_get_dual_link_pixel_order(oldi0_port, >> + oldi1_port); >> + switch (pixel_order) { >> + case -EINVAL: >> + /* >> + * The dual link properties were not found in at least one of >> + * the sink nodes. Since 2 OLDI ports are present in the DT, it >> + * can be safely assumed that the required configuration is >> + * Clone Mode. >> + */ >> + return OLDI_SINGLE_LINK_CLONE_MODE; >> + >> + case DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS: >> + /* >> + * Note that the OLDI TX 0 transmits the odd set of pixels while >> + * the OLDI TX 1 transmits the even set. This is a fixed >> + * configuration in the IP and an cannot be change vis SW. These >> + * properties have been used to merely identify if a Dual Link >> + * configuration is required. Swapping this property in the panel >> + * port DT nodes will not make any difference. >> + */ >> + pr_warn("EVEN-ODD config for dual-link sinks is not supported in HW. Switching to ODD-EVEN.\n"); > > Please use dev_warn() instead, so that it will be clear where the print > comes from. > > In any case, isn't this an error? Do you really want to accept the wrong > pixel order? > >> + fallthrough; >> + >> + case DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS: >> + return OLDI_DUAL_LINK_MODE; >> + >> + default: >> + return OLDI_MODE_OFF; > > When do we get here? Shouldn't this be OLDI_MODE_UNSUPPORTED? > >> + } >> +} >> + >> static int tidss_dispc_modeset_init(struct tidss_device *tidss) >> { >> struct device *dev = tidss->dev; >> unsigned int fourccs_len; >> const u32 *fourccs = dispc_plane_formats(tidss->dispc, >> &fourccs_len); >> - unsigned int i; >> + unsigned int i, j; >> struct pipe { >> u32 hw_videoport; >> - struct drm_bridge *bridge; >> + struct drm_bridge *bridge[TIDSS_MAX_BRIDGES_PER_PIPE]; >> u32 enc_type; >> + u32 num_bridges; >> }; >> const struct dispc_features *feat = tidss->feat; >> - u32 max_vps = feat->num_vps; >> + u32 output_ports = feat->num_output_ports; >> u32 max_planes = feat->num_planes; >> - struct pipe pipes[TIDSS_MAX_VPS]; >> + struct pipe pipes[TIDSS_MAX_VPS] = {0}; >> + >> u32 num_pipes = 0; >> u32 crtc_mask; >> + u32 portnum_oldi0 = 0, portnum_oldi1 = 2; > > These two are bit of hacks. First, they should be const, or maybe > defines. If they're const, they can be inside the block below. > > And they're very much tied to the HW in question, so just having > hardcoded values here inside a function without any mention of the > situation is not good. > > Doing this in a generic and future proof manner is... challenging. So I > think using the hardcoded port numbers is fine. But they are only ok for > the two AM6xx SoCs we have now so the 'oldi_supported' is not very good > fit. In fact, it might be good to drop 'oldi_supported' altogether, and > just check for the SoC versions instead, as (with a quick glance), all > the 'oldi_supported' checks are really SoC specific. > I will be make these portnum variables const as you suggested and moving these as well as get-node and put-node function calls to the get_oldi_mode function to keep things clear.
However, I believe the 'oldi_supported' variable should still be kept (after renaming it to has_oldi as per your suggestion in the previous patch) because having this variable will help distinguish from the cases where an SoC *can* support OLDI output but its output by-passes the OLDI TXes and a DPI output is expected.
> This also again brings up a thing that rubs me the wrong way: the new > OLDI port is port 2. I really think that on AM62x, the two OLDI ports > should be 0 and 1, and the DPI should be 2. Would we need a new > dt-binding doc for that, or could it still be described in the same doc? > Would that change cause changes elsewhere in the dss driver? > >> + enum dispc_oldi_modes oldi_mode = OLDI_MODE_OFF; >> + u32 num_oldi = 0; >> + u32 oldi_pipe_index = 0; >> + u32 num_encoders = 0; >> + >> + if (feat->oldi_supported) { >> + struct device_node *oldi0_port, *oldi1_port; >> + >> + oldi0_port = of_graph_get_port_by_id(dev->of_node, >> + portnum_oldi0); >> + oldi1_port = of_graph_get_port_by_id(dev->of_node, >> + portnum_oldi1); >> + >> + oldi_mode = tidss_get_oldi_mode(oldi0_port, oldi1_port); >> + >> + of_node_put(oldi0_port); >> + of_node_put(oldi1_port); >> + >> + dispc_set_oldi_mode(tidss->dispc, oldi_mode); >> + } >> /* first find all the connected panels & bridges */ >> - for (i = 0; i < max_vps; i++) { >> + for (i = 0; i < output_ports; i++) { >> struct drm_panel *panel; >> struct drm_bridge *bridge; >> u32 enc_type = DRM_MODE_ENCODER_NONE; >> @@ -145,16 +230,24 @@ static int tidss_dispc_modeset_init(struct >> tidss_device *tidss) >> return ret; >> } >> + if (feat->output_port_bus_type[i] == DISPC_VP_OLDI && >> + oldi_mode == OLDI_MODE_UNSUPPORTED) { >> + dev_err(dev, >> + "Single Mode over OLDI 1 is not supported in HW. Keeping OLDI off.\n"); >> + continue; > > Should we error out here? > >> + } >> + >> if (panel) { >> u32 conn_type; >> dev_dbg(dev, "Setting up panel for port %d\n", i); >> - switch (feat->vp_bus_type[i]) { >> + switch (feat->output_port_bus_type[i]) { >> case DISPC_VP_OLDI: >> enc_type = DRM_MODE_ENCODER_LVDS; >> conn_type = DRM_MODE_CONNECTOR_LVDS; >> break; >> + >> case DISPC_VP_DPI: >> enc_type = DRM_MODE_ENCODER_DPI; >> conn_type = DRM_MODE_CONNECTOR_DPI; >> @@ -172,6 +265,17 @@ static int tidss_dispc_modeset_init(struct >> tidss_device *tidss) >> return -EINVAL; >> } >> + /* >> + * If the 2nd OLDI port is discovered connected to a panel >> + * which is not to be connected in the Clone Mode then a >> + * bridge is not required because the detected port is the >> + * 2nd port for the previously connected panel. >> + */ >> + if (feat->output_port_bus_type[i] == DISPC_VP_OLDI && >> + oldi_mode != OLDI_SINGLE_LINK_CLONE_MODE && > > Hmm, shouldn't this be oldi_mode == OLDI_DUAL_LINK_MODE? Or rather, Yes. I will make the change...
> shouldn't we test here if this is the second oldi display, and if so > which oldi-mode are we using. If dual-link, break. If clone, continue. > Otherwise, error. but, if I implement the oldi-mode-find logic over here, that would be limited to panels and will skip out the bridges. And the mode-find should really be only done once.
That said, I see that this can get a little confusing, So, while keeping the mode-find logic separate, I have organized the other OLDI specific things separately in the next patch.
> >> + num_oldi) >> + break; >> + >> bridge = devm_drm_panel_bridge_add(dev, panel); >> if (IS_ERR(bridge)) { >> dev_err(dev, >> @@ -181,10 +285,47 @@ static int tidss_dispc_modeset_init(struct >> tidss_device *tidss) >> } >> } >> - pipes[num_pipes].hw_videoport = i; >> - pipes[num_pipes].bridge = bridge; >> - pipes[num_pipes].enc_type = enc_type; >> - num_pipes++; >> + if (feat->output_port_bus_type[i] == DISPC_VP_OLDI) { >> + if (++num_oldi == 1) { >> + /* Setting up pipe parameters when 1st OLDI port is detected */ >> + >> + pipes[num_pipes].hw_videoport = i; >> + pipes[num_pipes].enc_type = enc_type; >> + >> + /* >> + * Saving the pipe index in case its required for >> + * 2nd OLDI Port. >> + */ >> + oldi_pipe_index = num_pipes; >> + >> + /* >> + * No additional pipe is required for the 2nd OLDI >> + * port, because the 2nd Encoder -> Bridge connection >> + * is the part of the first OLDI Port pipe. >> + * >> + * num_pipes will only be incremented when the first >> + * OLDI port is discovered. >> + */ >> + num_pipes++; >> + } >> + >> + /* >> + * Bridge is required to be added only if the detected >> + * port is the first OLDI port or a subsequent port in >> + * Clone Mode. >> + */ >> + if (oldi_mode == OLDI_SINGLE_LINK_CLONE_MODE || >> + num_oldi == 1) { >> + pipes[oldi_pipe_index].bridge[num_oldi - 1] = bridge; >> + pipes[oldi_pipe_index].num_bridges++; >> + } >> + } else { >> + pipes[num_pipes].hw_videoport = i; >> + pipes[num_pipes].bridge[0] = bridge; >> + pipes[num_pipes].num_bridges++; >> + pipes[num_pipes].enc_type = enc_type; >> + num_pipes++; >> + } >> } >> /* all planes can be on any crtc */ >> @@ -196,6 +337,7 @@ static int tidss_dispc_modeset_init(struct >> tidss_device *tidss) >> struct tidss_plane *tplane; >> struct tidss_crtc *tcrtc; >> struct drm_encoder *enc; >> + u32 possible_clones = 0; >> u32 hw_plane_id = feat->vid_order[tidss->num_planes]; >> int ret; >> @@ -218,16 +360,24 @@ static int tidss_dispc_modeset_init(struct >> tidss_device *tidss) >> tidss->crtcs[tidss->num_crtcs++] = &tcrtc->crtc; >> - enc = tidss_encoder_create(tidss, pipes[i].enc_type, >> - 1 << tcrtc->crtc.index); >> - if (IS_ERR(enc)) { >> - dev_err(tidss->dev, "encoder create failed\n"); >> - return PTR_ERR(enc); >> - } >> + for (j = 0; j < pipes[i].num_bridges; j++) { >> + if (pipes[i].num_bridges > 1) >> + possible_clones = (((1 << pipes[i].num_bridges) - 1) >> + << num_encoders); > > I think the above possible_clones assignment can be outside the for loop. > >> + >> + enc = tidss_encoder_create(tidss, pipes[i].enc_type, >> + 1 << tcrtc->crtc.index, >> + possible_clones); >> + if (IS_ERR(enc)) { >> + dev_err(tidss->dev, "encoder create failed\n"); >> + return PTR_ERR(enc); >> + } >> - ret = drm_bridge_attach(enc, pipes[i].bridge, NULL, 0); >> - if (ret) >> - return ret; >> + ret = drm_bridge_attach(enc, pipes[i].bridge[j], NULL, 0); >> + if (ret) >> + return ret; >> + } >> + num_encoders += pipes[i].num_bridges; >> } >> /* create overlay planes of the leftover planes */
Regards Aradhya
| |