Re: [PATCH 07/11] drm/amdgpu: add UAPI to query GFX shadow sizes

[Alex Deucher <alexdeucher@gmail.com>]

On Wed, Mar 22, 2023 at 10:12 AM Marek Olšák <maraeo@gmail.com> wrote:
>
> On Tue, Mar 21, 2023 at 3:51 PM Alex Deucher <alexdeucher@gmail.com> wrote:
>>
>> On Mon, Mar 20, 2023 at 8:30 PM Marek Olšák <maraeo@gmail.com> wrote:
>> >
>> >
>> > On Mon, Mar 20, 2023 at 1:38 PM Alex Deucher <alexander.deucher@amd.com> wrote:
>> >>
>> >> Add UAPI to query the GFX shadow buffer requirements
>> >> for preemption on GFX11.  UMDs need to specify the shadow
>> >> areas for preemption.
>> >>
>> >> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
>> >> ---
>> >>  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 3d9474af6566..19a806145371 100644
>> >> --- a/include/uapi/drm/amdgpu_drm.h
>> >> +++ b/include/uapi/drm/amdgpu_drm.h
>> >> @@ -886,6 +886,7 @@ struct drm_amdgpu_cs_chunk_cp_gfx_shadow {
>> >>         #define AMDGPU_INFO_VIDEO_CAPS_DECODE           0
>> >>         /* Subquery id: Encode */
>> >>         #define AMDGPU_INFO_VIDEO_CAPS_ENCODE           1
>> >> +#define AMDGPU_INFO_CP_GFX_SHADOW_SIZE         0x22
>> >>
>> >>  #define AMDGPU_INFO_MMR_SE_INDEX_SHIFT 0
>> >>  #define AMDGPU_INFO_MMR_SE_INDEX_MASK  0xff
>> >> @@ -1203,6 +1204,15 @@ struct drm_amdgpu_info_video_caps {
>> >>         struct drm_amdgpu_info_video_codec_info codec_info[AMDGPU_INFO_VIDEO_CAPS_CODEC_IDX_COUNT];
>> >>  };
>> >>
>> >> +struct drm_amdgpu_info_cp_gfx_shadow_size {
>> >> +       __u32 shadow_size;
>> >> +       __u32 shadow_alignment;
>> >> +       __u32 csa_size;
>> >> +       __u32 csa_alignment;
>> >> +       __u32 gds_size;
>> >> +       __u32 gds_alignment;
>> >
>> >
>> > Can you document the fields? What is CSA? Also, why is GDS there when the hw deprecated it and replaced it with GDS registers?
>>
>> Will add documentation.  For reference:
>> CSA (Context Save Area) - used as a scratch area for FW for saving
>> various things
>> Shadow - stores the pipeline state
>> GDS backup - stores the GDS state used by the pipeline.  I'm not sure
>> if this is registers or the old GDS memory.  Presumably the former.
>
>
> 1. The POR for gfx11 was not to use GDS memory. I don't know why it's there, but it would be unused uapi.

It still needs to be allocated because the FW requires it.

>
> 2. Is it secure to give userspace write access to the CSA and shadow buffers? In the case of CSA, it looks like userspace could break the firmware.

Yes, it should be fine. It's the same way it has always been, it's
just that on older chips the kernel mapped them into all of the GPUVMs
because it was global. If the userspace screws it up, they are only
hurting themselves because the FW uses it to save the UMD's hardware
state.

Alex

>
> Marek