Hi

Am 08.03.22 um 15:42 schrieb Javier Martinez Canillas:
> On 3/3/22 21:58, Thomas Zimmermann wrote:
>> Store the per-page state for fbdev's deferred I/O in struct
>> fb_deferred_io_pageref. Maintain a list of pagerefs for the pages
>> that have to be flushed out to video memory. Update all affected
>> drivers.
>>
>> Like with pages before, fbdev acquires a pageref when an mmaped page
>> of the framebuffer is being written to. It holds the pageref in a
>> list of all currently written pagerefs until it flushes the written
>> pages to video memory. Writeback occurs periodically. After writeback
>> fbdev releases all pagerefs and builds up a new dirty list until the
>> next writeback occurs.
>>
>> Using pagerefs has a number of benefits.
>>
>> For pages of the framebuffer, the deferred I/O code used struct
>> page.lru as an entry into the list of dirty pages. The lru field is
>> owned by the page cache, which makes deferred I/O incompatible with
>> some memory pages (e.g., most notably DRM's GEM SHMEM allocator).
>> struct fb_deferred_io_pageref now provides an entry into a list of
>> dirty framebuffer pages, free'ing lru for use with the page cache.
>>
>> Drivers also assumed that struct page.index is the page offset into
>> the framebuffer. This is not true for DRM buffers, which are located
>> at various offset within a mapped area. struct fb_deferred_io_pageref
>> explicitly stores an offset into the framebuffer. struct page.index
>> is now only the page offset into the mapped area.
>>
>> These changes will allow DRM to use fbdev deferred I/O without an
>> intermediate shadow buffer.
>>
> 
> As mentioned, this is a very nice cleanup.
> 
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
> 
> [snip]
> 
>> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c b/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c
>> index 33f355907fbb..a35f695727c9 100644
>> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c
>> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c
>> @@ -322,12 +322,13 @@ static void vmw_deferred_io(struct fb_info *info,
>>   	struct vmw_fb_par *par = info->par;
>>   	unsigned long start, end, min, max;
>>   	unsigned long flags;
>> -	struct page *page;
>> +	struct fb_deferred_io_pageref *pageref;
>>   	int y1, y2;
>>   
>>   	min = ULONG_MAX;
>>   	max = 0;
>> -	list_for_each_entry(page, pagelist, lru) {
>> +	list_for_each_entry(pageref, pagelist, list) {
>> +		struct page *page = pageref->page;
>>   		start = page->index << PAGE_SHIFT;
> 
> Do you think that may be worth it to also decouple the struct page.index and
> store the index as a struct fb_deferred_io_pageref.index field ?
> 
> It's unlikely that this would change but it feels as if the layers are more
> separated that way, since no driver will access struct page fields directly.
> 
> The mapping would be 1:1 anyways just like it's the case for the page offset.
> 
> In fact, that could allow to even avoid declaring a struct page *page here.

There are two related fields page->index and pageref->offset. The former 
is the offset into the mapped file, the later is the offset in the 
mapped video memory (or fbdev buffer).  It's the same value for fbdev 
drivers. But for DRM, it's different: because BOs are mapped at an 
offset in the DRM device file, page->index has this offset added to it 
as well.

The value of page->index is required by page_mkclean(), [1] which we 
call to reset the mappings during the writeback phase of the deferred 
I/O. pageref->offset is for conveniently getting the video-memory offset 
in fb helpers.

I thought about using pageref->offset in fbdev drivers as well. It would 
be more correct, but didn't want to add unnecessary churn. Especially 
since I cannot test most of the fbdev drivers. If you think it's worth 
it, I'd change the drivers as well.

Best regards
Thomas

[1] https://elixir.bootlin.com/linux/latest/source/include/linux/rmap.h#L304


> 
> [snip]
> 
>> @@ -340,7 +340,8 @@ static void fbtft_deferred_io(struct fb_info *info, struct list_head *pagelist)
>>   	spin_unlock(&par->dirty_lock);
>>   
>>   	/* Mark display lines as dirty */
>> -	list_for_each_entry(page, pagelist, lru) {
>> +	list_for_each_entry(pageref, pagelist, list) {
>> +		struct page *page = pageref->page;
> 
> Same here and the other drivers. You only need the page for the index AFAICT.
> 
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> 

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