On Fri, Jul 26, 2024 at 11:07:19PM +1000, Ben Skeggs wrote:
> On 27/7/24 01:41, Danilo Krummrich wrote:
>
> > On Fri, Jul 26, 2024 at 02:27:53PM +1000, Ben Skeggs wrote:
> > > > > > > +
> > > > > > > +static struct nouveau_drm *
> > > > > > > +nouveau_drm_device_new(const struct drm_driver *drm_driver, struct device *parent,
> > > > > > > + struct nvkm_device *device)
> > > > > > > +{
> > > > > > > + struct nouveau_drm *drm;
> > > > > > > + int ret;
> > > > > > > +
> > > > > > > + drm = kzalloc(sizeof(*drm), GFP_KERNEL);
> > > > > > > + if (!drm)
> > > > > > > + return ERR_PTR(-ENOMEM);
> > > > > > > +
> > > > > > > + drm->dev = drm_dev_alloc(drm_driver, parent);
> > > > > > Since you're reworking this anyways, can we switch to devm_drm_dev_alloc()?
> > > > > >
> > > > > > This also gets us rid of nouveau_drm_device_del().
> > > > > No, we can't. I originally had this change as a cleanup patch in the series
> > > > > I posted implementing aux bus support. However it turns out that in order
> > > > > to avoid breaking udev etc, we can't use the aux device as parent of the drm
> > > > Can you please expand a bit on what was breaking?
> > > Sorry, I meant to say PRIME, not udev. The device selection logic ties the
> > > DRM device back to its sysfs node, and doesn't understand the auxiliary
> > > bus. So, if nouveau were to use its auxiliary device as parent of the DRM
> > > device, PRIME breaks.
> > The Vulkan device selector stuff looks like it should mostly work.
> >
> > However, I guess you refer to the loader stuff in Mesa that uses
> > drmGetDevices2() from libdrm? This stuff indeed whitelists busses it accepts to
> > report DRM device from:
> >
> > { "/pci", DRM_BUS_PCI },
> > { "/usb", DRM_BUS_USB },
> > { "/platform", DRM_BUS_PLATFORM },
> > { "/spi", DRM_BUS_PLATFORM },
> > { "/host1x", DRM_BUS_HOST1X },
> > { "/virtio", DRM_BUS_VIRTIO },
> >
> > Not a big deal to just add it for a new driver, but obviously we can't just do
> > this for an existing one.
> >
> > > Fortunately it didn't turn out to be necessary, and we
> > > can happily probe() against the auxiliary device and still use the PCI
> > > device as the DRM device's parent.
> > At a first glance, I guess this should work. But, before we introduce
> > workarounds like this one and add even more complexity, I wonder what's the
> > benefit of doing this for Nouveau in the first place? I think we agreed to this
> > split for Nova, for the reasons discussed in [1].
>
> Because, as I already mentioned in the cover letter for series I posted
For some reason your auxbus series never hit my inbox - fetched it from lore
now.
> implementing the auxiliary bus support, this brings immediate benefits to
> users, such as eliminating the long pauses on systems using prime whilst the
> entire GPU is woken up for some PCI query by userspace.
Sounds good, what is the difference we're talking about?
>
> It also (finally) integrates Tegra in a reasonably clean fashion, and would
> allow the DRM-level suspend/resume code to be shared there too if someone
> were to implement the platform-level code for it. That was not possible
> before.
I agree it's an advantage, but it's probably not too bad as it currently is
either.
>
> >
> > [1] https://lore.kernel.org/dri-devel/20240613170211.88779-1-bskeggs@xxxxxxxxxx/
> >
> > > > > device and instead have to continue passing the pci/platform device as we do
> > > > > now.
> > > > >
> > > > > Using devm_drm_dev_alloc() with the pci device as parent would tie the
> > > > > lifetime of the drm device to the pci device, which is owned by nvkm (after
> > > > How does this tie the lifetime of the drm device to the pci device? It's the
> > > > other way around, the drm device takes a reference of its parent (i.e. the pci
> > > > device).
> > > >
> > > > I don't think there's anything wrong with that.
> > > My understanding is that devres will cleanup allocations when the driver
> > > detaches from the device.
> > Right, I think I took that too literally.
> >
> > The lifetime of the DRM device (or more precisely one of its references) is
> > bound to the binding between the parent device and its corresponding driver.
> >
> > But the lifetime of the parent device itself is bound to the DRM device.
> >
> > So, yes this doesn't work, and proves the point that initializing the DRM device
> > with the parent's parent is just a workaround.
>
> You're greatly overstating the "complexity" that's added here. It's a minor
> inconvenience that doesn't require much code at all to implement, and is
> essentially irrelevant outside of module load/unload.
When I was talking about complexity I was referring to the changes required to
integrate the auxbus stuff.
Having to call drm_dev_put() from .remove() by hand obviously isn't something
I'm overly concerned about in terms of complexity...
>
> I agree it's not ideal, and userspace should gain auxiliary bus support
> before a new driver implements a similar architecture, but it's really not
> that big a deal.
>
> >
> > > With the auxdev changes, it's *NVKM* that's
> > > attached to the PCI device, not the DRM driver (which is attached to an
> > > auxiliary device instead).
> > >
> > > This means that the devm_drm_dev_init_release() won't be called when the DRM
> > > driver detaches from its auxiliary device as it should, but when NVKM
> > > detaches from the PCI device, which isn't the most obvious and could lead to
> > > confusion.
> > >
> > > It also entirely blows up in the split module case as nouveau.ko is unloaded
> > > already by the time NVKM detaches and drm_dev_put() gets called.
> > >
> > > > > the auxdev series). We could look at changing devm_drm_dev_alloc() of
> > > > > course, but I think that's best left until later.
> > > > I don't think that this is necessary.
>
