| Message ID | 1656090912-18074-3-git-send-email-quic_khsieh@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | fix primary corruption issue | expand |
Quoting Kuogee Hsieh (2022-06-24 10:15:11) > Current the index (dp->id) of DP descriptor table (scxxxx_dp_cfg[]) are tightly > coupled with DP controller_id. This means DP use controller id 0 must be placed > at first entry of DP descriptor table (scxxxx_dp_cfg[]). Otherwise the internal > INTF will mismatch controller_id. This will cause controller kickoff wrong > interface timing engine and cause dpu_encoder_phys_vid_wait_for_commit_done > vblank timeout error. > > This patch add controller_id field into struct msm_dp_desc to break the tightly > coupled relationship between index (dp->id) of DP descriptor table with DP > controller_id. Please no. This reverts the intention of commit bb3de286d992 ("drm/msm/dp: Support up to 3 DP controllers") A new enum is introduced to document the connection between the instances referenced in the dpu_intf_cfg array and the controllers in the DP driver and sc7180 is updated. It sounds like the intent of that commit failed to make a strong enough connection. Now it needs to match the INTF number as well? I can't really figure out what is actually wrong, because this patch undoes that intentional tight coupling. Is the next patch the important part that flips the order of the two interfaces?
Quoting Kuogee Hsieh (2022-06-24 14:49:57) > > On 6/24/2022 2:40 PM, Stephen Boyd wrote: > > Quoting Kuogee Hsieh (2022-06-24 14:17:50) > >> On 6/24/2022 1:00 PM, Stephen Boyd wrote: > >>> Quoting Kuogee Hsieh (2022-06-24 10:15:11) > >>>> Current the index (dp->id) of DP descriptor table (scxxxx_dp_cfg[]) are tightly > >>>> coupled with DP controller_id. This means DP use controller id 0 must be placed > >>>> at first entry of DP descriptor table (scxxxx_dp_cfg[]). Otherwise the internal > >>>> INTF will mismatch controller_id. This will cause controller kickoff wrong > >>>> interface timing engine and cause dpu_encoder_phys_vid_wait_for_commit_done > >>>> vblank timeout error. > >>>> > >>>> This patch add controller_id field into struct msm_dp_desc to break the tightly > >>>> coupled relationship between index (dp->id) of DP descriptor table with DP > >>>> controller_id. > >>> Please no. This reverts the intention of commit bb3de286d992 > >>> ("drm/msm/dp: Support up to 3 DP controllers") > >>> > >>> A new enum is introduced to document the connection between the > >>> instances referenced in the dpu_intf_cfg array and the controllers in > >>> the DP driver and sc7180 is updated. > >>> > >>> It sounds like the intent of that commit failed to make a strong enough > >>> connection. Now it needs to match the INTF number as well? I can't > >>> really figure out what is actually wrong, because this patch undoes that > >>> intentional tight coupling. Is the next patch the important part that > >>> flips the order of the two interfaces? > >> The commit bb3de286d992have two problems, > >> > >> 1) The below sc7280_dp_cfg will not work, if eDP use > >> MSM_DP_CONTROLLER_2 instead of MSM_DP_CONTROLLER_1 > > Why would we use three indices for an soc that only has two indices > > possible? This is not a real problem? > > I do not what will happen at future, it may have more dp controller use > late. > > at current soc, below table has only one eDP will not work either. > > static const struct msm_dp_config sc7280_dp_cfg = { > .descs = (const struct msm_dp_desc[]) { > [MSM_DP_CONTROLLER_1] = { .io_start = 0x0aea0000, > .connector_type = DRM_MODE_CONNECTOR_eDP, .wide_bus_en = true }, > > .num_descs = 1, So the MSM_DP_CONTROLLER_* number needs to match what exactly? > > > > >> since it have num_descs =2 but eDP is at index 2 (CONTROLLER_2) which > >> never be reached. > >> > >> static const struct msm_dp_config sc7280_dp_cfg = { > >> .descs = (const struct msm_dp_desc[]) { > >> [MSM_DP_CONTROLLER_2] = { .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, > >> }; > >> > >> 2) DP always has index of 0 (dp->id = 0) and the first one to call > >> msm_dp_modeset_init(). This make DP always place at head of bridge chain. > > What does this mean? Are you talking about the list of bridges in drm > > core, i.e. 'bridge_list'? > yes, I changed the drm_bridge_add() API and that doesn't make any difference. The corruption is still seen. That would imply it is not the order of the list of bridges. ---8<--- diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index e275b4ca344b..e3518101b65e 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -165,7 +165,7 @@ void drm_bridge_add(struct drm_bridge *bridge) mutex_init(&bridge->hpd_mutex); mutex_lock(&bridge_lock); - list_add_tail(&bridge->list, &bridge_list); + list_add(&bridge->list, &bridge_list); mutex_unlock(&bridge_lock); } EXPORT_SYMBOL(drm_bridge_add); > > > >> At next patch eDP must be placed at head of bridge chain to fix eDP > >> corruption issue. This is the purpose of this patch. I will revise the > >> commit text. > >> > > Wouldn't that be "broken" again if we decided to change drm_bridge_add() > > to add to the list head instead of list tail? Or if somehow > > msm_dp_modeset_init() was called in a different order so that the DP > > bridge was added before the eDP bridge? > > we have no control of drm_bridge_add(). > > Since drm perform screen update following bridge chain sequentially, we > have to make sure primary always update first. >
On 6/24/2022 3:19 PM, Stephen Boyd wrote: > Quoting Kuogee Hsieh (2022-06-24 14:49:57) >> On 6/24/2022 2:40 PM, Stephen Boyd wrote: >>> Quoting Kuogee Hsieh (2022-06-24 14:17:50) >>>> On 6/24/2022 1:00 PM, Stephen Boyd wrote: >>>>> Quoting Kuogee Hsieh (2022-06-24 10:15:11) >>>>>> Current the index (dp->id) of DP descriptor table (scxxxx_dp_cfg[]) are tightly >>>>>> coupled with DP controller_id. This means DP use controller id 0 must be placed >>>>>> at first entry of DP descriptor table (scxxxx_dp_cfg[]). Otherwise the internal >>>>>> INTF will mismatch controller_id. This will cause controller kickoff wrong >>>>>> interface timing engine and cause dpu_encoder_phys_vid_wait_for_commit_done >>>>>> vblank timeout error. >>>>>> >>>>>> This patch add controller_id field into struct msm_dp_desc to break the tightly >>>>>> coupled relationship between index (dp->id) of DP descriptor table with DP >>>>>> controller_id. >>>>> Please no. This reverts the intention of commit bb3de286d992 >>>>> ("drm/msm/dp: Support up to 3 DP controllers") >>>>> >>>>> A new enum is introduced to document the connection between the >>>>> instances referenced in the dpu_intf_cfg array and the controllers in >>>>> the DP driver and sc7180 is updated. >>>>> >>>>> It sounds like the intent of that commit failed to make a strong enough >>>>> connection. Now it needs to match the INTF number as well? I can't >>>>> really figure out what is actually wrong, because this patch undoes that >>>>> intentional tight coupling. Is the next patch the important part that >>>>> flips the order of the two interfaces? >>>> The commit bb3de286d992have two problems, >>>> >>>> 1) The below sc7280_dp_cfg will not work, if eDP use >>>> MSM_DP_CONTROLLER_2 instead of MSM_DP_CONTROLLER_1 >>> Why would we use three indices for an soc that only has two indices >>> possible? This is not a real problem? >> I do not what will happen at future, it may have more dp controller use >> late. >> >> at current soc, below table has only one eDP will not work either. >> >> static const struct msm_dp_config sc7280_dp_cfg = { >> .descs = (const struct msm_dp_desc[]) { >> [MSM_DP_CONTROLLER_1] = { .io_start = 0x0aea0000, >> .connector_type = DRM_MODE_CONNECTOR_eDP, .wide_bus_en = true }, >> >> .num_descs = 1, > So the MSM_DP_CONTROLLER_* number needs to match what exactly?MSM 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(). > >>>> since it have num_descs =2 but eDP is at index 2 (CONTROLLER_2) which >>>> never be reached. >>>> >>>> static const struct msm_dp_config sc7280_dp_cfg = { >>>> .descs = (const struct msm_dp_desc[]) { >>>> [MSM_DP_CONTROLLER_2] = { .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, >>>> }; >>>> >>>> 2) DP always has index of 0 (dp->id = 0) and the first one to call >>>> msm_dp_modeset_init(). This make DP always place at head of bridge chain. >>> What does this mean? Are you talking about the list of bridges in drm >>> core, i.e. 'bridge_list'? >> yes, > I changed the drm_bridge_add() API and that doesn't make any difference. > The corruption is still seen. That would imply it is not the order of > the list of bridges. Sorry, my mistake. it is not in drm_bridge_add. It should be in dpu_encoder_init() of _dpu_kms_initialize_displayport(). can you make below changes (patch) to _dpu_kms_initialize_displayport(). kuogee: go backward for dp modeset_init diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c index 3a4da0d..b271a4b 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c @@ -611,9 +611,15 @@ static int _dpu_kms_initialize_displayport(struct drm_device *dev, struct drm_encoder *encoder = NULL; struct msm_display_info info; int rc; - int i; + int i,num; + + num = ARRAY_SIZE(priv->dp); +#ifdef XXXX for (i = 0; i < ARRAY_SIZE(priv->dp); i++) { +#else + for (i = num - 1; i >= 0 ; i--) { +#endif if (!priv->dp[i]) continue; > > ---8<--- > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c > index e275b4ca344b..e3518101b65e 100644 > --- a/drivers/gpu/drm/drm_bridge.c > +++ b/drivers/gpu/drm/drm_bridge.c > @@ -165,7 +165,7 @@ void drm_bridge_add(struct drm_bridge *bridge) > mutex_init(&bridge->hpd_mutex); > > mutex_lock(&bridge_lock); > - list_add_tail(&bridge->list, &bridge_list); > + list_add(&bridge->list, &bridge_list); > mutex_unlock(&bridge_lock); > } > EXPORT_SYMBOL(drm_bridge_add); > >>>> At next patch eDP must be placed at head of bridge chain to fix eDP >>>> corruption issue. This is the purpose of this patch. I will revise the >>>> commit text. >>>> >>> Wouldn't that be "broken" again if we decided to change drm_bridge_add() >>> to add to the list head instead of list tail? Or if somehow >>> msm_dp_modeset_init() was called in a different order so that the DP >>> bridge was added before the eDP bridge? >> we have no control of drm_bridge_add(). >> >> Since drm perform screen update following bridge chain sequentially, we >> have to make sure primary always update first. >>
On Sat, 25 Jun 2022 at 00:17, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote: > > > On 6/24/2022 1:00 PM, Stephen Boyd wrote: > > Quoting Kuogee Hsieh (2022-06-24 10:15:11) > >> Current the index (dp->id) of DP descriptor table (scxxxx_dp_cfg[]) are tightly > >> coupled with DP controller_id. This means DP use controller id 0 must be placed > >> at first entry of DP descriptor table (scxxxx_dp_cfg[]). Otherwise the internal > >> INTF will mismatch controller_id. This will cause controller kickoff wrong > >> interface timing engine and cause dpu_encoder_phys_vid_wait_for_commit_done > >> vblank timeout error. > >> > >> This patch add controller_id field into struct msm_dp_desc to break the tightly > >> coupled relationship between index (dp->id) of DP descriptor table with DP > >> controller_id. > > Please no. This reverts the intention of commit bb3de286d992 > > ("drm/msm/dp: Support up to 3 DP controllers") > > > > A new enum is introduced to document the connection between the > > instances referenced in the dpu_intf_cfg array and the controllers in > > the DP driver and sc7180 is updated. > > > > It sounds like the intent of that commit failed to make a strong enough > > connection. Now it needs to match the INTF number as well? I can't > > really figure out what is actually wrong, because this patch undoes that > > intentional tight coupling. Is the next patch the important part that > > flips the order of the two interfaces? > > The commit bb3de286d992have two problems, > > 1) The below sc7280_dp_cfg will not work, if eDP use > MSM_DP_CONTROLLER_2 instead of MSM_DP_CONTROLLER_1 > > since it have num_descs =2 but eDP is at index 2 (CONTROLLER_2) which > never be reached. > > static const struct msm_dp_config sc7280_dp_cfg = { > .descs = (const struct msm_dp_desc[]) { > [MSM_DP_CONTROLLER_2] = { .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, Please change num_descs to 3. Or better eliminate it completely and iterate up to MSM_DP_CONTROLLER_MAX, checking whether the entry contains real values or is just a zero sentinel entry. > }; > > 2) DP always has index of 0 (dp->id = 0) and the first one to call > msm_dp_modeset_init(). This make DP always place at head of bridge chain. > > At next patch eDP must be placed at head of bridge chain to fix eDP > corruption issue. This is the purpose of this patch. I will revise the > commit text. This text doesn't make sense to me. The dp->id has nothing to do with the bridge chains. Each dp entry is a head of the corresponding bridge chain. DP with dp->id = 0 and eDP with dp->id = whatever will be parts of different encoder -> bridges -> connector chains.
On Sat, 25 Jun 2022 at 02:45, Stephen Boyd <swboyd@chromium.org> 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. You both are correct here. We are searching through the _intf[] array for the corresponding controller_id. And yes, the controller_ids are being passed through the h_tile_instance[] array.
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);
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
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.
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. > 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
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 > >
On Sat, 25 Jun 2022 at 03:19, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote: > How can I have eDP call dpu_encoder_init() before DP calls with > _dpu_kms_initialize_displayport()? Why do you want to do it? They are two different encoders.
Quoting Stephen Boyd (2022-06-24 17:11:01) > Quoting Abhinav Kumar (2022-06-24 17:03:37) > > > > 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. It seems like somewhere else the order of the array matters, presumably while setting up encoders? > > > > > 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. > Ah nevermind. I mixed up dp->id and h_tile_instance thinking one was overwriting the other but that doesn't make any sense.
On 6/24/2022 5:21 PM, Dmitry Baryshkov wrote: > On Sat, 25 Jun 2022 at 03:19, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote: >> How can I have eDP call dpu_encoder_init() before DP calls with >> _dpu_kms_initialize_displayport()? > Why do you want to do it? They are two different encoders. eDP is primary display which in normal case should be bring up first if DP is also presented. We want eDP encoder help functions be executed before DP.
On Sat, 25 Jun 2022 at 03:23, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote: > On 6/24/2022 5:21 PM, Dmitry Baryshkov wrote: > > On Sat, 25 Jun 2022 at 03:19, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote: > >> How can I have eDP call dpu_encoder_init() before DP calls with > >> _dpu_kms_initialize_displayport()? > > Why do you want to do it? They are two different encoders. > > eDP is primary display which in normal case should be bring up first if > DP is also presented. I do not like the concept of primary display. It is the user, who must decide which display is primary to him. I have seen people using just external monitors and ignoring built-in eDP completely. Also, why does the bring up order matters here? What do you gain by bringing up eDP before the DP? > > We want eDP encoder help functions be executed before DP. Why?
On Sat, 25 Jun 2022 at 03:28, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > On Sat, 25 Jun 2022 at 03:23, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote: > > On 6/24/2022 5:21 PM, Dmitry Baryshkov wrote: > > > On Sat, 25 Jun 2022 at 03:19, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote: > > >> How can I have eDP call dpu_encoder_init() before DP calls with > > >> _dpu_kms_initialize_displayport()? > > > Why do you want to do it? They are two different encoders. > > > > eDP is primary display which in normal case should be bring up first if > > DP is also presented. > > I do not like the concept of primary display. It is the user, who must > decide which display is primary to him. I have seen people using just > external monitors and ignoring built-in eDP completely. > > Also, why does the bring up order matters here? What do you gain by > bringing up eDP before the DP? I should probably rephrase my question to be more clear. How does changing the order of DP vs eDP bringup help you 'to fix screen corruption'.
On 6/24/2022 5:46 PM, Dmitry Baryshkov wrote: > On Sat, 25 Jun 2022 at 03:28, Dmitry Baryshkov > <dmitry.baryshkov@linaro.org> wrote: >> On Sat, 25 Jun 2022 at 03:23, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote: >>> On 6/24/2022 5:21 PM, Dmitry Baryshkov wrote: >>>> On Sat, 25 Jun 2022 at 03:19, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote: >>>>> How can I have eDP call dpu_encoder_init() before DP calls with >>>>> _dpu_kms_initialize_displayport()? >>>> Why do you want to do it? They are two different encoders. >>> eDP is primary display which in normal case should be bring up first if >>> DP is also presented. >> I do not like the concept of primary display. It is the user, who must >> decide which display is primary to him. I have seen people using just >> external monitors and ignoring built-in eDP completely.from >> Also, why does the bring up order matters here? What do you gain by >> bringing up eDP before the DP? > I should probably rephrase my question to be more clear. How does > changing the order of DP vs eDP bringup help you 'to fix screen > corruption'. it did fix the primary display correction issue if edp go first and i do not know the root cause yet. We are still investigating it. However I do think currently msm_dp_config sc7280_dp_cfg has issues need be addressed. >
On 6/24/2022 5:23 PM, Stephen Boyd wrote: > Quoting Stephen Boyd (2022-06-24 17:11:01) >> Quoting Abhinav Kumar (2022-06-24 17:03:37) >>> >>> 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. > > It seems like somewhere else the order of the array matters, presumably > while setting up encoders? > >> >>> >>> 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. >> > > Ah nevermind. I mixed up dp->id and h_tile_instance thinking one was > overwriting the other but that doesn't make any sense. Yes, I also misunderstood one point. Even if we re-order like above, we are still retaining the index correctly so that will still work. I checked with kuogee again now, he mentioned he needs this only for patch 3. He is not sure of the root-cause of why turning ON the first display fixes the issue . I think that needs to be debugged correctly to answers questions posted by you / Dmitry. Lets hold on these patches till we have the answers.
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. Right, this is where I misunderstood his explanation. Even if we swap the order, but retain the index correctly it will still work today. Hes not sure of the root-cause of why turning on the primary display first fixes the issue. I think till we root-cause that, lets put this on hold. > >> 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. > >> 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 > > >
On Sat, 25 Jun 2022 at 04:23, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > 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: > >> On 6/24/2022 4:56 PM, Kuogee Hsieh wrote: > >> 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. > > Right, this is where I misunderstood his explanation. > > Even if we swap the order, but retain the index correctly it will still > work today. > > Hes not sure of the root-cause of why turning on the primary display > first fixes the issue. > > I think till we root-cause that, lets put this on hold. Agreed. Let's find the root cause.
Hi, On Sat, Jun 25, 2022 at 1:48 AM Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > On Sat, 25 Jun 2022 at 04:23, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > > 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: > > >> On 6/24/2022 4:56 PM, Kuogee Hsieh wrote: > > >> 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. > > > > Right, this is where I misunderstood his explanation. > > > > Even if we swap the order, but retain the index correctly it will still > > work today. > > > > Hes not sure of the root-cause of why turning on the primary display > > first fixes the issue. > > > > I think till we root-cause that, lets put this on hold. > > Agreed. Let's find the root cause. FWIW, I was poking a little bit about the glitch that Kuogee was trying to fix here. Through a bunch of hacky heuristics I think the dpu_hw_ctl_trigger_flush_v1() is the function that "causes" the corruption. Specifically I managed to do something like: if (hacky_heuristic) pr_info("About to call flush\n); mdelay(2000); } ctl->ops.trigger_flush(ctl) if (hacky_heuristic) pr_info("Finished calling flush\n); mdelay(2000); pr_info("Finished calling flush delay done\n); } I then watched my display and reproduced the problem. When I saw the problem I looked over at the console and saw "Finished calling flush" was the last thing printed. I don't know if this helps much. Presumably we're flushing a bunch of previous operations so whatever we had queued up probably matters more, but maybe it'll give a clue? Other notes FWIW: * If you increase the amount of time of the glitching, you can actually see that we are glitching both the internal and external displays. * You can actually make the glitch stay on the screen "permanently" by unplugging the external display while the internal screen is off. I don't know why it doesn't always happen, but it seems to happen often enough. Presumably if someone knew the display controller well they could try to figure out what state it was in and debug the problem. -Doug
On 6/27/2022 4:20 PM, Doug Anderson wrote: > Hi, > > On Sat, Jun 25, 2022 at 1:48 AM Dmitry Baryshkov > <dmitry.baryshkov@linaro.org> wrote: >> On Sat, 25 Jun 2022 at 04:23, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: >>> 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: >>>>> On 6/24/2022 4:56 PM, Kuogee Hsieh wrote: >>>>> 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. >>> Right, this is where I misunderstood his explanation. >>> >>> Even if we swap the order, but retain the index correctly it will still >>> work today. >>> >>> Hes not sure of the root-cause of why turning on the primary display >>> first fixes the issue. >>> >>> I think till we root-cause that, lets put this on hold. >> Agreed. Let's find the root cause. > FWIW, I was poking a little bit about the glitch that Kuogee was > trying to fix here. Through a bunch of hacky heuristics I think the > dpu_hw_ctl_trigger_flush_v1() is the function that "causes" the > corruption. Specifically I managed to do something like: > > if (hacky_heuristic) > pr_info("About to call flush\n); > mdelay(2000); > } > ctl->ops.trigger_flush(ctl) > if (hacky_heuristic) > pr_info("Finished calling flush\n); > mdelay(2000); > pr_info("Finished calling flush delay done\n); > } flush bit need to up update at real time. otherwise unexpected side effects will happen. i try same thing, but I got fence timeout error. Anyway, I had submit new patch to fix corruption issue. Thanks for your efforts and helps. > I then watched my display and reproduced the problem. When I saw the > problem I looked over at the console and saw "Finished calling flush" > was the last thing printed. > > I don't know if this helps much. Presumably we're flushing a bunch of > previous operations so whatever we had queued up probably matters > more, but maybe it'll give a clue? > > > Other notes FWIW: > > * If you increase the amount of time of the glitching, you can > actually see that we are glitching both the internal and external > displays. > > * You can actually make the glitch stay on the screen "permanently" by > unplugging the external display while the internal screen is off. I > don't know why it doesn't always happen, but it seems to happen often > enough. Presumably if someone knew the display controller well they > could try to figure out what state it was in and debug the problem. > > > -Doug
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c index 2b9d931..8feeb89 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c @@ -615,7 +615,7 @@ static int _dpu_kms_initialize_displayport(struct drm_device *dev, struct dpu_kms *dpu_kms) { struct drm_encoder *encoder = NULL; - struct msm_display_info info; + struct msm_display_info *info; int rc; int i; @@ -637,11 +637,15 @@ static int _dpu_kms_initialize_displayport(struct drm_device *dev, return rc; } - info.num_of_h_tiles = 1; - info.h_tile_instance[0] = i; - info.capabilities = MSM_DISPLAY_CAP_VID_MODE; - info.intf_type = encoder->encoder_type; - rc = dpu_encoder_setup(dev, encoder, &info); + info = &priv->info[i]; + info->intf_type = encoder->encoder_type; + /* + * info->capabilities, info->num_of_h_tiles and + * info->h_tile_instance are populated at + * dp_display_bind() + */ + + rc = dpu_encoder_setup(dev, encoder, info); if (rc) { DPU_ERROR("failed to setup DPU encoder %d: rc:%d\n", encoder->base.id, rc); diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c index da5c03a..a87a9d8 100644 --- a/drivers/gpu/drm/msm/dp/dp_display.c +++ b/drivers/gpu/drm/msm/dp/dp_display.c @@ -77,6 +77,7 @@ struct dp_display_private { int irq; unsigned int id; + unsigned int controller_id; /* state variables */ bool core_initialized; @@ -123,6 +124,7 @@ struct dp_display_private { struct msm_dp_desc { phys_addr_t io_start; unsigned int connector_type; + unsigned int controller_id; bool wide_bus_en; }; @@ -133,31 +135,38 @@ struct msm_dp_config { static const struct msm_dp_config sc7180_dp_cfg = { .descs = (const struct msm_dp_desc[]) { - [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort }, + { .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort, + .controller_id = MSM_DP_CONTROLLER_0 }, }, .num_descs = 1, }; 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 }, + { .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort, + .controller_id = MSM_DP_CONTROLLER_0, .wide_bus_en = true }, + { .io_start = 0x0aea0000, .connector_type = DRM_MODE_CONNECTOR_eDP, + .controller_id = MSM_DP_CONTROLLER_1, .wide_bus_en = true }, }, .num_descs = 2, }; static const struct msm_dp_config sc8180x_dp_cfg = { .descs = (const struct msm_dp_desc[]) { - [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort }, - [MSM_DP_CONTROLLER_1] = { .io_start = 0x0ae98000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort }, - [MSM_DP_CONTROLLER_2] = { .io_start = 0x0ae9a000, .connector_type = DRM_MODE_CONNECTOR_eDP }, + {.io_start = 0x0ae9a000, .connector_type = DRM_MODE_CONNECTOR_eDP, + .controller_id = MSM_DP_CONTROLLER_2 }, + { .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort, + .controller_id = MSM_DP_CONTROLLER_0 }, + { .io_start = 0x0ae98000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort, + .controller_id = MSM_DP_CONTROLLER_1 }, }, .num_descs = 3, }; static const struct msm_dp_config sm8350_dp_cfg = { .descs = (const struct msm_dp_desc[]) { - [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort }, + { .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort, + .controller_id = MSM_DP_CONTROLLER_0 }, }, .num_descs = 1, }; @@ -260,10 +269,16 @@ static int dp_display_bind(struct device *dev, struct device *master, struct dp_display_private *dp = dev_get_dp_display_private(dev); struct msm_drm_private *priv = dev_get_drvdata(master); struct drm_device *drm = priv->dev; + struct msm_display_info *info; dp->dp_display.drm_dev = drm; priv->dp[dp->id] = &dp->dp_display; + info = &priv->info[dp->id]; + info->num_of_h_tiles = 1; + info->h_tile_instance[0] = dp->controller_id; + info->capabilities = MSM_DISPLAY_CAP_VID_MODE; + rc = dp->parser->parse(dp->parser); if (rc) { DRM_ERROR("device tree parsing failed\n"); @@ -1308,6 +1323,7 @@ static int dp_display_probe(struct platform_device *pdev) dp->pdev = pdev; dp->name = "drm_dp"; dp->dp_display.connector_type = desc->connector_type; + dp->controller_id = desc->controller_id; dp->wide_bus_en = desc->wide_bus_en; dp->dp_display.is_edp = (dp->dp_display.connector_type == DRM_MODE_CONNECTOR_eDP); diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h index f9c263b..71ab699 100644 --- a/drivers/gpu/drm/msm/msm_drv.h +++ b/drivers/gpu/drm/msm/msm_drv.h @@ -150,6 +150,8 @@ struct msm_drm_private { struct msm_dp *dp[MSM_DP_CONTROLLER_COUNT]; + struct msm_display_info info[MSM_DP_CONTROLLER_COUNT]; + /* when we have more than one 'msm_gpu' these need to be an array: */ struct msm_gpu *gpu;
Current the index (dp->id) of DP descriptor table (scxxxx_dp_cfg[]) are tightly coupled with DP controller_id. This means DP use controller id 0 must be placed at first entry of DP descriptor table (scxxxx_dp_cfg[]). Otherwise the internal INTF will mismatch controller_id. This will cause controller kickoff wrong interface timing engine and cause dpu_encoder_phys_vid_wait_for_commit_done vblank timeout error. This patch add controller_id field into struct msm_dp_desc to break the tightly coupled relationship between index (dp->id) of DP descriptor table with DP controller_id. Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com> --- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 16 ++++++++++------ drivers/gpu/drm/msm/dp/dp_display.c | 30 +++++++++++++++++++++++------- drivers/gpu/drm/msm/msm_drv.h | 2 ++ 3 files changed, 35 insertions(+), 13 deletions(-)