Re: [RFC PATCH v5 4/6] drm/tidss: Add support to configure OLDI mode for am625-dss.
From: Tomi Valkeinen
Date: Mon Oct 24 2022 - 03:17:23 EST
On 18/10/2022 10:00, Aradhya Bhatia wrote:
Hi Tomi
Thank you for the comprehensive feedback across all the patches. I am
working on them.
I do have some concerns which I have talked about, below.
On 12-Oct-22 17:53, Tomi Valkeinen wrote:
On 28/09/2022 20:52, 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 |
| | | | | |
+------+ +---------+ +-------+
Can you have single link on OLDI 1 (OLDI 0 off)? I don't know if that
make sense on this platform, but if the pins for OLDI 0 and 1 are
different, there might be a reason on some cases for that.
HW does not support a case where single link is enabled over OLDI 1 with
OLDI 0 off, even though the pins are different.
One could still put 2 panel nodes in DT to set OLDI in a Clone Mode and
simply not use OLDI 0 pins, but I dont think that is a valid case that
should be supported.
2. OLDI_SINGLE_LINK_CLONE_MODE
Duplicate Output over OLDI 0 and 1.
+------+ +---------+ +-------+
| | | | | |
| CRTC +---+--->| ENCODER +----->| PANEL |
| | | | | | |
+------+ | +---------+ +-------+
|
I think you've got a tab in the line above, but otherwise use spaces.
| +---------+ +-------+
| | | | |
+--->| 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 confgure the OLDI modes using of and lvds DRM helper
"configuring"
functions.
Signed-off-by: Aradhya Bhatia <a-bhatia1@xxxxxx>
---
drivers/gpu/drm/tidss/tidss_dispc.c | 11 +++
drivers/gpu/drm/tidss/tidss_dispc.h | 8 ++
drivers/gpu/drm/tidss/tidss_drv.h | 3 +
drivers/gpu/drm/tidss/tidss_kms.c | 146 +++++++++++++++++++++++-----
4 files changed, 145 insertions(+), 23 deletions(-)
diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c
b/drivers/gpu/drm/tidss/tidss_dispc.c
index 34f0da4bb3e3..88008ad39b55 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.c
+++ b/drivers/gpu/drm/tidss/tidss_dispc.c
@@ -354,6 +354,8 @@ struct dispc_device {
bool is_enabled;
+ enum dispc_oldi_modes oldi_mode;
+
struct dss_vp_data vp_data[TIDSS_MAX_PORTS];
u32 *fourccs;
@@ -1958,6 +1960,15 @@ const u32 *dispc_plane_formats(struct
dispc_device *dispc, unsigned int *len)
return dispc->fourccs;
}
+int dispc_configure_oldi_mode(struct dispc_device *dispc,
+ enum dispc_oldi_modes oldi_mode)
+{
+ WARN_ON(!dispc);
+
+ dispc->oldi_mode = oldi_mode;
+ return 0;
+}
I think "configure" means more than just storing the value. Maybe
dispc_set_oldi_mode(). And an empty line above the return.
+
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 b66418e583ee..45cce1054832 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.h
+++ b/drivers/gpu/drm/tidss/tidss_dispc.h
@@ -64,6 +64,13 @@ 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. */
+};
+
struct dispc_features {
int min_pclk_khz;
int max_pclk_khz[DISPC_VP_MAX_BUS_TYPE];
@@ -131,6 +138,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_configure_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 d7f27b0b0315..2252ba0222ca 100644
--- a/drivers/gpu/drm/tidss/tidss_drv.h
+++ b/drivers/gpu/drm/tidss/tidss_drv.h
@@ -12,6 +12,9 @@
#define TIDSS_MAX_PORTS 4
#define TIDSS_MAX_PLANES 4
+/* For AM625-DSS with 2 OLDI TXes */
+#define TIDSS_MAX_BRIDGE_PER_PIPE 2
"BRIDGES"?
+
typedef u32 dispc_irq_t;
struct tidss_device {
diff --git a/drivers/gpu/drm/tidss/tidss_kms.c
b/drivers/gpu/drm/tidss/tidss_kms.c
index 666e527a0acf..73afe390f36d 100644
--- a/drivers/gpu/drm/tidss/tidss_kms.c
+++ b/drivers/gpu/drm/tidss/tidss_kms.c
@@ -107,32 +107,84 @@ static const struct drm_mode_config_funcs
mode_config_funcs = {
.atomic_commit = drm_atomic_helper_commit,
};
+static int tidss_get_oldi_mode(struct tidss_device *tidss)
Return enum dispc_oldi_modes, not int.
+{
+ int pixel_order;
+ struct device_node *dss_ports, *oldi0_port, *oldi1_port;
+
+ dss_ports = of_get_next_child(tidss->dev->of_node, NULL);
Hmm you get the next child and hope that it's the ports node?
In any case, I think you can call of_graph_get_port_by_id() with the
tidss->dev->of_node and it'll do the right thing.
I think this will only work if the child of dss node is just "ports",
but we've been using "dss_ports" as the child.
However, you are right. I shouldn't expect the first child to be
dss_ports. I will use the "of_get_child_by_name" helper to get the
dss_ports node.
I don't think you need to. of_graph_get_port_by_id() should work fine
with the dss node.
+ oldi0_port = of_graph_get_port_by_id(dss_ports, 0);
+ oldi1_port = of_graph_get_port_by_id(dss_ports, 2);
I think you need to of_put these at some point.
+ if (!(oldi0_port && oldi1_port))
+ return OLDI_SINGLE_LINK_SINGLE_MODE;
This one matches also for !oldi0 && oldi1. If oldi1 cannot be used in
single-link mode, the above should take it into account.
Right. I will print a warning if somebody's trying to use (!oldi0 &&
oldi1) but since its a single link requirement, I will still set the
OLDI for single link single mode.
+
+ /*
+ * 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:
+ case DRM_LVDS_DUAL_LINK_ODD_EVEN_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.
+ */
But if they are in the wrong order, shouldn't we fail or at least give
a warning?
>> + return OLDI_DUAL_LINK_MODE;
+
+ default:
+ return OLDI_MODE_OFF;
+ }
+}
+
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_BRIDGE_PER_PIPE];
u32 enc_type;
+ u32 num_bridges;
};
const struct dispc_features *feat = tidss->feat;
- u32 max_vps = feat->num_vps;
+ u32 max_ports = feat->num_max_ports;
u32 max_planes = feat->num_planes;
struct pipe pipes[TIDSS_MAX_PORTS];
u32 num_pipes = 0;
+ u32 pipe_number = 0;
u32 crtc_mask;
+ u32 num_oldi = 0;
+ u32 oldi0_port = 0;
+ u32 hw_vp = 0;
+ enum dispc_oldi_modes oldi_mode;
/* first find all the connected panels & bridges */
- for (i = 0; i < max_vps; i++) {
+ for (i = 0; i < max_ports; i++) {
struct drm_panel *panel;
struct drm_bridge *bridge;
+ bool bridge_req = true;
u32 enc_type = DRM_MODE_ENCODER_NONE;
int ret;
@@ -146,6 +198,11 @@ static int tidss_dispc_modeset_init(struct
tidss_device *tidss)
return ret;
}
+ /* default number of bridges required for a panel/bridge*/
+ pipe_number = num_pipes;
+ pipes[pipe_number].num_bridges = 1;
+ hw_vp = i;
+
if (panel) {
u32 conn_type;
@@ -155,7 +212,43 @@ static int tidss_dispc_modeset_init(struct
tidss_device *tidss)
case DISPC_VP_OLDI:
enc_type = DRM_MODE_ENCODER_LVDS;
conn_type = DRM_MODE_CONNECTOR_LVDS;
+
+ /*
+ * A single DSS controller cannot support 2
+ * independent displays. If 2nd node is detected,
+ * it is for Dual Link Mode or Clone Mode.
+ *
+ * A new pipe instance is not required.
+ */
+ if (++num_oldi == 2) {
+ pipe_number = oldi0_port;
+ hw_vp = i;
+
+ /* 2nd OLDI DT node detected. Get its mode */
+ oldi_mode = tidss_get_oldi_mode(tidss);
+ bridge_req = false;
+
+ /*
+ * A separate panel bridge will only be
+ * required if 2 panels are connected for
+ * the OLDI Clone Mode.
+ */
+ if (oldi_mode == OLDI_SINGLE_LINK_CLONE_MODE) {
+ bridge_req = true;
+ (pipes[pipe_number].num_bridges)++;
+ }
+ } else {
+ /*
+ * First OLDI DT node detected. Save it
+ * in case there is another node for Dual
+ * Link Mode or Clone Mode.
+ */
+ oldi0_port = i;
+ oldi_mode = OLDI_SINGLE_LINK_SINGLE_MODE;
+ }
+ dispc_configure_oldi_mode(tidss->dispc, oldi_mode);
break;
+
case DISPC_VP_DPI:
enc_type = DRM_MODE_ENCODER_DPI;
conn_type = DRM_MODE_CONNECTOR_DPI;
@@ -173,19 +266,23 @@ static int tidss_dispc_modeset_init(struct
tidss_device *tidss)
return -EINVAL;
}
- bridge = devm_drm_panel_bridge_add(dev, panel);
- if (IS_ERR(bridge)) {
- dev_err(dev,
- "failed to set up panel bridge for port %d\n",
- i);
- return PTR_ERR(bridge);
+ if (bridge_req) {
+ bridge = devm_drm_panel_bridge_add(dev, panel);
+ if (IS_ERR(bridge)) {
+ dev_err(dev,
+ "failed to set up panel bridge for port %d\n",
+ i);
+ return PTR_ERR(bridge);
+ }
}
}
- pipes[num_pipes].hw_videoport = i;
- pipes[num_pipes].bridge = bridge;
- pipes[num_pipes].enc_type = enc_type;
- num_pipes++;
+ if (bridge_req) {
+ pipes[pipe_number].hw_videoport = hw_vp;
+ pipes[pipe_number].bridge[pipes[pipe_number].num_bridges
- 1] = bridge;
+ pipes[pipe_number].enc_type = enc_type;
+ num_pipes++;
+ }
I need to look at this with better time. But I started to wonder,
would it be clearer to first figure out the oldi setup before the
loop, rather than figuring it out inside the loop. I'm not sure if it
would help much, though.
I had not thought about taking this approach, but it might actually be
better.
These patches, at the moment, do not support a case where a clone mode
or dual link mode is used on a bridge instead of a panel. My edits
inside the loop are panel dependent. If we do have oldi setup
information prior to the beginning of the loop, the panel dependency can
be removed and some commond code can be written to support an additional
encoder - bridge connection should it be required.
Let me know what you think!
If this apparch is better indeed, I will make these changes before
sending out the next revision.
I'll say if it's better when I see the code =). But generally speaking,
I think it's often better to first figure out what is needed and only
after that do the actual work. Especially in probe-time code where it's
not a big deal if you iterate over the ports multiple times, instead of
doing all in a single loop.
Tomi