Re: [PATCH v2 3/3] fbdev: Refactor implementation of page_mkwrite

Hi

Am 25.04.22 um 20:24 schrieb Sam Ravnborg:

Hi Thomas.

On Mon, Apr 25, 2022 at 01:27:51PM +0200, Thomas Zimmermann wrote:

...

Refactor the page-write handler for deferred I/O. Drivers use the function to let fbdev track written pages of mmap'ed framebuffer memory.

I like how the comments got a brush up and a little more info was added. But I do not see the point of the refactoring - the code is equally readable before and after - maybe even easier before (modulus the improved comments).

But if you consider it better keep it. Again just my thoughts when reading the code.

The comes from patch of the larger GEM refactoring patches. [1] fb_deferred_io_page_mkwrite() is later supposed to be exported for use with DRM.

The patch isn't strictly necessary, but it doesn't do any harm either. I added it to the patchset, so that I don't have to deal with potential bit rot later on.

Best regards Thomas

[1] https://lore.kernel.org/dri-devel/20220303205839.28484-5-tzimmermann@suse.de...

Sam

...

v2:

• don't export the helper until we have an external caller

Signed-off-by: Thomas Zimmermann tzimmermann@suse.de Reviewed-by: Javier Martinez Canillas javierm@redhat.com

---

drivers/video/fbdev/core/fb_defio.c | 68 ++++++++++++++++++++--------- 1 file changed, 48 insertions(+), 20 deletions(-)

diff --git a/drivers/video/fbdev/core/fb_defio.c b/drivers/video/fbdev/core/fb_defio.c index a03b9c64fc61..214581ce5840 100644 --- a/drivers/video/fbdev/core/fb_defio.c +++ b/drivers/video/fbdev/core/fb_defio.c @@ -143,29 +143,18 @@ int fb_deferred_io_fsync(struct file *file, loff_t start, loff_t end, int datasy } EXPORT_SYMBOL_GPL(fb_deferred_io_fsync);

-/* vm_ops->page_mkwrite handler */ -static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault *vmf) +/*

• • Adds a page to the dirty list. Requires caller to hold
• • struct fb_deferred_io.lock. Call this from struct
• • vm_operations_struct.page_mkwrite.
• */

+static vm_fault_t __fb_deferred_io_track_page(struct fb_info *info, unsigned long offset,

• 			      struct page *page)
  

  {

• struct page *page = vmf->page;

• struct fb_info *info = vmf->vma->vm_private_data; struct fb_deferred_io *fbdefio = info->fbdefio; struct fb_deferred_io_pageref *pageref;

• unsigned long offset; vm_fault_t ret;

• offset = (vmf->address - vmf->vma->vm_start);

• /* this is a callback we get when userspace first tries to

• write to the page. we schedule a workqueue. that workqueue

• will eventually mkclean the touched pages and execute the

• deferred framebuffer IO. then if userspace touches a page

• again, we repeat the same scheme */

• file_update_time(vmf->vma->vm_file);

• /* protect against the workqueue changing the page list */

• mutex_lock(&fbdefio->lock);

• /* first write in this cycle, notify the driver */ if (fbdefio->first_io && list_empty(&fbdefio->pagelist)) fbdefio->first_io(info);

@@ -186,8 +175,6 @@ static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault *vmf) */ lock_page(pageref->page);

• mutex_unlock(&fbdefio->lock);
• /* come back after delay to process the deferred IO */ schedule_delayed_work(&info->deferred_work, fbdefio->delay); return VM_FAULT_LOCKED;

@@ -197,6 +184,47 @@ static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault *vmf) return ret; }

+/*

• • fb_deferred_io_page_mkwrite - Mark a page as written for deferred I/O
• • @fb_info: The fbdev info structure
• • @vmf: The VM fault
• • This is a callback we get when userspace first tries to
• • write to the page. We schedule a workqueue. That workqueue
• • will eventually mkclean the touched pages and execute the
• • deferred framebuffer IO. Then if userspace touches a page
• • again, we repeat the same scheme.
• • Returns:
• • VM_FAULT_LOCKED on success, or a VM_FAULT error otherwise.
• */

+static vm_fault_t fb_deferred_io_page_mkwrite(struct fb_info *info, struct vm_fault *vmf) +{

• struct page *page = vmf->page;
• struct fb_deferred_io *fbdefio = info->fbdefio;
• unsigned long offset;
• vm_fault_t ret;
• offset = (vmf->address - vmf->vma->vm_start);
• file_update_time(vmf->vma->vm_file);
• /* protect against the workqueue changing the page list */
• mutex_lock(&fbdefio->lock);
• ret = __fb_deferred_io_track_page(info, offset, page);
• mutex_unlock(&fbdefio->lock);
• return ret;

+}

+/* vm_ops->page_mkwrite handler */ +static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault *vmf) +{

• struct fb_info *info = vmf->vma->vm_private_data;
• return fb_deferred_io_page_mkwrite(info, vmf);

+}

• static const struct vm_operations_struct fb_deferred_io_vm_ops = { .fault = fb_deferred_io_fault, .page_mkwrite = fb_deferred_io_mkwrite,

-- 2.36.0