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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi

Am 20.10.22 um 20:37 schrieb Zack Rusin:
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@xxxxxxxxxx>
[...]
@@ -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.

Calling drm_fbdev_generic_setup() with 32 is the 'official' workaround for the problem. If you run into the bug, it's ok to call the function with a bpp value. The whole usage of formats, depth and bpp in fbdev helpers is confusing and needs work.

Best regards
Thomas



+
   	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

--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

Attachment: OpenPGP_signature
Description: OpenPGP digital signature


[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux