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@xxxxxx>
---
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
