diff mbox series

[v3,7/8] drm/panfrost: Flush and disable address space when freeing page tables

Message ID 20190826223317.28509-8-robh@kernel.org (mailing list archive)
State New, archived
Headers show
Series panfrost: Locking and runtime PM fixes | expand

Commit Message

Rob Herring (Arm) Aug. 26, 2019, 10:33 p.m. UTC 
Currently, page tables are freed without disabling the address space first.
This probably is fine as we'll switch to new page tables when the address
space is allocated again and runtime PM suspend will reset the GPU
clearing the registers. However, it's better to clean up after ourselves.
There is also a problem that we could be accessing the h/w in
tlb_inv_context() when suspended.

Rework the disable code to make sure we flush caches/TLBs and disable the
address space before freeing the page tables if we are not suspended. As
the tlb_inv_context() hook is only called when freeing the page tables and
we do a flush before disabling the AS, lets remove the flush from
tlb_inv_context and avoid any runtime PM issues.

Fixes: 7282f7645d06 ("drm/panfrost: Implement per FD address spaces")
Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com>
Cc: Steven Price <steven.price@arm.com>
Cc: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
Cc: David Airlie <airlied@linux.ie>
Cc: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Rob Herring <robh@kernel.org>
---
v3:
 - New patch replacing "drm/panfrost: Remove unnecessary flushing from tlb_inv_context"

 drivers/gpu/drm/panfrost/panfrost_mmu.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

--
2.20.1

Comments

Robin Murphy Aug. 27, 2019, 11:24 a.m. UTC | #1 
On 26/08/2019 23:33, Rob Herring wrote:
> Currently, page tables are freed without disabling the address space first.
> This probably is fine as we'll switch to new page tables when the address
> space is allocated again and runtime PM suspend will reset the GPU
> clearing the registers. However, it's better to clean up after ourselves.
> There is also a problem that we could be accessing the h/w in
> tlb_inv_context() when suspended.
> 
> Rework the disable code to make sure we flush caches/TLBs and disable the
> address space before freeing the page tables if we are not suspended. As
> the tlb_inv_context() hook is only called when freeing the page tables and
> we do a flush before disabling the AS, lets remove the flush from
> tlb_inv_context and avoid any runtime PM issues.

Reviewed-by: Robin Murphy <robin.murphy@arm.com>

