From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 51A9FC77B6E for ; Thu, 13 Apr 2023 18:24:22 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id B7C5710EBAB; Thu, 13 Apr 2023 18:24:21 +0000 (UTC) Received: from mail-ej1-x630.google.com (mail-ej1-x630.google.com [IPv6:2a00:1450:4864:20::630]) by gabe.freedesktop.org (Postfix) with ESMTPS id E7D1810EBAB for ; Thu, 13 Apr 2023 18:24:19 +0000 (UTC) Received: by mail-ej1-x630.google.com with SMTP id xd13so5738842ejb.4 for ; Thu, 13 Apr 2023 11:24:19 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1681410256; x=1684002256; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=Ku9gRQuLVWJ9nY/Am2FOOCqxmL/I93x3A+ektHrct4A=; b=VctV0WSGElrQmO/10bkw6JxrO+jH1umsZu7HsWsKx6yzt3cfSHDy5Jrw87tyQv/E6p JwnFD+PY/EEmOJFmq8hE/iwfWrspYEYseU/WO6pX4usbvlk+ntjkZ7Rj9b1WLfokJiin 4GsAttmglO4WeaB568Yw0REi37WLXQKPNC5EjMHT1wTX5lWWCWmMy6Fi3vQAslH+Vtwg f2KFnriWo/5psyp68f/SJrV+fCrHiCgOvpBCqvWy4Z4PGMFRYN1tsAIxGg71LuNn4JpK jSFMkzi0AayW+/3t86v8JXZqMJ+4+monjVnwLpTb03G2IDIHGnT6aUtRk/1jyTkFUTAs 3MJA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1681410256; x=1684002256; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=Ku9gRQuLVWJ9nY/Am2FOOCqxmL/I93x3A+ektHrct4A=; b=ZSKwfWxxi012My2ND7FTDNwHvTIOduzQWOnhXfDsWsrlkk3mxAwrq0eRR0UTblYvHy Qnej7kGTbq0XGG+uak+dUCrNE2+QjXxCPAqbgEQ1ekxb4nxoxoEXIyG489RtZM+R546F z0HXfo28rsgs4yYVh7nH0efB0U2G2nD1hoPNPyGkOCQKEI+Yv/tCzTtksUJoO5ESQndD dU+MBk/bHwsPpQuGTgLHD1NncofueQ8xuRJVhWcztT0bwCS42ftUdJmcRTueYFTeB2i4 FQd9fTYnHwzyWFks/aZBV2nJjEt3/UXOhHstUgJYJuxxphQUGI0EI/lGKwLLi/QCusWi Gssg== X-Gm-Message-State: AAQBX9fFKLEO3j1nxDMugdu4rcT4eW9HMaOTodU08fkmW5GWJhOaxuul 5pgFWpUKkpdJ4/pXY9du7+p7/kyL1YVL3Go9J1M= X-Google-Smtp-Source: AKy350a6OmHQT7Bb36zZJIaAH0TmYSWeeZWKtYGlVGoOr9yuBU9kY+waNfxrmqIVaZEwkqc6H0atO9ntC0SUGxq3FOk= X-Received: by 2002:a17:906:37cd:b0:93d:1b82:3c13 with SMTP id o13-20020a17090637cd00b0093d1b823c13mr1737135ejc.7.1681410256444; Thu, 13 Apr 2023 11:24:16 -0700 (PDT) MIME-Version: 1.0 References: <20230330191750.1134210-1-alexander.deucher@amd.com> <20230330191750.1134210-4-alexander.deucher@amd.com> <8a98b160-5276-54f5-d166-e12a572892e9@amd.com> <2897fa79-f0ef-1ddb-dc38-05ef429fe0d9@gmail.com> <0e2e1902-54e8-0626-c7ca-3f818f8792fb@gmail.com> <8b2df4e1-ad00-a834-3382-d25f1c667978@gmail.com> In-Reply-To: From: =?UTF-8?B?TWFyZWsgT2zFocOhaw==?= Date: Thu, 13 Apr 2023 14:23:40 -0400 Message-ID: Subject: Re: [PATCH 03/13] drm/amdgpu/UAPI: add new CS chunk for GFX shadow buffers To: Alex Deucher Content-Type: multipart/alternative; boundary="0000000000000ea03b05f93bd517" X-BeenThere: amd-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Discussion list for AMD gfx List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Alex Deucher , =?UTF-8?Q?Christian_K=C3=B6nig?= , amd-gfx mailing list Errors-To: amd-gfx-bounces@lists.freedesktop.org Sender: "amd-gfx" --0000000000000ea03b05f93bd517 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable I'm OK with that. Marek On Thu, Apr 13, 2023 at 12:56=E2=80=AFPM Alex Deucher wrote: > On Thu, Apr 13, 2023 at 11:54=E2=80=AFAM Christian K=C3=B6nig > wrote: > > > > Am 13.04.23 um 14:26 schrieb Alex Deucher: > > > On Thu, Apr 13, 2023 at 7:32=E2=80=AFAM Christian K=C3=B6nig > > > wrote: > > >> Ok, then we have a problem. > > >> > > >> Alex what do you think? > > > If you program it to 0, FW skips the GDS backup I think so UMD's can > > > decide whether they want to use it or not, depending on whether they > > > use GDS. > > > > Yeah, but when Mesa isn't using it we have a hard way justifying to > > upstream that because it isn't tested at all. > > Well, the interface would still get used, it's just that mesa would > likely only ever pass 0 for the virtual address. It's just a > passthrough to the packet. If we discover we need it at some point, > it would be nice to not have to add a new interface to add it. > > Alex > > > > > Christian. > > > > > > > > Alex > > > > > > > > >> Christian. > > >> > > >> Am 13.04.23 um 11:21 schrieb Marek Ol=C5=A1=C3=A1k: > > >> > > >> That's not why it was removed. It was removed because userspace > doesn't use GDS memory and gds_va is always going to be 0. > > >> > > >> Firmware shouldn't use it because using it would increase preemption > latency. > > >> > > >> Marek > > >> > > >> On Sun, Apr 9, 2023, 11:21 Christian K=C3=B6nig < > ckoenig.leichtzumerken@gmail.com> wrote: > > >>> We removed the GDS information because they were unnecessary. The > GDS size was already part of the device info before we added the shadow > info. > > >>> > > >>> But as far as I know the firmware needs valid VAs for all three > buffers or won't work correctly. > > >>> > > >>> Christian. > > >>> > > >>> Am 06.04.23 um 17:01 schrieb Marek Ol=C5=A1=C3=A1k: > > >>> > > >>> There is no GDS shadowing info in the device info uapi, so userspac= e > can't create any GDS buffer and thus can't have any GDS va. It's a uapi > issue, not what firmware wants to do. > > >>> > > >>> Marek > > >>> > > >>> On Thu, Apr 6, 2023 at 6:31=E2=80=AFAM Christian K=C3=B6nig < > ckoenig.leichtzumerken@gmail.com> wrote: > > >>>> That's what I thought as well, but Mitch/Hans insisted on that. > > >>>> > > >>>> We should probably double check internally. > > >>>> > > >>>> Christian. > > >>>> > > >>>> Am 06.04.23 um 11:43 schrieb Marek Ol=C5=A1=C3=A1k: > > >>>> > > >>>> GDS memory isn't used on gfx11. Only GDS OA is used. > > >>>> > > >>>> Marek > > >>>> > > >>>> On Thu, Apr 6, 2023 at 5:09=E2=80=AFAM Christian K=C3=B6nig < > christian.koenig@amd.com> wrote: > > >>>>> Why that? > > >>>>> > > >>>>> This is the save buffer for GDS, not the old style GDS BOs. > > >>>>> > > >>>>> Christian. > > >>>>> > > >>>>> Am 06.04.23 um 09:36 schrieb Marek Ol=C5=A1=C3=A1k: > > >>>>> > > >>>>> gds_va is unnecessary. > > >>>>> > > >>>>> Marek > > >>>>> > > >>>>> On Thu, Mar 30, 2023 at 3:18=E2=80=AFPM Alex Deucher < > alexander.deucher@amd.com> wrote: > > >>>>>> For GFX11, the UMD needs to allocate some shadow buffers > > >>>>>> to be used for preemption. The UMD allocates the buffers > > >>>>>> and passes the GPU virtual address to the kernel since the > > >>>>>> kernel will program the packet that specified these > > >>>>>> addresses as part of its IB submission frame. > > >>>>>> > > >>>>>> v2: UMD passes shadow init to tell kernel when to initialize > > >>>>>> the shadow > > >>>>>> > > >>>>>> Reviewed-by: Christian K=C3=B6nig > > >>>>>> Signed-off-by: Alex Deucher > > >>>>>> --- > > >>>>>> include/uapi/drm/amdgpu_drm.h | 10 ++++++++++ > > >>>>>> 1 file changed, 10 insertions(+) > > >>>>>> > > >>>>>> diff --git a/include/uapi/drm/amdgpu_drm.h > b/include/uapi/drm/amdgpu_drm.h > > >>>>>> index b6eb90df5d05..3d9474af6566 100644 > > >>>>>> --- a/include/uapi/drm/amdgpu_drm.h > > >>>>>> +++ b/include/uapi/drm/amdgpu_drm.h > > >>>>>> @@ -592,6 +592,7 @@ struct drm_amdgpu_gem_va { > > >>>>>> #define AMDGPU_CHUNK_ID_SCHEDULED_DEPENDENCIES 0x07 > > >>>>>> #define AMDGPU_CHUNK_ID_SYNCOBJ_TIMELINE_WAIT 0x08 > > >>>>>> #define AMDGPU_CHUNK_ID_SYNCOBJ_TIMELINE_SIGNAL 0x09 > > >>>>>> +#define AMDGPU_CHUNK_ID_CP_GFX_SHADOW 0x0a > > >>>>>> > > >>>>>> struct drm_amdgpu_cs_chunk { > > >>>>>> __u32 chunk_id; > > >>>>>> @@ -708,6 +709,15 @@ struct drm_amdgpu_cs_chunk_data { > > >>>>>> }; > > >>>>>> }; > > >>>>>> > > >>>>>> +#define AMDGPU_CS_CHUNK_CP_GFX_SHADOW_FLAGS_INIT_SHADOW > 0x1 > > >>>>>> + > > >>>>>> +struct drm_amdgpu_cs_chunk_cp_gfx_shadow { > > >>>>>> + __u64 shadow_va; > > >>>>>> + __u64 csa_va; > > >>>>>> + __u64 gds_va; > > >>>>>> + __u64 flags; > > >>>>>> +}; > > >>>>>> + > > >>>>>> /* > > >>>>>> * Query h/w info: Flag that this is integrated (a.h.a. > fusion) GPU > > >>>>>> * > > >>>>>> -- > > >>>>>> 2.39.2 > > >>>>>> > > > --0000000000000ea03b05f93bd517 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
I'm OK with that.

