| Message ID | 20240318145119.368582-1-steven.price@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/panthor: Don't use virt_to_pfn() | expand |
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; > }; >
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; > }; >
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; > > }; > >
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; > > }; > >
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; };
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(-)