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