[RFC PATCH v5 4/6] drm/tidss: Add support to configure OLDI mode for am625-dss.
Tomi Valkeinen
tomi.valkeinen at ideasonboard.com
Mon Oct 24 07:17:03 UTC 2022
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 at ti.com>
>>> ---
>>> 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
More information about the dri-devel
mailing list