Re: [PATCH] qxl: refactor to use drm_fb_helper_fbdev_setup

Re: [PATCH] qxl: refactor to use drm_fb_helper_fbdev_setup

[kbuild test robot]

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-rc3 next-20180910]
[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/qxl-refactor-to-use-drm_fb_helper_fbdev_setup/20180911-071413
reproduce:
        # apt-get install sparse
        make ARCH=x86_64 allmodconfig
        make C=1 CF=-D__CHECK_ENDIAN__


sparse warnings: (new ones prefixed by >>)

   drivers/gpu/drm/qxl/qxl_drv.c:144:9: sparse: undefined identifier 'qxl_fbdev_set_suspend'
   drivers/gpu/drm/qxl/qxl_drv.c:181:9: sparse: undefined identifier 'qxl_fbdev_set_suspend'
>> drivers/gpu/drm/qxl/qxl_drv.c:144:30: sparse: call with no type!
   drivers/gpu/drm/qxl/qxl_drv.c:181:30: sparse: call with no type!
   drivers/gpu/drm/qxl/qxl_drv.c: In function 'qxl_drm_freeze':
   drivers/gpu/drm/qxl/qxl_drv.c:144:2: error: implicit declaration of function 'qxl_fbdev_set_suspend'; did you mean 'fb_set_suspend'? [-Werror=implicit-function-declaration]
     qxl_fbdev_set_suspend(qdev, 1);
     ^~~~~~~~~~~~~~~~~~~~~
     fb_set_suspend
   cc1: some warnings being treated as errors
--
>> drivers/gpu/drm/qxl/qxl_fb.c:166:21: sparse: incorrect type in assignment (different address spaces) @@    expected char const *data @@    got char [noderchar const *data @@
   drivers/gpu/drm/qxl/qxl_fb.c:166:21:    expected char const *data
   drivers/gpu/drm/qxl/qxl_fb.c:166:21:    got char [noderef] <asn:2>*
   include/linux/overflow.h:251:13: sparse: undefined identifier '__builtin_mul_overflow'
   include/linux/overflow.h:251:13: sparse: incorrect type in conditional
   include/linux/overflow.h:251:13:    got void
   include/linux/overflow.h:251:13: sparse: call with no type!

vim +166 drivers/gpu/drm/qxl/qxl_fb.c

   130	
   131	/*
   132	 * FIXME
   133	 * It should not be necessary to have a special dirty() callback for fbdev.
   134	 */
   135	static int qxlfb_framebuffer_dirty(struct drm_framebuffer *fb,
   136					   struct drm_file *file_priv,
   137					   unsigned flags, unsigned color,
   138					   struct drm_clip_rect *clips,
   139					   unsigned num_clips)
   140	{
   141		struct qxl_device *qdev = fb->dev->dev_private;
   142		struct fb_info *info = qdev->fb_helper.fbdev;
   143		struct qxl_fb_image qxl_fb_image;
   144		struct fb_image *image = &qxl_fb_image.fb_image;
   145	
   146		/* TODO: hard coding 32 bpp */
   147		int stride = fb->pitches[0];
   148	
   149		/*
   150		 * we are using a shadow draw buffer, at qdev->surface0_shadow
   151		 */
   152		image->dx = clips->x1;
   153		image->dy = clips->y1;
   154		image->width = clips->x2 - clips->x1;
   155		image->height = clips->y2 - clips->y1;
   156		image->fg_color = 0xffffffff; /* unused, just to avoid uninitialized
   157						 warnings */
   158		image->bg_color = 0;
   159		image->depth = 32;	     /* TODO: take from somewhere? */
   160		image->cmap.start = 0;
   161		image->cmap.len = 0;
   162		image->cmap.red = NULL;
   163		image->cmap.green = NULL;
   164		image->cmap.blue = NULL;
   165		image->cmap.transp = NULL;
 > 166		image->data = info->screen_base + (clips->x1 * 4) + (stride * clips->y1);
   167	
   168		qxl_fb_image_init(&qxl_fb_image, qdev, info, NULL);
   169		qxl_draw_opaque_fb(&qxl_fb_image, stride);
   170	
   171		return 0;
   172	}
   173	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

Re: [PATCH] qxl: refactor to use drm_fb_helper_fbdev_setup

[Gerd Hoffmann]

On Mon, Sep 10, 2018 at 03:21:56PM +0200, Peter Wu wrote:
> Lots of code can be removed by relying on fb-helper:
> - "struct drm_framebuffer" moves to fb_helper.fb.
> - "struct drm_gem_object" moves to fb_helper.obj[0].
> - "struct qxl_device" can be inferred as drm_fb_helper is embedded.
> - qxl_user_framebuffer_create -> drm_gem_fb_create.
> - qxl_user_framebuffer_destroy -> drm_gem_fb_destroy.
> - qxl_fbdev_destroy -> drm_fb_helper_fbdev_teardown + vfree(shadow).
> 
> Remove unused code:
> - qxl_fbdev_qobj_is_fb, qxl_fbdev_set_suspend.
> - Unused fields of qxl_fbdev: delayed_ops, delayed_ops_lock, size.
> 
> Misc notes:
> - The dirty callback is preserved as it is necessary to trigger update
>   commands in the hw (the screen stays black otherwise).
> - No idea when .create_handle in drm_framebuffer_funcs is used, but use
>   the same drm_gem_fb_create_handle to match drm_gem_fb_funcs.
> - I don't know why qxl_fb_find_or_create_single used to check for an
>   existing framebuffer and removed that check to match other drivers.
> - Use of drm_fb_helper_fbdev_teardown also requires "info->fbdefio" to
>   be dynamically allocated. Replace the existing defio config by
>   drm_fb_helper_defio_init to accomodate this.
> 
> Testing results: startx with fbdev, modesetting and qxl all seems to
> work. Tested also with CONFIG_DRM_FBDEV_EMULATION=n, fbdev obviously
> fails but others are fine. QEMU -spice and QEMU -spice with vdagent and
> multiple (resized) displays (via remote-viewer) also works.
> unbind vtconsole and rmmod has *not* regressed (i.e. it still trips on a
> use-after-free in qxl_check_idle via qxl_ttm_fini).
> 
> Ideally setup/teardown is replaced by drm_fbdev_generic_setup as that
> would result in further code reduction, improve error handling (like not
> leaking shadow memory), but unfortunately QXL has no implementation for
> qxl_gem_prime_vmap.

Pushed to drm-misc-next.

thanks,
  Gerd