[PATCH] drm/panthor: Don't use virt_to_pfn()

[PATCH] drm/panthor: Don't use virt_to_pfn()

[Steven Price]

virt_to_pfn() isn't available on x86 (except to xen) so breaks
COMPILE_TEST builds. Avoid its use completely by instead storing the
struct page pointer allocated in panthor_device_init() and using
page_to_pfn() instead.

Signed-off-by: Steven Price <steven.price@arm.com>
---
 drivers/gpu/drm/panthor/panthor_device.c | 10 ++++++----
 drivers/gpu/drm/panthor/panthor_device.h |  2 +-
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/panthor/panthor_device.c b/drivers/gpu/drm/panthor/panthor_device.c
index 69deb8e17778..3c30da03fa48 100644
--- a/drivers/gpu/drm/panthor/panthor_device.c
+++ b/drivers/gpu/drm/panthor/panthor_device.c
@@ -154,6 +154,7 @@ int panthor_device_init(struct panthor_device *ptdev)
 {
 	struct resource *res;
 	struct page *p;
+	u32 *dummy_page_virt;
 	int ret;
 
 	ptdev->coherent = device_get_dma_attr(ptdev->base.dev) == DEV_DMA_COHERENT;
@@ -172,9 +173,10 @@ int panthor_device_init(struct panthor_device *ptdev)
 	if (!p)
 		return -ENOMEM;
 
-	ptdev->pm.dummy_latest_flush = page_address(p);
+	ptdev->pm.dummy_latest_flush = p;
+	dummy_page_virt = page_address(p);
 	ret = drmm_add_action_or_reset(&ptdev->base, panthor_device_free_page,
-				       ptdev->pm.dummy_latest_flush);
+				       dummy_page_virt);
 	if (ret)
 		return ret;
 
@@ -184,7 +186,7 @@ int panthor_device_init(struct panthor_device *ptdev)
 	 * happens while the dummy page is mapped. Zero cannot be used because
 	 * that means 'always flush'.
 	 */
-	*ptdev->pm.dummy_latest_flush = 1;
+	*dummy_page_virt = 1;
 
 	INIT_WORK(&ptdev->reset.work, panthor_device_reset_work);
 	ptdev->reset.wq = alloc_ordered_workqueue("panthor-reset-wq", 0);
@@ -353,7 +355,7 @@ static vm_fault_t panthor_mmio_vm_fault(struct vm_fault *vmf)
 		if (active)
 			pfn = __phys_to_pfn(ptdev->phys_addr + CSF_GPU_LATEST_FLUSH_ID);
 		else
-			pfn = virt_to_pfn(ptdev->pm.dummy_latest_flush);
+			pfn = page_to_pfn(ptdev->pm.dummy_latest_flush);
 		break;
 
 	default:
diff --git a/drivers/gpu/drm/panthor/panthor_device.h b/drivers/gpu/drm/panthor/panthor_device.h
index 51c9d61b6796..c84c27dcc92c 100644
--- a/drivers/gpu/drm/panthor/panthor_device.h
+++ b/drivers/gpu/drm/panthor/panthor_device.h
@@ -160,7 +160,7 @@ struct panthor_device {
 		 * Used to replace the real LATEST_FLUSH page when the GPU
 		 * is suspended.
 		 */
-		u32 *dummy_latest_flush;
+		struct page *dummy_latest_flush;
 	} pm;
 };
 
-- 
2.39.2

Re: [PATCH] drm/panthor: Don't use virt_to_pfn()

[Boris Brezillon]

On Mon, 18 Mar 2024 14:51:19 +0000
Steven Price <steven.price@arm.com> wrote:

> virt_to_pfn() isn't available on x86 (except to xen) so breaks
> COMPILE_TEST builds. Avoid its use completely by instead storing the
> struct page pointer allocated in panthor_device_init() and using
> page_to_pfn() instead.
> 
> Signed-off-by: Steven Price <steven.price@arm.com>

Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>

Thanks,

Boris

