Re: [PATCH] drm/fb-helper: Mark screen buffers in system memory with FB_VIRTFB

[Thomas Zimmermann <tzimmermann@suse.de>]

[-- Attachment #1.1: Type: text/plain, Size: 3686 bytes --]

Hi

Am 27.01.22 um 16:03 schrieb Daniel Vetter:
> On Thu, Jan 27, 2022 at 12:58:30PM +0100, Thomas Zimmermann wrote:
>> Hi
>>
>> Am 27.01.22 um 12:42 schrieb Daniel Vetter:
>>> On Thu, Jan 27, 2022 at 11:26:21AM +0100, Thomas Zimmermann wrote:
>>>> Mark screen buffers in system memory with FB_VIRTFB. Otherwise, the
>>>> buffers are mmap'ed as I/O memory (i.e., VM_IO). For shadow buffers,
>>>> also set the FB_READS_FAST hint.
>>>
>>> Maybe clarify that this only holds for the defio case, and since we have
>>> our own shadow copy for that anyway it shouldn't matter. I'm also not sure
>>> how much the memcpy gains us compared to just redrawing ...
>>>
>>> What's the motivation here, or just something you spotted?
>>
>> Correctness mostly. fbdev's fbdefio tests for (the absence of) this flag and
>> sets VM_IO accordingly.
>>
>> It's actually for userspace. Maybe userspace tests these flags as well and
>> can optimize memcpy pattern for different types of caching. But I wouldn't
>> expect it TBH.
> 
> Hm I thought so too, but the #define is in the internal header, not the
> uapi header. And I don't see any ioctl code in fbmem.c that would shove
> fb_info->flags to userspace. That's why I wondered why you care about
> this? Or did I miss something somewhere?

You didn't.  I just grepped it myself and the only user of VIRTFB is the 
mmap code in fb_deferio.c, which (not) sets VM_IO. READS_FAST is unused. 
I'd then set the former, but not the latter. Ok?

Best regards
Thomas

> -Daniel
> 
>>
>> Best regards
>> Thomas
>>
>>> -Daniel
>>>
>>>>
>>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>>> ---
>>>>    drivers/gpu/drm/drm_fb_helper.c | 9 ++++++---
>>>>    1 file changed, 6 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
>>>> index ed43b987d306..f15127a32f7a 100644
>>>> --- a/drivers/gpu/drm/drm_fb_helper.c
>>>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>>>> @@ -2346,6 +2346,7 @@ static int drm_fb_helper_generic_probe(struct drm_fb_helper *fb_helper,
>>>>    	fbi->fbops = &drm_fbdev_fb_ops;
>>>>    	fbi->screen_size = sizes->surface_height * fb->pitches[0];
>>>>    	fbi->fix.smem_len = fbi->screen_size;
>>>> +	fbi->flags = FBINFO_DEFAULT;
>>>>    	drm_fb_helper_fill_info(fbi, fb_helper, sizes);
>>>> @@ -2353,19 +2354,21 @@ static int drm_fb_helper_generic_probe(struct drm_fb_helper *fb_helper,
>>>>    		fbi->screen_buffer = vzalloc(fbi->screen_size);
>>>>    		if (!fbi->screen_buffer)
>>>>    			return -ENOMEM;
>>>> +		fbi->flags |= FBINFO_VIRTFB | FBINFO_READS_FAST;
>>>>    		fbi->fbdefio = &drm_fbdev_defio;
>>>> -
>>>>    		fb_deferred_io_init(fbi);
>>>>    	} else {
>>>>    		/* buffer is mapped for HW framebuffer */
>>>>    		ret = drm_client_buffer_vmap(fb_helper->buffer, &map);
>>>>    		if (ret)
>>>>    			return ret;
>>>> -		if (map.is_iomem)
>>>> +		if (map.is_iomem) {
>>>>    			fbi->screen_base = map.vaddr_iomem;
>>>> -		else
>>>> +		} else {
>>>>    			fbi->screen_buffer = map.vaddr;
>>>> +			fbi->flags |= FBINFO_VIRTFB;
>>>> +		}
>>>>    		/*
>>>>    		 * Shamelessly leak the physical address to user-space. As
>>>> -- 
>>>> 2.34.1
>>>>
>>>
>>
>> -- 
>> 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
> 
> 
> 
> 

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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]