> Fixes: 7282f7645d06 ("drm/panfrost: Implement per FD address spaces")
> Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> Cc: Steven Price <steven.price@arm.com>
> Cc: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Signed-off-by: Rob Herring <robh@kernel.org>
> ---
> v3:
>   - New patch replacing "drm/panfrost: Remove unnecessary flushing from tlb_inv_context"
> 
>   drivers/gpu/drm/panfrost/panfrost_mmu.c | 16 +++++++++-------
>   1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> index d1ebde3327fe..387d830cb7cf 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> @@ -129,8 +129,10 @@ static void panfrost_mmu_enable(struct panfrost_device *pfdev, struct panfrost_m
>   	write_cmd(pfdev, as_nr, AS_COMMAND_UPDATE);
>   }
> 
> -static void mmu_disable(struct panfrost_device *pfdev, u32 as_nr)
> +static void panfrost_mmu_disable(struct panfrost_device *pfdev, u32 as_nr)
>   {
> +	mmu_hw_do_operation_locked(pfdev, as_nr, 0, ~0UL, AS_COMMAND_FLUSH_MEM);
> +
>   	mmu_write(pfdev, AS_TRANSTAB_LO(as_nr), 0);
>   	mmu_write(pfdev, AS_TRANSTAB_HI(as_nr), 0);
> 
> @@ -321,11 +323,7 @@ void panfrost_mmu_unmap(struct panfrost_gem_object *bo)
>   }
> 
>   static void mmu_tlb_inv_context_s1(void *cookie)
> -{
> -	struct panfrost_file_priv *priv = cookie;
> -
> -	mmu_hw_do_operation(priv->pfdev, &priv->mmu, 0, ~0UL, AS_COMMAND_FLUSH_MEM);
> -}
> +{}
> 
>   static void mmu_tlb_inv_range_nosync(unsigned long iova, size_t size,
>   				     size_t granule, bool leaf, void *cookie)
> @@ -374,6 +372,11 @@ void panfrost_mmu_pgtable_free(struct panfrost_file_priv *priv)
> 
>   	spin_lock(&pfdev->as_lock);
>   	if (mmu->as >= 0) {
> +		pm_runtime_get_noresume(pfdev->dev);
> +		if (pm_runtime_active(pfdev->dev))
> +			panfrost_mmu_disable(pfdev, mmu->as);
> +		pm_runtime_put_autosuspend(pfdev->dev);
> +
>   		clear_bit(mmu->as, &pfdev->as_alloc_mask);
>   		clear_bit(mmu->as, &pfdev->as_in_use_mask);
>   		list_del(&mmu->list);
> @@ -618,5 +621,4 @@ int panfrost_mmu_init(struct panfrost_device *pfdev)
>   void panfrost_mmu_fini(struct panfrost_device *pfdev)
>   {
>   	mmu_write(pfdev, MMU_INT_MASK, 0);
> -	mmu_disable(pfdev, 0);
>   }
> --
> 2.20.1
>
Steven Price Aug. 28, 2019, 10:55 a.m. UTC | #2 
On 26/08/2019 23:33, Rob Herring wrote:
> Currently, page tables are freed without disabling the address space first.
> This probably is fine as we'll switch to new page tables when the address
> space is allocated again and runtime PM suspend will reset the GPU
> clearing the registers. However, it's better to clean up after ourselves.
> There is also a problem that we could be accessing the h/w in
> tlb_inv_context() when suspended.
> 
> Rework the disable code to make sure we flush caches/TLBs and disable the
> address space before freeing the page tables if we are not suspended. As
> the tlb_inv_context() hook is only called when freeing the page tables and
> we do a flush before disabling the AS, lets remove the flush from
> tlb_inv_context and avoid any runtime PM issues.
> 
> Fixes: 7282f7645d06 ("drm/panfrost: Implement per FD address spaces")
> Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> Cc: Steven Price <steven.price@arm.com>
> Cc: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Signed-off-by: Rob Herring <robh@kernel.org>
> ---
> v3:
>  - New patch replacing "drm/panfrost: Remove unnecessary flushing from tlb_inv_context"
> 
>  drivers/gpu/drm/panfrost/panfrost_mmu.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> index d1ebde3327fe..387d830cb7cf 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> @@ -129,8 +129,10 @@ static void panfrost_mmu_enable(struct panfrost_device *pfdev, struct panfrost_m
>  	write_cmd(pfdev, as_nr, AS_COMMAND_UPDATE);
>  }
> 
> -static void mmu_disable(struct panfrost_device *pfdev, u32 as_nr)
> +static void panfrost_mmu_disable(struct panfrost_device *pfdev, u32 as_nr)
>  {
> +	mmu_hw_do_operation_locked(pfdev, as_nr, 0, ~0UL, AS_COMMAND_FLUSH_MEM);
> +
>  	mmu_write(pfdev, AS_TRANSTAB_LO(as_nr), 0);
>  	mmu_write(pfdev, AS_TRANSTAB_HI(as_nr), 0);
> 

At the end of this function we have:

| 	write_cmd(pfdev, as_nr, AS_COMMAND_UPDATE);

which should negate the need for AS_COMMAND_FLUSH_MEM as well. However
one thing I have just noticed is that write_cmd() doesn't wait for
AS_ACTIVE to be cleared. This means that the GPU has accepted the
command but might not have finished the flush.

When freeing page tables we obviously need to wait for the MMU flush to
complete. The extra mmu_hw_do_operation_locked() 'fixes' this partly
because there's a back-to-back set of MMU commands so the second one
will be blocked until AS_COMMAND_FLUSH_MEM has completed, but also
mmu_hw_do_operation() waits for the flush to complete.

I'm not really sure why mmu_enable()/mmu_disable() have bare calls to
write_cmd - could they use mmu_hw_do_operation_locked() instead?

> @@ -321,11 +323,7 @@ void panfrost_mmu_unmap(struct panfrost_gem_object *bo)
>  }
> 
>  static void mmu_tlb_inv_context_s1(void *cookie)
> -{
> -	struct panfrost_file_priv *priv = cookie;
> -
> -	mmu_hw_do_operation(priv->pfdev, &priv->mmu, 0, ~0UL, AS_COMMAND_FLUSH_MEM);
> -}
> +{}
> 
>  static void mmu_tlb_inv_range_nosync(unsigned long iova, size_t size,
>  				     size_t granule, bool leaf, void *cookie)
> @@ -374,6 +372,11 @@ void panfrost_mmu_pgtable_free(struct panfrost_file_priv *priv)
> 
>  	spin_lock(&pfdev->as_lock);
>  	if (mmu->as >= 0) {
> +		pm_runtime_get_noresume(pfdev->dev);
> +		if (pm_runtime_active(pfdev->dev))
> +			panfrost_mmu_disable(pfdev, mmu->as);
> +		pm_runtime_put_autosuspend(pfdev->dev);
> +
>  		clear_bit(mmu->as, &pfdev->as_alloc_mask);
>  		clear_bit(mmu->as, &pfdev->as_in_use_mask);
>  		list_del(&mmu->list);
> @@ -618,5 +621,4 @@ int panfrost_mmu_init(struct panfrost_device *pfdev)
>  void panfrost_mmu_fini(struct panfrost_device *pfdev)
>  {
>  	mmu_write(pfdev, MMU_INT_MASK, 0);
> -	mmu_disable(pfdev, 0);
>  }
> --
> 2.20.1
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
Rob Herring (Arm) Aug. 28, 2019, 12:35 p.m. UTC | #3 
On Wed, Aug 28, 2019 at 5:55 AM Steven Price <steven.price@arm.com> wrote:
>
> On 26/08/2019 23:33, Rob Herring wrote:
> > Currently, page tables are freed without disabling the address space first.
> > This probably is fine as we'll switch to new page tables when the address
> > space is allocated again and runtime PM suspend will reset the GPU
> > clearing the registers. However, it's better to clean up after ourselves.
> > There is also a problem that we could be accessing the h/w in
> > tlb_inv_context() when suspended.
> >
> > Rework the disable code to make sure we flush caches/TLBs and disable the
> > address space before freeing the page tables if we are not suspended. As
> > the tlb_inv_context() hook is only called when freeing the page tables and
> > we do a flush before disabling the AS, lets remove the flush from
> > tlb_inv_context and avoid any runtime PM issues.
> >
> > Fixes: 7282f7645d06 ("drm/panfrost: Implement per FD address spaces")
> > Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> > Cc: Steven Price <steven.price@arm.com>
> > Cc: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
> > Cc: David Airlie <airlied@linux.ie>
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > Signed-off-by: Rob Herring <robh@kernel.org>
> > ---
> > v3:
> >  - New patch replacing "drm/panfrost: Remove unnecessary flushing from tlb_inv_context"
> >
> >  drivers/gpu/drm/panfrost/panfrost_mmu.c | 16 +++++++++-------
> >  1 file changed, 9 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> > index d1ebde3327fe..387d830cb7cf 100644
> > --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
> > +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> > @@ -129,8 +129,10 @@ static void panfrost_mmu_enable(struct panfrost_device *pfdev, struct panfrost_m
> >       write_cmd(pfdev, as_nr, AS_COMMAND_UPDATE);
> >  }
> >
> > -static void mmu_disable(struct panfrost_device *pfdev, u32 as_nr)
> > +static void panfrost_mmu_disable(struct panfrost_device *pfdev, u32 as_nr)
> >  {
> > +     mmu_hw_do_operation_locked(pfdev, as_nr, 0, ~0UL, AS_COMMAND_FLUSH_MEM);
> > +
> >       mmu_write(pfdev, AS_TRANSTAB_LO(as_nr), 0);
> >       mmu_write(pfdev, AS_TRANSTAB_HI(as_nr), 0);
> >
>
> At the end of this function we have:
>
> |       write_cmd(pfdev, as_nr, AS_COMMAND_UPDATE);
>
> which should negate the need for AS_COMMAND_FLUSH_MEM as well. However
> one thing I have just noticed is that write_cmd() doesn't wait for
> AS_ACTIVE to be cleared. This means that the GPU has accepted the
> command but might not have finished the flush.
>
> When freeing page tables we obviously need to wait for the MMU flush to
> complete. The extra mmu_hw_do_operation_locked() 'fixes' this partly
> because there's a back-to-back set of MMU commands so the second one
> will be blocked until AS_COMMAND_FLUSH_MEM has completed, but also
> mmu_hw_do_operation() waits for the flush to complete.

I've copied what's in kbase which doesn't wait AFAICT.

> I'm not really sure why mmu_enable()/mmu_disable() have bare calls to
> write_cmd - could they use mmu_hw_do_operation_locked() instead?

mmu_hw_do_operation_locked() also does a lock_region. I guess that
would be harmless?

Rob
Steven Price Aug. 28, 2019, 1:12 p.m. UTC | #4 
On 28/08/2019 13:35, Rob Herring wrote:
> On Wed, Aug 28, 2019 at 5:55 AM Steven Price <steven.price@arm.com> wrote:
>>
>> On 26/08/2019 23:33, Rob Herring wrote:
>>> Currently, page tables are freed without disabling the address space first.
>>> This probably is fine as we'll switch to new page tables when the address
>>> space is allocated again and runtime PM suspend will reset the GPU
>>> clearing the registers. However, it's better to clean up after ourselves.
>>> There is also a problem that we could be accessing the h/w in
>>> tlb_inv_context() when suspended.
>>>
>>> Rework the disable code to make sure we flush caches/TLBs and disable the
>>> address space before freeing the page tables if we are not suspended. As
>>> the tlb_inv_context() hook is only called when freeing the page tables and
>>> we do a flush before disabling the AS, lets remove the flush from
>>> tlb_inv_context and avoid any runtime PM issues.
>>>
>>> Fixes: 7282f7645d06 ("drm/panfrost: Implement per FD address spaces")
>>> Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com>
>>> Cc: Steven Price <steven.price@arm.com>
>>> Cc: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
>>> Cc: David Airlie <airlied@linux.ie>
>>> Cc: Daniel Vetter <daniel@ffwll.ch>
>>> Signed-off-by: Rob Herring <robh@kernel.org>
>>> ---
>>> v3:
>>>  - New patch replacing "drm/panfrost: Remove unnecessary flushing from tlb_inv_context"
>>>
>>>  drivers/gpu/drm/panfrost/panfrost_mmu.c | 16 +++++++++-------
>>>  1 file changed, 9 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
>>> index d1ebde3327fe..387d830cb7cf 100644
>>> --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
>>> +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
>>> @@ -129,8 +129,10 @@ static void panfrost_mmu_enable(struct panfrost_device *pfdev, struct panfrost_m
>>>       write_cmd(pfdev, as_nr, AS_COMMAND_UPDATE);
>>>  }
>>>
>>> -static void mmu_disable(struct panfrost_device *pfdev, u32 as_nr)
>>> +static void panfrost_mmu_disable(struct panfrost_device *pfdev, u32 as_nr)
>>>  {
>>> +     mmu_hw_do_operation_locked(pfdev, as_nr, 0, ~0UL, AS_COMMAND_FLUSH_MEM);
>>> +
>>>       mmu_write(pfdev, AS_TRANSTAB_LO(as_nr), 0);
>>>       mmu_write(pfdev, AS_TRANSTAB_HI(as_nr), 0);
>>>
>>
>> At the end of this function we have:
>>
>> |       write_cmd(pfdev, as_nr, AS_COMMAND_UPDATE);
>>
>> which should negate the need for AS_COMMAND_FLUSH_MEM as well. However
>> one thing I have just noticed is that write_cmd() doesn't wait for
>> AS_ACTIVE to be cleared. This means that the GPU has accepted the
>> command but might not have finished the flush.
>>
>> When freeing page tables we obviously need to wait for the MMU flush to
>> complete. The extra mmu_hw_do_operation_locked() 'fixes' this partly
>> because there's a back-to-back set of MMU commands so the second one
>> will be blocked until AS_COMMAND_FLUSH_MEM has completed, but also
>> mmu_hw_do_operation() waits for the flush to complete.
> 
> I've copied what's in kbase which doesn't wait AFAICT.

Hmm, well it does in that it calls kbase_mmu_flush_invalidate_noretain()
before disable_as() [1]. But that is equivalent to what you have above.

Oh well, I guess matching kbase is safer here, even if kbase seems to be
doing extra work.

[1]
https://gitlab.freedesktop.org/panfrost/mali_kbase/blob/master/driver/product/kernel/drivers/gpu/arm/midgard/mali_kbase_mmu.c#L1568

>> I'm not really sure why mmu_enable()/mmu_disable() have bare calls to
>> write_cmd - could they use mmu_hw_do_operation_locked() instead?
> 
> mmu_hw_do_operation_locked() also does a lock_region. I guess that
> would be harmless?

Yes, locking regions should be harmless - it just prevents the MMU from
translating the locked region. However you do need to ensure that the
region is unlocked again and UPDATE isn't specified to perform the UNLOCK.

So actually all things considered your patch looks fine - it matches
kbase (even if I don't fully understand why kbase is doing it that way),
and fixes a bug. So:

Reviewed-by: Steven Price <steven.price@arm.com>

(and sorry for the noise!)

Steve
diff mbox series

Patch

diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
index d1ebde3327fe..387d830cb7cf 100644
--- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
+++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
@@ -129,8 +129,10 @@  static void panfrost_mmu_enable(struct panfrost_device *pfdev, struct panfrost_m
 	write_cmd(pfdev, as_nr, AS_COMMAND_UPDATE);
 }

-static void mmu_disable(struct panfrost_device *pfdev, u32 as_nr)
+static void panfrost_mmu_disable(struct panfrost_device *pfdev, u32 as_nr)
 {
+	mmu_hw_do_operation_locked(pfdev, as_nr, 0, ~0UL, AS_COMMAND_FLUSH_MEM);
+
 	mmu_write(pfdev, AS_TRANSTAB_LO(as_nr), 0);
 	mmu_write(pfdev, AS_TRANSTAB_HI(as_nr), 0);

@@ -321,11 +323,7 @@  void panfrost_mmu_unmap(struct panfrost_gem_object *bo)
 }

 static void mmu_tlb_inv_context_s1(void *cookie)
-{
-	struct panfrost_file_priv *priv = cookie;
-
-	mmu_hw_do_operation(priv->pfdev, &priv->mmu, 0, ~0UL, AS_COMMAND_FLUSH_MEM);
-}
+{}

 static void mmu_tlb_inv_range_nosync(unsigned long iova, size_t size,
 				     size_t granule, bool leaf, void *cookie)
@@ -374,6 +372,11 @@  void panfrost_mmu_pgtable_free(struct panfrost_file_priv *priv)

 	spin_lock(&pfdev->as_lock);
 	if (mmu->as >= 0) {
+		pm_runtime_get_noresume(pfdev->dev);
+		if (pm_runtime_active(pfdev->dev))
+			panfrost_mmu_disable(pfdev, mmu->as);
+		pm_runtime_put_autosuspend(pfdev->dev);
+
 		clear_bit(mmu->as, &pfdev->as_alloc_mask);
 		clear_bit(mmu->as, &pfdev->as_in_use_mask);
 		list_del(&mmu->list);
@@ -618,5 +621,4 @@  int panfrost_mmu_init(struct panfrost_device *pfdev)
 void panfrost_mmu_fini(struct panfrost_device *pfdev)
 {
 	mmu_write(pfdev, MMU_INT_MASK, 0);
-	mmu_disable(pfdev, 0);
 }