> ---
>  drivers/gpu/drm/panthor/panthor_device.c | 10 ++++++----
>  drivers/gpu/drm/panthor/panthor_device.h |  2 +-
>  2 files changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_device.c b/drivers/gpu/drm/panthor/panthor_device.c
> index 69deb8e17778..3c30da03fa48 100644
> --- a/drivers/gpu/drm/panthor/panthor_device.c
> +++ b/drivers/gpu/drm/panthor/panthor_device.c
> @@ -154,6 +154,7 @@ int panthor_device_init(struct panthor_device *ptdev)
>  {
>  	struct resource *res;
>  	struct page *p;
> +	u32 *dummy_page_virt;
>  	int ret;
>  
>  	ptdev->coherent = device_get_dma_attr(ptdev->base.dev) == DEV_DMA_COHERENT;
> @@ -172,9 +173,10 @@ int panthor_device_init(struct panthor_device *ptdev)
>  	if (!p)
>  		return -ENOMEM;
>  
> -	ptdev->pm.dummy_latest_flush = page_address(p);
> +	ptdev->pm.dummy_latest_flush = p;
> +	dummy_page_virt = page_address(p);
>  	ret = drmm_add_action_or_reset(&ptdev->base, panthor_device_free_page,
> -				       ptdev->pm.dummy_latest_flush);
> +				       dummy_page_virt);
>  	if (ret)
>  		return ret;
>  
> @@ -184,7 +186,7 @@ int panthor_device_init(struct panthor_device *ptdev)
>  	 * happens while the dummy page is mapped. Zero cannot be used because
>  	 * that means 'always flush'.
>  	 */
> -	*ptdev->pm.dummy_latest_flush = 1;
> +	*dummy_page_virt = 1;
>  
>  	INIT_WORK(&ptdev->reset.work, panthor_device_reset_work);
>  	ptdev->reset.wq = alloc_ordered_workqueue("panthor-reset-wq", 0);
> @@ -353,7 +355,7 @@ static vm_fault_t panthor_mmio_vm_fault(struct vm_fault *vmf)
>  		if (active)
>  			pfn = __phys_to_pfn(ptdev->phys_addr + CSF_GPU_LATEST_FLUSH_ID);
>  		else
> -			pfn = virt_to_pfn(ptdev->pm.dummy_latest_flush);
> +			pfn = page_to_pfn(ptdev->pm.dummy_latest_flush);
>  		break;
>  
>  	default:
> diff --git a/drivers/gpu/drm/panthor/panthor_device.h b/drivers/gpu/drm/panthor/panthor_device.h
> index 51c9d61b6796..c84c27dcc92c 100644
> --- a/drivers/gpu/drm/panthor/panthor_device.h
> +++ b/drivers/gpu/drm/panthor/panthor_device.h
> @@ -160,7 +160,7 @@ struct panthor_device {
>  		 * Used to replace the real LATEST_FLUSH page when the GPU
>  		 * is suspended.
>  		 */
> -		u32 *dummy_latest_flush;
> +		struct page *dummy_latest_flush;
>  	} pm;
>  };
>  

Re: [PATCH] drm/panthor: Don't use virt_to_pfn()

[Robin Murphy]

On 18/03/2024 2:51 pm, Steven Price wrote:
> virt_to_pfn() isn't available on x86 (except to xen) so breaks
> COMPILE_TEST builds. Avoid its use completely by instead storing the
> struct page pointer allocated in panthor_device_init() and using
> page_to_pfn() instead.
> 
> Signed-off-by: Steven Price <steven.price@arm.com>
> ---
>   drivers/gpu/drm/panthor/panthor_device.c | 10 ++++++----
>   drivers/gpu/drm/panthor/panthor_device.h |  2 +-
>   2 files changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_device.c b/drivers/gpu/drm/panthor/panthor_device.c
> index 69deb8e17778..3c30da03fa48 100644
> --- a/drivers/gpu/drm/panthor/panthor_device.c
> +++ b/drivers/gpu/drm/panthor/panthor_device.c
> @@ -154,6 +154,7 @@ int panthor_device_init(struct panthor_device *ptdev)
>   {
>   	struct resource *res;
>   	struct page *p;
> +	u32 *dummy_page_virt;
>   	int ret;
>   
>   	ptdev->coherent = device_get_dma_attr(ptdev->base.dev) == DEV_DMA_COHERENT;
> @@ -172,9 +173,10 @@ int panthor_device_init(struct panthor_device *ptdev)
>   	if (!p)
>   		return -ENOMEM;
>   
> -	ptdev->pm.dummy_latest_flush = page_address(p);
> +	ptdev->pm.dummy_latest_flush = p;
> +	dummy_page_virt = page_address(p);
>   	ret = drmm_add_action_or_reset(&ptdev->base, panthor_device_free_page,
> -				       ptdev->pm.dummy_latest_flush);
> +				       dummy_page_virt);

