[PATCH] drm/vboxvideo: Add missing remove_conflicting_pci_framebuffers call

[Daniel Vetter]

On Thu, Mar 26, 2020 at 3:40 PM Hans de Goede <hdegoede at redhat.com> wrote:
>
> 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.

Hm can you capture this idea in a todo then instead? I don't really
like the idea of everyone having to add bogus
remove_conflicting_framebuffer calls just because the generic fbdev
helper falls short of expectations. Kinda not happy to land the
vboxvideo patch since it's just a hack to work around a helper bug.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch