On Tue, Sep 14, 2021 at 05:06:41PM +0200, Maxime Ripard wrote:
Hi Sam,
On Tue, Sep 14, 2021 at 12:41:55PM +0200, Sam Ravnborg wrote:
On Tue, Sep 14, 2021 at 12:17:23PM +0200, Maxime Ripard wrote:
The drm_helper_hpd_irq_event() function is iterating over all the connectors when an hotplug event is detected.
During that iteration, it will call each connector detect function and figure out if its status changed.
Finally, if any connector changed, it will notify the user-space and the clients that something changed on the DRM device.
This is supposed to be used for drivers that don't have a hotplug interrupt for individual connectors. However, drivers that can use an interrupt for a single connector are left in the dust and can either reimplement the logic used during the iteration for each connector or use that helper and iterate over all connectors all the time.
Since both are suboptimal, let's create a helper that will only perform the status detection on a single connector.
Signed-off-by: Maxime Ripard maxime@cerno.tech
Changes from v2:
- Skip connectors with DRM_CONNECTOR_POLL_HPD in drm_helper_hpd_irq_event
- Add drm_connector_helper_hpd_irq_event returned value documentation
- Improve logging
Changes from v1:
- Rename the shared function
- Move the hotplug event notification out of the shared function
- Added missing locks
- Improve the documentation
- Switched to drm_dbg_kms
drivers/gpu/drm/drm_probe_helper.c | 117 +++++++++++++++++++++-------- include/drm/drm_probe_helper.h | 1 + 2 files changed, 87 insertions(+), 31 deletions(-)
diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c index 5b77fb5c1a32..a1ffc0c30b3a 100644 --- a/drivers/gpu/drm/drm_probe_helper.c +++ b/drivers/gpu/drm/drm_probe_helper.c @@ -795,6 +795,87 @@ void drm_kms_helper_poll_fini(struct drm_device *dev) } EXPORT_SYMBOL(drm_kms_helper_poll_fini);
+static bool check_connector_changed(struct drm_connector *connector) +{
- struct drm_device *dev = connector->dev;
- enum drm_connector_status old_status;
- u64 old_epoch_counter;
- bool changed = false;
- /* Only handle HPD capable connectors. */
- drm_WARN_ON(dev, !(connector->polled & DRM_CONNECTOR_POLL_HPD));
This will WARN if DRM_CONNECTOR_POLL_HPD is not set - which the previous code did not. I am not sure this is intentional. Or have I missed something?
Sorry, I misunderstood your previous comment and thought you wanted to skip the !HPD connectors in the drm_helper_hpd_irq_event loop.
What do you think would be the proper scenario here? Just return false?
Re-reading the patch it looks good. I missed that the check is no longer deleted - and it is now done exactly as I thought it should be.
So patch is: Reviewed-by: Sam Ravnborg sam@ravnborg.org
Sam