Re: [PATCH v2] drm/vc4: Set DRM DMA device directly from HVS and V3D

["Maíra Canal" <mcanal@igalia.com>]

Hi Daniel,

LGTM, just some nits below:

On 28/09/26 17:16, Daniel Drake wrote:
> vc4 uses of_dma_configure() during bind to set the DMA configuration of
> the drm device by borrowing the configuration of one of its component
> devices.
> 
> This violates driver model expectations that IOMMU probing and domain
> attachment occur before driver binding - see commit bcb81ac6ae3c ("iommu:
> Get DT/ACPI parsing into the proper probe path").
> 
> With the introduction of the bcm2712-iommu driver behind the vc4 hvs
> device, a warning is triggered:
> 
>      vc4-drm gpu: late IOMMU probe at driver bind, something fishy here!
> 
> Instead of configuring the virtual aggregate device, adopt the pattern
> used by sun4i/sun8i/exynos where candidate DMA-capable hardware components
> register themselves as the device to use for DRM allocations via
> drm_dev_set_dma_dev(), provided a DMA device has not already been
> assigned.
> 
> HVS will typically bind first and claim the DMA device. The gen6 36-bit
> DMA mask configuration was moved into vc4_hvs_bind() accordingly. For
> older generations, vc4's v3d component continues to be available as a
> fallback, and the default platform bus 32-bit DMA mask is retained.
> 
> Fixes: da8e393e23ef ("drm/vc4: drv: Adopt the dma configuration from the HVS or V3D component")
> Signed-off-by: Daniel Drake <dan@reactivated.net>
> ---
> Changes in v2:
> - Fix vc4_bo_purge() to also use the selected DMA device for freeing
>    allocations
> - Link to v1: https://lore.kernel.org/r/20260927-drm-dma-device-2-v1-1-3230afcdc851@reactivated.net
> ---
>   drivers/gpu/drm/vc4/vc4_bo.c  |  3 ++-
>   drivers/gpu/drm/vc4/vc4_drv.c | 27 ---------------------------
>   drivers/gpu/drm/vc4/vc4_hvs.c | 14 ++++++++++++++
>   drivers/gpu/drm/vc4/vc4_v3d.c |  8 ++++++++
>   4 files changed, 24 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vc4/vc4_bo.c b/drivers/gpu/drm/vc4/vc4_bo.c
> index 49ea2ed0996b..320c5a0c71ed 100644
> --- a/drivers/gpu/drm/vc4/vc4_bo.c
> +++ b/drivers/gpu/drm/vc4/vc4_bo.c
> @@ -303,7 +303,8 @@ static void vc4_bo_purge(struct drm_gem_object *obj)
>   
>   	drm_vma_node_unmap(&obj->vma_node, dev->anon_inode->i_mapping);
>   
> -	dma_free_wc(dev->dev, obj->size, bo->base.vaddr, bo->base.dma_addr);
> +	dma_free_wc(drm_dev_dma_dev(dev), obj->size, bo->base.vaddr,
> +		    bo->base.dma_addr);
>   	bo->base.vaddr = NULL;
>   	bo->madv = __VC4_MADV_PURGED;
>   }
> diff --git a/drivers/gpu/drm/vc4/vc4_drv.c b/drivers/gpu/drm/vc4/vc4_drv.c
> index 616caf9d9915..62f1a5731e33 100644
> --- a/drivers/gpu/drm/vc4/vc4_drv.c
> +++ b/drivers/gpu/drm/vc4/vc4_drv.c

Could you clean up the headers of vc4_drv.c? I believe of_device.h and
dma-mapping.h might not be needed anymore.

