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 <[email protected]>
---
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(-)
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;
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
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?
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,
};
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.
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,
};
>
>> 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,
>
>> 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.
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?
>
> 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'?
>
> 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?
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.
>>
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[].
>
> 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().
>
Yes, I've made that change to try to understand the problem. I still
don't understand, sadly. Does flipping the order of iteration through
'priv->dp' somehow mean that the crtc that is assigned to the eDP
connector is left unchanged? Whereas without registering the eDP encoder
first means we have to change the crtc for the eDP encoder and that
can't be done atomically?
On Sat, 25 Jun 2022 at 00:17, Kuogee Hsieh <[email protected]> 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.
--
With best wishes
Dmitry
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.
>
>> 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().
>>
> Yes, I've made that change to try to understand the problem. I still
> don't understand, sadly. Does flipping the order of iteration through
> 'priv->dp' somehow mean that the crtc that is assigned to the eDP
> connector is left unchanged? Whereas without registering the eDP encoder
> first means we have to change the crtc for the eDP encoder and that
> can't be done atomically?
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.
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
On Sat, 25 Jun 2022 at 02:45, Stephen Boyd <[email protected]> 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.
--
With best wishes
Dmitry
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 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);
On Sat, 25 Jun 2022 at 03:19, Kuogee Hsieh <[email protected]> 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.
--
With best wishes
Dmitry
On Sat, 25 Jun 2022 at 03:23, Kuogee Hsieh <[email protected]> wrote:
> On 6/24/2022 5:21 PM, Dmitry Baryshkov wrote:
> > On Sat, 25 Jun 2022 at 03:19, Kuogee Hsieh <[email protected]> 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?
--
With best wishes
Dmitry
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 Sat, 25 Jun 2022 at 03:03, Abhinav Kumar <[email protected]> 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
--
With best wishes
Dmitry
On 6/24/2022 5:11 PM, Dmitry Baryshkov wrote:
> On Sat, 25 Jun 2022 at 03:03, Abhinav Kumar <[email protected]> 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 6/24/2022 5:21 PM, Dmitry Baryshkov wrote:
> On Sat, 25 Jun 2022 at 03:19, Kuogee Hsieh <[email protected]> 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:28, Dmitry Baryshkov
<[email protected]> wrote:
>
> On Sat, 25 Jun 2022 at 03:23, Kuogee Hsieh <[email protected]> wrote:
> > On 6/24/2022 5:21 PM, Dmitry Baryshkov wrote:
> > > On Sat, 25 Jun 2022 at 03:19, Kuogee Hsieh <[email protected]> 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'.
--
With best wishes
Dmitry
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.
Quoting Kuogee Hsieh (2022-06-24 18:02:50)
>
> On 6/24/2022 5:46 PM, Dmitry Baryshkov wrote:
> > On Sat, 25 Jun 2022 at 03:28, Dmitry Baryshkov
> > <[email protected]> wrote:
> >> On Sat, 25 Jun 2022 at 03:23, Kuogee Hsieh <[email protected]> wrote:
> >>> On 6/24/2022 5:21 PM, Dmitry Baryshkov wrote:
> >>>> On Sat, 25 Jun 2022 at 03:19, Kuogee Hsieh <[email protected]> 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.
>
What issues exist with sc7280_dp_cfg? It looks correct to me.
On 6/24/2022 5:46 PM, Dmitry Baryshkov wrote:
> On Sat, 25 Jun 2022 at 03:28, Dmitry Baryshkov
> <[email protected]> wrote:
>> On Sat, 25 Jun 2022 at 03:23, Kuogee Hsieh <[email protected]> wrote:
>>> On 6/24/2022 5:21 PM, Dmitry Baryshkov wrote:
>>>> On Sat, 25 Jun 2022 at 03:19, Kuogee Hsieh <[email protected]> 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:11 PM, Dmitry Baryshkov wrote:
> On Sat, 25 Jun 2022 at 03:03, Abhinav Kumar <[email protected]> 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 <[email protected]> wrote:
> On 6/24/2022 5:11 PM, Dmitry Baryshkov wrote:
> > On Sat, 25 Jun 2022 at 03:03, Abhinav Kumar <[email protected]> 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.
--
With best wishes
Dmitry
On 6/24/2022 6:15 PM, Stephen Boyd wrote:
> Quoting Kuogee Hsieh (2022-06-24 18:02:50)
>> On 6/24/2022 5:46 PM, Dmitry Baryshkov wrote:
>>> On Sat, 25 Jun 2022 at 03:28, Dmitry Baryshkov
>>> <[email protected]> wrote:
>>>> On Sat, 25 Jun 2022 at 03:23, Kuogee Hsieh <[email protected]> wrote:
>>>>> On 6/24/2022 5:21 PM, Dmitry Baryshkov wrote:
>>>>>> On Sat, 25 Jun 2022 at 03:19, Kuogee Hsieh <[email protected]> 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.
>>
> What issues exist with sc7280_dp_cfg? It looks correct to me.
If we are going to bring up a new chipset with edp as primary only, i am
not sure the below configuration will work?
> 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,
> };
On Mon, 27 Jun 2022 at 18:33, Kuogee Hsieh <[email protected]> wrote:
>
>
> On 6/24/2022 6:15 PM, Stephen Boyd wrote:
> > Quoting Kuogee Hsieh (2022-06-24 18:02:50)
> >> On 6/24/2022 5:46 PM, Dmitry Baryshkov wrote:
> >>> On Sat, 25 Jun 2022 at 03:28, Dmitry Baryshkov
> >>> <[email protected]> wrote:
> >>>> On Sat, 25 Jun 2022 at 03:23, Kuogee Hsieh <[email protected]> wrote:
> >>>>> On 6/24/2022 5:21 PM, Dmitry Baryshkov wrote:
> >>>>>> On Sat, 25 Jun 2022 at 03:19, Kuogee Hsieh <[email protected]> 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.
> >>
> > What issues exist with sc7280_dp_cfg? It looks correct to me.
>
>
> If we are going to bring up a new chipset with edp as primary only, i am
> not sure the below configuration will work?
>
> > 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,
> > };
As I wrote in one of the comments, there is an issue with num_descs
being not obvious (in your example it should be 2, not 1). I thought
about dropping it and looping until the MSM_DP_CONTROLLER_COUNT, but
this would result in other kinds of hard-to-catch issues. Let me muse
about it.
--
With best wishes
Dmitry
On 6/27/2022 8:38 AM, Dmitry Baryshkov wrote:
> On Mon, 27 Jun 2022 at 18:33, Kuogee Hsieh <[email protected]> wrote:
>>
>> On 6/24/2022 6:15 PM, Stephen Boyd wrote:
>>> Quoting Kuogee Hsieh (2022-06-24 18:02:50)
>>>> On 6/24/2022 5:46 PM, Dmitry Baryshkov wrote:
>>>>> On Sat, 25 Jun 2022 at 03:28, Dmitry Baryshkov
>>>>> <[email protected]> wrote:
>>>>>> On Sat, 25 Jun 2022 at 03:23, Kuogee Hsieh <[email protected]> wrote:
>>>>>>> On 6/24/2022 5:21 PM, Dmitry Baryshkov wrote:
>>>>>>>> On Sat, 25 Jun 2022 at 03:19, Kuogee Hsieh <[email protected]> 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.
>>>>
>>> What issues exist with sc7280_dp_cfg? It looks correct to me.
>>
>> If we are going to bring up a new chipset with edp as primary only, i am
>> not sure the below configuration will work?
>>
>>> 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,
>>> };
> As I wrote in one of the comments, there is an issue with num_descs
> being not obvious (in your example it should be 2, not 1). I thought
> about dropping it and looping until the MSM_DP_CONTROLLER_COUNT, but
> this would result in other kinds of hard-to-catch issues. Let me muse
> about it.
Thanks for consideration.
Hi,
On Sat, Jun 25, 2022 at 1:48 AM Dmitry Baryshkov
<[email protected]> wrote:
>
> On Sat, 25 Jun 2022 at 04:23, Abhinav Kumar <[email protected]> wrote:
> > On 6/24/2022 5:11 PM, Dmitry Baryshkov wrote:
> > > On Sat, 25 Jun 2022 at 03:03, Abhinav Kumar <[email protected]> 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
> <[email protected]> wrote:
>> On Sat, 25 Jun 2022 at 04:23, Abhinav Kumar <[email protected]> wrote:
>>> On 6/24/2022 5:11 PM, Dmitry Baryshkov wrote:
>>>> On Sat, 25 Jun 2022 at 03:03, Abhinav Kumar <[email protected]> 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