[PATCH] drm/fbdev-dma: Only set smem_start is enable per module option

[Thomas Zimmermann]

Hi

Am 18.06.24 um 11:35 schrieb Daniel Vetter:
> On Mon, Jun 17, 2024 at 05:26:37PM +0200, Thomas Zimmermann wrote:
>> Only export struct fb_info.fix.smem_start if that is required by the
>> user and the memory does not come from vmalloc().
>>
>> Setting struct fb_info.fix.smem_start breaks systems where DMA
>> memory is backed by vmalloc address space. An example error is
>> shown below.
>>
>> [    3.536043] ------------[ cut here ]------------
>> [    3.540716] virt_to_phys used for non-linear address: 000000007fc4f540 (0xffff800086001000)
>> [    3.552628] WARNING: CPU: 4 PID: 61 at arch/arm64/mm/physaddr.c:12 __virt_to_phys+0x68/0x98
>> [    3.565455] Modules linked in:
>> [    3.568525] CPU: 4 PID: 61 Comm: kworker/u12:5 Not tainted 6.6.23-06226-g4986cc3e1b75-dirty #250
>> [    3.577310] Hardware name: NXP i.MX95 19X19 board (DT)
>> [    3.582452] Workqueue: events_unbound deferred_probe_work_func
>> [    3.588291] pstate: 60400009 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
>> [    3.595233] pc : __virt_to_phys+0x68/0x98
>> [    3.599246] lr : __virt_to_phys+0x68/0x98
>> [    3.603276] sp : ffff800083603990
>> [    3.677939] Call trace:
>> [    3.680393]  __virt_to_phys+0x68/0x98
>> [    3.684067]  drm_fbdev_dma_helper_fb_probe+0x138/0x238
>> [    3.689214]  __drm_fb_helper_initial_config_and_unlock+0x2b0/0x4c0
>> [    3.695385]  drm_fb_helper_initial_config+0x4c/0x68
>> [    3.700264]  drm_fbdev_dma_client_hotplug+0x8c/0xe0
>> [    3.705161]  drm_client_register+0x60/0xb0
>> [    3.709269]  drm_fbdev_dma_setup+0x94/0x148
>>
>> Additionally, DMA memory is assumed to by contiguous in physical
>> address space, which is not guaranteed by vmalloc().
>>
>> Resolve this by checking the module flag drm_leak_fbdev_smem when
>> DRM allocated the instance of struct fb_info. Fbdev-dma then only
>> sets smem_start only if required (via FBINFO_HIDE_SMEM_START). Also
>> guarantee that the framebuffer is not located in vmalloc address
>> space.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann at suse.de>
>> Reported-by: Peng Fan (OSS) <peng.fan at oss.nxp.com>
>> Closes: https://lore.kernel.org/dri-devel/20240604080328.4024838-1-peng.fan@oss.nxp.com/
>> Fixes: b79fe9abd58b ("drm/fbdev-dma: Implement fbdev emulation for GEM DMA helpers")
>> ---
>>   drivers/gpu/drm/drm_fb_helper.c | 6 +++---
>>   drivers/gpu/drm/drm_fbdev_dma.c | 5 ++++-
>>   2 files changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
>> index e2e19f49342e1..18565ec684517 100644
>> --- a/drivers/gpu/drm/drm_fb_helper.c
>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>> @@ -517,6 +517,9 @@ struct fb_info *drm_fb_helper_alloc_info(struct drm_fb_helper *fb_helper)
>>   	if (!info)
>>   		return ERR_PTR(-ENOMEM);
>>   
>> +	if (!drm_leak_fbdev_smem)
>> +		info->flags |= FBINFO_HIDE_SMEM_START;
>> +
>>   	ret = fb_alloc_cmap(&info->cmap, 256, 0);
>>   	if (ret)
>>   		goto err_release;
>> @@ -1853,9 +1856,6 @@ __drm_fb_helper_initial_config_and_unlock(struct drm_fb_helper *fb_helper)
>>   	info = fb_helper->info;
>>   	info->var.pixclock = 0;
>>   
>> -	if (!drm_leak_fbdev_smem)
>> -		info->flags |= FBINFO_HIDE_SMEM_START;
>> -
>>   	/* Need to drop locks to avoid recursive deadlock in
>>   	 * register_framebuffer. This is ok because the only thing left to do is
>>   	 * register the fbdev emulation instance in kernel_fb_helper_list. */
>> diff --git a/drivers/gpu/drm/drm_fbdev_dma.c b/drivers/gpu/drm/drm_fbdev_dma.c
>> index 97ef6300d47e0..4b0527ddeb5a7 100644
>> --- a/drivers/gpu/drm/drm_fbdev_dma.c
>> +++ b/drivers/gpu/drm/drm_fbdev_dma.c
>> @@ -137,7 +137,10 @@ static int drm_fbdev_dma_helper_fb_probe(struct drm_fb_helper *fb_helper,
>>   		info->flags |= FBINFO_READS_FAST; /* signal caching */
>>   	info->screen_size = sizes->surface_height * fb->pitches[0];
>>   	info->screen_buffer = map.vaddr;
>> -	info->fix.smem_start = page_to_phys(virt_to_page(info->screen_buffer));
>> +	if (!(info->flags & FBINFO_HIDE_SMEM_START)) {
> I guess since the WARN is conditional on the explicit opt-in with
> drm_leak_fbdev_smem it should be fine.

Yes, that's the intention. If someone enables this flag, it warns if 
that's not supported. By default, smem_start is clear.

>
> Reviewed-by: Daniel Vetter <daniel.vetter at ffwll.ch>

Thanks

Best regards
Thomas

>
>> +		if (!drm_WARN_ON(dev, is_vmalloc_addr(info->screen_buffer)))
>> +			info->fix.smem_start = page_to_phys(virt_to_page(info->screen_buffer));
>> +	}
>>   	info->fix.smem_len = info->screen_size;
>>   
>>   	/* deferred I/O */
>> -- 
>> 2.45.2
>>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)