Nit: I was about to say I'd be inclined to switch the callback to 
__free_page() instead, but then I realise there's no real need to be 
reinventing that in the first place:

	dummy_page_virt = (void *)devm_get_free_pages(ptdev->base.dev,
					GFP_KERNEL | GFP_ZERO, 0);
	if (!dummy_page_virt)
		return -ENOMEM;

	ptdev->pm.dummy_latest_flush = virt_to_page(dummy_page_virt);

Cheers,
Robin.

>   	if (ret)
>   		return ret;
>   
> @@ -184,7 +186,7 @@ int panthor_device_init(struct panthor_device *ptdev)
>   	 * happens while the dummy page is mapped. Zero cannot be used because
>   	 * that means 'always flush'.
>   	 */
> -	*ptdev->pm.dummy_latest_flush = 1;
> +	*dummy_page_virt = 1;
>   
>   	INIT_WORK(&ptdev->reset.work, panthor_device_reset_work);
>   	ptdev->reset.wq = alloc_ordered_workqueue("panthor-reset-wq", 0);
> @@ -353,7 +355,7 @@ static vm_fault_t panthor_mmio_vm_fault(struct vm_fault *vmf)
>   		if (active)
>   			pfn = __phys_to_pfn(ptdev->phys_addr + CSF_GPU_LATEST_FLUSH_ID);
>   		else
> -			pfn = virt_to_pfn(ptdev->pm.dummy_latest_flush);
> +			pfn = page_to_pfn(ptdev->pm.dummy_latest_flush);
>   		break;
>   
>   	default:
> diff --git a/drivers/gpu/drm/panthor/panthor_device.h b/drivers/gpu/drm/panthor/panthor_device.h
> index 51c9d61b6796..c84c27dcc92c 100644
> --- a/drivers/gpu/drm/panthor/panthor_device.h
> +++ b/drivers/gpu/drm/panthor/panthor_device.h
> @@ -160,7 +160,7 @@ struct panthor_device {
>   		 * Used to replace the real LATEST_FLUSH page when the GPU
>   		 * is suspended.
>   		 */
> -		u32 *dummy_latest_flush;
> +		struct page *dummy_latest_flush;
>   	} pm;
>   };
>   

Re: [PATCH] drm/panthor: Don't use virt_to_pfn()

[Boris Brezillon]

On Mon, 18 Mar 2024 15:11:42 +0000
Robin Murphy <robin.murphy@arm.com> wrote:

