Re: [PATCH v1 2/3] drm/msm/dp: decoupling dp->id out of dp controller_id at scxxxx_dp_cfg table

[Abhinav Kumar <quic_abhinavk@quicinc.com>]

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