Messages in this thread | | | Date | Fri, 24 Jun 2022 17:19:15 -0700 | Subject | Re: [PATCH v1 2/3] drm/msm/dp: decoupling dp->id out of dp controller_id at scxxxx_dp_cfg table | From | Kuogee Hsieh <> |
| |
On 6/24/2022 5:11 PM, Dmitry Baryshkov wrote: > On Sat, 25 Jun 2022 at 03:03, Abhinav Kumar <quic_abhinavk@quicinc.com> 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. > >> 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. How can I have eDP call dpu_encoder_init() before DP calls with _dpu_kms_initialize_displayport()? >> 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 > >
| |