[4/5] fbdev/simplefb: Request memory region in driver

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 <[e-mail]>
---
 drivers/video/fbdev/simplefb.c | 59 ++++++++++++++++++++++++----------
 1 file changed, 42 insertions(+), 17 deletions(-)

Comments

Javier Martinez Canillas Jan. 24, 2022, 2:24 p.m. UTC | #1

On 1/24/22 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 <[e-mail]>
> ---
>  drivers/video/fbdev/simplefb.c | 59 ++++++++++++++++++++++++----------
>  1 file changed, 42 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
> index 57541887188b..84452028ecc9 100644
> --- a/drivers/video/fbdev/simplefb.c
> +++ b/drivers/video/fbdev/simplefb.c
> @@ -66,16 +66,36 @@ static int simplefb_setcolreg(u_int regno, u_int red, u_int green, u_int blue,
>  	return 0;
>  }
>  
> -struct simplefb_par;
> +struct simplefb_par {
> +	u32 palette[PSEUDO_PALETTE_SIZE];
> +#if defined CONFIG_OF && defined CONFIG_COMMON_CLK
> +	bool clks_enabled;
> +	unsigned int clk_count;
> +	struct clk **clks;
> +#endif
> +#if defined CONFIG_OF && defined CONFIG_REGULATOR
> +	bool regulators_enabled;
> +	u32 regulator_count;
> +	struct regulator **regulators;
> +#endif
> +	bool release_mem_region;

Maybe instead you could have:

	struct resource *mem; 

that would be more consistent with the fields for the other
resources like clocks, regulators, etc.

> +};
> +
>  static void simplefb_clocks_destroy(struct simplefb_par *par);
>  static void simplefb_regulators_destroy(struct simplefb_par *par);
>  
>  static void simplefb_destroy(struct fb_info *info)
>  {
> +	struct simplefb_par *par = info->par;
> +
>  	simplefb_regulators_destroy(info->par);
>  	simplefb_clocks_destroy(info->par);
>  	if (info->screen_base)
>  		iounmap(info->screen_base);
> +
> +	if (par->release_mem_region)
> +		release_mem_region(info->apertures->ranges[0].base,
> +				   info->apertures->ranges[0].size);

and here you could instead use the pointer to the resource:

	if (par->mem)
		release_mem_region(par->mem->start, resource_size(par->mem));

[snip]

>  static int simplefb_probe(struct platform_device *pdev)
>  {
> +	bool request_mem_succeeded = false;
>  	int ret;
>  	struct simplefb_params params;
>  	struct fb_info *info;
> @@ -436,9 +443,22 @@ static int simplefb_probe(struct platform_device *pdev)
>  		return -EINVAL;
>  	}
>  
> +	if (request_mem_region(mem->start, resource_size(mem), "simplefb")) {
> +		request_mem_succeeded = true;

and if you do the request_mem_region() after the struct fb_info allocation then
this could just be:

		par->mem = mem;

Best regards,

Patch

diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
index 57541887188b..84452028ecc9 100644
--- a/drivers/video/fbdev/simplefb.c
+++ b/drivers/video/fbdev/simplefb.c
@@ -66,16 +66,36 @@  static int simplefb_setcolreg(u_int regno, u_int red, u_int green, u_int blue,
 	return 0;
 }
 
-struct simplefb_par;
+struct simplefb_par {
+	u32 palette[PSEUDO_PALETTE_SIZE];
+#if defined CONFIG_OF && defined CONFIG_COMMON_CLK
+	bool clks_enabled;
+	unsigned int clk_count;
+	struct clk **clks;
+#endif
+#if defined CONFIG_OF && defined CONFIG_REGULATOR
+	bool regulators_enabled;
+	u32 regulator_count;
+	struct regulator **regulators;
+#endif
+	bool release_mem_region;
+};
+
 static void simplefb_clocks_destroy(struct simplefb_par *par);
 static void simplefb_regulators_destroy(struct simplefb_par *par);
 
 static void simplefb_destroy(struct fb_info *info)
 {
+	struct simplefb_par *par = info->par;
+
 	simplefb_regulators_destroy(info->par);
 	simplefb_clocks_destroy(info->par);
 	if (info->screen_base)
 		iounmap(info->screen_base);
+
+	if (par->release_mem_region)
+		release_mem_region(info->apertures->ranges[0].base,
+				   info->apertures->ranges[0].size);
 }
 
 static const struct fb_ops simplefb_ops = {
@@ -169,20 +189,6 @@  static int simplefb_parse_pd(struct platform_device *pdev,
 	return 0;
 }
 
-struct simplefb_par {
-	u32 palette[PSEUDO_PALETTE_SIZE];
-#if defined CONFIG_OF && defined CONFIG_COMMON_CLK
-	bool clks_enabled;
-	unsigned int clk_count;
-	struct clk **clks;
-#endif
-#if defined CONFIG_OF && defined CONFIG_REGULATOR
-	bool regulators_enabled;
-	u32 regulator_count;
-	struct regulator **regulators;
-#endif
-};
-
 #if defined CONFIG_OF && defined CONFIG_COMMON_CLK
 /*
  * Clock handling code.
@@ -401,6 +407,7 @@  static void simplefb_regulators_destroy(struct simplefb_par *par) { }
 
 static int simplefb_probe(struct platform_device *pdev)
 {
+	bool request_mem_succeeded = false;
 	int ret;
 	struct simplefb_params params;
 	struct fb_info *info;
@@ -436,9 +443,22 @@  static int simplefb_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
+	if (request_mem_region(mem->start, resource_size(mem), "simplefb")) {
+		request_mem_succeeded = true;
+	} else {
+		/*
+		 * We cannot make this fatal. Sometimes this comes from magic
+		 * spaces our resource handlers simply don't know about
+		 */
+		dev_warn(&pdev->dev, "simplefb: cannot reserve video memory at %pR\n",
+			 mem);
+	}
+
 	info = framebuffer_alloc(sizeof(struct simplefb_par), &pdev->dev);
-	if (!info)
-		return -ENOMEM;
+	if (!info) {
+		ret = -ENOMEM;
+		goto error_release_mem_region;
+	}
 	platform_set_drvdata(pdev, info);
 
 	par = info->par;
@@ -495,6 +515,8 @@  static int simplefb_probe(struct platform_device *pdev)
 			     info->var.xres, info->var.yres,
 			     info->var.bits_per_pixel, info->fix.line_length);
 
+	par->release_mem_region = request_mem_succeeded;
+
 	ret = register_framebuffer(info);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "Unable to register simplefb: %d\n", ret);
@@ -513,6 +535,9 @@  static int simplefb_probe(struct platform_device *pdev)
 	iounmap(info->screen_base);
 error_fb_release:
 	framebuffer_release(info);
+error_release_mem_region:
+	if (request_mem_succeeded)
+		release_mem_region(mem->start, resource_size(mem));
 	return ret;
 }