From: Daniel Vetter <daniel@ffwll.ch>
To: Emil Velikov <emil.l.velikov@gmail.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>,
Laszlo Ersek <lersek@redhat.com>,
Gerd Hoffmann <kraxel@redhat.com>,
ML dri-devel <dri-devel@lists.freedesktop.org>,
Hans de Goede <hdegoede@redhat.com>
Subject: Re: [PATCH 3/4] drm/virtio: remove drm_dev_set_unique workaround
Date: Mon, 9 Apr 2018 10:26:38 +0200 [thread overview]
Message-ID: <20180409082638.GH31310@phenom.ffwll.local> (raw)
In-Reply-To: <CACvgo515CtUTw_uXDazGxi8D_6DHshiy=RbAA-chGcykBPZzCQ@mail.gmail.com>
On Fri, Apr 06, 2018 at 01:56:15PM +0100, Emil Velikov wrote:
> Hi Laszlo,
>
> On 6 April 2018 at 13:15, Laszlo Ersek <lersek@redhat.com> wrote:
> > On 04/04/18 19:29, Laszlo Ersek wrote:
> >> Hi Emil,
> >>
> >> On 04/03/18 19:13, Emil Velikov wrote:
> >>> On 29 March 2018 at 12:17, Laszlo Ersek <lersek@redhat.com> wrote:
> >>>> On 03/28/18 16:35, Emil Velikov wrote:
> >>>>> On 28 March 2018 at 11:27, Laszlo Ersek <lersek@redhat.com> wrote:
> >>>>>> On 03/28/18 03:24, Emil Velikov wrote:
> >>>>
> >>>>>>> Gents, can someone double-check this please? Just in case.
> >>>>>>
> >>>>>> I guess I should test whether this series regresses the use case
> >>>>>> described in c2cbc38b97; is that correct?
> >>>>>>
> >>>>> Precisely.
> >>>>
> >>>>> [3] https://github.com/evelikov/linux/commits/ioctl-cleanups
> >>>>
> >>>> Unfortunately, this series seems to reintroduce the regression fixed
> >>>> / described earlier in commit c2cbc38b97.
> >>>>
> >>> Thank you very much for testing.
> >>>
> >>> Believe I've tracked it down to a broken commit from 2014 ;-)
> >>> Please try the following branch [1] - it's untested, but I'm 99% sure
> >>> it will work like a charm.
> >>
> >> Alas, it does not work.
> >
> > I've done some more digging. Let me quote the commit message on the
> > proposed patch again:
> >
> >> Ealier commit a325725633c26aa66ab940f762a6b0778edf76c0 did not attribute
> >> that virtio can be either PCI or a platform device and removed the
> >> .set_busid hook. Whereas only the "platform" instance should have been
> >> removed.
> >>
> >> Since then, two things have happened:
> >> - the driver manually calls drm_dev_set_unique, addressing the Xserver
> >> regression - see commit 9785b4321b0bd701f8d21d3d3c676a7739a5cf22
> >> - core itself calls drm_pci_set_busid, on drm_set_busid IOCTL setting
> >> the busid, so we don't need to fallback to dev->unique - see commit
> >> 5c484cee7ef9c4fd29fa0ba09640d55960977145
> >>
> >> With that in place we can remove the local workaround.
> >
> > This write-up of events is not precise enough. Instead, I think the
> > timeline is as follows:
> >
> > (1) Commit a325725633c2 ("drm: Lobotomize set_busid nonsense for !pci
> > drivers", 2016-06-21) introduced the regression. By removing
> > drm_virtio_set_busid(), commit a325725633c2 changed the behavior of
> > the following function:
> >
> >> static int drm_set_busid(struct drm_device *dev, struct drm_file *file_priv)
> >> {
> >> struct drm_master *master = file_priv->master;
> >> int ret;
> >>
> >> if (master->unique != NULL)
> >> drm_unset_busid(dev, master);
> >>
> >> if (dev->driver->set_busid) {
> >> ret = dev->driver->set_busid(dev, master);
> >> if (ret) {
> >> drm_unset_busid(dev, master);
> >> return ret;
> >> }
> >> } else {
> >> WARN_ON(!dev->unique);
> >> master->unique = kstrdup(dev->unique, GFP_KERNEL);
> >> if (master->unique)
> >> master->unique_len = strlen(dev->unique);
> >> }
> >>
> >> return 0;
> >> }
> >
> > When a325725633c2 removed drm_virtio_set_busid(), drm_set_busid()
> > started taking the second branch, which wasn't doing the right thing
> > for virtio-vga at the time.
> >
> > (2) There were two ways to fix the regression: either (a) return
> > drm_set_busid() to taking the first branch, or (b) tweak the
> > virtio-vga driver so that the second branch in drm_set_busid() start
> > to behave right.
> >
> > My commit c2cbc38b9715 ("drm: virtio: reinstate
> > drm_virtio_set_busid()", 2016-10-04) implemented (a), by reverting a
> > few chunks of a325725633c2.
> >
> > (3) Gerd thought that approach (b) was superior (and I totally defer to
> > him on this, now that I'm learning of his patches in the first place
> > :) ). Namely, in commit 9785b4321b0b ("drm/virtio: fix busid
> > regression", 2016-11-15), he implemented approach (b), and right
> > after, in commit 1775db074a32 ("Revert "drm: virtio: reinstate
> > drm_virtio_set_busid()"", 2016-11-15), he undid my commit
> > c2cbc38b9715.
> >
> > In other words, Gerd replaced approach (a) with approach (b).
> >
> > (4) Subsequently, commit 5c484cee7ef9 ("drm: Remove
> > drm_driver->set_busid hook", 2017-06-20), changed drm_set_busid()
> > to the following:
> >
> >> static int drm_set_busid(struct drm_device *dev, struct drm_file *file_priv)
> >> {
> >> struct drm_master *master = file_priv->master;
> >> int ret;
> >>
> >> if (master->unique != NULL)
> >> drm_unset_busid(dev, master);
> >>
> >> if (dev_is_pci(dev->dev)) {
> >> ret = drm_pci_set_busid(dev, master);
> >> if (ret) {
> >> drm_unset_busid(dev, master);
> >> return ret;
> >> }
> >> } else {
> >> WARN_ON(!dev->unique);
> >> master->unique = kstrdup(dev->unique, GFP_KERNEL);
> >> if (master->unique)
> >> master->unique_len = strlen(dev->unique);
> >> }
> >>
> >> return 0;
> >> }
> >
> > Perhaps surprisingly, this change did not affect (or "help")
> > virtio-vga at all, despite the fact that drm_virtio_set_busid() also
> > used to call drm_pci_set_busid().
> >
> > The reason for commit 5c484cee7ef9 not affecting virtio-vga is that
> > the first branch would not be taken just the same, because
> > dev_is_pci() returns false for virtio-vga. (So, the difference with
> > drm_virtio_set_busid() is that drm_virtio_set_busid() would call
> > drm_pci_set_busid() without checking dev_is_pci() first.)
> >
> > Here's the definition of the dev_is_pci() macro, from
> > "include/linux/pci.h":
> >
> >> #define dev_is_pci(d) ((d)->bus == &pci_bus_type)
> >
> > However, the bus type for virtio-vga is "virtio_bus", not
> > "pci_bus_type". Namely, virtio_pci_probe()
> > [drivers/virtio/virtio_pci_common.c] calls register_virtio_device()
> > [drivers/virtio/virtio.c], and there we have
> >
> >> int register_virtio_device(struct virtio_device *dev)
> >> {
> >> int err;
> >>
> >> dev->dev.bus = &virtio_bus;
> >
> > This means that post-5c484cee7ef9, we remain reliant on the second
> > branch in drm_set_busid(), and therefore Gerd's commit 9785b4321b0b,
> > from point (3), should not be backed out.
> >
> > What I could see as justified is a loud comment in drm_virtio_init(),
> > just above the call to drm_dev_set_unique(), explaining why it is
> > necessary to set "unique" manually. The reason is that virtio-vga
> > technically has "virtio_bus", not "pci_bus_type", for bus type, and so
> > the generic PCI BusID-setting will not cover it.
> >
> I've reached to roughly the same conclusion yesterday. Or to put it in
> slightly differently:
>
> Unlike the other virtual GPU drivers (vmxgfx, qxl, bosh...) virtio
> abstracts the underlying bus type by using struct virtio_device.
> Hence the dev_is_pci() check will fail and the unique returned will be
> the virtio_device' "virtio0", while the "pci:..." one is required.
>
> Apart from a beefy comment I've also considered:
> - Extending the dev_is_pci() case [in drm_set_busid] to consider virtio.
> It seems like a bigger hack that what we have currently.
> - point drm_device::dev to the parent of the virtio_device
> The biggest hack imaginable, if even possible.
What would/could break if we do this? Why is this a hack?
-Daniel
> Above said, consider patches 3+4 dropped and I'll follow-up with a
> patch adding inline documentation about this.
>
> Thank you very much for the help.
> Emil
> P.S. On the plus side, I am working on a X/modesetting series that
> removes all the mess which requires this workaround ;-)
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
next prev parent reply other threads:[~2018-04-09 8:26 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-03-28 1:24 [PATCH 1/4] drm/vgem: Fix vgem_init to get drm device avaliable Emil Velikov
2018-03-28 1:24 ` [PATCH 2/4] drm: BUG_ON if passing NULL parent to drm_dev_init Emil Velikov
2018-03-28 7:17 ` Daniel Vetter
2018-03-28 1:24 ` [PATCH 3/4] drm/virtio: remove drm_dev_set_unique workaround Emil Velikov
2018-03-28 7:18 ` Daniel Vetter
2018-03-28 10:27 ` Laszlo Ersek
2018-03-28 14:35 ` Emil Velikov
2018-03-29 11:17 ` Laszlo Ersek
2018-04-03 17:13 ` Emil Velikov
2018-04-04 17:29 ` Laszlo Ersek
2018-04-06 12:15 ` Laszlo Ersek
2018-04-06 12:56 ` Emil Velikov
2018-04-09 8:26 ` Daniel Vetter [this message]
2018-04-09 10:24 ` Emil Velikov
2018-04-09 11:25 ` Emil Velikov
2018-04-13 15:45 ` Daniel Vetter
2018-04-06 13:06 ` Gerd Hoffmann
2018-03-28 1:24 ` [PATCH 4/4] drm: drm_dev_set_unique private, again Emil Velikov
2018-03-28 7:18 ` Daniel Vetter
2018-03-28 7:16 ` [PATCH 1/4] drm/vgem: Fix vgem_init to get drm device avaliable Daniel Vetter
2018-03-28 14:45 ` Emil Velikov
2018-03-28 14:49 ` Chris Wilson
2018-03-28 15:11 ` Emil Velikov
2018-03-29 7:17 ` Daniel Vetter
2018-04-04 10:46 ` Emil Velikov
2018-04-04 12:07 ` Daniel Vetter
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20180409082638.GH31310@phenom.ffwll.local \
--to=daniel@ffwll.ch \
--cc=daniel.vetter@ffwll.ch \
--cc=dri-devel@lists.freedesktop.org \
--cc=emil.l.velikov@gmail.com \
--cc=hdegoede@redhat.com \
--cc=kraxel@redhat.com \
--cc=lersek@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).