RE: [PATCH] drm/amdgpu/dm/mst: Fix incorrect usage of drm_dp_add_payload_part2()

[Lin, Wayne]

[Public]



> -----Original Message-----
> From: Lyude Paul <lyude@xxxxxxxxxx>
> Sent: Thursday, October 6, 2022 3:37 AM
> To: Siqueira, Rodrigo <Rodrigo.Siqueira@xxxxxxx>; dri-
> devel@xxxxxxxxxxxxxxxxxxxxx; amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> Cc: Wentland, Harry <Harry.Wentland@xxxxxxx>; Li, Sun peng (Leo)
> <Sunpeng.Li@xxxxxxx>; Deucher, Alexander
> <Alexander.Deucher@xxxxxxx>; Koenig, Christian
> <Christian.Koenig@xxxxxxx>; Pan, Xinhui <Xinhui.Pan@xxxxxxx>; David
> Airlie <airlied@xxxxxxxxx>; Daniel Vetter <daniel@xxxxxxxx>; Zuo, Jerry
> <Jerry.Zuo@xxxxxxx>; Lin, Wayne <Wayne.Lin@xxxxxxx>; Chen, Ian
> <Ian.Chen@xxxxxxx>; Mikita Lipski <mikita.lipski@xxxxxxx>; Mahfooz,
> Hamza <Hamza.Mahfooz@xxxxxxx>; Claudio Suarez <cssk@xxxxxxxx>; Colin
> Ian King <colin.i.king@xxxxxxxxx>; Jani Nikula <jani.nikula@xxxxxxxxx>; open
> list <linux-kernel@xxxxxxxxxxxxxxx>
> 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@xxxxxxxxxx>
> > > Issue:
> > >
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgi
> > > tlab.freedesktop.org%2Fdrm%2Famd%2F-
> %2Fissues%2F2171&amp;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&amp;sdata=nMIGnUKS6EDrdKJ0rR%2BAh
> FRa4ST0%2
> > > BYr9bILmXv40yv0%3D&amp;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