| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 24 Jun 2022 17:03:37 -0700
From: Abhinav Kumar <quic_abhinavk@...cinc.com>
To: Kuogee Hsieh <quic_khsieh@...cinc.com>,
Stephen Boyd <swboyd@...omium.org>, <agross@...nel.org>,
<airlied@...ux.ie>, <bjorn.andersson@...aro.org>,
<daniel@...ll.ch>, <dianders@...omium.org>,
<dmitry.baryshkov@...aro.org>, <dri-devel@...ts.freedesktop.org>,
<robdclark@...il.com>, <sean@...rly.run>, <vkoul@...nel.org>
CC: <quic_aravindh@...cinc.com>, <quic_sbillaka@...cinc.com>,
<freedreno@...ts.freedesktop.org>, <linux-arm-msm@...r.kernel.org>,
<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v1 2/3] drm/msm/dp: decoupling dp->id out of dp
controller_id at scxxxx_dp_cfg table
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:
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;
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;
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.
info.h_tile_instance[0] is then used as the controller id to find from
the catalog table.
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
Powered by blists - more mailing lists