Re: [PATCH v6 1/8] drm/msm/dp: Add eDP support via aux_bus
Hi Doug and Dmitry
On 4/8/2022 7:58 AM, Dmitry Baryshkov wrote:
On Fri, 8 Apr 2022 at 16:43, Doug Anderson wrote:
Hi,
On Fri, Apr 8, 2022 at 5:20 AM Dmitry Baryshkov
wrote:
I guess my thought was that in DP you could still create the AUX bus
at probe time. Then for DP you just return an instant "transfer
failed" from the AUX bus if HPD isn't asserted. For eDP (as discussed
elsewhere) when we try to do an AUX transfer then we delay until HPD
is there.
I think panel-edp would already handle the delay, so we do not need to
have this logic in the DP driver.
There's a whole discussion about this between Stephen and me in patch
#5 ("drm/msm/dp: wait for hpd high before any sink interaction").
Basically:
* If panel HPD is hooked up to the dedicated HPD pin on the eDP
controller then the panel driver doesn't have a way to read it.
I refreshed that dialog. I must admit, I have missed the fact that the
HPD pin might not be visible as the GPIO pin.
* We can't leverage the existing "HPD" query functions in DRM because
those indicate whether a panel is _physically_ connected. For eDP, it
always is.
Yes, I was thinking about (mis)using the
drm_bridge_connector_hpd_notify() for generic HPD-related
notifications (to tell eDP that it should check the current state). I
have abandoned that idea.
For now the rule is that the AUX transfer function is in charge of
waiting for HPD for eDP if the dedicated HPD pin is used. If we want
to re-invent this we could, but that system works, isn't _too_ ugly,
and we're already making big enough changes in this series.
The is_hpd_asserted() looks like a good callback for the aux bus.
It will allow the panel driver to check if the panel is powered up (in
the absence of the GPIO pin).
So we can still acquire resources (clocks, PHY, io maps, etc) at probe
time for DP and create the AUX bus, right? It will just return
"-ENODEV" if HPD isn't asserted and you're DP?
Yes, please. I still suppose that we'd need a separate case to
power_on eDP's PHY during the probe time. Maybe I'm mistaken here.
I think the ideal way is to do it like Kieran's proposal for sn65dsi86:
https://lore.kernel.org/r/20220317131250.1481275-4-kieran.bingham+rene...@ideasonboard.com/
* When enabling HPD (physical hot plug detect) in the hpd_enable()
callback you do a pm_runtime_get(). You do the
pm_runtime_put_autosuspend() when disabling. This is only used for DP
since we only provide DRM_BRIDGE_OP_HPD for DP, not for eDP.
* We do a pm_runtime_get() / pm_runtime_put_autosuspend() in the AUX
transfer routine. While holding the pm_runtime reference we check HPD.
For DP we return immediately if HPD isn't asserted. For eDP, we delay.
* We do the pm_runtime_get() in pre_enable and the pm_runtime_put() in
post_disable. For DP this will add a 2nd refcount (since we probably
were holding the reference for HPD). For eDP this will cause us to
power on.
* If there's any other time we need to read HW registers, and we
aren't guaranteed to already have a pm_runtime reference (like during
probe), we can do a temporary pm_runtime_get() /
pm_runtime_put_autosuspend().
This looks good. I'd be more than welcome to review such series.
Note: I think this would require using
drm_bridge_connector_enable_hpd() in the DP code.
Hopefully at some point we would be able to move all
drm_bridge_connector calls to the core msm layer.
--
With best wishes
Dmitry
Thanks for the proposals.
In general I would break up this task as follows:
1) Re-factoring dp/edp parsing code to move it to probe ( its currently
done in bind ). So not sure what dependencies we will uncover there.
Nonetheless, lets assume for now it can be done.
2) Then bind all the power resources needed for AUX in pm_runtime_ops
3) Handle EPROBE_DEFER cases of the panel-eDP aux device
4) Probably the biggest from our point of view --- makes sure none of
this breaks DP/eDP
Since QC will be taking ownership of all of this, I would still suggest
land this series first so that basic display functionality on sc7280
chromebooks works, unblocks more developers and this program and we can
internally evaluate all of this and post the changes as-and-when ready
for review.
So, I suggest/request acking this current one after
fixing the other comments (unrelated to this re-factor) which have been
given so far ofcourse, as we all agree this is not breaking and seems
pretty reasonable short term.
Doug, you can track this re-factor with a different bug so that all this
discussion remains intact.
Thanks
Abhinav
Re: [PATCH v6 1/8] drm/msm/dp: Add eDP support via aux_bus
On Fri, 8 Apr 2022 at 16:43, Doug Anderson wrote:
>
> Hi,
>
> On Fri, Apr 8, 2022 at 5:20 AM Dmitry Baryshkov
> wrote:
> >
> > > I guess my thought was that in DP you could still create the AUX bus
> > > at probe time. Then for DP you just return an instant "transfer
> > > failed" from the AUX bus if HPD isn't asserted. For eDP (as discussed
> > > elsewhere) when we try to do an AUX transfer then we delay until HPD
> > > is there.
> >
> > I think panel-edp would already handle the delay, so we do not need to
> > have this logic in the DP driver.
>
> There's a whole discussion about this between Stephen and me in patch
> #5 ("drm/msm/dp: wait for hpd high before any sink interaction").
> Basically:
>
> * If panel HPD is hooked up to the dedicated HPD pin on the eDP
> controller then the panel driver doesn't have a way to read it.
I refreshed that dialog. I must admit, I have missed the fact that the
HPD pin might not be visible as the GPIO pin.
> * We can't leverage the existing "HPD" query functions in DRM because
> those indicate whether a panel is _physically_ connected. For eDP, it
> always is.
Yes, I was thinking about (mis)using the
drm_bridge_connector_hpd_notify() for generic HPD-related
notifications (to tell eDP that it should check the current state). I
have abandoned that idea.
> For now the rule is that the AUX transfer function is in charge of
> waiting for HPD for eDP if the dedicated HPD pin is used. If we want
> to re-invent this we could, but that system works, isn't _too_ ugly,
> and we're already making big enough changes in this series.
The is_hpd_asserted() looks like a good callback for the aux bus.
It will allow the panel driver to check if the panel is powered up (in
the absence of the GPIO pin).
> > > So we can still acquire resources (clocks, PHY, io maps, etc) at probe
> > > time for DP and create the AUX bus, right? It will just return
> > > "-ENODEV" if HPD isn't asserted and you're DP?
> >
> > Yes, please. I still suppose that we'd need a separate case to
> > power_on eDP's PHY during the probe time. Maybe I'm mistaken here.
>
> I think the ideal way is to do it like Kieran's proposal for sn65dsi86:
>
> https://lore.kernel.org/r/20220317131250.1481275-4-kieran.bingham+rene...@ideasonboard.com/
>
> * When enabling HPD (physical hot plug detect) in the hpd_enable()
> callback you do a pm_runtime_get(). You do the
> pm_runtime_put_autosuspend() when disabling. This is only used for DP
> since we only provide DRM_BRIDGE_OP_HPD for DP, not for eDP.
>
> * We do a pm_runtime_get() / pm_runtime_put_autosuspend() in the AUX
> transfer routine. While holding the pm_runtime reference we check HPD.
> For DP we return immediately if HPD isn't asserted. For eDP, we delay.
>
> * We do the pm_runtime_get() in pre_enable and the pm_runtime_put() in
> post_disable. For DP this will add a 2nd refcount (since we probably
> were holding the reference for HPD). For eDP this will cause us to
> power on.
>
> * If there's any other time we need to read HW registers, and we
> aren't guaranteed to already have a pm_runtime reference (like during
> probe), we can do a temporary pm_runtime_get() /
> pm_runtime_put_autosuspend().
This looks good. I'd be more than welcome to review such series.
Note: I think this would require using
drm_bridge_connector_enable_hpd() in the DP code.
Hopefully at some point we would be able to move all
drm_bridge_connector calls to the core msm layer.
--
With best wishes
Dmitry
Re: [PATCH v6 1/8] drm/msm/dp: Add eDP support via aux_bus
On Fri, 8 Apr 2022 at 16:56, Doug Anderson wrote: > > Hi, > > On Fri, Apr 8, 2022 at 5:13 AM Dmitry Baryshkov > wrote: > > > > On Fri, 8 Apr 2022 at 03:28, Doug Anderson wrote: > > > > > > Hi, > > > > > > On Thu, Apr 7, 2022 at 4:36 PM Dmitry Baryshkov > > > wrote: > > > > > > > > The ps8640 driver looks 'working by coincidence'. It calls > > > > dp_aux_populate, then immediately after the function returns it checks > > > > for the panel. If panel-edp is built as a module, the probe might fail > > > > easily. > > > > The anx7625 driver has the same kind of issue. The DP AUX bus is > > > > populated from the probe() and after some additional work the panel is > > > > being checked. > > > > This design is fragile and from my quick glance it can break (or be > > > > broken) too easy. It reminds me of our drm msm 'probe' loops > > > > preventing the device to boot completely if the dsi bridge/panel could > > > > not be probed in time. > > > > > > I did spend some time thinking about this, at least for ps8640. I > > > believe that as long as the panel's probe isn't asynchronous. > > > > By panel probe (as a probe of any device) is potentially asynchronous. > > As in your example, the PWM might not be present, the regulator probe > > might have been delayed, the panel-edp might be built as a module, > > which is not present for some reason. > > Well, in those cases it's not exactly asynchronous, or at least not in > the way I was thinking about. Either it will work now, or we should > try again later when more drivers have probed. The case I was worried > about was: > > 1. We call devm_of_dp_aux_populate_ep_devices() > > 2. devm_of_dp_aux_populate_ep_devices() kicks off a probe to the panel > in the background > > 3. devm_of_dp_aux_populate_ep_devices() returns to us without waiting > for the panel's probe to finish. > > I think that's possible if the panel driver sets PROBE_PREFER_ASYNCHRONOUS. That would be a separate story, yes. However the general case is still valid: one can not guarantee that the panel device is available immediately after aux bus population. So ps8640 works at this moment and in the particular configuration. > > I was less worried about the resources missing since I thought that > would be handled by the normal probe deferral case. IIRC the "infinite > loop" that we used to end up with MSM's probe was because something > about the way the MSM DRM driver worked would cause the deferral > mechanisms to retry instantly each time we deferred. I don't remember > exactly what triggered it, but I don't _think_ that's true for ps8640? I'm not sure either. If you have a system with that bridge, it can be easily tried by returning -EPROBE_DEFER from the panel driver's probe(). For the msm driver it was the following sequence of events: - mdss probes - mdss creates subdevices including dsi host - subdevices are probed - mdss drivers tries to perform component binding - dsi host determines that the next item is not available - it returns -EPROBE_DEFER to the component bind - mdss reverts registration of subdevices, returning probe defer. However as there were devices added to the device list, the deferral list was retried immediately. Thus we faced the probe loop. I think this looks close to the ps8640, but I wouldn't bet on that. > > > Basically if the panel isn't ready then ps8640 should return and we'll > > > retry later. I do remember the probe loops that we used to have with > > > msm and I don't _think_ this would trigger it. > > > > I don't have proof here. But as I wrote, this thing might break at some > > point. > > > > > That being said, if we need to separate out the AUX bus into a > > > sub-device like we did in sn65dsi86 we certainly could. > > > > Ideally we should separate the "bridge" subdevice, like it was done in > > sn65dsi86. > > So that the aux host probes, registers the EP device, then the bridge > > device can not be probed (and thus the drm_bridge is not created) > > until the next bridge becomes available. > > You're definitely right, that's best. I was hesitant to force the > issue during review of the ps8640 because it adds a bunch of > complexity and didn't _seem_ to be needed. Maybe it makes sense to > just code it up, though... As I wrote, the test is easy. If the system boots fine, there is no need to fix that immediately. However I think in general we should stop depending on the panel being available right after populating the aux bus. -- With best wishes Dmitry
Re: [PATCH v6 1/8] drm/msm/dp: Add eDP support via aux_bus
Hi, On Fri, Apr 8, 2022 at 5:13 AM Dmitry Baryshkov wrote: > > On Fri, 8 Apr 2022 at 03:28, Doug Anderson wrote: > > > > Hi, > > > > On Thu, Apr 7, 2022 at 4:36 PM Dmitry Baryshkov > > wrote: > > > > > > The ps8640 driver looks 'working by coincidence'. It calls > > > dp_aux_populate, then immediately after the function returns it checks > > > for the panel. If panel-edp is built as a module, the probe might fail > > > easily. > > > The anx7625 driver has the same kind of issue. The DP AUX bus is > > > populated from the probe() and after some additional work the panel is > > > being checked. > > > This design is fragile and from my quick glance it can break (or be > > > broken) too easy. It reminds me of our drm msm 'probe' loops > > > preventing the device to boot completely if the dsi bridge/panel could > > > not be probed in time. > > > > I did spend some time thinking about this, at least for ps8640. I > > believe that as long as the panel's probe isn't asynchronous. > > By panel probe (as a probe of any device) is potentially asynchronous. > As in your example, the PWM might not be present, the regulator probe > might have been delayed, the panel-edp might be built as a module, > which is not present for some reason. Well, in those cases it's not exactly asynchronous, or at least not in the way I was thinking about. Either it will work now, or we should try again later when more drivers have probed. The case I was worried about was: 1. We call devm_of_dp_aux_populate_ep_devices() 2. devm_of_dp_aux_populate_ep_devices() kicks off a probe to the panel in the background 3. devm_of_dp_aux_populate_ep_devices() returns to us without waiting for the panel's probe to finish. I think that's possible if the panel driver sets PROBE_PREFER_ASYNCHRONOUS. I was less worried about the resources missing since I thought that would be handled by the normal probe deferral case. IIRC the "infinite loop" that we used to end up with MSM's probe was because something about the way the MSM DRM driver worked would cause the deferral mechanisms to retry instantly each time we deferred. I don't remember exactly what triggered it, but I don't _think_ that's true for ps8640? > > Basically if the panel isn't ready then ps8640 should return and we'll > > retry later. I do remember the probe loops that we used to have with > > msm and I don't _think_ this would trigger it. > > I don't have proof here. But as I wrote, this thing might break at some point. > > > That being said, if we need to separate out the AUX bus into a > > sub-device like we did in sn65dsi86 we certainly could. > > Ideally we should separate the "bridge" subdevice, like it was done in > sn65dsi86. > So that the aux host probes, registers the EP device, then the bridge > device can not be probed (and thus the drm_bridge is not created) > until the next bridge becomes available. You're definitely right, that's best. I was hesitant to force the issue during review of the ps8640 because it adds a bunch of complexity and didn't _seem_ to be needed. Maybe it makes sense to just code it up, though... -Doug
Re: [PATCH v6 1/8] drm/msm/dp: Add eDP support via aux_bus
Hi,
On Fri, Apr 8, 2022 at 5:20 AM Dmitry Baryshkov
wrote:
>
> > I guess my thought was that in DP you could still create the AUX bus
> > at probe time. Then for DP you just return an instant "transfer
> > failed" from the AUX bus if HPD isn't asserted. For eDP (as discussed
> > elsewhere) when we try to do an AUX transfer then we delay until HPD
> > is there.
>
> I think panel-edp would already handle the delay, so we do not need to
> have this logic in the DP driver.
There's a whole discussion about this between Stephen and me in patch
#5 ("drm/msm/dp: wait for hpd high before any sink interaction").
Basically:
* If panel HPD is hooked up to the dedicated HPD pin on the eDP
controller then the panel driver doesn't have a way to read it.
* We can't leverage the existing "HPD" query functions in DRM because
those indicate whether a panel is _physically_ connected. For eDP, it
always is.
For now the rule is that the AUX transfer function is in charge of
waiting for HPD for eDP if the dedicated HPD pin is used. If we want
to re-invent this we could, but that system works, isn't _too_ ugly,
and we're already making big enough changes in this series.
> > So we can still acquire resources (clocks, PHY, io maps, etc) at probe
> > time for DP and create the AUX bus, right? It will just return
> > "-ENODEV" if HPD isn't asserted and you're DP?
>
> Yes, please. I still suppose that we'd need a separate case to
> power_on eDP's PHY during the probe time. Maybe I'm mistaken here.
I think the ideal way is to do it like Kieran's proposal for sn65dsi86:
https://lore.kernel.org/r/20220317131250.1481275-4-kieran.bingham+rene...@ideasonboard.com/
* When enabling HPD (physical hot plug detect) in the hpd_enable()
callback you do a pm_runtime_get(). You do the
pm_runtime_put_autosuspend() when disabling. This is only used for DP
since we only provide DRM_BRIDGE_OP_HPD for DP, not for eDP.
* We do a pm_runtime_get() / pm_runtime_put_autosuspend() in the AUX
transfer routine. While holding the pm_runtime reference we check HPD.
For DP we return immediately if HPD isn't asserted. For eDP, we delay.
* We do the pm_runtime_get() in pre_enable and the pm_runtime_put() in
post_disable. For DP this will add a 2nd refcount (since we probably
were holding the reference for HPD). For eDP this will cause us to
power on.
* If there's any other time we need to read HW registers, and we
aren't guaranteed to already have a pm_runtime reference (like during
probe), we can do a temporary pm_runtime_get() /
pm_runtime_put_autosuspend().
Re: [PATCH v6 1/8] drm/msm/dp: Add eDP support via aux_bus
On Fri, 8 Apr 2022 at 03:26, Doug Anderson wrote: > > Hi, > > On Thu, Apr 7, 2022 at 4:46 PM Dmitry Baryshkov > wrote: > > > > > The way I'm arguing it should work is that: > > > > > > 1. A whole bunch of the DP init code should move to the DP driver's > > > probe function. This includes parsing the DT, acquiring clocks, > > > getting a handle to our PHY, and IO mapping registers. As far as I > > > know, there's no reason to wait on all the components being probed in > > > order to do this stuff. > > > > Yes. And that's one of the reasons I tried to stay away from the DP > > driver. Each time I open the source code, my hands itch to start > > refactoring the code. > > > > > > > > 2. Once we have done the above things, it should be possible to do AUX > > > transfers, correct? ...and then we can populate the AUX bus from the > > > probe function too. > > > > No. In the DP case the AUX bus is inaccessible until the dongle is > > plugged (see all the HPD handling, phy_init()/phy_power_on() is hidden > > somewhere in that path) > > I guess my thought was that in DP you could still create the AUX bus > at probe time. Then for DP you just return an instant "transfer > failed" from the AUX bus if HPD isn't asserted. For eDP (as discussed > elsewhere) when we try to do an AUX transfer then we delay until HPD > is there. I think panel-edp would already handle the delay, so we do not need to have this logic in the DP driver. > So we can still acquire resources (clocks, PHY, io maps, etc) at probe > time for DP and create the AUX bus, right? It will just return > "-ENODEV" if HPD isn't asserted and you're DP? Yes, please. I still suppose that we'd need a separate case to power_on eDP's PHY during the probe time. Maybe I'm mistaken here. -- With best wishes Dmitry
Re: [PATCH v6 1/8] drm/msm/dp: Add eDP support via aux_bus
On Fri, 8 Apr 2022 at 03:28, Doug Anderson wrote: > > Hi, > > On Thu, Apr 7, 2022 at 4:36 PM Dmitry Baryshkov > wrote: > > > > The ps8640 driver looks 'working by coincidence'. It calls > > dp_aux_populate, then immediately after the function returns it checks > > for the panel. If panel-edp is built as a module, the probe might fail > > easily. > > The anx7625 driver has the same kind of issue. The DP AUX bus is > > populated from the probe() and after some additional work the panel is > > being checked. > > This design is fragile and from my quick glance it can break (or be > > broken) too easy. It reminds me of our drm msm 'probe' loops > > preventing the device to boot completely if the dsi bridge/panel could > > not be probed in time. > > I did spend some time thinking about this, at least for ps8640. I > believe that as long as the panel's probe isn't asynchronous. By panel probe (as a probe of any device) is potentially asynchronous. As in your example, the PWM might not be present, the regulator probe might have been delayed, the panel-edp might be built as a module, which is not present for some reason. > Basically if the panel isn't ready then ps8640 should return and we'll > retry later. I do remember the probe loops that we used to have with > msm and I don't _think_ this would trigger it. I don't have proof here. But as I wrote, this thing might break at some point. > That being said, if we need to separate out the AUX bus into a > sub-device like we did in sn65dsi86 we certainly could. Ideally we should separate the "bridge" subdevice, like it was done in sn65dsi86. So that the aux host probes, registers the EP device, then the bridge device can not be probed (and thus the drm_bridge is not created) until the next bridge becomes available. -- With best wishes Dmitry
Re: [PATCH v6 1/8] drm/msm/dp: Add eDP support via aux_bus
Hi, On Thu, Apr 7, 2022 at 4:46 PM Dmitry Baryshkov wrote: > > > The way I'm arguing it should work is that: > > > > 1. A whole bunch of the DP init code should move to the DP driver's > > probe function. This includes parsing the DT, acquiring clocks, > > getting a handle to our PHY, and IO mapping registers. As far as I > > know, there's no reason to wait on all the components being probed in > > order to do this stuff. > > Yes. And that's one of the reasons I tried to stay away from the DP > driver. Each time I open the source code, my hands itch to start > refactoring the code. > > > > > 2. Once we have done the above things, it should be possible to do AUX > > transfers, correct? ...and then we can populate the AUX bus from the > > probe function too. > > No. In the DP case the AUX bus is inaccessible until the dongle is > plugged (see all the HPD handling, phy_init()/phy_power_on() is hidden > somewhere in that path) I guess my thought was that in DP you could still create the AUX bus at probe time. Then for DP you just return an instant "transfer failed" from the AUX bus if HPD isn't asserted. For eDP (as discussed elsewhere) when we try to do an AUX transfer then we delay until HPD is there. So we can still acquire resources (clocks, PHY, io maps, etc) at probe time for DP and create the AUX bus, right? It will just return "-ENODEV" if HPD isn't asserted and you're DP? -Doug
Re: [PATCH v6 1/8] drm/msm/dp: Add eDP support via aux_bus
Hi, On Thu, Apr 7, 2022 at 4:36 PM Dmitry Baryshkov wrote: > > The ps8640 driver looks 'working by coincidence'. It calls > dp_aux_populate, then immediately after the function returns it checks > for the panel. If panel-edp is built as a module, the probe might fail > easily. > The anx7625 driver has the same kind of issue. The DP AUX bus is > populated from the probe() and after some additional work the panel is > being checked. > This design is fragile and from my quick glance it can break (or be > broken) too easy. It reminds me of our drm msm 'probe' loops > preventing the device to boot completely if the dsi bridge/panel could > not be probed in time. I did spend some time thinking about this, at least for ps8640. I believe that as long as the panel's probe isn't asynchronous. Basically if the panel isn't ready then ps8640 should return and we'll retry later. I do remember the probe loops that we used to have with msm and I don't _think_ this would trigger it. That being said, if we need to separate out the AUX bus into a sub-device like we did in sn65dsi86 we certainly could. -Doug
Re: [PATCH v6 1/8] drm/msm/dp: Add eDP support via aux_bus
On Fri, 8 Apr 2022 at 02:35, Doug Anderson wrote:
>
> Hi,
>
> On Thu, Apr 7, 2022 at 3:03 PM Abhinav Kumar
> wrote:
> >
> > Hi Doug
> >
> > Thanks for the response, some comments below.
> >
> > Abhinav
> > On 4/7/2022 1:47 PM, Doug Anderson wrote:
> > > Hi,
> > >
> > > On Thu, Apr 7, 2022 at 1:11 PM Abhinav Kumar
> > > wrote:
> > >>
> > >> Hi Doug and Dmitry
> > >>
> > >> Sorry, but I caught up on this email just now.
> > >>
> > >> Some comments below.
> > >>
> > >> Thanks
> > >>
> > >> Abhinav
> > >> On 4/7/2022 10:07 AM, Doug Anderson wrote:
> > >>> Hi,
> > >>>
> > >>> On Thu, Apr 7, 2022 at 7:19 AM Sankeerth Billakanti (QUIC)
> > >>> wrote:
> >
> > Hi Dmitry and Doug,
> >
> > > Hi,
> > >
> > > On Tue, Apr 5, 2022 at 10:36 AM Dmitry Baryshkov
> > > wrote:
> > >>
> > >> On 05/04/2022 20:02, Doug Anderson wrote:
> > >>> Hi,
> > >>>
> > >>> On Tue, Apr 5, 2022 at 5:54 AM Dmitry Baryshkov
> > >>> wrote:
> > > 3. For DP and eDP HPD means something a little different.
> > > Essentially there are two concepts: a) is a display physically
> > > connected and b) is the display powered up and ready. For DP, the
> > > two are really tied together. From the kernel's point of view you
> > > never "power down" a DP display and you can't detect that it's
> > > physically connected until it's ready. Said another way, on you
> > > tie "is a display there" to the HPD line and the moment a display
> > > is there it's ready for you to do AUX transfers. For eDP, in the
> > > lowest power state of a display it _won't_ assert its "HPD"
> > > signal. However, it's still physically present. For eDP you simply
> > > have to _assume_ it's present without any actual proof since you
> > > can't get proof until you power it up. Thus for eDP, you report
> > > that the display is there as soon as we're asked. We can't _talk_
> > > to the display yet, though. So in get_modes() we need to be able
> > > to power the display on enough to talk over the AUX channel to it.
> > > As part of this, we wait for the signal named "HPD" which really
> > > means
> > > "panel finished powering on" in this context.
> > >
> > > NOTE: for aux transfer, we don't have the _display_ pipe and
> > > clocks running. We only have enough stuff running to do the AUX
> > > transfer.
> > > We're not clocking out pixels. We haven't fully powered on the
> > > display. The AUX transfer is designed to be something that can be
> > > done early _before_ you turn on the display.
> > >
> > >
> > > OK, so basically that was a longwinded way of saying: yes, we
> > > could avoid the AUX transfer in probe, but we can't wait all the
> > > way to enable. We have to be able to transfer in get_modes(). If
> > > you think that's helpful I think it'd be a pretty easy patch to
> > > write even if it would look a tad bit awkward IMO. Let me know if
> > > you want me to post it up.
> >
> > I think it would be a good idea. At least it will allow us to
> > judge, which is the more correct way.
> > >>>
> > >>> I'm still happy to prototype this, but the more I think about it the
> > >>> more it feels like a workaround for the Qualcomm driver. The eDP
> > >>> panel driver is actually given a pointer to the AUX bus at probe
> > >>> time. It's really weird to say that we can't do a transfer on it
> > >>> yet... As you said, this is a little sideband bus. It should be able
> > >>> to be used without all the full blown infra of the rest of the
> > >>> driver.
> > >>
> > >> Yes, I have that feeling too. However I also have a feeling that just
> > >> powering up the PHY before the bus probe is ... a hack. There are no
> > >> obvious stopgaps for the driver not to power it down later.
> > >
> > >>
> > >> Lets go back to why we need to power up the PHY before the bus probe.
> > >>
> > >> We need to power up PHY before bus probe because panel-eDP tries to read
> > >> the EDID in probe() for the panel_id. Not get_modes().
> > >>
> > >> So doug, I didnt follow your comment that panel-eDP only does EDID read
> > >> in get_modes()
> > >>
> > >> panel_id = drm_edid_get_panel_id(panel->ddc);
> > >> if (!panel_id) {
> > >> dev_err(dev, "Couldn't identify panel via EDID\n");
> > >> ret = -EIO;
> > >> goto exit;
> > >> }
> > >>
> > >> If we do not need this part, we really dont need to power up the PHY
> > >> before the probe(). The hack which dmitry was referring to.
> > >
> > > Right. ...so we _could_ remove the above from the panel-edp probe and
> > > defer it to get_modes() and it wouldn't be that hard. ...but:
> > >
> > > 1. It feels
Re: [PATCH v6 1/8] drm/msm/dp: Add eDP support via aux_bus
On Thu, 7 Apr 2022 at 23:11, Abhinav Kumar wrote:
>
> Hi Doug and Dmitry
>
> Sorry, but I caught up on this email just now.
>
> Some comments below.
>
> Thanks
>
> Abhinav
> On 4/7/2022 10:07 AM, Doug Anderson wrote:
> > Hi,
> >
> > On Thu, Apr 7, 2022 at 7:19 AM Sankeerth Billakanti (QUIC)
> > wrote:
> >>
> >> Hi Dmitry and Doug,
> >>
> >>> Hi,
> >>>
> >>> On Tue, Apr 5, 2022 at 10:36 AM Dmitry Baryshkov
> >>> wrote:
>
> On 05/04/2022 20:02, Doug Anderson wrote:
> > Hi,
> >
> > On Tue, Apr 5, 2022 at 5:54 AM Dmitry Baryshkov
> > wrote:
> >>> 3. For DP and eDP HPD means something a little different.
> >>> Essentially there are two concepts: a) is a display physically
> >>> connected and b) is the display powered up and ready. For DP, the
> >>> two are really tied together. From the kernel's point of view you
> >>> never "power down" a DP display and you can't detect that it's
> >>> physically connected until it's ready. Said another way, on you
> >>> tie "is a display there" to the HPD line and the moment a display
> >>> is there it's ready for you to do AUX transfers. For eDP, in the
> >>> lowest power state of a display it _won't_ assert its "HPD"
> >>> signal. However, it's still physically present. For eDP you simply
> >>> have to _assume_ it's present without any actual proof since you
> >>> can't get proof until you power it up. Thus for eDP, you report
> >>> that the display is there as soon as we're asked. We can't _talk_
> >>> to the display yet, though. So in get_modes() we need to be able
> >>> to power the display on enough to talk over the AUX channel to it.
> >>> As part of this, we wait for the signal named "HPD" which really means
> >>> "panel finished powering on" in this context.
> >>>
> >>> NOTE: for aux transfer, we don't have the _display_ pipe and
> >>> clocks running. We only have enough stuff running to do the AUX
> >>> transfer.
> >>> We're not clocking out pixels. We haven't fully powered on the
> >>> display. The AUX transfer is designed to be something that can be
> >>> done early _before_ you turn on the display.
> >>>
> >>>
> >>> OK, so basically that was a longwinded way of saying: yes, we
> >>> could avoid the AUX transfer in probe, but we can't wait all the
> >>> way to enable. We have to be able to transfer in get_modes(). If
> >>> you think that's helpful I think it'd be a pretty easy patch to
> >>> write even if it would look a tad bit awkward IMO. Let me know if
> >>> you want me to post it up.
> >>
> >> I think it would be a good idea. At least it will allow us to
> >> judge, which is the more correct way.
> >
> > I'm still happy to prototype this, but the more I think about it the
> > more it feels like a workaround for the Qualcomm driver. The eDP
> > panel driver is actually given a pointer to the AUX bus at probe
> > time. It's really weird to say that we can't do a transfer on it
> > yet... As you said, this is a little sideband bus. It should be able
> > to be used without all the full blown infra of the rest of the driver.
>
> Yes, I have that feeling too. However I also have a feeling that just
> powering up the PHY before the bus probe is ... a hack. There are no
> obvious stopgaps for the driver not to power it down later.
> >>>
>
> Lets go back to why we need to power up the PHY before the bus probe.
>
> We need to power up PHY before bus probe because panel-eDP tries to read
> the EDID in probe() for the panel_id. Not get_modes().
>
> So doug, I didnt follow your comment that panel-eDP only does EDID read
> in get_modes()
>
> panel_id = drm_edid_get_panel_id(panel->ddc);
> if (!panel_id) {
> dev_err(dev, "Couldn't identify panel via EDID\n");
> ret = -EIO;
> goto exit;
> }
>
> If we do not need this part, we really dont need to power up the PHY
> before the probe(). The hack which dmitry was referring to.
>
> So this is boiling down to why or how panel-eDP was originally designed.
Well, it's not just panel-edp. It boils down to the DP-AUX bus design
vs DRM design. However in Doug's defense I should admit that I can not
come up with a significantly cleaner solution.
Just to emphasise (or to repeat): for all other display
panels/bridges, we either do not use a sidechannel bus or the
sidechannel bus (i2c, spi, platform) is managed outside of the DRM
framework. Thus it's possible to create the source in the drm's driver
probe path and then in the component's bind() path check (and return
an error) if the sink device wasn't yet probed successfully. The
source can then either communicate with the sink using the sidechannel
bus provided elsewhere or (e.g. in case of purely DSI panel), defer
communication till the moment the display path is fully available
(after encoder enablement).
Re: [PATCH v6 1/8] drm/msm/dp: Add eDP support via aux_bus
Hi,
On Thu, Apr 7, 2022 at 3:03 PM Abhinav Kumar wrote:
>
> Hi Doug
>
> Thanks for the response, some comments below.
>
> Abhinav
> On 4/7/2022 1:47 PM, Doug Anderson wrote:
> > Hi,
> >
> > On Thu, Apr 7, 2022 at 1:11 PM Abhinav Kumar
> > wrote:
> >>
> >> Hi Doug and Dmitry
> >>
> >> Sorry, but I caught up on this email just now.
> >>
> >> Some comments below.
> >>
> >> Thanks
> >>
> >> Abhinav
> >> On 4/7/2022 10:07 AM, Doug Anderson wrote:
> >>> Hi,
> >>>
> >>> On Thu, Apr 7, 2022 at 7:19 AM Sankeerth Billakanti (QUIC)
> >>> wrote:
>
> Hi Dmitry and Doug,
>
> > Hi,
> >
> > On Tue, Apr 5, 2022 at 10:36 AM Dmitry Baryshkov
> > wrote:
> >>
> >> On 05/04/2022 20:02, Doug Anderson wrote:
> >>> Hi,
> >>>
> >>> On Tue, Apr 5, 2022 at 5:54 AM Dmitry Baryshkov
> >>> wrote:
> > 3. For DP and eDP HPD means something a little different.
> > Essentially there are two concepts: a) is a display physically
> > connected and b) is the display powered up and ready. For DP, the
> > two are really tied together. From the kernel's point of view you
> > never "power down" a DP display and you can't detect that it's
> > physically connected until it's ready. Said another way, on you
> > tie "is a display there" to the HPD line and the moment a display
> > is there it's ready for you to do AUX transfers. For eDP, in the
> > lowest power state of a display it _won't_ assert its "HPD"
> > signal. However, it's still physically present. For eDP you simply
> > have to _assume_ it's present without any actual proof since you
> > can't get proof until you power it up. Thus for eDP, you report
> > that the display is there as soon as we're asked. We can't _talk_
> > to the display yet, though. So in get_modes() we need to be able
> > to power the display on enough to talk over the AUX channel to it.
> > As part of this, we wait for the signal named "HPD" which really
> > means
> > "panel finished powering on" in this context.
> >
> > NOTE: for aux transfer, we don't have the _display_ pipe and
> > clocks running. We only have enough stuff running to do the AUX
> > transfer.
> > We're not clocking out pixels. We haven't fully powered on the
> > display. The AUX transfer is designed to be something that can be
> > done early _before_ you turn on the display.
> >
> >
> > OK, so basically that was a longwinded way of saying: yes, we
> > could avoid the AUX transfer in probe, but we can't wait all the
> > way to enable. We have to be able to transfer in get_modes(). If
> > you think that's helpful I think it'd be a pretty easy patch to
> > write even if it would look a tad bit awkward IMO. Let me know if
> > you want me to post it up.
>
> I think it would be a good idea. At least it will allow us to
> judge, which is the more correct way.
> >>>
> >>> I'm still happy to prototype this, but the more I think about it the
> >>> more it feels like a workaround for the Qualcomm driver. The eDP
> >>> panel driver is actually given a pointer to the AUX bus at probe
> >>> time. It's really weird to say that we can't do a transfer on it
> >>> yet... As you said, this is a little sideband bus. It should be able
> >>> to be used without all the full blown infra of the rest of the driver.
> >>
> >> Yes, I have that feeling too. However I also have a feeling that just
> >> powering up the PHY before the bus probe is ... a hack. There are no
> >> obvious stopgaps for the driver not to power it down later.
> >
> >>
> >> Lets go back to why we need to power up the PHY before the bus probe.
> >>
> >> We need to power up PHY before bus probe because panel-eDP tries to read
> >> the EDID in probe() for the panel_id. Not get_modes().
> >>
> >> So doug, I didnt follow your comment that panel-eDP only does EDID read
> >> in get_modes()
> >>
> >> panel_id = drm_edid_get_panel_id(panel->ddc);
> >> if (!panel_id) {
> >> dev_err(dev, "Couldn't identify panel via EDID\n");
> >> ret = -EIO;
> >> goto exit;
> >> }
> >>
> >> If we do not need this part, we really dont need to power up the PHY
> >> before the probe(). The hack which dmitry was referring to.
> >
> > Right. ...so we _could_ remove the above from the panel-edp probe and
> > defer it to get_modes() and it wouldn't be that hard. ...but:
> >
> > 1. It feels like a hack to work around the Qualcomm driver. The way
> > the AUX bus is designed is that a pointer to the AUX bus is passed to
> > the panel-edp probe. It seems kinda strange to say that the panel
> > isn't allowed to do transfers with the pointer that's passed in.
> >
>
> And thats why to
Re: [PATCH v6 1/8] drm/msm/dp: Add eDP support via aux_bus
Hi Doug
Thanks for the response, some comments below.
Abhinav
On 4/7/2022 1:47 PM, Doug Anderson wrote:
Hi,
On Thu, Apr 7, 2022 at 1:11 PM Abhinav Kumar wrote:
Hi Doug and Dmitry
Sorry, but I caught up on this email just now.
Some comments below.
Thanks
Abhinav
On 4/7/2022 10:07 AM, Doug Anderson wrote:
Hi,
On Thu, Apr 7, 2022 at 7:19 AM Sankeerth Billakanti (QUIC)
wrote:
Hi Dmitry and Doug,
Hi,
On Tue, Apr 5, 2022 at 10:36 AM Dmitry Baryshkov
wrote:
On 05/04/2022 20:02, Doug Anderson wrote:
Hi,
On Tue, Apr 5, 2022 at 5:54 AM Dmitry Baryshkov
wrote:
3. For DP and eDP HPD means something a little different.
Essentially there are two concepts: a) is a display physically
connected and b) is the display powered up and ready. For DP, the
two are really tied together. From the kernel's point of view you
never "power down" a DP display and you can't detect that it's
physically connected until it's ready. Said another way, on you
tie "is a display there" to the HPD line and the moment a display
is there it's ready for you to do AUX transfers. For eDP, in the
lowest power state of a display it _won't_ assert its "HPD"
signal. However, it's still physically present. For eDP you simply
have to _assume_ it's present without any actual proof since you
can't get proof until you power it up. Thus for eDP, you report
that the display is there as soon as we're asked. We can't _talk_
to the display yet, though. So in get_modes() we need to be able
to power the display on enough to talk over the AUX channel to it.
As part of this, we wait for the signal named "HPD" which really means
"panel finished powering on" in this context.
NOTE: for aux transfer, we don't have the _display_ pipe and
clocks running. We only have enough stuff running to do the AUX
transfer.
We're not clocking out pixels. We haven't fully powered on the
display. The AUX transfer is designed to be something that can be
done early _before_ you turn on the display.
OK, so basically that was a longwinded way of saying: yes, we
could avoid the AUX transfer in probe, but we can't wait all the
way to enable. We have to be able to transfer in get_modes(). If
you think that's helpful I think it'd be a pretty easy patch to
write even if it would look a tad bit awkward IMO. Let me know if
you want me to post it up.
I think it would be a good idea. At least it will allow us to
judge, which is the more correct way.
I'm still happy to prototype this, but the more I think about it the
more it feels like a workaround for the Qualcomm driver. The eDP
panel driver is actually given a pointer to the AUX bus at probe
time. It's really weird to say that we can't do a transfer on it
yet... As you said, this is a little sideband bus. It should be able
to be used without all the full blown infra of the rest of the driver.
Yes, I have that feeling too. However I also have a feeling that just
powering up the PHY before the bus probe is ... a hack. There are no
obvious stopgaps for the driver not to power it down later.
Lets go back to why we need to power up the PHY before the bus probe.
We need to power up PHY before bus probe because panel-eDP tries to read
the EDID in probe() for the panel_id. Not get_modes().
So doug, I didnt follow your comment that panel-eDP only does EDID read
in get_modes()
panel_id = drm_edid_get_panel_id(panel->ddc);
if (!panel_id) {
dev_err(dev, "Couldn't identify panel via EDID\n");
ret = -EIO;
goto exit;
}
If we do not need this part, we really dont need to power up the PHY
before the probe(). The hack which dmitry was referring to.
Right. ...so we _could_ remove the above from the panel-edp probe and
defer it to get_modes() and it wouldn't be that hard. ...but:
1. It feels like a hack to work around the Qualcomm driver. The way
the AUX bus is designed is that a pointer to the AUX bus is passed to
the panel-edp probe. It seems kinda strange to say that the panel
isn't allowed to do transfers with the pointer that's passed in.
And thats why to satisfy the requirements of passing an initialized AUX,
sankeerth is delaying devm_of_dp_aux_populate_ep_devices() till PHY is
initialized which seems reasonable to satisfy the probe() time requirements.
Even if we move to pm_runtime(), yes I agree it will club all the
resources needed to control AUX in one place but you will still have to
initialize PHY before probe() under the hood of pm_runtime().
So how will it help this cause?
We just have to accept that initializing PHY is a requirement to use AUX
and before calling panel-eDP's probe(), we have to have an initialized AUX.
So we are not working around the driver but just satisfying the hardware
requirements to be able to satisfy panel-edp's and
drm_panel_dp_aux_backlight()'s aux bus requirements.
2. There's a second place where we might do an AUX transfer at probe
time which is when we're using the DP AUX
Re: [PATCH v6 1/8] drm/msm/dp: Add eDP support via aux_bus
Hi,
On Thu, Apr 7, 2022 at 1:11 PM Abhinav Kumar wrote:
>
> Hi Doug and Dmitry
>
> Sorry, but I caught up on this email just now.
>
> Some comments below.
>
> Thanks
>
> Abhinav
> On 4/7/2022 10:07 AM, Doug Anderson wrote:
> > Hi,
> >
> > On Thu, Apr 7, 2022 at 7:19 AM Sankeerth Billakanti (QUIC)
> > wrote:
> >>
> >> Hi Dmitry and Doug,
> >>
> >>> Hi,
> >>>
> >>> On Tue, Apr 5, 2022 at 10:36 AM Dmitry Baryshkov
> >>> wrote:
>
> On 05/04/2022 20:02, Doug Anderson wrote:
> > Hi,
> >
> > On Tue, Apr 5, 2022 at 5:54 AM Dmitry Baryshkov
> > wrote:
> >>> 3. For DP and eDP HPD means something a little different.
> >>> Essentially there are two concepts: a) is a display physically
> >>> connected and b) is the display powered up and ready. For DP, the
> >>> two are really tied together. From the kernel's point of view you
> >>> never "power down" a DP display and you can't detect that it's
> >>> physically connected until it's ready. Said another way, on you
> >>> tie "is a display there" to the HPD line and the moment a display
> >>> is there it's ready for you to do AUX transfers. For eDP, in the
> >>> lowest power state of a display it _won't_ assert its "HPD"
> >>> signal. However, it's still physically present. For eDP you simply
> >>> have to _assume_ it's present without any actual proof since you
> >>> can't get proof until you power it up. Thus for eDP, you report
> >>> that the display is there as soon as we're asked. We can't _talk_
> >>> to the display yet, though. So in get_modes() we need to be able
> >>> to power the display on enough to talk over the AUX channel to it.
> >>> As part of this, we wait for the signal named "HPD" which really means
> >>> "panel finished powering on" in this context.
> >>>
> >>> NOTE: for aux transfer, we don't have the _display_ pipe and
> >>> clocks running. We only have enough stuff running to do the AUX
> >>> transfer.
> >>> We're not clocking out pixels. We haven't fully powered on the
> >>> display. The AUX transfer is designed to be something that can be
> >>> done early _before_ you turn on the display.
> >>>
> >>>
> >>> OK, so basically that was a longwinded way of saying: yes, we
> >>> could avoid the AUX transfer in probe, but we can't wait all the
> >>> way to enable. We have to be able to transfer in get_modes(). If
> >>> you think that's helpful I think it'd be a pretty easy patch to
> >>> write even if it would look a tad bit awkward IMO. Let me know if
> >>> you want me to post it up.
> >>
> >> I think it would be a good idea. At least it will allow us to
> >> judge, which is the more correct way.
> >
> > I'm still happy to prototype this, but the more I think about it the
> > more it feels like a workaround for the Qualcomm driver. The eDP
> > panel driver is actually given a pointer to the AUX bus at probe
> > time. It's really weird to say that we can't do a transfer on it
> > yet... As you said, this is a little sideband bus. It should be able
> > to be used without all the full blown infra of the rest of the driver.
>
> Yes, I have that feeling too. However I also have a feeling that just
> powering up the PHY before the bus probe is ... a hack. There are no
> obvious stopgaps for the driver not to power it down later.
> >>>
>
> Lets go back to why we need to power up the PHY before the bus probe.
>
> We need to power up PHY before bus probe because panel-eDP tries to read
> the EDID in probe() for the panel_id. Not get_modes().
>
> So doug, I didnt follow your comment that panel-eDP only does EDID read
> in get_modes()
>
> panel_id = drm_edid_get_panel_id(panel->ddc);
> if (!panel_id) {
> dev_err(dev, "Couldn't identify panel via EDID\n");
> ret = -EIO;
> goto exit;
> }
>
> If we do not need this part, we really dont need to power up the PHY
> before the probe(). The hack which dmitry was referring to.
Right. ...so we _could_ remove the above from the panel-edp probe and
defer it to get_modes() and it wouldn't be that hard. ...but:
1. It feels like a hack to work around the Qualcomm driver. The way
the AUX bus is designed is that a pointer to the AUX bus is passed to
the panel-edp probe. It seems kinda strange to say that the panel
isn't allowed to do transfers with the pointer that's passed in.
2. There's a second place where we might do an AUX transfer at probe
time which is when we're using the DP AUX backlight. There we call
drm_panel_dp_aux_backlight(). Conceivably this too could be deferred
until the get_modes(), but now it feels even more like a hack. We're
going to be registering the backlight in the first call to
get_modes()? That's, ummm, unexpected. We could look at perhaps
breaking the "DP AUX backlight" in two parts also, but that gets
involved. I think
Re: [PATCH v6 1/8] drm/msm/dp: Add eDP support via aux_bus
Hi Doug and Dmitry
Sorry, but I caught up on this email just now.
Some comments below.
Thanks
Abhinav
On 4/7/2022 10:07 AM, Doug Anderson wrote:
Hi,
On Thu, Apr 7, 2022 at 7:19 AM Sankeerth Billakanti (QUIC)
wrote:
Hi Dmitry and Doug,
Hi,
On Tue, Apr 5, 2022 at 10:36 AM Dmitry Baryshkov
wrote:
On 05/04/2022 20:02, Doug Anderson wrote:
Hi,
On Tue, Apr 5, 2022 at 5:54 AM Dmitry Baryshkov
wrote:
3. For DP and eDP HPD means something a little different.
Essentially there are two concepts: a) is a display physically
connected and b) is the display powered up and ready. For DP, the
two are really tied together. From the kernel's point of view you
never "power down" a DP display and you can't detect that it's
physically connected until it's ready. Said another way, on you
tie "is a display there" to the HPD line and the moment a display
is there it's ready for you to do AUX transfers. For eDP, in the
lowest power state of a display it _won't_ assert its "HPD"
signal. However, it's still physically present. For eDP you simply
have to _assume_ it's present without any actual proof since you
can't get proof until you power it up. Thus for eDP, you report
that the display is there as soon as we're asked. We can't _talk_
to the display yet, though. So in get_modes() we need to be able
to power the display on enough to talk over the AUX channel to it.
As part of this, we wait for the signal named "HPD" which really means
"panel finished powering on" in this context.
NOTE: for aux transfer, we don't have the _display_ pipe and
clocks running. We only have enough stuff running to do the AUX
transfer.
We're not clocking out pixels. We haven't fully powered on the
display. The AUX transfer is designed to be something that can be
done early _before_ you turn on the display.
OK, so basically that was a longwinded way of saying: yes, we
could avoid the AUX transfer in probe, but we can't wait all the
way to enable. We have to be able to transfer in get_modes(). If
you think that's helpful I think it'd be a pretty easy patch to
write even if it would look a tad bit awkward IMO. Let me know if
you want me to post it up.
I think it would be a good idea. At least it will allow us to
judge, which is the more correct way.
I'm still happy to prototype this, but the more I think about it the
more it feels like a workaround for the Qualcomm driver. The eDP
panel driver is actually given a pointer to the AUX bus at probe
time. It's really weird to say that we can't do a transfer on it
yet... As you said, this is a little sideband bus. It should be able
to be used without all the full blown infra of the rest of the driver.
Yes, I have that feeling too. However I also have a feeling that just
powering up the PHY before the bus probe is ... a hack. There are no
obvious stopgaps for the driver not to power it down later.
Lets go back to why we need to power up the PHY before the bus probe.
We need to power up PHY before bus probe because panel-eDP tries to read
the EDID in probe() for the panel_id. Not get_modes().
So doug, I didnt follow your comment that panel-eDP only does EDID read
in get_modes()
panel_id = drm_edid_get_panel_id(panel->ddc);
if (!panel_id) {
dev_err(dev, "Couldn't identify panel via EDID\n");
ret = -EIO;
goto exit;
}
If we do not need this part, we really dont need to power up the PHY
before the probe(). The hack which dmitry was referring to.
So this is boiling down to why or how panel-eDP was originally designed.
This is why I think we need to move to Runtime PM to manage this. Basically:
1. When an AUX transfer happens, you grab a PM runtime reference that
_that_ powers up the PHY.
This will not be trivial and needs to be scoped out as sankeerth said
but if the above is the only concern, why do we need to do this? There
seems to be an explanation why we are doing this and its not a hack.
How would Dmitry's rework address this? We need some RFC to conclude on
that first.
2. At the end of the AUX transfer function, you do a "put_autosuspend".
Then it becomes not a hack, right?
pm runtime ops needs to be implemented for both eDP and DP. This change
take good amount of planning and code changes as it affects DP also.
Because this patch series consist of basic eDP changes for SC7280 bootup,
shall we take this pm_runtime implementation in subsequent patch series?
Dmitry is the real decision maker here, but in my opinion it would be
OK to get something landed first that worked OK and wasn't taking us
too far in the wrong direction and then we could get a follow up patch
to move to pm_runtime.
I would say the discussion changed into a direction of implementing
pm-runtime because the current patch series does what it takes to adhere
to panel-eDP's design along with aux bus requirements of PHY needing to
be on.
So doug, to answer your questions here:
"So I guess the net result
Re: [PATCH v6 1/8] drm/msm/dp: Add eDP support via aux_bus
Hi, On Thu, Apr 7, 2022 at 7:19 AM Sankeerth Billakanti (QUIC) wrote: > > Hi Dmitry and Doug, > > > Hi, > > > > On Tue, Apr 5, 2022 at 10:36 AM Dmitry Baryshkov > > wrote: > > > > > > On 05/04/2022 20:02, Doug Anderson wrote: > > > > Hi, > > > > > > > > On Tue, Apr 5, 2022 at 5:54 AM Dmitry Baryshkov > > > > wrote: > > > >>> 3. For DP and eDP HPD means something a little different. > > > >>> Essentially there are two concepts: a) is a display physically > > > >>> connected and b) is the display powered up and ready. For DP, the > > > >>> two are really tied together. From the kernel's point of view you > > > >>> never "power down" a DP display and you can't detect that it's > > > >>> physically connected until it's ready. Said another way, on you > > > >>> tie "is a display there" to the HPD line and the moment a display > > > >>> is there it's ready for you to do AUX transfers. For eDP, in the > > > >>> lowest power state of a display it _won't_ assert its "HPD" > > > >>> signal. However, it's still physically present. For eDP you simply > > > >>> have to _assume_ it's present without any actual proof since you > > > >>> can't get proof until you power it up. Thus for eDP, you report > > > >>> that the display is there as soon as we're asked. We can't _talk_ > > > >>> to the display yet, though. So in get_modes() we need to be able > > > >>> to power the display on enough to talk over the AUX channel to it. > > > >>> As part of this, we wait for the signal named "HPD" which really means > > "panel finished powering on" in this context. > > > >>> > > > >>> NOTE: for aux transfer, we don't have the _display_ pipe and > > > >>> clocks running. We only have enough stuff running to do the AUX > > transfer. > > > >>> We're not clocking out pixels. We haven't fully powered on the > > > >>> display. The AUX transfer is designed to be something that can be > > > >>> done early _before_ you turn on the display. > > > >>> > > > >>> > > > >>> OK, so basically that was a longwinded way of saying: yes, we > > > >>> could avoid the AUX transfer in probe, but we can't wait all the > > > >>> way to enable. We have to be able to transfer in get_modes(). If > > > >>> you think that's helpful I think it'd be a pretty easy patch to > > > >>> write even if it would look a tad bit awkward IMO. Let me know if > > > >>> you want me to post it up. > > > >> > > > >> I think it would be a good idea. At least it will allow us to > > > >> judge, which is the more correct way. > > > > > > > > I'm still happy to prototype this, but the more I think about it the > > > > more it feels like a workaround for the Qualcomm driver. The eDP > > > > panel driver is actually given a pointer to the AUX bus at probe > > > > time. It's really weird to say that we can't do a transfer on it > > > > yet... As you said, this is a little sideband bus. It should be able > > > > to be used without all the full blown infra of the rest of the driver. > > > > > > Yes, I have that feeling too. However I also have a feeling that just > > > powering up the PHY before the bus probe is ... a hack. There are no > > > obvious stopgaps for the driver not to power it down later. > > > > This is why I think we need to move to Runtime PM to manage this. Basically: > > > > 1. When an AUX transfer happens, you grab a PM runtime reference that > > _that_ powers up the PHY. > > > > 2. At the end of the AUX transfer function, you do a "put_autosuspend". > > > > Then it becomes not a hack, right? > > > > > > pm runtime ops needs to be implemented for both eDP and DP. This change > take good amount of planning and code changes as it affects DP also. > > Because this patch series consist of basic eDP changes for SC7280 bootup, > shall we take this pm_runtime implementation in subsequent patch series? Dmitry is the real decision maker here, but in my opinion it would be OK to get something landed first that worked OK and wasn't taking us too far in the wrong direction and then we could get a follow up patch to move to pm_runtime.
RE: [PATCH v6 1/8] drm/msm/dp: Add eDP support via aux_bus
Hi Dmitry and Doug, > Hi, > > On Tue, Apr 5, 2022 at 10:36 AM Dmitry Baryshkov > wrote: > > > > On 05/04/2022 20:02, Doug Anderson wrote: > > > Hi, > > > > > > On Tue, Apr 5, 2022 at 5:54 AM Dmitry Baryshkov > > > wrote: > > >>> 3. For DP and eDP HPD means something a little different. > > >>> Essentially there are two concepts: a) is a display physically > > >>> connected and b) is the display powered up and ready. For DP, the > > >>> two are really tied together. From the kernel's point of view you > > >>> never "power down" a DP display and you can't detect that it's > > >>> physically connected until it's ready. Said another way, on you > > >>> tie "is a display there" to the HPD line and the moment a display > > >>> is there it's ready for you to do AUX transfers. For eDP, in the > > >>> lowest power state of a display it _won't_ assert its "HPD" > > >>> signal. However, it's still physically present. For eDP you simply > > >>> have to _assume_ it's present without any actual proof since you > > >>> can't get proof until you power it up. Thus for eDP, you report > > >>> that the display is there as soon as we're asked. We can't _talk_ > > >>> to the display yet, though. So in get_modes() we need to be able > > >>> to power the display on enough to talk over the AUX channel to it. > > >>> As part of this, we wait for the signal named "HPD" which really means > "panel finished powering on" in this context. > > >>> > > >>> NOTE: for aux transfer, we don't have the _display_ pipe and > > >>> clocks running. We only have enough stuff running to do the AUX > transfer. > > >>> We're not clocking out pixels. We haven't fully powered on the > > >>> display. The AUX transfer is designed to be something that can be > > >>> done early _before_ you turn on the display. > > >>> > > >>> > > >>> OK, so basically that was a longwinded way of saying: yes, we > > >>> could avoid the AUX transfer in probe, but we can't wait all the > > >>> way to enable. We have to be able to transfer in get_modes(). If > > >>> you think that's helpful I think it'd be a pretty easy patch to > > >>> write even if it would look a tad bit awkward IMO. Let me know if > > >>> you want me to post it up. > > >> > > >> I think it would be a good idea. At least it will allow us to > > >> judge, which is the more correct way. > > > > > > I'm still happy to prototype this, but the more I think about it the > > > more it feels like a workaround for the Qualcomm driver. The eDP > > > panel driver is actually given a pointer to the AUX bus at probe > > > time. It's really weird to say that we can't do a transfer on it > > > yet... As you said, this is a little sideband bus. It should be able > > > to be used without all the full blown infra of the rest of the driver. > > > > Yes, I have that feeling too. However I also have a feeling that just > > powering up the PHY before the bus probe is ... a hack. There are no > > obvious stopgaps for the driver not to power it down later. > > This is why I think we need to move to Runtime PM to manage this. Basically: > > 1. When an AUX transfer happens, you grab a PM runtime reference that > _that_ powers up the PHY. > > 2. At the end of the AUX transfer function, you do a "put_autosuspend". > > Then it becomes not a hack, right? > > pm runtime ops needs to be implemented for both eDP and DP. This change take good amount of planning and code changes as it affects DP also. Because this patch series consist of basic eDP changes for SC7280 bootup, shall we take this pm_runtime implementation in subsequent patch series? > > A cleaner design might be to split all hotplug event handling from the > > dp_display, provide a lightweight state machine for the eDP and select > > which state machine to use depending on the hardware connector type. > > The dp_display_bind/unbind would probably also be duplicated and > > receive correct code flows for calling dp_parser_get_next_bridge, etc. > > Basically that means that depending on the device data we'd use either > > dp_display_comp_ops or (new) edp_comp_ops. > > > > WDYT? > > I don't think I know the structure of the MSM DP code to make a definitive > answer here. I think I'd have to see a patch. However I'd agree in general > terms that we need some different flows for the two. > ;-) We definitely want to limit the differences but some of them will be > unavoidable... > > > -Doug Thank you, Sankeerth
Re: [PATCH v6 1/8] drm/msm/dp: Add eDP support via aux_bus
Hi, On Tue, Apr 5, 2022 at 10:36 AM Dmitry Baryshkov wrote: > > On 05/04/2022 20:02, Doug Anderson wrote: > > Hi, > > > > On Tue, Apr 5, 2022 at 5:54 AM Dmitry Baryshkov > > wrote: > >>> 3. For DP and eDP HPD means something a little different. Essentially > >>> there are two concepts: a) is a display physically connected and b) is > >>> the display powered up and ready. For DP, the two are really tied > >>> together. From the kernel's point of view you never "power down" a DP > >>> display and you can't detect that it's physically connected until it's > >>> ready. Said another way, on you tie "is a display there" to the HPD > >>> line and the moment a display is there it's ready for you to do AUX > >>> transfers. For eDP, in the lowest power state of a display it _won't_ > >>> assert its "HPD" signal. However, it's still physically present. For > >>> eDP you simply have to _assume_ it's present without any actual proof > >>> since you can't get proof until you power it up. Thus for eDP, you > >>> report that the display is there as soon as we're asked. We can't > >>> _talk_ to the display yet, though. So in get_modes() we need to be > >>> able to power the display on enough to talk over the AUX channel to > >>> it. As part of this, we wait for the signal named "HPD" which really > >>> means "panel finished powering on" in this context. > >>> > >>> NOTE: for aux transfer, we don't have the _display_ pipe and clocks > >>> running. We only have enough stuff running to do the AUX transfer. > >>> We're not clocking out pixels. We haven't fully powered on the > >>> display. The AUX transfer is designed to be something that can be done > >>> early _before_ you turn on the display. > >>> > >>> > >>> OK, so basically that was a longwinded way of saying: yes, we could > >>> avoid the AUX transfer in probe, but we can't wait all the way to > >>> enable. We have to be able to transfer in get_modes(). If you think > >>> that's helpful I think it'd be a pretty easy patch to write even if it > >>> would look a tad bit awkward IMO. Let me know if you want me to post > >>> it up. > >> > >> I think it would be a good idea. At least it will allow us to judge, > >> which is the more correct way. > > > > I'm still happy to prototype this, but the more I think about it the > > more it feels like a workaround for the Qualcomm driver. The eDP panel > > driver is actually given a pointer to the AUX bus at probe time. It's > > really weird to say that we can't do a transfer on it yet... As you > > said, this is a little sideband bus. It should be able to be used > > without all the full blown infra of the rest of the driver. > > Yes, I have that feeling too. However I also have a feeling that just > powering up the PHY before the bus probe is ... a hack. There are no > obvious stopgaps for the driver not to power it down later. This is why I think we need to move to Runtime PM to manage this. Basically: 1. When an AUX transfer happens, you grab a PM runtime reference that _that_ powers up the PHY. 2. At the end of the AUX transfer function, you do a "put_autosuspend". Then it becomes not a hack, right? > A cleaner design might be to split all hotplug event handling from the > dp_display, provide a lightweight state machine for the eDP and select > which state machine to use depending on the hardware connector type. The > dp_display_bind/unbind would probably also be duplicated and receive > correct code flows for calling dp_parser_get_next_bridge, etc. > Basically that means that depending on the device data we'd use either > dp_display_comp_ops or (new) edp_comp_ops. > > WDYT? I don't think I know the structure of the MSM DP code to make a definitive answer here. I think I'd have to see a patch. However I'd agree in general terms that we need some different flows for the two. ;-) We definitely want to limit the differences but some of them will be unavoidable... -Doug
Re: [PATCH v6 1/8] drm/msm/dp: Add eDP support via aux_bus
On 05/04/2022 20:02, Doug Anderson wrote: Hi, On Tue, Apr 5, 2022 at 5:54 AM Dmitry Baryshkov wrote: 3. For DP and eDP HPD means something a little different. Essentially there are two concepts: a) is a display physically connected and b) is the display powered up and ready. For DP, the two are really tied together. From the kernel's point of view you never "power down" a DP display and you can't detect that it's physically connected until it's ready. Said another way, on you tie "is a display there" to the HPD line and the moment a display is there it's ready for you to do AUX transfers. For eDP, in the lowest power state of a display it _won't_ assert its "HPD" signal. However, it's still physically present. For eDP you simply have to _assume_ it's present without any actual proof since you can't get proof until you power it up. Thus for eDP, you report that the display is there as soon as we're asked. We can't _talk_ to the display yet, though. So in get_modes() we need to be able to power the display on enough to talk over the AUX channel to it. As part of this, we wait for the signal named "HPD" which really means "panel finished powering on" in this context. NOTE: for aux transfer, we don't have the _display_ pipe and clocks running. We only have enough stuff running to do the AUX transfer. We're not clocking out pixels. We haven't fully powered on the display. The AUX transfer is designed to be something that can be done early _before_ you turn on the display. OK, so basically that was a longwinded way of saying: yes, we could avoid the AUX transfer in probe, but we can't wait all the way to enable. We have to be able to transfer in get_modes(). If you think that's helpful I think it'd be a pretty easy patch to write even if it would look a tad bit awkward IMO. Let me know if you want me to post it up. I think it would be a good idea. At least it will allow us to judge, which is the more correct way. I'm still happy to prototype this, but the more I think about it the more it feels like a workaround for the Qualcomm driver. The eDP panel driver is actually given a pointer to the AUX bus at probe time. It's really weird to say that we can't do a transfer on it yet... As you said, this is a little sideband bus. It should be able to be used without all the full blown infra of the rest of the driver. Yes, I have that feeling too. However I also have a feeling that just powering up the PHY before the bus probe is ... a hack. There are no obvious stopgaps for the driver not to power it down later. A cleaner design might be to split all hotplug event handling from the dp_display, provide a lightweight state machine for the eDP and select which state machine to use depending on the hardware connector type. The dp_display_bind/unbind would probably also be duplicated and receive correct code flows for calling dp_parser_get_next_bridge, etc. Basically that means that depending on the device data we'd use either dp_display_comp_ops or (new) edp_comp_ops. WDYT? And I also think it might help the ti,sn65dsi86 driver, as it won't have to ensure that gpio is available during the AUX bus probe. The ti,sn65dsi86 GPIO issue has been solved for a while, though so not sure why we need to do something there? I'm also unclear how it would have helped. In this discussion, we've agreed that the panel driver would still acquire resources during its probe time and the only thing that would be delayed would be the first AUX transfer. The GPIO is a resource here and it's ideal to acquire it at probe time so we could EPROBE_DEFER if needed. Ack. I agree here. The world at 5 a.m. looks differently :D BTW, another random idea, before you start coding. We have the bridge's hpd_notify call. Currently it is called only by the means of drm_bridge_connector's HPD mechanism, tied to the bridge registering as DRM_BRIDGE_OP_HPD. It looks to me like it might be a perfect fit for the first aux-bus related reads. We'd need to trigger it manually once and tie it to the new drm_panel_funcs callback, which in turn would probe the aux bus, create backlight, etc. Regarding the Sankeerth's patch. I have been comparing it with the hpd_event_thread()'s calls. It looks to me like we should reuse dp_display_config_hpd() /EV_HPD_INIT_SETUP and maybe others. What I'm trying to say is that if we split AUX probing and first AUX transfers, it would be possible to reuse a significant part of MSM DP HPD machine rather than hacking around it and replicating it manually. I'm not sure I completely understand, but I'm pretty wary here. It's my assertion that all of the current "HPD" infrastructure in DRM all relates to the physical presence of the panel. If you start implementing these functions for eDP I think you're going to confuse the heck out of everything. The kernel will think that this is a display that's sometimes not there. Whenever the display is powered off then HPD will be low and it will look like there's
Re: [PATCH v6 1/8] drm/msm/dp: Add eDP support via aux_bus
Hi, On Tue, Apr 5, 2022 at 5:54 AM Dmitry Baryshkov wrote: > > 3. For DP and eDP HPD means something a little different. Essentially > > there are two concepts: a) is a display physically connected and b) is > > the display powered up and ready. For DP, the two are really tied > > together. From the kernel's point of view you never "power down" a DP > > display and you can't detect that it's physically connected until it's > > ready. Said another way, on you tie "is a display there" to the HPD > > line and the moment a display is there it's ready for you to do AUX > > transfers. For eDP, in the lowest power state of a display it _won't_ > > assert its "HPD" signal. However, it's still physically present. For > > eDP you simply have to _assume_ it's present without any actual proof > > since you can't get proof until you power it up. Thus for eDP, you > > report that the display is there as soon as we're asked. We can't > > _talk_ to the display yet, though. So in get_modes() we need to be > > able to power the display on enough to talk over the AUX channel to > > it. As part of this, we wait for the signal named "HPD" which really > > means "panel finished powering on" in this context. > > > > NOTE: for aux transfer, we don't have the _display_ pipe and clocks > > running. We only have enough stuff running to do the AUX transfer. > > We're not clocking out pixels. We haven't fully powered on the > > display. The AUX transfer is designed to be something that can be done > > early _before_ you turn on the display. > > > > > > OK, so basically that was a longwinded way of saying: yes, we could > > avoid the AUX transfer in probe, but we can't wait all the way to > > enable. We have to be able to transfer in get_modes(). If you think > > that's helpful I think it'd be a pretty easy patch to write even if it > > would look a tad bit awkward IMO. Let me know if you want me to post > > it up. > > I think it would be a good idea. At least it will allow us to judge, > which is the more correct way. I'm still happy to prototype this, but the more I think about it the more it feels like a workaround for the Qualcomm driver. The eDP panel driver is actually given a pointer to the AUX bus at probe time. It's really weird to say that we can't do a transfer on it yet... As you said, this is a little sideband bus. It should be able to be used without all the full blown infra of the rest of the driver. > And I also think it might help the ti,sn65dsi86 driver, as it won't > have to ensure that gpio is available during the AUX bus probe. The ti,sn65dsi86 GPIO issue has been solved for a while, though so not sure why we need to do something there? I'm also unclear how it would have helped. In this discussion, we've agreed that the panel driver would still acquire resources during its probe time and the only thing that would be delayed would be the first AUX transfer. The GPIO is a resource here and it's ideal to acquire it at probe time so we could EPROBE_DEFER if needed. > BTW, another random idea, before you start coding. > > We have the bridge's hpd_notify call. Currently it is called only by > the means of drm_bridge_connector's HPD mechanism, tied to the bridge > registering as DRM_BRIDGE_OP_HPD. > It looks to me like it might be a perfect fit for the first aux-bus > related reads. > > We'd need to trigger it manually once and tie it to the new > drm_panel_funcs callback, which in turn would probe the aux bus, > create backlight, etc. > > Regarding the Sankeerth's patch. I have been comparing it with the > hpd_event_thread()'s calls. > It looks to me like we should reuse dp_display_config_hpd() > /EV_HPD_INIT_SETUP and maybe others. > > What I'm trying to say is that if we split AUX probing and first AUX > transfers, it would be possible to reuse a significant part of MSM DP > HPD machine rather than hacking around it and replicating it manually. I'm not sure I completely understand, but I'm pretty wary here. It's my assertion that all of the current "HPD" infrastructure in DRM all relates to the physical presence of the panel. If you start implementing these functions for eDP I think you're going to confuse the heck out of everything. The kernel will think that this is a display that's sometimes not there. Whenever the display is powered off then HPD will be low and it will look like there's no display. Nothing will ever try to power it on because it looks like there's no display. I think your idea is to "trigger once" at bootup and then it all magically works, right? ...but what about after bootup? If you turn the display off for whatever reason (modeset or you simply close the lid of your laptop because you're using an external display) and then you want to use the eDP display again, how do you kickstart the process another time? You can't reboot, and when the display is off the HPD line is low. I can't say it enough times, HPD on eDP _does not mean hot plug detect_. The panel is always there. HPD is
Re: [PATCH v6 1/8] drm/msm/dp: Add eDP support via aux_bus
On Mon, 4 Apr 2022 at 23:53, Doug Anderson wrote: > > Hi, > > On Sat, Apr 2, 2022 at 1:26 PM Dmitry Baryshkov > wrote: > > > > On Sat, 2 Apr 2022 at 20:06, Doug Anderson wrote: > > > > > > Hi, > > > > > > On Sat, Apr 2, 2022 at 3:37 AM Dmitry Baryshkov > > > wrote: > > > > > > > > On 01/04/2022 02:22, Doug Anderson wrote: > > > > > Hi, > > > > > > > > > > On Wed, Mar 30, 2022 at 9:03 AM Sankeerth Billakanti > > > > > wrote: > > > > >> > > > > >> @@ -1547,6 +1593,10 @@ int msm_dp_modeset_init(struct msm_dp > > > > >> *dp_display, struct drm_device *dev, > > > > >> > > > > >> dp_display->encoder = encoder; > > > > >> > > > > >> + ret = dp_display_get_next_bridge(dp_display); > > > > >> + if (ret) > > > > >> + return ret; > > > > > > > > > > It feels weird to me that this is in a function called "modeset_init", > > > > > though I certainly don't know the structure of the MSM display code > > > > > well enough to fully comment. > > > > > > > > It's called modeset_init() as it initializes KMS objects used by DP > > > > driver. We have similar functions for dsi and hdmi > > > > > > Sorry, I wasn't meaning to imply that modeset_init() was a bad name or > > > anything. Mostly saying that I wasn't sure that modeset init was the > > > proper time to populate the aux bus. ...but then again, perhaps it is > > > given the current structure of this driver? > > > > > > > > > > > My expectation would have been that > > > > > devm_of_dp_aux_populate_ep_devices() would have been called from your > > > > > probe routine and then you would have returned -EPROBE_DEFER from your > > > > > probe if you were unable to find the panel afterwards. > > > > > > > > I don't think it's possible to call it from probe() since > > > > drm_dp_aux_register() is called only from dp_display_bind(). > > > > The PHY also isn't initialized at that moment, so we can not probe AUX > > > > devices. > > > > > > > > The overall semantics of the AUX bus is not clear to me. > > > > Typically the bus is populated (and probed) when devices are accessible. > > > > But for the display related buses this might not be the case. > > > > > > In general the AUX bus is modeled much like the i2c bus. You probe the > > > sub-device when you're able to transfer. Then you can confirm that the > > > device is actually there and init the device. > > > > > > > > > > For example for the DSI bus we clearly define that DSI transfer are not > > > > possible before the corresponding bridge's (or panel's) enable call. > > > > > > > > Maybe the same approach should be adopted for the AUX bus. This would > > > > allow us to populate the AUX bus before hardware access is actually > > > > possible, thus creating all the DRM bridges before the hardware is > > > > actually up and running. > > > > > > So I guess what you're proposing is that you could probe the devices > > > under the AUX bus and they could acquire resources (and possibly > > > return EPROBE_DEFER) at a point in time _before_ it's actually > > > possible to transfer. Then I guess you'd later do the transfer? > > > > Exactly. > > > > > > > > I guess conceivably one could re-design the DRM subsystem like that, > > > but I don't think it's trivial. > > > > The problem is that the DRM subsystem is already designed like that. > > All the bridge chains are static. They are created during the device > > probe. And the modes are populated later (via the get_modes() > > callback), after the HPD signal is delivered. > > For the encoder/bridge chains it is explicitly stated that the display > > pipe (clocks and timing signals) are not running before bridge's > > enable() callback or after the disable() callback being called. > > > > > Why? I believe that we need to know > > > things about the panel at probe time. For instance, we need to be able > > > to populate the panel's modes. > > > > As I said, panel modes are not needed at the probe time. The fact that > > most (if not all) of the panel drivers provide them in the platform > > data (and thus modes are typically populated at the probe time) comes > > from the fact that the panel is usually a known static piece of > > hardware. With the generic edp-panel this is no longer the case. A > > single device handles a (probed) variety of panels. > > OK, so I misspoke when I said that the modes are populated during > probe time for edp-panel. They're not and I guess I managed to confuse > myself when I wrote my previous email. Instead they (and everything > else that comes from the EDID) isn't needed until the panel's > get_modes() is called, as you said. So, ignoring the backlight problem > talked about below, we could conceivably delay the reading of the EDID > until get_modes. Yes. > > ...but I think something still isn't quite right with your description > of how it works. Specifically: > > 1. I'm 99% certain that get_modes() is called _before_ enable, even > for the DP case. I have no idea how that works for you for DP if the >
Re: [PATCH v6 1/8] drm/msm/dp: Add eDP support via aux_bus
Hi, On Sat, Apr 2, 2022 at 1:26 PM Dmitry Baryshkov wrote: > > On Sat, 2 Apr 2022 at 20:06, Doug Anderson wrote: > > > > Hi, > > > > On Sat, Apr 2, 2022 at 3:37 AM Dmitry Baryshkov > > wrote: > > > > > > On 01/04/2022 02:22, Doug Anderson wrote: > > > > Hi, > > > > > > > > On Wed, Mar 30, 2022 at 9:03 AM Sankeerth Billakanti > > > > wrote: > > > >> > > > >> @@ -1547,6 +1593,10 @@ int msm_dp_modeset_init(struct msm_dp > > > >> *dp_display, struct drm_device *dev, > > > >> > > > >> dp_display->encoder = encoder; > > > >> > > > >> + ret = dp_display_get_next_bridge(dp_display); > > > >> + if (ret) > > > >> + return ret; > > > > > > > > It feels weird to me that this is in a function called "modeset_init", > > > > though I certainly don't know the structure of the MSM display code > > > > well enough to fully comment. > > > > > > It's called modeset_init() as it initializes KMS objects used by DP > > > driver. We have similar functions for dsi and hdmi > > > > Sorry, I wasn't meaning to imply that modeset_init() was a bad name or > > anything. Mostly saying that I wasn't sure that modeset init was the > > proper time to populate the aux bus. ...but then again, perhaps it is > > given the current structure of this driver? > > > > > > > > My expectation would have been that > > > > devm_of_dp_aux_populate_ep_devices() would have been called from your > > > > probe routine and then you would have returned -EPROBE_DEFER from your > > > > probe if you were unable to find the panel afterwards. > > > > > > I don't think it's possible to call it from probe() since > > > drm_dp_aux_register() is called only from dp_display_bind(). > > > The PHY also isn't initialized at that moment, so we can not probe AUX > > > devices. > > > > > > The overall semantics of the AUX bus is not clear to me. > > > Typically the bus is populated (and probed) when devices are accessible. > > > But for the display related buses this might not be the case. > > > > In general the AUX bus is modeled much like the i2c bus. You probe the > > sub-device when you're able to transfer. Then you can confirm that the > > device is actually there and init the device. > > > > > > > For example for the DSI bus we clearly define that DSI transfer are not > > > possible before the corresponding bridge's (or panel's) enable call. > > > > > > Maybe the same approach should be adopted for the AUX bus. This would > > > allow us to populate the AUX bus before hardware access is actually > > > possible, thus creating all the DRM bridges before the hardware is > > > actually up and running. > > > > So I guess what you're proposing is that you could probe the devices > > under the AUX bus and they could acquire resources (and possibly > > return EPROBE_DEFER) at a point in time _before_ it's actually > > possible to transfer. Then I guess you'd later do the transfer? > > Exactly. > > > > > I guess conceivably one could re-design the DRM subsystem like that, > > but I don't think it's trivial. > > The problem is that the DRM subsystem is already designed like that. > All the bridge chains are static. They are created during the device > probe. And the modes are populated later (via the get_modes() > callback), after the HPD signal is delivered. > For the encoder/bridge chains it is explicitly stated that the display > pipe (clocks and timing signals) are not running before bridge's > enable() callback or after the disable() callback being called. > > > Why? I believe that we need to know > > things about the panel at probe time. For instance, we need to be able > > to populate the panel's modes. > > As I said, panel modes are not needed at the probe time. The fact that > most (if not all) of the panel drivers provide them in the platform > data (and thus modes are typically populated at the probe time) comes > from the fact that the panel is usually a known static piece of > hardware. With the generic edp-panel this is no longer the case. A > single device handles a (probed) variety of panels. OK, so I misspoke when I said that the modes are populated during probe time for edp-panel. They're not and I guess I managed to confuse myself when I wrote my previous email. Instead they (and everything else that comes from the EDID) isn't needed until the panel's get_modes() is called, as you said. So, ignoring the backlight problem talked about below, we could conceivably delay the reading of the EDID until get_modes. ...but I think something still isn't quite right with your description of how it works. Specifically: 1. I'm 99% certain that get_modes() is called _before_ enable, even for the DP case. I have no idea how that works for you for DP if the clocks / timing signals don't work until enable happens. Aside from my previous observations of this, it also logically makes sense that get_modes() needs to be called before enable(), doesn't it? When enable() is called then don't you already know what mode userspace has picked
Re: [PATCH v6 1/8] drm/msm/dp: Add eDP support via aux_bus
On Sat, 2 Apr 2022 at 20:06, Doug Anderson wrote: > > Hi, > > On Sat, Apr 2, 2022 at 3:37 AM Dmitry Baryshkov > wrote: > > > > On 01/04/2022 02:22, Doug Anderson wrote: > > > Hi, > > > > > > On Wed, Mar 30, 2022 at 9:03 AM Sankeerth Billakanti > > > wrote: > > >> > > >> @@ -1547,6 +1593,10 @@ int msm_dp_modeset_init(struct msm_dp > > >> *dp_display, struct drm_device *dev, > > >> > > >> dp_display->encoder = encoder; > > >> > > >> + ret = dp_display_get_next_bridge(dp_display); > > >> + if (ret) > > >> + return ret; > > > > > > It feels weird to me that this is in a function called "modeset_init", > > > though I certainly don't know the structure of the MSM display code > > > well enough to fully comment. > > > > It's called modeset_init() as it initializes KMS objects used by DP > > driver. We have similar functions for dsi and hdmi > > Sorry, I wasn't meaning to imply that modeset_init() was a bad name or > anything. Mostly saying that I wasn't sure that modeset init was the > proper time to populate the aux bus. ...but then again, perhaps it is > given the current structure of this driver? > > > > > My expectation would have been that > > > devm_of_dp_aux_populate_ep_devices() would have been called from your > > > probe routine and then you would have returned -EPROBE_DEFER from your > > > probe if you were unable to find the panel afterwards. > > > > I don't think it's possible to call it from probe() since > > drm_dp_aux_register() is called only from dp_display_bind(). > > The PHY also isn't initialized at that moment, so we can not probe AUX > > devices. > > > > The overall semantics of the AUX bus is not clear to me. > > Typically the bus is populated (and probed) when devices are accessible. > > But for the display related buses this might not be the case. > > In general the AUX bus is modeled much like the i2c bus. You probe the > sub-device when you're able to transfer. Then you can confirm that the > device is actually there and init the device. > > > > For example for the DSI bus we clearly define that DSI transfer are not > > possible before the corresponding bridge's (or panel's) enable call. > > > > Maybe the same approach should be adopted for the AUX bus. This would > > allow us to populate the AUX bus before hardware access is actually > > possible, thus creating all the DRM bridges before the hardware is > > actually up and running. > > So I guess what you're proposing is that you could probe the devices > under the AUX bus and they could acquire resources (and possibly > return EPROBE_DEFER) at a point in time _before_ it's actually > possible to transfer. Then I guess you'd later do the transfer? Exactly. > > I guess conceivably one could re-design the DRM subsystem like that, > but I don't think it's trivial. The problem is that the DRM subsystem is already designed like that. All the bridge chains are static. They are created during the device probe. And the modes are populated later (via the get_modes() callback), after the HPD signal is delivered. For the encoder/bridge chains it is explicitly stated that the display pipe (clocks and timing signals) are not running before bridge's enable() callback or after the disable() callback being called. > Why? I believe that we need to know > things about the panel at probe time. For instance, we need to be able > to populate the panel's modes. As I said, panel modes are not needed at the probe time. The fact that most (if not all) of the panel drivers provide them in the platform data (and thus modes are typically populated at the probe time) comes from the fact that the panel is usually a known static piece of hardware. With the generic edp-panel this is no longer the case. A single device handles a (probed) variety of panels. Compare it with the generic monitor: We have a known bridge (display-connector.c), so the driver can build the display chain. However a set of modes is not known till the actual monitor is plugged into the device. > To get this information we need the > EDID which means we need to be able to do a transfer. If we're using > an AUX backlight we also need to add info about the backlight at probe > time and that also needs the transfer to work. Yes, the backlight is the problem in the suggested design. I'm not sure when panel->backlight has to be populated for things to work. If we can set it after the probe but before calling into mode_set/drm_bridge_chain_enable(), then it should be fine. > So I guess the net result is maybe we should just keep it where it is. > Long term I'd be interested in knowing if there's a reason why we > can't structure the driver so that AUX transfers can happen with less > intertwining with the rest of the code, but that can happen later. I > would expect that you'd basically just need clocks and regulators on > and maybe your PHY on. Ideally with some pm_runtime fun we should be > able to do that independently with anything else the driver
Re: [PATCH v6 1/8] drm/msm/dp: Add eDP support via aux_bus
Hi, On Sat, Apr 2, 2022 at 3:37 AM Dmitry Baryshkov wrote: > > On 01/04/2022 02:22, Doug Anderson wrote: > > Hi, > > > > On Wed, Mar 30, 2022 at 9:03 AM Sankeerth Billakanti > > wrote: > >> > >> @@ -1547,6 +1593,10 @@ int msm_dp_modeset_init(struct msm_dp *dp_display, > >> struct drm_device *dev, > >> > >> dp_display->encoder = encoder; > >> > >> + ret = dp_display_get_next_bridge(dp_display); > >> + if (ret) > >> + return ret; > > > > It feels weird to me that this is in a function called "modeset_init", > > though I certainly don't know the structure of the MSM display code > > well enough to fully comment. > > It's called modeset_init() as it initializes KMS objects used by DP > driver. We have similar functions for dsi and hdmi Sorry, I wasn't meaning to imply that modeset_init() was a bad name or anything. Mostly saying that I wasn't sure that modeset init was the proper time to populate the aux bus. ...but then again, perhaps it is given the current structure of this driver? > > My expectation would have been that > > devm_of_dp_aux_populate_ep_devices() would have been called from your > > probe routine and then you would have returned -EPROBE_DEFER from your > > probe if you were unable to find the panel afterwards. > > I don't think it's possible to call it from probe() since > drm_dp_aux_register() is called only from dp_display_bind(). > The PHY also isn't initialized at that moment, so we can not probe AUX > devices. > > The overall semantics of the AUX bus is not clear to me. > Typically the bus is populated (and probed) when devices are accessible. > But for the display related buses this might not be the case. In general the AUX bus is modeled much like the i2c bus. You probe the sub-device when you're able to transfer. Then you can confirm that the device is actually there and init the device. > For example for the DSI bus we clearly define that DSI transfer are not > possible before the corresponding bridge's (or panel's) enable call. > > Maybe the same approach should be adopted for the AUX bus. This would > allow us to populate the AUX bus before hardware access is actually > possible, thus creating all the DRM bridges before the hardware is > actually up and running. So I guess what you're proposing is that you could probe the devices under the AUX bus and they could acquire resources (and possibly return EPROBE_DEFER) at a point in time _before_ it's actually possible to transfer. Then I guess you'd later do the transfer? I guess conceivably one could re-design the DRM subsystem like that, but I don't think it's trivial. Why? I believe that we need to know things about the panel at probe time. For instance, we need to be able to populate the panel's modes. To get this information we need the EDID which means we need to be able to do a transfer. If we're using an AUX backlight we also need to add info about the backlight at probe time and that also needs the transfer to work. So I guess the net result is maybe we should just keep it where it is. Long term I'd be interested in knowing if there's a reason why we can't structure the driver so that AUX transfers can happen with less intertwining with the rest of the code, but that can happen later. I would expect that you'd basically just need clocks and regulators on and maybe your PHY on. Ideally with some pm_runtime fun we should be able to do that independently with anything else the driver needs to do? -Doug
Re: [PATCH v6 1/8] drm/msm/dp: Add eDP support via aux_bus
On 01/04/2022 02:22, Doug Anderson wrote:
Hi,
On Wed, Mar 30, 2022 at 9:03 AM Sankeerth Billakanti
wrote:
@@ -1547,6 +1593,10 @@ int msm_dp_modeset_init(struct msm_dp *dp_display,
struct drm_device *dev,
dp_display->encoder = encoder;
+ ret = dp_display_get_next_bridge(dp_display);
+ if (ret)
+ return ret;
It feels weird to me that this is in a function called "modeset_init",
though I certainly don't know the structure of the MSM display code
well enough to fully comment.
It's called modeset_init() as it initializes KMS objects used by DP
driver. We have similar functions for dsi and hdmi
My expectation would have been that
devm_of_dp_aux_populate_ep_devices() would have been called from your
probe routine and then you would have returned -EPROBE_DEFER from your
probe if you were unable to find the panel afterwards.
I don't think it's possible to call it from probe() since
drm_dp_aux_register() is called only from dp_display_bind().
The PHY also isn't initialized at that moment, so we can not probe AUX
devices.
The overall semantics of the AUX bus is not clear to me.
Typically the bus is populated (and probed) when devices are accessible.
But for the display related buses this might not be the case.
For example for the DSI bus we clearly define that DSI transfer are not
possible before the corresponding bridge's (or panel's) enable call.
Maybe the same approach should be adopted for the AUX bus. This would
allow us to populate the AUX bus before hardware access is actually
possible, thus creating all the DRM bridges before the hardware is
actually up and running.
Huh, but I guess you _are_ getting called (indirectly) from
dpu_kms_hw_init() and I can't imagine AUX transfers working before
that function is called, so maybe I should just accept that it's
complicated and let those who understand this driver better confirm
that it's OK. ;-)
@@ -140,5 +140,6 @@ struct dp_parser {
* can be parsed using this module.
*/
struct dp_parser *dp_parser_get(struct platform_device *pdev);
+int dp_parser_find_next_bridge(struct dp_parser *parser);
Everything else in this file is described w/ kerneldoc. Shouldn't your
function also have a kerneldoc comment?
--
With best wishes
Dmitry
Re: [PATCH v6 1/8] drm/msm/dp: Add eDP support via aux_bus
Hi,
On Wed, Mar 30, 2022 at 9:03 AM Sankeerth Billakanti
wrote:
>
> @@ -1547,6 +1593,10 @@ int msm_dp_modeset_init(struct msm_dp *dp_display,
> struct drm_device *dev,
>
> dp_display->encoder = encoder;
>
> + ret = dp_display_get_next_bridge(dp_display);
> + if (ret)
> + return ret;
It feels weird to me that this is in a function called "modeset_init",
though I certainly don't know the structure of the MSM display code
well enough to fully comment. My expectation would have been that
devm_of_dp_aux_populate_ep_devices() would have been called from your
probe routine and then you would have returned -EPROBE_DEFER from your
probe if you were unable to find the panel afterwards.
Huh, but I guess you _are_ getting called (indirectly) from
dpu_kms_hw_init() and I can't imagine AUX transfers working before
that function is called, so maybe I should just accept that it's
complicated and let those who understand this driver better confirm
that it's OK. ;-)
> @@ -140,5 +140,6 @@ struct dp_parser {
> * can be parsed using this module.
> */
> struct dp_parser *dp_parser_get(struct platform_device *pdev);
> +int dp_parser_find_next_bridge(struct dp_parser *parser);
Everything else in this file is described w/ kerneldoc. Shouldn't your
function also have a kerneldoc comment?
Re: [PATCH v6 1/8] drm/msm/dp: Add eDP support via aux_bus
Hi, On Wed, Mar 30, 2022 at 4:19 PM Dmitry Baryshkov wrote: > > + bridge->ops = > > + DRM_BRIDGE_OP_DETECT | > > + DRM_BRIDGE_OP_HPD | > > + DRM_BRIDGE_OP_MODES; > > I think OP_MODES should be used for eDP, shouldn't it? No. It's confusing, but basically to get the power sequencing correct we end up driving the EDID read from the panel driver in the eDP case. -Doug
Re: [PATCH v6 1/8] drm/msm/dp: Add eDP support via aux_bus
On 30/03/2022 19:02, Sankeerth Billakanti wrote:
This patch adds support for generic eDP sink through aux_bus. The eDP/DP
controller driver should support aux transactions originating from the
panel-edp driver and hence should be initialized and ready.
The panel bridge supporting the panel should be ready before the bridge
connector is initialized. The generic panel probe needs the controller
resources to be enabled to support the aux transactions originating from
the panel probe.
Signed-off-by: Sankeerth Billakanti
---
Changes in v6:
- Remove initialization
- Fix aux_bus node leak
- Split the patches
drivers/gpu/drm/msm/dp/dp_display.c | 54 +++--
drivers/gpu/drm/msm/dp/dp_drm.c | 10 ---
drivers/gpu/drm/msm/dp/dp_parser.c | 21 +--
drivers/gpu/drm/msm/dp/dp_parser.h | 1 +
4 files changed, 60 insertions(+), 26 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
b/drivers/gpu/drm/msm/dp/dp_display.c
index 382b3aa..e082d02 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -10,6 +10,7 @@
#include
#include
#include
+#include
#include "msm_drv.h"
#include "msm_kms.h"
@@ -265,8 +266,6 @@ static int dp_display_bind(struct device *dev, struct
device *master,
goto end;
}
- dp->dp_display.next_bridge = dp->parser->next_bridge;
-
dp->aux->drm_dev = drm;
rc = dp_aux_register(dp->aux);
if (rc) {
@@ -1524,6 +1523,53 @@ void msm_dp_debugfs_init(struct msm_dp *dp_display,
struct drm_minor *minor)
}
}
+static int dp_display_get_next_bridge(struct msm_dp *dp)
+{
+ int rc;
+ struct dp_display_private *dp_priv;
+ struct device_node *aux_bus;
+ struct device *dev;
+
+ dp_priv = container_of(dp, struct dp_display_private, dp_display);
+ dev = _priv->pdev->dev;
+ aux_bus = of_get_child_by_name(dev->of_node, "aux-bus");
+
+ if (aux_bus) {
+ dp_display_host_init(dp_priv);
+ dp_catalog_ctrl_hpd_config(dp_priv->catalog);
+ enable_irq(dp_priv->irq);
+ dp_display_host_phy_init(dp_priv);
+
+ devm_of_dp_aux_populate_ep_devices(dp_priv->aux);
+
+ disable_irq(dp_priv->irq);
+ of_node_put(aux_bus);
+ }
+
+ /*
+* External bridges are mandatory for eDP interfaces: one has to
+* provide at least an eDP panel (which gets wrapped into panel-bridge).
+*
+* For DisplayPort interfaces external bridges are optional, so
+* silently ignore an error if one is not present (-ENODEV).
+*/
+ rc = dp_parser_find_next_bridge(dp_priv->parser);
+ if (rc == -ENODEV) {
+ if (dp->connector_type == DRM_MODE_CONNECTOR_eDP) {
The more I think about these conditions, the closer I dislike them (yes,
I added this one in one of the patches). I'd suggest to change
dp->connector_type to boolean 'is_edp' field use it in all conditions
instead.
+ DRM_ERROR("eDP: next bridge is not present\n");
+ return rc;
+ }
+ } else if (rc) {
+ if (rc != -EPROBE_DEFER)
+ DRM_ERROR("DP: error parsing next bridge: %d\n", rc);
+ return rc;
+ }
+
+ dp->next_bridge = dp_priv->parser->next_bridge;
+
+ return 0;
+}
+
int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev,
struct drm_encoder *encoder)
{
@@ -1547,6 +1593,10 @@ int msm_dp_modeset_init(struct msm_dp *dp_display,
struct drm_device *dev,
dp_display->encoder = encoder;
+ ret = dp_display_get_next_bridge(dp_display);
+ if (ret)
+ return ret;
+
dp_display->bridge = dp_bridge_init(dp_display, dev, encoder);
if (IS_ERR(dp_display->bridge)) {
ret = PTR_ERR(dp_display->bridge);
diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c
index 7ce1aca..5254bd6 100644
--- a/drivers/gpu/drm/msm/dp/dp_drm.c
+++ b/drivers/gpu/drm/msm/dp/dp_drm.c
@@ -114,10 +114,12 @@ struct drm_bridge *dp_bridge_init(struct msm_dp
*dp_display, struct drm_device *
bridge->funcs = _bridge_ops;
bridge->type = dp_display->connector_type;
- bridge->ops =
- DRM_BRIDGE_OP_DETECT |
- DRM_BRIDGE_OP_HPD |
- DRM_BRIDGE_OP_MODES;
+ if (bridge->type == DRM_MODE_CONNECTOR_DisplayPort) {
And in this case we can also check dp_display->connector_type (or the
suggested dp_display->is_edp) for the uniformity of the code.
+ bridge->ops =
+ DRM_BRIDGE_OP_DETECT |
+ DRM_BRIDGE_OP_HPD |
+ DRM_BRIDGE_OP_MODES;
I think OP_MODES should be used for eDP, shouldn't it?
+ }
rc = drm_bridge_attach(encoder, bridge,
