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

[Thomas Zimmermann <tzimmermann@suse.de>]

[-- Attachment #1.1: Type: text/plain, Size: 3491 bytes --]

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@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.

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 #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]