Hi Yong,
looks pretty good to me, but still quite a few comments.
First of all please send the patches directly to the mailing list using
"git send-email" and not as attachment.
Patch #1:
> +
> + switch (word_size) {
> + case 4:
> + num_dw = num_loops * adev->mman.buffer_funcs->fill_num_dw;
> + break;
> + case 8: /* 10 double words for each SDMA_OP_PTEPDE cmd */
> + num_dw = num_loops * 10;
> + break;
> + default:
> + return -EINVAL;
> + }
That is to complicated, we don't use the 32bit pattern during the fill
anyway. So just change that to a 64bit pattern and always use the
amdgpu_vm_set_pte_pde() function.
Patch #2:
> +/* Flag that supports ATS through PTE on GFX9 */
> +#define AMDGPU_GEM_CLEAR_PTE_WITH_ATS_SUPPORT (1 << 6)
NAK to that approach, GEM flags are visible to userspace.
Instead add the pattern to use for the clear to
amdgpu_bo_create()/amdgpu_bo_create_restricted().
> /* Flag to indicate if VM tables are updated by CPU or GPU (SDMA) */
> bool use_cpu_for_update;
>
> - /* Whether this is a Compute or GFX Context */
> - int vm_context;
> + /* flags indicating the properties of VM context */
> + int vm_context_flags;
Either merge use_cpu_for_update into the flags as well or use a separate
boolean for this (I prefer the later).
If you want to merge drop the "vm_context_" prefix in the name, just
flags should be sufficient.
Regards,
Christian.
Am 26.07.2017 um 00:48 schrieb Yong Zhao:
> Hi there,
>
> Attached are two patches made to amdgpu in order to support ATS on
> Raven. Please review them.
>
> Regards,
>
> Yong
>
>
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20170726/93322b93/attachment.html>
