* [PATCH] drm: do not call detect for connectors which are forced on @ 2022-08-26 9:11 Michael Rodin 2022-09-29 23:33 ` Laurent Pinchart 2022-11-07 12:36 ` Maxime Ripard 0 siblings, 2 replies; 8+ messages in thread From: Michael Rodin @ 2022-08-26 9:11 UTC (permalink / raw) To: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie, Daniel Vetter, Alex Deucher, Philipp Zabel, Sean Paul, Vincent Abriou, dri-devel, linux-kernel Cc: Michael Rodin, michael, erosca "detect" should not be called and its return value shall not be used when a connector is forced as hinted in the commit d50ba256b5f1 ("drm/kms: start adding command line interface using fb.") and the commit 6fe14acd496e ("drm: Document drm_connector_funcs"). One negative side effect of doing this is observed on the RCar3 SoCs which use the dw-hdmi driver. It continues executing drm_helper_hpd_irq_event even if its connector is forced to ON. As consequence drm_helper_hpd_irq_event calls "detect" so the connector status is updated to "disconnected": [ 420.201527] [drm:drm_helper_hpd_irq_event] [CONNECTOR:76:HDMI-A-1] status updated from connected to disconnected This status is corrected by drm_helper_probe_single_connector_modes shortly after this because this function checks if a connector is forced: [ 420.218703] [drm:drm_helper_probe_single_connector_modes] [CONNECTOR:76:HDMI-A-1] status updated from disconnected to connected To avoid similar issues this commit adapts functions which call "detect" so they check if a connector is forced and return the correct status. Fixes: 949f08862d66 ("drm: Make the connector .detect() callback optional") Signed-off-by: Michael Rodin <mrodin@de.adit-jv.com> --- drivers/gpu/drm/drm_probe_helper.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c index bb427c5a4f1f..1691047d0472 100644 --- a/drivers/gpu/drm/drm_probe_helper.c +++ b/drivers/gpu/drm/drm_probe_helper.c @@ -289,7 +289,12 @@ drm_helper_probe_detect_ctx(struct drm_connector *connector, bool force) retry: ret = drm_modeset_lock(&connector->dev->mode_config.connection_mutex, &ctx); if (!ret) { - if (funcs->detect_ctx) + if (connector->force == DRM_FORCE_ON || + connector->force == DRM_FORCE_ON_DIGITAL) + ret = connector_status_connected; + else if (connector->force == DRM_FORCE_OFF) + ret = connector_status_disconnected; + else if (funcs->detect_ctx) ret = funcs->detect_ctx(connector, &ctx, force); else if (connector->funcs->detect) ret = connector->funcs->detect(connector, force); @@ -340,7 +345,14 @@ drm_helper_probe_detect(struct drm_connector *connector, if (ret) return ret; - if (funcs->detect_ctx) + if (connector->force == DRM_FORCE_ON || + connector->force == DRM_FORCE_ON_DIGITAL) + ret = connector_status_connected; + else if (connector->force == DRM_FORCE_OFF) + ret = connector_status_disconnected; + else if (funcs->detect_ctx) + ret = funcs->detect_ctx(connector, ctx, force); + else if (funcs->detect_ctx) ret = funcs->detect_ctx(connector, ctx, force); else if (connector->funcs->detect) ret = connector->funcs->detect(connector, force); -- 2.25.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] drm: do not call detect for connectors which are forced on 2022-08-26 9:11 [PATCH] drm: do not call detect for connectors which are forced on Michael Rodin @ 2022-09-29 23:33 ` Laurent Pinchart 2022-11-07 12:25 ` Maxime Ripard 2022-11-07 12:36 ` Maxime Ripard 1 sibling, 1 reply; 8+ messages in thread From: Laurent Pinchart @ 2022-09-29 23:33 UTC (permalink / raw) To: Michael Rodin, Maarten Lankhorst, Maxime Ripard Cc: Thomas Zimmermann, David Airlie, Daniel Vetter, Alex Deucher, Philipp Zabel, Sean Paul, Vincent Abriou, dri-devel, linux-kernel, michael, erosca Hello Michael, Thank you for the patch. Sorry for the late reply, I wasn't on the CC list so I didn't notice it. On Fri, Aug 26, 2022 at 11:11:21AM +0200, Michael Rodin wrote: > "detect" should not be called and its return value shall not be used when a > connector is forced as hinted in the commit d50ba256b5f1 ("drm/kms: start > adding command line interface using fb.") and the commit 6fe14acd496e > ("drm: Document drm_connector_funcs"). One negative side effect of doing > this is observed on the RCar3 SoCs which use the dw-hdmi driver. It > continues executing drm_helper_hpd_irq_event even if its connector is > forced to ON. As consequence drm_helper_hpd_irq_event calls "detect" so the > connector status is updated to "disconnected": > > [ 420.201527] [drm:drm_helper_hpd_irq_event] [CONNECTOR:76:HDMI-A-1] status updated from connected to disconnected > > This status is corrected by drm_helper_probe_single_connector_modes shortly > after this because this function checks if a connector is forced: > > [ 420.218703] [drm:drm_helper_probe_single_connector_modes] [CONNECTOR:76:HDMI-A-1] status updated from disconnected to connected > > To avoid similar issues this commit adapts functions which call "detect" > so they check if a connector is forced and return the correct status. > > Fixes: 949f08862d66 ("drm: Make the connector .detect() callback optional") Is this really the right fixes tag ? The call to .detect() in drm_helper_hpd_irq_event() predates that commit. > Signed-off-by: Michael Rodin <mrodin@de.adit-jv.com> > --- > drivers/gpu/drm/drm_probe_helper.c | 16 ++++++++++++++-- > 1 file changed, 14 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c > index bb427c5a4f1f..1691047d0472 100644 > --- a/drivers/gpu/drm/drm_probe_helper.c > +++ b/drivers/gpu/drm/drm_probe_helper.c > @@ -289,7 +289,12 @@ drm_helper_probe_detect_ctx(struct drm_connector *connector, bool force) > retry: > ret = drm_modeset_lock(&connector->dev->mode_config.connection_mutex, &ctx); > if (!ret) { > - if (funcs->detect_ctx) > + if (connector->force == DRM_FORCE_ON || > + connector->force == DRM_FORCE_ON_DIGITAL) > + ret = connector_status_connected; > + else if (connector->force == DRM_FORCE_OFF) > + ret = connector_status_disconnected; > + else if (funcs->detect_ctx) > ret = funcs->detect_ctx(connector, &ctx, force); > else if (connector->funcs->detect) > ret = connector->funcs->detect(connector, force); > @@ -340,7 +345,14 @@ drm_helper_probe_detect(struct drm_connector *connector, > if (ret) > return ret; > > - if (funcs->detect_ctx) > + if (connector->force == DRM_FORCE_ON || > + connector->force == DRM_FORCE_ON_DIGITAL) > + ret = connector_status_connected; > + else if (connector->force == DRM_FORCE_OFF) > + ret = connector_status_disconnected; > + else if (funcs->detect_ctx) > + ret = funcs->detect_ctx(connector, ctx, force); > + else if (funcs->detect_ctx) > ret = funcs->detect_ctx(connector, ctx, force); Those two conditions are identical. > else if (connector->funcs->detect) > ret = connector->funcs->detect(connector, force); The same logic is used in two places in this patch. Could this be factored out to a separate function ? It may even be possible to refactor drm_helper_probe_detect() and drm_helper_probe_detect_ctx() to share more code between the two functions. This being said, I'm not sure this is the right fix. Beside the i915 and nouveau drivers, the only callers of drm_helper_probe_detect() are drm_helper_probe_single_connector_modes(), output_poll_execute() and check_connector_changed() in this file. The first two functions already check connector->force and skip the call to drm_helper_probe_detect() if the connector is forced. Only check_connector_changed() is missing that check. Wouldn't it be simpler to return false in that function if connector->force is set ? Another question is whether it is valid for the dw-hdmi driver to call drm_helper_hpd_irq_event() when the connector status is forced. Shouldn't HPD events be ignored in that case ? The detection code has grown quite complex over time, I would really appreciate input from Maxime and Maarten on this. -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm: do not call detect for connectors which are forced on 2022-09-29 23:33 ` Laurent Pinchart @ 2022-11-07 12:25 ` Maxime Ripard 0 siblings, 0 replies; 8+ messages in thread From: Maxime Ripard @ 2022-11-07 12:25 UTC (permalink / raw) To: Laurent Pinchart Cc: Michael Rodin, Maarten Lankhorst, Thomas Zimmermann, David Airlie, Daniel Vetter, Alex Deucher, Philipp Zabel, Sean Paul, Vincent Abriou, dri-devel, linux-kernel, michael, erosca [-- Attachment #1: Type: text/plain, Size: 5820 bytes --] On Fri, Sep 30, 2022 at 02:33:08AM +0300, Laurent Pinchart wrote: > Hello Michael, > > Thank you for the patch. Sorry for the late reply, I wasn't on the CC > list so I didn't notice it. > > On Fri, Aug 26, 2022 at 11:11:21AM +0200, Michael Rodin wrote: > > "detect" should not be called and its return value shall not be used when a > > connector is forced as hinted in the commit d50ba256b5f1 ("drm/kms: start > > adding command line interface using fb.") and the commit 6fe14acd496e > > ("drm: Document drm_connector_funcs"). One negative side effect of doing > > this is observed on the RCar3 SoCs which use the dw-hdmi driver. It > > continues executing drm_helper_hpd_irq_event even if its connector is > > forced to ON. As consequence drm_helper_hpd_irq_event calls "detect" so the > > connector status is updated to "disconnected": > > > > [ 420.201527] [drm:drm_helper_hpd_irq_event] [CONNECTOR:76:HDMI-A-1] status updated from connected to disconnected > > > > This status is corrected by drm_helper_probe_single_connector_modes shortly > > after this because this function checks if a connector is forced: > > > > [ 420.218703] [drm:drm_helper_probe_single_connector_modes] [CONNECTOR:76:HDMI-A-1] status updated from disconnected to connected > > > > To avoid similar issues this commit adapts functions which call "detect" > > so they check if a connector is forced and return the correct status. > > > > Fixes: 949f08862d66 ("drm: Make the connector .detect() callback optional") > > Is this really the right fixes tag ? The call to .detect() in > drm_helper_hpd_irq_event() predates that commit. > > > Signed-off-by: Michael Rodin <mrodin@de.adit-jv.com> > > --- > > drivers/gpu/drm/drm_probe_helper.c | 16 ++++++++++++++-- > > 1 file changed, 14 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c > > index bb427c5a4f1f..1691047d0472 100644 > > --- a/drivers/gpu/drm/drm_probe_helper.c > > +++ b/drivers/gpu/drm/drm_probe_helper.c > > @@ -289,7 +289,12 @@ drm_helper_probe_detect_ctx(struct drm_connector *connector, bool force) > > retry: > > ret = drm_modeset_lock(&connector->dev->mode_config.connection_mutex, &ctx); > > if (!ret) { > > - if (funcs->detect_ctx) > > + if (connector->force == DRM_FORCE_ON || > > + connector->force == DRM_FORCE_ON_DIGITAL) > > + ret = connector_status_connected; > > + else if (connector->force == DRM_FORCE_OFF) > > + ret = connector_status_disconnected; > > + else if (funcs->detect_ctx) > > ret = funcs->detect_ctx(connector, &ctx, force); > > else if (connector->funcs->detect) > > ret = connector->funcs->detect(connector, force); > > @@ -340,7 +345,14 @@ drm_helper_probe_detect(struct drm_connector *connector, > > if (ret) > > return ret; > > > > - if (funcs->detect_ctx) > > + if (connector->force == DRM_FORCE_ON || > > + connector->force == DRM_FORCE_ON_DIGITAL) > > + ret = connector_status_connected; > > + else if (connector->force == DRM_FORCE_OFF) > > + ret = connector_status_disconnected; > > + else if (funcs->detect_ctx) > > + ret = funcs->detect_ctx(connector, ctx, force); > > + else if (funcs->detect_ctx) > > ret = funcs->detect_ctx(connector, ctx, force); > > Those two conditions are identical. > > > else if (connector->funcs->detect) > > ret = connector->funcs->detect(connector, force); > > The same logic is used in two places in this patch. Could this be > factored out to a separate function ? It may even be possible to > refactor drm_helper_probe_detect() and drm_helper_probe_detect_ctx() to > share more code between the two functions. I just had a look, and it doesn't seem trivial. The obvious way would be to make drm_helper_probe_detect_ctx allocate a context and call drm_helper_probe_detect. The thing is, drm_helper_probe_detect will call drm_helper_probe_detect_ctx if its context is NULL. I guess we could have drm_helper_probe_detect allocate the context itself if it's null, but handling differently the back-off and freeing logic is probably going to add more complexity. I'm not sure it's worth it for simple functions like this. > This being said, I'm not sure this is the right fix. Beside the i915 and > nouveau drivers, the only callers of drm_helper_probe_detect() are > drm_helper_probe_single_connector_modes(), output_poll_execute() and > check_connector_changed() in this file. The first two functions already > check connector->force and skip the call to drm_helper_probe_detect() if > the connector is forced. Only check_connector_changed() is missing that > check. Wouldn't it be simpler to return false in that function if > connector->force is set ? I guess, but the drm_helper_probe_detect documentation states that it "probe connector status" and "This function calls the detect callbacks of the connector.", which I guess could be interpreted as it always runs the detect callback but won't do more. But it also returns that the connector is connected if the detect callback is missing and thus it feels like putting it here both respect the "probe connector status " (even though it's forced), and the general idea behind that function. > Another question is whether it is valid for the dw-hdmi driver to call > drm_helper_hpd_irq_event() when the connector status is forced. I guess drm_connector_helper_hpd_irq_event would be a better choice, but it seems fine to me. Especially since pretty much every other driver does it that way, I'd rather assume that the driver doesn't have to track the connection status itself and leave that to the framework. It's more convenient, and it's what virtually all drivers are doing. Maxime [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm: do not call detect for connectors which are forced on 2022-08-26 9:11 [PATCH] drm: do not call detect for connectors which are forced on Michael Rodin 2022-09-29 23:33 ` Laurent Pinchart @ 2022-11-07 12:36 ` Maxime Ripard 2022-12-15 17:03 ` [PATCH] drm: override detected status " Michael Rodin 1 sibling, 1 reply; 8+ messages in thread From: Maxime Ripard @ 2022-11-07 12:36 UTC (permalink / raw) To: Michael Rodin Cc: Maarten Lankhorst, Thomas Zimmermann, David Airlie, Daniel Vetter, Alex Deucher, Philipp Zabel, Sean Paul, Vincent Abriou, dri-devel, linux-kernel, michael, erosca [-- Attachment #1: Type: text/plain, Size: 2972 bytes --] On Fri, Aug 26, 2022 at 11:11:21AM +0200, Michael Rodin wrote: > "detect" should not be called and its return value shall not be used when a > connector is forced as hinted in the commit d50ba256b5f1 ("drm/kms: start > adding command line interface using fb.") and the commit 6fe14acd496e > ("drm: Document drm_connector_funcs"). One negative side effect of doing > this is observed on the RCar3 SoCs which use the dw-hdmi driver. It > continues executing drm_helper_hpd_irq_event even if its connector is > forced to ON. As consequence drm_helper_hpd_irq_event calls "detect" so the > connector status is updated to "disconnected": > > [ 420.201527] [drm:drm_helper_hpd_irq_event] [CONNECTOR:76:HDMI-A-1] status updated from connected to disconnected > > This status is corrected by drm_helper_probe_single_connector_modes shortly > after this because this function checks if a connector is forced: > > [ 420.218703] [drm:drm_helper_probe_single_connector_modes] [CONNECTOR:76:HDMI-A-1] status updated from disconnected to connected > > To avoid similar issues this commit adapts functions which call "detect" > so they check if a connector is forced and return the correct status. > > Fixes: 949f08862d66 ("drm: Make the connector .detect() callback optional") > Signed-off-by: Michael Rodin <mrodin@de.adit-jv.com> > --- > drivers/gpu/drm/drm_probe_helper.c | 16 ++++++++++++++-- > 1 file changed, 14 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c > index bb427c5a4f1f..1691047d0472 100644 > --- a/drivers/gpu/drm/drm_probe_helper.c > +++ b/drivers/gpu/drm/drm_probe_helper.c > @@ -289,7 +289,12 @@ drm_helper_probe_detect_ctx(struct drm_connector *connector, bool force) > retry: > ret = drm_modeset_lock(&connector->dev->mode_config.connection_mutex, &ctx); > if (!ret) { > - if (funcs->detect_ctx) > + if (connector->force == DRM_FORCE_ON || > + connector->force == DRM_FORCE_ON_DIGITAL) > + ret = connector_status_connected; > + else if (connector->force == DRM_FORCE_OFF) > + ret = connector_status_disconnected; > + else if (funcs->detect_ctx) > ret = funcs->detect_ctx(connector, &ctx, force); > else if (connector->funcs->detect) > ret = connector->funcs->detect(connector, force); Actually, I think this creates a regression on vc4 at least (and possibly i915?), when the HDMI scrambling is on. When a monitor is disconnected and then connected again, the mode won't change by itself, but it will lose its scrambling status. The way we dealt with it (and documented here: https://lore.kernel.org/all/20220829134731.213478-9-maxime@cerno.tech/) is to have detect enabling the scrambler again if it detects a disconnection/connection transition. If we force the status on all the time, then we will never get called and we won't have the opportunity to enable the scrambler again. Maxime [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] drm: override detected status for connectors which are forced on 2022-11-07 12:36 ` Maxime Ripard @ 2022-12-15 17:03 ` Michael Rodin 2022-12-22 17:40 ` Maxime Ripard 0 siblings, 1 reply; 8+ messages in thread From: Michael Rodin @ 2022-12-15 17:03 UTC (permalink / raw) To: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie, Daniel Vetter, Dave Airlie, Alex Deucher, dri-devel, linux-kernel Cc: Michael Rodin, michael, erosca, laurent.pinchart The detected status of a connector should be ignored when a connector is forced as hinted in the commit d50ba256b5f1 ("drm/kms: start adding command line interface using fb."). One negative side effect of not ignoring this is observed on the RCar3 SoCs which use the dw-hdmi driver. It continues executing drm_helper_hpd_irq_event even if its connector is forced to ON. As consequence drm_helper_hpd_irq_event calls "detect" so the connector status is updated to "disconnected": [ 420.201527] [drm:drm_helper_hpd_irq_event] [CONNECTOR:76:HDMI-A-1] status updated from connected to disconnected This status is corrected by drm_helper_probe_single_connector_modes shortly after this because this function checks if a connector is forced: [ 420.218703] [drm:drm_helper_probe_single_connector_modes] [CONNECTOR:76:HDMI-A-1] status updated from disconnected to connected To avoid similar issues this commit adapts functions which call "detect" so they check additionally if a connector is forced and override the status returned by "detect". Fixes: 816da85a0990 ("drm: handle HPD and polled connectors separately") Signed-off-by: Michael Rodin <mrodin@de.adit-jv.com> --- drivers/gpu/drm/drm_probe_helper.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c index bcd9611dabfd..0a3b8223c87a 100644 --- a/drivers/gpu/drm/drm_probe_helper.c +++ b/drivers/gpu/drm/drm_probe_helper.c @@ -304,6 +304,12 @@ drm_helper_probe_detect_ctx(struct drm_connector *connector, bool force) if (WARN_ON(ret < 0)) ret = connector_status_unknown; + if (connector->force == DRM_FORCE_ON || + connector->force == DRM_FORCE_ON_DIGITAL) + ret = connector_status_connected; + else if (connector->force == DRM_FORCE_OFF) + ret = connector_status_disconnected; + if (ret != connector->status) connector->epoch_counter += 1; @@ -346,6 +352,12 @@ drm_helper_probe_detect(struct drm_connector *connector, else ret = connector_status_connected; + if (connector->force == DRM_FORCE_ON || + connector->force == DRM_FORCE_ON_DIGITAL) + ret = connector_status_connected; + else if (connector->force == DRM_FORCE_OFF) + ret = connector_status_disconnected; + if (ret != connector->status) connector->epoch_counter += 1; -- 2.25.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] drm: override detected status for connectors which are forced on 2022-12-15 17:03 ` [PATCH] drm: override detected status " Michael Rodin @ 2022-12-22 17:40 ` Maxime Ripard 2023-01-20 9:03 ` Michael Rodin 0 siblings, 1 reply; 8+ messages in thread From: Maxime Ripard @ 2022-12-22 17:40 UTC (permalink / raw) To: Michael Rodin Cc: Maarten Lankhorst, Thomas Zimmermann, David Airlie, Daniel Vetter, Dave Airlie, Alex Deucher, dri-devel, linux-kernel, michael, erosca, laurent.pinchart [-- Attachment #1: Type: text/plain, Size: 1445 bytes --] Hi, On Thu, Dec 15, 2022 at 06:03:59PM +0100, Michael Rodin wrote: > The detected status of a connector should be ignored when a connector is > forced as hinted in the commit d50ba256b5f1 ("drm/kms: start > adding command line interface using fb."). One negative side effect of > not ignoring this is observed on the RCar3 SoCs which use the dw-hdmi > driver. It continues executing drm_helper_hpd_irq_event even if its > connector is forced to ON. As consequence drm_helper_hpd_irq_event calls > "detect" so the connector status is updated to "disconnected": > > [ 420.201527] [drm:drm_helper_hpd_irq_event] [CONNECTOR:76:HDMI-A-1] status updated from connected to disconnected > > This status is corrected by drm_helper_probe_single_connector_modes shortly > after this because this function checks if a connector is forced: > > [ 420.218703] [drm:drm_helper_probe_single_connector_modes] [CONNECTOR:76:HDMI-A-1] status updated from disconnected to connected > > To avoid similar issues this commit adapts functions which call "detect" > so they check additionally if a connector is forced and override the status > returned by "detect". > > Fixes: 816da85a0990 ("drm: handle HPD and polled connectors separately") > Signed-off-by: Michael Rodin <mrodin@de.adit-jv.com> As reported here, this breaks vc4, and probably i915: https://lore.kernel.org/dri-devel/20221107123657.24vbgep3jqeklb2s@houat/ Maxime [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm: override detected status for connectors which are forced on 2022-12-22 17:40 ` Maxime Ripard @ 2023-01-20 9:03 ` Michael Rodin 2023-01-27 13:57 ` Maxime Ripard 0 siblings, 1 reply; 8+ messages in thread From: Michael Rodin @ 2023-01-20 9:03 UTC (permalink / raw) To: Maxime Ripard Cc: Michael Rodin, Maarten Lankhorst, Thomas Zimmermann, David Airlie, Daniel Vetter, Dave Airlie, Alex Deucher, dri-devel, linux-kernel, michael, erosca, laurent.pinchart Hi Maxime, thank you for your feedback! On Thu, Dec 22, 2022 at 06:40:54PM +0100, Maxime Ripard wrote: > Hi, > > On Thu, Dec 15, 2022 at 06:03:59PM +0100, Michael Rodin wrote: > > The detected status of a connector should be ignored when a connector is > > forced as hinted in the commit d50ba256b5f1 ("drm/kms: start > > adding command line interface using fb."). One negative side effect of > > not ignoring this is observed on the RCar3 SoCs which use the dw-hdmi > > driver. It continues executing drm_helper_hpd_irq_event even if its > > connector is forced to ON. As consequence drm_helper_hpd_irq_event calls > > "detect" so the connector status is updated to "disconnected": > > > > [ 420.201527] [drm:drm_helper_hpd_irq_event] [CONNECTOR:76:HDMI-A-1] status updated from connected to disconnected > > > > This status is corrected by drm_helper_probe_single_connector_modes shortly > > after this because this function checks if a connector is forced: > > > > [ 420.218703] [drm:drm_helper_probe_single_connector_modes] [CONNECTOR:76:HDMI-A-1] status updated from disconnected to connected > > > > To avoid similar issues this commit adapts functions which call "detect" > > so they check additionally if a connector is forced and override the status > > returned by "detect". > > > > Fixes: 816da85a0990 ("drm: handle HPD and polled connectors separately") > > Signed-off-by: Michael Rodin <mrodin@de.adit-jv.com> > > As reported here, this breaks vc4, and probably i915: > https://lore.kernel.org/dri-devel/20221107123657.24vbgep3jqeklb2s@houat/ > > Maxime My understanding from [1,2] was that the way to avoid such regressions is to make sure that the "detect" callbacks of connector drivers are always called even if a connector is forced. This is what I have implemented in my second patch where "detect" is called first and then the return value is adjusted based on the "force" status. If my understanding was wrong, I would very much appreciate if you could give me some hints for the implementation of an acceptable solution. Maybe it would be safer to simply avoid calling drm_helper_hpd_irq_event in the dw-hdmi driver when the connector is forced like mentioned by Laurent [3]? Although this would fix this global problem only for one particular driver. [1] https://lore.kernel.org/dri-devel/20221107123657.24vbgep3jqeklb2s@houat/ [2] https://lore.kernel.org/dri-devel/20221107122504.tejlb24bjbaxw5t6@houat/ [3] https://lore.kernel.org/dri-devel/YzYrNJbfGcch1UtX@pendragon.ideasonboard.com/ -- Best Regards, Michael ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm: override detected status for connectors which are forced on 2023-01-20 9:03 ` Michael Rodin @ 2023-01-27 13:57 ` Maxime Ripard 0 siblings, 0 replies; 8+ messages in thread From: Maxime Ripard @ 2023-01-27 13:57 UTC (permalink / raw) To: David Airlie, Daniel Vetter, Michael Rodin, Hans Verkuil, Thomas Zimmermann, Dom Cobley Cc: Maarten Lankhorst, Dave Airlie, Alex Deucher, dri-devel, linux-kernel, michael, erosca, Laurent Pinchart [-- Attachment #1: Type: text/plain, Size: 4626 bytes --] Hi Michael, all, On Fri, Jan 20, 2023 at 10:03:48AM +0100, Michael Rodin wrote: > Hi Maxime, > > thank you for your feedback! > On Thu, Dec 22, 2022 at 06:40:54PM +0100, Maxime Ripard wrote: > > Hi, > > > > On Thu, Dec 15, 2022 at 06:03:59PM +0100, Michael Rodin wrote: > > > The detected status of a connector should be ignored when a connector is > > > forced as hinted in the commit d50ba256b5f1 ("drm/kms: start > > > adding command line interface using fb."). One negative side effect of > > > not ignoring this is observed on the RCar3 SoCs which use the dw-hdmi > > > driver. It continues executing drm_helper_hpd_irq_event even if its > > > connector is forced to ON. As consequence drm_helper_hpd_irq_event calls > > > "detect" so the connector status is updated to "disconnected": > > > > > > [ 420.201527] [drm:drm_helper_hpd_irq_event] [CONNECTOR:76:HDMI-A-1] status updated from connected to disconnected > > > > > > This status is corrected by drm_helper_probe_single_connector_modes shortly > > > after this because this function checks if a connector is forced: > > > > > > [ 420.218703] [drm:drm_helper_probe_single_connector_modes] [CONNECTOR:76:HDMI-A-1] status updated from disconnected to connected > > > > > > To avoid similar issues this commit adapts functions which call "detect" > > > so they check additionally if a connector is forced and override the status > > > returned by "detect". > > > > > > Fixes: 816da85a0990 ("drm: handle HPD and polled connectors separately") > > > Signed-off-by: Michael Rodin <mrodin@de.adit-jv.com> > > > > As reported here, this breaks vc4, and probably i915: > > https://lore.kernel.org/dri-devel/20221107123657.24vbgep3jqeklb2s@houat/ > > > > Maxime > > My understanding from [1,2] was that the way to avoid such regressions is > to make sure that the "detect" callbacks of connector drivers are always > called even if a connector is forced. This is what I have implemented in my > second patch where "detect" is called first and then the return value is > adjusted based on the "force" status. If my understanding was wrong, I > would very much appreciate if you could give me some hints for the > implementation of an acceptable solution. Ah, sorry, you're right. I was confused since you didn't mention it was a new version, and didn't provide a changelog, I just assumed you resent the same patch. In the time between, we also got a report for the RaspberryPi that the behaviour is also broken on CEC: https://github.com/raspberrypi/linux/pull/5052 If we get back to the problem we're trying to solve, it means that if nothing is provided on the command line, we should rely on the polling or IRQ based detection that will call detect on a regular basis. The current detect side effects (for HDMI) are that: * the CEC address will be invalidated if it's disconnected, and set if it's connected. * if the scrambler was active, we re-enable the HDMI scrambler If we want to force the connector to be disconnected, everything is fine. If we want to force the connector on, then we should ignore the CEC invalidation, but should keep enabling the scrambler. And you want to avoid state transitions when the connector is forced, which also makes sense. I think we can get it to work by: - Merging your patch to call detect, but no matter the returned status, if it's forced to a state by the command line or some other mechanism, we return what was forced. - in the detect hook: * if connector->force is set to off, we just return connector_status_disconnected. * if connector->force is set to on, and if: + the actual status is that the display is disconnected, we return but don't invalidate the CEC address. + the actual status is that the display is connected, we setup the scrambler again if needed, and set the CEC address. So in addition to your patch, a skeleton detect hook would be something like: static int detect(struct drm_connector *connector, bool force) { if (connector->force == DRM_FORCE_OFF) return connector_status_disconnected; status = /* whatever is needed to fetch the status from the hardware */; if (status == connector_status_disconnected) { if (connector->force == DRM_FORCE_ON) return connector_status_connected; cec_phys_addr_invalidate(vc4_hdmi->cec_adap); } edid = drm_get_edid(...); cec_s_phys_addr_from_edid(..., edid); reset_scrambler(); return status; } Does that make sense? Maxime [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2023-01-27 13:58 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-08-26 9:11 [PATCH] drm: do not call detect for connectors which are forced on Michael Rodin 2022-09-29 23:33 ` Laurent Pinchart 2022-11-07 12:25 ` Maxime Ripard 2022-11-07 12:36 ` Maxime Ripard 2022-12-15 17:03 ` [PATCH] drm: override detected status " Michael Rodin 2022-12-22 17:40 ` Maxime Ripard 2023-01-20 9:03 ` Michael Rodin 2023-01-27 13:57 ` Maxime Ripard
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).