There's no need to give the page an address_space. Leaving the
page->mapping as NULL will cause the VM to handle set_page_dirty()
the same way that it's handled now, and that was the only reason to
set the address_space in the first place.
Signed-off-by: Matthew Wilcox (Oracle) <[email protected]>
Reviewed-by: Christoph Hellwig <[email protected]>
Reviewed-by: William Kucharski <[email protected]>
---
v2: Delete local variable definitions
drivers/video/fbdev/core/fb_defio.c | 35 -----------------------------
drivers/video/fbdev/core/fbmem.c | 4 ----
include/linux/fb.h | 3 ---
3 files changed, 42 deletions(-)
diff --git a/drivers/video/fbdev/core/fb_defio.c b/drivers/video/fbdev/core/fb_defio.c
index a591d291b231..b292887a2481 100644
--- a/drivers/video/fbdev/core/fb_defio.c
+++ b/drivers/video/fbdev/core/fb_defio.c
@@ -52,13 +52,6 @@ static vm_fault_t fb_deferred_io_fault(struct vm_fault *vmf)
return VM_FAULT_SIGBUS;
get_page(page);
-
- if (vmf->vma->vm_file)
- page->mapping = vmf->vma->vm_file->f_mapping;
- else
- printk(KERN_ERR "no mapping available\n");
-
- BUG_ON(!page->mapping);
page->index = vmf->pgoff;
vmf->page = page;
@@ -151,17 +144,6 @@ static const struct vm_operations_struct fb_deferred_io_vm_ops = {
.page_mkwrite = fb_deferred_io_mkwrite,
};
-static int fb_deferred_io_set_page_dirty(struct page *page)
-{
- if (!PageDirty(page))
- SetPageDirty(page);
- return 0;
-}
-
-static const struct address_space_operations fb_deferred_io_aops = {
- .set_page_dirty = fb_deferred_io_set_page_dirty,
-};
-
int fb_deferred_io_mmap(struct fb_info *info, struct vm_area_struct *vma)
{
vma->vm_ops = &fb_deferred_io_vm_ops;
@@ -212,29 +194,12 @@ void fb_deferred_io_init(struct fb_info *info)
}
EXPORT_SYMBOL_GPL(fb_deferred_io_init);
-void fb_deferred_io_open(struct fb_info *info,
- struct inode *inode,
- struct file *file)
-{
- file->f_mapping->a_ops = &fb_deferred_io_aops;
-}
-EXPORT_SYMBOL_GPL(fb_deferred_io_open);
-
void fb_deferred_io_cleanup(struct fb_info *info)
{
struct fb_deferred_io *fbdefio = info->fbdefio;
- struct page *page;
- int i;
BUG_ON(!fbdefio);
cancel_delayed_work_sync(&info->deferred_work);
-
- /* clear out the mapping that we setup */
- for (i = 0 ; i < info->fix.smem_len; i += PAGE_SIZE) {
- page = fb_deferred_io_page(info, i);
- page->mapping = NULL;
- }
-
mutex_destroy(&fbdefio->lock);
}
EXPORT_SYMBOL_GPL(fb_deferred_io_cleanup);
diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index 06f5805de2de..372b52a2befa 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -1415,10 +1415,6 @@ __releases(&info->lock)
if (res)
module_put(info->fbops->owner);
}
-#ifdef CONFIG_FB_DEFERRED_IO
- if (info->fbdefio)
- fb_deferred_io_open(info, inode, file);
-#endif
out:
unlock_fb_info(info);
if (res)
diff --git a/include/linux/fb.h b/include/linux/fb.h
index ecfbcc0553a5..a8dccd23c249 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -659,9 +659,6 @@ static inline void __fb_pad_aligned_buffer(u8 *dst, u32 d_pitch,
/* drivers/video/fb_defio.c */
int fb_deferred_io_mmap(struct fb_info *info, struct vm_area_struct *vma);
extern void fb_deferred_io_init(struct fb_info *info);
-extern void fb_deferred_io_open(struct fb_info *info,
- struct inode *inode,
- struct file *file);
extern void fb_deferred_io_cleanup(struct fb_info *info);
extern int fb_deferred_io_fsync(struct file *file, loff_t start,
loff_t end, int datasync);
--
2.30.0
Looks good; my apologies for missing the leftover declaration of struct page in the same routine which you also found and removed this time around.
> On Mar 10, 2021, at 11:55 AM, Matthew Wilcox (Oracle) <[email protected]> wrote:
>
> There's no need to give the page an address_space. Leaving the
> page->mapping as NULL will cause the VM to handle set_page_dirty()
> the same way that it's handled now, and that was the only reason to
> set the address_space in the first place.
>
> Signed-off-by: Matthew Wilcox (Oracle) <[email protected]>
> Reviewed-by: Christoph Hellwig <[email protected]>
> Reviewed-by: William Kucharski <[email protected]>
> ---
> v2: Delete local variable definitions
> drivers/video/fbdev/core/fb_defio.c | 35 -----------------------------
> drivers/video/fbdev/core/fbmem.c | 4 ----
> include/linux/fb.h | 3 ---
> 3 files changed, 42 deletions(-)
>
> diff --git a/drivers/video/fbdev/core/fb_defio.c b/drivers/video/fbdev/core/fb_defio.c
> index a591d291b231..b292887a2481 100644
> --- a/drivers/video/fbdev/core/fb_defio.c
> +++ b/drivers/video/fbdev/core/fb_defio.c
> @@ -52,13 +52,6 @@ static vm_fault_t fb_deferred_io_fault(struct vm_fault *vmf)
> return VM_FAULT_SIGBUS;
>
> get_page(page);
> -
> - if (vmf->vma->vm_file)
> - page->mapping = vmf->vma->vm_file->f_mapping;
> - else
> - printk(KERN_ERR "no mapping available\n");
> -
> - BUG_ON(!page->mapping);
> page->index = vmf->pgoff;
>
> vmf->page = page;
> @@ -151,17 +144,6 @@ static const struct vm_operations_struct fb_deferred_io_vm_ops = {
> .page_mkwrite = fb_deferred_io_mkwrite,
> };
>
> -static int fb_deferred_io_set_page_dirty(struct page *page)
> -{
> - if (!PageDirty(page))
> - SetPageDirty(page);
> - return 0;
> -}
> -
> -static const struct address_space_operations fb_deferred_io_aops = {
> - .set_page_dirty = fb_deferred_io_set_page_dirty,
> -};
> -
> int fb_deferred_io_mmap(struct fb_info *info, struct vm_area_struct *vma)
> {
> vma->vm_ops = &fb_deferred_io_vm_ops;
> @@ -212,29 +194,12 @@ void fb_deferred_io_init(struct fb_info *info)
> }
> EXPORT_SYMBOL_GPL(fb_deferred_io_init);
>
> -void fb_deferred_io_open(struct fb_info *info,
> - struct inode *inode,
> - struct file *file)
> -{
> - file->f_mapping->a_ops = &fb_deferred_io_aops;
> -}
> -EXPORT_SYMBOL_GPL(fb_deferred_io_open);
> -
> void fb_deferred_io_cleanup(struct fb_info *info)
> {
> struct fb_deferred_io *fbdefio = info->fbdefio;
> - struct page *page;
> - int i;
>
> BUG_ON(!fbdefio);
> cancel_delayed_work_sync(&info->deferred_work);
> -
> - /* clear out the mapping that we setup */
> - for (i = 0 ; i < info->fix.smem_len; i += PAGE_SIZE) {
> - page = fb_deferred_io_page(info, i);
> - page->mapping = NULL;
> - }
> -
> mutex_destroy(&fbdefio->lock);
> }
> EXPORT_SYMBOL_GPL(fb_deferred_io_cleanup);
> diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
> index 06f5805de2de..372b52a2befa 100644
> --- a/drivers/video/fbdev/core/fbmem.c
> +++ b/drivers/video/fbdev/core/fbmem.c
> @@ -1415,10 +1415,6 @@ __releases(&info->lock)
> if (res)
> module_put(info->fbops->owner);
> }
> -#ifdef CONFIG_FB_DEFERRED_IO
> - if (info->fbdefio)
> - fb_deferred_io_open(info, inode, file);
> -#endif
> out:
> unlock_fb_info(info);
> if (res)
> diff --git a/include/linux/fb.h b/include/linux/fb.h
> index ecfbcc0553a5..a8dccd23c249 100644
> --- a/include/linux/fb.h
> +++ b/include/linux/fb.h
> @@ -659,9 +659,6 @@ static inline void __fb_pad_aligned_buffer(u8 *dst, u32 d_pitch,
> /* drivers/video/fb_defio.c */
> int fb_deferred_io_mmap(struct fb_info *info, struct vm_area_struct *vma);
> extern void fb_deferred_io_init(struct fb_info *info);
> -extern void fb_deferred_io_open(struct fb_info *info,
> - struct inode *inode,
> - struct file *file);
> extern void fb_deferred_io_cleanup(struct fb_info *info);
> extern int fb_deferred_io_fsync(struct file *file, loff_t start,
> loff_t end, int datasync);
> --
> 2.30.0
>
>
On Wed, Mar 10, 2021 at 06:55:30PM +0000, Matthew Wilcox (Oracle) wrote:
> There's no need to give the page an address_space. Leaving the
> page->mapping as NULL will cause the VM to handle set_page_dirty()
> the same way that it's handled now, and that was the only reason to
> set the address_space in the first place.
>
> Signed-off-by: Matthew Wilcox (Oracle) <[email protected]>
> Reviewed-by: Christoph Hellwig <[email protected]>
> Reviewed-by: William Kucharski <[email protected]>
Thanks for your patch, merged to drm-misc-next for 5.13.
While I have an expert here, does this mean that for a VM_PFNMAP we could
pull of the same trick without any struct page backing, assuming we pulle
the per-page dirty state into some tracking of our own?
I'm asking since for DRM drivers we currently have a fairly awkward
situation with a bounce buffer in system memory going on that we copy out
of, because we can't directly use the gpu buffers. If we can track
directly in the gpu buffers, maybe even as some kind of overlay over the
vma, we could avoid that copy.
Otoh no one cares about fbcon performance, so *shrug*.
Cheers, Daniel
> ---
> v2: Delete local variable definitions
> drivers/video/fbdev/core/fb_defio.c | 35 -----------------------------
> drivers/video/fbdev/core/fbmem.c | 4 ----
> include/linux/fb.h | 3 ---
> 3 files changed, 42 deletions(-)
>
> diff --git a/drivers/video/fbdev/core/fb_defio.c b/drivers/video/fbdev/core/fb_defio.c
> index a591d291b231..b292887a2481 100644
> --- a/drivers/video/fbdev/core/fb_defio.c
> +++ b/drivers/video/fbdev/core/fb_defio.c
> @@ -52,13 +52,6 @@ static vm_fault_t fb_deferred_io_fault(struct vm_fault *vmf)
> return VM_FAULT_SIGBUS;
>
> get_page(page);
> -
> - if (vmf->vma->vm_file)
> - page->mapping = vmf->vma->vm_file->f_mapping;
> - else
> - printk(KERN_ERR "no mapping available\n");
> -
> - BUG_ON(!page->mapping);
> page->index = vmf->pgoff;
>
> vmf->page = page;
> @@ -151,17 +144,6 @@ static const struct vm_operations_struct fb_deferred_io_vm_ops = {
> .page_mkwrite = fb_deferred_io_mkwrite,
> };
>
> -static int fb_deferred_io_set_page_dirty(struct page *page)
> -{
> - if (!PageDirty(page))
> - SetPageDirty(page);
> - return 0;
> -}
> -
> -static const struct address_space_operations fb_deferred_io_aops = {
> - .set_page_dirty = fb_deferred_io_set_page_dirty,
> -};
> -
> int fb_deferred_io_mmap(struct fb_info *info, struct vm_area_struct *vma)
> {
> vma->vm_ops = &fb_deferred_io_vm_ops;
> @@ -212,29 +194,12 @@ void fb_deferred_io_init(struct fb_info *info)
> }
> EXPORT_SYMBOL_GPL(fb_deferred_io_init);
>
> -void fb_deferred_io_open(struct fb_info *info,
> - struct inode *inode,
> - struct file *file)
> -{
> - file->f_mapping->a_ops = &fb_deferred_io_aops;
> -}
> -EXPORT_SYMBOL_GPL(fb_deferred_io_open);
> -
> void fb_deferred_io_cleanup(struct fb_info *info)
> {
> struct fb_deferred_io *fbdefio = info->fbdefio;
> - struct page *page;
> - int i;
>
> BUG_ON(!fbdefio);
> cancel_delayed_work_sync(&info->deferred_work);
> -
> - /* clear out the mapping that we setup */
> - for (i = 0 ; i < info->fix.smem_len; i += PAGE_SIZE) {
> - page = fb_deferred_io_page(info, i);
> - page->mapping = NULL;
> - }
> -
> mutex_destroy(&fbdefio->lock);
> }
> EXPORT_SYMBOL_GPL(fb_deferred_io_cleanup);
> diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
> index 06f5805de2de..372b52a2befa 100644
> --- a/drivers/video/fbdev/core/fbmem.c
> +++ b/drivers/video/fbdev/core/fbmem.c
> @@ -1415,10 +1415,6 @@ __releases(&info->lock)
> if (res)
> module_put(info->fbops->owner);
> }
> -#ifdef CONFIG_FB_DEFERRED_IO
> - if (info->fbdefio)
> - fb_deferred_io_open(info, inode, file);
> -#endif
> out:
> unlock_fb_info(info);
> if (res)
> diff --git a/include/linux/fb.h b/include/linux/fb.h
> index ecfbcc0553a5..a8dccd23c249 100644
> --- a/include/linux/fb.h
> +++ b/include/linux/fb.h
> @@ -659,9 +659,6 @@ static inline void __fb_pad_aligned_buffer(u8 *dst, u32 d_pitch,
> /* drivers/video/fb_defio.c */
> int fb_deferred_io_mmap(struct fb_info *info, struct vm_area_struct *vma);
> extern void fb_deferred_io_init(struct fb_info *info);
> -extern void fb_deferred_io_open(struct fb_info *info,
> - struct inode *inode,
> - struct file *file);
> extern void fb_deferred_io_cleanup(struct fb_info *info);
> extern int fb_deferred_io_fsync(struct file *file, loff_t start,
> loff_t end, int datasync);
> --
> 2.30.0
>
> _______________________________________________
> dri-devel mailing list
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
Hi Matthew,
On Wed, Mar 10, 2021 at 06:55:30PM +0000, Matthew Wilcox (Oracle) wrote:
> There's no need to give the page an address_space. Leaving the
> page->mapping as NULL will cause the VM to handle set_page_dirty()
> the same way that it's handled now, and that was the only reason to
> set the address_space in the first place.
>
> Signed-off-by: Matthew Wilcox (Oracle) <[email protected]>
> Reviewed-by: Christoph Hellwig <[email protected]>
> Reviewed-by: William Kucharski <[email protected]>
This patch in mainline as commit ccf953d8f3d6 ("fb_defio: Remove custom
address_space_operations") causes my Hyper-V based VM to no longer make
it to a graphical environment.
$ git bisect log
# bad: [6efb943b8616ec53a5e444193dccf1af9ad627b5] Linux 5.13-rc1
# good: [9f4ad9e425a1d3b6a34617b8ea226d56a119a717] Linux 5.12
git bisect start 'v5.13-rc1' 'v5.12'
# bad: [71a5cc28e88b0db69c3f83d4061ad4cc684af09f] Merge tag 'mfd-next-5.13' of git://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd
git bisect bad 71a5cc28e88b0db69c3f83d4061ad4cc684af09f
# good: [2a19866b6e4cf554b57660549d12496ea84aa7d7] Merge tag '5.12-rc-smb3-fixes-part1' of git://git.samba.org/sfrench/cifs-2.6
git bisect good 2a19866b6e4cf554b57660549d12496ea84aa7d7
# bad: [a1a1ca70deb3ec600eeabb21de7f3f48aaae5695] Merge tag 'drm-misc-next-fixes-2021-04-22' of git://anongit.freedesktop.org/drm/drm-misc into drm-next
git bisect bad a1a1ca70deb3ec600eeabb21de7f3f48aaae5695
# good: [2cbcb78c9ee5520c8d836c7ff57d1b60ebe8e9b7] Merge tag 'amd-drm-next-5.13-2021-03-23' of https://gitlab.freedesktop.org/agd5f/linux into drm-next
git bisect good 2cbcb78c9ee5520c8d836c7ff57d1b60ebe8e9b7
# bad: [9c0fed84d5750e1eea6c664e073ffa2534a17743] Merge tag 'drm-intel-next-2021-04-01' of git://anongit.freedesktop.org/drm/drm-intel into drm-next
git bisect bad 9c0fed84d5750e1eea6c664e073ffa2534a17743
# bad: [1539f71602edf09bb33666afddc5a781c42e768d] Merge tag 'drm-misc-next-2021-04-01' of git://anongit.freedesktop.org/drm/drm-misc into drm-next
git bisect bad 1539f71602edf09bb33666afddc5a781c42e768d
# good: [2f835b5dd8f7fc1e58d73fc2cd2ec33c2b054036] Merge tag 'topic/i915-gem-next-2021-03-26' of ssh://git.freedesktop.org/git/drm/drm into drm-next
git bisect good 2f835b5dd8f7fc1e58d73fc2cd2ec33c2b054036
# bad: [c42712c6e9bea042ea9696713024af7417f5ce18] drm/bridge/analogix/anx6345: Cleanup on errors in anx6345_bridge_attach()
git bisect bad c42712c6e9bea042ea9696713024af7417f5ce18
# bad: [dc659a4e852b591771fc2e5abb60f4455b0cf316] drm/probe-helper: Check epoch counter in output_poll_execute()
git bisect bad dc659a4e852b591771fc2e5abb60f4455b0cf316
# good: [5e7222a3674ea7422370779884dd53aabe9e4a9d] drm/panel-simple: Undo enable if HPD never asserts
git bisect good 5e7222a3674ea7422370779884dd53aabe9e4a9d
# good: [67cc24ac17fe2a2496456c8ca281ef89f7a6fd89] drm: panel: simple: Set enable delay for BOE NV110WTM-N61
git bisect good 67cc24ac17fe2a2496456c8ca281ef89f7a6fd89
# bad: [ccf953d8f3d68e85e577e843fdcde8872b0a9769] fb_defio: Remove custom address_space_operations
git bisect bad ccf953d8f3d68e85e577e843fdcde8872b0a9769
# good: [8613385cb2856e2ee9c4efb1f95eeaca0e1a0963] dma-fence: Document recoverable page fault implications
git bisect good 8613385cb2856e2ee9c4efb1f95eeaca0e1a0963
# first bad commit: [ccf953d8f3d68e85e577e843fdcde8872b0a9769] fb_defio: Remove custom address_space_operations
I did verify that I do not see the issue fixed with next-20210528. I
have attached the output of 'journalctl -k' from v5.13-rc3 and
next-20210528, they do not seem to provide any smoking gun to me but I
am not really sure what to look for. I am happy to provide any further
information or do any debugging as you see fit.
Cheers,
Nathan
On Sun, May 30, 2021 at 12:13:05PM -0700, Nathan Chancellor wrote:
> Hi Matthew,
>
> On Wed, Mar 10, 2021 at 06:55:30PM +0000, Matthew Wilcox (Oracle) wrote:
> > There's no need to give the page an address_space. Leaving the
> > page->mapping as NULL will cause the VM to handle set_page_dirty()
> > the same way that it's handled now, and that was the only reason to
> > set the address_space in the first place.
> >
> > Signed-off-by: Matthew Wilcox (Oracle) <[email protected]>
> > Reviewed-by: Christoph Hellwig <[email protected]>
> > Reviewed-by: William Kucharski <[email protected]>
>
> This patch in mainline as commit ccf953d8f3d6 ("fb_defio: Remove custom
> address_space_operations") causes my Hyper-V based VM to no longer make
> it to a graphical environment.
Hi Nathan,
Thanks for the report. I sent Daniel a revert patch with a full
explanation last week, which I assume he'll queue up for a pull soon.
You can just git revert ccf953d8f3d6 for yourself until that shows up.
Sorry for the inconvenience.
On 5/30/2021 2:14 PM, Matthew Wilcox wrote:
> On Sun, May 30, 2021 at 12:13:05PM -0700, Nathan Chancellor wrote:
>> Hi Matthew,
>>
>> On Wed, Mar 10, 2021 at 06:55:30PM +0000, Matthew Wilcox (Oracle) wrote:
>>> There's no need to give the page an address_space. Leaving the
>>> page->mapping as NULL will cause the VM to handle set_page_dirty()
>>> the same way that it's handled now, and that was the only reason to
>>> set the address_space in the first place.
>>>
>>> Signed-off-by: Matthew Wilcox (Oracle) <[email protected]>
>>> Reviewed-by: Christoph Hellwig <[email protected]>
>>> Reviewed-by: William Kucharski <[email protected]>
>>
>> This patch in mainline as commit ccf953d8f3d6 ("fb_defio: Remove custom
>> address_space_operations") causes my Hyper-V based VM to no longer make
>> it to a graphical environment.
>
> Hi Nathan,
>
> Thanks for the report. I sent Daniel a revert patch with a full
> explanation last week, which I assume he'll queue up for a pull soon.
> You can just git revert ccf953d8f3d6 for yourself until that shows up.
> Sorry for the inconvenience.
>
Thank you for the quick response! I will keep an eye out for the patch
while reverting it locally in the meantime.
Cheers,
Nathan
On Sun, May 30, 2021 at 10:14:22PM +0100, Matthew Wilcox wrote:
> On Sun, May 30, 2021 at 12:13:05PM -0700, Nathan Chancellor wrote:
> > Hi Matthew,
> >
> > On Wed, Mar 10, 2021 at 06:55:30PM +0000, Matthew Wilcox (Oracle) wrote:
> > > There's no need to give the page an address_space. Leaving the
> > > page->mapping as NULL will cause the VM to handle set_page_dirty()
> > > the same way that it's handled now, and that was the only reason to
> > > set the address_space in the first place.
> > >
> > > Signed-off-by: Matthew Wilcox (Oracle) <[email protected]>
> > > Reviewed-by: Christoph Hellwig <[email protected]>
> > > Reviewed-by: William Kucharski <[email protected]>
> >
> > This patch in mainline as commit ccf953d8f3d6 ("fb_defio: Remove custom
> > address_space_operations") causes my Hyper-V based VM to no longer make
> > it to a graphical environment.
>
> Hi Nathan,
>
> Thanks for the report. I sent Daniel a revert patch with a full
> explanation last week, which I assume he'll queue up for a pull soon.
> You can just git revert ccf953d8f3d6 for yourself until that shows up.
> Sorry for the inconvenience.
Uh that patch didn't get cc'ed to any list so I've ignored it. I've found
it now, but lack of lore link is awkward. Can you pls resubmit with
dri-devel on cc? fbdev list is dead, I don't look there.
Thanks, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
On Tue, Jun 01, 2021 at 04:10:32PM +0200, Daniel Vetter wrote:
> On Sun, May 30, 2021 at 10:14:22PM +0100, Matthew Wilcox wrote:
> > On Sun, May 30, 2021 at 12:13:05PM -0700, Nathan Chancellor wrote:
> > > Hi Matthew,
> > >
> > > On Wed, Mar 10, 2021 at 06:55:30PM +0000, Matthew Wilcox (Oracle) wrote:
> > > > There's no need to give the page an address_space. Leaving the
> > > > page->mapping as NULL will cause the VM to handle set_page_dirty()
> > > > the same way that it's handled now, and that was the only reason to
> > > > set the address_space in the first place.
> > > >
> > > > Signed-off-by: Matthew Wilcox (Oracle) <[email protected]>
> > > > Reviewed-by: Christoph Hellwig <[email protected]>
> > > > Reviewed-by: William Kucharski <[email protected]>
> > >
> > > This patch in mainline as commit ccf953d8f3d6 ("fb_defio: Remove custom
> > > address_space_operations") causes my Hyper-V based VM to no longer make
> > > it to a graphical environment.
> >
> > Hi Nathan,
> >
> > Thanks for the report. I sent Daniel a revert patch with a full
> > explanation last week, which I assume he'll queue up for a pull soon.
> > You can just git revert ccf953d8f3d6 for yourself until that shows up.
> > Sorry for the inconvenience.
>
> Uh that patch didn't get cc'ed to any list so I've ignored it. I've found
> it now, but lack of lore link is awkward. Can you pls resubmit with
> dri-devel on cc? fbdev list is dead, I don't look there.
How about I just attach it here?
On Tue, Jun 01, 2021 at 03:30:30PM +0100, Matthew Wilcox wrote:
> On Tue, Jun 01, 2021 at 04:10:32PM +0200, Daniel Vetter wrote:
> > On Sun, May 30, 2021 at 10:14:22PM +0100, Matthew Wilcox wrote:
> > > On Sun, May 30, 2021 at 12:13:05PM -0700, Nathan Chancellor wrote:
> > > > Hi Matthew,
> > > >
> > > > On Wed, Mar 10, 2021 at 06:55:30PM +0000, Matthew Wilcox (Oracle) wrote:
> > > > > There's no need to give the page an address_space. Leaving the
> > > > > page->mapping as NULL will cause the VM to handle set_page_dirty()
> > > > > the same way that it's handled now, and that was the only reason to
> > > > > set the address_space in the first place.
> > > > >
> > > > > Signed-off-by: Matthew Wilcox (Oracle) <[email protected]>
> > > > > Reviewed-by: Christoph Hellwig <[email protected]>
> > > > > Reviewed-by: William Kucharski <[email protected]>
> > > >
> > > > This patch in mainline as commit ccf953d8f3d6 ("fb_defio: Remove custom
> > > > address_space_operations") causes my Hyper-V based VM to no longer make
> > > > it to a graphical environment.
> > >
> > > Hi Nathan,
> > >
> > > Thanks for the report. I sent Daniel a revert patch with a full
> > > explanation last week, which I assume he'll queue up for a pull soon.
> > > You can just git revert ccf953d8f3d6 for yourself until that shows up.
> > > Sorry for the inconvenience.
> >
> > Uh that patch didn't get cc'ed to any list so I've ignored it. I've found
> > it now, but lack of lore link is awkward. Can you pls resubmit with
> > dri-devel on cc? fbdev list is dead, I don't look there.
>
> How about I just attach it here?
Thanks, that worked with Link: and everything and no choking of my script
:-)
Cheers, Daniel
> From e88921d0775d87323a8688af37dfd7cdebdde5a9 Mon Sep 17 00:00:00 2001
> From: "Matthew Wilcox (Oracle)" <[email protected]>
> Date: Tue, 25 May 2021 08:37:33 -0400
> Subject: [PATCH] Revert "fb_defio: Remove custom address_space_operations"
>
> Commit ccf953d8f3d6 makes framebuffers which use deferred I/O stop
> displaying updates after the first one. This is because the pages
> handled by fb_defio no longer have a page_mapping(). That prevents
> page_mkclean() from marking the PTEs as clean, and so writes are only
> noticed the first time.
>
> Reported-by: Andy Shevchenko <[email protected]>
> Signed-off-by: Matthew Wilcox (Oracle) <[email protected]>
> ---
> drivers/video/fbdev/core/fb_defio.c | 35 +++++++++++++++++++++++++++++
> drivers/video/fbdev/core/fbmem.c | 4 ++++
> include/linux/fb.h | 3 +++
> 3 files changed, 42 insertions(+)
>
> diff --git a/drivers/video/fbdev/core/fb_defio.c b/drivers/video/fbdev/core/fb_defio.c
> index b292887a2481..a591d291b231 100644
> --- a/drivers/video/fbdev/core/fb_defio.c
> +++ b/drivers/video/fbdev/core/fb_defio.c
> @@ -52,6 +52,13 @@ static vm_fault_t fb_deferred_io_fault(struct vm_fault *vmf)
> return VM_FAULT_SIGBUS;
>
> get_page(page);
> +
> + if (vmf->vma->vm_file)
> + page->mapping = vmf->vma->vm_file->f_mapping;
> + else
> + printk(KERN_ERR "no mapping available\n");
> +
> + BUG_ON(!page->mapping);
> page->index = vmf->pgoff;
>
> vmf->page = page;
> @@ -144,6 +151,17 @@ static const struct vm_operations_struct fb_deferred_io_vm_ops = {
> .page_mkwrite = fb_deferred_io_mkwrite,
> };
>
> +static int fb_deferred_io_set_page_dirty(struct page *page)
> +{
> + if (!PageDirty(page))
> + SetPageDirty(page);
> + return 0;
> +}
> +
> +static const struct address_space_operations fb_deferred_io_aops = {
> + .set_page_dirty = fb_deferred_io_set_page_dirty,
> +};
> +
> int fb_deferred_io_mmap(struct fb_info *info, struct vm_area_struct *vma)
> {
> vma->vm_ops = &fb_deferred_io_vm_ops;
> @@ -194,12 +212,29 @@ void fb_deferred_io_init(struct fb_info *info)
> }
> EXPORT_SYMBOL_GPL(fb_deferred_io_init);
>
> +void fb_deferred_io_open(struct fb_info *info,
> + struct inode *inode,
> + struct file *file)
> +{
> + file->f_mapping->a_ops = &fb_deferred_io_aops;
> +}
> +EXPORT_SYMBOL_GPL(fb_deferred_io_open);
> +
> void fb_deferred_io_cleanup(struct fb_info *info)
> {
> struct fb_deferred_io *fbdefio = info->fbdefio;
> + struct page *page;
> + int i;
>
> BUG_ON(!fbdefio);
> cancel_delayed_work_sync(&info->deferred_work);
> +
> + /* clear out the mapping that we setup */
> + for (i = 0 ; i < info->fix.smem_len; i += PAGE_SIZE) {
> + page = fb_deferred_io_page(info, i);
> + page->mapping = NULL;
> + }
> +
> mutex_destroy(&fbdefio->lock);
> }
> EXPORT_SYMBOL_GPL(fb_deferred_io_cleanup);
> diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
> index 072780b0e570..98f193078c05 100644
> --- a/drivers/video/fbdev/core/fbmem.c
> +++ b/drivers/video/fbdev/core/fbmem.c
> @@ -1415,6 +1415,10 @@ __releases(&info->lock)
> if (res)
> module_put(info->fbops->owner);
> }
> +#ifdef CONFIG_FB_DEFERRED_IO
> + if (info->fbdefio)
> + fb_deferred_io_open(info, inode, file);
> +#endif
> out:
> unlock_fb_info(info);
> if (res)
> diff --git a/include/linux/fb.h b/include/linux/fb.h
> index a8dccd23c249..ecfbcc0553a5 100644
> --- a/include/linux/fb.h
> +++ b/include/linux/fb.h
> @@ -659,6 +659,9 @@ static inline void __fb_pad_aligned_buffer(u8 *dst, u32 d_pitch,
> /* drivers/video/fb_defio.c */
> int fb_deferred_io_mmap(struct fb_info *info, struct vm_area_struct *vma);
> extern void fb_deferred_io_init(struct fb_info *info);
> +extern void fb_deferred_io_open(struct fb_info *info,
> + struct inode *inode,
> + struct file *file);
> extern void fb_deferred_io_cleanup(struct fb_info *info);
> extern int fb_deferred_io_fsync(struct file *file, loff_t start,
> loff_t end, int datasync);
> --
> 2.30.2
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch