Hi Stephen
On 9/22/2023 2:54 PM, Stephen Boyd wrote:
Quoting Dmitry Baryshkov (2023-09-19 02:50:12)
On Mon, 18 Sept 2023 at 20:48, Kuogee Hsieh <quic_khsieh@xxxxxxxxxxx> wrote:
On 9/15/2023 6:21 PM, Dmitry Baryshkov wrote:
On Sat, 16 Sept 2023 at 00:38, Kuogee Hsieh <quic_khsieh@xxxxxxxxxxx> wrote:
Add pm_runtime_force_suspend()/resume() to complete incorporating pm
runtime framework into DP driver. Both dp_pm_prepare() and dp_pm_complete()
are added to set hpd_state to correct state. After resume, DP driver will
re training its main link after .hpd_enable() callback enabled HPD
interrupts and bring up display accordingly.
How will it re-train the main link? What is the code path for that?
1) for edp, dp_bridge_atomic_enable(), called from framework, to start
link training and bring up display.
And this path doesn't use .hpd_enable() which you have mentioned in
the commit message. Please don't try to shorten the commit message.
You see, I have had questions to several of them, which means that
they were not verbose enough.
2) for external DP, HPD_PLUG_INT will be generated to start link
training and bring up display.
This should be hpd_notify, who starts link training, not some event.
I think this driver should train the link during atomic_enable(), not
hpd_notify() (or directly from the irq handler). The drm_bridge_funcs
talk a bit about when the clocks and timing signals are supposed to be
enabled. For example, struct drm_bridge_funcs::atomic_pre_enable() says
the "display pipe (i.e. clocks and timing signals) feeding this bridge
will not yet be running when this callback is called". And struct
drm_bridge_funcs::atomic_enable() says "this callback must enable the
display link feeding the next bridge in the chain if there is one."
That looks to me like link training, i.e. the display link, should
happen in the enable path and not hpd_notify. It looks like link
training could fail, but when that happens I believe the driver should
call drm_connector_set_link_status_property() with
DRM_MODE_LINK_STATUS_BAD. The two callers of that which exist in the
tree also call drm_kms_helper_hotplug_event() or
drm_kms_helper_connector_hotplug_event() after updating the link so that
userspace knows to try again.
It would be nice if there was some drm_bridge_set_link_status_bad() API
that bridge drivers could use to signal that the link status is bad and
call the hotplug helper. Maybe it could also record some diagnostics
about which bridge failed to setup the link and stop the atomic_enable()
chain for that connector.
Doing link training when we get hpd instead of atomic_enable() is a
design choice we have been following for a while because for the case
when link training fails in atomic_enable() and setting the link status
property as you mentioned, the compositor needs to be able to handle
that and also needs to try with a different resolution or take some
other corrective action. We have seen many compositors not able to
handle this complexity. So the design sends the hotplug to usermode only
after link training succeeds.
I do not think we should change this design unless prototyped with an
existing compositor such as chrome or android at this point.
Thanks
Abhinav
