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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux