| Message ID | 20221004202429.124422-1-lyude@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/amdgpu/dm/mst: Fix incorrect usage of drm_dp_add_payload_part2() | expand |
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 <lyude@redhat.com> > 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(&aconnector->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
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 <lyude@redhat.com> > > 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(&aconnector->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 >
[Public] > -----Original Message----- > From: Lyude Paul <lyude@redhat.com> > Sent: Thursday, October 6, 2022 3:37 AM > To: Siqueira, Rodrigo <Rodrigo.Siqueira@amd.com>; dri- > devel@lists.freedesktop.org; amd-gfx@lists.freedesktop.org > Cc: Wentland, Harry <Harry.Wentland@amd.com>; Li, Sun peng (Leo) > <Sunpeng.Li@amd.com>; Deucher, Alexander > <Alexander.Deucher@amd.com>; Koenig, Christian > <Christian.Koenig@amd.com>; Pan, Xinhui <Xinhui.Pan@amd.com>; David > Airlie <airlied@gmail.com>; Daniel Vetter <daniel@ffwll.ch>; Zuo, Jerry > <Jerry.Zuo@amd.com>; Lin, Wayne <Wayne.Lin@amd.com>; Chen, Ian > <Ian.Chen@amd.com>; Mikita Lipski <mikita.lipski@amd.com>; Mahfooz, > Hamza <Hamza.Mahfooz@amd.com>; Claudio Suarez <cssk@net-c.es>; Colin > Ian King <colin.i.king@gmail.com>; Jani Nikula <jani.nikula@intel.com>; open > list <linux-kernel@vger.kernel.org> > 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 <lyude@redhat.com> > > > Issue: > > > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgi > > > tlab.freedesktop.org%2Fdrm%2Famd%2F- > %2Fissues%2F2171&data=05%7C0 > > > > 1%7Cwayne.lin%40amd.com%7Ccd5a63120e064f4bb6aa08daa7090baf%7C3d > d8961 > > > > fe4884e608e11a82d994e183d%7C0%7C0%7C638005954559719396%7CUnkno > wn%7CT > > > > WFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLC > JXV > > > > CI6Mn0%3D%7C3000%7C%7C%7C&sdata=nMIGnUKS6EDrdKJ0rR%2BAh > FRa4ST0%2 > > > BYr9bILmXv40yv0%3D&reserved=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(&aconnector->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
Gotcha, I'll take another look at this tomorrow On Mon, 2022-10-17 at 03:09 +0000, Lin, Wayne wrote: > [Public] > > > > > -----Original Message----- > > From: Lyude Paul <lyude@redhat.com> > > Sent: Thursday, October 6, 2022 3:37 AM > > To: Siqueira, Rodrigo <Rodrigo.Siqueira@amd.com>; dri- > > devel@lists.freedesktop.org; amd-gfx@lists.freedesktop.org > > Cc: Wentland, Harry <Harry.Wentland@amd.com>; Li, Sun peng (Leo) > > <Sunpeng.Li@amd.com>; Deucher, Alexander > > <Alexander.Deucher@amd.com>; Koenig, Christian > > <Christian.Koenig@amd.com>; Pan, Xinhui <Xinhui.Pan@amd.com>; David > > Airlie <airlied@gmail.com>; Daniel Vetter <daniel@ffwll.ch>; Zuo, Jerry > > <Jerry.Zuo@amd.com>; Lin, Wayne <Wayne.Lin@amd.com>; Chen, Ian > > <Ian.Chen@amd.com>; Mikita Lipski <mikita.lipski@amd.com>; Mahfooz, > > Hamza <Hamza.Mahfooz@amd.com>; Claudio Suarez <cssk@net-c.es>; Colin > > Ian King <colin.i.king@gmail.com>; Jani Nikula <jani.nikula@intel.com>; open > > list <linux-kernel@vger.kernel.org> > > 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 <lyude@redhat.com> > > > > Issue: > > > > > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgi > > > > tlab.freedesktop.org%2Fdrm%2Famd%2F- > > %2Fissues%2F2171&data=05%7C0 > > > > > > 1%7Cwayne.lin%40amd.com%7Ccd5a63120e064f4bb6aa08daa7090baf%7C3d > > d8961 > > > > > > fe4884e608e11a82d994e183d%7C0%7C0%7C638005954559719396%7CUnkno > > wn%7CT > > > > > > WFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLC > > JXV > > > > > > CI6Mn0%3D%7C3000%7C%7C%7C&sdata=nMIGnUKS6EDrdKJ0rR%2BAh > > FRa4ST0%2 > > > > BYr9bILmXv40yv0%3D&reserved=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(&aconnector->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 >
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(&aconnector->mst_status, set_flag, false); } else {
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 <lyude@redhat.com> 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(-)