[PATCH] drm/vboxvideo: Add missing remove_conflicting_pci_framebuffers call

[Hans de Goede]

Hi,

On 3/26/20 2:39 PM, Daniel Vetter wrote:
> On Thu, Mar 26, 2020 at 2:18 PM Hans de Goede <hdegoede at redhat.com> wrote:
>>
>> Hi,
>>
>> On 3/26/20 12:29 PM, Daniel Vetter wrote:
>>> On Wed, Mar 25, 2020 at 03:43:10PM +0100, Hans de Goede wrote:
>>>> The vboxvideo driver is missing a call to remove conflicting framebuffers.
>>>>
>>>> Surprisingly, when using legacy BIOS booting this does not really cause
>>>> any issues. But when using UEFI to boot the VM then plymouth will draw
>>>> on both the efifb /dev/fb0 and /dev/drm/card0 (which has registered
>>>> /dev/fb1 as fbdev emulation).
>>>>
>>>> VirtualBox will actual display the output of both devices (I guess it is
>>>> showing whatever was drawn last), this causes weird artifacts because of
>>>> pitch issues in the efifb when the VM window is not sized at 1024x768
>>>> (the window will resize to its last size once the vboxvideo driver loads,
>>>> changing the pitch).
>>>>
>>>> Adding the missing drm_fb_helper_remove_conflicting_pci_framebuffers()
>>>> call fixes this.
>>>>
>>>> Cc: stable at vger.kernel.org
>>>> Fixes: 2695eae1f6d3 ("drm/vboxvideo: Switch to generic fbdev emulation")
>>>> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
>>>> ---
>>>>    drivers/gpu/drm/vboxvideo/vbox_drv.c | 4 ++++
>>>>    1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/vboxvideo/vbox_drv.c b/drivers/gpu/drm/vboxvideo/vbox_drv.c
>>>> index 8512d970a09f..261255085918 100644
>>>> --- a/drivers/gpu/drm/vboxvideo/vbox_drv.c
>>>> +++ b/drivers/gpu/drm/vboxvideo/vbox_drv.c
>>>> @@ -76,6 +76,10 @@ static int vbox_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>>>>       if (ret)
>>>>               goto err_mode_fini;
>>>>
>>>> +    ret = drm_fb_helper_remove_conflicting_pci_framebuffers(pdev, "vboxvideodrmfb");
>>>> +    if (ret)
>>>> +            goto err_irq_fini;
>>>
>>> To avoid transient issues this should be done as early as possible,
>>> definitely before the drm driver starts to touch the "hw".>
>>
>> Ok will fix and then push this to drm-misc-next-fixes, thank you.
>>
>>> With that
>>>
>>> Reviewed-by: Daniel Vetter <daniel.vetter at ffwll.ch>
>>>
>>> I do wonder though why the automatic removal of conflicting framebuffers
>>> doesn't work, fbdev should already do that from register_framebuffer(),
>>> which is called somewhere in drm_fbdev_generic_setup (after a few layers).
>>>
>>> Did you check why the two framebuffers don't conflict, and why the uefi
>>> one doesn't get thrown out?
>>
>> I did not check, I was not aware that this check was already happening
>> in register_framebuffer(). So I just checked and the reason why this
>> is not working is because the fbdev emulation done by drm_fbdev_generic_setup
>> does not fill in fb_info.apertures->ranges[0] .
>>
>> So fb_info.apertures->ranges[0].base is left as 0 which does not match
>> with the registered efifb's aperture.
>>
>> We could try to fix this, but that is not entirely trivial, we would
>> need to pass the pci_dev pointer down into drm_fb_helper_alloc_fbi()
>> then and then like remove_conflicting_pci_framebuffers() does add
>> apertures for all IORESOURCE_MEM PCI bars, but that would conflict
>> with drivers which do rely on drm_fb_helper_alloc_fbi() currently
>> allocating one empty aperture and then actually fill that itself...
> 
> You don't need the pci device, because resources are attached to the
> struct device directly. So you could just go through all
> IORESOURCE_MEM ranges, and add them. And the struct device we always
> have through drm_device->dev. This idea just crossed my mind since you
> brought up IORESOURCE_MEM, might be good to try that out instead of
> having to litter all drivers with explicit removal calls. The explicit
> removal is really only for races (vga hw tends to blow up if 2 drivers
> touch it, stuff like that), or when there's resources conflicts. Can
> you try to look into that?

Interesting idea, but I'm afraid that my plate is quite full with a lot of
more urgent things atm, so I really do not have time for this, sorry.

Regards,

Hans