Re: [PATCH v2 13/16] drm/vmwgfx: Port the framebuffer code to drm fb helpers

[Zack Rusin <zackr@vmware.com>]

On Thu, 2022-10-20 at 11:06 +0200, Thomas Zimmermann wrote:
> Hi Zack
> 
> Am 20.10.22 um 05:41 schrieb Zack Rusin:
> > From: Zack Rusin <zackr@vmware.com>
> [...]
> > @@ -1670,6 +1640,10 @@ static int vmw_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> >   	if (ret)
> >   		goto out_unload;
> >   
> > +	vmw_fifo_resource_inc(vmw);
> > +	vmw_svga_enable(vmw);
> > +	drm_fbdev_generic_setup(&vmw->drm,  vmw->assume_16bpp ? 16 : 32);
> 
> The preferred way of setting the color depth is with struct 
> drm_mode_config.preferred_depth. [1] Note that it is the color depth; 
> not the pixel size. In your case:
> 
> if (vmw->assume_16bpp)
> 	dev->mode_config.preferred_depth = 16;
> else
> 	dev->mode_config.preferred_depth = 24;
> 
> It's also a hint to userspace. [2]
> 
> The prefer_bpp parameter of drm_fbdev_generic_setup() should be 0. It is 
> a fallback to force a certain pixel size, as preferred_depth fails.
> 

Ah, that makes sense. I'll fix that, btw, the dev->mode_config.preferred_depth = 24
part, we should probably have some check in drm_fbdev_generic_setup that it is not
24. 

That's because 24 will invoke the buggy code in drm fbdev helpers that confuses
depth and bpp and will endup invoking dumb create with args->bpp == 24 and that's
specifically disallowed for dumb_create. IGT's has explicit
(dumb_buffer::invalid_bpp) test that checks whether dumb_create with bpp == 24
fails. An earlier commit in this series actually fixes that specific test in vmwgfx.
A lot of drivers will work because even though they set preferred_depth to 24, they
call the  dev->mode_config.preferred_depth = 24 call drm_fbdev_generic_setup with 32
but it's definitely confusing.

> 
> > +
> >   	vmw_debugfs_gem_init(vmw);
> >   	vmw_debugfs_resource_managers_init(vmw);
> >   
> [...]
> > -
> > -/**
> > - * vmw_fb_dirty_flush - flush dirty regions to the kms framebuffer
> > - *
> > - * @work: The struct work_struct associated with this task.
> > - *
> > - * This function flushes the dirty regions of the vmalloc framebuffer to the
> > - * kms framebuffer, and if the kms framebuffer is visible, also updated the
> > - * corresponding displays. Note that this function runs even if the kms
> > - * framebuffer is not bound to a crtc and thus not visible, but it's turned
> > - * off during hibernation using the par->dirty.active bool.
> > - */
> > -static void vmw_fb_dirty_flush(struct work_struct *work)
> 
> This is the flush function for vmwgfx' deferred I/O. If you want to 
> implement deferred I/O with the generic fbdev emulation, you have to set 
> struct drm_mode_config.prefer_shadow_fbdev to true. [3]

Yea, we don't need it anymore. But it probably is a good idea to preserve the old
behaviour for systems that didn't have guest backed memory support. I'll adjust
that. Thanks for taking a look at this!

z