Re: [RFC PATCH 1/6] drm/dp: Helpers to make it easier for drivers to use DP AUX bus properly
Hi,
On Mon, Apr 18, 2022 at 4:10 PM Doug Anderson wrote:
>
> > > 5. In general I've been asserting that it should be up to the panel to
> > > power things on and drive all AUX transactions. ...but clearly my
> > > model isn't reality. We certainly do AUX transactions from the DP
> > > driver because the DP driver needs to know things about the connected
> > > device, like the number of lanes it has, the version of eDP it
> > > supports, and the available bit rates to name a few. Those things all
> > > work today by relying on the fact that pre-enable powers the panel on.
> > > It's pretty easy to say that reading the EDID (and I guess AUX
> > > backlight) is the odd one out. So right now I guess my model is:
> > >
> > > 5a) If the panel code wants to access the AUX bus it can do so by
> > > powering itself on and then just doing an AUX transaction and assuming
> > > that the provider of the AUX bus can power itself on as needed.
> > >
> > > 5b) If the DP code wants to access the AUX bus it should make sure
> > > that the next bridge's pre_enable() has been called. It can then
> > > assume that the device is powered on until the next bridge's
> > > post_disable() has been called.
> > >
> > > So I guess tl;dr: I'm not really a huge fan of the DP driver powering
> > > the panel on by doing a pm_runtime_get() on it. I'd prefer to keep
> > > with the interface that we have to pre_enable() the panel to turn it
> > > on.
> >
> > Again, thank for the explanation. Your concerns make more sense now.
> > As much as I hate writing docs, maybe we should put at least basic notes
> > (regarding panel requirements) somewhere to the DP/DP AUX documentation?
>
> Sure. I actually don't mind writing docs, but my problem is trying to
> figure out where the heck to put them where someone would actually
> notice them. I could throw a blurb in the kernel doc for `struct
> drm_dp_aux` I guess? How about a deal: if you can tell me where to put
> the above facts (essentially 5a and 5b) then I'm happy to post a patch
> adding them.
>
> Huh, actually, maybe the right place is in the "transfer" function of
> that structure which, as of commit bacbab58f09d ("drm: Mention the
> power state requirement on side-channel operations"), actually has a
> blurb. ...but I don't think the blurb there is totally complete. What
> if I changed it to this:
>
> * Also note that this callback can be called no matter the
> * state @dev is in and also no matter what state the panel is
> * in. It's expected:
> * - If the @dev providing the AUX bus is currently unpowered then
> * it will power itself up for the transfer.
> * - If the panel is not in a state where it can respond (it's not
> * powered or it's in a low power state) then this function will
> * fail. Note that if a panel driver is initiating a DP AUX transfer
> * it may power itself up however it wants. All other code should
> * use the pre_enable() bridge chain (which eventually calls the
> * panel prepare function) to power the panel.
I didn't ignore this documentation request. Please take a look here
and see what you think:
https://lore.kernel.org/r/20220503162033.1.Ia8651894026707e4fa61267da944ff739610d180@changeid
-Doug
Re: [RFC PATCH 1/6] drm/dp: Helpers to make it easier for drivers to use DP AUX bus properly
Hi,
On Mon, Apr 18, 2022 at 4:10 PM Doug Anderson wrote:
>
> So I guess where does that leave us? Maybe:
>
> 1. I'll add a WARN_ON() in of_dp_aux_populate_ep_devices() if there is
> more than one DP AUX endpoint with a comment explaining why we assume
> one DP AUX endpoint.
>
> 2. I'll create this new structure in drm_dp_aux_bus.h:
>
> struct dp_aux_populate_callbacks {
> int (*done_probing)(struct drm_dp_aux *aux);
> void (*pre_remove)(struct drm_dp_aux *aux);
> };
>
> 3. I'll add a second version of the populate functions that AUX bus
> providers can use if they want callbacks:
>
> int of_dp_aux_populate_ep_devices_cb(struct drm_dp_aux *aux,
> struct dp_aux_populate_callbacks *cb);
> int devm_of_dp_aux_populate_ep_devices_cb(struct drm_dp_aux *aux,
> struct dp_aux_populate_callbacks
> *cb);
>
> The old functions will just be changed to wrap the above and pass NULL
> for the callbacks. To me, this seems better/simpler than notifiers or
> any other scheme, but yell if you disagree.
>
> 4. I'll call the callsbacks in dp_aux_ep_probe() after a successful
> probe. I'll add a second callback and will call it in
> dp_aux_ep_remove() before passing the remove through to the panel.
>
>
> If that sounds peachy then I think it should be pretty doable.
I never heard any response about whether people liked the above, but I
went ahead and did something similar to it. It can be found at:
https://lore.kernel.org/r/20220503224029.3195306-1-diand...@chromium.org
Re: [RFC PATCH 1/6] drm/dp: Helpers to make it easier for drivers to use DP AUX bus properly
Hi, On Fri, Apr 15, 2022 at 5:54 PM Dmitry Baryshkov wrote: > > >>> * They rely on there being exactly 1 AUX device, or we make it a rule > >>> that we wait for all AUX devices to probe (?) > >> > >> Is the backlight a separate device on an AUX bus? Judging from > >> drm_panel_dp_aux_backlight(), it isn't. I assumed that aux bus is just > >> a point-to-point bus, so there is always a single client. > > > > Define "device". ;-) > > "a device on the AUX bus" = the device, which lists dp_aux_bus_type as > dev->bus_type. Right. So I guess I was thinking that we _could_ have modeled the backlight as a device which lists dp_aux_bus_type as dev->bus_type. AKA: 1. We could have had a second node in the sc7180-trogdor-homestar device tree under the DP controller for the DP AUX backlight. 2. Instead of manually calling drm_panel_dp_aux_backlight() from panel-edp.c and panel-samsung-atna33xc20.c the backlight could have just probed on its own. 3. In the device tree, the panel could have had a link to the backlight's phandle which would have made the existing drm_panel_of_backlight() "just work" and we wouldn't have needed the manual call to drm_panel_dp_aux_backlight(). Oh. But. Maybe. Not. We couldn't really have done that because we would have been able to do the DP AUX transactions for the backlight without powering on the panel. So we couldn't really have probed them separately. OK, you guys win this round. ;-) > > It's a seperate "struct device" from a Linux point of view since it's > > a backlight class device. Certainly it's highly correlated to the > > display, but one can conceptually think of them as different devices, > > sorta. ;-) > > > > I actually dug a tiny bit more into the whole "touchscreen over aux". > > I guess DP 1.2 has a standard of "USB over DP AUX". No idea how that > > would be modeled, of course. > > Ugh. Do you have any details of the standard itself? Like how does it > looks like from the host point of view. And if the AUX is required to be > powered for this USB bus to work? > > In other words: if we were to model it at this moment, would it be the > child of the panel device (like backlight) or a separate device sitting > on the same AUX bus? I spent a bunch of time searching and I couldn't find more than a reference, like "hey we came up with this idea and we're gonna write down in the spec that you could totally do USB over the AUX channel, but u, we haven't actually implemented it yet". ;-) I certainly could be wrong, but all of the references I could find were distinctly lacking in details or pointers to other docs w/ more info. ...but while searching I _did_ find a lot of details (in the eDP 1.4 spec) about "Multi-touch over AUX". That looks like something that's actually more well thought out and maybe even implemented somewhere. >From what I can tell though, it looks as if it's the same thing as the backlight. In other words it's only available when the panel is powered. I don't think we want to do anything so drastic like moving the ownership of panel power to the AUX bus or anything, so I guess this means that: a) The panel driver will remain in charge of powering the panel (including anything on the DP AUX bus) on and off and getting the power sequencing right. b) That means that we can really think of the panel as the _only_ thing on the DP AUX bus. c) Anything else on the DP AUX bus will be underneath the panel driver. > > I guess the summary is that I'm OK w/ changing it to assume one device > > for now, but I'm still not sure it's compelling to move to normal > > callbacks. The API for callbacks is pretty much the same as the one I > > proposed and IMO leaving it the way it is (with an extra struct > > device) doesn't really add much complexity and has a few (small) nice > > benefits. > > I think Stephen didn't like too many similarities between > dp_aux_ep_client and dp_aux_ep_device. And I'd second him here. > > > >>> * We need to come up with a system for detecting when everything > >>> probes or is going to be removed, though that's probably not too hard. > >>> I guess the DP AUX bus could just replace the panel's probe function > >>> with its own and essentially "tail patch" it. I guess it could "head > >>> patch" the remove call? ...or is there some better way you were > >>> thinking of knowing when all our children probed? > >>> > >>> * The callback on the aux bus controller side would not be able to > >>> DEFER. In other words trying to acquire a reference to the panel can > >>> always be the last thing we do so we know there can be no reasons to > >>> defer after. This should be doable, but at least in the ps8640 case it > >>> will require changing the code a bit. I notice that today it actually > >>> tries to get the panel side _before_ it gets the MIPI side and it > >>> potentially can return -EPROBE_DEFER if it can't find the MIPI side. I > >>> guess I have a niggling feeling that we'll find some reason in the > >>> future that we can't
Re: [RFC PATCH 1/6] drm/dp: Helpers to make it easier for drivers to use DP AUX bus properly
On 16/04/2022 03:09, Doug Anderson wrote:
Hi,
On Fri, Apr 15, 2022 at 3:45 PM Dmitry Baryshkov
wrote:
On Sat, 16 Apr 2022 at 00:13, Doug Anderson wrote:
Hi,
On Thu, Apr 14, 2022 at 5:47 PM Dmitry Baryshkov
wrote:
On 09/04/2022 05:36, Douglas Anderson wrote:
As talked about in the kerneldoc for "struct dp_aux_ep_client" in this
patch and also in the past in commit a1e3667a9835 ("drm/bridge:
ti-sn65dsi86: Promote the AUX channel to its own sub-dev"), to use the
DP AUX bus properly we really need two "struct device"s. One "struct
device" is in charge of providing the DP AUX bus and the other is
where we'll try to get a reference to the newly probed endpoint
devices.
In ti-sn65dsi86 this wasn't too difficult to accomplish. That driver
is already broken up into several "struct devices" anyway because it
also provides a PWM and some GPIOs. Adding one more wasn't that
difficult / ugly.
When I tried to do the same solution in parade-ps8640, it felt like I
was copying too much boilerplate code. I made the realization that I
didn't _really_ need a separate "driver" for each person that wanted
to do the same thing. By putting all the "driver" related code in a
common place then we could save a bit of hassle. This change
effectively adds a new "ep_client" driver that can be used by
anyone. The devices instantiated by this driver will just call through
to the probe/remove/shutdown calls provided.
At the moment, the "ep_client" driver is backed by the Linux auxiliary
bus (unfortunate naming--this has nothing to do with DP AUX). I didn't
want to expose this to clients, though, so as far as clients are
concerned they get a vanilla "struct device".
I have been thinking about your approach for quite some time. I think
that enforcing a use of auxilliary device is an overkill. What do we
really need is the the set callbacks in the bus struct or a notifier. We
have to notify the aux_bus controller side that the client has been
probed successfully or that the client is going to be removed.
It seems like these new callbacks would be nearly the same as the
probe/remove callbacks in my proposal except:
* They rely on there being exactly 1 AUX device, or we make it a rule
that we wait for all AUX devices to probe (?)
Is the backlight a separate device on an AUX bus? Judging from
drm_panel_dp_aux_backlight(), it isn't. I assumed that aux bus is just
a point-to-point bus, so there is always a single client.
Define "device". ;-)
"a device on the AUX bus" = the device, which lists dp_aux_bus_type as
dev->bus_type.
It's a seperate "struct device" from a Linux point of view since it's
a backlight class device. Certainly it's highly correlated to the
display, but one can conceptually think of them as different devices,
sorta. ;-)
I actually dug a tiny bit more into the whole "touchscreen over aux".
I guess DP 1.2 has a standard of "USB over DP AUX". No idea how that
would be modeled, of course.
Ugh. Do you have any details of the standard itself? Like how does it
looks like from the host point of view. And if the AUX is required to be
powered for this USB bus to work?
In other words: if we were to model it at this moment, would it be the
child of the panel device (like backlight) or a separate device sitting
on the same AUX bus?
I guess the summary is that I'm OK w/ changing it to assume one device
for now, but I'm still not sure it's compelling to move to normal
callbacks. The API for callbacks is pretty much the same as the one I
proposed and IMO leaving it the way it is (with an extra struct
device) doesn't really add much complexity and has a few (small) nice
benefits.
I think Stephen didn't like too many similarities between
dp_aux_ep_client and dp_aux_ep_device. And I'd second him here.
* We need to come up with a system for detecting when everything
probes or is going to be removed, though that's probably not too hard.
I guess the DP AUX bus could just replace the panel's probe function
with its own and essentially "tail patch" it. I guess it could "head
patch" the remove call? ...or is there some better way you were
thinking of knowing when all our children probed?
* The callback on the aux bus controller side would not be able to
DEFER. In other words trying to acquire a reference to the panel can
always be the last thing we do so we know there can be no reasons to
defer after. This should be doable, but at least in the ps8640 case it
will require changing the code a bit. I notice that today it actually
tries to get the panel side _before_ it gets the MIPI side and it
potentially can return -EPROBE_DEFER if it can't find the MIPI side. I
guess I have a niggling feeling that we'll find some reason in the
future that we can't be last, but we can probably ignore that. ;-)
I can switch this all to normal callbacks if that's what everyone
wants, but it doesn't feel significantly cleaner to me and does seem
to have some (small) downsides.
And this
approach would make driver's
Re: [RFC PATCH 1/6] drm/dp: Helpers to make it easier for drivers to use DP AUX bus properly
Hi,
On Fri, Apr 15, 2022 at 3:45 PM Dmitry Baryshkov
wrote:
>
> On Sat, 16 Apr 2022 at 00:13, Doug Anderson wrote:
> >
> > Hi,
> >
> > On Thu, Apr 14, 2022 at 5:47 PM Dmitry Baryshkov
> > wrote:
> > >
> > > On 09/04/2022 05:36, Douglas Anderson wrote:
> > > > As talked about in the kerneldoc for "struct dp_aux_ep_client" in this
> > > > patch and also in the past in commit a1e3667a9835 ("drm/bridge:
> > > > ti-sn65dsi86: Promote the AUX channel to its own sub-dev"), to use the
> > > > DP AUX bus properly we really need two "struct device"s. One "struct
> > > > device" is in charge of providing the DP AUX bus and the other is
> > > > where we'll try to get a reference to the newly probed endpoint
> > > > devices.
> > > >
> > > > In ti-sn65dsi86 this wasn't too difficult to accomplish. That driver
> > > > is already broken up into several "struct devices" anyway because it
> > > > also provides a PWM and some GPIOs. Adding one more wasn't that
> > > > difficult / ugly.
> > > >
> > > > When I tried to do the same solution in parade-ps8640, it felt like I
> > > > was copying too much boilerplate code. I made the realization that I
> > > > didn't _really_ need a separate "driver" for each person that wanted
> > > > to do the same thing. By putting all the "driver" related code in a
> > > > common place then we could save a bit of hassle. This change
> > > > effectively adds a new "ep_client" driver that can be used by
> > > > anyone. The devices instantiated by this driver will just call through
> > > > to the probe/remove/shutdown calls provided.
> > > >
> > > > At the moment, the "ep_client" driver is backed by the Linux auxiliary
> > > > bus (unfortunate naming--this has nothing to do with DP AUX). I didn't
> > > > want to expose this to clients, though, so as far as clients are
> > > > concerned they get a vanilla "struct device".
> > >
> > > I have been thinking about your approach for quite some time. I think
> > > that enforcing a use of auxilliary device is an overkill. What do we
> > > really need is the the set callbacks in the bus struct or a notifier. We
> > > have to notify the aux_bus controller side that the client has been
> > > probed successfully or that the client is going to be removed.
> >
> > It seems like these new callbacks would be nearly the same as the
> > probe/remove callbacks in my proposal except:
> >
> > * They rely on there being exactly 1 AUX device, or we make it a rule
> > that we wait for all AUX devices to probe (?)
>
> Is the backlight a separate device on an AUX bus? Judging from
> drm_panel_dp_aux_backlight(), it isn't. I assumed that aux bus is just
> a point-to-point bus, so there is always a single client.
Define "device". ;-)
It's a seperate "struct device" from a Linux point of view since it's
a backlight class device. Certainly it's highly correlated to the
display, but one can conceptually think of them as different devices,
sorta. ;-)
I actually dug a tiny bit more into the whole "touchscreen over aux".
I guess DP 1.2 has a standard of "USB over DP AUX". No idea how that
would be modeled, of course.
I guess the summary is that I'm OK w/ changing it to assume one device
for now, but I'm still not sure it's compelling to move to normal
callbacks. The API for callbacks is pretty much the same as the one I
proposed and IMO leaving it the way it is (with an extra struct
device) doesn't really add much complexity and has a few (small) nice
benefits.
> > * We need to come up with a system for detecting when everything
> > probes or is going to be removed, though that's probably not too hard.
> > I guess the DP AUX bus could just replace the panel's probe function
> > with its own and essentially "tail patch" it. I guess it could "head
> > patch" the remove call? ...or is there some better way you were
> > thinking of knowing when all our children probed?
> >
> > * The callback on the aux bus controller side would not be able to
> > DEFER. In other words trying to acquire a reference to the panel can
> > always be the last thing we do so we know there can be no reasons to
> > defer after. This should be doable, but at least in the ps8640 case it
> > will require changing the code a bit. I notice that today it actually
> > tries to get the panel side _before_ it gets the MIPI side and it
> > potentially can return -EPROBE_DEFER if it can't find the MIPI side. I
> > guess I have a niggling feeling that we'll find some reason in the
> > future that we can't be last, but we can probably ignore that. ;-)
> >
> > I can switch this all to normal callbacks if that's what everyone
> > wants, but it doesn't feel significantly cleaner to me and does seem
> > to have some (small) downsides.
> >
> >
> > > And this
> > > approach would make driver's life easier, since e.g. the bus code can
> > > pm_get the EP device before calling callbacks/notifiers and
> > > pm_put_autosuspend it afterwards.
> >
> > Not sure about doing the pm calls on behalf of the EP device. What's
> >
Re: [RFC PATCH 1/6] drm/dp: Helpers to make it easier for drivers to use DP AUX bus properly
On Sat, 16 Apr 2022 at 00:13, Doug Anderson wrote:
>
> Hi,
>
> On Thu, Apr 14, 2022 at 5:47 PM Dmitry Baryshkov
> wrote:
> >
> > On 09/04/2022 05:36, Douglas Anderson wrote:
> > > As talked about in the kerneldoc for "struct dp_aux_ep_client" in this
> > > patch and also in the past in commit a1e3667a9835 ("drm/bridge:
> > > ti-sn65dsi86: Promote the AUX channel to its own sub-dev"), to use the
> > > DP AUX bus properly we really need two "struct device"s. One "struct
> > > device" is in charge of providing the DP AUX bus and the other is
> > > where we'll try to get a reference to the newly probed endpoint
> > > devices.
> > >
> > > In ti-sn65dsi86 this wasn't too difficult to accomplish. That driver
> > > is already broken up into several "struct devices" anyway because it
> > > also provides a PWM and some GPIOs. Adding one more wasn't that
> > > difficult / ugly.
> > >
> > > When I tried to do the same solution in parade-ps8640, it felt like I
> > > was copying too much boilerplate code. I made the realization that I
> > > didn't _really_ need a separate "driver" for each person that wanted
> > > to do the same thing. By putting all the "driver" related code in a
> > > common place then we could save a bit of hassle. This change
> > > effectively adds a new "ep_client" driver that can be used by
> > > anyone. The devices instantiated by this driver will just call through
> > > to the probe/remove/shutdown calls provided.
> > >
> > > At the moment, the "ep_client" driver is backed by the Linux auxiliary
> > > bus (unfortunate naming--this has nothing to do with DP AUX). I didn't
> > > want to expose this to clients, though, so as far as clients are
> > > concerned they get a vanilla "struct device".
> >
> > I have been thinking about your approach for quite some time. I think
> > that enforcing a use of auxilliary device is an overkill. What do we
> > really need is the the set callbacks in the bus struct or a notifier. We
> > have to notify the aux_bus controller side that the client has been
> > probed successfully or that the client is going to be removed.
>
> It seems like these new callbacks would be nearly the same as the
> probe/remove callbacks in my proposal except:
>
> * They rely on there being exactly 1 AUX device, or we make it a rule
> that we wait for all AUX devices to probe (?)
Is the backlight a separate device on an AUX bus? Judging from
drm_panel_dp_aux_backlight(), it isn't. I assumed that aux bus is just
a point-to-point bus, so there is always a single client.
>
> * We need to come up with a system for detecting when everything
> probes or is going to be removed, though that's probably not too hard.
> I guess the DP AUX bus could just replace the panel's probe function
> with its own and essentially "tail patch" it. I guess it could "head
> patch" the remove call? ...or is there some better way you were
> thinking of knowing when all our children probed?
>
> * The callback on the aux bus controller side would not be able to
> DEFER. In other words trying to acquire a reference to the panel can
> always be the last thing we do so we know there can be no reasons to
> defer after. This should be doable, but at least in the ps8640 case it
> will require changing the code a bit. I notice that today it actually
> tries to get the panel side _before_ it gets the MIPI side and it
> potentially can return -EPROBE_DEFER if it can't find the MIPI side. I
> guess I have a niggling feeling that we'll find some reason in the
> future that we can't be last, but we can probably ignore that. ;-)
>
> I can switch this all to normal callbacks if that's what everyone
> wants, but it doesn't feel significantly cleaner to me and does seem
> to have some (small) downsides.
>
>
> > And this
> > approach would make driver's life easier, since e.g. the bus code can
> > pm_get the EP device before calling callbacks/notifiers and
> > pm_put_autosuspend it afterwards.
>
> Not sure about doing the pm calls on behalf of the EP device. What's
> the goal there?
I think any driver can pm_runtime_get another device. The goal is to
let the 'post_probe' callback to power up the panel, read the EDID,
etc.
BTW: as I'm slowly diving into DP vs eDP differences. Do we need to
write the EDID checksum like we do for DP?
Do you have any good summary for eDP vs DP differences?
--
With best wishes
Dmitry
Re: [RFC PATCH 1/6] drm/dp: Helpers to make it easier for drivers to use DP AUX bus properly
Hi,
On Thu, Apr 14, 2022 at 5:47 PM Dmitry Baryshkov
wrote:
>
> On 09/04/2022 05:36, Douglas Anderson wrote:
> > As talked about in the kerneldoc for "struct dp_aux_ep_client" in this
> > patch and also in the past in commit a1e3667a9835 ("drm/bridge:
> > ti-sn65dsi86: Promote the AUX channel to its own sub-dev"), to use the
> > DP AUX bus properly we really need two "struct device"s. One "struct
> > device" is in charge of providing the DP AUX bus and the other is
> > where we'll try to get a reference to the newly probed endpoint
> > devices.
> >
> > In ti-sn65dsi86 this wasn't too difficult to accomplish. That driver
> > is already broken up into several "struct devices" anyway because it
> > also provides a PWM and some GPIOs. Adding one more wasn't that
> > difficult / ugly.
> >
> > When I tried to do the same solution in parade-ps8640, it felt like I
> > was copying too much boilerplate code. I made the realization that I
> > didn't _really_ need a separate "driver" for each person that wanted
> > to do the same thing. By putting all the "driver" related code in a
> > common place then we could save a bit of hassle. This change
> > effectively adds a new "ep_client" driver that can be used by
> > anyone. The devices instantiated by this driver will just call through
> > to the probe/remove/shutdown calls provided.
> >
> > At the moment, the "ep_client" driver is backed by the Linux auxiliary
> > bus (unfortunate naming--this has nothing to do with DP AUX). I didn't
> > want to expose this to clients, though, so as far as clients are
> > concerned they get a vanilla "struct device".
>
> I have been thinking about your approach for quite some time. I think
> that enforcing a use of auxilliary device is an overkill. What do we
> really need is the the set callbacks in the bus struct or a notifier. We
> have to notify the aux_bus controller side that the client has been
> probed successfully or that the client is going to be removed.
It seems like these new callbacks would be nearly the same as the
probe/remove callbacks in my proposal except:
* They rely on there being exactly 1 AUX device, or we make it a rule
that we wait for all AUX devices to probe (?)
* We need to come up with a system for detecting when everything
probes or is going to be removed, though that's probably not too hard.
I guess the DP AUX bus could just replace the panel's probe function
with its own and essentially "tail patch" it. I guess it could "head
patch" the remove call? ...or is there some better way you were
thinking of knowing when all our children probed?
* The callback on the aux bus controller side would not be able to
DEFER. In other words trying to acquire a reference to the panel can
always be the last thing we do so we know there can be no reasons to
defer after. This should be doable, but at least in the ps8640 case it
will require changing the code a bit. I notice that today it actually
tries to get the panel side _before_ it gets the MIPI side and it
potentially can return -EPROBE_DEFER if it can't find the MIPI side. I
guess I have a niggling feeling that we'll find some reason in the
future that we can't be last, but we can probably ignore that. ;-)
I can switch this all to normal callbacks if that's what everyone
wants, but it doesn't feel significantly cleaner to me and does seem
to have some (small) downsides.
> And this
> approach would make driver's life easier, since e.g. the bus code can
> pm_get the EP device before calling callbacks/notifiers and
> pm_put_autosuspend it afterwards.
Not sure about doing the pm calls on behalf of the EP device. What's
the goal there?
Re: [RFC PATCH 1/6] drm/dp: Helpers to make it easier for drivers to use DP AUX bus properly
Hi,
On Thu, Apr 14, 2022 at 4:52 PM Stephen Boyd wrote:
>
> Quoting Douglas Anderson (2022-04-08 19:36:23)
> > As talked about in the kerneldoc for "struct dp_aux_ep_client" in this
> > patch and also in the past in commit a1e3667a9835 ("drm/bridge:
> > ti-sn65dsi86: Promote the AUX channel to its own sub-dev"), to use the
> > DP AUX bus properly we really need two "struct device"s. One "struct
> > device" is in charge of providing the DP AUX bus and the other is
> > where we'll try to get a reference to the newly probed endpoint
> > devices.
> >
> > In ti-sn65dsi86 this wasn't too difficult to accomplish. That driver
> > is already broken up into several "struct devices" anyway because it
> > also provides a PWM and some GPIOs. Adding one more wasn't that
> > difficult / ugly.
> >
> > When I tried to do the same solution in parade-ps8640, it felt like I
> > was copying too much boilerplate code. I made the realization that I
> > didn't _really_ need a separate "driver" for each person that wanted
> > to do the same thing. By putting all the "driver" related code in a
> > common place then we could save a bit of hassle. This change
> > effectively adds a new "ep_client" driver that can be used by
> > anyone. The devices instantiated by this driver will just call through
> > to the probe/remove/shutdown calls provided.
> >
> > At the moment, the "ep_client" driver is backed by the Linux auxiliary
> > bus (unfortunate naming--this has nothing to do with DP AUX). I didn't
> > want to expose this to clients, though, so as far as clients are
> > concerned they get a vanilla "struct device".
> >
> > Signed-off-by: Douglas Anderson
> > ---
>
> Is it correct that the struct dp_aux_ep_client is largely equivalent to
> a struct dp_aux_ep_device? What's the difference? I think it is a fully
> probed dp_aux_ep_device? Or a way to tell the driver that calls
> of_dp_aux_populate_ep_devices() that the endpoint has probed now?
They're not the same. The "DP AUX Endpoint Device" is essentially the
panel, though at one point in time someone argued that conceivably one
could put other devices on it even though this might be a really bad
idea. At some point in the discussion someone mentioned the concept of
a touchscreen running over DP Aux had been discussed and, indeed, "dp
aux touchscreen" gets hits if you search for it. The idea that it
could be something different is one reason why I refrained from
calling it a panel in all the function names and always tried to call
it a "DP AUX Endpoint".
The "DP AUX Endpoint Device Client" is the client of the panel, or the
code that needs to get a reference to the panel. Logically, I guess
this is the part of the eDP controller that's responsible for shoving
bits to the panel. Essentially the drm_bridge. Most importantly, it's
_not_ the part of the eDP controller providing the AUX bus.
> I read the commit text but it didn't click until I read the next patch
> that this is solving a probe ordering/loop problem. The driver that
> creates the 'struct drm_dp_aux' and populates devices on the DP aux bus
> with of_dp_aux_populate_ep_devices() wants to be sure that the
> drm_bridge made by the 'struct dp_aux_ep_driver' probe routine in
> edp-panel is going to be there before calling drm_of_get_bridge().
>
> of_dp_aux_populate_ep_devices();
> [No idea if the bridge is registered yet]
> drm_of_get_bridge();
>
> The solution is to retry the drm_of_get_bridge() until 'struct
> dp_aux_ep_driver' probes and registers the next bridge. It looks like a
> wait_for_completion() on top of drm_of_get_bridge() implemented through
> driver probe and -EPROBE_DEFER; no disrespect!
Yes, that's exactly what it is.
> Is there another problem here that the driver that creates the 'struct
> drm_dp_aux' and populates devices on the DP aux bus with
> of_dp_aux_populate_ep_devices() wants to use that same 'struct
> drm_dp_aux' directly to poke at some 'struct dp_aux_ep_device' that was
> populated? And the 'struct dp_aux_ep_device' may either not be probed
> and bound to a 'struct dp_aux_ep_driver' or it may not be powered on
> because it went to runtime PM suspend?
>
> Put another way, I worry that the owner of 'struct drm_dp_aux' wants to
> use some function in include/drm/dp/drm_dp_helper.h that takes the
> 'struct drm_dp_aux' directly without considering the device model state
> of the endpoint device (the 'struct dp_aux_ep_device'). That would be a
> similar problem as waiting for the endpoint to be powered on and probed.
> Solving that through a sub-driver feels awkward.
>
> What if we had some function like drm_dp_get_aux_ep() that took a
> 'struct drm_dp_aux' and looked for the endpoint device (there can only
> be one?),
The code is designed to handle the fact that there could be more than
one AUX endpoint device. I don't know if this will ever happen but it
is plausible. The "touchscreen over DP AUX", if that ever became a
thing supported in Linux, could be an example. In
Re: [RFC PATCH 1/6] drm/dp: Helpers to make it easier for drivers to use DP AUX bus properly
On 09/04/2022 05:36, Douglas Anderson wrote:
As talked about in the kerneldoc for "struct dp_aux_ep_client" in this
patch and also in the past in commit a1e3667a9835 ("drm/bridge:
ti-sn65dsi86: Promote the AUX channel to its own sub-dev"), to use the
DP AUX bus properly we really need two "struct device"s. One "struct
device" is in charge of providing the DP AUX bus and the other is
where we'll try to get a reference to the newly probed endpoint
devices.
In ti-sn65dsi86 this wasn't too difficult to accomplish. That driver
is already broken up into several "struct devices" anyway because it
also provides a PWM and some GPIOs. Adding one more wasn't that
difficult / ugly.
When I tried to do the same solution in parade-ps8640, it felt like I
was copying too much boilerplate code. I made the realization that I
didn't _really_ need a separate "driver" for each person that wanted
to do the same thing. By putting all the "driver" related code in a
common place then we could save a bit of hassle. This change
effectively adds a new "ep_client" driver that can be used by
anyone. The devices instantiated by this driver will just call through
to the probe/remove/shutdown calls provided.
At the moment, the "ep_client" driver is backed by the Linux auxiliary
bus (unfortunate naming--this has nothing to do with DP AUX). I didn't
want to expose this to clients, though, so as far as clients are
concerned they get a vanilla "struct device".
I have been thinking about your approach for quite some time. I think
that enforcing a use of auxilliary device is an overkill. What do we
really need is the the set callbacks in the bus struct or a notifier. We
have to notify the aux_bus controller side that the client has been
probed successfully or that the client is going to be removed. And this
approach would make driver's life easier, since e.g. the bus code can
pm_get the EP device before calling callbacks/notifiers and
pm_put_autosuspend it afterwards.
Signed-off-by: Douglas Anderson
---
drivers/gpu/drm/dp/drm_dp_aux_bus.c | 165 +++-
include/drm/dp/drm_dp_aux_bus.h | 58 ++
2 files changed, 222 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/dp/drm_dp_aux_bus.c
b/drivers/gpu/drm/dp/drm_dp_aux_bus.c
index 415afce3cf96..5386ceacf133 100644
--- a/drivers/gpu/drm/dp/drm_dp_aux_bus.c
+++ b/drivers/gpu/drm/dp/drm_dp_aux_bus.c
@@ -12,6 +12,7 @@
* to perform transactions on that bus.
*/
+#include
#include
#include
#include
@@ -299,6 +300,163 @@ void dp_aux_dp_driver_unregister(struct dp_aux_ep_driver
*drv)
}
EXPORT_SYMBOL_GPL(dp_aux_dp_driver_unregister);
+/* -
+ * DP AUX EP Client
+ */
+
+struct dp_aux_ep_client_data {
+ struct dp_aux_ep_client *client;
+ struct auxiliary_device adev;
+};
+
+static int dp_aux_ep_client_probe(struct auxiliary_device *adev,
+ const struct auxiliary_device_id *id)
+{
+ struct dp_aux_ep_client_data *data =
+ container_of(adev, struct dp_aux_ep_client_data, adev);
+
+ if (!data->client->probe)
+ return 0;
+
+ return data->client->probe(>dev, data->client);
+}
+
+static void dp_aux_ep_client_remove(struct auxiliary_device *adev)
+{
+ struct dp_aux_ep_client_data *data =
+ container_of(adev, struct dp_aux_ep_client_data, adev);
+
+ if (data->client->remove)
+ data->client->remove(>dev, data->client);
+}
+
+static void dp_aux_ep_client_shutdown(struct auxiliary_device *adev)
+{
+ struct dp_aux_ep_client_data *data =
+ container_of(adev, struct dp_aux_ep_client_data, adev);
+
+ if (data->client->shutdown)
+ data->client->shutdown(>dev, data->client);
+}
+
+static void dp_aux_ep_client_dev_release(struct device *dev)
+{
+ struct auxiliary_device *adev = to_auxiliary_dev(dev);
+ struct dp_aux_ep_client_data *data =
+ container_of(adev, struct dp_aux_ep_client_data, adev);
+
+ kfree(data);
+}
+
+/**
+ * dp_aux_register_ep_client() - Register an DP AUX EP client
+ * @client: The structure describing the client. It's the client's
+ * responsibility to keep this memory around until
+ * dp_aux_unregister_ep_client() is called, either explicitly or
+ * implicitly via devm.
+ *
+ * See the description of "struct dp_aux_ep_client" for a full explanation
+ * of when you should use this and why.
+ *
+ * Return: 0 if no error or negative error code.
+ */
+int dp_aux_register_ep_client(struct dp_aux_ep_client *client)
+{
+ struct dp_aux_ep_client_data *data;
+ int ret;
+
+ data = kzalloc(sizeof(*data), GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
+
+ data->client = client;
+ data->adev.name = "ep_client";
+ data->adev.dev.parent = client->aux->dev;
+
Re: [RFC PATCH 1/6] drm/dp: Helpers to make it easier for drivers to use DP AUX bus properly
Quoting Douglas Anderson (2022-04-08 19:36:23)
> As talked about in the kerneldoc for "struct dp_aux_ep_client" in this
> patch and also in the past in commit a1e3667a9835 ("drm/bridge:
> ti-sn65dsi86: Promote the AUX channel to its own sub-dev"), to use the
> DP AUX bus properly we really need two "struct device"s. One "struct
> device" is in charge of providing the DP AUX bus and the other is
> where we'll try to get a reference to the newly probed endpoint
> devices.
>
> In ti-sn65dsi86 this wasn't too difficult to accomplish. That driver
> is already broken up into several "struct devices" anyway because it
> also provides a PWM and some GPIOs. Adding one more wasn't that
> difficult / ugly.
>
> When I tried to do the same solution in parade-ps8640, it felt like I
> was copying too much boilerplate code. I made the realization that I
> didn't _really_ need a separate "driver" for each person that wanted
> to do the same thing. By putting all the "driver" related code in a
> common place then we could save a bit of hassle. This change
> effectively adds a new "ep_client" driver that can be used by
> anyone. The devices instantiated by this driver will just call through
> to the probe/remove/shutdown calls provided.
>
> At the moment, the "ep_client" driver is backed by the Linux auxiliary
> bus (unfortunate naming--this has nothing to do with DP AUX). I didn't
> want to expose this to clients, though, so as far as clients are
> concerned they get a vanilla "struct device".
>
> Signed-off-by: Douglas Anderson
> ---
Is it correct that the struct dp_aux_ep_client is largely equivalent to
a struct dp_aux_ep_device? What's the difference? I think it is a fully
probed dp_aux_ep_device? Or a way to tell the driver that calls
of_dp_aux_populate_ep_devices() that the endpoint has probed now?
I read the commit text but it didn't click until I read the next patch
that this is solving a probe ordering/loop problem. The driver that
creates the 'struct drm_dp_aux' and populates devices on the DP aux bus
with of_dp_aux_populate_ep_devices() wants to be sure that the
drm_bridge made by the 'struct dp_aux_ep_driver' probe routine in
edp-panel is going to be there before calling drm_of_get_bridge().
of_dp_aux_populate_ep_devices();
[No idea if the bridge is registered yet]
drm_of_get_bridge();
The solution is to retry the drm_of_get_bridge() until 'struct
dp_aux_ep_driver' probes and registers the next bridge. It looks like a
wait_for_completion() on top of drm_of_get_bridge() implemented through
driver probe and -EPROBE_DEFER; no disrespect!
Is there another problem here that the driver that creates the 'struct
drm_dp_aux' and populates devices on the DP aux bus with
of_dp_aux_populate_ep_devices() wants to use that same 'struct
drm_dp_aux' directly to poke at some 'struct dp_aux_ep_device' that was
populated? And the 'struct dp_aux_ep_device' may either not be probed
and bound to a 'struct dp_aux_ep_driver' or it may not be powered on
because it went to runtime PM suspend?
Put another way, I worry that the owner of 'struct drm_dp_aux' wants to
use some function in include/drm/dp/drm_dp_helper.h that takes the
'struct drm_dp_aux' directly without considering the device model state
of the endpoint device (the 'struct dp_aux_ep_device'). That would be a
similar problem as waiting for the endpoint to be powered on and probed.
Solving that through a sub-driver feels awkward.
What if we had some function like drm_dp_get_aux_ep() that took a
'struct drm_dp_aux' and looked for the endpoint device (there can only
be one?), waited for it to be probed, and then powered it up via some
pm_runtime_get_sync()? My understanding is that with edp-panel we have
two power "domains" of importance, the panel power domain and the DP/eDP
power domain. Access to the AUX bus via 'struct drm_dp_aux' needs both
power domains to be powered on. If the 'struct dp_aux_ep_device' is in
hand, then both power domains can be powered on by making sure the power
state of the 'struct dp_aux_ep_device::dev' is enabled. If only the
'struct drm_dp_aux' is in hand then we'll need to do something more like
find the child device and power it on.
Could the 'struct drm_dp_aux' functions in drm_dp_helper.h do this
automatically somehow? Maybe we can drop a variable in 'struct
drm_dp_aux' when of_dp_aux_populate_ep_devices() is called that the
other side may not be powered up. Then if something tries to transfer on
that aux channel it powers on all children of 'struct drm_dp_aux::dev'
that are on the 'dp_aux_bus_type' or bails out if those devices haven't
probed yet or can't be powered on.
> diff --git a/include/drm/dp/drm_dp_aux_bus.h b/include/drm/dp/drm_dp_aux_bus.h
> index 4f19b20b1dd6..ecf68b6873bd 100644
> --- a/include/drm/dp/drm_dp_aux_bus.h
> +++ b/include/drm/dp/drm_dp_aux_bus.h
> @@ -54,4 +54,62 @@ int __dp_aux_dp_driver_register(struct dp_aux_ep_driver
> *aux_ep_drv,
> struct module
Re: [RFC PATCH 1/6] drm/dp: Helpers to make it easier for drivers to use DP AUX bus properly
Hi,
On Mon, Apr 11, 2022 at 1:34 AM Jani Nikula wrote:
>
> On Fri, 08 Apr 2022, Douglas Anderson wrote:
> > As talked about in the kerneldoc for "struct dp_aux_ep_client" in this
> > patch and also in the past in commit a1e3667a9835 ("drm/bridge:
> > ti-sn65dsi86: Promote the AUX channel to its own sub-dev"), to use the
> > DP AUX bus properly we really need two "struct device"s. One "struct
> > device" is in charge of providing the DP AUX bus and the other is
> > where we'll try to get a reference to the newly probed endpoint
> > devices.
> >
> > In ti-sn65dsi86 this wasn't too difficult to accomplish. That driver
> > is already broken up into several "struct devices" anyway because it
> > also provides a PWM and some GPIOs. Adding one more wasn't that
> > difficult / ugly.
> >
> > When I tried to do the same solution in parade-ps8640, it felt like I
> > was copying too much boilerplate code. I made the realization that I
> > didn't _really_ need a separate "driver" for each person that wanted
> > to do the same thing. By putting all the "driver" related code in a
> > common place then we could save a bit of hassle. This change
> > effectively adds a new "ep_client" driver that can be used by
> > anyone. The devices instantiated by this driver will just call through
> > to the probe/remove/shutdown calls provided.
> >
> > At the moment, the "ep_client" driver is backed by the Linux auxiliary
> > bus (unfortunate naming--this has nothing to do with DP AUX). I didn't
> > want to expose this to clients, though, so as far as clients are
> > concerned they get a vanilla "struct device".
> >
> > Signed-off-by: Douglas Anderson
>
> What is an "EP" client or device?
The DP AUX EndPoint (or DP AUX EP) is just the generic name I called
the thing on the other side of the DP AUX bus, AKA the "panel".
The "DP AUX EP client" (ep_client) is the code that needs a reference
to the panel.
I'll beef up the patch description and the comments around the
structure to try to make this clearer.
-Doug
Re: [RFC PATCH 1/6] drm/dp: Helpers to make it easier for drivers to use DP AUX bus properly
On Fri, 08 Apr 2022, Douglas Anderson wrote:
> As talked about in the kerneldoc for "struct dp_aux_ep_client" in this
> patch and also in the past in commit a1e3667a9835 ("drm/bridge:
> ti-sn65dsi86: Promote the AUX channel to its own sub-dev"), to use the
> DP AUX bus properly we really need two "struct device"s. One "struct
> device" is in charge of providing the DP AUX bus and the other is
> where we'll try to get a reference to the newly probed endpoint
> devices.
>
> In ti-sn65dsi86 this wasn't too difficult to accomplish. That driver
> is already broken up into several "struct devices" anyway because it
> also provides a PWM and some GPIOs. Adding one more wasn't that
> difficult / ugly.
>
> When I tried to do the same solution in parade-ps8640, it felt like I
> was copying too much boilerplate code. I made the realization that I
> didn't _really_ need a separate "driver" for each person that wanted
> to do the same thing. By putting all the "driver" related code in a
> common place then we could save a bit of hassle. This change
> effectively adds a new "ep_client" driver that can be used by
> anyone. The devices instantiated by this driver will just call through
> to the probe/remove/shutdown calls provided.
>
> At the moment, the "ep_client" driver is backed by the Linux auxiliary
> bus (unfortunate naming--this has nothing to do with DP AUX). I didn't
> want to expose this to clients, though, so as far as clients are
> concerned they get a vanilla "struct device".
>
> Signed-off-by: Douglas Anderson
What is an "EP" client or device?
BR,
Jani.
> ---
>
> drivers/gpu/drm/dp/drm_dp_aux_bus.c | 165 +++-
> include/drm/dp/drm_dp_aux_bus.h | 58 ++
> 2 files changed, 222 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/dp/drm_dp_aux_bus.c
> b/drivers/gpu/drm/dp/drm_dp_aux_bus.c
> index 415afce3cf96..5386ceacf133 100644
> --- a/drivers/gpu/drm/dp/drm_dp_aux_bus.c
> +++ b/drivers/gpu/drm/dp/drm_dp_aux_bus.c
> @@ -12,6 +12,7 @@
> * to perform transactions on that bus.
> */
>
> +#include
> #include
> #include
> #include
> @@ -299,6 +300,163 @@ void dp_aux_dp_driver_unregister(struct
> dp_aux_ep_driver *drv)
> }
> EXPORT_SYMBOL_GPL(dp_aux_dp_driver_unregister);
>
> +/*
> -
> + * DP AUX EP Client
> + */
> +
> +struct dp_aux_ep_client_data {
> + struct dp_aux_ep_client *client;
> + struct auxiliary_device adev;
> +};
> +
> +static int dp_aux_ep_client_probe(struct auxiliary_device *adev,
> + const struct auxiliary_device_id *id)
> +{
> + struct dp_aux_ep_client_data *data =
> + container_of(adev, struct dp_aux_ep_client_data, adev);
> +
> + if (!data->client->probe)
> + return 0;
> +
> + return data->client->probe(>dev, data->client);
> +}
> +
> +static void dp_aux_ep_client_remove(struct auxiliary_device *adev)
> +{
> + struct dp_aux_ep_client_data *data =
> + container_of(adev, struct dp_aux_ep_client_data, adev);
> +
> + if (data->client->remove)
> + data->client->remove(>dev, data->client);
> +}
> +
> +static void dp_aux_ep_client_shutdown(struct auxiliary_device *adev)
> +{
> + struct dp_aux_ep_client_data *data =
> + container_of(adev, struct dp_aux_ep_client_data, adev);
> +
> + if (data->client->shutdown)
> + data->client->shutdown(>dev, data->client);
> +}
> +
> +static void dp_aux_ep_client_dev_release(struct device *dev)
> +{
> + struct auxiliary_device *adev = to_auxiliary_dev(dev);
> + struct dp_aux_ep_client_data *data =
> + container_of(adev, struct dp_aux_ep_client_data, adev);
> +
> + kfree(data);
> +}
> +
> +/**
> + * dp_aux_register_ep_client() - Register an DP AUX EP client
> + * @client: The structure describing the client. It's the client's
> + * responsibility to keep this memory around until
> + * dp_aux_unregister_ep_client() is called, either explicitly or
> + * implicitly via devm.
> + *
> + * See the description of "struct dp_aux_ep_client" for a full explanation
> + * of when you should use this and why.
> + *
> + * Return: 0 if no error or negative error code.
> + */
> +int dp_aux_register_ep_client(struct dp_aux_ep_client *client)
> +{
> + struct dp_aux_ep_client_data *data;
> + int ret;
> +
> + data = kzalloc(sizeof(*data), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + data->client = client;
> + data->adev.name = "ep_client";
> + data->adev.dev.parent = client->aux->dev;
> + data->adev.dev.release = dp_aux_ep_client_dev_release;
> + device_set_of_node_from_dev(>adev.dev, client->aux->dev);
> +
> + ret = auxiliary_device_init(>adev);
> + if (ret) {
> + /*
> + * NOTE: if init doesn't fail then it takes ownership
> + * of memory and this
