diff mbox series

[3/5] drm/simpledrm: Request memory region in driver

Message ID 20220124123659.4692-4-tzimmermann@suse.de
State Superseded
Headers show
Series sysfb: Fix memory-region management | expand

Commit Message

Thomas Zimmermann Jan. 24, 2022, 12:36 p.m. UTC
Requesting the framebuffer memory in simpledrm marks the memory
range as busy. This used to be done by the firmware sysfb code,
but the driver is the correct place.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/tiny/simpledrm.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

Comments

Jocelyn Falempe Jan. 24, 2022, 2:23 p.m. UTC | #1
On 24/01/2022 13:36, Thomas Zimmermann wrote:
> Requesting the framebuffer memory in simpledrm marks the memory
> range as busy. This used to be done by the firmware sysfb code,
> but the driver is the correct place.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>   drivers/gpu/drm/tiny/simpledrm.c | 20 +++++++++++++++-----
>   1 file changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
> index 04146da2d1d8..f72b71511a65 100644
> --- a/drivers/gpu/drm/tiny/simpledrm.c
> +++ b/drivers/gpu/drm/tiny/simpledrm.c
> @@ -526,21 +526,31 @@ static int simpledrm_device_init_mm(struct simpledrm_device *sdev)
>   {
>   	struct drm_device *dev = &sdev->dev;
>   	struct platform_device *pdev = sdev->pdev;
> -	struct resource *mem;
> +	struct resource *res, *mem;
>   	void __iomem *screen_base;
>   	int ret;
>   
> -	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -	if (!mem)
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res)
>   		return -EINVAL;
>   
> -	ret = devm_aperture_acquire_from_firmware(dev, mem->start, resource_size(mem));
> +	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",
> -			mem, ret);
> +			res, ret);
>   		return ret;
>   	}
>   
> +	mem = devm_request_mem_region(&pdev->dev, res->start, resource_size(res),
> +				      sdev->dev.driver->name);
> +	if (!mem) {
> +		/*
> +		 * We cannot make this fatal. Sometimes this comes from magic
> +		 * spaces our resource handlers simply don't know about
> +		 */
> +		drm_warn(dev, "could not acquire memory region %pr\n", res);
> +	}
> +
>   	screen_base = devm_ioremap_wc(&pdev->dev, mem->start,
>   				      resource_size(mem));

if mem is NULL, accessing mem->start will segfault after the warning.
I think you renamed "mem" to "res" so probably it should be renamed here 
too ?

>   	if (!screen_base)
Thomas Zimmermann Jan. 25, 2022, 8:31 a.m. UTC | #2
Hi

Am 24.01.22 um 15:23 schrieb Jocelyn Falempe:
> On 24/01/2022 13:36, Thomas Zimmermann wrote:
>> Requesting the framebuffer memory in simpledrm marks the memory
>> range as busy. This used to be done by the firmware sysfb code,
>> but the driver is the correct place.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>   drivers/gpu/drm/tiny/simpledrm.c | 20 +++++++++++++++-----
>>   1 file changed, 15 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/tiny/simpledrm.c 
>> b/drivers/gpu/drm/tiny/simpledrm.c
>> index 04146da2d1d8..f72b71511a65 100644
>> --- a/drivers/gpu/drm/tiny/simpledrm.c
>> +++ b/drivers/gpu/drm/tiny/simpledrm.c
>> @@ -526,21 +526,31 @@ static int simpledrm_device_init_mm(struct 
>> simpledrm_device *sdev)
>>   {
>>       struct drm_device *dev = &sdev->dev;
>>       struct platform_device *pdev = sdev->pdev;
>> -    struct resource *mem;
>> +    struct resource *res, *mem;
>>       void __iomem *screen_base;
>>       int ret;
>> -    mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> -    if (!mem)
>> +    res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +    if (!res)
>>           return -EINVAL;
>> -    ret = devm_aperture_acquire_from_firmware(dev, mem->start, 
>> resource_size(mem));
>> +    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",
>> -            mem, ret);
>> +            res, ret);
>>           return ret;
>>       }
>> +    mem = devm_request_mem_region(&pdev->dev, res->start, 
>> resource_size(res),
>> +                      sdev->dev.driver->name);
>> +    if (!mem) {
>> +        /*
>> +         * We cannot make this fatal. Sometimes this comes from magic
>> +         * spaces our resource handlers simply don't know about
>> +         */
>> +        drm_warn(dev, "could not acquire memory region %pr\n", res);
>> +    }
>> +
>>       screen_base = devm_ioremap_wc(&pdev->dev, mem->start,
>>                         resource_size(mem));
> 
> if mem is NULL, accessing mem->start will segfault after the warning.
> I think you renamed "mem" to "res" so probably it should be renamed here 
> too ?

Thanks for reviewing. Will be fixed in the next version. That code used 
to fail and i changed it to a warning after sync'ing with the simplefb 
driver. :/

Best regards
Thomas

> 
>>       if (!screen_base)
>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
index 04146da2d1d8..f72b71511a65 100644
--- a/drivers/gpu/drm/tiny/simpledrm.c
+++ b/drivers/gpu/drm/tiny/simpledrm.c
@@ -526,21 +526,31 @@  static int simpledrm_device_init_mm(struct simpledrm_device *sdev)
 {
 	struct drm_device *dev = &sdev->dev;
 	struct platform_device *pdev = sdev->pdev;
-	struct resource *mem;
+	struct resource *res, *mem;
 	void __iomem *screen_base;
 	int ret;
 
-	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (!mem)
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res)
 		return -EINVAL;
 
-	ret = devm_aperture_acquire_from_firmware(dev, mem->start, resource_size(mem));
+	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",
-			mem, ret);
+			res, ret);
 		return ret;
 	}
 
+	mem = devm_request_mem_region(&pdev->dev, res->start, resource_size(res),
+				      sdev->dev.driver->name);
+	if (!mem) {
+		/*
+		 * We cannot make this fatal. Sometimes this comes from magic
+		 * spaces our resource handlers simply don't know about
+		 */
+		drm_warn(dev, "could not acquire memory region %pr\n", res);
+	}
+
 	screen_base = devm_ioremap_wc(&pdev->dev, mem->start,
 				      resource_size(mem));
 	if (!screen_base)