[PATCH] drm: do not call detect for connectors which are forced on

[PATCH] drm: do not call detect for connectors which are forced on

[Michael Rodin]

"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

Re: [PATCH] drm: do not call detect for connectors which are forced on

[Laurent Pinchart]

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

Re: [PATCH] drm: do not call detect for connectors which are forced on

[Maxime Ripard]

[-- 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 --]

Re: [PATCH] drm: do not call detect for connectors which are forced on

[Maxime Ripard]

[-- 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 --]

[PATCH] drm: override detected status for connectors which are forced on

[Michael Rodin]

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

Re: [PATCH] drm: override detected status for connectors which are forced on

[Maxime Ripard]

[-- 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 --]

Re: [PATCH] drm: override detected status for connectors which are forced on

[Michael Rodin]

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

Re: [PATCH] drm: override detected status for connectors which are forced on

[Maxime Ripard]

[-- 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 --]