> @@ -273,16 +273,6 @@ static void vc4_component_unbind_all(void *ptr)
>   	component_unbind_all(vc4->dev, &vc4->base);
>   }
>   
> -static const struct of_device_id vc4_dma_range_matches[] = {
> -	{ .compatible = "brcm,bcm2711-hvs" },
> -	{ .compatible = "brcm,bcm2712-hvs" },
> -	{ .compatible = "brcm,bcm2835-hvs" },
> -	{ .compatible = "brcm,bcm2835-v3d" },
> -	{ .compatible = "brcm,cygnus-v3d" },
> -	{ .compatible = "brcm,vc4-v3d" },
> -	{}
> -};
> -
>   static int vc4_drm_bind(struct device *dev)
>   {
>   	struct platform_device *pdev = to_platform_device(dev);
> @@ -295,8 +285,6 @@ static int vc4_drm_bind(struct device *dev)
>   	enum vc4_gen gen;
>   	int ret = 0;
>   
> -	dev->coherent_dma_mask = DMA_BIT_MASK(32);
> -
>   	gen = (enum vc4_gen)of_device_get_match_data(dev);
>   
>   	if (gen > VC4_GEN_4)
> @@ -304,21 +292,6 @@ static int vc4_drm_bind(struct device *dev)
>   	else
>   		driver = &vc4_drm_driver;
>   
> -	if (gen >= VC4_GEN_6_C)
> -		dma_set_mask_and_coherent(dev, DMA_BIT_MASK(36));
> -	else
> -		dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
> -
> -	node = of_find_matching_node_and_match(NULL, vc4_dma_range_matches,
> -					       NULL);
> -	if (node) {
> -		ret = of_dma_configure(dev, node, true);
> -		of_node_put(node);
> -
> -		if (ret)
> -			return ret;
> -	}
> -
>   	vc4 = devm_drm_dev_alloc(dev, driver, struct vc4_dev, base);
>   	if (IS_ERR(vc4))
>   		return PTR_ERR(vc4);
> diff --git a/drivers/gpu/drm/vc4/vc4_hvs.c b/drivers/gpu/drm/vc4/vc4_hvs.c
> index e715147d091c..305770c87dcf 100644
> --- a/drivers/gpu/drm/vc4/vc4_hvs.c
> +++ b/drivers/gpu/drm/vc4/vc4_hvs.c
> @@ -22,6 +22,7 @@
>   #include <linux/bitfield.h>
>   #include <linux/clk.h>
>   #include <linux/component.h>
> +#include <linux/dma-mapping.h>
>   #include <linux/platform_device.h>
>   
>   #include <drm/drm_atomic_helper.h>
> @@ -1662,6 +1663,19 @@ static int vc4_hvs_bind(struct device *dev, struct device *master, void *data)
>   		hvs->regset.nregs = ARRAY_SIZE(vc4_hvs_regs);
>   	}
>   
> +	if (vc4->gen >= VC4_GEN_6_C) {
> +		ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(36));
> +		if (ret)
> +			return ret;
> +	}
> +
> +	/*
> +	 * Use the HVS as the DRM device's DMA controller for buffer
> +	 * allocations if one has not already been configured.
> +	 */

I have the impression that this comment is not needed. v3d's comment
adds an interesting information, but this one seems redudant.

> +	if (drm_dev_dma_dev(drm) == drm->dev)
> +		drm_dev_set_dma_dev(drm, dev);
> +
>   	if (vc4->gen >= VC4_GEN_5) {
>   		struct rpi_firmware *firmware;
>   		struct device_node *node;
> diff --git a/drivers/gpu/drm/vc4/vc4_v3d.c b/drivers/gpu/drm/vc4/vc4_v3d.c
> index f32410420d3e..1f76d4850122 100644
> --- a/drivers/gpu/drm/vc4/vc4_v3d.c
> +++ b/drivers/gpu/drm/vc4/vc4_v3d.c
> @@ -10,6 +10,7 @@
>   #include <linux/platform_device.h>
>   #include <linux/pm_runtime.h>
>   
> +#include <drm/drm_drv.h>

I believe this include is not needed.

Best regards,
- Maíra