* [PATCH] drm/panthor: Don't use virt_to_pfn()
@ 2024-03-18 14:51 Steven Price
2024-03-18 15:09 ` Boris Brezillon
2024-03-18 15:11 ` Robin Murphy
0 siblings, 2 replies; 5+ messages in thread
From: Steven Price @ 2024-03-18 14:51 UTC (permalink / raw)
To: Boris Brezillon, Liviu Dudau; +Cc: Steven Price, dri-devel, Robin Murphy
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
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] drm/panthor: Don't use virt_to_pfn()
2024-03-18 14:51 [PATCH] drm/panthor: Don't use virt_to_pfn() Steven Price
@ 2024-03-18 15:09 ` Boris Brezillon
2024-03-18 15:11 ` Robin Murphy
1 sibling, 0 replies; 5+ messages in thread
From: Boris Brezillon @ 2024-03-18 15:09 UTC (permalink / raw)
To: Steven Price; +Cc: Liviu Dudau, dri-devel, Robin Murphy
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;
> };
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] drm/panthor: Don't use virt_to_pfn()
2024-03-18 14:51 [PATCH] drm/panthor: Don't use virt_to_pfn() Steven Price
2024-03-18 15:09 ` Boris Brezillon
@ 2024-03-18 15:11 ` Robin Murphy
2024-03-18 15:25 ` Boris Brezillon
2024-03-25 10:05 ` Boris Brezillon
1 sibling, 2 replies; 5+ messages in thread
From: Robin Murphy @ 2024-03-18 15:11 UTC (permalink / raw)
To: Steven Price, Boris Brezillon, Liviu Dudau; +Cc: dri-devel
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;
> };
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] drm/panthor: Don't use virt_to_pfn()
2024-03-18 15:11 ` Robin Murphy
@ 2024-03-18 15:25 ` Boris Brezillon
2024-03-25 10:05 ` Boris Brezillon
1 sibling, 0 replies; 5+ messages in thread
From: Boris Brezillon @ 2024-03-18 15:25 UTC (permalink / raw)
To: Robin Murphy; +Cc: Steven Price, Liviu Dudau, dri-devel
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;
> > };
> >
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] drm/panthor: Don't use virt_to_pfn()
2024-03-18 15:11 ` Robin Murphy
2024-03-18 15:25 ` Boris Brezillon
@ 2024-03-25 10:05 ` Boris Brezillon
1 sibling, 0 replies; 5+ messages in thread
From: Boris Brezillon @ 2024-03-25 10:05 UTC (permalink / raw)
To: Robin Murphy; +Cc: Steven Price, Liviu Dudau, dri-devel
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;
> > };
> >
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2024-03-25 10:05 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-03-18 14:51 [PATCH] drm/panthor: Don't use virt_to_pfn() Steven Price
2024-03-18 15:09 ` Boris Brezillon
2024-03-18 15:11 ` Robin Murphy
2024-03-18 15:25 ` Boris Brezillon
2024-03-25 10:05 ` Boris Brezillon
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.