Re: [PATCH] bochs: convert to drm_fb_helper_fbdev_setup/teardown
Hi Peter, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on linus/master] [also build test WARNING on v4.19-rc2 next-20180906] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Peter-Wu/bochs-convert-to-drm_fb_helper_fbdev_setup-teardown/20180907-154819 config: i386-randconfig-x006-201835 (attached as .config) compiler: gcc-7 (Debian 7.3.0-1) 7.3.0 reproduce: # save the attached .config to linux build tree make ARCH=i386 All warnings (new ones prefixed by >>): In file included from include/asm-generic/bug.h:5:0, from arch/x86/include/asm/bug.h:83, from include/linux/bug.h:5, from include/linux/mmdebug.h:5, from include/linux/mm.h:9, from drivers/gpu/drm/bochs/bochs_drv.c:8: drivers/gpu/drm/bochs/bochs_drv.c: In function 'bochs_pm_suspend': drivers/gpu/drm/bochs/bochs_drv.c:110:15: error: 'struct ' has no member named 'initialized' if (bochs->fb.initialized) { ^ include/linux/compiler.h:58:30: note: in definition of macro '__trace_if' if (__builtin_constant_p(!!(cond)) ? !!(cond) : \ ^~~~ >> drivers/gpu/drm/bochs/bochs_drv.c:110:2: note: in expansion of macro 'if' if (bochs->fb.initialized) { ^~ drivers/gpu/drm/bochs/bochs_drv.c:110:15: error: 'struct ' has no member named 'initialized' if (bochs->fb.initialized) { ^ include/linux/compiler.h:58:42: note: in definition of macro '__trace_if' if (__builtin_constant_p(!!(cond)) ? !!(cond) : \ ^~~~ >> drivers/gpu/drm/bochs/bochs_drv.c:110:2: note: in expansion of macro 'if' if (bochs->fb.initialized) { ^~ drivers/gpu/drm/bochs/bochs_drv.c:110:15: error: 'struct ' has no member named 'initialized' if (bochs->fb.initialized) { ^ include/linux/compiler.h:69:16: note: in definition of macro '__trace_if' __r = !!(cond); \ ^~~~ >> drivers/gpu/drm/bochs/bochs_drv.c:110:2: note: in expansion of macro 'if' if (bochs->fb.initialized) { ^~ drivers/gpu/drm/bochs/bochs_drv.c: In function 'bochs_pm_resume': drivers/gpu/drm/bochs/bochs_drv.c:127:15: error: 'struct ' has no member named 'initialized' if (bochs->fb.initialized) { ^ include/linux/compiler.h:58:30: note: in definition of macro '__trace_if' if (__builtin_constant_p(!!(cond)) ? !!(cond) : \ ^~~~ drivers/gpu/drm/bochs/bochs_drv.c:127:2: note: in expansion of macro 'if' if (bochs->fb.initialized) { ^~ drivers/gpu/drm/bochs/bochs_drv.c:127:15: error: 'struct ' has no member named 'initialized' if (bochs->fb.initialized) { ^ include/linux/compiler.h:58:42: note: in definition of macro '__trace_if' if (__builtin_constant_p(!!(cond)) ? !!(cond) : \ ^~~~ drivers/gpu/drm/bochs/bochs_drv.c:127:2: note: in expansion of macro 'if' if (bochs->fb.initialized) { ^~ drivers/gpu/drm/bochs/bochs_drv.c:127:15: error: 'struct ' has no member named 'initialized' if (bochs->fb.initialized) { ^ include/linux/compiler.h:69:16: note: in definition of macro '__trace_if' __r = !!(cond); \ ^~~~ drivers/gpu/drm/bochs/bochs_drv.c:127:2: note: in expansion of macro 'if' if (bochs->fb.initialized) { ^~ vim +/if +110 drivers/gpu/drm/bochs/bochs_drv.c 0a6659bd Gerd Hoffmann 2013-12-17 @8 #include 0a6659bd Gerd Hoffmann 2013-12-179 #include 0a6659bd Gerd Hoffmann 2013-12-17 10 #include 44adece5 Daniel Vetter 2016-08-10 11 #include 0a6659bd Gerd Hoffmann 2013-12-17 12 0a6659bd Gerd Hoffmann 2013-12-17 13 #include "bochs.h" 0a6659bd Gerd Hoffmann 2013-12-17 14 1acf5661 Max Staudt 2017-01-18 15 static int bochs_modeset = -1; 1acf5661 Max Staudt 2017-01-18 16 module_param_named(modeset, bochs_modeset, int, 0444); 1acf5661 Max Staudt 2017-01-18 17 MODULE_PARM_DESC(modeset, "enable/disable kernel modesetting"); 1acf5661 Max Staudt 2017-01-18 18 0a6659bd Gerd Hoffmann 2013-12-17 19 static bool enable_fbdev = true; 0a6659bd Gerd Hoffmann 2013-12-17 20 module_param_named(fbdev, enable_fbdev, bool, 0444); 0a6659bd Gerd Hoffmann 2013-12-17 21 MODULE_PARM_DESC(fbdev, "register fbdev device"); 0a6659bd Gerd Hoffmann 2013-12-17 22 0a6659bd Gerd Hoffmann 2013-12-17 23 /* -- */ 0a6659bd Gerd Hoffmann
Re: [PATCH] bochs: convert to drm_fb_helper_fbdev_setup/teardown
Hi, > > You can probably get rid of this one if you're refactoring even more. The > > generic fb_probe implementation (already merged) plus gem-shmem support > > for it (still in flight) from Noralf should be able to pull that off. That > > gives you the fb_mmap implementation, but with 100% generic code instead > > of a driver specific hack like Max did. > > Aside from the warning, I have not observed actual issues. This patch > was prepared on top of v4.18.1 but the new drm_fb_helper_generic_probe > helper is in master (future 4.19). I suppose that it can be done as a > future cleanup. Nice work Noralf on reducing duplication! FYI: qemu kms driver patches go through drm-misc-next, so you can also work against that branch. > > I'll leave merging to Gerd. > > Thanks, I somehow missed a patch. This one does not compile due to > "fb.initialized" still being used in bochs_drv.c. Removal is trivial, > I'll wait for some more feedback and then send a v2 with another patch > prepended. Patch looks good to me (except for the build failure which you've noticed already). cheers, Gerd ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] bochs: convert to drm_fb_helper_fbdev_setup/teardown
On Wed, Sep 05, 2018 at 04:46:29PM +0200, Daniel Vetter wrote: > On Wed, Sep 05, 2018 at 04:41:27PM +0200, Peter Wu wrote: > > Currently unloading bochs_drm (after unbinding the vtconsole) results in > > a warning about a leaked connector: > > > > [drm:drm_mode_config_cleanup] *ERROR* connector Virtual-3 leaked! > > > > While investigating a potential fix I noticed that a lot of open-coded > > functionality is already implemented elsewhere, so start converting it: > > bochs_fbdev_init -> drm_fb_helper_fbdev_setup: trivial (similar impl). > > bochs_fbdev_fini -> drm_fb_helper_fbdev_teardown: requires unembedding > > "struct drm_framebuffer" from "struct bochs_framebuffer". > > > > Unembedding drm_framebuffer is made easy using drm_gem_fbdev_fb_create > > which can replace bochs_fbdev_destroy and custom routines in bochs_mm.c. > > For this to work, the GEM object is moved into "drm_framebuffer". After > > that, "bochs_framebuffer" is no longer needed and therefore removed. > > > > Remove the unused "size" and "initialized" fields from fb, the latter is > > not necessary as drm_fb_helper_fbdev_teardown can be called even if > > bochsfb_create fails. This theory was tested by returning early and > > late (just before drm_gem_fbdev_fb_create). Both scenarios fail > > gracefully although the latter seems to leak the object from > > bochsfb_create_object (not a regression). > > > > Guess on the reason for the encoder leak: drm_framebuffer_cleanup was > > previously used, but did not destroy much. drm_fb_helper_fbdev_teardown > > is now used and calls drm_framebuffer_remove which does a bit more work. > > > > Tested with 'echo 0 > /sys/class/vtconsole/vtcon1/bind; rmmod bochs_drm' > > and also with Xorg + fbdev (startx -> xterm). The latter triggered a > > warning in ttm_bo_vm_open that existed before, see > > https://lkml.kernel.org/r/1464000533-13140-4-git-send-email-msta...@suse.de > > You can probably get rid of this one if you're refactoring even more. The > generic fb_probe implementation (already merged) plus gem-shmem support > for it (still in flight) from Noralf should be able to pull that off. That > gives you the fb_mmap implementation, but with 100% generic code instead > of a driver specific hack like Max did. Aside from the warning, I have not observed actual issues. This patch was prepared on top of v4.18.1 but the new drm_fb_helper_generic_probe helper is in master (future 4.19). I suppose that it can be done as a future cleanup. Nice work Noralf on reducing duplication! . > > > > Signed-off-by: Peter Wu > > lgtm. Acked-by: Daniel Vetter > > I'll leave merging to Gerd. > -Daniel Thanks, I somehow missed a patch. This one does not compile due to "fb.initialized" still being used in bochs_drv.c. Removal is trivial, I'll wait for some more feedback and then send a v2 with another patch prepended. Kind regards, Peter > > --- > > drivers/gpu/drm/bochs/bochs.h | 19 ++- > > drivers/gpu/drm/bochs/bochs_fbdev.c | 79 +++-- > > drivers/gpu/drm/bochs/bochs_kms.c | 7 +-- > > drivers/gpu/drm/bochs/bochs_mm.c| 74 --- > > 4 files changed, 22 insertions(+), 157 deletions(-) > > > > diff --git a/drivers/gpu/drm/bochs/bochs.h b/drivers/gpu/drm/bochs/bochs.h > > index 375bf92cd04f..8514a84fbdbe 100644 > > --- a/drivers/gpu/drm/bochs/bochs.h > > +++ b/drivers/gpu/drm/bochs/bochs.h > > @@ -51,11 +51,6 @@ enum bochs_types { > > BOCHS_UNKNOWN, > > }; > > > > -struct bochs_framebuffer { > > - struct drm_framebuffer base; > > - struct drm_gem_object *obj; > > -}; > > - > > struct bochs_device { > > /* hw */ > > void __iomem *mmio; > > @@ -88,15 +83,11 @@ struct bochs_device { > > > > /* fbdev */ > > struct { > > - struct bochs_framebuffer gfb; > > + struct drm_framebuffer *fb; > > struct drm_fb_helper helper; > > - int size; > > - bool initialized; > > } fb; > > }; > > > > -#define to_bochs_framebuffer(x) container_of(x, struct bochs_framebuffer, > > base) > > - > > struct bochs_bo { > > struct ttm_buffer_object bo; > > struct ttm_placement placement; > > @@ -148,15 +139,9 @@ int bochs_dumb_create(struct drm_file *file, struct > > drm_device *dev, > > int bochs_dumb_mmap_offset(struct drm_file *file, struct drm_device *dev, > >uint32_t handle, uint64_t *offset); > > > > -int bochs_framebuffer_init(struct drm_device *dev, > > - struct bochs_framebuffer *gfb, > > - const struct drm_mode_fb_cmd2 *mode_cmd, > > - struct drm_gem_object *obj); > > int bochs_bo_pin(struct bochs_bo *bo, u32 pl_flag, u64 *gpu_addr); > > int bochs_bo_unpin(struct bochs_bo *bo); > > > > -extern const struct drm_mode_config_funcs bochs_mode_funcs; > > - > > /* bochs_kms.c */ > > int bochs_kms_init(struct bochs_device *bochs); > > void bochs_kms_fini(struct
Re: [PATCH] bochs: convert to drm_fb_helper_fbdev_setup/teardown
On Wed, Sep 05, 2018 at 04:41:27PM +0200, Peter Wu wrote: > Currently unloading bochs_drm (after unbinding the vtconsole) results in > a warning about a leaked connector: > > [drm:drm_mode_config_cleanup] *ERROR* connector Virtual-3 leaked! > > While investigating a potential fix I noticed that a lot of open-coded > functionality is already implemented elsewhere, so start converting it: > bochs_fbdev_init -> drm_fb_helper_fbdev_setup: trivial (similar impl). > bochs_fbdev_fini -> drm_fb_helper_fbdev_teardown: requires unembedding > "struct drm_framebuffer" from "struct bochs_framebuffer". > > Unembedding drm_framebuffer is made easy using drm_gem_fbdev_fb_create > which can replace bochs_fbdev_destroy and custom routines in bochs_mm.c. > For this to work, the GEM object is moved into "drm_framebuffer". After > that, "bochs_framebuffer" is no longer needed and therefore removed. > > Remove the unused "size" and "initialized" fields from fb, the latter is > not necessary as drm_fb_helper_fbdev_teardown can be called even if > bochsfb_create fails. This theory was tested by returning early and > late (just before drm_gem_fbdev_fb_create). Both scenarios fail > gracefully although the latter seems to leak the object from > bochsfb_create_object (not a regression). > > Guess on the reason for the encoder leak: drm_framebuffer_cleanup was > previously used, but did not destroy much. drm_fb_helper_fbdev_teardown > is now used and calls drm_framebuffer_remove which does a bit more work. > > Tested with 'echo 0 > /sys/class/vtconsole/vtcon1/bind; rmmod bochs_drm' > and also with Xorg + fbdev (startx -> xterm). The latter triggered a > warning in ttm_bo_vm_open that existed before, see > https://lkml.kernel.org/r/1464000533-13140-4-git-send-email-msta...@suse.de You can probably get rid of this one if you're refactoring even more. The generic fb_probe implementation (already merged) plus gem-shmem support for it (still in flight) from Noralf should be able to pull that off. That gives you the fb_mmap implementation, but with 100% generic code instead of a driver specific hack like Max did. > > Signed-off-by: Peter Wu lgtm. Acked-by: Daniel Vetter I'll leave merging to Gerd. -Daniel > --- > drivers/gpu/drm/bochs/bochs.h | 19 ++- > drivers/gpu/drm/bochs/bochs_fbdev.c | 79 +++-- > drivers/gpu/drm/bochs/bochs_kms.c | 7 +-- > drivers/gpu/drm/bochs/bochs_mm.c| 74 --- > 4 files changed, 22 insertions(+), 157 deletions(-) > > diff --git a/drivers/gpu/drm/bochs/bochs.h b/drivers/gpu/drm/bochs/bochs.h > index 375bf92cd04f..8514a84fbdbe 100644 > --- a/drivers/gpu/drm/bochs/bochs.h > +++ b/drivers/gpu/drm/bochs/bochs.h > @@ -51,11 +51,6 @@ enum bochs_types { > BOCHS_UNKNOWN, > }; > > -struct bochs_framebuffer { > - struct drm_framebuffer base; > - struct drm_gem_object *obj; > -}; > - > struct bochs_device { > /* hw */ > void __iomem *mmio; > @@ -88,15 +83,11 @@ struct bochs_device { > > /* fbdev */ > struct { > - struct bochs_framebuffer gfb; > + struct drm_framebuffer *fb; > struct drm_fb_helper helper; > - int size; > - bool initialized; > } fb; > }; > > -#define to_bochs_framebuffer(x) container_of(x, struct bochs_framebuffer, > base) > - > struct bochs_bo { > struct ttm_buffer_object bo; > struct ttm_placement placement; > @@ -148,15 +139,9 @@ int bochs_dumb_create(struct drm_file *file, struct > drm_device *dev, > int bochs_dumb_mmap_offset(struct drm_file *file, struct drm_device *dev, > uint32_t handle, uint64_t *offset); > > -int bochs_framebuffer_init(struct drm_device *dev, > -struct bochs_framebuffer *gfb, > -const struct drm_mode_fb_cmd2 *mode_cmd, > -struct drm_gem_object *obj); > int bochs_bo_pin(struct bochs_bo *bo, u32 pl_flag, u64 *gpu_addr); > int bochs_bo_unpin(struct bochs_bo *bo); > > -extern const struct drm_mode_config_funcs bochs_mode_funcs; > - > /* bochs_kms.c */ > int bochs_kms_init(struct bochs_device *bochs); > void bochs_kms_fini(struct bochs_device *bochs); > @@ -164,3 +149,5 @@ void bochs_kms_fini(struct bochs_device *bochs); > /* bochs_fbdev.c */ > int bochs_fbdev_init(struct bochs_device *bochs); > void bochs_fbdev_fini(struct bochs_device *bochs); > + > +extern const struct drm_mode_config_funcs bochs_mode_funcs; > diff --git a/drivers/gpu/drm/bochs/bochs_fbdev.c > b/drivers/gpu/drm/bochs/bochs_fbdev.c > index 14eb8d0d5a00..8f4d6c052f7b 100644 > --- a/drivers/gpu/drm/bochs/bochs_fbdev.c > +++ b/drivers/gpu/drm/bochs/bochs_fbdev.c > @@ -6,6 +6,7 @@ > */ > > #include "bochs.h" > +#include > > /* -- */ > > @@ -13,9 +14,7 @@ static int bochsfb_mmap(struct fb_info *info, >
[PATCH] bochs: convert to drm_fb_helper_fbdev_setup/teardown
Currently unloading bochs_drm (after unbinding the vtconsole) results in a warning about a leaked connector: [drm:drm_mode_config_cleanup] *ERROR* connector Virtual-3 leaked! While investigating a potential fix I noticed that a lot of open-coded functionality is already implemented elsewhere, so start converting it: bochs_fbdev_init -> drm_fb_helper_fbdev_setup: trivial (similar impl). bochs_fbdev_fini -> drm_fb_helper_fbdev_teardown: requires unembedding "struct drm_framebuffer" from "struct bochs_framebuffer". Unembedding drm_framebuffer is made easy using drm_gem_fbdev_fb_create which can replace bochs_fbdev_destroy and custom routines in bochs_mm.c. For this to work, the GEM object is moved into "drm_framebuffer". After that, "bochs_framebuffer" is no longer needed and therefore removed. Remove the unused "size" and "initialized" fields from fb, the latter is not necessary as drm_fb_helper_fbdev_teardown can be called even if bochsfb_create fails. This theory was tested by returning early and late (just before drm_gem_fbdev_fb_create). Both scenarios fail gracefully although the latter seems to leak the object from bochsfb_create_object (not a regression). Guess on the reason for the encoder leak: drm_framebuffer_cleanup was previously used, but did not destroy much. drm_fb_helper_fbdev_teardown is now used and calls drm_framebuffer_remove which does a bit more work. Tested with 'echo 0 > /sys/class/vtconsole/vtcon1/bind; rmmod bochs_drm' and also with Xorg + fbdev (startx -> xterm). The latter triggered a warning in ttm_bo_vm_open that existed before, see https://lkml.kernel.org/r/1464000533-13140-4-git-send-email-msta...@suse.de Signed-off-by: Peter Wu --- drivers/gpu/drm/bochs/bochs.h | 19 ++- drivers/gpu/drm/bochs/bochs_fbdev.c | 79 +++-- drivers/gpu/drm/bochs/bochs_kms.c | 7 +-- drivers/gpu/drm/bochs/bochs_mm.c| 74 --- 4 files changed, 22 insertions(+), 157 deletions(-) diff --git a/drivers/gpu/drm/bochs/bochs.h b/drivers/gpu/drm/bochs/bochs.h index 375bf92cd04f..8514a84fbdbe 100644 --- a/drivers/gpu/drm/bochs/bochs.h +++ b/drivers/gpu/drm/bochs/bochs.h @@ -51,11 +51,6 @@ enum bochs_types { BOCHS_UNKNOWN, }; -struct bochs_framebuffer { - struct drm_framebuffer base; - struct drm_gem_object *obj; -}; - struct bochs_device { /* hw */ void __iomem *mmio; @@ -88,15 +83,11 @@ struct bochs_device { /* fbdev */ struct { - struct bochs_framebuffer gfb; + struct drm_framebuffer *fb; struct drm_fb_helper helper; - int size; - bool initialized; } fb; }; -#define to_bochs_framebuffer(x) container_of(x, struct bochs_framebuffer, base) - struct bochs_bo { struct ttm_buffer_object bo; struct ttm_placement placement; @@ -148,15 +139,9 @@ int bochs_dumb_create(struct drm_file *file, struct drm_device *dev, int bochs_dumb_mmap_offset(struct drm_file *file, struct drm_device *dev, uint32_t handle, uint64_t *offset); -int bochs_framebuffer_init(struct drm_device *dev, - struct bochs_framebuffer *gfb, - const struct drm_mode_fb_cmd2 *mode_cmd, - struct drm_gem_object *obj); int bochs_bo_pin(struct bochs_bo *bo, u32 pl_flag, u64 *gpu_addr); int bochs_bo_unpin(struct bochs_bo *bo); -extern const struct drm_mode_config_funcs bochs_mode_funcs; - /* bochs_kms.c */ int bochs_kms_init(struct bochs_device *bochs); void bochs_kms_fini(struct bochs_device *bochs); @@ -164,3 +149,5 @@ void bochs_kms_fini(struct bochs_device *bochs); /* bochs_fbdev.c */ int bochs_fbdev_init(struct bochs_device *bochs); void bochs_fbdev_fini(struct bochs_device *bochs); + +extern const struct drm_mode_config_funcs bochs_mode_funcs; diff --git a/drivers/gpu/drm/bochs/bochs_fbdev.c b/drivers/gpu/drm/bochs/bochs_fbdev.c index 14eb8d0d5a00..8f4d6c052f7b 100644 --- a/drivers/gpu/drm/bochs/bochs_fbdev.c +++ b/drivers/gpu/drm/bochs/bochs_fbdev.c @@ -6,6 +6,7 @@ */ #include "bochs.h" +#include /* -- */ @@ -13,9 +14,7 @@ static int bochsfb_mmap(struct fb_info *info, struct vm_area_struct *vma) { struct drm_fb_helper *fb_helper = info->par; - struct bochs_device *bochs = - container_of(fb_helper, struct bochs_device, fb.helper); - struct bochs_bo *bo = gem_to_bochs_bo(bochs->fb.gfb.obj); + struct bochs_bo *bo = gem_to_bochs_bo(fb_helper->fb->obj[0]); return ttm_fbdev_mmap(vma, >bo); } @@ -101,19 +100,20 @@ static int bochsfb_create(struct drm_fb_helper *helper, /* init fb device */ info = drm_fb_helper_alloc_fbi(helper); - if (IS_ERR(info)) + if (IS_ERR(info)) { + DRM_ERROR("Failed to allocate fbi: %ld\n",