Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <41ffe32931a21961a3f368b30bd4335faa5ac6a4.camel@redhat.com>
Subject: Re: [PATCH] drm/amdgpu/dm/mst: Fix incorrect usage of
 drm_dp_add_payload_part2()
From:   Lyude Paul <lyude@redhat.com>
To:     "Lin, Wayne" <Wayne.Lin@amd.com>,
        "Siqueira, Rodrigo" <Rodrigo.Siqueira@amd.com>,
        "dri-devel@lists.freedesktop.org" <dri-devel@lists.freedesktop.org>,
        "amd-gfx@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>, "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>
Date:   Wed, 19 Oct 2022 17:32:11 -0400
In-Reply-To: <CO6PR12MB548939841653ABD0EAA95DCAFC299@CO6PR12MB5489.namprd12.prod.outlook.com>
References: <20221004202429.124422-1-lyude@redhat.com>
         <d3b272e1-3b5d-c843-e8ac-57dc5e3a7ced@amd.com>
         <128762cfb6524d17e6ee7308aa7e859dd350fa63.camel@redhat.com>
         <CO6PR12MB548939841653ABD0EAA95DCAFC299@CO6PR12MB5489.namprd12.prod.outlook.com>
Organization: Red Hat Inc.
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.42.4 (3.42.4-2.fc35) 
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

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&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
> 

-- 
Cheers,
 Lyude Paul (she/her)
 Software Engineer at Red Hat