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

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(-)

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

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");

The reg property is converted to a device resource when we create the 
device at [1].

I have another question, which I was discussing with Javier recently. Is 
it possible to handle memory-region there automatically? If, for 
exmaple, it would create a resource with IORESOURCE_CACHEABLE, simpledrm 
would handle it as memory region. Without the CACHEABLE flag, it would 
be a regular resource as before.

Best regards
Thomas

[1] 
https://elixir.bootlin.com/linux/v6.0.9/source/drivers/of/platform.c#L586

> +
> +	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);
>   
> -	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);
> +
> +		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

On Fri, Nov 18, 2022 at 03:21:14PM +0100, Thomas Zimmermann wrote:
> 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");
> 
> The reg property is converted to a device resource when we create the device
> at [1].
> 
> I have another question, which I was discussing with Javier recently. Is it
> possible to handle memory-region there automatically? If, for exmaple, it
> would create a resource with IORESOURCE_CACHEABLE, simpledrm would handle it
> as memory region. Without the CACHEABLE flag, it would be a regular resource
> as before.

memory-region properties are not typically converted into a standard
resource automatically. One reason may be that they can have additional
properties associated with them and so something like a CACHEABLE type
may not apply.

It's also standard to convert "reg" properties into struct resource and
that's what many drivers will expect. I don't know if all drivers will
gracefully handle being passed a struct resource that was created in
this way from a memory-region property. If at all I think this would
need to be special-cased for simple-framebuffer, in which case I'm not
convinced that putting the special case into the core OF code is any
better than putting it into the simpledrm driver.

Also, even if we did so, what would it really change? We may be able to
avoid the explicit DT lookup, but the bulk of the memory-region code is
actually mapping it, etc. That part we won't be able to automatically
handle, I think.

Ultimately this is up to Rob, not sure if he'll want to extend the
simple-framebuffer node creation code any further.

Thierry

> 
> Best regards
> Thomas
> 
> [1]
> https://elixir.bootlin.com/linux/v6.0.9/source/drivers/of/platform.c#L586
> 
> > +
> > +	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);
> > -	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);
> > +
> > +		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

Hi

Am 18.11.22 um 16:38 schrieb Thierry Reding:
> On Fri, Nov 18, 2022 at 03:21:14PM +0100, Thomas Zimmermann wrote:
>> 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");
>>
>> The reg property is converted to a device resource when we create the device
>> at [1].
>>
>> I have another question, which I was discussing with Javier recently. Is it
>> possible to handle memory-region there automatically? If, for exmaple, it
>> would create a resource with IORESOURCE_CACHEABLE, simpledrm would handle it
>> as memory region. Without the CACHEABLE flag, it would be a regular resource
>> as before.
> 
> memory-region properties are not typically converted into a standard
> resource automatically. One reason may be that they can have additional
> properties associated with them and so something like a CACHEABLE type
> may not apply.
> 
> It's also standard to convert "reg" properties into struct resource and
> that's what many drivers will expect. I don't know if all drivers will
> gracefully handle being passed a struct resource that was created in
> this way from a memory-region property. If at all I think this would
> need to be special-cased for simple-framebuffer, in which case I'm not
> convinced that putting the special case into the core OF code is any
> better than putting it into the simpledrm driver.
> 
> Also, even if we did so, what would it really change? We may be able to
> avoid the explicit DT lookup, but the bulk of the memory-region code is
> actually mapping it, etc. That part we won't be able to automatically
> handle, I think.
> 
> Ultimately this is up to Rob, not sure if he'll want to extend the
> simple-framebuffer node creation code any further.

Thanks for explaining. It was simply something we wondered about.

The simpledrm device driver hands over device ownership to the 
hardware's native driver during the boot process. To make this work in 
all cases, the OF code needs to be involved. So at some point, we'll 
need to move some of the memory-region code into the OF code. But how 
exactly this has to be done can be discussed later.

Best regards
Thomas

> 
> Thierry
> 
>>
>> Best regards
>> Thomas
>>
>> [1]
>> https://elixir.bootlin.com/linux/v6.0.9/source/drivers/of/platform.c#L586
>>
>>> +
>>> +	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);
>>> -	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);
>>> +
>>> +		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
> 
> 
>

On Fri, Nov 18, 2022 at 04:54:31PM +0100, Thomas Zimmermann wrote:
> Hi
> 
> Am 18.11.22 um 16:38 schrieb Thierry Reding:
> > On Fri, Nov 18, 2022 at 03:21:14PM +0100, Thomas Zimmermann wrote:
> > > 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");
> > > 
> > > The reg property is converted to a device resource when we create the device
> > > at [1].
> > > 
> > > I have another question, which I was discussing with Javier recently. Is it
> > > possible to handle memory-region there automatically? If, for exmaple, it
> > > would create a resource with IORESOURCE_CACHEABLE, simpledrm would handle it
> > > as memory region. Without the CACHEABLE flag, it would be a regular resource
> > > as before.
> > 
> > memory-region properties are not typically converted into a standard
> > resource automatically. One reason may be that they can have additional
> > properties associated with them and so something like a CACHEABLE type
> > may not apply.
> > 
> > It's also standard to convert "reg" properties into struct resource and
> > that's what many drivers will expect. I don't know if all drivers will
> > gracefully handle being passed a struct resource that was created in
> > this way from a memory-region property. If at all I think this would
> > need to be special-cased for simple-framebuffer, in which case I'm not
> > convinced that putting the special case into the core OF code is any
> > better than putting it into the simpledrm driver.
> > 
> > Also, even if we did so, what would it really change? We may be able to
> > avoid the explicit DT lookup, but the bulk of the memory-region code is
> > actually mapping it, etc. That part we won't be able to automatically
> > handle, I think.
> > 
> > Ultimately this is up to Rob, not sure if he'll want to extend the
> > simple-framebuffer node creation code any further.
> 
> Thanks for explaining. It was simply something we wondered about.
> 
> The simpledrm device driver hands over device ownership to the hardware's
> native driver during the boot process. To make this work in all cases, the
> OF code needs to be involved. So at some point, we'll need to move some of
> the memory-region code into the OF code. But how exactly this has to be done
> can be discussed later.

Currently the simpledrm driver will be removed when the native driver is
loaded (on Tegra at least) and then the Tegra DRM driver will set itself
up from scratch and anything that simpledrm had set up will be discarded
after that.

The tentative plan for Tegra is to eventually take over the memory from
simpledrm (basically via the same memory-region mechanism) and copy it
into a native buffer and also adopt the display configuration so that
the transition can happen seamlessly. I'm not sure to what degree the OF
core needs to get involved. Once we have the reserved-memory region, we
don't really need OF anymore.

One thing I'm not sure about yet is what to do with the reserved-memory
region. Ideally I think we would want to return the memory to the buddy
allocator so that it can be reused. I'm not sure if that's possible or
how to do it, since most of the low-level memblock code that is
responsible for cleaning things up has already run at this point. We'd
probably need to manually return the buffer somehow (perhaps with
something like generic_online_page()). Alternatively we may also hang
onto the memory and reuse it in the native driver, though that could be
a bit messy since it can't be transparently handled like any other
buffer.

I could find a few drivers that use memory-region, but none of them seem
to return that region to the buddy allocator.

Thierry