> On 18/03/2024 2:51 pm, Steven Price wrote:
> > virt_to_pfn() isn't available on x86 (except to xen) so breaks
> > COMPILE_TEST builds. Avoid its use completely by instead storing the
> > struct page pointer allocated in panthor_device_init() and using
> > page_to_pfn() instead.
> > 
> > Signed-off-by: Steven Price <steven.price@arm.com>
> > ---
> >   drivers/gpu/drm/panthor/panthor_device.c | 10 ++++++----
> >   drivers/gpu/drm/panthor/panthor_device.h |  2 +-
> >   2 files changed, 7 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/panthor/panthor_device.c b/drivers/gpu/drm/panthor/panthor_device.c
> > index 69deb8e17778..3c30da03fa48 100644
> > --- a/drivers/gpu/drm/panthor/panthor_device.c
> > +++ b/drivers/gpu/drm/panthor/panthor_device.c
> > @@ -154,6 +154,7 @@ int panthor_device_init(struct panthor_device *ptdev)
> >   {
> >   	struct resource *res;
> >   	struct page *p;
> > +	u32 *dummy_page_virt;
> >   	int ret;
> >   
> >   	ptdev->coherent = device_get_dma_attr(ptdev->base.dev) == DEV_DMA_COHERENT;
> > @@ -172,9 +173,10 @@ int panthor_device_init(struct panthor_device *ptdev)
> >   	if (!p)
> >   		return -ENOMEM;
> >   
> > -	ptdev->pm.dummy_latest_flush = page_address(p);
> > +	ptdev->pm.dummy_latest_flush = p;
> > +	dummy_page_virt = page_address(p);
> >   	ret = drmm_add_action_or_reset(&ptdev->base, panthor_device_free_page,
> > -				       ptdev->pm.dummy_latest_flush);
> > +				       dummy_page_virt);  
> 
> Nit: I was about to say I'd be inclined to switch the callback to 
> __free_page() instead, but then I realise there's no real need to be 
> reinventing that in the first place:
> 
> 	dummy_page_virt = (void *)devm_get_free_pages(ptdev->base.dev,
> 					GFP_KERNEL | GFP_ZERO, 0);

IIRC, the drm_device object might outlive the device it's been created
from if a process holds on the DRM dev FD after an unplug happened,
which can happen if panthor_device_unplug() is called from the reset
path (when a reset fails).

> 	if (!dummy_page_virt)
> 		return -ENOMEM;
> 
> 	ptdev->pm.dummy_latest_flush = virt_to_page(dummy_page_virt);
> 
> Cheers,
> Robin.
> 
> >   	if (ret)
> >   		return ret;
> >   
> > @@ -184,7 +186,7 @@ int panthor_device_init(struct panthor_device *ptdev)
> >   	 * happens while the dummy page is mapped. Zero cannot be used because
> >   	 * that means 'always flush'.
> >   	 */
> > -	*ptdev->pm.dummy_latest_flush = 1;
> > +	*dummy_page_virt = 1;
> >   
> >   	INIT_WORK(&ptdev->reset.work, panthor_device_reset_work);
> >   	ptdev->reset.wq = alloc_ordered_workqueue("panthor-reset-wq", 0);
> > @@ -353,7 +355,7 @@ static vm_fault_t panthor_mmio_vm_fault(struct vm_fault *vmf)
> >   		if (active)
> >   			pfn = __phys_to_pfn(ptdev->phys_addr + CSF_GPU_LATEST_FLUSH_ID);
> >   		else
> > -			pfn = virt_to_pfn(ptdev->pm.dummy_latest_flush);
> > +			pfn = page_to_pfn(ptdev->pm.dummy_latest_flush);
> >   		break;
> >   
> >   	default:
> > diff --git a/drivers/gpu/drm/panthor/panthor_device.h b/drivers/gpu/drm/panthor/panthor_device.h
> > index 51c9d61b6796..c84c27dcc92c 100644
> > --- a/drivers/gpu/drm/panthor/panthor_device.h
> > +++ b/drivers/gpu/drm/panthor/panthor_device.h
> > @@ -160,7 +160,7 @@ struct panthor_device {
> >   		 * Used to replace the real LATEST_FLUSH page when the GPU
> >   		 * is suspended.
> >   		 */
> > -		u32 *dummy_latest_flush;
> > +		struct page *dummy_latest_flush;
> >   	} pm;
> >   };
> >     

Re: [PATCH] drm/panthor: Don't use virt_to_pfn()

[Boris Brezillon]

On Mon, 18 Mar 2024 15:11:42 +0000
Robin Murphy <robin.murphy@arm.com> wrote:

