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

[Stephen Boyd]

Quoting Abhinav Kumar (2022-06-24 17:03:37)
> 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.

The order of the array should not matter. That's the problem.

>
> 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)

Thanks! We should fix this function to not overwrite the 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

The code seems to be brittle. This sort of explanation would have been
helpful to have in the commit text.