| Message ID | 20220124123659.4692-4-tzimmermann@suse.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | sysfb: Fix memory-region management | expand |
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 <tzimmermann@suse.de> > --- Looks good to me. Reviewed-by: Javier Martinez Canillas <javierm@redhat.com> Best regards,
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)
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 --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)
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(-)