| Message ID | 20180905144127.5690-1-peter@lekensteyn.nl (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | bochs: convert to drm_fb_helper_fbdev_setup/teardown | expand |
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-mstaudt@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 <peter@lekensteyn.nl> lgtm. Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch> 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 <drm/drm_gem_framebuffer_helper.h> > > /* ---------------------------------------------------------------------- */ > > @@ -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->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", PTR_ERR(info)); > return PTR_ERR(info); > + } > > info->par = &bochs->fb.helper; > > - ret = bochs_framebuffer_init(bochs->dev, &bochs->fb.gfb, &mode_cmd, gobj); > - if (ret) > - return ret; > - > - bochs->fb.size = size; > + fb = drm_gem_fbdev_fb_create(bochs->dev, sizes, 0, gobj, NULL); > + if (IS_ERR(fb)) { > + DRM_ERROR("Failed to create framebuffer: %ld\n", PTR_ERR(fb)); > + return PTR_ERR(fb); > + } > > /* setup helper */ > - fb = &bochs->fb.gfb.base; > bochs->fb.helper.fb = fb; > > strcpy(info->fix.id, "bochsdrmfb"); > @@ -130,27 +130,6 @@ static int bochsfb_create(struct drm_fb_helper *helper, > drm_vma_offset_remove(&bo->bo.bdev->vma_manager, &bo->bo.vma_node); > info->fix.smem_start = 0; > info->fix.smem_len = size; > - > - bochs->fb.initialized = true; > - return 0; > -} > - > -static int bochs_fbdev_destroy(struct bochs_device *bochs) > -{ > - struct bochs_framebuffer *gfb = &bochs->fb.gfb; > - > - DRM_DEBUG_DRIVER("\n"); > - > - drm_fb_helper_unregister_fbi(&bochs->fb.helper); > - > - if (gfb->obj) { > - drm_gem_object_unreference_unlocked(gfb->obj); > - gfb->obj = NULL; > - } > - > - drm_framebuffer_unregister_private(&gfb->base); > - drm_framebuffer_cleanup(&gfb->base); > - > return 0; > } > > @@ -158,41 +137,17 @@ static const struct drm_fb_helper_funcs bochs_fb_helper_funcs = { > .fb_probe = bochsfb_create, > }; > > +const struct drm_mode_config_funcs bochs_mode_funcs = { > + .fb_create = drm_gem_fb_create, > +}; > + > int bochs_fbdev_init(struct bochs_device *bochs) > { > - int ret; > - > - drm_fb_helper_prepare(bochs->dev, &bochs->fb.helper, > - &bochs_fb_helper_funcs); > - > - ret = drm_fb_helper_init(bochs->dev, &bochs->fb.helper, 1); > - if (ret) > - return ret; > - > - ret = drm_fb_helper_single_add_all_connectors(&bochs->fb.helper); > - if (ret) > - goto fini; > - > - drm_helper_disable_unused_functions(bochs->dev); > - > - ret = drm_fb_helper_initial_config(&bochs->fb.helper, 32); > - if (ret) > - goto fini; > - > - return 0; > - > -fini: > - drm_fb_helper_fini(&bochs->fb.helper); > - return ret; > + return drm_fb_helper_fbdev_setup(bochs->dev, &bochs->fb.helper, > + &bochs_fb_helper_funcs, 32, 1); > } > > void bochs_fbdev_fini(struct bochs_device *bochs) > { > - if (bochs->fb.initialized) > - bochs_fbdev_destroy(bochs); > - > - if (bochs->fb.helper.fbdev) > - drm_fb_helper_fini(&bochs->fb.helper); > - > - bochs->fb.initialized = false; > + drm_fb_helper_fbdev_teardown(bochs->dev); > } > diff --git a/drivers/gpu/drm/bochs/bochs_kms.c b/drivers/gpu/drm/bochs/bochs_kms.c > index 233980a78591..c7e575511d2f 100644 > --- a/drivers/gpu/drm/bochs/bochs_kms.c > +++ b/drivers/gpu/drm/bochs/bochs_kms.c > @@ -35,14 +35,12 @@ static int bochs_crtc_mode_set_base(struct drm_crtc *crtc, int x, int y, > { > struct bochs_device *bochs = > container_of(crtc, struct bochs_device, crtc); > - struct bochs_framebuffer *bochs_fb; > struct bochs_bo *bo; > u64 gpu_addr = 0; > int ret; > > if (old_fb) { > - bochs_fb = to_bochs_framebuffer(old_fb); > - bo = gem_to_bochs_bo(bochs_fb->obj); > + bo = gem_to_bochs_bo(old_fb->obj[0]); > ret = ttm_bo_reserve(&bo->bo, true, false, NULL); > if (ret) { > DRM_ERROR("failed to reserve old_fb bo\n"); > @@ -55,8 +53,7 @@ static int bochs_crtc_mode_set_base(struct drm_crtc *crtc, int x, int y, > if (WARN_ON(crtc->primary->fb == NULL)) > return -EINVAL; > > - bochs_fb = to_bochs_framebuffer(crtc->primary->fb); > - bo = gem_to_bochs_bo(bochs_fb->obj); > + bo = gem_to_bochs_bo(crtc->primary->fb->obj[0]); > ret = ttm_bo_reserve(&bo->bo, true, false, NULL); > if (ret) > return ret; > diff --git a/drivers/gpu/drm/bochs/bochs_mm.c b/drivers/gpu/drm/bochs/bochs_mm.c > index 39cd08416773..cdbc3ca3ec51 100644 > --- a/drivers/gpu/drm/bochs/bochs_mm.c > +++ b/drivers/gpu/drm/bochs/bochs_mm.c > @@ -457,77 +457,3 @@ int bochs_dumb_mmap_offset(struct drm_file *file, struct drm_device *dev, > drm_gem_object_unreference_unlocked(obj); > return 0; > } > - > -/* ---------------------------------------------------------------------- */ > - > -static void bochs_user_framebuffer_destroy(struct drm_framebuffer *fb) > -{ > - struct bochs_framebuffer *bochs_fb = to_bochs_framebuffer(fb); > - > - drm_gem_object_unreference_unlocked(bochs_fb->obj); > - drm_framebuffer_cleanup(fb); > - kfree(fb); > -} > - > -static const struct drm_framebuffer_funcs bochs_fb_funcs = { > - .destroy = bochs_user_framebuffer_destroy, > -}; > - > -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 ret; > - > - drm_helper_mode_fill_fb_struct(dev, &gfb->base, mode_cmd); > - gfb->obj = obj; > - ret = drm_framebuffer_init(dev, &gfb->base, &bochs_fb_funcs); > - if (ret) { > - DRM_ERROR("drm_framebuffer_init failed: %d\n", ret); > - return ret; > - } > - return 0; > -} > - > -static struct drm_framebuffer * > -bochs_user_framebuffer_create(struct drm_device *dev, > - struct drm_file *filp, > - const struct drm_mode_fb_cmd2 *mode_cmd) > -{ > - struct drm_gem_object *obj; > - struct bochs_framebuffer *bochs_fb; > - int ret; > - > - DRM_DEBUG_DRIVER("%dx%d, format %c%c%c%c\n", > - mode_cmd->width, mode_cmd->height, > - (mode_cmd->pixel_format) & 0xff, > - (mode_cmd->pixel_format >> 8) & 0xff, > - (mode_cmd->pixel_format >> 16) & 0xff, > - (mode_cmd->pixel_format >> 24) & 0xff); > - > - if (mode_cmd->pixel_format != DRM_FORMAT_XRGB8888) > - return ERR_PTR(-ENOENT); > - > - obj = drm_gem_object_lookup(filp, mode_cmd->handles[0]); > - if (obj == NULL) > - return ERR_PTR(-ENOENT); > - > - bochs_fb = kzalloc(sizeof(*bochs_fb), GFP_KERNEL); > - if (!bochs_fb) { > - drm_gem_object_unreference_unlocked(obj); > - return ERR_PTR(-ENOMEM); > - } > - > - ret = bochs_framebuffer_init(dev, bochs_fb, mode_cmd, obj); > - if (ret) { > - drm_gem_object_unreference_unlocked(obj); > - kfree(bochs_fb); > - return ERR_PTR(ret); > - } > - return &bochs_fb->base; > -} > - > -const struct drm_mode_config_funcs bochs_mode_funcs = { > - .fb_create = bochs_user_framebuffer_create, > -}; > -- > 2.18.0 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
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-mstaudt@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 <peter@lekensteyn.nl> > > lgtm. Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > 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 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 <drm/drm_gem_framebuffer_helper.h> > > > > /* ---------------------------------------------------------------------- */ > > > > @@ -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->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", PTR_ERR(info)); > > return PTR_ERR(info); > > + } > > > > info->par = &bochs->fb.helper; > > > > - ret = bochs_framebuffer_init(bochs->dev, &bochs->fb.gfb, &mode_cmd, gobj); > > - if (ret) > > - return ret; > > - > > - bochs->fb.size = size; > > + fb = drm_gem_fbdev_fb_create(bochs->dev, sizes, 0, gobj, NULL); > > + if (IS_ERR(fb)) { > > + DRM_ERROR("Failed to create framebuffer: %ld\n", PTR_ERR(fb)); > > + return PTR_ERR(fb); > > + } > > > > /* setup helper */ > > - fb = &bochs->fb.gfb.base; > > bochs->fb.helper.fb = fb; > > > > strcpy(info->fix.id, "bochsdrmfb"); > > @@ -130,27 +130,6 @@ static int bochsfb_create(struct drm_fb_helper *helper, > > drm_vma_offset_remove(&bo->bo.bdev->vma_manager, &bo->bo.vma_node); > > info->fix.smem_start = 0; > > info->fix.smem_len = size; > > - > > - bochs->fb.initialized = true; > > - return 0; > > -} > > - > > -static int bochs_fbdev_destroy(struct bochs_device *bochs) > > -{ > > - struct bochs_framebuffer *gfb = &bochs->fb.gfb; > > - > > - DRM_DEBUG_DRIVER("\n"); > > - > > - drm_fb_helper_unregister_fbi(&bochs->fb.helper); > > - > > - if (gfb->obj) { > > - drm_gem_object_unreference_unlocked(gfb->obj); > > - gfb->obj = NULL; > > - } > > - > > - drm_framebuffer_unregister_private(&gfb->base); > > - drm_framebuffer_cleanup(&gfb->base); > > - > > return 0; > > } > > > > @@ -158,41 +137,17 @@ static const struct drm_fb_helper_funcs bochs_fb_helper_funcs = { > > .fb_probe = bochsfb_create, > > }; > > > > +const struct drm_mode_config_funcs bochs_mode_funcs = { > > + .fb_create = drm_gem_fb_create, > > +}; > > + > > int bochs_fbdev_init(struct bochs_device *bochs) > > { > > - int ret; > > - > > - drm_fb_helper_prepare(bochs->dev, &bochs->fb.helper, > > - &bochs_fb_helper_funcs); > > - > > - ret = drm_fb_helper_init(bochs->dev, &bochs->fb.helper, 1); > > - if (ret) > > - return ret; > > - > > - ret = drm_fb_helper_single_add_all_connectors(&bochs->fb.helper); > > - if (ret) > > - goto fini; > > - > > - drm_helper_disable_unused_functions(bochs->dev); > > - > > - ret = drm_fb_helper_initial_config(&bochs->fb.helper, 32); > > - if (ret) > > - goto fini; > > - > > - return 0; > > - > > -fini: > > - drm_fb_helper_fini(&bochs->fb.helper); > > - return ret; > > + return drm_fb_helper_fbdev_setup(bochs->dev, &bochs->fb.helper, > > + &bochs_fb_helper_funcs, 32, 1); > > } > > > > void bochs_fbdev_fini(struct bochs_device *bochs) > > { > > - if (bochs->fb.initialized) > > - bochs_fbdev_destroy(bochs); > > - > > - if (bochs->fb.helper.fbdev) > > - drm_fb_helper_fini(&bochs->fb.helper); > > - > > - bochs->fb.initialized = false; > > + drm_fb_helper_fbdev_teardown(bochs->dev); > > } > > diff --git a/drivers/gpu/drm/bochs/bochs_kms.c b/drivers/gpu/drm/bochs/bochs_kms.c > > index 233980a78591..c7e575511d2f 100644 > > --- a/drivers/gpu/drm/bochs/bochs_kms.c > > +++ b/drivers/gpu/drm/bochs/bochs_kms.c > > @@ -35,14 +35,12 @@ static int bochs_crtc_mode_set_base(struct drm_crtc *crtc, int x, int y, > > { > > struct bochs_device *bochs = > > container_of(crtc, struct bochs_device, crtc); > > - struct bochs_framebuffer *bochs_fb; > > struct bochs_bo *bo; > > u64 gpu_addr = 0; > > int ret; > > > > if (old_fb) { > > - bochs_fb = to_bochs_framebuffer(old_fb); > > - bo = gem_to_bochs_bo(bochs_fb->obj); > > + bo = gem_to_bochs_bo(old_fb->obj[0]); > > ret = ttm_bo_reserve(&bo->bo, true, false, NULL); > > if (ret) { > > DRM_ERROR("failed to reserve old_fb bo\n"); > > @@ -55,8 +53,7 @@ static int bochs_crtc_mode_set_base(struct drm_crtc *crtc, int x, int y, > > if (WARN_ON(crtc->primary->fb == NULL)) > > return -EINVAL; > > > > - bochs_fb = to_bochs_framebuffer(crtc->primary->fb); > > - bo = gem_to_bochs_bo(bochs_fb->obj); > > + bo = gem_to_bochs_bo(crtc->primary->fb->obj[0]); > > ret = ttm_bo_reserve(&bo->bo, true, false, NULL); > > if (ret) > > return ret; > > diff --git a/drivers/gpu/drm/bochs/bochs_mm.c b/drivers/gpu/drm/bochs/bochs_mm.c > > index 39cd08416773..cdbc3ca3ec51 100644 > > --- a/drivers/gpu/drm/bochs/bochs_mm.c > > +++ b/drivers/gpu/drm/bochs/bochs_mm.c > > @@ -457,77 +457,3 @@ int bochs_dumb_mmap_offset(struct drm_file *file, struct drm_device *dev, > > drm_gem_object_unreference_unlocked(obj); > > return 0; > > } > > - > > -/* ---------------------------------------------------------------------- */ > > - > > -static void bochs_user_framebuffer_destroy(struct drm_framebuffer *fb) > > -{ > > - struct bochs_framebuffer *bochs_fb = to_bochs_framebuffer(fb); > > - > > - drm_gem_object_unreference_unlocked(bochs_fb->obj); > > - drm_framebuffer_cleanup(fb); > > - kfree(fb); > > -} > > - > > -static const struct drm_framebuffer_funcs bochs_fb_funcs = { > > - .destroy = bochs_user_framebuffer_destroy, > > -}; > > - > > -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 ret; > > - > > - drm_helper_mode_fill_fb_struct(dev, &gfb->base, mode_cmd); > > - gfb->obj = obj; > > - ret = drm_framebuffer_init(dev, &gfb->base, &bochs_fb_funcs); > > - if (ret) { > > - DRM_ERROR("drm_framebuffer_init failed: %d\n", ret); > > - return ret; > > - } > > - return 0; > > -} > > - > > -static struct drm_framebuffer * > > -bochs_user_framebuffer_create(struct drm_device *dev, > > - struct drm_file *filp, > > - const struct drm_mode_fb_cmd2 *mode_cmd) > > -{ > > - struct drm_gem_object *obj; > > - struct bochs_framebuffer *bochs_fb; > > - int ret; > > - > > - DRM_DEBUG_DRIVER("%dx%d, format %c%c%c%c\n", > > - mode_cmd->width, mode_cmd->height, > > - (mode_cmd->pixel_format) & 0xff, > > - (mode_cmd->pixel_format >> 8) & 0xff, > > - (mode_cmd->pixel_format >> 16) & 0xff, > > - (mode_cmd->pixel_format >> 24) & 0xff); > > - > > - if (mode_cmd->pixel_format != DRM_FORMAT_XRGB8888) > > - return ERR_PTR(-ENOENT); > > - > > - obj = drm_gem_object_lookup(filp, mode_cmd->handles[0]); > > - if (obj == NULL) > > - return ERR_PTR(-ENOENT); > > - > > - bochs_fb = kzalloc(sizeof(*bochs_fb), GFP_KERNEL); > > - if (!bochs_fb) { > > - drm_gem_object_unreference_unlocked(obj); > > - return ERR_PTR(-ENOMEM); > > - } > > - > > - ret = bochs_framebuffer_init(dev, bochs_fb, mode_cmd, obj); > > - if (ret) { > > - drm_gem_object_unreference_unlocked(obj); > > - kfree(bochs_fb); > > - return ERR_PTR(ret); > > - } > > - return &bochs_fb->base; > > -} > > - > > -const struct drm_mode_config_funcs bochs_mode_funcs = { > > - .fb_create = bochs_user_framebuffer_create, > > -}; > > -- > > 2.18.0 > > > > _______________________________________________ > > dri-devel mailing list > > dri-devel@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
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
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 <anonymous>' 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 <anonymous>' 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 <anonymous>' 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 <anonymous>' 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 <anonymous>' 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 <anonymous>' 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 <linux/mm.h>
0a6659bd Gerd Hoffmann 2013-12-17 9 #include <linux/module.h>
0a6659bd Gerd Hoffmann 2013-12-17 10 #include <linux/slab.h>
44adece5 Daniel Vetter 2016-08-10 11 #include <drm/drm_fb_helper.h>
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 2013-12-17 24 /* drm interface */
0a6659bd Gerd Hoffmann 2013-12-17 25
11b3c20b Gabriel Krisman Bertazi 2017-01-06 26 static void bochs_unload(struct drm_device *dev)
0a6659bd Gerd Hoffmann 2013-12-17 27 {
0a6659bd Gerd Hoffmann 2013-12-17 28 struct bochs_device *bochs = dev->dev_private;
0a6659bd Gerd Hoffmann 2013-12-17 29
0a6659bd Gerd Hoffmann 2013-12-17 30 bochs_fbdev_fini(bochs);
0a6659bd Gerd Hoffmann 2013-12-17 31 bochs_kms_fini(bochs);
0a6659bd Gerd Hoffmann 2013-12-17 32 bochs_mm_fini(bochs);
0a6659bd Gerd Hoffmann 2013-12-17 33 bochs_hw_fini(dev);
0a6659bd Gerd Hoffmann 2013-12-17 34 kfree(bochs);
0a6659bd Gerd Hoffmann 2013-12-17 35 dev->dev_private = NULL;
0a6659bd Gerd Hoffmann 2013-12-17 36 }
0a6659bd Gerd Hoffmann 2013-12-17 37
0a6659bd Gerd Hoffmann 2013-12-17 38 static int bochs_load(struct drm_device *dev, unsigned long flags)
0a6659bd Gerd Hoffmann 2013-12-17 39 {
0a6659bd Gerd Hoffmann 2013-12-17 40 struct bochs_device *bochs;
0a6659bd Gerd Hoffmann 2013-12-17 41 int ret;
0a6659bd Gerd Hoffmann 2013-12-17 42
0a6659bd Gerd Hoffmann 2013-12-17 43 bochs = kzalloc(sizeof(*bochs), GFP_KERNEL);
0a6659bd Gerd Hoffmann 2013-12-17 44 if (bochs == NULL)
0a6659bd Gerd Hoffmann 2013-12-17 45 return -ENOMEM;
0a6659bd Gerd Hoffmann 2013-12-17 46 dev->dev_private = bochs;
0a6659bd Gerd Hoffmann 2013-12-17 47 bochs->dev = dev;
0a6659bd Gerd Hoffmann 2013-12-17 48
0a6659bd Gerd Hoffmann 2013-12-17 49 ret = bochs_hw_init(dev, flags);
0a6659bd Gerd Hoffmann 2013-12-17 50 if (ret)
0a6659bd Gerd Hoffmann 2013-12-17 51 goto err;
0a6659bd Gerd Hoffmann 2013-12-17 52
0a6659bd Gerd Hoffmann 2013-12-17 53 ret = bochs_mm_init(bochs);
0a6659bd Gerd Hoffmann 2013-12-17 54 if (ret)
0a6659bd Gerd Hoffmann 2013-12-17 55 goto err;
0a6659bd Gerd Hoffmann 2013-12-17 56
0a6659bd Gerd Hoffmann 2013-12-17 57 ret = bochs_kms_init(bochs);
0a6659bd Gerd Hoffmann 2013-12-17 58 if (ret)
0a6659bd Gerd Hoffmann 2013-12-17 59 goto err;
0a6659bd Gerd Hoffmann 2013-12-17 60
0a6659bd Gerd Hoffmann 2013-12-17 61 if (enable_fbdev)
0a6659bd Gerd Hoffmann 2013-12-17 62 bochs_fbdev_init(bochs);
0a6659bd Gerd Hoffmann 2013-12-17 63
0a6659bd Gerd Hoffmann 2013-12-17 64 return 0;
0a6659bd Gerd Hoffmann 2013-12-17 65
0a6659bd Gerd Hoffmann 2013-12-17 66 err:
0a6659bd Gerd Hoffmann 2013-12-17 67 bochs_unload(dev);
0a6659bd Gerd Hoffmann 2013-12-17 68 return ret;
0a6659bd Gerd Hoffmann 2013-12-17 69 }
0a6659bd Gerd Hoffmann 2013-12-17 70
0a6659bd Gerd Hoffmann 2013-12-17 71 static const struct file_operations bochs_fops = {
0a6659bd Gerd Hoffmann 2013-12-17 72 .owner = THIS_MODULE,
0a6659bd Gerd Hoffmann 2013-12-17 73 .open = drm_open,
0a6659bd Gerd Hoffmann 2013-12-17 74 .release = drm_release,
0a6659bd Gerd Hoffmann 2013-12-17 75 .unlocked_ioctl = drm_ioctl,
0a6659bd Gerd Hoffmann 2013-12-17 76 .compat_ioctl = drm_compat_ioctl,
0a6659bd Gerd Hoffmann 2013-12-17 77 .poll = drm_poll,
0a6659bd Gerd Hoffmann 2013-12-17 78 .read = drm_read,
0a6659bd Gerd Hoffmann 2013-12-17 79 .llseek = no_llseek,
0a6659bd Gerd Hoffmann 2013-12-17 80 .mmap = bochs_mmap,
0a6659bd Gerd Hoffmann 2013-12-17 81 };
0a6659bd Gerd Hoffmann 2013-12-17 82
0a6659bd Gerd Hoffmann 2013-12-17 83 static struct drm_driver bochs_driver = {
0a6659bd Gerd Hoffmann 2013-12-17 84 .driver_features = DRIVER_GEM | DRIVER_MODESET,
0a6659bd Gerd Hoffmann 2013-12-17 85 .load = bochs_load,
0a6659bd Gerd Hoffmann 2013-12-17 86 .unload = bochs_unload,
0a6659bd Gerd Hoffmann 2013-12-17 87 .fops = &bochs_fops,
0a6659bd Gerd Hoffmann 2013-12-17 88 .name = "bochs-drm",
0a6659bd Gerd Hoffmann 2013-12-17 89 .desc = "bochs dispi vga interface (qemu stdvga)",
0a6659bd Gerd Hoffmann 2013-12-17 90 .date = "20130925",
0a6659bd Gerd Hoffmann 2013-12-17 91 .major = 1,
0a6659bd Gerd Hoffmann 2013-12-17 92 .minor = 0,
6f2c1c15 Daniel Vetter 2016-05-30 93 .gem_free_object_unlocked = bochs_gem_free_object,
0a6659bd Gerd Hoffmann 2013-12-17 94 .dumb_create = bochs_dumb_create,
0a6659bd Gerd Hoffmann 2013-12-17 95 .dumb_map_offset = bochs_dumb_mmap_offset,
0a6659bd Gerd Hoffmann 2013-12-17 96 };
0a6659bd Gerd Hoffmann 2013-12-17 97
0a6659bd Gerd Hoffmann 2013-12-17 98 /* ---------------------------------------------------------------------- */
b8ccd70f Gerd Hoffmann 2014-04-14 99 /* pm interface */
b8ccd70f Gerd Hoffmann 2014-04-14 100
150cee9c Russell King 2014-07-12 101 #ifdef CONFIG_PM_SLEEP
b8ccd70f Gerd Hoffmann 2014-04-14 102 static int bochs_pm_suspend(struct device *dev)
b8ccd70f Gerd Hoffmann 2014-04-14 103 {
b8ccd70f Gerd Hoffmann 2014-04-14 104 struct pci_dev *pdev = to_pci_dev(dev);
b8ccd70f Gerd Hoffmann 2014-04-14 105 struct drm_device *drm_dev = pci_get_drvdata(pdev);
b8ccd70f Gerd Hoffmann 2014-04-14 106 struct bochs_device *bochs = drm_dev->dev_private;
b8ccd70f Gerd Hoffmann 2014-04-14 107
b8ccd70f Gerd Hoffmann 2014-04-14 108 drm_kms_helper_poll_disable(drm_dev);
b8ccd70f Gerd Hoffmann 2014-04-14 109
b8ccd70f Gerd Hoffmann 2014-04-14 @110 if (bochs->fb.initialized) {
b8ccd70f Gerd Hoffmann 2014-04-14 111 console_lock();
6a752972 Archit Taneja 2015-07-31 112 drm_fb_helper_set_suspend(&bochs->fb.helper, 1);
b8ccd70f Gerd Hoffmann 2014-04-14 113 console_unlock();
b8ccd70f Gerd Hoffmann 2014-04-14 114 }
b8ccd70f Gerd Hoffmann 2014-04-14 115
b8ccd70f Gerd Hoffmann 2014-04-14 116 return 0;
b8ccd70f Gerd Hoffmann 2014-04-14 117 }
b8ccd70f Gerd Hoffmann 2014-04-14 118
:::::: The code at line 110 was first introduced by commit
:::::: b8ccd70f1363f7d4e49219dbc46ec973a14f49cd drm: bochs: add power management support
:::::: TO: Gerd Hoffmann <kraxel@redhat.com>
:::::: CC: Dave Airlie <airlied@redhat.com>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
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 <drm/drm_gem_framebuffer_helper.h> /* ---------------------------------------------------------------------- */ @@ -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->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", PTR_ERR(info)); return PTR_ERR(info); + } info->par = &bochs->fb.helper; - ret = bochs_framebuffer_init(bochs->dev, &bochs->fb.gfb, &mode_cmd, gobj); - if (ret) - return ret; - - bochs->fb.size = size; + fb = drm_gem_fbdev_fb_create(bochs->dev, sizes, 0, gobj, NULL); + if (IS_ERR(fb)) { + DRM_ERROR("Failed to create framebuffer: %ld\n", PTR_ERR(fb)); + return PTR_ERR(fb); + } /* setup helper */ - fb = &bochs->fb.gfb.base; bochs->fb.helper.fb = fb; strcpy(info->fix.id, "bochsdrmfb"); @@ -130,27 +130,6 @@ static int bochsfb_create(struct drm_fb_helper *helper, drm_vma_offset_remove(&bo->bo.bdev->vma_manager, &bo->bo.vma_node); info->fix.smem_start = 0; info->fix.smem_len = size; - - bochs->fb.initialized = true; - return 0; -} - -static int bochs_fbdev_destroy(struct bochs_device *bochs) -{ - struct bochs_framebuffer *gfb = &bochs->fb.gfb; - - DRM_DEBUG_DRIVER("\n"); - - drm_fb_helper_unregister_fbi(&bochs->fb.helper); - - if (gfb->obj) { - drm_gem_object_unreference_unlocked(gfb->obj); - gfb->obj = NULL; - } - - drm_framebuffer_unregister_private(&gfb->base); - drm_framebuffer_cleanup(&gfb->base); - return 0; } @@ -158,41 +137,17 @@ static const struct drm_fb_helper_funcs bochs_fb_helper_funcs = { .fb_probe = bochsfb_create, }; +const struct drm_mode_config_funcs bochs_mode_funcs = { + .fb_create = drm_gem_fb_create, +}; + int bochs_fbdev_init(struct bochs_device *bochs) { - int ret; - - drm_fb_helper_prepare(bochs->dev, &bochs->fb.helper, - &bochs_fb_helper_funcs); - - ret = drm_fb_helper_init(bochs->dev, &bochs->fb.helper, 1); - if (ret) - return ret; - - ret = drm_fb_helper_single_add_all_connectors(&bochs->fb.helper); - if (ret) - goto fini; - - drm_helper_disable_unused_functions(bochs->dev); - - ret = drm_fb_helper_initial_config(&bochs->fb.helper, 32); - if (ret) - goto fini; - - return 0; - -fini: - drm_fb_helper_fini(&bochs->fb.helper); - return ret; + return drm_fb_helper_fbdev_setup(bochs->dev, &bochs->fb.helper, + &bochs_fb_helper_funcs, 32, 1); } void bochs_fbdev_fini(struct bochs_device *bochs) { - if (bochs->fb.initialized) - bochs_fbdev_destroy(bochs); - - if (bochs->fb.helper.fbdev) - drm_fb_helper_fini(&bochs->fb.helper); - - bochs->fb.initialized = false; + drm_fb_helper_fbdev_teardown(bochs->dev); } diff --git a/drivers/gpu/drm/bochs/bochs_kms.c b/drivers/gpu/drm/bochs/bochs_kms.c index 233980a78591..c7e575511d2f 100644 --- a/drivers/gpu/drm/bochs/bochs_kms.c +++ b/drivers/gpu/drm/bochs/bochs_kms.c @@ -35,14 +35,12 @@ static int bochs_crtc_mode_set_base(struct drm_crtc *crtc, int x, int y, { struct bochs_device *bochs = container_of(crtc, struct bochs_device, crtc); - struct bochs_framebuffer *bochs_fb; struct bochs_bo *bo; u64 gpu_addr = 0; int ret; if (old_fb) { - bochs_fb = to_bochs_framebuffer(old_fb); - bo = gem_to_bochs_bo(bochs_fb->obj); + bo = gem_to_bochs_bo(old_fb->obj[0]); ret = ttm_bo_reserve(&bo->bo, true, false, NULL); if (ret) { DRM_ERROR("failed to reserve old_fb bo\n"); @@ -55,8 +53,7 @@ static int bochs_crtc_mode_set_base(struct drm_crtc *crtc, int x, int y, if (WARN_ON(crtc->primary->fb == NULL)) return -EINVAL; - bochs_fb = to_bochs_framebuffer(crtc->primary->fb); - bo = gem_to_bochs_bo(bochs_fb->obj); + bo = gem_to_bochs_bo(crtc->primary->fb->obj[0]); ret = ttm_bo_reserve(&bo->bo, true, false, NULL); if (ret) return ret; diff --git a/drivers/gpu/drm/bochs/bochs_mm.c b/drivers/gpu/drm/bochs/bochs_mm.c index 39cd08416773..cdbc3ca3ec51 100644 --- a/drivers/gpu/drm/bochs/bochs_mm.c +++ b/drivers/gpu/drm/bochs/bochs_mm.c @@ -457,77 +457,3 @@ int bochs_dumb_mmap_offset(struct drm_file *file, struct drm_device *dev, drm_gem_object_unreference_unlocked(obj); return 0; } - -/* ---------------------------------------------------------------------- */ - -static void bochs_user_framebuffer_destroy(struct drm_framebuffer *fb) -{ - struct bochs_framebuffer *bochs_fb = to_bochs_framebuffer(fb); - - drm_gem_object_unreference_unlocked(bochs_fb->obj); - drm_framebuffer_cleanup(fb); - kfree(fb); -} - -static const struct drm_framebuffer_funcs bochs_fb_funcs = { - .destroy = bochs_user_framebuffer_destroy, -}; - -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 ret; - - drm_helper_mode_fill_fb_struct(dev, &gfb->base, mode_cmd); - gfb->obj = obj; - ret = drm_framebuffer_init(dev, &gfb->base, &bochs_fb_funcs); - if (ret) { - DRM_ERROR("drm_framebuffer_init failed: %d\n", ret); - return ret; - } - return 0; -} - -static struct drm_framebuffer * -bochs_user_framebuffer_create(struct drm_device *dev, - struct drm_file *filp, - const struct drm_mode_fb_cmd2 *mode_cmd) -{ - struct drm_gem_object *obj; - struct bochs_framebuffer *bochs_fb; - int ret; - - DRM_DEBUG_DRIVER("%dx%d, format %c%c%c%c\n", - mode_cmd->width, mode_cmd->height, - (mode_cmd->pixel_format) & 0xff, - (mode_cmd->pixel_format >> 8) & 0xff, - (mode_cmd->pixel_format >> 16) & 0xff, - (mode_cmd->pixel_format >> 24) & 0xff); - - if (mode_cmd->pixel_format != DRM_FORMAT_XRGB8888) - return ERR_PTR(-ENOENT); - - obj = drm_gem_object_lookup(filp, mode_cmd->handles[0]); - if (obj == NULL) - return ERR_PTR(-ENOENT); - - bochs_fb = kzalloc(sizeof(*bochs_fb), GFP_KERNEL); - if (!bochs_fb) { - drm_gem_object_unreference_unlocked(obj); - return ERR_PTR(-ENOMEM); - } - - ret = bochs_framebuffer_init(dev, bochs_fb, mode_cmd, obj); - if (ret) { - drm_gem_object_unreference_unlocked(obj); - kfree(bochs_fb); - return ERR_PTR(ret); - } - return &bochs_fb->base; -} - -const struct drm_mode_config_funcs bochs_mode_funcs = { - .fb_create = bochs_user_framebuffer_create, -};
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-mstaudt@suse.de Signed-off-by: Peter Wu <peter@lekensteyn.nl> --- 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(-)