On Mon, Aug 29, 2022 at 03:47:27PM +0200, Maxime Ripard wrote:
> Our detect callback has a bunch of operations to perform depending on
> the current and last status of the connector, such a setting the CEC
> physical address or enabling the scrambling again.
>
> This is currently dealt with a bunch of if / else statetements that make
> it fairly difficult to read and extend.
>
> Let's move all that logic to a function of its own.
>
> Signed-off-by: Maxime Ripard <maxime@xxxxxxxxxx>
> ---
> drivers/gpu/drm/vc4/vc4_hdmi.c | 66 ++++++++++++++++++++++------------
> 1 file changed, 44 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> index b786645eaeb7..9fad57ebdd11 100644
> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> @@ -273,17 +273,53 @@ static void vc4_hdmi_cec_update_clk_div(struct vc4_hdmi *vc4_hdmi) {}
>
> static void vc4_hdmi_enable_scrambling(struct drm_encoder *encoder);
>
> +static void vc4_hdmi_handle_hotplug(struct vc4_hdmi *vc4_hdmi,
> + enum drm_connector_status status)
> +{
> + struct drm_connector *connector = &vc4_hdmi->connector;
> + struct edid *edid;
> +
> + /*
> + * NOTE: This function should really be called with
> + * vc4_hdmi->mutex held, but doing so results in reentrancy
> + * issues since cec_s_phys_addr_from_edid might call
> + * .adap_enable, which leads to that funtion being called with
> + * our mutex held.
> + *
> + * Concurrency isn't an issue at the moment since we don't share
> + * any state with any of the other frameworks so we can ignore
> + * the lock for now.
> + */
> +
> + if (status == connector->status)
> + return;
This looks to have a change in behaviour to not call
vc4_hdmi_enable_scrambling() unless a change in the
connector status was detected. The previous code called
it regarless.
When I was doing the i915 stuff I did have a sink that
left hpd asserted while turned off, and when powering
back up it instead generated a pulse on the hpd line.
Not sure if such a pulse is always slow enough that
you can reasonably guarantee a detection of both edges...
Apart from that (and the cec locking mess that I dared
not even look at) the rest of the series looks OK to me.
> +
> + if (status == connector_status_disconnected) {
> + cec_phys_addr_invalidate(vc4_hdmi->cec_adap);
> + return;
> + }
> +
> + edid = drm_get_edid(connector, vc4_hdmi->ddc);
> + if (!edid)
> + return;
> +
> + cec_s_phys_addr_from_edid(vc4_hdmi->cec_adap, edid);
> + kfree(edid);
> +
> + vc4_hdmi_enable_scrambling(&vc4_hdmi->encoder.base.base);
> +}
> +
> static enum drm_connector_status
> vc4_hdmi_connector_detect(struct drm_connector *connector, bool force)
> {
> struct vc4_hdmi *vc4_hdmi = connector_to_vc4_hdmi(connector);
> - bool connected = false;
> + enum drm_connector_status status = connector_status_disconnected;
>
> /*
> * NOTE: This function should really take vc4_hdmi->mutex, but
> * doing so results in reentrancy issues since
> - * cec_s_phys_addr_from_edid might call .adap_enable, which
> - * leads to that funtion being called with our mutex held.
> + * vc4_hdmi_handle_hotplug() can call into other functions that
> + * would take the mutex while it's held here.
> *
> * Concurrency isn't an issue at the moment since we don't share
> * any state with any of the other frameworks so we can ignore
> @@ -294,31 +330,17 @@ vc4_hdmi_connector_detect(struct drm_connector *connector, bool force)
>
> if (vc4_hdmi->hpd_gpio) {
> if (gpiod_get_value_cansleep(vc4_hdmi->hpd_gpio))
> - connected = true;
> + status = connector_status_connected;
> } else {
> if (vc4_hdmi->variant->hp_detect &&
> vc4_hdmi->variant->hp_detect(vc4_hdmi))
> - connected = true;
> + status = connector_status_connected;
> }
>
> - if (connected) {
> - if (connector->status != connector_status_connected) {
> - struct edid *edid = drm_get_edid(connector, vc4_hdmi->ddc);
> -
> - if (edid) {
> - cec_s_phys_addr_from_edid(vc4_hdmi->cec_adap, edid);
> - kfree(edid);
> - }
> - }
> -
> - vc4_hdmi_enable_scrambling(&vc4_hdmi->encoder.base);
> - pm_runtime_put(&vc4_hdmi->pdev->dev);
> - return connector_status_connected;
> - }
> -
> - cec_phys_addr_invalidate(vc4_hdmi->cec_adap);
> + vc4_hdmi_handle_hotplug(vc4_hdmi, status);
> pm_runtime_put(&vc4_hdmi->pdev->dev);
> - return connector_status_disconnected;
> +
> + return status;
> }
>
> static int vc4_hdmi_connector_get_modes(struct drm_connector *connector)
> --
> 2.37.1
--
Ville Syrjälä
Intel
