Re: [PATCH v1 2/3] drm/msm/dp: decoupling dp->id out of dp controller_id at scxxxx_dp_cfg table
From: Abhinav Kumar
Date: Fri Jun 24 2022 - 21:23:18 EST
On 6/24/2022 5:11 PM, Dmitry Baryshkov wrote:
On Sat, 25 Jun 2022 at 03:03, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote:
Hi Stephen / Dmitry
Let me try to explain the issue kuogee is trying to fix below:
On 6/24/2022 4:56 PM, Kuogee Hsieh wrote:
On 6/24/2022 4:45 PM, Stephen Boyd wrote:
Quoting Kuogee Hsieh (2022-06-24 16:30:59)
On 6/24/2022 4:12 PM, Stephen Boyd wrote:
Quoting Kuogee Hsieh (2022-06-24 15:53:45)
MSM_DP_CONTROLLER_1 need to match to the index = 1 of
sc7280_dp_cfg[] <== This is correct
The problem is sc7280_dp_cfg[] have two entries since eDP place at
index
of MSM_DP_CONTROLLER_1.
but .num_desc = 1 <== this said only have one entry at
sc7280_dp_cfg[]
table. Therefore eDP will never be found at for loop at
_dpu_kms_initialize_displayport().
Yes, but what else does the MSM_DP_CONTROLLER_1 need to match? Because
the intention of the previous commit was to make it so the order of
sc7280_dp_cfg couldn't be messed up and not match the
MSM_DP_CONTROLLER_1 value that lives in sc7280_intf[].
at _dpu_kms_initialize_displayport()
- info.h_tile_instance[0] = i; <== assign i to become dp
controller id, "i" is index of scxxxx_dp_cfg[]
This what I mean MSM_DP_CONTROLLER_1 need to match to index = 1 of
scxxxx_dp_cfg[].
it it is not match, then MSM_DP_CONTROLLER_1 with match to different
INTF.
I thought we matched the INTF instance by searching through
sc7280_intf[] for a matching MSM_DP_CONTROLLER_1 and then returning that
INTF number. See dpu_encoder_get_intf() and the caller.
yes, but the controller_id had been over written by dp->id.
u32 controller_id = disp_info->h_tile_instance[i];
See below code.
for (i = 0; i < disp_info->num_of_h_tiles && !ret; i++) {
/*
* Left-most tile is at index 0, content is
controller id
* h_tile_instance_ids[2] = {0, 1}; DSI0 = left, DSI1
= right
* h_tile_instance_ids[2] = {1, 0}; DSI1 = left, DSI0
= right
*/
u32 controller_id = disp_info->h_tile_instance[i];
<== kuogee assign dp->id to controller_id
if (disp_info->num_of_h_tiles > 1) {
if (i == 0)
phys_params.split_role =
ENC_ROLE_MASTER;
else
phys_params.split_role = ENC_ROLE_SLAVE;
} else {
phys_params.split_role = ENC_ROLE_SOLO;
}
DPU_DEBUG("h_tile_instance %d = %d, split_role %d\n",
i, controller_id,
phys_params.split_role);
phys_params.intf_idx =
dpu_encoder_get_intf(dpu_kms->catalog,
intf_type,
controller_id);
So let me try to explain this as this is what i understood from the
patch and how kuogee explained me.
The ordering of the array still matters here and thats what he is trying
to address with the second change.
So as per him, he tried to swap the order of entries like below and that
did not work and that is incorrect behavior because he still retained
the MSM_DP_CONTROLLER_x field for the table like below:
I'd like to understand why did he try to change the order in the first place.
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
b/drivers/gpu/drm/msm/dp/dp_display.c
index dcd80c8a794c..7816e82452ca 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -140,8 +140,8 @@ static const struct msm_dp_config sc7180_dp_cfg = {
static const struct msm_dp_config sc7280_dp_cfg = {
.descs = (const struct msm_dp_desc[]) {
- [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000,
.connector_type = DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_en = true },
[MSM_DP_CONTROLLER_1] = { .io_start = 0x0aea0000,
.connector_type = DRM_MODE_CONNECTOR_eDP, .wide_bus_en = true },
+ [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000,
.connector_type = DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_en = true },
},
.num_descs = 2,
};
The reason order is important is because in this function below, even
though it matches the address to find which one to use it loops through
the array and so the value of *id will change depending on which one is
located where.
static const struct msm_dp_desc *dp_display_get_desc(struct
platform_device *pdev,
unsigned int *id)
{
const struct msm_dp_config *cfg = of_device_get_match_data(&pdev->dev);
struct resource *res;
int i;
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
if (!res)
return NULL;
for (i = 0; i < cfg->num_descs; i++) {
if (cfg->descs[i].io_start == res->start) {
*id = i;
The id is set to the index of the corresponding DP instance in the
descs array, which is MSM_DP_CONTROLLER_n. Correct up to now.
Right, this is where I misunderstood his explanation.
Even if we swap the order, but retain the index correctly it will still
work today.
Hes not sure of the root-cause of why turning on the primary display
first fixes the issue.
I think till we root-cause that, lets put this on hold.
return &cfg->descs[i];
}
}
In dp_display_bind(), dp->id is used as the index of assigning the
dp_display,
priv->dp[dp->id] = &dp->dp_display;
dp->id earlier is set to the id returned by dp_display_get_desc.
So the priv->dp is now indexed by MSM_DP_CONTROLLER_n. Again, correct.
And now in _dpu_kms_initialize_displayport(), in the array this will
decide the value of info.h_tile_instance[0] which will be assigned to
just the index i.
i is iterated over priv->dp indices (MSM_DP_CONTROLLER_n, see above),
which means that that h_tile_instance[0] is now set to the
MSM_DP_CONTROLLER_n. Still correct.
info.h_tile_instance[0] is then used as the controller id to find from
the catalog table.
This sounds good.
So if this order is not retained it does not work.
Thats the issue he is trying to address to make the order of entries
irrelevant in the table in dp_display.c