Re: [PATCH] drm/amdgpu/dm/mst: Fix incorrect usage of drm_dp_add_payload_part2()
Gotcha, I'll take another look at this tomorrow
On Mon, 2022-10-17 at 03:09 +, Lin, Wayne wrote:
> [Public]
>
>
>
> > -Original Message-
> > From: Lyude Paul
> > Sent: Thursday, October 6, 2022 3:37 AM
> > To: Siqueira, Rodrigo ; dri-
> > de...@lists.freedesktop.org; amd-...@lists.freedesktop.org
> > Cc: Wentland, Harry ; Li, Sun peng (Leo)
> > ; Deucher, Alexander
> > ; Koenig, Christian
> > ; Pan, Xinhui ; David
> > Airlie ; Daniel Vetter ; Zuo, Jerry
> > ; Lin, Wayne ; Chen, Ian
> > ; Mikita Lipski ; Mahfooz,
> > Hamza ; Claudio Suarez ; Colin
> > Ian King ; Jani Nikula ; open
> > list
> > Subject: Re: [PATCH] drm/amdgpu/dm/mst: Fix incorrect usage of
> > drm_dp_add_payload_part2()
> >
> > On Tue, 2022-10-04 at 16:46 -0400, Rodrigo Siqueira Jordao wrote:
> > >
> > > On 2022-10-04 16:24, Lyude Paul wrote:
> > > > Yikes, it appears somehow I totally made a mistake here. We're
> > > > currently checking to see if drm_dp_add_payload_part2() returns a
> > > > non-zero value to indicate success. That's totally wrong though, as
> > > > this function only returns a zero value on success - not the other way
> > around.
> > > >
> > > > So, fix that.
> > > >
> > > > Signed-off-by: Lyude Paul
> > > > Issue:
> > > >
> > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgi
> > > > tlab.freedesktop.org%2Fdrm%2Famd%2F-
> > %2Fissues%2F2171data=05%7C0
> > > >
> > 1%7Cwayne.lin%40amd.com%7Ccd5a63120e064f4bb6aa08daa7090baf%7C3d
> > d8961
> > > >
> > fe4884e608e11a82d994e183d%7C0%7C0%7C638005954559719396%7CUnkno
> > wn%7CT
> > > >
> > WFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLC
> > JXV
> > > >
> > CI6Mn0%3D%7C3000%7C%7C%7Csdata=nMIGnUKS6EDrdKJ0rR%2BAh
> > FRa4ST0%2
> > > > BYr9bILmXv40yv0%3Dreserved=0
> > > > Fixes: 4d07b0bc4034 ("drm/display/dp_mst: Move all payload info into
> > > > the atomic state")
> > > > ---
> > > > drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 2
> > +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git
> > > > a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> > > > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> > > > index b8077fcd4651..00598def5b39 100644
> > > > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> > > > +++
> > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> > > > @@ -297,7 +297,7 @@ bool
> > dm_helpers_dp_mst_send_payload_allocation(
> > > > clr_flag = MST_ALLOCATE_NEW_PAYLOAD;
> > > > }
> > > >
> > > > - if (enable && drm_dp_add_payload_part2(mst_mgr, mst_state-
> > > base.state, payload)) {
> > > > + if (enable && drm_dp_add_payload_part2(mst_mgr,
> > > > +mst_state->base.state, payload) == 0) {
>
> Hi Lyude,
>
> This line changes the original logic a bit. The 'if' case was trying to catch
> failure
> while sending ALLOCATE_PAYLOAD. If the msg fails, set the set_flag to false.
> If succeed, set the set_flag to true and clear the clr_flag.
>
> Sorry if the code wording misleading. Thanks!
>
> > > > amdgpu_dm_set_mst_status(>mst_status,
> > > > set_flag, false);
> > > > } else {
> > >
> > > Hi Lyude,
> > >
> > > Maybe I'm missing something, but I can't find the
> > > drm_dp_add_payload_part2() function on amd-staging-drm-next. Which
> > > repo are you using?
> >
> > If it's not on amd-staging-drm-next then it likely hasn't gotten backported
> > to
> > amd's branch yet and is in drm-misc-next
> >
> > >
> > > Thanks
> > > Siqueira
> > >
> >
> > --
> > Cheers,
> > Lyude Paul (she/her)
> > Software Engineer at Red Hat
> --
> Regards,
> Wayne Lin
>
--
Cheers,
Lyude Paul (she/her)
Software Engineer at Red Hat
RE: [PATCH] drm/amdgpu/dm/mst: Fix incorrect usage of drm_dp_add_payload_part2()
[Public]
> -Original Message-
> From: Lyude Paul
> Sent: Thursday, October 6, 2022 3:37 AM
> To: Siqueira, Rodrigo ; dri-
> de...@lists.freedesktop.org; amd-...@lists.freedesktop.org
> Cc: Wentland, Harry ; Li, Sun peng (Leo)
> ; Deucher, Alexander
> ; Koenig, Christian
> ; Pan, Xinhui ; David
> Airlie ; Daniel Vetter ; Zuo, Jerry
> ; Lin, Wayne ; Chen, Ian
> ; Mikita Lipski ; Mahfooz,
> Hamza ; Claudio Suarez ; Colin
> Ian King ; Jani Nikula ; open
> list
> Subject: Re: [PATCH] drm/amdgpu/dm/mst: Fix incorrect usage of
> drm_dp_add_payload_part2()
>
> On Tue, 2022-10-04 at 16:46 -0400, Rodrigo Siqueira Jordao wrote:
> >
> > On 2022-10-04 16:24, Lyude Paul wrote:
> > > Yikes, it appears somehow I totally made a mistake here. We're
> > > currently checking to see if drm_dp_add_payload_part2() returns a
> > > non-zero value to indicate success. That's totally wrong though, as
> > > this function only returns a zero value on success - not the other way
> around.
> > >
> > > So, fix that.
> > >
> > > Signed-off-by: Lyude Paul
> > > Issue:
> > >
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgi
> > > tlab.freedesktop.org%2Fdrm%2Famd%2F-
> %2Fissues%2F2171data=05%7C0
> > >
> 1%7Cwayne.lin%40amd.com%7Ccd5a63120e064f4bb6aa08daa7090baf%7C3d
> d8961
> > >
> fe4884e608e11a82d994e183d%7C0%7C0%7C638005954559719396%7CUnkno
> wn%7CT
> > >
> WFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLC
> JXV
> > >
> CI6Mn0%3D%7C3000%7C%7C%7Csdata=nMIGnUKS6EDrdKJ0rR%2BAh
> FRa4ST0%2
> > > BYr9bILmXv40yv0%3Dreserved=0
> > > Fixes: 4d07b0bc4034 ("drm/display/dp_mst: Move all payload info into
> > > the atomic state")
> > > ---
> > > drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 2
> +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git
> > > a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> > > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> > > index b8077fcd4651..00598def5b39 100644
> > > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> > > +++
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> > > @@ -297,7 +297,7 @@ bool
> dm_helpers_dp_mst_send_payload_allocation(
> > > clr_flag = MST_ALLOCATE_NEW_PAYLOAD;
> > > }
> > >
> > > - if (enable && drm_dp_add_payload_part2(mst_mgr, mst_state-
> >base.state, payload)) {
> > > + if (enable && drm_dp_add_payload_part2(mst_mgr,
> > > +mst_state->base.state, payload) == 0) {
Hi Lyude,
This line changes the original logic a bit. The 'if' case was trying to catch
failure
while sending ALLOCATE_PAYLOAD. If the msg fails, set the set_flag to false.
If succeed, set the set_flag to true and clear the clr_flag.
Sorry if the code wording misleading. Thanks!
> > > amdgpu_dm_set_mst_status(>mst_status,
> > > set_flag, false);
> > > } else {
> >
> > Hi Lyude,
> >
> > Maybe I'm missing something, but I can't find the
> > drm_dp_add_payload_part2() function on amd-staging-drm-next. Which
> > repo are you using?
>
> If it's not on amd-staging-drm-next then it likely hasn't gotten backported to
> amd's branch yet and is in drm-misc-next
>
> >
> > Thanks
> > Siqueira
> >
>
> --
> Cheers,
> Lyude Paul (she/her)
> Software Engineer at Red Hat
--
Regards,
Wayne Lin
Re: [PATCH] drm/amdgpu/dm/mst: Fix incorrect usage of drm_dp_add_payload_part2()
On Tue, 2022-10-04 at 16:46 -0400, Rodrigo Siqueira Jordao wrote:
>
> On 2022-10-04 16:24, Lyude Paul wrote:
> > Yikes, it appears somehow I totally made a mistake here. We're currently
> > checking to see if drm_dp_add_payload_part2() returns a non-zero value to
> > indicate success. That's totally wrong though, as this function only
> > returns a zero value on success - not the other way around.
> >
> > So, fix that.
> >
> > Signed-off-by: Lyude Paul
> > Issue: https://gitlab.freedesktop.org/drm/amd/-/issues/2171
> > Fixes: 4d07b0bc4034 ("drm/display/dp_mst: Move all payload info into the
> > atomic state")
> > ---
> > drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> > index b8077fcd4651..00598def5b39 100644
> > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> > @@ -297,7 +297,7 @@ bool dm_helpers_dp_mst_send_payload_allocation(
> > clr_flag = MST_ALLOCATE_NEW_PAYLOAD;
> > }
> >
> > - if (enable && drm_dp_add_payload_part2(mst_mgr, mst_state->base.state,
> > payload)) {
> > + if (enable && drm_dp_add_payload_part2(mst_mgr, mst_state->base.state,
> > payload) == 0) {
> > amdgpu_dm_set_mst_status(>mst_status,
> > set_flag, false);
> > } else {
>
> Hi Lyude,
>
> Maybe I'm missing something, but I can't find the
> drm_dp_add_payload_part2() function on amd-staging-drm-next. Which repo
> are you using?
If it's not on amd-staging-drm-next then it likely hasn't gotten backported to
amd's branch yet and is in drm-misc-next
>
> Thanks
> Siqueira
>
--
Cheers,
Lyude Paul (she/her)
Software Engineer at Red Hat
Re: [PATCH] drm/amdgpu/dm/mst: Fix incorrect usage of drm_dp_add_payload_part2()
On 2022-10-04 16:24, Lyude Paul wrote:
Yikes, it appears somehow I totally made a mistake here. We're currently
checking to see if drm_dp_add_payload_part2() returns a non-zero value to
indicate success. That's totally wrong though, as this function only
returns a zero value on success - not the other way around.
So, fix that.
Signed-off-by: Lyude Paul
Issue: https://gitlab.freedesktop.org/drm/amd/-/issues/2171
Fixes: 4d07b0bc4034 ("drm/display/dp_mst: Move all payload info into the atomic
state")
---
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
index b8077fcd4651..00598def5b39 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
@@ -297,7 +297,7 @@ bool dm_helpers_dp_mst_send_payload_allocation(
clr_flag = MST_ALLOCATE_NEW_PAYLOAD;
}
- if (enable && drm_dp_add_payload_part2(mst_mgr, mst_state->base.state, payload)) {
+ if (enable && drm_dp_add_payload_part2(mst_mgr, mst_state->base.state,
payload) == 0) {
amdgpu_dm_set_mst_status(>mst_status,
set_flag, false);
} else {
Hi Lyude,
Maybe I'm missing something, but I can't find the
drm_dp_add_payload_part2() function on amd-staging-drm-next. Which repo
are you using?
Thanks
Siqueira
[PATCH] drm/amdgpu/dm/mst: Fix incorrect usage of drm_dp_add_payload_part2()
Yikes, it appears somehow I totally made a mistake here. We're currently
checking to see if drm_dp_add_payload_part2() returns a non-zero value to
indicate success. That's totally wrong though, as this function only
returns a zero value on success - not the other way around.
So, fix that.
Signed-off-by: Lyude Paul
Issue: https://gitlab.freedesktop.org/drm/amd/-/issues/2171
Fixes: 4d07b0bc4034 ("drm/display/dp_mst: Move all payload info into the atomic
state")
---
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
index b8077fcd4651..00598def5b39 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
@@ -297,7 +297,7 @@ bool dm_helpers_dp_mst_send_payload_allocation(
clr_flag = MST_ALLOCATE_NEW_PAYLOAD;
}
- if (enable && drm_dp_add_payload_part2(mst_mgr, mst_state->base.state,
payload)) {
+ if (enable && drm_dp_add_payload_part2(mst_mgr, mst_state->base.state,
payload) == 0) {
amdgpu_dm_set_mst_status(>mst_status,
set_flag, false);
} else {
--
2.37.3
