| 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: <CAD=FV=WiORm+rawNZoinXFZWdt_yqKQfTFWCiBcJFyJKXj4spQ@mail.gmail.com>
Date: Fri, 29 Aug 2025 09:40:26 -0700
From: Doug Anderson <dianders@...omium.org>
To: John Ripple <john.ripple@...sight.com>
Cc: andrzej.hajda@...el.com, neil.armstrong@...aro.org, rfoss@...nel.org,
maarten.lankhorst@...ux.intel.com, mripard@...nel.org, tzimmermann@...e.de,
airlied@...il.com, simona@...ll.ch, Laurent.pinchart@...asonboard.com,
jonas@...boo.se, jernej.skrabec@...il.com, dri-devel@...ts.freedesktop.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] drm/bridge: ti-sn65dsi86: Add support for DisplayPort
mode with HPD
Hi,
On Wed, Aug 20, 2025 at 8:24 AM John Ripple <john.ripple@...sight.com> wrote:
>
> Add support for DisplayPort to the bridge, which entails the following:
> - Register the proper connector type;
> - Get and use an interrupt for HPD;
> - Properly clear all status bits in the interrupt handler;
> - Implement bridge and connector detection;
> - Report DSI channel errors;
> - Report Display Port errors;
> - Disable runtime pm entirely;
>
> Signed-off-by: John Ripple <john.ripple@...sight.com>
> ---
> drivers/gpu/drm/bridge/ti-sn65dsi86.c | 287 +++++++++++++++++++++++++-
> 1 file changed, 281 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index 464390372b34..75f9be347b41 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -37,6 +37,8 @@
>
> #define SN_DEVICE_ID_REGS 0x00 /* up to 0x07 */
> #define SN_DEVICE_REV_REG 0x08
> +#define SN_RESET_REG 0x09
> +#define SOFT_RESET BIT(0)
> #define SN_DPPLL_SRC_REG 0x0A
> #define DPPLL_CLK_SRC_DSICLK BIT(0)
> #define REFCLK_FREQ_MASK GENMASK(3, 1)
> @@ -48,7 +50,9 @@
> #define CHA_DSI_LANES(x) ((x) << 3)
> #define SN_DSIA_CLK_FREQ_REG 0x12
> #define SN_CHA_ACTIVE_LINE_LENGTH_LOW_REG 0x20
> +#define SN_CHA_ACTIVE_LINE_LENGTH_HIGH_REG 0x21
This and many other #defines you added aren't actually used in your patch.
One can make an argument that adding #defines for all the registers
and bits in the datasheet (even if they're not used) is a good thing.
...but one can also make the argument that we should avoid cluttering
the driver with extra #defines until they're needed, especially since
the datasheet for this part is public. We can certainly have that
debate if you want, but let's please do it in a separate patch. Adding
all of these defines is not required for your HPD case so shouldn't be
in the HPD patch.
> @@ -189,6 +305,7 @@ struct ti_sn65dsi86 {
> int dp_lanes;
> u8 ln_assign;
> u8 ln_polrs;
> + bool no_hpd;
nit: in my editor the indentation seems wrong here.
> @@ -987,6 +1104,11 @@ static int ti_sn_link_training(struct ti_sn65dsi86 *pdata, int dp_rate_idx,
> int ret;
> int i;
>
> + /*
> + * DP data rate and lanes number will be set by the bridge by writing
> + * to DP_LINK_BW_SET and DP_LANE_COUNT_SET.
> + */
> +
This comment seems unrelated to your patch. If we want to add it,
please do so in a separate patch.
Also, please match the names used elsewhere in the file. Searching the
file for DP_LINK_BW_SET and DP_LANE_COUNT_SET shows no hits.
> @@ -1105,7 +1227,10 @@ static void ti_sn_bridge_atomic_enable(struct drm_bridge *bridge,
>
> valid_rates = ti_sn_bridge_read_valid_rates(pdata);
>
> - /* Train until we run out of rates */
> + /*
> + * Train until we run out of rates. Start with the lowest possible rate
> + * and move up in order to select the lowest working functioning point.
> + */
Similar to the last comment. If we want to improve comments it should
be done in a separate patch.
> for (dp_rate_idx = ti_sn_bridge_calc_min_dp_rate_idx(pdata, state, bpp);
> dp_rate_idx < ARRAY_SIZE(ti_sn_bridge_dp_rate_lut);
> dp_rate_idx++) {
> @@ -1116,9 +1241,13 @@ static void ti_sn_bridge_atomic_enable(struct drm_bridge *bridge,
> if (!ret)
> break;
> }
> - if (ret) {
> + if (ret || dp_rate_idx == ARRAY_SIZE(ti_sn_bridge_dp_rate_lut)) {
Why did you need to change? We'll always run through the above loop at
least once, right? That means we'll always set "ret"
> DRM_DEV_ERROR(pdata->dev, "%s (%d)\n", last_err_str, ret);
> return;
> + } else {
> + DRM_DEV_INFO(pdata->dev,
> + "Link training selected rate: %u MHz\n",
> + ti_sn_bridge_dp_rate_lut[dp_rate_idx]);
Again, this should be in a separate patch.
Also: this is logspam. If there's really a reason we need to logspam
every time we link train then that reason needs to be justified. IMO
it would be fine to make this a "debug" level log, but I'd be against
leaving it at INFO level.
> @@ -1298,6 +1427,69 @@ static int ti_sn_bridge_parse_dsi_host(struct ti_sn65dsi86 *pdata)
> return 0;
> }
>
> +static irqreturn_t ti_sn_bridge_interrupt(int irq, void *private)
> +{
> + struct ti_sn65dsi86 *pdata = private;
> + struct drm_device *dev = pdata->bridge.dev;
> + u32 status = 0;
> + bool hpd_event = false;
> +
> + regmap_read(pdata->regmap, SN_IRQ_STATUS_REG, &status);
Please check the return code from regmap_read(), since i2c transfers
can fail. I know this driver isn't terribly good about it with
existing code, but that doesn't mean we should keep being bad about it
with new code.
> + if (status & (HPD_REMOVAL_STATUS | HPD_INSERTION_STATUS))
> + hpd_event = true;
hpd_event = status & (HPD_REMOVAL_STATUS | HPD_INSERTION_STATUS);
> + /*
> + * Writing back the status register to acknowledge the IRQ apparently
> + * needs to take place right after reading it or the bridge will get
> + * confused and fail to report subsequent IRQs.
> + */
Really? Is this documented?
In general for edge triggered interrupts you always want to
acknowledge before you actually act on them. Is it just that, or does
this specifically have to be before other stuff below?
> + if (status)
> + drm_err(dev, "(SN_IRQ_STATUS_REG = %#x)\n", status);
Getting interrupts is an "error" ? That doesn't seem right.
> + regmap_write(pdata->regmap, SN_IRQ_STATUS_REG, status);
Shouldn't it only write if non-zero?
> + regmap_read(pdata->regmap, SN_CHA_IRQ_STATUS0_REG, &status);
> + if (status)
> + drm_err(dev, "DSI CHA error reported (status0 = %#x)\n", status);
> + regmap_write(pdata->regmap, SN_CHA_IRQ_STATUS0_REG, status);
> + if (status)
> + regmap_write(pdata->regmap, SN_RESET_REG, SOFT_RESET);
> +
> + regmap_read(pdata->regmap, SN_CHA_IRQ_STATUS1_REG, &status);
> + if (status)
> + drm_err(dev, "DSI CHA error reported (status1 = %#x)\n", status);
> + regmap_write(pdata->regmap, SN_CHA_IRQ_STATUS1_REG, status);
> + if (status)
> + regmap_write(pdata->regmap, SN_RESET_REG, SOFT_RESET);
> +
> + /* Dirty hack to reset the soft if any error occurs on the DP side */
> + regmap_read(pdata->regmap, SN_IRQ_EVENTS_DPTL_REG_1, &status);
> + if (status)
> + drm_err(dev, "(SN_IRQ_EVENTS_DPTL_REG_1 = %#x)\n", status);
> + regmap_write(pdata->regmap, SN_IRQ_EVENTS_DPTL_REG_1, status);
> + if (status)
> + regmap_write(pdata->regmap, SN_RESET_REG, SOFT_RESET);
> +
> + regmap_read(pdata->regmap, SN_IRQ_EVENTS_DPTL_REG_2, &status);
> + if (status)
> + drm_err(dev, "(SN_IRQ_EVENTS_DPTL_REG_2 = %#x)\n", status);
> + regmap_write(pdata->regmap, SN_IRQ_EVENTS_DPTL_REG_2, status);
> + if (status)
> + regmap_write(pdata->regmap, SN_RESET_REG, SOFT_RESET);
> +
> + regmap_read(pdata->regmap, SN_IRQ_LT, &status);
> + if (status)
> + drm_err(dev, "(SN_IRQ_LT = %#x)\n", status);
> + regmap_write(pdata->regmap, SN_IRQ_LT, status);
> + if (status)
> + regmap_write(pdata->regmap, SN_RESET_REG, SOFT_RESET);
If we want to start handling error interrupts, let's do it in a
separate patch please. Then we can talk about what kind of value this
brings and how you've tested it.
> @@ -1335,9 +1527,48 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev,
> * for eDP.
> */
> mutex_lock(&pdata->comms_mutex);
> - if (pdata->comms_enabled)
> + if (pdata->comms_enabled) {
> + /* Enable HPD and PLL events. */
> + regmap_write(pdata->regmap, SN_IRQ_EVENTS_EN_REG,
> + PLL_UNLOCK_EN |
> + HPD_REPLUG_EN |
> + HPD_REMOVAL_EN |
> + HPD_INSERTION_EN |
> + IRQ_HPD_EN);
> +
> + /* Enable DSI CHA error reporting events. */
> + regmap_write(pdata->regmap, SN_CHA_IRQ_EN0_REG,
> + CHA_CONTENTION_DET_EN |
> + CHA_FALSE_CTRL_EN |
> + CHA_TIMEOUT_EN |
> + CHA_LP_TX_SYNC_EN |
> + CHA_ESC_ENTRY_EN |
> + CHA_EOT_SYNC_EN |
> + CHA_SOT_SYNC_EN |
> + CHA_SOT_BIT_EN);
> +
> + regmap_write(pdata->regmap, SN_CHA_IRQ_EN1_REG,
> + CHA_DSI_PROTOCOL_EN |
> + CHA_INVALID_LENGTH_EN |
> + CHA_DATATYPE_EN |
> + CHA_CHECKSUM_EN |
> + CHA_UNC_ECC_EN |
> + CHA_COR_ECC_EN);
> +
> + /* Disable DSI CHB error reporting events. */
> + regmap_write(pdata->regmap, SN_CHB_IRQ_EN0_REG, 0);
> + regmap_write(pdata->regmap, SN_CHB_IRQ_EN1_REG, 0);
> +
> regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG,
> - HPD_DISABLE, 0);
> + HPD_DISABLE, 0);
> +
> + /* Enable DisplayPort error reporting events. */
> + regmap_write(pdata->regmap, SN_DPTL_IRQ_EN0_REG, 0xFF);
> + regmap_write(pdata->regmap, SN_DPTL_IRQ_EN1_REG, 0xFF);
> +
> + regmap_update_bits(pdata->regmap, SN_IRQ_EN_REG, IRQ_EN,
As per previous comment, enabling the error interrupts should be in a
separate patch.
> @@ -1884,8 +2115,12 @@ static inline void ti_sn_gpio_unregister(void) {}
>
> static void ti_sn65dsi86_runtime_disable(void *data)
> {
> - pm_runtime_dont_use_autosuspend(data);
> - pm_runtime_disable(data);
> + if (pm_runtime_enabled(data)) {
> + pm_runtime_dont_use_autosuspend(data);
> + pm_runtime_disable(data);
> + } else {
> + ti_sn65dsi86_suspend(data);
> + }
See below--we should leverage the existing code to keep PM Runtime on
when HPD is used.
> @@ -1943,6 +2178,7 @@ static int ti_sn65dsi86_probe(struct i2c_client *client)
> return dev_err_probe(dev, PTR_ERR(pdata->refclk),
> "failed to get reference clock\n");
>
> + pdata->no_hpd = of_property_read_bool(pdata->host_node, "no-hpd");
> pm_runtime_enable(dev);
> pm_runtime_set_autosuspend_delay(pdata->dev, 500);
> pm_runtime_use_autosuspend(pdata->dev);
> @@ -1950,6 +2186,45 @@ static int ti_sn65dsi86_probe(struct i2c_client *client)
> if (ret)
> return ret;
>
> + if (client->irq && !pdata->no_hpd) {
> + enum drm_connector_status status;
> +
> + pm_runtime_disable(pdata->dev);
You can't permanently disable pm_runtime (AKA always keep things
powered on) based on just having an IRQ and lacking "no-hpd".
Instead, this decision needs to be made based on "DP" vs. "eDP". When
using the bridge in "eDP" mode you still want to be able to power the
bridge down even if interrupts and HPD are hooked up. This is because
in the "eDP" case you always just assume that the panel is there and
you don't need to detect it. This allows you to go into a lower power
state when the eDP panel is turned off because you can power the
bridge off.
We also already handle powering the bridge on in that case. See commit
55e8ff842051 ("drm/bridge: ti-sn65dsi86: Add HPD for DisplayPort
connector type"). You can see there that when
ti_sn_bridge_hpd_enable() is called we power the bridge on and it will
stay on.
What you should be doing is hooking into the ti_sn_bridge_hpd_enable()
function. When you see that then you should enable the interrupt. IMO
in probe you could still request the HPD interrupt. One way to do this
would be to set the `SN_IRQ_EVENTS_EN_REG` to turn on the HPD
interrupts in ti_sn_bridge_hpd_enable() after grabbing the runtime PM
reference and turn them off in ti_sn_bridge_hpd_disable() before
dropping it.
-Doug
Powered by blists - more mailing lists