Re: [PATCH v3 5/8] drm/simpledrm: Add support for system memory framebuffers

From: Thomas Zimmermann <tzimmermann@suse.de>
To: Thierry Reding <thierry.reding@gmail.com>,
	David Airlie <airlied@redhat.com>,
	Daniel Vetter <daniel@ffwll.ch>
Cc: linux-tegra@vger.kernel.org, devicetree@vger.kernel.org,
	Robin Murphy <robin.murphy@arm.com>,
	dri-devel@lists.freedesktop.org,
	Jon Hunter <jonathanh@nvidia.com>
Subject: Re: [PATCH v3 5/8] drm/simpledrm: Add support for system memory framebuffers
Date: Fri, 18 Nov 2022 15:11:23 +0100	[thread overview]
Message-ID: <9550a02f-6858-41e8-d45a-9e05cec19d78@suse.de> (raw)
In-Reply-To: <20221117184039.2291937-6-thierry.reding@gmail.com>

[-- Attachment #1.1: Type: text/plain, Size: 6160 bytes --]

Hi

Am 17.11.22 um 19:40 schrieb Thierry Reding:
> From: Thierry Reding <treding@nvidia.com>
> 
> Simple framebuffers can be set up in system memory, which cannot be
> requested and/or I/O remapped using the I/O resource helpers. Add a
> separate code path that obtains system memory framebuffers from the
> reserved memory region referenced in the memory-region property.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
> Changes in v3:
> - simplify memory code and move back to simpledrm_device_create()
> - extract screen_base iosys_map fix into separate patch
> 
> Changes in v2:
> - make screen base a struct iosys_map to avoid sparse warnings
> 
>   drivers/gpu/drm/tiny/simpledrm.c | 99 ++++++++++++++++++++++++--------
>   1 file changed, 75 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
> index 3673a42e4bf4..7f39bc58da52 100644
> --- a/drivers/gpu/drm/tiny/simpledrm.c
> +++ b/drivers/gpu/drm/tiny/simpledrm.c
> @@ -3,6 +3,7 @@
>   #include <linux/clk.h>
>   #include <linux/of_clk.h>
>   #include <linux/minmax.h>
> +#include <linux/of_address.h>
>   #include <linux/platform_data/simplefb.h>
>   #include <linux/platform_device.h>
>   #include <linux/regulator/consumer.h>
> @@ -184,6 +185,31 @@ simplefb_get_format_of(struct drm_device *dev, struct device_node *of_node)
>   	return simplefb_get_validated_format(dev, format);
>   }
>   
> +static struct resource *
> +simplefb_get_memory_of(struct drm_device *dev, struct device_node *of_node)
> +{
> +	struct device_node *np;
> +	struct resource *res;
> +	int err;
> +
> +	np = of_parse_phandle(of_node, "memory-region", 0);
> +	if (!np)
> +		return NULL;
> +
> +	res = devm_kzalloc(dev->dev, sizeof(*res), GFP_KERNEL);
> +	if (!res)
> +		return ERR_PTR(-ENOMEM);
> +
> +	err = of_address_to_resource(np, 0, res);
> +	if (err)
> +		return ERR_PTR(err);
> +
> +	if (of_get_property(of_node, "reg", NULL))
> +		drm_warn(dev, "preferring \"memory-region\" over \"reg\" property\n");
> +
> +	return res;
> +}
> +
>   /*
>    * Simple Framebuffer device
>    */
> @@ -623,8 +649,7 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
>   	struct drm_device *dev;
>   	int width, height, stride;
>   	const struct drm_format_info *format;
> -	struct resource *res, *mem;
> -	void __iomem *screen_base;
> +	struct resource *res, *mem = NULL;
>   	struct drm_plane *primary_plane;
>   	struct drm_crtc *crtc;
>   	struct drm_encoder *encoder;
> @@ -676,6 +701,9 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
>   		format = simplefb_get_format_of(dev, of_node);
>   		if (IS_ERR(format))
>   			return ERR_CAST(format);
> +		mem = simplefb_get_memory_of(dev, of_node);
> +		if (IS_ERR(mem))
> +			return ERR_CAST(mem);
>   	} else {
>   		drm_err(dev, "no simplefb configuration found\n");
>   		return ERR_PTR(-ENODEV);
> @@ -698,32 +726,55 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
>   	 * Memory management
>   	 */
>   
> -	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -	if (!res)
> -		return ERR_PTR(-EINVAL);
> +	if (mem) {
> +		void *screen_base;
>   
> -	ret = devm_aperture_acquire_from_firmware(dev, res->start, resource_size(res));
> -	if (ret) {
> -		drm_err(dev, "could not acquire memory range %pr: error %d\n", res, ret);
> -		return ERR_PTR(ret);
> -	}
> +		ret = devm_aperture_acquire_from_firmware(dev, mem->start, resource_size(mem));
> +		if (ret) {
> +			drm_err(dev, "could not acquire memory range %pr: %d\n", mem, ret);
> +			return ERR_PTR(ret);
> +		}
>   
> -	mem = devm_request_mem_region(&pdev->dev, res->start, resource_size(res), drv->name);
> -	if (!mem) {
> -		/*
> -		 * We cannot make this fatal. Sometimes this comes from magic
> -		 * spaces our resource handlers simply don't know about. Use
> -		 * the I/O-memory resource as-is and try to map that instead.
> -		 */
> -		drm_warn(dev, "could not acquire memory region %pr\n", res);
> -		mem = res;
> -	}
> +		drm_info(dev, "using system memory framebuffer at %pr\n", mem);

drm_dbg() please.

>   
> -	screen_base = devm_ioremap_wc(&pdev->dev, mem->start, resource_size(mem));
> -	if (!screen_base)
> -		return ERR_PTR(-ENOMEM);
> +		screen_base = devm_memremap(dev->dev, mem->start, resource_size(mem), MEMREMAP_WB);
> +		if (!screen_base)
> +			return ERR_PTR(-ENOMEM);
> +
> +		iosys_map_set_vaddr(&sdev->screen_base, screen_base);
> +	} else {
> +		void __iomem *screen_base;
> +
> +		res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +		if (!res)
> +			return ERR_PTR(-EINVAL);
>   
> -	iosys_map_set_vaddr_iomem(&sdev->screen_base, screen_base);
> +		ret = devm_aperture_acquire_from_firmware(dev, res->start, resource_size(res));
> +		if (ret) {
> +			drm_err(dev, "could not acquire memory range %pr: %d\n", &res, ret);
> +			return ERR_PTR(ret);
> +		}
> +
> +		drm_info(dev, "using I/O memory framebuffer at %pr\n", res);

drm_dbg()

With the small fixes, please add

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

Best regards
Thomas

> +
> +		mem = devm_request_mem_region(&pdev->dev, res->start, resource_size(res),
> +					      drv->name);
> +		if (!mem) {
> +			/*
> +			 * We cannot make this fatal. Sometimes this comes from magic
> +			 * spaces our resource handlers simply don't know about. Use
> +			 * the I/O-memory resource as-is and try to map that instead.
> +			 */
> +			drm_warn(dev, "could not acquire memory region %pr\n", res);
> +			mem = res;
> +		}
> +
> +		screen_base = devm_ioremap_wc(&pdev->dev, mem->start, resource_size(mem));
> +		if (!screen_base)
> +			return ERR_PTR(-ENOMEM);
> +
> +		iosys_map_set_vaddr_iomem(&sdev->screen_base, screen_base);
> +	}
>   
>   	/*
>   	 * Modesetting

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]