[PATCH] drm/amdgpu: Add a GEM_CREATE mask and bugfix

[Huang Rui]

On Tue, Feb 18, 2020 at 04:46:21PM -0500, Luben Tuikov wrote:
> On 2020-02-17 10:08 a.m., Christian König wrote:
> > Am 17.02.20 um 15:44 schrieb Alex Deucher:
> >> On Fri, Feb 14, 2020 at 7:17 PM Luben Tuikov <luben.tuikov at amd.com> wrote:
> >>> Add a AMDGPU_GEM_CREATE_MASK and use it to check
> >>> for valid/invalid GEM create flags coming in from
> >>> userspace.
> >>>
> >>> Fix a bug in checking whether TMZ is supported at
> >>> GEM create time.
> >>>
> >>> Signed-off-by: Luben Tuikov <luben.tuikov at amd.com>
> >>> ---
> >>>   drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 11 ++---------
> >>>   include/uapi/drm/amdgpu_drm.h           |  2 ++
> >>>   2 files changed, 4 insertions(+), 9 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> >>> index b51a060c637d..74bb79e64fa3 100644
> >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> >>> @@ -221,21 +221,14 @@ int amdgpu_gem_create_ioctl(struct drm_device *dev, void *data,
> >>>          int r;
> >>>
> >>>          /* reject invalid gem flags */
> >>> -       if (flags & ~(AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED |
> >>> -                     AMDGPU_GEM_CREATE_NO_CPU_ACCESS |
> >>> -                     AMDGPU_GEM_CREATE_CPU_GTT_USWC |
> >>> -                     AMDGPU_GEM_CREATE_VRAM_CLEARED |
> >>> -                     AMDGPU_GEM_CREATE_VM_ALWAYS_VALID |
> >>> -                     AMDGPU_GEM_CREATE_EXPLICIT_SYNC |
> >>> -                     AMDGPU_GEM_CREATE_ENCRYPTED))
> >>> -
> >> I'd rather keep the list explicit so no one ends up forgetting to
> >> update the mask the next time new flags are added.
> > 
> > Additional to that this patch is bogus.
> 
> So the only "additional" thing you're contributing to the review of
> this patch is calling it "bogus". Characterizing the patch with an adjective
> as "bogus" is very disturbing. What's next?
> 
> > 
> > We intentionally filter out the flags here which userspace is allowed to 
> > specify in the GEM interface, but after this patch we would allow all 
> > flags to be specified.
> 
> I thought that that could be the case. But in your review why not
> recommend we have a mask to check user-settable flags? So the
> actual flags are collected and visible in one place and well
> understood that that is what is being checked and well-defined
> by a macro with an appropriate name?
> 
> Why did NO ONE comment on the bug fix below? No one.
> 

I missed the bugfix patch before, sorry. (There are many patches from
amd-gfx one day, if you didn't cc me, the mail may be missed). So I found
the issue after sync up with drm-next and then give the fix. Hope you can
understand and don't take it to heart.

Thanks,
Ray