| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAJMQK-it9YMod4rHKnACq4O-iaGieK2SN4x5vQ018CghsA631A@mail.gmail.com>
Date: Wed, 14 Feb 2024 14:24:53 +0800
From: Hsin-Yi Wang <hsinyi@...omium.org>
To: Douglas Anderson <dianders@...omium.org>
Cc: dri-devel@...ts.freedesktop.org, eizan@...omium.org,
Ankit Nautiyal <ankit.k.nautiyal@...el.com>, Daniel Vetter <daniel@...ll.ch>,
David Airlie <airlied@...il.com>, Heiner Kallweit <hkallweit1@...il.com>,
Imre Deak <imre.deak@...el.com>, Jani Nikula <jani.nikula@...el.com>,
Jessica Zhang <quic_jesszhan@...cinc.com>,
Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, Maxime Ripard <mripard@...nel.org>,
Neil Armstrong <neil.armstrong@...aro.org>, Sam Ravnborg <sam@...nborg.org>,
Stanislav Lisovskiy <stanislav.lisovskiy@...el.com>, Thomas Zimmermann <tzimmermann@...e.de>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] drm/dp: Don't attempt AUX transfers when eDP panels are
not powered
On Wed, Feb 14, 2024 at 2:23 PM Douglas Anderson <dianders@...omiumorg> wrote:
>
> If an eDP panel is not powered on then any attempts to talk to it over
> the DP AUX channel will timeout. Unfortunately these attempts may be
> quite slow. Userspace can initiate these attempts either via a
> /dev/drm_dp_auxN device or via the created i2c device.
>
> Making the DP AUX drivers timeout faster is a difficult proposition.
> In theory we could just poll the panel's HPD line in the AUX transfer
> function and immediately return an error there. However, this is
> easier said than done. For one thing, there's no hard requirement to
> hook the HPD line up for eDP panels and it's OK to just delay a fixed
> amount. For another thing, the HPD line may not be fast to probe. On
> parade-ps8640 we need to wait for the bridge chip's firmware to boot
> before we can get the HPD line and this is a slow process.
>
> The fact that the transfers are taking so long to timeout is causing
> real problems. The open source fwupd daemon sometimes scans DP busses
> looking for devices whose firmware need updating. If it happens to
> scan while a panel is turned off this scan can take a long time. The
> fwupd daemon could try to be smarter and only scan when eDP panels are
> turned on, but we can also improve the behavior in the kernel.
>
> Let's let eDP panels drivers specify that a panel is turned off and
> then modify the common AUX transfer code not to attempt a transfer in
> this case.
>
> Signed-off-by: Douglas Anderson <dianders@...omium.org>
> ---
Reviewed-by: Hsin-Yi Wang <hsinyi@...omium.org>
>
> drivers/gpu/drm/display/drm_dp_helper.c | 35 +++++++++++++++++++
> drivers/gpu/drm/panel/panel-edp.c | 3 ++
> .../gpu/drm/panel/panel-samsung-atna33xc20.c | 2 ++
> include/drm/display/drm_dp_helper.h | 6 ++++
> 4 files changed, 46 insertions(+)
>
> diff --git a/drivers/gpu/drm/display/drm_dp_helper.c b/drivers/gpu/drm/display/drm_dp_helper.c
> index b1ca3a1100da..6fa705d82c8f 100644
> --- a/drivers/gpu/drm/display/drm_dp_helper.c
> +++ b/drivers/gpu/drm/display/drm_dp_helper.c
> @@ -532,6 +532,15 @@ static int drm_dp_dpcd_access(struct drm_dp_aux *aux, u8 request,
>
> mutex_lock(&aux->hw_mutex);
>
> + /*
> + * If the device attached to the aux bus is powered down then there's
> + * no reason to attempt a transfer. Error out immediately.
> + */
> + if (aux->powered_down) {
> + ret = -EBUSY;
> + goto unlock;
> + }
> +
> /*
> * The specification doesn't give any recommendation on how often to
> * retry native transactions. We used to retry 7 times like for
> @@ -599,6 +608,29 @@ int drm_dp_dpcd_probe(struct drm_dp_aux *aux, unsigned int offset)
> }
> EXPORT_SYMBOL(drm_dp_dpcd_probe);
>
> +/**
> + * drm_dp_dpcd_set_powered() - Set whether the DP device is powered
> + * @aux: DisplayPort AUX channel; for convenience it's OK to pass NULL here
> + * and the function will be a no-op.
> + * @powered: true if powered; false if not
> + *
> + * If the endpoint device on the DP AUX bus is known to be powered down
> + * then this function can be called to make future transfers fail immediately
> + * instead of needing to time out.
> + *
> + * If this function is never called then a device defaults to being powered.
> + */
> +void drm_dp_dpcd_set_powered(struct drm_dp_aux *aux, bool powered)
> +{
> + if (!aux)
> + return;
> +
> + mutex_lock(&aux->hw_mutex);
> + aux->powered_down = !powered;
> + mutex_unlock(&aux->hw_mutex);
> +}
> +EXPORT_SYMBOL(drm_dp_dpcd_set_powered);
> +
> /**
> * drm_dp_dpcd_read() - read a series of bytes from the DPCD
> * @aux: DisplayPort AUX channel (SST or MST)
> @@ -1858,6 +1890,9 @@ static int drm_dp_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs,
> struct drm_dp_aux_msg msg;
> int err = 0;
>
> + if (aux->powered_down)
> + return -EBUSY;
> +
> dp_aux_i2c_transfer_size = clamp(dp_aux_i2c_transfer_size, 1, DP_AUX_MAX_PAYLOAD_BYTES);
>
> memset(&msg, 0, sizeof(msg));
> diff --git a/drivers/gpu/drm/panel/panel-edp.c b/drivers/gpu/drm/panel/panel-edp.c
> index 7d556b1bfa82..d2a4e514d8fd 100644
> --- a/drivers/gpu/drm/panel/panel-edp.c
> +++ b/drivers/gpu/drm/panel/panel-edp.c
> @@ -413,6 +413,7 @@ static int panel_edp_suspend(struct device *dev)
> {
> struct panel_edp *p = dev_get_drvdata(dev);
>
> + drm_dp_dpcd_set_powered(p->aux, false);
> gpiod_set_value_cansleep(p->enable_gpio, 0);
> regulator_disable(p->supply);
> p->unprepared_time = ktime_get_boottime();
> @@ -469,6 +470,7 @@ static int panel_edp_prepare_once(struct panel_edp *p)
> }
>
> gpiod_set_value_cansleep(p->enable_gpio, 1);
> + drm_dp_dpcd_set_powered(p->aux, true);
>
> p->powered_on_time = ktime_get_boottime();
>
> @@ -507,6 +509,7 @@ static int panel_edp_prepare_once(struct panel_edp *p)
> return 0;
>
> error:
> + drm_dp_dpcd_set_powered(p->aux, false);
> gpiod_set_value_cansleep(p->enable_gpio, 0);
> regulator_disable(p->supply);
> p->unprepared_time = ktime_get_boottime();
> diff --git a/drivers/gpu/drm/panel/panel-samsung-atna33xc20.c b/drivers/gpu/drm/panel/panel-samsung-atna33xc20.c
> index 5703f4712d96..76c2a8f6718c 100644
> --- a/drivers/gpu/drm/panel/panel-samsung-atna33xc20.c
> +++ b/drivers/gpu/drm/panel/panel-samsung-atna33xc20.c
> @@ -72,6 +72,7 @@ static int atana33xc20_suspend(struct device *dev)
> if (p->el3_was_on)
> atana33xc20_wait(p->el_on3_off_time, 150);
>
> + drm_dp_dpcd_set_powered(p->aux, false);
> ret = regulator_disable(p->supply);
> if (ret)
> return ret;
> @@ -93,6 +94,7 @@ static int atana33xc20_resume(struct device *dev)
> ret = regulator_enable(p->supply);
> if (ret)
> return ret;
> + drm_dp_dpcd_set_powered(p->aux, true);
> p->powered_on_time = ktime_get_boottime();
>
> if (p->no_hpd) {
> diff --git a/include/drm/display/drm_dp_helper.h b/include/drm/display/drm_dp_helper.h
> index 863b2e7add29..472359a9d675 100644
> --- a/include/drm/display/drm_dp_helper.h
> +++ b/include/drm/display/drm_dp_helper.h
> @@ -463,9 +463,15 @@ struct drm_dp_aux {
> * @is_remote: Is this AUX CH actually using sideband messaging.
> */
> bool is_remote;
> +
> + /**
> + * @powered_down: If true then the remote endpoint is powered down.
> + */
> + bool powered_down;
> };
>
> int drm_dp_dpcd_probe(struct drm_dp_aux *aux, unsigned int offset);
> +void drm_dp_dpcd_set_powered(struct drm_dp_aux *aux, bool powered);
> ssize_t drm_dp_dpcd_read(struct drm_dp_aux *aux, unsigned int offset,
> void *buffer, size_t size);
> ssize_t drm_dp_dpcd_write(struct drm_dp_aux *aux, unsigned int offset,
> --
> 2.43.0.594.gd9cf4e227d-goog
>
Powered by blists - more mailing lists