Marek<= br>

On Thu, Apr 13, 2023 at 12:56=E2=80=AFPM Alex Deucher <alexdeucher@gmail.com> wrote:
<= /div>
On Thu, Apr 13, 2023= at 11:54=E2=80=AFAM Christian K=C3=B6nig
<c= koenig.leichtzumerken@gmail.com> wrote:
>
> Am 13.04.23 um 14:26 schrieb Alex Deucher:
> > On Thu, Apr 13, 2023 at 7:32=E2=80=AFAM Christian K=C3=B6nig
> > <ckoenig.leichtzumerken@gmail.com> wrote:
> >> Ok, then we have a problem.
> >>
> >> Alex what do you think?
> > If you program it to 0, FW skips the GDS backup I think so UMD= 9;s can
> > decide whether they want to use it or not, depending on whether t= hey
> > use GDS.
>
> Yeah, but when Mesa isn't using it we have a hard way justifying t= o
> upstream that because it isn't tested at all.

Well, the interface would still get used, it's just that mesa would
likely only ever pass 0 for the virtual address.=C2=A0 It's just a
passthrough to the packet.=C2=A0 If we discover we need it at some point, it would be nice to not have to add a new interface to add it.

Alex

>
> Christian.
>
> >
> > Alex
> >
> >
> >> Christian.
> >>
> >> Am 13.04.23 um 11:21 schrieb Marek Ol=C5=A1=C3=A1k:
> >>
> >> That's not why it was removed. It was removed because use= rspace doesn't use GDS memory and gds_va is always going to be 0.
> >>
> >> Firmware shouldn't use it because using it would increase= preemption latency.
> >>
> >> Marek
> >>
> >> On Sun, Apr 9, 2023, 11:21 Christian K=C3=B6nig <ckoenig.leic= htzumerken@gmail.com> wrote:
> >>> We removed the GDS information because they were unnecess= ary. The GDS size was already part of the device info before we added the s= hadow info.
> >>>
> >>> But as far as I know the firmware needs valid VAs for all= three buffers or won't work correctly.
> >>>
> >>> Christian.
> >>>
> >>> Am 06.04.23 um 17:01 schrieb Marek Ol=C5=A1=C3=A1k:
> >>>
> >>> There is no GDS shadowing info in the device info uapi, s= o userspace can't create any GDS buffer and thus can't have any GDS= va. It's a uapi issue, not what firmware wants to do.
> >>>
> >>> Marek
> >>>
> >>> On Thu, Apr 6, 2023 at 6:31=E2=80=AFAM Christian K=C3=B6n= ig <ckoenig.leichtzumerken@gmail.com> wrote:
> >>>> That's what I thought as well, but Mitch/Hans ins= isted on that.
> >>>>
> >>>> We should probably double check internally.
> >>>>
> >>>> Christian.
> >>>>
> >>>> Am 06.04.23 um 11:43 schrieb Marek Ol=C5=A1=C3=A1k: > >>>>
> >>>> GDS memory isn't used on gfx11. Only GDS OA is us= ed.
> >>>>
> >>>> Marek
> >>>>
> >>>> On Thu, Apr 6, 2023 at 5:09=E2=80=AFAM Christian K=C3= =B6nig <ch= ristian.koenig@amd.com> wrote:
> >>>>> Why that?
> >>>>>
> >>>>> This is the save buffer for GDS, not the old styl= e GDS BOs.
> >>>>>
> >>>>> Christian.
> >>>>>
> >>>>> Am 06.04.23 um 09:36 schrieb Marek Ol=C5=A1=C3=A1= k:
> >>>>>
> >>>>> gds_va is unnecessary.
> >>>>>
> >>>>> Marek
> >>>>>
> >>>>> On Thu, Mar 30, 2023 at 3:18=E2=80=AFPM Alex Deuc= her <alex= ander.deucher@amd.com> wrote:
> >>>>>> For GFX11, the UMD needs to allocate some sha= dow buffers
> >>>>>> to be used for preemption.=C2=A0 The UMD allo= cates the buffers
> >>>>>> and passes the GPU virtual address to the ker= nel since the
> >>>>>> kernel will program the packet that specified= these
> >>>>>> addresses as part of its IB submission frame.=
> >>>>>>
> >>>>>> v2: UMD passes shadow init to tell kernel whe= n to initialize
> >>>>>>=C2=A0 =C2=A0 =C2=A0 the shadow
> >>>>>>
> >>>>>> Reviewed-by: Christian K=C3=B6nig <christian.koenig@amd= .com>
> >>>>>> Signed-off-by: Alex Deucher <alexander.deucher@amd.com= >
> >>>>>> ---
> >>>>>>=C2=A0 =C2=A0include/uapi/drm/amdgpu_drm.h | 1= 0 ++++++++++
> >>>>>>=C2=A0 =C2=A01 file changed, 10 insertions(+)<= br> > >>>>>>
> >>>>>> diff --git a/include/uapi/drm/amdgpu_drm.h b/= include/uapi/drm/amdgpu_drm.h
> >>>>>> index b6eb90df5d05..3d9474af6566 100644
> >>>>>> --- a/include/uapi/drm/amdgpu_drm.h
> >>>>>> +++ b/include/uapi/drm/amdgpu_drm.h
> >>>>>> @@ -592,6 +592,7 @@ struct drm_amdgpu_gem_va = {
> >>>>>>=C2=A0 =C2=A0#define AMDGPU_CHUNK_ID_SCHEDULED= _DEPENDENCIES 0x07
> >>>>>>=C2=A0 =C2=A0#define AMDGPU_CHUNK_ID_SYNCOBJ_T= IMELINE_WAIT=C2=A0 =C2=A0 0x08
> >>>>>>=C2=A0 =C2=A0#define AMDGPU_CHUNK_ID_SYNCOBJ_T= IMELINE_SIGNAL=C2=A0 0x09
> >>>>>> +#define AMDGPU_CHUNK_ID_CP_GFX_SHADOW=C2=A0 = =C2=A00x0a
> >>>>>>
> >>>>>>=C2=A0 =C2=A0struct drm_amdgpu_cs_chunk {
> >>>>>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 __u32=C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0chunk_id;
> >>>>>> @@ -708,6 +709,15 @@ struct drm_amdgpu_cs_chu= nk_data {
> >>>>>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 };
> >>>>>>=C2=A0 =C2=A0};
> >>>>>>
> >>>>>> +#define AMDGPU_CS_CHUNK_CP_GFX_SHADOW_FLAGS_= INIT_SHADOW=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A00x1
> >>>>>> +
> >>>>>> +struct drm_amdgpu_cs_chunk_cp_gfx_shadow { > >>>>>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0__u64 shadow_va;<= br> > >>>>>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0__u64 csa_va;
> >>>>>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0__u64 gds_va;
> >>>>>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0__u64 flags;
> >>>>>> +};
> >>>>>> +
> >>>>>>=C2=A0 =C2=A0/*
> >>>>>>=C2=A0 =C2=A0 *=C2=A0 Query h/w info: Flag tha= t this is integrated (a.h.a. fusion) GPU
> >>>>>>=C2=A0 =C2=A0 *
> >>>>>> --
> >>>>>> 2.39.2
> >>>>>>
>
--0000000000000ea03b05f93bd517--