From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Xie, AlexBin" Subject: Re: [PATCH] drm/amdgpu: Move compute vm bug logic to amdgpu_vm.c Date: Thu, 1 Jun 2017 13:40:16 +0000 Message-ID: References: <1496272081-21584-1-git-send-email-AlexBin.Xie@amd.com>, <183887cb-17a0-e457-0c92-c04cc4b7fb53@vodafone.de> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1367496318==" Return-path: In-Reply-To: <183887cb-17a0-e457-0c92-c04cc4b7fb53-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org> Content-Language: en-US List-Id: Discussion list for AMD gfx List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: amd-gfx-bounces-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org Sender: "amd-gfx" To: =?iso-8859-1?Q?Christian_K=F6nig?= , "amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org" --===============1367496318== Content-Language: en-US Content-Type: multipart/alternative; boundary="_000_MWHPR1201MB0045729996FF89B47DCD29A3F2F60MWHPR1201MB0045_" --_000_MWHPR1201MB0045729996FF89B47DCD29A3F2F60MWHPR1201MB0045_ Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Ok. I will add the v2 next time. I did not add the v2 in case this email li= st does not put the two patches under same thread... -Alex Bin ________________________________ From: Christian K=F6nig Sent: Thursday, June 1, 2017 9:26 AM To: Xie, AlexBin; amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org Subject: Re: [PATCH] drm/amdgpu: Move compute vm bug logic to amdgpu_vm.c Am 01.06.2017 um 01:08 schrieb Alex Xie: > In review, Christian would like to keep the logic > inside amdgpu_vm.c with a cost of slightly slower. > The loop is still optimized out with this patch. > > v2: remove the if statement. Now it is not slower. > > Signed-off-by: Alex Xie When you create a v2 of a patch please note that in the subject line as well. Either by using the "-v2" option with "git send-email" or by adding the v2 manually on the subject line. I sometimes get mails twice because they are send to multiple lists I'm registered on and that helps allot seeing that a patch changed and is not just a duplicated mail. Anyway this version is Reviewed-by: Christian K=F6nig as well. Regards, Christian. > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 2 ++ > drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c | 32 -----------------------= -- > drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h | 5 ---- > drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 38 +++++++++++++++++++++++= +++++-- > drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 1 + > 5 files changed, 39 insertions(+), 39 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm= /amd/amdgpu/amdgpu_device.c > index 89bc34a..2f1a4e9 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > @@ -2227,6 +2227,8 @@ int amdgpu_device_init(struct amdgpu_device *adev, > > adev->accel_working =3D true; > > + amdgpu_vm_check_compute_bug(adev); > + > /* Initialize the buffer migration limit. */ > if (amdgpu_moverate >=3D 0) > max_MBps =3D amdgpu_moverate; > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c b/drivers/gpu/drm/a= md/amdgpu/amdgpu_ring.c > index 7d95435..31aa51d 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c > @@ -153,36 +153,6 @@ void amdgpu_ring_undo(struct amdgpu_ring *ring) > } > > /** > - * amdgpu_ring_check_compute_vm_bug - check whether this ring has comput= e vm bug > - * > - * @adev: amdgpu_device pointer > - * @ring: amdgpu_ring structure holding ring information > - */ > -static void amdgpu_ring_check_compute_vm_bug(struct amdgpu_device *adev, > - struct amdgpu_ring *ring) > -{ > - const struct amdgpu_ip_block *ip_block; > - > - ring->has_compute_vm_bug =3D false; > - > - if (ring->funcs->type !=3D AMDGPU_RING_TYPE_COMPUTE) > - /* only compute rings */ > - return; > - > - ip_block =3D amdgpu_get_ip_block(adev, AMD_IP_BLOCK_TYPE_GFX); > - if (!ip_block) > - return; > - > - /* Compute ring has a VM bug for GFX version < 7. > - And compute ring has a VM bug for GFX 8 MEC firmware version = < 673.*/ > - if (ip_block->version->major <=3D 7) { > - ring->has_compute_vm_bug =3D true; > - } else if (ip_block->version->major =3D=3D 8) > - if (adev->gfx.mec_fw_version < 673) > - ring->has_compute_vm_bug =3D true; > -} > - > -/** > * amdgpu_ring_init - init driver ring struct. > * > * @adev: amdgpu_device pointer > @@ -288,8 +258,6 @@ int amdgpu_ring_init(struct amdgpu_device *adev, stru= ct amdgpu_ring *ring, > DRM_ERROR("Failed to register debugfs file for rings !\n")= ; > } > > - amdgpu_ring_check_compute_vm_bug(adev, ring); > - > return 0; > } > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h b/drivers/gpu/drm/a= md/amdgpu/amdgpu_ring.h > index 334307e..ad399c8 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h > @@ -208,9 +208,4 @@ static inline void amdgpu_ring_clear_ring(struct amdg= pu_ring *ring) > > } > > -static inline bool amdgpu_ring_has_compute_vm_bug(struct amdgpu_ring *ri= ng) > -{ > - return ring->has_compute_vm_bug; > -} > - > #endif > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd= /amdgpu/amdgpu_vm.c > index d4d05a8..6e32748 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c > @@ -656,6 +656,41 @@ static int amdgpu_vm_alloc_reserved_vmid(struct amdg= pu_device *adev, > return r; > } > > +/** > + * amdgpu_vm_check_compute_bug - check whether asic has compute vm bug > + * > + * @adev: amdgpu_device pointer > + */ > +void amdgpu_vm_check_compute_bug(struct amdgpu_device *adev) > +{ > + const struct amdgpu_ip_block *ip_block; > + bool has_compute_vm_bug; > + struct amdgpu_ring *ring; > + int i; > + > + has_compute_vm_bug =3D false; > + > + ip_block =3D amdgpu_get_ip_block(adev, AMD_IP_BLOCK_TYPE_GFX); > + if (ip_block) { > + /* Compute has a VM bug for GFX version < 7. > + Compute has a VM bug for GFX 8 MEC firmware version < 67= 3.*/ > + if (ip_block->version->major <=3D 7) > + has_compute_vm_bug =3D true; > + else if (ip_block->version->major =3D=3D 8) > + if (adev->gfx.mec_fw_version < 673) > + has_compute_vm_bug =3D true; > + } > + > + for (i =3D 0; i < adev->num_rings; i++) { > + ring =3D adev->rings[i]; > + if (ring->funcs->type =3D=3D AMDGPU_RING_TYPE_COMPUTE) > + /* only compute rings */ > + ring->has_compute_vm_bug =3D has_compute_vm_bug; > + else > + ring->has_compute_vm_bug =3D false; > + } > +} > + > bool amdgpu_vm_need_pipeline_sync(struct amdgpu_ring *ring, > struct amdgpu_job *job) > { > @@ -664,8 +699,7 @@ bool amdgpu_vm_need_pipeline_sync(struct amdgpu_ring = *ring, > struct amdgpu_vm_id_manager *id_mgr =3D &adev->vm_manager.id_mgr[v= mhub]; > struct amdgpu_vm_id *id; > bool gds_switch_needed; > - bool vm_flush_needed =3D job->vm_needs_flush || > - amdgpu_ring_has_compute_vm_bug(ring); > + bool vm_flush_needed =3D job->vm_needs_flush || ring->has_compute_v= m_bug; > > if (job->vm_id =3D=3D 0) > return false; > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd= /amdgpu/amdgpu_vm.h > index 8309bc7..f5dba9c 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h > @@ -245,5 +245,6 @@ void amdgpu_vm_adjust_size(struct amdgpu_device *adev= , uint64_t vm_size); > int amdgpu_vm_ioctl(struct drm_device *dev, void *data, struct drm_file= *filp); > bool amdgpu_vm_need_pipeline_sync(struct amdgpu_ring *ring, > struct amdgpu_job *job); > +void amdgpu_vm_check_compute_bug(struct amdgpu_device *adev); > > #endif --_000_MWHPR1201MB0045729996FF89B47DCD29A3F2F60MWHPR1201MB0045_ Content-Type: text/html; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable

