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

[Maxime Ripard]

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@xxxxxxxxxxxxxx>
> > 
> > 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: signature.asc
Description: PGP signature