> On 18/03/2024 2:51 pm, Steven Price wrote:
> > virt_to_pfn() isn't available on x86 (except to xen) so breaks
> > COMPILE_TEST builds. Avoid its use completely by instead storing the
> > struct page pointer allocated in panthor_device_init() and using
> > page_to_pfn() instead.
> > 
> > Signed-off-by: Steven Price <steven.price@arm.com>
> > ---
> >   drivers/gpu/drm/panthor/panthor_device.c | 10 ++++++----
> >   drivers/gpu/drm/panthor/panthor_device.h |  2 +-
> >   2 files changed, 7 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/panthor/panthor_device.c b/drivers/gpu/drm/panthor/panthor_device.c
> > index 69deb8e17778..3c30da03fa48 100644
> > --- a/drivers/gpu/drm/panthor/panthor_device.c
> > +++ b/drivers/gpu/drm/panthor/panthor_device.c
> > @@ -154,6 +154,7 @@ int panthor_device_init(struct panthor_device *ptdev)
> >   {
> >   	struct resource *res;
> >   	struct page *p;
> > +	u32 *dummy_page_virt;
> >   	int ret;
> >   
> >   	ptdev->coherent = device_get_dma_attr(ptdev->base.dev) == DEV_DMA_COHERENT;
> > @@ -172,9 +173,10 @@ int panthor_device_init(struct panthor_device *ptdev)
> >   	if (!p)
> >   		return -ENOMEM;
> >   
> > -	ptdev->pm.dummy_latest_flush = page_address(p);
> > +	ptdev->pm.dummy_latest_flush = p;
> > +	dummy_page_virt = page_address(p);
> >   	ret = drmm_add_action_or_reset(&ptdev->base, panthor_device_free_page,
> > -				       ptdev->pm.dummy_latest_flush);
> > +				       dummy_page_virt);  
> 
> Nit: I was about to say I'd be inclined to switch the callback to 
> __free_page() instead, but then I realise there's no real need to be 
> reinventing that in the first place:
> 
> 	dummy_page_virt = (void *)devm_get_free_pages(ptdev->base.dev,
> 					GFP_KERNEL | GFP_ZERO, 0);
> 	if (!dummy_page_virt)
> 		return -ENOMEM;
> 
> 	ptdev->pm.dummy_latest_flush = virt_to_page(dummy_page_virt);

Queued to drm-misc-next after turning the free_page() into a
__free_page() in panthor_device_free_page(). If other drivers need to
allocate pages that are attached to the drm_device, we might want to
consider adding a drmm_get_free_pages() helper to drm_managed.h, but I
think we're the only ones to need that right now.

> 
> Cheers,
> Robin.
> 
> >   	if (ret)
> >   		return ret;
> >   
> > @@ -184,7 +186,7 @@ int panthor_device_init(struct panthor_device *ptdev)
> >   	 * happens while the dummy page is mapped. Zero cannot be used because
> >   	 * that means 'always flush'.
> >   	 */
> > -	*ptdev->pm.dummy_latest_flush = 1;
> > +	*dummy_page_virt = 1;
> >   
> >   	INIT_WORK(&ptdev->reset.work, panthor_device_reset_work);
> >   	ptdev->reset.wq = alloc_ordered_workqueue("panthor-reset-wq", 0);
> > @@ -353,7 +355,7 @@ static vm_fault_t panthor_mmio_vm_fault(struct vm_fault *vmf)
> >   		if (active)
> >   			pfn = __phys_to_pfn(ptdev->phys_addr + CSF_GPU_LATEST_FLUSH_ID);
> >   		else
> > -			pfn = virt_to_pfn(ptdev->pm.dummy_latest_flush);
> > +			pfn = page_to_pfn(ptdev->pm.dummy_latest_flush);
> >   		break;
> >   
> >   	default:
> > diff --git a/drivers/gpu/drm/panthor/panthor_device.h b/drivers/gpu/drm/panthor/panthor_device.h
> > index 51c9d61b6796..c84c27dcc92c 100644
> > --- a/drivers/gpu/drm/panthor/panthor_device.h
> > +++ b/drivers/gpu/drm/panthor/panthor_device.h
> > @@ -160,7 +160,7 @@ struct panthor_device {
> >   		 * Used to replace the real LATEST_FLUSH page when the GPU
> >   		 * is suspended.
> >   		 */
> > -		u32 *dummy_latest_flush;
> > +		struct page *dummy_latest_flush;
> >   	} pm;
> >   };
> >