Ok. I will add the v2 next time. I did not add the v2 in case this email= list does not put the two patches under same thread...


-Alex Bin

From: Christian K=F6nig &= lt;deathsimple-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org>
Sent: Thursday, June 1, 2017 9:26 AM
To: Xie, AlexBin; amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
Subject: Re: [PATCH] drm/amdgpu: Move compute vm bug logic to amdgpu= _vm.c
 
Am 01.06.2017 um 01:08 schrieb Alex Xie:
>    In review, Christian would like to keep the logic >    inside amdgpu_vm.c with a cost of slightly slower. >    The loop is still optimized out with this patch.
>
> v2: remove the if statement. Now it is not slower.
>
> Signed-off-by: Alex Xie <AlexBin.Xie-5C7GfCeVMHo@public.gmane.org>

When you create a v2 of a patch please note that in the subject line as well.

Either by using the "-v2" option with "git send-email" = or by adding the
v2 manually on the subject line.

I sometimes get mails twice because they are send to multiple lists I'm registered on and that helps allot seeing that a patch changed and is
not just a duplicated mail.

Anyway this version is Reviewed-by: Christian K=F6nig
<christian.koeng-5C7GfCeVMHo@public.gmane.org> as well.

Regards,
Christian.

> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c |  2 += +
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c   | 32 = -------------------------
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h   |&nbs= p; 5 ----
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c   &n= bsp; | 38 +++++++++++++= +++++++++++++++= --
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h   &n= bsp; |  1 +
>   5 files changed, 39 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/= drm/amd/amdgpu/amdgpu_device.c
> index 89bc34a..2f1a4e9 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -2227,6 +2227,8 @@ int amdgpu_device_init(struct amdgpu_device = *adev,
>  
>        adev->accel_working =3D t= rue;
>  
> +     amdgpu_vm_check_compute_bug(adev);
> +
>        /* Initialize the buffer mig= ration limit. */
>        if (amdgpu_moverate >=3D = 0)
>            = ;    max_MBps =3D amdgpu_moverate;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c b/drivers/gpu/dr= m/amd/amdgpu/amdgpu_ring.c
> index 7d95435..31aa51d 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
> @@ -153,36 +153,6 @@ void amdgpu_ring_undo(struct amdgpu_ring *rin= g)
>   }
>  
>   /**
> - * amdgpu_ring_check_compute_vm_bug - check whether this ring has com= pute vm bug
> - *
> - * @adev: amdgpu_device pointer
> - * @ring: amdgpu_ring structure holding ring information
> - */
> -static void amdgpu_ring_check_compute_vm_bug(struct amdgpu_device *ad= ev,
> -           &nb= sp;            =              st= ruct amdgpu_ring *ring)
> -{
> -     const struct amdgpu_ip_block *ip_block;
> -
> -     ring->has_compute_vm_bug =3D false;
> -
> -     if (ring->funcs->type !=3D AMDGPU_RING= _TYPE_COMPUTE)
> -           &nb= sp; /* only compute rings */
> -           &nb= sp; return;
> -
> -     ip_block =3D amdgpu_get_ip_block(adev, AMD_I= P_BLOCK_TYPE_GFX);
> -     if (!ip_block)
> -           &nb= sp; return;
> -
> -     /* Compute ring has a VM bug for GFX version= < 7.
> -           And comp= ute ring has a VM bug for GFX 8 MEC firmware version < 673.*/
> -     if (ip_block->version->major <=3D 7= ) {
> -           &nb= sp; ring->has_compute_vm_bug =3D true;
> -     } else if (ip_block->version->major = =3D=3D 8)
> -           &nb= sp; if (adev->gfx.mec_fw_version < 673)
> -           &nb= sp;         ring->has_compute_vm= _bug =3D true;
> -}
> -
> -/**
>    * amdgpu_ring_init - init driver ring struct.
>    *
>    * @adev: amdgpu_device pointer
> @@ -288,8 +258,6 @@ int amdgpu_ring_init(struct amdgpu_device *ade= v, struct amdgpu_ring *ring,
>            = ;    DRM_ERROR("Failed to register debugfs file for rin= gs !\n");
>        }
>  
> -     amdgpu_ring_check_compute_vm_bug(adev, ring)= ;
> -
>        return 0;
>   }
>  
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h b/drivers/gpu/dr= m/amd/amdgpu/amdgpu_ring.h
> index 334307e..ad399c8 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
> @@ -208,9 +208,4 @@ static inline void amdgpu_ring_clear_ring(stru= ct amdgpu_ring *ring)
>  
>   }
>  
> -static inline bool amdgpu_ring_has_compute_vm_bug(struct amdgpu_ring = *ring)
> -{
> -     return ring->has_compute_vm_bug;
> -}
> -
>   #endif
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/= amd/amdgpu/amdgpu_vm.c
> index d4d05a8..6e32748 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -656,6 +656,41 @@ static int amdgpu_vm_alloc_reserved_vmid(stru= ct amdgpu_device *adev,
>        return r;
>   }
>  
> +/**
> + * amdgpu_vm_check_compute_bug - check whether asic has compute v= m bug
> + *
> + * @adev: amdgpu_device pointer
> + */
> +void amdgpu_vm_check_compute_bug(struct amdgpu_device *adev)
> +{
> +     const struct amdgpu_ip_block *ip_block;<= br> > +     bool has_compute_vm_bug;
> +     struct amdgpu_ring *ring;
> +     int i;
> +
> +     has_compute_vm_bug =3D false;
> +
> +     ip_block =3D amdgpu_get_ip_block(adev, A= MD_IP_BLOCK_TYPE_GFX);
> +     if (ip_block) {
> +           = ;  /* Compute has a VM bug for GFX version < 7.
> +           = ;     Compute has a VM bug for GFX 8 MEC firmware versi= on < 673.*/
> +           = ;  if (ip_block->version->major <=3D 7)
> +           = ;          has_compute_vm_bug = =3D true;
> +           = ;  else if (ip_block->version->major =3D=3D 8)
> +           = ;          if (adev->gfx.me= c_fw_version < 673)
> +           = ;            &n= bsp;     has_compute_vm_bug =3D true;
> +     }
> +
> +     for (i =3D 0; i < adev->num_rings;= i++) {
> +           = ;  ring =3D adev->rings[i];
> +           = ;  if (ring->funcs->type =3D=3D AMDGPU_RING_TYPE_COMPUTE)
> +           = ;          /* only compute rin= gs */
> +           = ;          ring->has_comput= e_vm_bug =3D has_compute_vm_bug;
> +           = ;  else
> +           = ;          ring->has_comput= e_vm_bug =3D false;
> +     }
> +}
> +
>   bool amdgpu_vm_need_pipeline_sync(struct amdgpu_ring *ring= ,
>            = ;            &n= bsp;         struct amdgpu_job *job= )
>   {
> @@ -664,8 +699,7 @@ bool amdgpu_vm_need_pipeline_sync(struct amdgp= u_ring *ring,
>        struct amdgpu_vm_id_manager = *id_mgr =3D &adev->vm_manager.id_mgr[vmhub];
>        struct amdgpu_vm_id *id;
>        bool gds_switch_needed;
> -     bool vm_flush_needed =3D job->vm_needs_fl= ush ||
> -           &nb= sp; amdgpu_ring_has_compute_vm_bug(ring);
> +     bool vm_flush_needed =3D job->vm_need= s_flush || ring->has_compute_vm_bug;
>  
>        if (job->vm_id =3D=3D 0)<= br> >            = ;    return false;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/= amd/amdgpu/amdgpu_vm.h
> index 8309bc7..f5dba9c 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> @@ -245,5 +245,6 @@ void amdgpu_vm_adjust_size(struct amdgpu_devic= e *adev, uint64_t vm_size);
>   int amdgpu_vm_ioctl(struct drm_device *dev, void *data, st= ruct drm_file *filp);
>   bool amdgpu_vm_need_pipeline_sync(struct amdgpu_ring *ring= ,
>            = ;            &n= bsp;         struct amdgpu_job *job= );
> +void amdgpu_vm_check_compute_bug(struct amdgpu_device *adev);
>  
>   #endif


--_000_MWHPR1201MB0045729996FF89B47DCD29A3F2F60MWHPR1201MB0045_-- --===============1367496318== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KYW1kLWdmeCBt YWlsaW5nIGxpc3QKYW1kLWdmeEBsaXN0cy5mcmVlZGVza3RvcC5vcmcKaHR0cHM6Ly9saXN0cy5m cmVlZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0aW5mby9hbWQtZ2Z4Cg== --===============1367496318==--