Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:6 as permitted sender) client-ip=2620:137:e000::3:6;
MIME-Version: 1.0
In-Reply-To: <65566a68-3510-2e5f-7d57-e4dba08c008c@quicinc.com>
References: <1694813901-26952-1-git-send-email-quic_khsieh@quicinc.com>
 <1694813901-26952-7-git-send-email-quic_khsieh@quicinc.com>
 <CAA8EJpqPXoFX4LXyXYgfh07Vpxg-KgD8VBR6x5bXf4GOJmbOtw@mail.gmail.com>
 <2f98d5f1-57c1-d9fe-cb1c-b975db057287@quicinc.com> <CAA8EJpr2wRq6Txi7YAQpJKa_9UGqH_nmHzvVOaAPkwOrtDg4Tw@mail.gmail.com>
 <CAE-0n53dqHONzMTd_ZC-fKWTzDVq6Wqwo4OFZMUcghZ5SD5RhA@mail.gmail.com> <65566a68-3510-2e5f-7d57-e4dba08c008c@quicinc.com>
From:   Stephen Boyd <swboyd@chromium.org>
User-Agent: alot/0.10
Date:   Wed, 27 Sep 2023 16:41:29 -0500
Message-ID: <CAE-0n51Ep4zBx6bswL4Yb+F0+8dW+L2kaKQaZBi-91jsVqm_9Q@mail.gmail.com>
Subject: Re: [PATCH v3 6/7] drm/msm/dp: add pm_runtime_force_suspend()/resume()
To:     Abhinav Kumar <quic_abhinavk@quicinc.com>,
        Dmitry Baryshkov <dmitry.baryshkov@linaro.org>,
        Kuogee Hsieh <quic_khsieh@quicinc.com>
Cc:     dri-devel@lists.freedesktop.org, robdclark@gmail.com,
        sean@poorly.run, dianders@chromium.org, vkoul@kernel.org,
        daniel@ffwll.ch, airlied@gmail.com, agross@kernel.org,
        andersson@kernel.org, quic_jesszhan@quicinc.com,
        quic_sbillaka@quicinc.com, marijn.suijten@somainline.org,
        freedreno@lists.freedesktop.org, linux-arm-msm@vger.kernel.org,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Quoting Abhinav Kumar (2023-09-22 18:35:27)
> On 9/22/2023 2:54 PM, Stephen Boyd wrote:
> > Quoting Dmitry Baryshkov (2023-09-19 02:50:12)
> >>
> >> This should be hpd_notify, who starts link training, not some event.
> >
> > I think this driver should train the link during atomic_enable(), not
> > hpd_notify() (or directly from the irq handler). The drm_bridge_funcs
> > talk a bit about when the clocks and timing signals are supposed to be
> > enabled. For example, struct drm_bridge_funcs::atomic_pre_enable() says
> > the "display pipe (i.e.  clocks and timing signals) feeding this bridge
> > will not yet be running when this callback is called". And struct
> > drm_bridge_funcs::atomic_enable() says "this callback must enable the
> > display link feeding the next bridge in the chain if there is one."
> >
> > That looks to me like link training, i.e. the display link, should
> > happen in the enable path and not hpd_notify. It looks like link
> > training could fail, but when that happens I believe the driver should
> > call drm_connector_set_link_status_property() with
> > DRM_MODE_LINK_STATUS_BAD. The two callers of that which exist in the
> > tree also call drm_kms_helper_hotplug_event() or
> > drm_kms_helper_connector_hotplug_event() after updating the link so that
> > userspace knows to try again.
> >
> > It would be nice if there was some drm_bridge_set_link_status_bad() API
> > that bridge drivers could use to signal that the link status is bad and
> > call the hotplug helper. Maybe it could also record some diagnostics
> > about which bridge failed to setup the link and stop the atomic_enable()
> > chain for that connector.
>
> Doing link training when we get hpd instead of atomic_enable() is a
> design choice we have been following for a while because for the case
> when link training fails in atomic_enable() and setting the link status
> property as you mentioned, the compositor needs to be able to handle
> that and also needs to try with a different resolution or take some
> other corrective action. We have seen many compositors not able to
> handle this complexity.

The chrome compositor handles this case[1]. If the link status is set to
bad and there are non-zero number of modes on a connected connector it
resets the status to good to try again.

> So the design sends the hotplug to usermode only
> after link training succeeds.

I suspect this is why my trogdor device turns off after rebooting when I
apply a ChromeOS update with the lid closed and DP connected. Userspace
wants to know that a display is connected, but this driver is still link
training by the time userspace boots up so we don't see any drm
connector indicating status is connected. No drm connectors connected
means the OS should shutdown.

>
> I do not think we should change this design unless prototyped with an
> existing compositor such as chrome or android at this point.

Is this driver used with android?

[1] https://source.chromium.org/chromium/chromium/src/+/main:ui/ozone/platform/drm/gpu/hardware_display_plane_manager_atomic.cc;l=114;drc=67520ac99db89395b10f2ab728b540eef0da8292