Hi,
On 4/15/20 8:29 PM, Daniel Vetter wrote:
On Wed, Apr 15, 2020 at 8:19 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
Hi,
On 4/15/20 7:54 PM, Daniel Vetter wrote:
On Wed, Apr 15, 2020 at 03:02:53PM +0200, Hans de Goede wrote:
Hi,
On 4/15/20 2:01 PM, Daniel Vetter wrote:
On Wed, Apr 15, 2020 at 01:39:23PM +0200, Hans de Goede wrote:
Hi,
On 4/15/20 12:22 PM, Daniel Vetter wrote:
On Wed, Apr 15, 2020 at 12:11 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
Hi,
On 4/15/20 11:52 AM, Daniel Vetter wrote:
<snip>
iv. What every SoC subsystem does:
- lcdshadow drivers register drivers
- drm drivers look them up
- if stuff isn't there yet, we delay loading with EPROBE_DEFER until
the entire thing is assembled.
That's what we're doing already for other standardized components like
drm_bridge or drm_panel, and I think that's also the right approach
for backlight and anything else like that. Hand-rolling our own
EPROBE_DEFER handling, or some other duct-tape monsters imo just leads
to real pain. Also, with EPROBE_DEFER we have one standard way of
building a driver from component, which spans subsystems and is also
the underlying magic that makes stuff like component.c work.
On the SoCs we have devicetree telling us what components there
are, so we can wait for them to show up. The only way to figure out
if the lcdshadow thing is there on a ThinkPad is asking thinkpad_acpi,
or duplicating a lot of code from thinkpad_acpi. Edit:
also see below for a possible solution.
Yup it sucks. I think all we can do is have a small acpi match
function (which yes will duplicate some of the thinkpad_acpi driver
logic) to re-create that information and give us a "should we have a
lcdshadow driver for this $pci_device" answer.
Ok, so questions about this solution:
1. Where should that match-function live ?
2. An acpi_thinkpad derived match-function will only be able to
answer if there is an lcdshadow device/driver for the internal
panel. It will not be able to tie this info to a certain PCI
device. My plan is to pass NULL as dev_name when registering
the lcdshadow-device and have lcdshadow_get(dev, <connector-name>)
skip device-name matching (consider everything a match) for
lcdshadow-devices registered with NULL as dev_name.
So I guess in this case the mini match function should just
ignore the passed in device?
Yeah I think we can't really avoid that. I also expect that we'll need
ACPI and dt versions of this, and driver needs to know which one to call.
Since at least in a dt world the driver knows exactly for which dt node it
needs a lcdshadow driver for (with the phandle stuff), so we can be a lot
more strict.
For the acpi version I'm not even sure we can do more than provide the
struct device * pointer of the gpu. I think if we ever get more than 1
lcdshadow driver on acpi systems we can add more stuff later on, for now
I'd just leave that out.
So maybe
acpi_lcdshadow_get(struct device *dev);
of_lcdshadow_get(struct device_node *np);
And with maybe a future plan to add some kind of enum or whatever to
acpi_lcdshadow_get(). Both would return either the lcdshadow pointer, or
an PTR_ERR() so that we could encode EPROBE_DEFER vs ENOENT.
Ok, note I plan to only implement the acpi version for now, I do
expect some non ACPI/x86 devices to show up with his feature
eventually but I believe it is best to implement this once
those actually show up. Esp. since this will also involve adding
some devicetree bindings for this.
ofc, just wanted to lay out the entire thing. The DT version needs some
good bikeshed on the dt schema first anyway (so that the helper can decode
that directly).
We might also want a low-level lcdshadow_get() which only returns ENOENT
when the driver isn't there, and which leaves "do we really need one?" to
higher levels to answer.
Right, so my latest idea on that is indeed a high-level lcdshadow_get()
which takes a struct device * and a connector-name and which never
returns EPROBE_DEFER.
As for leaving things to the higher levels to answer, as explained
in my other follow-up email I think that we should probably add a
lcdshadow_probe_defer() helper for this and call that early on
in the PCI-driver probe functions for the 3 major x86 GPU drivers.
Does that sound ok to you?
Uh ... not pretty. There's still a lifetime problem that strictly speaking
there's nothing stopping the other driver from getting unloaded between
your _probe_defer and the subsequent _get. I think fixing this properly
(and screaming a bit at the error code, oh well) is better.
I would really like to separate the discussion and the work
on getting the 3 major x86 GPU drivers ready to deal with EPROBE_DEFER
from the lcdshadow discussion and work. I expect getting these
3 drivers ready for EPROBE_DEFER is going to be a major undertaking
and I would like avoid introducing this significant scope creep
to the lcdshadow discussion, because it simply is a too big undertaking
to undertake without us getting a significant amount of manpower
specifically for this from somewhere.
Note I do agree with you that getting these 3 drivers ready
for EPROBE_DEFER handling is a worthwhile undertaking, but
it simply will take too much extra time and as such IMHO it
really is out of scope for the lcdshadow stuff.
I expect the amount of work (esp. also dealing with testing
and regressions) for the EPROBE_DEFER project by itself
to be a lot *more* work then the actual lcdshadow project.
So going with the assumption/decision that adding proper
EPROBE_DEFER handling to these 3 drivers is out of scope,
I believe that adding a lcdshadow_probe_defer() helper is
an ok solution/workaround for now.
As for your case of the other driver getting unloaded in between
the check and use of it, that can only happen by explicit user
action and in that case the worst thing what will happen
is the "privacy-screen" property missing from the connector,
which in that case is more or less exactly what the user
asked for.
For i915 we've had patches, for mei-hdcp integration. Until it became
clear that the mei subsystem is bonkers, and handles suspend/resume by
unloading! drivers.
Which forced i915 to unload and broke the world :-/
So at least for i915 the work should be done already, and just amount
to updating the patches Ram already had. No ideas about the other 2.
Ok.
What I don't really want is an interface with races. So if fixing the
other drivers is too much work, what we could do is roughly:
- in the top-level probe function to an acpi_lcdshadow_get. This might
fail with EPROBE_DEFER.
- this gives us a dangling reference, but we can drop that one when we
exit the top-level probe function (both on success and on error cases)
- the 2nd acpi_lcdshadow_get call deep down should always succeed at
that point (since the top level holds a reference), so you could wrap
that in a WARNING(IS_ERR_PTR()). Ok that's a lie, if we concurrently
unload then the 2nd call still fails (the reference can never prevent
a hotunbind in the linux kernel device model), so this is exactly as
buggy as your version, so still needs a comment about that.
Adding a acpi_lcdshadow_probe_defer() function otoh just gives people
the impression that that's actually a correct way of doing this.
Then it's up to the driver maintainers whether they're ok with the
above hack of a fake reference, or not. As I said, I think for i915 it
should be fairly ok to just roll it out, but maybe the patches don't
apply at all anymore.
Ok trying to taking a ref early on and handling EPROBE_DEFER
at that first attempt to take a ref works for me. So lets go with
that. I will try to whip up a v1 patch-set for this, ETA aprox.
1-2 weeks I guess.
btw to make everything work you also need to set up a device_link
between the lcdshadow device (and it's driver, that's the more
important thing) and the gpu device. That device link will make sure
that
- suspend/resume is done in the right order
- driver load/unload is done in the right order, i.e. unloading of the
lcdshadow driver will automatically force an unbind of the gpu driver
first.
That is an interesting idea, but that does assume that their
is an actual struct device which the code handling the lcdshadow
binds to, which in case of thinkpad_acpi is not really the case.
Anyways passing in a parent device when registering a lcdshadow_dev
seems like a good idea and we can do the device_link thing if the parent
is non NULL.
Regards,
Hans
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
