Re: [PATCH v3 12/17] clk: mediatek: mt8365-mm: fix DPI0 parent
Quoting Alexandre Mergnat (2024-04-18 07:17:00)
> To have a working display through DPI, a workaround has been
> implemented downstream to add "mm_dpi0_dpi0" and "dpi0_sel" to
> the DPI node. Shortly, that add an extra clock.
>
> It seems consistent to have the "dpi0_sel" as parent.
> Additionnaly, "vpll_dpix" isn't used/managed.
>
> Then, set the "mm_dpi0_dpi0" parent clock to "dpi0_sel".
>
> The new clock tree is:
>
> clk26m
> lvdspll
> lvdspll_X (2, 4, 8, 16)
> dpi0_sel
> mm_dpi0_dpi0
>
> Fixes: d46adccb7966 ("clk: mediatek: add driver for MT8365 SoC")
> Signed-off-by: Alexandre Mergnat
> ---
Applied to clk-next
Re: [PATCH v4 0/5] Pinephone video out fixes (flipping between two frames)
Quoting Frank Oltmanns (2024-04-03 08:31:47) > Dear clk and sunxi-ng maintainers, > > Patches 1-4 have been reviewed and there are no pending issues. If there > is something else you need me to do to get this applied, please let me > know. > I'm assuming sunxi maintainers will pick up the clk patches and send them to clk tree in a PR.
Re: [PATCH v3] phy/qcom-qmp-combo: propagate correct return value at phy_power_on()
Quoting Abhinav Kumar (2024-04-03 14:28:52) > > > On 4/3/2024 1:04 PM, Stephen Boyd wrote: > > Quoting Abhinav Kumar (2024-04-03 12:58:50) > >> > >> > >> On 4/3/2024 12:51 PM, Stephen Boyd wrote: > >>> Quoting Kuogee Hsieh (2024-03-29 12:50:35) > >>>> Currently qmp_combo_dp_power_on() always return 0 in regardless of > >>>> return value of cfg->configure_dp_phy(). This patch propagate > >>>> return value of cfg->configure_dp_phy() all the way back to caller. > >>> > >>> Is this found via code inspection or because the phy is failing to power > >>> on sometimes? I ask because I'm looking at a DP bug on Trogdor with > >>> chromeos' v6.6 based kernel and wondering if this is related. > >>> > >> > >> No, we actually hit an issue. This issue was originally reported as a > >> link training issue while bringing up DP on x1e80100. > >> > >> While debugging that we noticed that we should not have even proceeded > >> to link training because the PLL was not getting locked and it was > >> failing silently since there are no other error prints (and hence the > >> second part of the patch to improve the error logs), and we do not > >> return any error code from this driver, we could not catch the PLL > >> unlocked issue. > > > > Did link training succeed in that case and the screen was black? Also, > > did you figure out why the PLL failed to lock? I sometimes see reports > > now with an "Unexpected interrupt:" message from the DP driver and the > > interrupt is the PLL unlocked one (DP_INTR_PLL_UNLOCKED). > > > > No the link training had failed. > > Yes, root-cause was that the PLL registers were misconfigured in the > x1e80100 DP PHY for HBR2. Once we programmed the correct values it > worked. This was specific to x1e80100. Ah ok, so that's what the x1e80100 patch is about. > > Yes, Doug mentioned this to me on IRC that this issue is still there. > Surprising because I thought we had pushed > https://patchwork.freedesktop.org/patch/551847/ long ago and it was > fixed. It certainly did that time when I had tested this. I see it on v6.6 and it is also on v5.15.y (stable kernel) so that has been picked back. Somehow the aux interrupt is still happening though when the PLL isn't locked. Maybe that interrupt bit should be masked in most cases and only unmasked when something in the driver is going to care about it.
Re: [PATCH v3] phy/qcom-qmp-combo: propagate correct return value at phy_power_on()
Quoting Abhinav Kumar (2024-04-03 12:58:50) > > > On 4/3/2024 12:51 PM, Stephen Boyd wrote: > > Quoting Kuogee Hsieh (2024-03-29 12:50:35) > >> Currently qmp_combo_dp_power_on() always return 0 in regardless of > >> return value of cfg->configure_dp_phy(). This patch propagate > >> return value of cfg->configure_dp_phy() all the way back to caller. > > > > Is this found via code inspection or because the phy is failing to power > > on sometimes? I ask because I'm looking at a DP bug on Trogdor with > > chromeos' v6.6 based kernel and wondering if this is related. > > > > No, we actually hit an issue. This issue was originally reported as a > link training issue while bringing up DP on x1e80100. > > While debugging that we noticed that we should not have even proceeded > to link training because the PLL was not getting locked and it was > failing silently since there are no other error prints (and hence the > second part of the patch to improve the error logs), and we do not > return any error code from this driver, we could not catch the PLL > unlocked issue. Did link training succeed in that case and the screen was black? Also, did you figure out why the PLL failed to lock? I sometimes see reports now with an "Unexpected interrupt:" message from the DP driver and the interrupt is the PLL unlocked one (DP_INTR_PLL_UNLOCKED). > > > Also, is the call to phy_power_on() going to be checked in > > the DP driver? > > > > $ git grep -n phy_power_on -- drivers/gpu/drm/msm/dp/ > > drivers/gpu/drm/msm/dp/dp_ctrl.c:1453: phy_power_on(phy); > > Yes, this is a good point. We should also post the patch to add the > error checking in DP driver to fail if phy_power_on fails. Sounds great, thanks.
Re: [PATCH v3] phy/qcom-qmp-combo: propagate correct return value at phy_power_on()
Quoting Kuogee Hsieh (2024-03-29 12:50:35) > Currently qmp_combo_dp_power_on() always return 0 in regardless of > return value of cfg->configure_dp_phy(). This patch propagate > return value of cfg->configure_dp_phy() all the way back to caller. Is this found via code inspection or because the phy is failing to power on sometimes? I ask because I'm looking at a DP bug on Trogdor with chromeos' v6.6 based kernel and wondering if this is related. Also, is the call to phy_power_on() going to be checked in the DP driver? $ git grep -n phy_power_on -- drivers/gpu/drm/msm/dp/ drivers/gpu/drm/msm/dp/dp_ctrl.c:1453: phy_power_on(phy);
Re: [PATCH v1] phy/qcom-qmp-combo: propagate correct return value at phy_power_on()
Quoting Kuogee Hsieh (2024-03-28 14:07:15) > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c > b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c > index 36632fa..884973a 100644 > --- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c > +++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c > @@ -2754,6 +2754,7 @@ static int qmp_combo_dp_power_on(struct phy *phy) > const struct qmp_phy_cfg *cfg = qmp->cfg; > void __iomem *tx = qmp->dp_tx; > void __iomem *tx2 = qmp->dp_tx2; > + int ret = 0; Please don't initialize locals that are unused before being assigned unconditionally. > > mutex_lock(>phy_mutex); > > @@ -2766,11 +2767,11 @@ static int qmp_combo_dp_power_on(struct phy *phy) > cfg->configure_dp_tx(qmp); > > /* Configure link rate, swing, etc. */ > - cfg->configure_dp_phy(qmp); > + ret = cfg->configure_dp_phy(qmp); > > mutex_unlock(>phy_mutex); > > - return 0; > + return ret; > }
Re: [PATCH v1] drm/msm/dp: use dp_hpd_plug_handle() and dp_hpd_unplug_handle() directly
Quoting Abhinav Kumar (2024-03-28 13:24:34)
> + Johan and Bjorn for FYI
>
> On 3/28/2024 1:04 PM, Kuogee Hsieh wrote:
> > For internal HPD case, hpd_event_thread is created to handle HPD
> > interrupts generated by HPD block of DP controller. It converts
> > HPD interrupts into events and executed them under hpd_event_thread
> > context. For external HPD case, HPD events is delivered by way of
> > dp_bridge_hpd_notify() under thread context. Since they are executed
> > under thread context already, there is no reason to hand over those
> > events to hpd_event_thread. Hence dp_hpd_plug_handle() and
> > dp_hpd_unplug_hanlde() are called directly at dp_bridge_hpd_notify().
> >
> > Signed-off-by: Kuogee Hsieh
> > ---
> > drivers/gpu/drm/msm/dp/dp_display.c | 5 +++--
> > 1 file changed, 3 insertions(+), 2 deletions(-)
> >
>
> Fixes: 542b37efc20e ("drm/msm/dp: Implement hpd_notify()")
Is this a bug fix or an optimization? The commit text doesn't tell me.
>
> Looks right to me,
>
> Reviewed-by: Abhinav Kumar
[PATCH] drm/msm: Add newlines to some debug prints
These debug prints are missing newlines, leading to multiple messages
being printed on one line and hard to read logs. Add newlines to have
the debug prints on separate lines. The DBG macro used to add a newline,
but I missed that while migrating to drm_dbg wrappers.
Fixes: 7cb017db1896 ("drm/msm: Move FB debug prints to drm_dbg_state()")
Fixes: 721c6e0c6aed ("drm/msm: Move vblank debug prints to drm_dbg_vbl()")
Signed-off-by: Stephen Boyd
---
drivers/gpu/drm/msm/msm_fb.c | 6 +++---
drivers/gpu/drm/msm/msm_kms.c | 4 ++--
2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_fb.c b/drivers/gpu/drm/msm/msm_fb.c
index e3f61c39df69..80166f702a0d 100644
--- a/drivers/gpu/drm/msm/msm_fb.c
+++ b/drivers/gpu/drm/msm/msm_fb.c
@@ -89,7 +89,7 @@ int msm_framebuffer_prepare(struct drm_framebuffer *fb,
for (i = 0; i < n; i++) {
ret = msm_gem_get_and_pin_iova(fb->obj[i], aspace,
_fb->iova[i]);
- drm_dbg_state(fb->dev, "FB[%u]: iova[%d]: %08llx (%d)",
+ drm_dbg_state(fb->dev, "FB[%u]: iova[%d]: %08llx (%d)\n",
fb->base.id, i, msm_fb->iova[i], ret);
if (ret)
return ret;
@@ -176,7 +176,7 @@ static struct drm_framebuffer *msm_framebuffer_init(struct
drm_device *dev,
const struct msm_format *format;
int ret, i, n;
- drm_dbg_state(dev, "create framebuffer: mode_cmd=%p (%dx%d@%4.4s)",
+ drm_dbg_state(dev, "create framebuffer: mode_cmd=%p (%dx%d@%4.4s)\n",
mode_cmd, mode_cmd->width, mode_cmd->height,
(char *)_cmd->pixel_format);
@@ -232,7 +232,7 @@ static struct drm_framebuffer *msm_framebuffer_init(struct
drm_device *dev,
refcount_set(_fb->dirtyfb, 1);
- drm_dbg_state(dev, "create: FB ID: %d (%p)", fb->base.id, fb);
+ drm_dbg_state(dev, "create: FB ID: %d (%p)\n", fb->base.id, fb);
return fb;
diff --git a/drivers/gpu/drm/msm/msm_kms.c b/drivers/gpu/drm/msm/msm_kms.c
index 84c21ec2ceea..af6a6fcb1173 100644
--- a/drivers/gpu/drm/msm/msm_kms.c
+++ b/drivers/gpu/drm/msm/msm_kms.c
@@ -149,7 +149,7 @@ int msm_crtc_enable_vblank(struct drm_crtc *crtc)
struct msm_kms *kms = priv->kms;
if (!kms)
return -ENXIO;
- drm_dbg_vbl(dev, "crtc=%u", crtc->base.id);
+ drm_dbg_vbl(dev, "crtc=%u\n", crtc->base.id);
return vblank_ctrl_queue_work(priv, crtc, true);
}
@@ -160,7 +160,7 @@ void msm_crtc_disable_vblank(struct drm_crtc *crtc)
struct msm_kms *kms = priv->kms;
if (!kms)
return;
- drm_dbg_vbl(dev, "crtc=%u", crtc->base.id);
+ drm_dbg_vbl(dev, "crtc=%u\n", crtc->base.id);
vblank_ctrl_queue_work(priv, crtc, false);
}
base-commit: 4cece764965020c22cff7665b18a012006359095
--
https://chromeos.dev
Re: [PATCH v2 4/4] drm/msm/dp: Fix typo in static function (ststus => status)
Quoting Douglas Anderson (2024-03-15 14:36:32)
> This is a no-op change to just fix a typo in the name of a static function.
>
> Signed-off-by: Douglas Anderson
> ---
>
> Changes in v2:
> - ("Fix typo in static function (ststus => status)") new for v2.
This was sent at
https://lore.kernel.org/r/20240306193515.455388-1-quic_abhin...@quicinc.com
Re: [PATCH] drm/msm/dp: fix runtime_pm handling in dp_wait_hpd_asserted
Quoting Dmitry Baryshkov (2024-02-26 14:34:45)
> The function dp_wait_hpd_asserted() uses pm_runtime_get_sync() and
> doesn't care about the return value. Potentially this can lead to
> unclocked access if for some reason resuming of the DP controller fails.
>
> Change the function to use pm_runtime_resume_and_get() and return an
> error if resume fails.
>
> Fixes: e2969ee30252 ("drm/msm/dp: move of_dp_aux_populate_bus() to eDP
> probe()")
> Signed-off-by: Dmitry Baryshkov
> ---
Reviewed-by: Stephen Boyd
[PATCH 06/22] drm/bridge: Verify lane assignment is going to work during atomic_check
Verify during drm_atomic_bridge_check() that the lane assignment set in
a bridge's atomic_check() callback is going to be satisfied by the
previous bridge. If the next bridge is requiring something besides the
default 1:1 lane assignment on its input then there must be an output
lane assignment on the previous bridge's output. Otherwise the next
bridge won't get the lanes assigned that it needs.
Cc: Andrzej Hajda
Cc: Neil Armstrong
Cc: Robert Foss
Cc: Laurent Pinchart
Cc: Jonas Karlman
Cc: Jernej Skrabec
Cc: Maarten Lankhorst
Cc: Maxime Ripard
Cc: Thomas Zimmermann
Cc: David Airlie
Cc: Daniel Vetter
Cc:
Cc: Pin-yen Lin
Signed-off-by: Stephen Boyd
---
drivers/gpu/drm/drm_bridge.c | 16
1 file changed, 16 insertions(+)
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index 3fce0d8d7dcb..5097e7c65ddf 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -881,6 +881,10 @@ static int drm_atomic_bridge_check(struct drm_bridge
*bridge,
struct drm_crtc_state *crtc_state,
struct drm_connector_state *conn_state)
{
+ u8 num_input_lanes, num_output_lanes = 0;
+ const struct drm_lane_cfg *input_lanes;
+ int i;
+
if (bridge->funcs->atomic_check) {
struct drm_bridge_state *bridge_state;
int ret;
@@ -894,12 +898,24 @@ static int drm_atomic_bridge_check(struct drm_bridge
*bridge,
crtc_state, conn_state);
if (ret)
return ret;
+ num_output_lanes = bridge_state->output_bus_cfg.num_lanes;
} else if (bridge->funcs->mode_fixup) {
if (!bridge->funcs->mode_fixup(bridge, _state->mode,
_state->adjusted_mode))
return -EINVAL;
}
+ input_lanes = drm_bridge_next_bridge_lane_cfg(bridge,
+ crtc_state->state,
+ _input_lanes);
+ /*
+* Ensure this bridge is aware that the next bridge wants to
+* reassign lanes.
+*/
+ for (i = 0; i < num_input_lanes; i++)
+ if (i != input_lanes[i].logical && !num_output_lanes)
+ return -ENOTSUPP;
+
return 0;
}
--
https://chromeos.dev
[PATCH 05/22] drm/atomic-helper: Introduce lane remapping support to bridges
Add support to the DRM atomic logic to support lane remapping between
bridges, encoders and connectors. Typically lane mapping is handled
statically in firmware, e.g. on DT we use the data-lanes property to
assign lanes when connecting display bridges. Lane assignment is dynamic
with USB-C DisplayPort altmodes, e.g. pin conf D assigns 2 lanes of DP
to pins on the USB-C connector while pin conf C assigns 4 lanes of DP to
pins on the USB-C connector. The lane assignment can't be set statically
because the DP altmode repurposes USB-C pins for the DP lanes while also
limiting the number of DP lanes or their pin assignment at runtime.
Bridge drivers should point their 'struct drm_bus_cfg::lanes' pointer to
an allocated array of 'struct drm_lane_cfg' structures and indicate the
size of this allocated array with 'struct drm_bus_cfg::num_lanes' in
their atomic_check() callback. The previous bridge in the bridge chain
can look at this information by calling
drm_bridge_next_bridge_lane_cfg() in their atomic_check() callback to
figure out what lanes need to be logically assigned to the physical
output lanes to satisfy the next bridge's lane assignment.
Cc: Andrzej Hajda
Cc: Neil Armstrong
Cc: Robert Foss
Cc: Laurent Pinchart
Cc: Jonas Karlman
Cc: Jernej Skrabec
Cc: Maarten Lankhorst
Cc: Maxime Ripard
Cc: Thomas Zimmermann
Cc: David Airlie
Cc: Daniel Vetter
Cc:
Cc: Pin-yen Lin
Signed-off-by: Stephen Boyd
---
drivers/gpu/drm/drm_atomic_state_helper.c | 2 ++
drivers/gpu/drm/drm_bridge.c | 34 +++
include/drm/drm_atomic.h | 31 +
include/drm/drm_bridge.h | 4 +++
4 files changed, 71 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c
b/drivers/gpu/drm/drm_atomic_state_helper.c
index 784e63d70a42..2e989fbeb503 100644
--- a/drivers/gpu/drm/drm_atomic_state_helper.c
+++ b/drivers/gpu/drm/drm_atomic_state_helper.c
@@ -764,6 +764,8 @@ EXPORT_SYMBOL(drm_atomic_helper_bridge_duplicate_state);
void drm_atomic_helper_bridge_destroy_state(struct drm_bridge *bridge,
struct drm_bridge_state *state)
{
+ kfree(state->input_bus_cfg.lanes);
+ kfree(state->output_bus_cfg.lanes);
kfree(state);
}
EXPORT_SYMBOL(drm_atomic_helper_bridge_destroy_state);
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index 30d66bee0ec6..3fce0d8d7dcb 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -843,6 +843,40 @@ void drm_atomic_bridge_chain_enable(struct drm_bridge
*bridge,
}
EXPORT_SYMBOL(drm_atomic_bridge_chain_enable);
+/**
+ * drm_bridge_next_bridge_lane_cfg - get the lane configuration of the next
bridge
+ * @bridge: bridge control structure
+ * @state: new atomic state
+ * @num_lanes: will contain the size of the returned array
+ *
+ * This function is typically called from _bridge_funcs.atomic_check().
+ * The @bridge driver calls this function to determine what the next bridge in
+ * the bridge chain requires for the physical to logical lane assignments.
+ *
+ * Return: Lane configuration array of size @num_lanes for the next bridge
+ * after @bridge in the bridge chain, or NULL if the lane configuration is
+ * unchanged from the default.
+ */
+const struct drm_lane_cfg *
+drm_bridge_next_bridge_lane_cfg(struct drm_bridge *bridge,
+ struct drm_atomic_state *state,
+ u8 *num_lanes)
+{
+ const struct drm_bridge_state *next_bridge_state;
+ struct drm_bridge *next_bridge = drm_bridge_get_next_bridge(bridge);
+
+ next_bridge_state = drm_atomic_get_new_bridge_state(state, next_bridge);
+ if (!next_bridge_state) {
+ *num_lanes = 0;
+ return NULL;
+ }
+
+ *num_lanes = next_bridge_state->input_bus_cfg.num_lanes;
+
+ return next_bridge_state->input_bus_cfg.lanes;
+}
+EXPORT_SYMBOL(drm_bridge_next_bridge_lane_cfg);
+
static int drm_atomic_bridge_check(struct drm_bridge *bridge,
struct drm_crtc_state *crtc_state,
struct drm_connector_state *conn_state)
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index cf8e1220a4ac..b206ae2654d8 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -1074,6 +1074,27 @@ drm_atomic_crtc_effectively_active(const struct
drm_crtc_state *state)
return state->active || state->self_refresh_active;
}
+/**
+ * struct drm_lane_cfg - lane configuration
+ *
+ * This structure stores the lange configuration of a physical bus between
+ * two components in an output pipeline, usually between two bridges, an
+ * encoder and a bridge, or a bridge and a connector.
+ *
+ * The lane configuration is stored in _bus_cfg.
+ */
+struct drm_lane_cfg {
+ /**
+* @logical: Logical lane number
+*/
[PATCH 00/22] platform/chrome: Add DT USB/DP muxing/topology to Trogdor
This series adds support for fully describing the USB/DP topology on ChromeOS Trogdor devices in DT. Trogdor devices have a single DP phy in the AP that is muxed to one of two usb type-c connectors depending on which port asserts HPD first to the EC. We'd like to know which port is connected to an external monitor to provide a better experience to the user about things like which type-c port is displaying DP or which type-c hub is acting up, etc. Describing the connection all the way from the source to the connector will allow us to do this. There will be some more work to do after this to wire up sysfs connections, but that work has already started or finished so it should be mostly minor changes to support DT there. This patch series is large, unfortunately, and is ordered in logical groups: gpio, USB, DRM, typec, and finally dts to put it all together. There's more that could be put in here, e.g. supporting ChromeOS Corsola devices, but I wanted to get something out there early instead of waiting to make this work with everything that exists today and posting it then. Onto the patches: First is the EC GPIO driver, which is dependency free and can be merged at any time. It's only needed to provide information about which port the EC is steering DP to, because the EC had a bug where it never told the AP about which port has HPD asserted or not. Second is the USB binding and hub patches. These are used to describe how the USB hub is wired up on all the Trogdor devices, and make the connect_type be something besides "unknown" on DT devices. ACPI has supported setting a proper connect_type for some time now. These can also be merged pretty much dependency free, except that the dt binding will be needed to avoid DT binding check failures. I don't think those checks are fatal though, so probably also fine to take this part independently. Third is the DRM bridge patches. These are used to implement lane assignment for DP altmode configurations through the drm_bridge code. The typec code will use this to tell the DP phy how many lanes of DP to drive and which lanes to drive out to the USB type-c connector. Adding support for lane assignment allows us to implement DP muxing as well, physically splitting the DP lanes on the DP phy so that hardware doesn't have to use an analog mux to steer two DP lanes to one or the other type-c port. These are a hard dependency for the typec code. Fourth is the typec patches, that ties together everything that comes before it in this series. The EC typec switch driver implements a drm_bridge that can signal HPD from the type-c connector through the bridge chain, mux the DP phy in software so that we don't have to use an analog mux, and implement orientation control for boards like Kukui that directly connect the DP phy to the type-c port, necessitating lane assignment to flip the lanes to match the cable orientation. Finally, the dts patches wire everything up to fully describe the USB/DT topology on Trogdor. This includes the USB hub, the EC gpios, the DP controller, and the external connectors like the usb-c and usb-a connectors. After this initial version I will probably split this series and send parts in pieces to more rapidly send new versions. Those parts will refer back to this version in the cover letter so we can all get the full context. I don't expect to merge this through one maintainer tree immediately, so I set the 'To' line to chrome-platform to reflect the overall target audience. Prashant Malani (1): platform/chrome: cros_ec_typec: Purge blocking switch devlinks Stephen Boyd (21): dt-bindings: gpio: Add binding for ChromeOS EC GPIO controller gpio: Add ChromeOS EC GPIO driver dt-bindings: usb: Add downstream facing ports to realtek binding usb: core: Set connect_type of ports based on DT node drm/atomic-helper: Introduce lane remapping support to bridges drm/bridge: Verify lane assignment is going to work during atomic_check device property: Add remote endpoint to devcon matcher platform/chrome: cros_typec_switch: Use read_poll_timeout helper platform/chrome: cros_typec_switch: Move port creation code to sub-function platform/chrome: cros_typec_switch: Use fwnode instead of ACPI APIs platform/chrome: cros_typec_switch: Use dev_err_probe() dt-bindings: chrome: Add google,cros-ec-typec-switch binding platform/chrome: cros_typec_switch: Add support for signaling HPD to drm_bridge platform/chrome: cros_typec_switch: Support DP muxing via DRM lane assignment platform/chrome: cros_typec_switch: Support orientation-switch platform/chrome: cros_typec_switch: Handle lack of HPD information dt-bindings: chrome: Add binding for ChromeOS Pogo pin connector arm64: dts: qcom: sc7180: quackingstick: Disable instead of delete usb_c1 arm64: dts: qcom: sc7180: pazquel: Add missing comment header arm64: dts: qcom: sc7180-trogdor: Make clamshell/detachable fragments arm64: dts: qcom: sc7180-trogdo
Re: [PATCH] drm/bridge: ps8640: Fix size mismatch warning w/ len
Quoting Douglas Anderson (2023-12-18 09:04:54)
> After commit 26195af57798 ("drm/bridge: ps8640: Drop the ability of
> ps8640 to fetch the EDID"), I got an error compiling:
>
> error: comparison of distinct pointer types
> ('typeof (len) *' (aka 'unsigned int *') and
>'typeof (msg->size) *' (aka 'unsigned long *'))
> [-Werror,-Wcompare-distinct-pointer-types]
>
> Fix it by declaring the `len` as size_t.
>
> Fixes: 26195af57798 ("drm/bridge: ps8640: Drop the ability of ps8640 to fetch
> the EDID")
> Signed-off-by: Douglas Anderson
> ---
Reviewed-by: Stephen Boyd
Re: [PATCH v3 2/2] drm/bridge: ti-sn65dsi86: Never store more than msg->size bytes in AUX xfer
Quoting Douglas Anderson (2023-12-14 12:37:52)
> For aux reads, the value `msg->size` indicates the size of the buffer
> provided by `msg->buffer`. We should never in any circumstances write
> more bytes to the buffer since it may overflow the buffer.
>
> In the ti-sn65dsi86 driver there is one code path that reads the
> transfer length from hardware. Even though it's never been seen to be
> a problem, we should make extra sure that the hardware isn't
> increasing the length since doing so would cause us to overrun the
> buffer.
>
> Fixes: 982f589bde7a ("drm/bridge: ti-sn65dsi86: Update reply on aux failures")
> Signed-off-by: Douglas Anderson
> ---
Reviewed-by: Stephen Boyd
Re: [PATCH v3 1/2] drm/bridge: parade-ps8640: Never store more than msg->size bytes in AUX xfer
Quoting Douglas Anderson (2023-12-14 12:37:51)
> While testing, I happened to notice a random crash that looked like:
>
> Kernel panic - not syncing: stack-protector:
> Kernel stack is corrupted in: drm_dp_dpcd_probe+0x120/0x120
>
> Analysis of drm_dp_dpcd_probe() shows that we pass in a 1-byte buffer
> (allocated on the stack) to the aux->transfer() function. Presumably
> if the aux->transfer() writes more than one byte to this buffer then
> we're in a bad shape.
>
> Dropping into kgdb, I noticed that "aux->transfer" pointed at
> ps8640_aux_transfer().
>
> Reading through ps8640_aux_transfer(), I can see that there are cases
> where it could write more bytes to msg->buffer than were specified by
> msg->size. This could happen if the hardware reported back something
> bogus to us. Let's fix this so we never write more than msg->size
> bytes. We'll still read all the bytes from the hardware just in case
> the hardware requires it since the aux transfer data comes through an
> auto-incrementing register.
>
> NOTE: I have no actual way to reproduce this issue but it seems likely
> this is what was happening in the crash I looked at.
>
> Fixes: 13afcdd7277e ("drm/bridge: parade-ps8640: Add support for AUX channel")
> Signed-off-by: Douglas Anderson
> ---
Reviewed-by: Stephen Boyd
Re: [PATCH] drm/msm/dpu: drop obsolete documentation for dpu_encoder_virt
Quoting Dmitry Baryshkov (2023-12-16 16:01:58)
> Drop obsolete kerneldoc for several fields in struct dpu_encoder_virt
>
> Reported-by: kernel test robot
> Closes:
> https://lore.kernel.org/oe-kbuild-all/202312170641.5exlvqqx-...@intel.com/
> Fixes: 62d35629da80 ("drm/msm/dpu: move encoder status to standard encoder
> debugfs dir")
> Fixes: 25fdd5933e4c ("drm/msm: Add SDM845 DPU support")
> Signed-off-by: Dmitry Baryshkov
> ---
Reviewed-by: Stephen Boyd
Re: [PATCH 1/2] drm/bridge: parade-ps8640: Never increase the length when reading from AUX
Quoting Douglas Anderson (2023-12-11 16:55:26) > diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c > b/drivers/gpu/drm/bridge/parade-ps8640.c > index 8161b1a1a4b1..fb2ec4264549 100644 > --- a/drivers/gpu/drm/bridge/parade-ps8640.c > +++ b/drivers/gpu/drm/bridge/parade-ps8640.c > @@ -302,7 +302,7 @@ static ssize_t ps8640_aux_transfer_msg(struct drm_dp_aux > *aux, > > fallthrough; > case SWAUX_STATUS_ACKM: > - len = data & SWAUX_M_MASK; > + len = min(len, (unsigned int)(data & SWAUX_M_MASK)); > break; > case SWAUX_STATUS_DEFER: > case SWAUX_STATUS_I2C_DEFER: > @@ -310,7 +310,7 @@ static ssize_t ps8640_aux_transfer_msg(struct drm_dp_aux > *aux, > msg->reply |= DP_AUX_NATIVE_REPLY_DEFER; > else > msg->reply |= DP_AUX_I2C_REPLY_DEFER; > - len = data & SWAUX_M_MASK; > + len = min(len, (unsigned int)(data & SWAUX_M_MASK)); > break; > case SWAUX_STATUS_INVALID: > return -EOPNOTSUPP; If the hardware indicates the len is larger than the length of 'buf' do we need to throw away reads of the fifo until we read the length that we're told? I'm specifically looking at the read loop at the end of ps8640_aux_transfer_msg() where it reads a byte at a time out of 'PAGE0_SWAUX_RDATA'. So maybe what we need to do is have 'buf_len' and 'len' and then return the min of the two at the end of the function but only copy over 'buf_len' amount.
Re: [PATCH v7 0/7] incorporate pm runtime framework and eDP clean up
Quoting Kuogee Hsieh (2023-10-06 15:55:03) > The purpose of this patch series is to incorporate pm runtime framework > into MSM eDP/DP driver so that eDP panel can be detected by DRM eDP panel > driver during system probe time. During incorporating procedure, original > customized pm realted fucntions, such as dp_pm_prepare(), dp_pm_suspend(), > dp_pm_resume() and dp_pm_prepare(), are removed and replaced with functions > provided by pm runtiem framework such as pm_runtime_force_suspend() and > pm_runtime_force_resume(). In addition, both eDP aux-bus and irq handler > are bound at system probe time too. > > Kuogee Hsieh (7): > drm/msm/dp: tie dp_display_irq_handler() with dp driver > drm/msm/dp: rename is_connected with link_ready > drm/msm/dp: use drm_bridge_hpd_notify() to report HPD status changes > drm/msm/dp: move parser->parse() and dp_power_client_init() to probe > drm/msm/dp: incorporate pm_runtime framework into DP driver > drm/msm/dp: delete EV_HPD_INIT_SETUP > drm/msm/dp: move of_dp_aux_populate_bus() to eDP probe() > > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 4 - > drivers/gpu/drm/msm/dp/dp_aux.c | 39 +++- > drivers/gpu/drm/msm/dp/dp_display.c | 333 > Tested-by: Stephen Boyd # Trogdor.Lazor I ran some suspend cycles too with the lid open and closed.
Re: [PATCH] drm/msm/dp: support setting the DP subconnector type
Quoting Dmitry Baryshkov (2023-09-03 15:24:32) > Read the downstream port info and set the subconnector type accordingly. > > Signed-off-by: Dmitry Baryshkov > --- Reviewed-by: Stephen Boyd
Re: [PATCH] drm/bridge: ti-sn65dsi86: Associate DSI device lifetime with auxiliary device
Quoting Doug Anderson (2023-10-02 17:31:41)
> Hi,
>
> On Mon, Oct 2, 2023 at 4:54 PM Stephen Boyd wrote:
> >
> > The kernel produces a warning splat and the DSI device fails to register
> > in this driver if the i2c driver probes, populates child auxiliary
> > devices, and then somewhere in ti_sn_bridge_probe() a function call
> > returns -EPROBE_DEFER. When the auxiliary driver probe defers, the dsi
> > device created by devm_mipi_dsi_device_register_full() is left
> > registered because the devm managed device used to manage the lifetime
> > of the DSI device is the parent i2c device, not the auxiliary device
> > that is being probed.
> >
> > Associate the DSI device created and managed by this driver to the
> > lifetime of the auxiliary device, not the i2c device, so that the DSI
> > device is removed when the auxiliary driver unbinds. Similarly change
> > the device pointer used for dev_err_probe() so the deferred probe errors
> > are associated with the auxiliary device instead of the parent i2c
> > device so we can narrow down future problems faster.
> >
> > Cc: Douglas Anderson
> > Cc: Maxime Ripard
> > Fixes: c3b75d4734cb ("drm/bridge: sn65dsi86: Register and attach our DSI
> > device at probe")
>
> Even before that commit I think it was using the main "dev" instead of
> the auxiliary device's "dev" for some "devm" stuff. I guess the
> difference is that it wouldn't mess with probe deferral? Searching
> back, I think the first instance of a case that was using "devm_" with
> the wrong device was commit 4e5763f03e10 ("drm/bridge: ti-sn65dsi86:
> Wrap panel with panel-bridge")? Would it make sense to use that as a
> Fixes, you think?
The problem for me is that the dsi device is registered twice. That
happens because probe for the auxiliary device happens twice. I was
cautious about the fixes tag here because it didn't look like probe
deferral was happening before commit c3b75d4734cb.
>
> In any case, this looks reasonable to me:
>
> Reviewed-by: Douglas Anderson
>
> I'll give it a week and then apply to "-fixes" if everything is quiet.
Thanks!
[PATCH] drm/bridge: ti-sn65dsi86: Associate DSI device lifetime with auxiliary device
The kernel produces a warning splat and the DSI device fails to register
in this driver if the i2c driver probes, populates child auxiliary
devices, and then somewhere in ti_sn_bridge_probe() a function call
returns -EPROBE_DEFER. When the auxiliary driver probe defers, the dsi
device created by devm_mipi_dsi_device_register_full() is left
registered because the devm managed device used to manage the lifetime
of the DSI device is the parent i2c device, not the auxiliary device
that is being probed.
Associate the DSI device created and managed by this driver to the
lifetime of the auxiliary device, not the i2c device, so that the DSI
device is removed when the auxiliary driver unbinds. Similarly change
the device pointer used for dev_err_probe() so the deferred probe errors
are associated with the auxiliary device instead of the parent i2c
device so we can narrow down future problems faster.
Cc: Douglas Anderson
Cc: Maxime Ripard
Fixes: c3b75d4734cb ("drm/bridge: sn65dsi86: Register and attach our DSI device
at probe")
Signed-off-by: Stephen Boyd
---
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 14 +++---
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index f448b903e190..84148a79414b 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -692,7 +692,7 @@ static struct ti_sn65dsi86 *bridge_to_ti_sn65dsi86(struct
drm_bridge *bridge)
return container_of(bridge, struct ti_sn65dsi86, bridge);
}
-static int ti_sn_attach_host(struct ti_sn65dsi86 *pdata)
+static int ti_sn_attach_host(struct auxiliary_device *adev, struct
ti_sn65dsi86 *pdata)
{
int val;
struct mipi_dsi_host *host;
@@ -707,7 +707,7 @@ static int ti_sn_attach_host(struct ti_sn65dsi86 *pdata)
if (!host)
return -EPROBE_DEFER;
- dsi = devm_mipi_dsi_device_register_full(dev, host, );
+ dsi = devm_mipi_dsi_device_register_full(>dev, host, );
if (IS_ERR(dsi))
return PTR_ERR(dsi);
@@ -725,7 +725,7 @@ static int ti_sn_attach_host(struct ti_sn65dsi86 *pdata)
pdata->dsi = dsi;
- return devm_mipi_dsi_attach(dev, dsi);
+ return devm_mipi_dsi_attach(>dev, dsi);
}
static int ti_sn_bridge_attach(struct drm_bridge *bridge,
@@ -1298,9 +1298,9 @@ static int ti_sn_bridge_probe(struct auxiliary_device
*adev,
struct device_node *np = pdata->dev->of_node;
int ret;
- pdata->next_bridge = devm_drm_of_get_bridge(pdata->dev, np, 1, 0);
+ pdata->next_bridge = devm_drm_of_get_bridge(>dev, np, 1, 0);
if (IS_ERR(pdata->next_bridge))
- return dev_err_probe(pdata->dev, PTR_ERR(pdata->next_bridge),
+ return dev_err_probe(>dev, PTR_ERR(pdata->next_bridge),
"failed to create panel bridge\n");
ti_sn_bridge_parse_lanes(pdata, np);
@@ -1319,9 +1319,9 @@ static int ti_sn_bridge_probe(struct auxiliary_device
*adev,
drm_bridge_add(>bridge);
- ret = ti_sn_attach_host(pdata);
+ ret = ti_sn_attach_host(adev, pdata);
if (ret) {
- dev_err_probe(pdata->dev, ret, "failed to attach dsi host\n");
+ dev_err_probe(>dev, ret, "failed to attach dsi host\n");
goto err_remove_bridge;
}
base-commit: 0bb80ecc33a8fb5a682236443c1e740d5c917d1d
--
https://chromeos.dev
Re: [PATCH v3 6/7] drm/msm/dp: add pm_runtime_force_suspend()/resume()
Quoting Abhinav Kumar (2023-09-28 17:46:11) > On 9/27/2023 3:01 PM, Stephen Boyd wrote: > > Quoting Kuogee Hsieh (2023-09-25 09:07:18) > >> > >> However for external DP case, link training can not be guarantee always > >> success without link rate or lane being reduced as Abhinav mentioned. > >> > >> In addition, CTS (compliance test) it required to complete link > >> training within 10ms after hpd asserted. > > > > Is it possible to change that timeout? I have to look around for the CTS > > parameters because I'm pretty confused how it can work. What do we do if > > DP wakes the system from suspend and asserts HPD? We need resume time to > > be < 10ms? That's not realistic. > > > > No, the CTS doesnt say we need to finish link training within 10ms after > HPD is asserted. It says it must be completed in 10ms after > TRAINING_PATTERN_SET dpcd write. > > "Wait until the Source DUT writes 00h to the TRAINING_PATTERN_SET byte > of Reference Sink DPCD Link Configuration Field to indicate the end of > the link training. Stop the link training timer. Verify that link > training completed in 10ms or less" > > That needs to be done independent of HPD so we can ignore the CTS point. Great! > > >> > >> I am not sure do link training at atomic_enable() can meet this timing > >> requirement. Why? It's putting some time bound on link training in general to only take 10ms, right? > >> > > > > At least in the DP spec itself it doesn't require the link to be trained > > within 10ms of HPD being asserted. Instead it simply recommends that the > > OS start configuring the display promptly after HPD is asserted, e.g. > > within 100ms. There's some strict timing on IRQ_HPD, so the driver must > > read DPCD registers within 100ms of IRQ_HPD rising edge; maybe that is > > what CTS is checking for? > > > > TL;DR: I don't see why CTS should stop us from link training in > > atomic_enable(). It would be beneficial to do so to make eDP and DP the > > same. It would also help to report a drm connector being connected > > _before_ link training so that userspace knows the link itself is the > > bad part of the equation (and not that the DP connector looks > > disconnected to userspace when in fact it really is connected and the > > monitor is asserting HPD, just the link training failed). > > Its the corrective action of the userspace when it finds link is bad is > the concern as I highlighted in the other response. Just reading and > resetting link_status is not enough to recover. What needs to be done to recover? Userspace will try to set a mode on the connector again if the link status is bad and there were some modes available. If there are zero modes and the link is bad, then it ignores the connector. I'm not sure what else could be done to recover besides try again and stop trying if no modes exist. Acting like the connector isn't connected makes the situation worse for ChromeOS because userspace thinks there's nothing there so it can't try to retrain the link again. Instead, userspace has to rely on the kernel driver to train the link again. The kernel should just tell userspace the link is bad so userspace can implement the policy to either ignore the connector entirely or to consider it a display that is having link training problems. So again, I see no reason why the kernel driver thinks it can implement a policy to train the link before indicating the drm connector is connected. It should stop doing that. Instead it should tell userspace that the connector is connected and then train the link when there's a modeset. If the modeset fails then userspace can take action to either figure out that the link is bad, or notify the user that the cable is bad, or to try replugging or power cycle the monitor, etc. None of that can be done if the kernel lies about the state of the connector because the link training failed.
Re: [PATCH v3 6/7] drm/msm/dp: add pm_runtime_force_suspend()/resume()
Quoting Kuogee Hsieh (2023-09-25 09:07:18) > > On 9/22/2023 6:35 PM, Abhinav Kumar wrote: > > > > 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. So the design sends the hotplug to usermode > > only after link training succeeds. > > > > I do not think we should change this design unless prototyped with an > > existing compositor such as chrome or android at this point. > > > > Thanks > > > > Abhinav > > > We did perform link training at atomic_enable() at eDP case since we can > assume link training will always success without link rate or link lane > being reduced. > > However for external DP case, link training can not be guarantee always > success without link rate or lane being reduced as Abhinav mentioned. > > In addition, CTS (compliance test) it required to complete link > training within 10ms after hpd asserted. Is it possible to change that timeout? I have to look around for the CTS parameters because I'm pretty confused how it can work. What do we do if DP wakes the system from suspend and asserts HPD? We need resume time to be < 10ms? That's not realistic. > > I am not sure do link training at atomic_enable() can meet this timing > requirement. > At least in the DP spec itself it doesn't require the link to be trained within 10ms of HPD being asserted. Instead it simply recommends that the OS start configuring the display promptly after HPD is asserted, e.g. within 100ms. There's some strict timing on IRQ_HPD, so the driver must read DPCD registers within 100ms of IRQ_HPD rising edge; maybe that is what CTS is checking for? TL;DR: I don't see why CTS should stop us from link training in atomic_enable(). It would be beneficial to do so to make eDP and DP the same. It would also help to report a drm connector being connected _before_ link training so that userspace knows the link itself is the bad part of the equation (and not that the DP connector looks disconnected to userspace when in fact it really is connected and the monitor is asserting HPD, just the link training failed).
Re: [PATCH v3 6/7] drm/msm/dp: add pm_runtime_force_suspend()/resume()
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
Re: [PATCH v3 6/7] drm/msm/dp: add pm_runtime_force_suspend()/resume()
Quoting Dmitry Baryshkov (2023-09-19 02:50:12) > On Mon, 18 Sept 2023 at 20:48, Kuogee Hsieh wrote: > > > > > > On 9/15/2023 6:21 PM, Dmitry Baryshkov wrote: > > > On Sat, 16 Sept 2023 at 00:38, Kuogee Hsieh > > > wrote: > > >> Add pm_runtime_force_suspend()/resume() to complete incorporating pm > > >> runtime framework into DP driver. Both dp_pm_prepare() and > > >> dp_pm_complete() > > >> are added to set hpd_state to correct state. After resume, DP driver will > > >> re training its main link after .hpd_enable() callback enabled HPD > > >> interrupts and bring up display accordingly. > > > How will it re-train the main link? What is the code path for that? > > > > 1) for edp, dp_bridge_atomic_enable(), called from framework, to start > > link training and bring up display. > > And this path doesn't use .hpd_enable() which you have mentioned in > the commit message. Please don't try to shorten the commit message. > You see, I have had questions to several of them, which means that > they were not verbose enough. > > > > > 2) for external DP, HPD_PLUG_INT will be generated to start link > > training and bring up display. > > 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.
Re: [PATCH] drm/msm/dp: support setting the DP subconnector type
Quoting Dmitry Baryshkov (2023-09-07 14:48:54) > On 08/09/2023 00:34, Stephen Boyd wrote: > > Quoting Dmitry Baryshkov (2023-09-03 15:24:32) > >> diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c > >> b/drivers/gpu/drm/msm/dp/dp_panel.c > >> index 97ba41593820..1cb54f26f5aa 100644 > >> --- a/drivers/gpu/drm/msm/dp/dp_panel.c > >> +++ b/drivers/gpu/drm/msm/dp/dp_panel.c > >> @@ -162,6 +162,11 @@ int dp_panel_read_sink_caps(struct dp_panel *dp_panel, > >> } > >> } > >> > >> + rc = drm_dp_read_downstream_info(panel->aux, dp_panel->dpcd, > >> +dp_panel->downstream_ports); > >> + if (rc) > >> + return rc; > > > > I haven't been able to test it yet, but I think with an apple dongle > > we'll never populate the 'downstream_ports' member if the HDMI cable is > > not connected when this runs. That's because this function bails out > > early before trying to read the downstream ports when there isn't a > > sink. Perhaps we need to read it again when an hpd_irq comes in, or we > > need to read it before bailing out from here? > > I don't have an Apple dongle here. But I'll run a check with first > connecting the dongle and plugging the HDMI afterwards. > > However my expectation based on my previous tests is that we only get > here when the actual display is connected. > We get here when HPD is high. With an apple dongle, hpd is high when just the dongle is plugged in. That calls dp_display_process_hpd_high() which calls dp_panel_read_sink_caps(), but that returns with an error (-ENOTCONN) and then we wait for something to change. When the HDMI cable is plugged in (i.e. an actual display) we get an irq_hpd. That causes dp_irq_hpd_handle() to call dp_display_usbpd_attention_cb() which calls dp_link_process_request() that sees 'sink_request & DS_PORT_STATUS_CHANGED' and thus calls dp_display_handle_port_ststus_changed() (that has a typo right?) which hits the else condition and calls dp_display_process_hpd_high(). So yes? We will eventually call dp_panel_read_sink_caps() again, and this time not bail out early. It's probably fine.
Re: [PATCH] drm/msm/dp: skip validity check for DP CTS EDID checksum
Quoting Jani Nikula (2023-09-01 07:20:34)
> The DP CTS test for EDID last block checksum expects the checksum for
> the last block, invalid or not. Skip the validity check.
>
> For the most part (*), the EDIDs returned by drm_get_edid() will be
> valid anyway, and there's the CTS workaround to get the checksum for
> completely invalid EDIDs. See commit 7948fe12d47a ("drm/msm/dp: return
> correct edid checksum after corrupted edid checksum read").
>
> This lets us remove one user of drm_edid_block_valid() with hopes the
> function can be removed altogether in the future.
>
> (*) drm_get_edid() ignores checksum errors on CTA extensions.
>
> Cc: Abhinav Kumar
> Cc: Dmitry Baryshkov
> Cc: Kuogee Hsieh
> Cc: Marijn Suijten
> Cc: Rob Clark
> Cc: Sean Paul
> Cc: Stephen Boyd
> Cc: linux-arm-...@vger.kernel.org
> Cc: freedr...@lists.freedesktop.org
> Signed-off-by: Jani Nikula
> ---
Reviewed-by: Stephen Boyd
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c
> b/drivers/gpu/drm/msm/dp/dp_panel.c
> index 42d52510ffd4..86a8e06c7a60 100644
> --- a/drivers/gpu/drm/msm/dp/dp_panel.c
> +++ b/drivers/gpu/drm/msm/dp/dp_panel.c
> @@ -289,26 +289,9 @@ int dp_panel_get_modes(struct dp_panel *dp_panel,
>
> static u8 dp_panel_get_edid_checksum(struct edid *edid)
It would be nice to make 'edid' const here in another patch.
Re: [PATCH] drm/msm/dp: support setting the DP subconnector type
Quoting Dmitry Baryshkov (2023-09-03 15:24:32) > diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c > b/drivers/gpu/drm/msm/dp/dp_panel.c > index 97ba41593820..1cb54f26f5aa 100644 > --- a/drivers/gpu/drm/msm/dp/dp_panel.c > +++ b/drivers/gpu/drm/msm/dp/dp_panel.c > @@ -162,6 +162,11 @@ int dp_panel_read_sink_caps(struct dp_panel *dp_panel, > } > } > > + rc = drm_dp_read_downstream_info(panel->aux, dp_panel->dpcd, > +dp_panel->downstream_ports); > + if (rc) > + return rc; I haven't been able to test it yet, but I think with an apple dongle we'll never populate the 'downstream_ports' member if the HDMI cable is not connected when this runs. That's because this function bails out early before trying to read the downstream ports when there isn't a sink. Perhaps we need to read it again when an hpd_irq comes in, or we need to read it before bailing out from here?
Re: [PATCH 4/6] drm/msm: add a kernel param to select between MDP5 and DPU drivers
Quoting Dmitry Baryshkov (2023-09-05 10:43:51) > For some of the platforms (e.g. SDM660, SDM630, MSM8996, etc.) it is > possible to support this platform via the DPU driver (e.g. to provide > support for DP, multirect, etc). Add a modparam to be able to switch > between these two drivers. > > All platforms supported by both drivers are by default handled by the > MDP5 driver. To let them be handled by the DPU driver pass the > `msm.prefer_mdp5=false` kernel param. > > Signed-off-by: Dmitry Baryshkov > --- Reviewed-by: Stephen Boyd
Re: [PATCH 3/6] drm/msm/dpu: support binding to the mdp5 devices
Quoting Dmitry Baryshkov (2023-09-05 10:43:50)
> diff --git a/drivers/gpu/drm/msm/msm_io_utils.c
> b/drivers/gpu/drm/msm/msm_io_utils.c
> index 59d2788c4510..9d0d76f3a319 100644
> --- a/drivers/gpu/drm/msm/msm_io_utils.c
> +++ b/drivers/gpu/drm/msm/msm_io_utils.c
> @@ -50,6 +50,24 @@ struct clk *msm_clk_get(struct platform_device *pdev,
> const char *name)
> return clk;
> }
>
> +void __iomem *msm_ioremap_mdss(struct platform_device *mdss_pdev,
> + struct platform_device *pdev,
> + const char *name)
> +{
> + struct resource *res;
> + void __iomem *ptr;
> +
> + res = platform_get_resource_byname(mdss_pdev, IORESOURCE_MEM, name);
> + if (!res)
> + return ERR_PTR(-EINVAL);
> +
> + ptr = devm_ioremap_resource(>dev, res);
> + if (!ptr)
devm_ioremap_resource() returns an error pointer. Too bad we can't use
devm_platform_ioremap_resource_byname() here.
> + return ERR_PTR(-ENOMEM);
> +
> + return ptr;
> +}
Re: [PATCH 2/6] drm/msm/mdss: generate MDSS data for MDP5 platforms
Quoting Dmitry Baryshkov (2023-09-05 10:43:49)
> diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c
> index 348c66b14683..fb6ee93b5abc 100644
> --- a/drivers/gpu/drm/msm/msm_mdss.c
> +++ b/drivers/gpu/drm/msm/msm_mdss.c
> @@ -222,6 +222,36 @@ static void msm_mdss_setup_ubwc_dec_40(struct msm_mdss
> *msm_mdss)
> }
> }
>
> +static struct msm_mdss_data *msm_mdss_generate_mdp5_mdss_data(struct
> msm_mdss *mdss)
const mdss?
> +{
> + struct msm_mdss_data *data;
> + u32 hw_rev;
> +
> + data = devm_kzalloc(mdss->dev, sizeof(*data), GFP_KERNEL);
> + if (!data)
> + return NULL;
> +
> + hw_rev = readl_relaxed(mdss->mmio + HW_REV) >> 16;
Or like this
hw_rev = upper_16_bits(readl_relaxed(...))
> +
> + if (hw_rev == 0x1007 || /* msm8996 */
> + hw_rev == 0x100e || /* msm8937 */
> + hw_rev == 0x1010 || /* msm8953 */
> + hw_rev == 0x3000 || /* msm8998 */
> + hw_rev == 0x3002 || /* sdm660 */
> + hw_rev == 0x3003) { /* sdm630 */
Can we have #defines for hw_revs and drop the comments?
> + data->ubwc_dec_version = UBWC_1_0;
> + data->ubwc_enc_version = UBWC_1_0;
> + }
> +
> + if (hw_rev == 0x1007 || /* msm8996 */
> + hw_rev == 0x3000) /* msm8998 */
Then we don't have to worry about these two having typos.
> + data->highest_bank_bit = 2;
> + else
> + data->highest_bank_bit = 1;
>
Re: [PATCH v3 5/8] drm/msm/dpu: enable INTF TE operations only when supported by HW
Quoting Dmitry Baryshkov (2023-09-03 19:04:51) > The DPU_INTF_TE bit is set for all INTF blocks on DPU >= 5.0, however > only INTF_1 and INTF_2 actually support tearing control (both are > INTF_DSI). Rather than trying to limit the DPU_INTF_TE feature bit to > those two INTF instances, check for the major && INTF type. > > Reviewed-by: Marijn Suijten > Signed-off-by: Dmitry Baryshkov > --- Reviewed-by: Stephen Boyd
Re: [PATCH v3 6/8] drm/msm/dpu: drop DPU_INTF_TE feature flag
Quoting Dmitry Baryshkov (2023-09-03 19:04:52) > Replace the only user of the DPU_INTF_TE feature flag with the direct > DPU version comparison. > > Reviewed-by: Marijn Suijten > Signed-off-by: Dmitry Baryshkov > --- Reviewed-by: Stephen Boyd
Re: [PATCH v3 7/8] drm/msm/dpu: drop useless check from dpu_encoder_phys_cmd_te_rd_ptr_irq()
Quoting Dmitry Baryshkov (2023-09-03 19:04:53) > The dpu_encoder_phys_cmd_te_rd_ptr_irq() function uses neither hw_intf > nor hw_pp data, so we can drop the corresponding check. > > Reviewed-by: Marijn Suijten > Signed-off-by: Dmitry Baryshkov > --- Reviewed-by: Stephen Boyd
Re: [PATCH v3 8/8] drm/msm/dpu: move INTF tearing checks to dpu_encoder_phys_cmd_init
Quoting Dmitry Baryshkov (2023-09-03 19:04:54) > As the INTF is fixed at the encoder creation time, we can move the > check whether INTF supports tearchck to dpu_encoder_phys_cmd_init(). > This function can return an error if INTF doesn't have required feature. > Performing this check in dpu_encoder_phys_cmd_tearcheck_config() is less > useful, as this function returns void. > > Signed-off-by: Dmitry Baryshkov > --- Reviewed-by: Stephen Boyd
Re: [PATCH v3 4/8] drm/msm/dpu: inline _setup_intf_ops()
Quoting Dmitry Baryshkov (2023-09-03 19:04:50)
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> index 8ec6505d9e78..dd67686f5403 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> @@ -524,31 +524,6 @@ static void dpu_hw_intf_program_intf_cmd_cfg(struct
> dpu_hw_intf *ctx,
[...]
> - ops->disable_autorefresh = dpu_hw_intf_disable_autorefresh;
> - }
> -
> - if (mdss_rev->core_major_ver >= 7)
> - ops->program_intf_cmd_cfg = dpu_hw_intf_program_intf_cmd_cfg;
> -}
> -
> struct dpu_hw_intf *dpu_hw_intf_init(const struct dpu_intf_cfg *cfg,
> void __iomem *addr, const struct dpu_mdss_version *mdss_rev)
> {
> @@ -571,7 +546,28 @@ struct dpu_hw_intf *dpu_hw_intf_init(const struct
> dpu_intf_cfg *cfg,
> */
> c->idx = cfg->id;
> c->cap = cfg;
> - _setup_intf_ops(>ops, c->cap->features, mdss_rev);
> +
> + c->ops.setup_timing_gen = dpu_hw_intf_setup_timing_engine;
Maybe grow a local variable 'ops' that can be assigned to so the code
doesn't change at all, only the location does.
> + c->ops.setup_prg_fetch = dpu_hw_intf_setup_prg_fetch;
> + c->ops.get_status = dpu_hw_intf_get_status;
Re: [PATCH v3 3/8] drm/msm/dpu: drop the DPU_PINGPONG_TE flag
Quoting Dmitry Baryshkov (2023-09-03 19:04:49) > The DPU_PINGPONG_TE flag became unused, we can drop it now. > > Reviewed-by: Marijn Suijten > Signed-off-by: Dmitry Baryshkov > --- Reviewed-by: Stephen Boyd
Re: [PATCH v3 2/8] drm/msm/dpu: enable PINGPONG TE operations only when supported by HW
Quoting Dmitry Baryshkov (2023-09-03 19:04:48) > The DPU_PINGPONG_TE bit is set for all PINGPONG blocks on DPU < 5.0. > Rather than checking for the flag, check for the presense of the s/presense/presence/ > corresponding interrupt line. The patch checks for the major version though?
Re: [PATCH v3 1/8] drm/msm/dpu: inline _setup_pingpong_ops()
Quoting Dmitry Baryshkov (2023-09-03 19:04:47) > Inline the _setup_pingpong_ops() function, it makes it easier to handle > different conditions involving PINGPONG configuration. > > Reviewed-by: Marijn Suijten > Signed-off-by: Dmitry Baryshkov > --- Reviewed-by: Stephen Boyd
[PATCH 2/2] drm/msm/dp: Remove error message when downstream port not connected
Plugging in an Apple dongle without the HDMI cable attached prints out
an error message in the kernel logs when nothing is actually wrong.
no downstream ports connected
This is because the downstream port for the HDMI connector is not
connected, so the Apple dongle reports that as a zero sink count device.
Cc: Vinod Polimera
Cc: Kuogee Hsieh
Signed-off-by: Stephen Boyd
---
drivers/gpu/drm/msm/dp/dp_panel.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c
b/drivers/gpu/drm/msm/dp/dp_panel.c
index 97ba41593820..ae778e1a6fd0 100644
--- a/drivers/gpu/drm/msm/dp/dp_panel.c
+++ b/drivers/gpu/drm/msm/dp/dp_panel.c
@@ -156,7 +156,6 @@ int dp_panel_read_sink_caps(struct dp_panel *dp_panel,
if (drm_dp_is_branch(dp_panel->dpcd)) {
count = drm_dp_read_sink_count(panel->aux);
if (!count) {
- DRM_ERROR("no downstream ports connected\n");
panel->link->sink_count = 0;
return -ENOTCONN;
}
--
https://chromeos.dev
[PATCH 1/2] drm/msm/dp: Inline dp_display_is_sink_count_zero()
This function is basically a one-liner when you ignore the debug
logging. Just inline the function and drop the log to simplify the code.
Suggested-by: Dmitry Baryshkov
Cc: Vinod Polimera
Cc: Kuogee Hsieh
Signed-off-by: Stephen Boyd
---
drivers/gpu/drm/msm/dp/dp_display.c | 10 +-
1 file changed, 1 insertion(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
b/drivers/gpu/drm/msm/dp/dp_display.c
index 96bbf6fec2f1..2a5f1ab9a65b 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -341,14 +341,6 @@ static const struct component_ops dp_display_comp_ops = {
.unbind = dp_display_unbind,
};
-static bool dp_display_is_sink_count_zero(struct dp_display_private *dp)
-{
- drm_dbg_dp(dp->drm_dev, "present=%#x sink_count=%d\n",
- dp->panel->dpcd[DP_DOWNSTREAMPORT_PRESENT],
- dp->link->sink_count);
- return drm_dp_is_branch(dp->panel->dpcd) && dp->link->sink_count == 0;
-}
-
static void dp_display_send_hpd_event(struct msm_dp *dp_display)
{
struct dp_display_private *dp;
@@ -507,7 +499,7 @@ static int dp_display_handle_port_ststus_changed(struct
dp_display_private *dp)
{
int rc = 0;
- if (dp_display_is_sink_count_zero(dp)) {
+ if (drm_dp_is_branch(dp->panel->dpcd) && dp->link->sink_count == 0) {
drm_dbg_dp(dp->drm_dev, "sink count is zero, nothing to do\n");
if (dp->hpd_state != ST_DISCONNECTED) {
dp->hpd_state = ST_DISCONNECT_PENDING;
--
https://chromeos.dev
[PATCH 0/2] drm/msm/dp: More DPCD cleanups
This is a follow-on to this series[1]. I can resend the whole pile if desired. [1] https://lore.kernel.org/r/20230829184735.2841739-1-swb...@chromium.org Stephen Boyd (2): drm/msm/dp: Inline dp_display_is_sink_count_zero() drm/msm/dp: Remove error message when downstream port not connected drivers/gpu/drm/msm/dp/dp_display.c | 10 +- drivers/gpu/drm/msm/dp/dp_panel.c | 1 - 2 files changed, 1 insertion(+), 10 deletions(-) base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c prerequisite-patch-id: c637903336fb0fd5593f0016f0c863305c43a6b9 prerequisite-patch-id: 8610693078de9cd5041266c70c4d044d15a5f701 prerequisite-patch-id: e10675f41cc68dcefa566f7f288b2df72afdb116 prerequisite-patch-id: 63280d764b830e3d25455ae642840cff5f90e118 prerequisite-patch-id: 567e00d48c5a296b454079a51483f2acce357347 prerequisite-patch-id: 1c18472728edc1ca8800dd6ed6ff12cb98084ea8 prerequisite-patch-id: c6f74b922b3a4f2255bcdab11fe3a2ecf7891262 -- https://chromeos.dev
Re: [PATCH 7/7] drm/msm/dp: Remove dp_display_is_ds_bridge()
Quoting Dmitry Baryshkov (2023-09-03 15:40:49)
> On 29/08/2023 21:47, Stephen Boyd wrote:
> > This function is simply drm_dp_is_branch() so use that instead of
> > open-coding it.
> >
> > Cc: Vinod Polimera
> > Cc: Kuogee Hsieh
> > Signed-off-by: Stephen Boyd
> > ---
> > drivers/gpu/drm/msm/dp/dp_display.c | 9 +
> > 1 file changed, 1 insertion(+), 8 deletions(-)
>
> Reviewed-by: Dmitry Baryshkov
Thanks.
>
> >
> > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> > b/drivers/gpu/drm/msm/dp/dp_display.c
> > index 76f13954015b..96bbf6fec2f1 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_display.c
> > +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> > @@ -341,19 +341,12 @@ static const struct component_ops dp_display_comp_ops
> > = {
> > .unbind = dp_display_unbind,
> > };
> >
> > -static bool dp_display_is_ds_bridge(struct dp_panel *panel)
> > -{
> > - return (panel->dpcd[DP_DOWNSTREAMPORT_PRESENT] &
> > - DP_DWN_STRM_PORT_PRESENT);
> > -}
> > -
> > static bool dp_display_is_sink_count_zero(struct dp_display_private *dp)
>
> Nit: you might as well inline this function
Ok. I'll send a followup to this series with a patch for that. I found
that with an Apple dongle it always prints out a message to the kernel
log when I have HDMI disconnected that there isn't a sink connected,
which is annoying.
So at least two more patches are incoming.
[PATCH 6/7] drm/msm/dp: Inline dp_link_parse_sink_count()
The function dp_link_parse_sink_count() is really just
drm_dp_read_sink_count(). It debug prints out the bit for content
protection (DP_SINK_CP_READY), but that is not useful beyond debug
because 'link->dp_link.sink_count' is overwritten to only contain the
sink_count in this same function. Just use drm_dp_read_sink_count() in
the one place this function is called to simplify.
Cc: Vinod Polimera
Cc: Kuogee Hsieh
Signed-off-by: Stephen Boyd
---
drivers/gpu/drm/msm/dp/dp_link.c | 38 +++-
1 file changed, 3 insertions(+), 35 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_link.c b/drivers/gpu/drm/msm/dp/dp_link.c
index 42427129acea..94a37914a47f 100644
--- a/drivers/gpu/drm/msm/dp/dp_link.c
+++ b/drivers/gpu/drm/msm/dp/dp_link.c
@@ -712,49 +712,17 @@ static int dp_link_parse_request(struct dp_link_private
*link)
return ret;
}
-/**
- * dp_link_parse_sink_count() - parses the sink count
- * @dp_link: pointer to link module data
- *
- * Parses the DPCD to check if there is an update to the sink count
- * (Byte 0x200), and whether all the sink devices connected have Content
- * Protection enabled.
- */
-static int dp_link_parse_sink_count(struct dp_link *dp_link)
-{
- ssize_t rlen;
- bool cp_ready;
-
- struct dp_link_private *link = container_of(dp_link,
- struct dp_link_private, dp_link);
-
- rlen = drm_dp_dpcd_readb(link->aux, DP_SINK_COUNT,
->dp_link.sink_count);
- if (rlen < 0) {
- DRM_ERROR("sink count read failed. rlen=%zd\n", rlen);
- return rlen;
- }
-
- cp_ready = link->dp_link.sink_count & DP_SINK_CP_READY;
-
- link->dp_link.sink_count =
- DP_GET_SINK_COUNT(link->dp_link.sink_count);
-
- drm_dbg_dp(link->drm_dev, "sink_count = 0x%x, cp_ready = 0x%x\n",
- link->dp_link.sink_count, cp_ready);
- return 0;
-}
-
static int dp_link_parse_sink_status_field(struct dp_link_private *link)
{
- int len = 0;
+ int len;
link->prev_sink_count = link->dp_link.sink_count;
- len = dp_link_parse_sink_count(>dp_link);
+ len = drm_dp_read_sink_count(link->aux);
if (len < 0) {
DRM_ERROR("DP parse sink count failed\n");
return len;
}
+ link->dp_link.sink_count = len;
len = drm_dp_dpcd_read_link_status(link->aux,
link->link_status);
--
https://chromeos.dev
[PATCH 2/7] drm/msm/dp: Use drm_dp_read_sink_count() helper
Use the common function drm_dp_read_sink_count() instead of open-coding
it. This shrinks the kernel text a tiny bit.
Cc: Vinod Polimera
Cc: Kuogee Hsieh
Signed-off-by: Stephen Boyd
---
drivers/gpu/drm/msm/dp/dp_panel.c | 19 +++
1 file changed, 7 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c
b/drivers/gpu/drm/msm/dp/dp_panel.c
index 09d4f6c38ef8..a0523b18b9e9 100644
--- a/drivers/gpu/drm/msm/dp/dp_panel.c
+++ b/drivers/gpu/drm/msm/dp/dp_panel.c
@@ -147,8 +147,8 @@ static int dp_panel_update_modes(struct drm_connector
*connector,
int dp_panel_read_sink_caps(struct dp_panel *dp_panel,
struct drm_connector *connector)
{
- int rc = 0, bw_code;
- int rlen, count;
+ int rc, bw_code;
+ int count;
struct dp_panel_private *panel;
if (!dp_panel || !connector) {
@@ -174,16 +174,11 @@ int dp_panel_read_sink_caps(struct dp_panel *dp_panel,
}
if (dp_panel->dfp_present) {
- rlen = drm_dp_dpcd_read(panel->aux, DP_SINK_COUNT,
- , 1);
- if (rlen == 1) {
- count = DP_GET_SINK_COUNT(count);
- if (!count) {
- DRM_ERROR("no downstream ports connected\n");
- panel->link->sink_count = 0;
- rc = -ENOTCONN;
- goto end;
- }
+ count = drm_dp_read_sink_count(panel->aux);
+ if (!count) {
+ DRM_ERROR("no downstream ports connected\n");
+ panel->link->sink_count = 0;
+ return -ENOTCONN;
}
}
--
https://chromeos.dev
[PATCH 7/7] drm/msm/dp: Remove dp_display_is_ds_bridge()
This function is simply drm_dp_is_branch() so use that instead of
open-coding it.
Cc: Vinod Polimera
Cc: Kuogee Hsieh
Signed-off-by: Stephen Boyd
---
drivers/gpu/drm/msm/dp/dp_display.c | 9 +
1 file changed, 1 insertion(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
b/drivers/gpu/drm/msm/dp/dp_display.c
index 76f13954015b..96bbf6fec2f1 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -341,19 +341,12 @@ static const struct component_ops dp_display_comp_ops = {
.unbind = dp_display_unbind,
};
-static bool dp_display_is_ds_bridge(struct dp_panel *panel)
-{
- return (panel->dpcd[DP_DOWNSTREAMPORT_PRESENT] &
- DP_DWN_STRM_PORT_PRESENT);
-}
-
static bool dp_display_is_sink_count_zero(struct dp_display_private *dp)
{
drm_dbg_dp(dp->drm_dev, "present=%#x sink_count=%d\n",
dp->panel->dpcd[DP_DOWNSTREAMPORT_PRESENT],
dp->link->sink_count);
- return dp_display_is_ds_bridge(dp->panel) &&
- (dp->link->sink_count == 0);
+ return drm_dp_is_branch(dp->panel->dpcd) && dp->link->sink_count == 0;
}
static void dp_display_send_hpd_event(struct msm_dp *dp_display)
--
https://chromeos.dev
[PATCH 5/7] drm/msm/dp: Simplify with drm_dp_{max_link_rate, max_lane_count}()
These are open-coded versions of common functions. Replace them with the common code to improve readability. Cc: Vinod Polimera Cc: Kuogee Hsieh Signed-off-by: Stephen Boyd --- drivers/gpu/drm/msm/dp/dp_panel.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c b/drivers/gpu/drm/msm/dp/dp_panel.c index 0893522ae158..97ba41593820 100644 --- a/drivers/gpu/drm/msm/dp/dp_panel.c +++ b/drivers/gpu/drm/msm/dp/dp_panel.c @@ -58,8 +58,8 @@ static int dp_panel_read_dpcd(struct dp_panel *dp_panel) major = (link_info->revision >> 4) & 0x0f; minor = link_info->revision & 0x0f; - link_info->rate = drm_dp_bw_code_to_link_rate(dpcd[DP_MAX_LINK_RATE]); - link_info->num_lanes = dpcd[DP_MAX_LANE_COUNT] & DP_MAX_LANE_COUNT_MASK; + link_info->rate = drm_dp_max_link_rate(dpcd); + link_info->num_lanes = drm_dp_max_lane_count(dpcd); /* Limit data lanes from data-lanes of endpoint property of dtsi */ if (link_info->num_lanes > dp_panel->max_dp_lanes) -- https://chromeos.dev
[PATCH 4/7] drm/msm/dp: Remove aux_cfg_update_done and related code
The member 'aux_cfg_update_done' is always false. This is dead code that
never runs. Remove it.
Cc: Vinod Polimera
Cc: Kuogee Hsieh
Signed-off-by: Stephen Boyd
---
drivers/gpu/drm/msm/dp/dp_panel.c | 15 ---
1 file changed, 15 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c
b/drivers/gpu/drm/msm/dp/dp_panel.c
index 9fb4e963fefb..0893522ae158 100644
--- a/drivers/gpu/drm/msm/dp/dp_panel.c
+++ b/drivers/gpu/drm/msm/dp/dp_panel.c
@@ -17,7 +17,6 @@ struct dp_panel_private {
struct dp_link *link;
struct dp_catalog *catalog;
bool panel_on;
- bool aux_cfg_update_done;
};
static void dp_panel_read_psr_cap(struct dp_panel_private *panel)
@@ -177,19 +176,6 @@ int dp_panel_read_sink_caps(struct dp_panel *dp_panel,
}
}
- if (panel->aux_cfg_update_done) {
- drm_dbg_dp(panel->drm_dev,
- "read DPCD with updated AUX config\n");
- rc = dp_panel_read_dpcd(dp_panel);
- bw_code = drm_dp_link_rate_to_bw_code(dp_panel->link_info.rate);
- if (rc || !is_link_rate_valid(bw_code) ||
- !is_lane_count_valid(dp_panel->link_info.num_lanes)
- || (bw_code > dp_panel->max_bw_code)) {
- DRM_ERROR("read dpcd failed %d\n", rc);
- return rc;
- }
- panel->aux_cfg_update_done = false;
- }
end:
return rc;
}
@@ -434,7 +420,6 @@ struct dp_panel *dp_panel_get(struct dp_panel_in *in)
dp_panel = >dp_panel;
dp_panel->max_bw_code = DP_LINK_BW_8_1;
- panel->aux_cfg_update_done = false;
return dp_panel;
}
--
https://chromeos.dev
[PATCH 3/7] drm/msm/dp: Remove dead code related to downstream cap info
We read the downstream port count and capability info but never use it
anywhere. Remove 'ds_port_cnt' and 'ds_cap_info' and any associated code
from this driver. Fold the check for 'dfp_present' into a call to
drm_dp_is_branch() at the one place it is used to get rid of any member
storage related to downstream ports.
Cc: Vinod Polimera
Cc: Kuogee Hsieh
Signed-off-by: Stephen Boyd
---
drivers/gpu/drm/msm/dp/dp_panel.c | 25 +++--
drivers/gpu/drm/msm/dp/dp_panel.h | 6 --
2 files changed, 3 insertions(+), 28 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c
b/drivers/gpu/drm/msm/dp/dp_panel.c
index a0523b18b9e9..9fb4e963fefb 100644
--- a/drivers/gpu/drm/msm/dp/dp_panel.c
+++ b/drivers/gpu/drm/msm/dp/dp_panel.c
@@ -43,9 +43,7 @@ static void dp_panel_read_psr_cap(struct dp_panel_private
*panel)
static int dp_panel_read_dpcd(struct dp_panel *dp_panel)
{
- int rc = 0;
- size_t len;
- ssize_t rlen;
+ int rc;
struct dp_panel_private *panel;
struct dp_link_info *link_info;
u8 *dpcd, major, minor;
@@ -79,25 +77,8 @@ static int dp_panel_read_dpcd(struct dp_panel *dp_panel)
if (drm_dp_enhanced_frame_cap(dpcd))
link_info->capabilities |= DP_LINK_CAP_ENHANCED_FRAMING;
- dp_panel->dfp_present = dpcd[DP_DOWNSTREAMPORT_PRESENT];
- dp_panel->dfp_present &= DP_DWN_STRM_PORT_PRESENT;
-
- if (dp_panel->dfp_present && (dpcd[DP_DPCD_REV] > 0x10)) {
- dp_panel->ds_port_cnt = dpcd[DP_DOWN_STREAM_PORT_COUNT];
- dp_panel->ds_port_cnt &= DP_PORT_COUNT_MASK;
- len = DP_DOWNSTREAM_PORTS * DP_DOWNSTREAM_CAP_SIZE;
-
- rlen = drm_dp_dpcd_read(panel->aux,
- DP_DOWNSTREAM_PORT_0, dp_panel->ds_cap_info, len);
- if (rlen < len) {
- DRM_ERROR("ds port status failed, rlen=%zd\n", rlen);
- rc = -EINVAL;
- goto end;
- }
- }
-
dp_panel_read_psr_cap(panel);
-end:
+
return rc;
}
@@ -173,7 +154,7 @@ int dp_panel_read_sink_caps(struct dp_panel *dp_panel,
return -EINVAL;
}
- if (dp_panel->dfp_present) {
+ if (drm_dp_is_branch(dp_panel->dpcd)) {
count = drm_dp_read_sink_count(panel->aux);
if (!count) {
DRM_ERROR("no downstream ports connected\n");
diff --git a/drivers/gpu/drm/msm/dp/dp_panel.h
b/drivers/gpu/drm/msm/dp/dp_panel.h
index 6d733480a62d..3cb1f8dcfd3b 100644
--- a/drivers/gpu/drm/msm/dp/dp_panel.h
+++ b/drivers/gpu/drm/msm/dp/dp_panel.h
@@ -13,9 +13,6 @@
struct edid;
-#define DP_DOWNSTREAM_PORTS4
-#define DP_DOWNSTREAM_CAP_SIZE 4
-
struct dp_display_mode {
struct drm_display_mode drm_mode;
u32 capabilities;
@@ -39,9 +36,6 @@ struct dp_panel_psr {
struct dp_panel {
/* dpcd raw data */
u8 dpcd[DP_RECEIVER_CAP_SIZE];
- u8 ds_cap_info[DP_DOWNSTREAM_PORTS * DP_DOWNSTREAM_CAP_SIZE];
- u32 ds_port_cnt;
- u32 dfp_present;
struct dp_link_info link_info;
struct drm_dp_desc desc;
--
https://chromeos.dev
[PATCH 1/7] drm/msm/dp: Replace open-coded drm_dp_read_dpcd_caps()
This function duplicates the common function drm_dp_read_dpcd_caps().
The array of DPCD registers filled in is one size larger than the
function takes, but from what I can tell that extra byte was never used.
Resize the array and use the common function to reduce the code here.
Cc: Vinod Polimera
Cc: Kuogee Hsieh
Signed-off-by: Stephen Boyd
---
drivers/gpu/drm/msm/dp/dp_panel.c | 42 ---
drivers/gpu/drm/msm/dp/dp_panel.h | 4 +--
2 files changed, 6 insertions(+), 40 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c
b/drivers/gpu/drm/msm/dp/dp_panel.c
index 42d52510ffd4..09d4f6c38ef8 100644
--- a/drivers/gpu/drm/msm/dp/dp_panel.c
+++ b/drivers/gpu/drm/msm/dp/dp_panel.c
@@ -48,47 +48,15 @@ static int dp_panel_read_dpcd(struct dp_panel *dp_panel)
ssize_t rlen;
struct dp_panel_private *panel;
struct dp_link_info *link_info;
- u8 *dpcd, major = 0, minor = 0, temp;
- u32 offset = DP_DPCD_REV;
+ u8 *dpcd, major, minor;
+ panel = container_of(dp_panel, struct dp_panel_private, dp_panel);
dpcd = dp_panel->dpcd;
+ rc = drm_dp_read_dpcd_caps(panel->aux, dpcd);
+ if (rc)
+ return rc;
- panel = container_of(dp_panel, struct dp_panel_private, dp_panel);
link_info = _panel->link_info;
-
- rlen = drm_dp_dpcd_read(panel->aux, offset,
- dpcd, (DP_RECEIVER_CAP_SIZE + 1));
- if (rlen < (DP_RECEIVER_CAP_SIZE + 1)) {
- DRM_ERROR("dpcd read failed, rlen=%zd\n", rlen);
- if (rlen == -ETIMEDOUT)
- rc = rlen;
- else
- rc = -EINVAL;
-
- goto end;
- }
-
- temp = dpcd[DP_TRAINING_AUX_RD_INTERVAL];
-
- /* check for EXTENDED_RECEIVER_CAPABILITY_FIELD_PRESENT */
- if (temp & BIT(7)) {
- drm_dbg_dp(panel->drm_dev,
- "using EXTENDED_RECEIVER_CAPABILITY_FIELD\n");
- offset = DPRX_EXTENDED_DPCD_FIELD;
- }
-
- rlen = drm_dp_dpcd_read(panel->aux, offset,
- dpcd, (DP_RECEIVER_CAP_SIZE + 1));
- if (rlen < (DP_RECEIVER_CAP_SIZE + 1)) {
- DRM_ERROR("dpcd read failed, rlen=%zd\n", rlen);
- if (rlen == -ETIMEDOUT)
- rc = rlen;
- else
- rc = -EINVAL;
-
- goto end;
- }
-
link_info->revision = dpcd[DP_DPCD_REV];
major = (link_info->revision >> 4) & 0x0f;
minor = link_info->revision & 0x0f;
diff --git a/drivers/gpu/drm/msm/dp/dp_panel.h
b/drivers/gpu/drm/msm/dp/dp_panel.h
index ed1030e17e1b..6d733480a62d 100644
--- a/drivers/gpu/drm/msm/dp/dp_panel.h
+++ b/drivers/gpu/drm/msm/dp/dp_panel.h
@@ -13,8 +13,6 @@
struct edid;
-#define DPRX_EXTENDED_DPCD_FIELD 0x2200
-
#define DP_DOWNSTREAM_PORTS4
#define DP_DOWNSTREAM_CAP_SIZE 4
@@ -40,7 +38,7 @@ struct dp_panel_psr {
struct dp_panel {
/* dpcd raw data */
- u8 dpcd[DP_RECEIVER_CAP_SIZE + 1];
+ u8 dpcd[DP_RECEIVER_CAP_SIZE];
u8 ds_cap_info[DP_DOWNSTREAM_PORTS * DP_DOWNSTREAM_CAP_SIZE];
u32 ds_port_cnt;
u32 dfp_present;
--
https://chromeos.dev
[PATCH 0/7] drm/msm/dp: Simplify DPCD related code with helpers
This driver open-codes a few of the DPCD register reads when it can be
simplified by using the helpers instead. This series reworks the MSM DP
driver to use the DPCD helpers and removes some dead code along the way.
There's the potential for even more code reduction around the test
registers, but I haven't tried to do that yet.
Stephen Boyd (7):
drm/msm/dp: Replace open-coded drm_dp_read_dpcd_caps()
drm/msm/dp: Use drm_dp_read_sink_count() helper
drm/msm/dp: Remove dead code related to downstream cap info
drm/msm/dp: Remove aux_cfg_update_done and related code
drm/msm/dp: Simplify with drm_dp_{max_link_rate,max_lane_count}()
drm/msm/dp: Inline dp_link_parse_sink_count()
drm/msm/dp: Remove dp_display_is_ds_bridge()
Cc: Vinod Polimera
Cc: Kuogee Hsieh
drivers/gpu/drm/msm/dp/dp_display.c | 9 +--
drivers/gpu/drm/msm/dp/dp_link.c| 38 +-
drivers/gpu/drm/msm/dp/dp_panel.c | 105 +---
drivers/gpu/drm/msm/dp/dp_panel.h | 10 +--
4 files changed, 22 insertions(+), 140 deletions(-)
base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c
--
https://chromeos.dev
[PATCH] drm/connector: Add newline to debug printk
This debug printk is missing a newline, causing drm debug logs to be
hard to read. Add a newline so that the message is on its own line.
Cc: Simon Ser
Cc: Pekka Paalanen
Cc: Daniel Vetter
Fixes: 8f86c82aba8b ("drm/connector: demote connector force-probes for
non-master clients")
Signed-off-by: Stephen Boyd
---
drivers/gpu/drm/drm_connector.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 3ed4cfcb350c..ab1467a15f9a 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -2923,7 +2923,7 @@ int drm_mode_getconnector(struct drm_device *dev, void
*data,
dev->mode_config.max_width,
dev->mode_config.max_height);
else
- drm_dbg_kms(dev, "User-space requested a forced probe
on [CONNECTOR:%d:%s] but is not the DRM master, demoting to read-only probe",
+ drm_dbg_kms(dev, "User-space requested a forced probe
on [CONNECTOR:%d:%s] but is not the DRM master, demoting to read-only probe\n",
connector->base.id, connector->name);
}
base-commit: 706a741595047797872e669b3101429ab8d378ef
--
https://chromeos.dev
[PATCH] drm/msm/dp: Add newlines to debug printks
These debug printks are missing newlines, causing drm debug logs to be
hard to read. Add newlines so that the messages are on their own line.
Cc: Kuogee Hsieh
Cc: Vinod Polimera
Signed-off-by: Stephen Boyd
---
drivers/gpu/drm/msm/dp/dp_link.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_link.c b/drivers/gpu/drm/msm/dp/dp_link.c
index 42427129acea..6375daaeb98e 100644
--- a/drivers/gpu/drm/msm/dp/dp_link.c
+++ b/drivers/gpu/drm/msm/dp/dp_link.c
@@ -1090,7 +1090,7 @@ int dp_link_process_request(struct dp_link *dp_link)
} else if (dp_link_read_psr_error_status(link)) {
DRM_ERROR("PSR IRQ_HPD received\n");
} else if (dp_link_psr_capability_changed(link)) {
- drm_dbg_dp(link->drm_dev, "PSR Capability changed");
+ drm_dbg_dp(link->drm_dev, "PSR Capability changed\n");
} else {
ret = dp_link_process_link_status_update(link);
if (!ret) {
@@ -1107,7 +1107,7 @@ int dp_link_process_request(struct dp_link *dp_link)
}
}
- drm_dbg_dp(link->drm_dev, "sink request=%#x",
+ drm_dbg_dp(link->drm_dev, "sink request=%#x\n",
dp_link->sink_request);
return ret;
}
base-commit: 706a741595047797872e669b3101429ab8d378ef
--
https://chromeos.dev
Re: [PATCH v1] drm/msm/dp: do not reinitialize phy unless retry during link training
Quoting Kuogee Hsieh (2023-08-08 15:19:50)
> DP PHY re-initialization done using dp_ctrl_reinitialize_mainlink() will
> cause PLL unlocked initially and then PLL gets locked at the end of
> initialization. PLL_UNLOCKED interrupt will fire during this time if the
> interrupt mask is enabled.
> However currently DP driver link training implementation incorrectly
> re-initializes PHY unconditionally during link training as the PHY was
> already configured in dp_ctrl_enable_mainlink_clocks().
>
> Fix this by re-initializing the PHY only if the previous link training
> failed.
>
> [drm:dp_aux_isr] *ERROR* Unexpected DP AUX IRQ 0x0100 when not busy
>
> Fixes: c943b4948b58 ("drm/msm/dp: add displayPort driver support")
> Closes: https://gitlab.freedesktop.org/drm/msm/-/issues/30
> Signed-off-by: Kuogee Hsieh
> ---
Reviewed-by: Stephen Boyd
Re: [PATCH] clk: Annotate struct clk_hw_onecell_data with __counted_by
Quoting Kees Cook (2023-08-17 13:30:22) > Prepare for the coming implementation by GCC and Clang of the __counted_by > attribute. Flexible array members annotated with __counted_by can have > their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS > (for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family > functions). > > As found with Coccinelle[1], add __counted_by for struct clk_hw_onecell_data. > Additionally, since the element count member must be set before accessing > the annotated flexible array member, move its initialization earlier. > > [1] > https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci > > Cc: Michael Turquette > Cc: Stephen Boyd > Cc: Joel Stanley > Cc: Andrew Jeffery > Cc: Taichi Sugaya > Cc: Takao Orito > Cc: Qin Jian > Cc: Andrew Lunn > Cc: Gregory Clement > Cc: Sebastian Hesselbarth > Cc: Andy Gross > Cc: Bjorn Andersson > Cc: Konrad Dybcio > Cc: Sergio Paracuellos > Cc: Matthias Brugger > Cc: AngeloGioacchino Del Regno > Cc: Maxime Ripard > Cc: Chen-Yu Tsai > Cc: Jernej Skrabec > Cc: David Airlie > Cc: Daniel Vetter > Cc: Samuel Holland > Cc: Vinod Koul > Cc: Kishon Vijay Abraham I > Cc: linux-...@vger.kernel.org > Cc: linux-arm-ker...@lists.infradead.org > Cc: linux-asp...@lists.ozlabs.org > Cc: linux-arm-...@vger.kernel.org > Cc: linux-media...@lists.infradead.org > Cc: dri-devel@lists.freedesktop.org > Cc: linux-su...@lists.linux.dev > Cc: linux-...@lists.infradead.org > Signed-off-by: Kees Cook > --- Applied to clk-next
Re: [PATCH v2] drm/bridge: ti-sn65dsi86: Fix auxiliary bus lifetime
Quoting Douglas Anderson (2023-06-13 06:58:13)
> Memory for the "struct device" for any given device isn't supposed to
> be released until the device's release() is called. This is important
> because someone might be holding a kobject reference to the "struct
> device" and might try to access one of its members even after any
> other cleanup/uninitialization has happened.
>
> Code analysis of ti-sn65dsi86 shows that this isn't quite right. When
> the code was written, it was believed that we could rely on the fact
> that the child devices would all be freed before the parent devices
> and thus we didn't need to worry about a release() function. While I
> still believe that the parent's "struct device" is guaranteed to
> outlive the child's "struct device" (because the child holds a kobject
> reference to the parent), the parent's "devm" allocated memory is a
> different story. That appears to be freed much earlier.
>
> Let's make this better for ti-sn65dsi86 by allocating each auxiliary
> with kzalloc and then free that memory in the release().
>
> Fixes: bf73537f411b ("drm/bridge: ti-sn65dsi86: Break GPIO and MIPI-to-eDP
> bridge into sub-drivers")
> Suggested-by: Stephen Boyd
> Signed-off-by: Douglas Anderson
> ---
Reviewed-by: Stephen Boyd
Re: [PATCH] drm/bridge: ti-sn65dsi86: Fix auxiliary bus lifetime
Quoting Douglas Anderson (2023-06-12 16:53:03)
> Memory for the "struct device" for any given device isn't supposed to
> be released until the device's release() is called. This is important
> because someone might be holding a kobject reference to the "struct
> device" and might try to access one of its members even after any
> other cleanup/uninitialization has happened.
>
> Code analysis of ti-sn65dsi86 shows that this isn't quite right. When
> the code was written, it was believed that we could rely on the fact
> that the child devices would all be freed before the parent devices
> and thus we didn't need to worry about a release() function. While I
> still believe that the parent's "struct device" is guaranteed to
> outlive the child's "struct device" (because the child holds a kobject
> reference to the parent), the parent's "devm" allocated memory is a
> different story. That appears to be freed much earlier.
>
> Let's make this better for ti-sn65dsi86 by allocating each auxiliary
> with kzalloc and then free that memory in the release().
>
> Fixes: bf73537f411b ("drm/bridge: ti-sn65dsi86: Break GPIO and MIPI-to-eDP
> bridge into sub-drivers")
> Suggested-by: Stephen Boyd
> Signed-off-by: Douglas Anderson
> ---
Thanks!
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index 597ceb7024e0..db1461cc3170 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -464,27 +464,32 @@ static void ti_sn65dsi86_delete_aux(void *data)
> auxiliary_device_delete(data);
> }
>
> -/*
> - * AUX bus docs say that a non-NULL release is mandatory, but it makes no
> - * sense for the model used here where all of the aux devices are allocated
> - * in the single shared structure. We'll use this noop as a workaround.
> - */
> -static void ti_sn65dsi86_noop(struct device *dev) {}
> +static void ti_sn65dsi86_aux_device_release(struct device *dev)
> +{
> + struct auxiliary_device *aux = container_of(dev, struct
> auxiliary_device, dev);
> +
> + kfree(aux);
> +}
>
> static int ti_sn65dsi86_add_aux_device(struct ti_sn65dsi86 *pdata,
> - struct auxiliary_device *aux,
> + struct auxiliary_device **aux_out,
>const char *name)
> {
> struct device *dev = pdata->dev;
> + struct auxiliary_device *aux;
> int ret;
>
> + aux = kzalloc(sizeof(*aux), GFP_KERNEL);
Check for allocation failure?
> +
> aux->name = name;
> aux->dev.parent = dev;
> - aux->dev.release = ti_sn65dsi86_noop;
> + aux->dev.release = ti_sn65dsi86_aux_device_release;
> device_set_of_node_from_dev(>dev, dev);
> ret = auxiliary_device_init(aux);
> - if (ret)
> + if (ret) {
> + kfree(aux);
> return ret;
> + }
> ret = devm_add_action_or_reset(dev, ti_sn65dsi86_uninit_aux, aux);
> if (ret)
> return ret;
> @@ -494,6 +499,9 @@ static int ti_sn65dsi86_add_aux_device(struct
> ti_sn65dsi86 *pdata,
> return ret;
> ret = devm_add_action_or_reset(dev, ti_sn65dsi86_delete_aux, aux);
>
Nitpick: Stick this if line to the line above
> + if (!ret)
> + *aux_out = aux;
> +
> return ret;
Re: [PATCH v4 00/68] clk: Make determine_rate mandatory for muxes
Quoting Maxime Ripard (2023-05-05 04:25:02) > Hi, > > This is a follow-up to a previous series that was printing a warning > when a mux has a set_parent implementation but is missing > determine_rate(). > > The rationale is that set_parent() is very likely to be useful when > changing the rate, but it's determine_rate() that takes the parenting > decision. If we're missing it, then the current parent is always going > to be used, and thus set_parent() will not be used. The only exception > being a direct call to clk_set_parent(), but those are fairly rare > compared to clk_set_rate(). > > Stephen then asked to promote the warning to an error, and to fix up all > the muxes that are in that situation first. So here it is :) > I've applied this to clk-next after squashing in an export. Thanks for taking this on. I'll have to monitor for wreckage with any in-flight drivers. I suspect I'll have to take out the last commit, but maybe we can just let those in-flight drivers get fixed up later.
Re: [PATCH v1 1/2] drm/msm/dp: enable HDP plugin/unplugged interrupts to hpd_enable/disable
Quoting Dmitry Baryshkov (2023-05-11 17:54:19)
> On Fri, 12 May 2023 at 03:16, Kuogee Hsieh wrote:
> > 1) DP with GPIO: No downstream drm_bridge are connected, is_edp = false
> > and internal HPD-logic is in used (internal_hpd = true). Power needs to
> > be on at all times etc.
> >
> > 2) DP without GPIO: Downstream drm_bridge connected, is_edp = false and
> > internal HPD-logic should not be used/enabled (internal_hpd = false).
> > Power doesn't need to be on unless hpd_notify is invoked to tell us that
> > there's something connected...
> >
> > - dp_bridge_hpd_notify(). What is the point of this check? <== i have
> > below two questions,
> >
> > 1) can you explain when/what this dp_bridge_hpd_notify() will be called?
>
> The call chain is drm_bridge_hpd_notify() ->
> drm_bridge_connector_hpd_notify() -> .hpd_notify() for all drm_bridge
> in chain
>
> One should add a call to drm_bridge_hpd_notify() when the hotplug
> event has been detected.
>
> Also please note the patch https://patchwork.freedesktop.org/patch/484432/
>
> >
> > 2) is dp_bridge_hpd_notify() only will be called at above case #2? and
> > it will not be used by case #1?
>
> Once the driver calls drm_bridge_hpd_notify() in the hpd path, the
> hpd_notify callbacks will be called in case#1 too.
>
> BTW: I don't see drm_bridge_hpd_notify() or
> drm_kms_{,connector_}_hotplug_event() HPD notifications in the DP
> driver at all. This should be fixed.
>
Is dp_bridge_hpd_notify() being called by
drm_helper_probe_single_connector_modes() when the connectors are
detected?
I see drm_helper_probe_detect() calls connector->funcs->detect() which I
think calls
drm_bridge_connector_funcs::drm_bridge_connector_hpd_notify() but I
haven't confirmed yet. The 'detect' bridge is the DP bridge in msm
driver
if (!dp_display->is_edp) {
bridge->ops =
DRM_BRIDGE_OP_DETECT |
so if the bridge_connector is being used then I think when fill_modes()
is called we'll run the detect cycle and call the hpd_notify callbacks
on the bridge chain.
Re: [PATCH v1 1/2] drm/msm/dp: enable HDP plugin/unplugged interrupts to hpd_enable/disable
Quoting Bjorn Andersson (2023-05-11 08:44:16) > On Wed, May 10, 2023 at 05:39:07PM -0700, Abhinav Kumar wrote: > > > > > > On 5/10/2023 4:55 PM, Stephen Boyd wrote: > > > Quoting Kuogee Hsieh (2023-05-10 13:31:04) > > > > The internal_hpd flag was introduced to handle external DP HPD derived > > > > from GPIO > > > > pinmuxed into DP controller. > > > > > > Was it? It looks more like it was done to differentiate between eDP and > > > DP, because internal_hpd is set only if DRM_BRIDGE_OP_HPD is set on the > > > bridge and we only set the bridge op if the connector type is DP. The > > > assumption looks like if you have DP connector_type, you have the gpio > > > pinmuxed for "dp_hot" mode, which isn't exactly true. We don't treat > > > that gpio as an irq either, because it isn't. Instead the gpio is muxed > > > to the mdss inside the SoC and then that generates an mdss interrupt > > > that's combined with non-HPD things like "video ready". > > > > > > If that all follows, then I don't quite understand why we're setting > > > internal_hpd to false at all at runtime. It should be set to true at > > > some point, but ideally that point is during probe. > > > > > > > Kuogee had the same thought originally but were not entirely sure of this > > part of the commit message in Bjorn's original commit which introduced these > > changes. > > > > "This difference is not appropriately represented by the "is_edp" > > boolean, but is properly represented by the frameworks invocation of the > > hpd_enable() and hpd_disable() callbacks. Switch the current condition > > to rely on these callbacks instead" > > > > Does this along with below documentation mean we should generate the hpd > > interrupts only after hpd_enable callback happens? > > > > " * Call _bridge_funcs.hpd_enable if implemented and register the given > > @cb > > * and @data as hot plug notification callback. From now on the @cb will be > > * called with @data when an output status change is detected by the bridge, > > * until hot plug notification gets disabled with drm_bridge_hpd_disable(). > > " > > > > Bjorn, can you please clarify this? > > > > We currently have 3 cases: > > 1) DP with GPIO: No downstream drm_bridge are connected, is_edp = false > and internal HPD-logic is in used (internal_hpd = true). Power needs to > be on at all times etc. > > 2) DP without GPIO: Downstream drm_bridge connected, is_edp = false and > internal HPD-logic should not be used/enabled (internal_hpd = false). > Power doesn't need to be on unless hpd_notify is invoked to tell us that > there's something connected... > > 3) eDP with or without HPD signal and/or HPD gpio. Downstream > drm_bridge/panel is connected, is_edp = true and internal HPD logic is > short-circuited regardless of the panel providing HPD signal or not. Oh weird. I thought that the "is_edp" controller on sc7280 didn't have HPD hardware in the PHY (phy@aec2a00), hence all the logic to avoid using the HPD interrupts and bits there. What is "is_edp" about then? > > > In #1 dp_bridge_hpd_enable() will be invoked to indicate that the DP > controller is expected to perform HPD handling. In #2 > dp_bridge_hpd_enable() will _not_ be invoked, instead some downstream > drm_bridge/panel will get the hpd_enable() callback and will be > responsible to updating the HPD state of the chain, which will cause > hpd_notify to be invoked. > > > Note that #3 is based entirely on the controller, it has currently no > relation to what is attached. It seems reasonable that this is just > another case of #2 (perhaps just always reporting > connector_status_connected?). > Looking at drm_bridge_connector_detect() the default is to consider DRM_MODE_CONNECTOR_eDP as connector_status_connected. I wonder if panel_bridge_bridge_funcs can gain a 'detect' function and also set DRM_BRIDGE_OP_DETECT if the connector_type is DRM_MODE_CONNECTOR_eDP.
Re: [PATCH v15 04/10] dt-bindings: display: bridge: anx7625: Add mode-switch support
Quoting Pin-yen Lin (2023-05-09 20:41:54) > On Sat, Apr 29, 2023 at 12:47 PM Stephen Boyd wrote: > > > > Good point. I'm suggesting to make the drm bridge chain into a tree. We > > need to teach drm_bridge core about a mux, and have some logic to > > navigate atomically switching from one output to another. I was talking > > with dianders@ and he was suggesting to use bridge attach/detach for > > this. I'm not sure that will work though because that hook is only > > called when the encoder is attached to the bridge. > > > > It may also turn out that this helps with DP multi-stream transport > > (MST). As far as I can recall DP MST doesn't mesh well with drm_bridge > > designs because it wants to operate on a drm_connector and > > drm_bridge_connector_init() wants to make only one drm_connector for a > > chain of bridges. If you squint, the anx7625 could be an MST "branch" > > that only supports one drm_connector being enabled at a time. Maybe that > > is what we should do here, make drm_bridge support creating more than > > one drm_connector and when there is a mux in the tree it walks both > > sides and runs a callback similar to struct > > drm_dp_mst_topology_cbs::add_connector() to tell the encoder that > > there's another possible drm_connector here. > > I have been surveying the approaches to change the bridge chain in > runtime, and I found this thread[1]. Quoting from Daniel: I find the lore links easier to read. > "... exchanging the bridge chain isn't supported, there's no locking > for that since it's assumed to be invariant over the lifetime of the > drm_device instance. The simplest way to make that happen right now is to > have 2 drm_encoder instances, one with the lvds bridge chain, the other > with the hdmi bridge chain, and select the right encoder/bridge chain > depending upon which output userspace picks. > ... > I wouldn't try to make bridge chains exchangeable instead, that's > headaches - e.g. with dp mst we've also opted for a bunch of fake > drm_encoders to model that kind of switching." > > I'm not sure how we register two encoders properly, though. Do we make > the encoder driver check all the downstream bridges and register two > drm_encoder when a bridge is acting as a mux? I honestly don't know because I'm not a DRM expert. Maybe Jagan or DRM bridge maintainers can add to the thread here. I kept reading the thread[2] and I think they settled on 2 drm_encoders because they're different display formats (LVDS vs. HDMI) and 2 drm_connector structures. But then I watched the youtube video from this thread[3] and it seems like 1 drm_encoder is actually what should be done because there is really only DSI at the root. There's at least three people working on this topic of muxing displays though. Good news? The analogy between their gpio controlled mux and this anx part with a crosspoint switch is that the gpio is like the crosspoint switch, but the gpio is like a virtual mux? If the gpio is asserted for them, one display bridge is enabled (HDMI) and the other one is not (LVDS). In this case, the type-c cables may be connected to both usb-c-connectors and HPD may be asserted on both, but only one drm_connector will be able to attach to the 1 drm_encoder at a time. If we had 2 drm_encoders it would be possible to attach both encoders to both drm_connectors at the same time, which is impossible because it's a mux. Indicating that each connector is connected is OK when HPD is high on both usb-c-connectors. Userspace will have to attach an encoder to the drm_connector it wants to use, and the drm_connector will indicate which drm_encoders are possible for it, so all the information will be provided to userspace in this design. I think it really comes down to implementing the tree walking logic in the drm bridge APIs. The problem is things like drm_bridge_get_next_bridge(), drm_bridge_get_prev_bridge(), and drm_for_each_bridge_in_chain() which will have to operate on a tree instead of a list. There's also drm_bridge_chain_get_first_bridge() and drm_bridge_attach(). The good news is most of these APIs are used sparingly. Maybe the simplest way to handle this is to maintain a tree of bridges and generate bridge chains when an encoder is attached to a connector in the tree. Presumably there is only one connector possible for a leaf of the bridge tree, and one encoder at the root of the bridge chain. From the drm_bridge structure, you'll only be able to iterate over the bridge chain that is currently configured. Same for the encoder, you'll only be able to walk the currently configured bridge chain from struct drm_encoder::bridge_chain. This hinges on the idea that the bridge_chain is a list, not a tree, and that it only needs to exist when an encoder is attached to a connector. When an encode
Re: [PATCH v1 1/2] drm/msm/dp: enable HDP plugin/unplugged interrupts to hpd_enable/disable
Quoting Kuogee Hsieh (2023-05-10 13:31:04)
> The internal_hpd flag was introduced to handle external DP HPD derived from
> GPIO
> pinmuxed into DP controller.
Was it? It looks more like it was done to differentiate between eDP and
DP, because internal_hpd is set only if DRM_BRIDGE_OP_HPD is set on the
bridge and we only set the bridge op if the connector type is DP. The
assumption looks like if you have DP connector_type, you have the gpio
pinmuxed for "dp_hot" mode, which isn't exactly true. We don't treat
that gpio as an irq either, because it isn't. Instead the gpio is muxed
to the mdss inside the SoC and then that generates an mdss interrupt
that's combined with non-HPD things like "video ready".
If that all follows, then I don't quite understand why we're setting
internal_hpd to false at all at runtime. It should be set to true at
some point, but ideally that point is during probe.
> HPD plug/unplug interrupts cannot be enabled until
> internal_hpd flag is set to true.
> At both bootup and resume time, the DP driver will enable external DP
> plugin interrupts and handle plugin interrupt accordingly. Unfortunately
> dp_bridge_hpd_enable() bridge ops function was called to set internal_hpd
> flag to true later than where DP driver expected during bootup time.
>
> This causes external DP plugin event to not get detected and display stays
> blank.
> Move enabling HDP plugin/unplugged interrupts to
> dp_bridge_hpd_enable()/disable() to
> set internal_hpd to true along with enabling HPD plugin/unplugged interrupts
> simultaneously to avoid timing issue during bootup and resume.
>
> Fixes: cd198caddea7 ("drm/msm/dp: Rely on hpd_enable/disable callbacks")
> Signed-off-by: Kuogee Hsieh
> ---
> drivers/gpu/drm/msm/dp/dp_display.c | 27 ++-
> 1 file changed, 14 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> b/drivers/gpu/drm/msm/dp/dp_display.c
> index 3e13acdf..71aa944 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -1801,15 +1788,29 @@ void dp_bridge_hpd_enable(struct drm_bridge *bridge)
> {
> struct msm_dp_bridge *dp_bridge = to_dp_bridge(bridge);
> struct msm_dp *dp_display = dp_bridge->dp_display;
> + struct dp_display_private *dp;
> +
> + dp = container_of(dp_display, struct dp_display_private, dp_display);
>
> dp_display->internal_hpd = true;
Can we set internal_hpd to true during probe when we see that the hpd
pinmux exists? Or do any of these bits toggle in the irq status register
when the gpio isn't muxed to "dp_hot" or the controller is for eDP and
it doesn't have any gpio connection internally? I'm wondering if we can
get by with simply enabling the "dp_hot" pin interrupts
(plug/unplug/replug/irq_hpd) unconditionally and not worrying about them
if eDP is there (because the pin doesn't exist inside the SoC), or if DP
HPD is being signalled out of band through type-c framework.
Re: [PATCH v1 2/2] drm/msm/dp: add mutex to protect internal_hpd against race condition between different threads
Quoting Kuogee Hsieh (2023-05-10 13:31:05) > Intrenal_hpd is referenced by event thread but set by drm bridge callback > context. Add mutex to protect internal_hpd to avoid conflicts between > threads. > > Signed-off-by: Kuogee Hsieh > --- This patch looks completely unnecessary. How can dp_bridge_hpd_enable() be called at the same time that dp_bridge_hpd_disable() is called or dp_bridge_hpd_notify() is called? Isn't there locking or ordering at a higher layer?
Re: [PATCH v15 04/10] dt-bindings: display: bridge: anx7625: Add mode-switch support
Quoting Pin-yen Lin (2023-04-20 02:10:46) > On Thu, Apr 20, 2023 at 2:10 PM Stephen Boyd wrote: > > > > Quoting Stephen Boyd (2023-04-13 17:22:46) > > > Quoting Pin-yen Lin (2023-04-13 02:50:44) > > > > > > > > Actually the `mode-switch` property here is mainly because > > > > `fwnode_typec_mux_get`[1] and `typec_mux_match`[2] only return matches > > > > when the property is present. I am not sure what side effects would be > > > > if I remove the ID-matching condition in `typec_mux_match`, so I added > > > > the property here. > > > > > > > > Is it feasible to remove the `mode-switch` property here given the > > > > existing implementation of the Type-C framework? > > > > > > Omitting the mode-switch property would require changes to the type-c > > > framework. > > > > > > I'm wondering if we can have this anx driver register mode switches for > > > however many endpoints exist in the output port all the time when the > > > aux-bus node doesn't exist. Then the type-c framework can walk from the > > > usb-c-connector to each connected node looking for a device that is both > > > a drm_bridge and a mode-switch. When it finds that combination, it knows > > > that the mode-switch has been found. This hinges on the idea that a > > > device that would have the mode-switch property is a drm_bridge and > > > would register a mode-switch with the type-c framework. > > > > > > It may be a little complicated though, because we would only register > > > one drm_bridge for the input to this anx device. The type-c walking code > > > would need to look at the graph endpoint, and find the parent device to > > > see if it is a drm_bridge. > > > > I've been thinking more about this. I think we should only have the > > 'mode-switch' property possible when the USB input pins (port@2) are > > connected and the DPI input pins are connected (port@0). Probably you > > don't have that case though? > > No we don't have the use case that uses the USB input pins on anx7625. > > > > In your case, this device should register either one or two drm_bridges > > that connect to whatever downstream is actually muxing the 2 DP lanes > > with the USB SS lanes onto the usb-c-connector. > > What do you mean by "muxing the 2 DP lanes with the USB SS lanes''? In > our use case, the USB data lanes from both ports are connected to a > USB hub, but the DP lanes are muxed by the crosspoint switch on > anx7625. HPD and AUX for the external display are muxed by the EC. You > can find the diagram at > https://lore.kernel.org/linux-usb/yxgzk6dnat0ac...@chromium.org/ I mean that you must have some sort of orientation switch hardware that takes the 2 DP lanes and the 2 USB SuperSpeed "lanes" or "pairs" and puts them all onto a usb-c-connector. The usb-c-connector node can't be connected directly to the anx7625 in your diagram because there must be some sort of "flipper" that does the orientation control. Otherwise the usb-c-connector wouldn't work if the user flipped the cable. Probably this is some TCPC or redriver controlled by the EC. > > > If that is the EC for > > ChromeOS, then the EC should have a binding that accepts some number of > > input ports for DP. The EC would act as a drm_bridge, or in this case > > probably two bridges, and also as two type-c switches for each > > drm_bridge corresponding to the usb-c-connector nodes. When DP is on the > > cable, the type-c switch/mux would signal to the drm_bridge that the > > display is 'connected' via DRM_BRIDGE_OP_DETECT and struct > > drm_bridge_funcs::detect(). Then the drm_bridge in this anx part would > > implement struct drm_bridge_funcs::atomic_enable() and configure the > > crosspoint switch the right way depending on the reg property of the > > output node in port@1. > > So there will be two drm bridges that act as the downstreams for > anx7625, and we find the downstream with connector_status_connected to > configure the crosspoint switch? How do we support that kind of > topology given that the drm bridge chain is currently a list? Are you > suggesting making the bridge topology to a tree, or maintaining the > two downstreams inside the anx7625 driver and not attaching them to > the bridge chain? Good point. I'm suggesting to make the drm bridge chain into a tree. We need to teach drm_bridge core about a mux, and have some logic to navigate atomically switching from one output to another. I was talking with dianders@ and he was suggesting to use bridge attach/detach for this. I'm not sure that will wo
Re: [PATCH v15 04/10] dt-bindings: display: bridge: anx7625: Add mode-switch support
Quoting Stephen Boyd (2023-04-13 17:22:46) > Quoting Pin-yen Lin (2023-04-13 02:50:44) > > > > Actually the `mode-switch` property here is mainly because > > `fwnode_typec_mux_get`[1] and `typec_mux_match`[2] only return matches > > when the property is present. I am not sure what side effects would be > > if I remove the ID-matching condition in `typec_mux_match`, so I added > > the property here. > > > > Is it feasible to remove the `mode-switch` property here given the > > existing implementation of the Type-C framework? > > Omitting the mode-switch property would require changes to the type-c > framework. > > I'm wondering if we can have this anx driver register mode switches for > however many endpoints exist in the output port all the time when the > aux-bus node doesn't exist. Then the type-c framework can walk from the > usb-c-connector to each connected node looking for a device that is both > a drm_bridge and a mode-switch. When it finds that combination, it knows > that the mode-switch has been found. This hinges on the idea that a > device that would have the mode-switch property is a drm_bridge and > would register a mode-switch with the type-c framework. > > It may be a little complicated though, because we would only register > one drm_bridge for the input to this anx device. The type-c walking code > would need to look at the graph endpoint, and find the parent device to > see if it is a drm_bridge. I've been thinking more about this. I think we should only have the 'mode-switch' property possible when the USB input pins (port@2) are connected and the DPI input pins are connected (port@0). Probably you don't have that case though? In your case, this device should register either one or two drm_bridges that connect to whatever downstream is actually muxing the 2 DP lanes with the USB SS lanes onto the usb-c-connector. If that is the EC for ChromeOS, then the EC should have a binding that accepts some number of input ports for DP. The EC would act as a drm_bridge, or in this case probably two bridges, and also as two type-c switches for each drm_bridge corresponding to the usb-c-connector nodes. When DP is on the cable, the type-c switch/mux would signal to the drm_bridge that the display is 'connected' via DRM_BRIDGE_OP_DETECT and struct drm_bridge_funcs::detect(). Then the drm_bridge in this anx part would implement struct drm_bridge_funcs::atomic_enable() and configure the crosspoint switch the right way depending on the reg property of the output node in port@1. Because you don't have the part that implements the orientation-switch, you don't need to implement the code for it. I think simply adding support in the binding for mode-switch and orientation-switch if this is directly wired to a usb-c-connector should be sufficient. Those properties would be at the top-level and not part of the graph binding.
Re: [PATCH v15 04/10] dt-bindings: display: bridge: anx7625: Add mode-switch support
Quoting Pin-yen Lin (2023-04-13 02:50:44) > Hi Stephen, > > On Wed, Apr 12, 2023 at 10:38 AM Stephen Boyd wrote: > > > > Quoting Pin-yen Lin (2023-03-31 02:11:39) > > > diff --git > > > a/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml > > > b/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml > > > index b42553ac505c..604c7391d74f 100644 > > > --- > > > a/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml > > > +++ > > > b/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml > > > @@ -12,7 +12,8 @@ maintainers: > > > > > > description: | > > >The ANX7625 is an ultra-low power 4K Mobile HD Transmitter > > > - designed for portable devices. > > > + designed for portable devices. Product brief is available at > > > + > > > https://www.analogix.com/en/system/files/AA-002291-PB-6-ANX7625_ProductBrief.pdf > > > > > > properties: > > >compatible: > > > @@ -112,9 +113,40 @@ properties: > > >data-lanes: true > > > > > >port@1: > > > -$ref: /schemas/graph.yaml#/properties/port > > > +$ref: /schemas/graph.yaml#/$defs/port-base > > > description: > > > - Video port for panel or connector. > > > + Video port for panel or connector. Each endpoint connects to a > > > video > > > + output downstream, and the "data-lanes" property is used to > > > describe > > > + the pin connections. 0, 1, 2, 3 in "data-lanes" maps to SSRX1, > > > SSTX1, > > > + SSRX2, SSTX2, respectively. > > > + > > > +patternProperties: > > > + "^endpoint@[01]$": > > > +$ref: /schemas/media/video-interfaces.yaml# > > > +properties: > > > + reg: true > > > + > > > + remote-endpoint: true > > > + > > > + data-lanes: > > > +oneOf: > > > + - items: > > > + - enum: [0, 1, 2, 3] > > > + > > > + - items: > > > + - const: 0 > > > + - const: 1 > > > + > > > + - items: > > > + - const: 2 > > > + - const: 3 > > > + > > > + mode-switch: > > > > Is it possible to not have this property? Can we have the driver for > > this anx device look at the remote-endpoint and if it sees that it is > > not a drm_bridge or panel on the other end, or a DP connector, that it > > should register a typec mode switch (or two depending on the number of > > endpoints in port@1)? Is there any case where that doesn't hold true? > > > > I see these possible scenarios: > > > > 1. DPI to DP bridge steering DP to one of two usb-c-connectors > > > > In this case, endpoint@0 is connected to one usb-c-connector and > > endpoint@1 is connected to another usb-c-connector. The input endpoint > > is only connected to DPI. The USB endpoint is not present (although I > > don't see this described in the binding either, so we would need a > > port@2, entirely optional to describe USB3 input). The driver will > > register two mode switches. > > > > 2. DPI to DP bridge with USB3 to one usb-c-connector > > > > In this case, endpoint@1 doesn't exist. The SSTX1/2 and SSRX1/2 pins are > > all connected to a usb-c-connector node. The input ports (0 and 2) are > > connected to both DPI and USB. The device acts as both a mode-switch and > > an orientation-switch. It registers both switches. I wonder if there is > > any benefit to describing SBU connections or CC connections? Maybe we > > don't register the orientation-switch if the SBU or CC connection isn't > > described? > > > > 3. DPI to DP bridge connected to eDP panel > > > > In this case, endpoint@1 doesn't exist. The USB endpoint is not present > > (port@2). Depending on how the crosspoint should be configured, we'll > > need to use data-lanes in the port@1 endpoint to describe which SSTRX > > pair to use (1 or 2). Or we'll have to use the endpoint's reg property > > to describe which pair to drive DP on. Presumably the default > > configuration is SSRX2/SSTX2 providing 2 lanes of DP to an eDP panel. > > The endpoin
Re: [PATCH v3 00/65] clk: Make determine_rate mandatory for muxes
Quoting Maxime Ripard (2023-04-04 03:10:50)
> Hi,
>
> This is a follow-up to a previous series that was printing a warning
> when a mux has a set_parent implementation but is missing
> determine_rate().
>
> The rationale is that set_parent() is very likely to be useful when
> changing the rate, but it's determine_rate() that takes the parenting
> decision. If we're missing it, then the current parent is always going
> to be used, and thus set_parent() will not be used. The only exception
> being a direct call to clk_set_parent(), but those are fairly rare
> compared to clk_set_rate().
>
> Stephen then asked to promote the warning to an error, and to fix up all
> the muxes that are in that situation first. So here it is :)
>
Thanks for resending.
I was thinking that we apply this patch first and then set
determine_rate clk_ops without setting the clk flag. The function name
is up for debate.
---8<---
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 27c30a533759..057dd3ca8920 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -594,45 +594,57 @@ clk_core_forward_rate_req(struct clk_core *core,
req->max_rate = old_req->max_rate;
}
-int clk_mux_determine_rate_flags(struct clk_hw *hw,
-struct clk_rate_request *req,
-unsigned long flags)
+static int
+clk_core_determine_rate_noreparent(struct clk_core *core,
+ struct clk_rate_request *req)
{
- struct clk_core *core = hw->core, *parent, *best_parent = NULL;
- int i, num_parents, ret;
+ struct clk_core *parent;
+ int ret;
unsigned long best = 0;
- /* if NO_REPARENT flag set, pass through to current parent */
- if (core->flags & CLK_SET_RATE_NO_REPARENT) {
- parent = core->parent;
- if (core->flags & CLK_SET_RATE_PARENT) {
- struct clk_rate_request parent_req;
-
- if (!parent) {
- req->rate = 0;
- return 0;
- }
+ parent = core->parent;
+ if (core->flags & CLK_SET_RATE_PARENT) {
+ struct clk_rate_request parent_req;
- clk_core_forward_rate_req(core, req, parent,
_req, req->rate);
+ if (!parent) {
+ req->rate = 0;
+ return 0;
+ }
- trace_clk_rate_request_start(_req);
+ clk_core_forward_rate_req(core, req, parent, _req,
req->rate);
- ret = clk_core_round_rate_nolock(parent, _req);
- if (ret)
- return ret;
+ trace_clk_rate_request_start(_req);
- trace_clk_rate_request_done(_req);
+ ret = clk_core_round_rate_nolock(parent, _req);
+ if (ret)
+ return ret;
- best = parent_req.rate;
- } else if (parent) {
- best = clk_core_get_rate_nolock(parent);
- } else {
- best = clk_core_get_rate_nolock(core);
- }
+ trace_clk_rate_request_done(_req);
- goto out;
+ best = parent_req.rate;
+ } else if (parent) {
+ best = clk_core_get_rate_nolock(parent);
+ } else {
+ best = clk_core_get_rate_nolock(core);
}
+ req->rate = best;
+
+ return 0;
+}
+
+int clk_mux_determine_rate_flags(struct clk_hw *hw,
+struct clk_rate_request *req,
+unsigned long flags)
+{
+ struct clk_core *core = hw->core, *parent, *best_parent = NULL;
+ int i, num_parents, ret;
+ unsigned long best = 0;
+
+ /* if NO_REPARENT flag set, pass through to current parent */
+ if (core->flags & CLK_SET_RATE_NO_REPARENT)
+ return clk_core_determine_rate_noreparent(core, req);
+
/* find the parent that can provide the fastest rate <= rate */
num_parents = core->num_parents;
for (i = 0; i < num_parents; i++) {
@@ -670,9 +682,7 @@ int clk_mux_determine_rate_flags(struct clk_hw *hw,
if (!best_parent)
return -EINVAL;
-out:
- if (best_parent)
- req->best_parent_hw = best_parent->hw;
+ req->best_parent_hw = best_parent->hw;
req->best_parent_rate = best;
req->rate = best;
@@ -772,6 +782,24 @@ int __clk_mux_determine_rate_closest(struct clk_hw *hw,
}
EXPORT_SYMBOL_GPL(__clk_mux_determine_rate_closest);
+/**
+ * clk_hw_determine_rate_noreparent - clk_ops::determine_rate implementation
for a clk that doesn't reparent
+ * @hw: clk to determine rate on
+ * @req: rate request
+ *
+ * Helper for finding best parent rate to provide a given frequency. This can
+ * be used
Re: [PATCH v15 04/10] dt-bindings: display: bridge: anx7625: Add mode-switch support
Quoting Pin-yen Lin (2023-03-31 02:11:39) > diff --git > a/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml > b/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml > index b42553ac505c..604c7391d74f 100644 > --- a/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml > +++ b/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml > @@ -12,7 +12,8 @@ maintainers: > > description: | >The ANX7625 is an ultra-low power 4K Mobile HD Transmitter > - designed for portable devices. > + designed for portable devices. Product brief is available at > + > https://www.analogix.com/en/system/files/AA-002291-PB-6-ANX7625_ProductBrief.pdf > > properties: >compatible: > @@ -112,9 +113,40 @@ properties: >data-lanes: true > >port@1: > -$ref: /schemas/graph.yaml#/properties/port > +$ref: /schemas/graph.yaml#/$defs/port-base > description: > - Video port for panel or connector. > + Video port for panel or connector. Each endpoint connects to a > video > + output downstream, and the "data-lanes" property is used to > describe > + the pin connections. 0, 1, 2, 3 in "data-lanes" maps to SSRX1, > SSTX1, > + SSRX2, SSTX2, respectively. > + > +patternProperties: > + "^endpoint@[01]$": > +$ref: /schemas/media/video-interfaces.yaml# > +properties: > + reg: true > + > + remote-endpoint: true > + > + data-lanes: > +oneOf: > + - items: > + - enum: [0, 1, 2, 3] > + > + - items: > + - const: 0 > + - const: 1 > + > + - items: > + - const: 2 > + - const: 3 > + > + mode-switch: Is it possible to not have this property? Can we have the driver for this anx device look at the remote-endpoint and if it sees that it is not a drm_bridge or panel on the other end, or a DP connector, that it should register a typec mode switch (or two depending on the number of endpoints in port@1)? Is there any case where that doesn't hold true? I see these possible scenarios: 1. DPI to DP bridge steering DP to one of two usb-c-connectors In this case, endpoint@0 is connected to one usb-c-connector and endpoint@1 is connected to another usb-c-connector. The input endpoint is only connected to DPI. The USB endpoint is not present (although I don't see this described in the binding either, so we would need a port@2, entirely optional to describe USB3 input). The driver will register two mode switches. 2. DPI to DP bridge with USB3 to one usb-c-connector In this case, endpoint@1 doesn't exist. The SSTX1/2 and SSRX1/2 pins are all connected to a usb-c-connector node. The input ports (0 and 2) are connected to both DPI and USB. The device acts as both a mode-switch and an orientation-switch. It registers both switches. I wonder if there is any benefit to describing SBU connections or CC connections? Maybe we don't register the orientation-switch if the SBU or CC connection isn't described? 3. DPI to DP bridge connected to eDP panel In this case, endpoint@1 doesn't exist. The USB endpoint is not present (port@2). Depending on how the crosspoint should be configured, we'll need to use data-lanes in the port@1 endpoint to describe which SSTRX pair to use (1 or 2). Or we'll have to use the endpoint's reg property to describe which pair to drive DP on. Presumably the default configuration is SSRX2/SSTX2 providing 2 lanes of DP to an eDP panel. The endpoint@0 in port@1 will be connected to a drm_panel, and the driver will be able to detect this properly by checking for the existence of an aux-bus node or the return value of of_dp_aux_populate_bus(). 4. DPI to DP bridge connected to DP connector This is similar to the eDP panel scenario #3. In this case, endpoint@1 doesn't exist. The USB endpoint is not present (port@2). Same story about port@1 and lane configuration, but we don't have an aux-bus node. In this case, the drivers/gpu/drm/bridge/display-connector.c driver will probe for the dp-connector node and add a drm_bridge. This anx driver will similarly add a drm_bridge, but it needs to look at the node connected on port@1:endpoint@0 with drm_of_get_bridge() and check if it is a drm_bridge (DP connector) or if it is some type-c thing (connector or orientation-switch). I think having this mode-switch property here lets us avoid calling drm_of_get_bridge() unconditionally in anx7625_parse_dt(). drm_of_get_bridge() will always return -EPROBE_DEFER when this is the last drm_bridge in the chain and the other side of the endpoint is a type-c thing (scenarios #1 and #2). Maybe we should teach drm_of_get_bridge() that a drm_bridge might be connected to a type-c device and have it not return -EPROBE_DEFER in that case. Or make
Re: [PATCH v2 2/2] drm/msm/adreno: change adreno_is_* functions to accept const argument
Quoting Dmitry Baryshkov (2023-04-11 09:19:03) > All adreno_is_*() functions do not modify their argument in any way, so > they can be changed to accept const struct adreno_gpu pointer. > > Suggested-by: Stephen Boyd > Signed-off-by: Dmitry Baryshkov > --- Reviewed-by: Stephen Boyd
Re: [PATCH v2 1/2] drm/msm/adreno: warn if chip revn is verified before being set
Quoting Dmitry Baryshkov (2023-04-11 09:19:02)
> The commit 010c8bbad2cb ("drm: msm: adreno: Disable preemption on Adreno
> 510") tried to check GPU's revn before revn being set. Add WARN_ON_ONCE
> to prevent such bugs from happening again. A separate helper is
> necessary so that the warning is displayed really just once instead of
> being displayed for each of comparisons.
>
> Suggested-by: Stephen Boyd
> Signed-off-by: Dmitry Baryshkov
> ---
Reviewed-by: Stephen Boyd
Re: [PATCH] drm/msm/adreno: warn if chip revn is verified before being set
Quoting Dmitry Baryshkov (2023-04-10 15:38:36)
> The commit 010c8bbad2cb ("drm: msm: adreno: Disable preemption on Adreno
> 510") tried to check GPU's revn before revn being set. Add WARN_ON_ONCE
> to prevent such bugs from happening again. A separate helper is
> necessary so that the warning is displayed really just once instead of
> being displayed for each of comparisons.
>
> Suggested-by: Stephen Boyd
> Signed-off-by: Dmitry Baryshkov
> ---
> drivers/gpu/drm/msm/adreno/adreno_gpu.h | 63 -
> 1 file changed, 40 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> index f62612a5c70f..47e21d44ac24 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> @@ -145,40 +145,51 @@ struct adreno_platform_config {
>
> bool adreno_cmp_rev(struct adreno_rev rev1, struct adreno_rev rev2);
>
> +static inline bool adreno_cmp_revn(struct adreno_gpu *gpu, uint32_t revn)
'cmp' in the name makes me think it's comparing. Maybe 'is' is better
because we're testing for equality.
adreno_is_revn()
Also 'const struct adreno_gpu *' because it isn't changing?
> +{
> + WARN_ON_ONCE(!gpu->revn);
> +
> + return gpu->revn == revn;
> +}
> +
> static inline bool adreno_is_a2xx(struct adreno_gpu *gpu)
And then these can all be const in a followup patch probably.
Re: [RFC PATCH] drm/msm/a5xx: really check for A510 in a5xx_gpu_init
Quoting Dmitry Baryshkov (2023-04-08 18:13:29)
> The commit 010c8bbad2cb ("drm: msm: adreno: Disable preemption on Adreno
> 510") added special handling for a510 (this SKU doesn't seem to support
> preemption, so the driver should clamp nr_rings to 1). However the
> gpu->revn is not yet set (it is set later, in adreno_gpu_init()) and
> thus the condition is always false. Check config->rev instead.
>
> Fixes: 010c8bbad2cb ("drm: msm: adreno: Disable preemption on Adreno 510")
> Reported-by: Adam Skladowski
> Signed-off-by: Dmitry Baryshkov
> ---
Maybe as a followup you can put a WARN_ON_ONCE() inside a new function
that gets gpu->revn and warns if the value is 0?
Re: [PATCH] drm/msm/adreno: fix sparse warnings in a6xx code
Quoting Dmitry Baryshkov (2023-04-06 18:07:41)
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
> b/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
> index 0bc3eb443fec..84d345af126f 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
> @@ -51,8 +51,8 @@ struct a6xx_gmu {
>
> struct msm_gem_address_space *aspace;
>
> - void * __iomem mmio;
> - void * __iomem rscc;
> + void __iomem * mmio;
> + void __iomem * rscc;
Should stick that * to the member name.
void __iomem *rscc;
with that
Reviewed-by: Stephen Boyd
Re: [PATCH v15 01/10] device property: Add remote endpoint to devcon matcher
Quoting Pin-yen Lin (2023-03-31 02:11:36) > From: Prashant Malani > > When searching the device graph for device matches, check the > remote-endpoint itself for a match. > > Some drivers register devices for individual endpoints. This allows > the matcher code to evaluate those for a match too, instead > of only looking at the remote parent devices. This is required when a > device supports two mode switches in its endpoints, so we can't simply > register the mode switch with the parent node. Looking at this in isolation I have no idea what a mode switch is and how it is related to drivers/base/property.c. Can you expand on this commit text? Maybe say two "usb typec mode switches"? And maybe include an example graph node snippet?
Re: [PATCH v2 43/65] ASoC: tlv320aic32x4: Add a determine_rate hook
Quoting Maxime Ripard (2023-03-29 12:50:49) > On Wed, Mar 22, 2023 at 04:31:04PM -0700, Stephen Boyd wrote: > > > The clk_set_parent() path is valid for those cases. Probably nobody > > cares about determine_rate because they don't set rates on these clks. > > Some drivers even explicitly left out determine_rate()/round_rate() > > because they didn't want to have some other clk round up to the mux > > and change the parent. > > > > Eventually we want drivers to migrate to determine_rate op so we can get > > rid of the round_rate op and save a pointer (we're so greedy). It's been > > 10 years though, and that hasn't been done. Sigh! I can see value in > > this series from the angle of migrating, but adding a determine_rate op > > when there isn't a round_rate op makes it hard to reason about. What if > > something copies the clk_ops or sets a different flag? Now we've just > > added parent changing support to clk_set_rate(). What if the clk has > > CLK_SET_RATE_PARENT flag set? Now we're going to ask the parent clk to > > change rate. Fun bugs. > > > > TL;DR: If the set_parent op exists but determine_rate/round_rate doesn't > > then the clk is a mux that doesn't want to support clk_set_rate(). Make > > a new mux function that's the contents of the CLK_SET_RATE_NO_REPARENT > > branch in clk_mux_determine_rate_flags() and call that directly from the > > clk_ops so it is clear what's happening, > > clk_hw_mux_same_parent_determine_rate() or something with a better name. > > Otherwise migrate the explicit determine_rate op to this new function > > and don't set the flag. > > > > It may be possible to entirely remove the CLK_SET_RATE_NO_REPARENT flag > > with this design, if the determine_rate clk_op can call the inner > > wrapper function instead of __clk_mux_determine_rate*() (those > > underscores are awful, we should just prefix them with clk_hw_mux_*() > > and live happier). That should be another patch series. > > Sorry but it's not really clear to me what you expect in the v2 of this > series (if you even expect one). It looks that you don't like the > assignment-if-missing idea Mark suggested, but should I just > rebase/resend or did you expect something else? > Yes, we want explicit code. Just rebase & resend. Don't add a determine_rate if there isn't a round_rate. Don't add more users of CLK_SET_RATE_NO_REPARENT. Instead, make an explicit determine_rate function for that. If you want to work on the removal of CLK_SET_RATE_NO_REPARENT go for it. Otherwise I'll take care of it after this series.
Re: [PATCH v14 14/14] drm/msm/dp: set self refresh aware based on PSR support
Quoting Bjorn Andersson (2023-03-26 09:35:56) > On Sun, Mar 26, 2023 at 09:27:23AM -0700, Bjorn Andersson wrote: > > On Thu, Mar 02, 2023 at 10:03:17PM +0530, Vinod Polimera wrote: > > > For the PSR to kick in, self_refresh_aware has to be set. > > > Initialize it based on the PSR support for the eDP interface. > > > > > > > When I boot my sc8280xp devices (CRD and X13s) to console with this > > patch included I get a login prompt, and then there are no more screen > > updates. > > > > Switching virtual terminal (ctrl+alt+fN) causes the screen to redraw. > > > > Blindly login in and launching Wayland works and from then on screen > > updates works as expected. > > > > Switching from Wayland to another virtual terminal causes the problem to > > re-appear, no updates after the initial refresh, switching back go the > > Wayland-terminal crashed the machine. > > > > Also, trying to bring the eDP-screen back from DPMS gives me: > > <3>[ 2355.218099] [drm:dp_catalog_ctrl_set_pattern_state_bit [msm]] *ERROR* > set state_bit for link_train=1 failed > <3>[ 2355.218926] [drm:dp_ctrl_setup_main_link [msm]] *ERROR* link training > #1 failed. ret=-110 > <3>[ 2355.262859] [drm:dp_catalog_ctrl_set_pattern_state_bit [msm]] *ERROR* > set state_bit for link_train=1 failed > <3>[ 2355.263600] [drm:dp_ctrl_setup_main_link [msm]] *ERROR* link training > #1 failed. ret=-110 > <3>[ 2355.305211] [drm:dp_catalog_ctrl_set_pattern_state_bit [msm]] *ERROR* > set state_bit for link_train=1 failed > <3>[ 2355.305955] [drm:dp_ctrl_setup_main_link [msm]] *ERROR* link training > #1 failed. ret=-110 > <3>[ 2355.345250] [drm:dp_catalog_ctrl_set_pattern_state_bit [msm]] *ERROR* > set state_bit for link_train=1 failed > <3>[ 2355.346026] [drm:dp_ctrl_setup_main_link [msm]] *ERROR* link training > #1 failed. ret=-110 > <3>[ 2355.405650] [drm:dp_display_process_hpd_high [msm]] *ERROR* failed to > complete DP link training > <3>[ 2355.668988] [drm:dpu_encoder_phys_vid_wait_for_commit_done:488] [dpu > error]vblank timeout > <3>[ 2355.669030] [drm:dpu_kms_wait_for_commit_done:510] [dpu error]wait for > commit done returned -110 > <3>[ 2355.699989] [drm:dpu_encoder_frame_done_timeout:2398] [dpu error]enc35 > frame done timeout > > And then the machine just resets. > I saw similar behavior on ChromeOS after we picked the PSR patches into our kernel. I suspect it's the same problem. I switched back and forth between VT2 and the OOBE screen with ctrl+alt+forward and that showed what I typed on the virtual terminal after switching back and forth. It's like the redraw only happens once on the switch and never again. I switched back and forth enough times that it eventually crashed the kernel and rebooted. This was on CRD (sc7280-herobrine-crd.dts). There's an animation on the OOBE screen that is working though, so perhaps PSR is working with the chrome compositor but not the virtual terminal? I haven't investigated.
Re: [PATCH v2 56/65] clk: ingenic: cgu: Switch to determine_rate
Quoting Maxime Ripard (2022-11-09 03:00:45) > On Mon, Nov 07, 2022 at 08:57:22PM +, Aidan MacDonald wrote: > > > > Maxime Ripard writes: > > > > > Hi, > > > > > > On Fri, Nov 04, 2022 at 05:35:29PM +, Aidan MacDonald wrote: > > > > Assigning the parent clock in the DT works once, at boot, but going off > > what you wrote in the commit message, if the clock driver has a > > .determine_rate() implementation that *can* reparent clocks then it > > probably *will* reparent them, and the DT assignment will be lost. > > Yes, indeed, but assigned-clock-parents never provided any sort of > guarantee on whether or not the clock was allowed to reparent or not. > It's just a one-off thing, right before probe, and a clk_set_parent() > call at probe will override that just fine. > > Just like assigned-clock-rates isn't permanent. > > > What I'm suggesting is a runtime constraint that the clock subsystem > > would enforce, and actively prevent drivers from changing the parent. > > Either explicitly with clk_set_parent() or due to .determine_rate(). > > > > That way you could write a .determine_rate() implementation that *can* > > select a better parent, but if the DT applies a constraint to fix the > > clock to a particular parent, the clock subsystem will force that parent > > to be used so you can be sure the clock is never reparented by accident. > > Yeah, that sounds like a good idea, and CLK_SET_RATE_NO_REPARENT isn't > too far off from this, it's just ignored by clk_set_parent() for now. I > guess we could rename CLK_SET_RATE_NO_REPARENT to CLK_NO_REPARENT, make > clk_set_parent handle it, and set that flag whenever > assigned-clock-parents is set on a clock. > > It's out of scope for this series though, and I certainly don't want to > deal with all the regressions it might create :) > This sounds like a new dt binding that says the assigned parent should never change. It sounds sort of like gpio hogs. A clock-hogs binding?
Re: [PATCH v2 43/65] ASoC: tlv320aic32x4: Add a determine_rate hook
Quoting Maxime Ripard (2022-11-07 07:26:03)
> On Mon, Nov 07, 2022 at 12:06:07PM +, Mark Brown wrote:
> > On Mon, Nov 07, 2022 at 09:43:22AM +0100, Maxime Ripard wrote:
> > > On Fri, Nov 04, 2022 at 03:59:53PM +, Mark Brown wrote:
> >
> > > > Well, hopefully everyone for whom it's an issue currently will be
> > > > objecting to this version of the change anyway so we'll either know
> > > > where to set the flag or we'll get the whack-a-mole with the series
> > > > being merged?
> >
> > > I'm sorry, I'm not sure what you mean here. The only issue to fix at the
> > > moment is that determine_rate and set_parent aren't coupled, and it led
> > > to issues due to oversight.
> >
> > > I initially added a warning but Stephen wanted to fix all users in that
> > > case and make that an error instead.
> >
> > My suggestion is that instead of doing either of these things it'd be
> > quicker and less error prone to just fix the core to provide the default
> > implementation if nothing more specific is provided. Any issues that
> > causes would already be present with your current series.
> >
> > > If I filled __clk_mux_determine_rate into clocks that weren't using it
> > > before, I would change their behavior. With that flag set, on all users
> > > I add __clk_mux_determine_rate to, the behavior is the same than what we
> > > previously had, so the risk of regressions is minimal, and everything
> > > should keep going like it was?
> >
> > The series does fill in __clk_mux_determine_rate for everything though -
> > if it was just assumed by default the only thing that'd be needed would
> > be adding the flag.
>
> The behavior assumed by default was equivalent to
> __clk_mux_determine_rate + CLK_SET_RATE_NO_REPARENT. We could indeed set
> both if determine_rate is missing in the core, but that's unprecedented
> in the clock framework so I think we'll want Stephen to comment here :)
The clk_ops pointer is const (no writeable jump tables) so we'd have to
copy the clk_ops struct on registration to set the
__clk_mux_determine_rate() op. We could set the flag though and then
check for the absence of a determine_rate op. Things like
clk_core_can_round() would need to check for the flag. I'd actually
forgotten about this flag. In hindsight I think we should delete it.
I'd expect it to be used when walking the clk tree during rate rounding,
but it's only used in the determine rate clk op.
>
> It's also replacing one implicit behavior by another. The point of this
> series was to raise awareness on that particular point, so I'm not sure
> it actually fixes things. We'll see what Stephen thinks about it.
>
Right. A decade ago (!) when determine_rate() was introduced we
introduced CLK_SET_RATE_NO_REPARENT and set it on each mux user (see
commit 819c1de344c5 ("clk: add CLK_SET_RATE_NO_REPARENT flag")). This
way driver behavior wouldn't change and the status quo would be
maintained, i.e. that clk_set_rate() on a mux wouldn't change parents.
We didn't enforce that determine_rate exists if the set_parent() op
existed at the same time though. Probably an oversight.
Most of the replies to this series have been "DT is setting the parent",
which makes me believe that there are 'assigned-clock-parents' being
used. The clk_set_parent() path is valid for those cases. Probably
nobody cares about determine_rate because they don't set rates on these
clks. Some drivers even explicitly left out
determine_rate()/round_rate() because they didn't want to have some
other clk round up to the mux and change the parent.
Eventually we want drivers to migrate to determine_rate op so we can get
rid of the round_rate op and save a pointer (we're so greedy). It's been
10 years though, and that hasn't been done. Sigh! I can see value in
this series from the angle of migrating, but adding a determine_rate op
when there isn't a round_rate op makes it hard to reason about. What if
something copies the clk_ops or sets a different flag? Now we've just
added parent changing support to clk_set_rate(). What if the clk has
CLK_SET_RATE_PARENT flag set? Now we're going to ask the parent clk to
change rate. Fun bugs.
TL;DR: If the set_parent op exists but determine_rate/round_rate doesn't
then the clk is a mux that doesn't want to support clk_set_rate(). Make
a new mux function that's the contents of the CLK_SET_RATE_NO_REPARENT
branch in clk_mux_determine_rate_flags() and call that directly from the
clk_ops so it is clear what's happening,
clk_hw_mux_same_parent_determine_rate() or something with a better name.
Otherwise migrate the explicit determine_rate op to this new function
and don't set the flag.
It may be possible to entirely remove the CLK_SET_RATE_NO_REPARENT flag
with this design, if the determine_rate clk_op can call the inner
wrapper function instead of __clk_mux_determine_rate*() (those
underscores are awful, we should just prefix them with clk_hw_mux_*()
and live happier). That should be another patch series.
Re: [PATCH v2 00/65] clk: Make determine_rate mandatory for muxes
Quoting Maxime Ripard (2023-03-22 03:01:53) > Hi Stephen, > > On Tue, Mar 21, 2023 at 04:55:03PM -0700, Stephen Boyd wrote: > > Quoting Maxime Ripard (2022-11-04 06:17:17) > > > Hi, > > > > > > This is a follow-up to a previous series that was printing a warning > > > when a mux has a set_parent implementation but is missing > > > determine_rate(). > > > > > > The rationale is that set_parent() is very likely to be useful when > > > changing the rate, but it's determine_rate() that takes the parenting > > > decision. If we're missing it, then the current parent is always going > > > to be used, and thus set_parent() will not be used. The only exception > > > being a direct call to clk_set_parent(), but those are fairly rare > > > compared to clk_set_rate(). > > > > > > Stephen then asked to promote the warning to an error, and to fix up all > > > the muxes that are in that situation first. So here it is :) > > > > > > Let me know what you think, > > > > What's the plan here? Are you going to resend? > > It wasn't clear to me whether or not this was something that you wanted, > and I got some pushback on the drivers so I kind of forgot about it. > > If you do want it (and it looks like you do), I'll resend it. Let me read the whole series first. I was ignoring it because I was assuming it was going to be resent.
Re: [PATCH v2 00/65] clk: Make determine_rate mandatory for muxes
Quoting Maxime Ripard (2022-11-04 06:17:17) > Hi, > > This is a follow-up to a previous series that was printing a warning > when a mux has a set_parent implementation but is missing > determine_rate(). > > The rationale is that set_parent() is very likely to be useful when > changing the rate, but it's determine_rate() that takes the parenting > decision. If we're missing it, then the current parent is always going > to be used, and thus set_parent() will not be used. The only exception > being a direct call to clk_set_parent(), but those are fairly rare > compared to clk_set_rate(). > > Stephen then asked to promote the warning to an error, and to fix up all > the muxes that are in that situation first. So here it is :) > > Let me know what you think, What's the plan here? Are you going to resend?
Re: [PATCH v4 2/8] kunit: drm/tests: move generic helpers
Quoting Matti Vaittinen (2023-03-20 22:45:52) > Morning Stephen, > > On 3/20/23 21:23, Stephen Boyd wrote: > > Quoting Matti Vaittinen (2023-03-18 23:36:20) > >>> > >>> I think you would have an easier time if you just copied and renamed > >>> them into the kunit folder as an preparation series. > >> > >> Yes. That would simplify the syncing between the trees. It slightly bugs > >> me to add dublicate code in kernel-but the clean-up series for DRM users > >> could be prepared at the same time. It would be even possible to just > >> change the drm-helper to be a wrapper for the generic one - and leave > >> the callers intact - although it leaves some seemingly unnecessary > >> "onion code" there. > >> > >>> That way, you wouldn't have to coordinate DRM, CCF and IIO, you'd just > >>> create new helpers that can be reused/converted to by everyone eventually > >> > >> Yes. Thanks - I think I may go with this approach for the v5 :) > > > > Which kunit directory? > > I was thinking of adding the platform_device.h (I liked your suggestion) > in the include/kunit/ Ok, thanks for clarifying. > > > I imagine if there are conflicts they will be > > trivial so it probably doesn't matter. > > Probably so. Still, I am not the one who needs to deal with the > conflicts. Hence I like at least asking if people see good way to avoid > them in the first place. Same for me. I'm not the maintainer of the drivers/base directory. > > Besides, I was not sure if you were planning to add similar helper or > just wrappers to individual functions. Wanted to ping you just in case > this has some impact to what you do. I don't have a need to bind a device to a driver to satisfy devm APIs currently. I could probably use it though to test some devm code in the clk APIs. The only impact is that we're modifying the same files. > > > Have you Cced kunit folks and the > > list on the kunit patches? They may have some opinion. > > This patch was should have contained the > include/kunit/platform_device.h. That file was pulling the Kunit people > in recipients but I messed up things with last minute changes so both > the header and people were dropped. I'll fix this for v5. > Ok, I'll be on the lookout for v5. From what I can tell kunit goes through the kernel selftest tree and there isn't a kunit git tree as such.
Re: [PATCH v4 2/8] kunit: drm/tests: move generic helpers
Quoting Matti Vaittinen (2023-03-18 23:36:20) > > > > I think you would have an easier time if you just copied and renamed > > them into the kunit folder as an preparation series. > > Yes. That would simplify the syncing between the trees. It slightly bugs > me to add dublicate code in kernel-but the clean-up series for DRM users > could be prepared at the same time. It would be even possible to just > change the drm-helper to be a wrapper for the generic one - and leave > the callers intact - although it leaves some seemingly unnecessary > "onion code" there. > > > That way, you wouldn't have to coordinate DRM, CCF and IIO, you'd just > > create new helpers that can be reused/converted to by everyone eventually > > Yes. Thanks - I think I may go with this approach for the v5 :) Which kunit directory? I imagine if there are conflicts they will be trivial so it probably doesn't matter. Have you Cced kunit folks and the list on the kunit patches? They may have some opinion.
Re: [PATCH] dt-bindings: yamllint: Require a space after a comment '#'
Quoting Rob Herring (2023-03-03 13:42:23)
> Enable yamllint to check the prefered commenting style of requiring a
> space after a comment character '#'. Fix the cases in the tree which
> have a warning with this enabled. Most cases just need a space after the
> '#'. A couple of cases with comments which were not intended to be
> comments are revealed. Those were in ti,sa2ul.yaml, ti,cal.yaml, and
> brcm,bcmgenet.yaml.
>
> Signed-off-by: Rob Herring
Reviewed-by: Stephen Boyd
> diff --git a/Documentation/devicetree/bindings/clock/qcom,a53pll.yaml
> b/Documentation/devicetree/bindings/clock/qcom,a53pll.yaml
> index 525ebaa93c85..64bfd0f5d4d0 100644
> --- a/Documentation/devicetree/bindings/clock/qcom,a53pll.yaml
> +++ b/Documentation/devicetree/bindings/clock/qcom,a53pll.yaml
> @@ -45,14 +45,14 @@ required:
> additionalProperties: false
>
> examples:
> - #Example 1 - A53 PLL found on MSM8916 devices
> + # Example 1 - A53 PLL found on MSM8916 devices
>- |
> a53pll: clock@b016000 {
> compatible = "qcom,msm8916-a53pll";
> reg = <0xb016000 0x40>;
> #clock-cells = <0>;
> };
> - #Example 2 - A53 PLL found on IPQ6018 devices
> + # Example 2 - A53 PLL found on IPQ6018 devices
>- |
> a53pll_ipq: clock-controller@b116000 {
> compatible = "qcom,ipq6018-a53pll";
Re: [PATCH] dt-bindings: Fix SPI and I2C bus node names in examples
Quoting Rob Herring (2023-02-28 13:54:33)
> SPI and I2C bus node names are expected to be "spi" or "i2c",
> respectively, with nothing else, a unit-address, or a '-N' index. A
> pattern of 'spi0' or 'i2c0' or similar has crept in. Fix all these
> cases. Mostly scripted with the following commands:
>
> git grep -l '\si2c[0-9] {' Documentation/devicetree/ | xargs sed -i -e
> 's/i2c[0-9] {/i2c {/'
> git grep -l '\sspi[0-9] {' Documentation/devicetree/ | xargs sed -i -e
> 's/spi[0-9] {/spi {/'
>
> With this, a few errors in examples were exposed and fixed.
>
> Signed-off-by: Rob Herring
> ---
Reviewed-by: Stephen Boyd
> diff --git a/Documentation/devicetree/bindings/clock/ti,lmk04832.yaml
> b/Documentation/devicetree/bindings/clock/ti,lmk04832.yaml
> index 73d17830f165..13d7b3d03d84 100644
> --- a/Documentation/devicetree/bindings/clock/ti,lmk04832.yaml
> +++ b/Documentation/devicetree/bindings/clock/ti,lmk04832.yaml
> @@ -160,7 +160,7 @@ examples:
> };
> };
>
> -spi0 {
> +spi {
> #address-cells = <1>;
> #size-cells = <0>;
>
Re: [PATCH] drm/msm/dp: check core_initialized flag at both host_init() and host_deinit()
Quoting Kuogee Hsieh (2023-02-24 10:29:58)
> There is a reboot/suspend test case where system suspend is forced during
> system booting up. Since host_init() of external DP is executed at hpd
dp_display_host_init()?
> thread context, this test case may created a scenario that host_deinit()
dp_display_host_deinit()?
> from pm_suspend() run before host_init() if hpd thread has no chance to
> run during booting up while suspend request command was issued.
> At this scenario system will crash at aux register access at host_deinit()
> since aux clock had not yet been enabled by host_init(). Therefore we
The aux clk is enabled in dp_power_clk_enable() right? Can you clarify?
> have to ensure aux clock enabled by checking core_initialized flag before
> access aux registers at pm_suspend.
I'd much more like to get rid of 'core_initialized'. What is preventing
us from enabling the power (i.e. dp_power_init()), or at least enough
clks and pm runtime state, during probe? That would fix this problem and
also clean things up. As I understand, the device is half initialized in
probe and half initialized in the kthread. If we put all power
management into the runtime PM ops and synced that state during probe so
that runtime PM state matched device probe state we could make runtime
PM be the only suspend function and then push the power state tracking
into the device core.
>
> Fixes: 989ebe7bc446 ("drm/msm/dp: do not initialize phy until plugin
> interrupt received")
> Signed-off-by: Kuogee Hsieh
The code looks OK to me, so
Reviewed-by: Stephen Boyd
once the commit text is cleaned up to indicate the proper function
names.
Re: [PATCH v1] drm/msm/disp/dpu: fix sc7280_pp base address
Quoting Kuogee Hsieh (2023-02-27 09:04:31)
> Correct sc7280 pp block base address.
What goes wrong if this is left unchanged? How important is it to fix
this? Does the display fail to work? Does it fix something for a new
feature that isn't yet enabled upstream?
This information is useful to put in the commit text so we know the
severity of the problem that is being fixed and so that maintainers can
understand the importance of the patch.
>
> Fixes: 591e34a091d1 ("drm/msm/disp/dpu1: add support for display for SC7280
> target")
> Signed-off-by: Kuogee Hsieh
Re: [PATCH v6 2/2] drm/msm/dp: enhance dp controller isr
Quoting Doug Anderson (2023-01-18 10:29:59)
> Hi,
>
> On Tue, Dec 27, 2022 at 6:16 PM Kuogee Hsieh wrote:
> > +
> > if (isr & DP_INTR_AUX_ERROR) {
> > aux->aux_error_num = DP_AUX_ERR_PHY;
> > dp_catalog_aux_clear_hw_interrupts(aux->catalog);
> > + ret = IRQ_HANDLED;
> > }
>
> The end result of the above is a weird mix of "if" and "else if" for
> no apparent reason. All except one of them just updates the exact same
> variable so doing more than one is mostly useless. If you made it
> consistently with "else" then the whole thing could be much easier,
> like this (untested):
Totally agreed. I even asked that when I posted the RFC[1]!
"Can we also simplify the aux handlers to be a big pile of
if-else-if conditions that don't overwrite the 'aux_error_num'? That
would simplify the patch below."
> > @@ -425,17 +464,15 @@ void dp_aux_isr(struct drm_dp_aux *dp_aux)
> >
> > /* no interrupts pending, return immediately */
> > if (!isr)
> > - return;
> > + return IRQ_NONE;
> >
> > if (!aux->cmd_busy)
> > - return;
> > + return IRQ_NONE;
> >
> > if (aux->native)
> > - dp_aux_native_handler(aux, isr);
> > + return dp_aux_native_handler(aux, isr);
> > else
> > - dp_aux_i2c_handler(aux, isr);
> > -
> > - complete(>comp);
> > + return dp_aux_i2c_handler(aux, isr);
>
> Personally, I wouldn't have done it this way. I guess that means I
> disagree with Stephen. I'm not dead-set against this way and it's fine
> if you want to continue with it. If I were doing it, though, then I
> would always return IRQ_HANDLED IF dp_catalog_aux_get_irq() returned
> anything non-zero. Why? Officially if dp_catalog_aux_get_irq() returns
> something non-zero then you know for sure that there was an interrupt
> for this device and officially you have "handled" it by acking it,
> since dp_catalog_aux_get_irq() acks all the bits that it returns. That
> means that even if dp_aux_native_handler() or dp_aux_i2c_handler()
> didn't do anything with the interrupt you at least know that it was
> for us (so if the IRQ is shared we properly report back to the IRQ
> subsystem) and that it won't keep firing over and over (because we
> acked it).
I'm primarily concerned with irq storms taking down the system. Can that
happen here? If not, then returning IRQ_NONE is not really useful. The
overall IRQ for DP looks to be level, because the driver requests the
IRQ that way. The aux interrupt status bits look to be edge style
interrupts though, because the driver acks them in the handler. I guess
that means the edges come in and latch into the interrupt status
register so the driver has to ack all of them to drop the IRQ level for
the overall DP interrupt? If the driver only acked the bits it looked at
instead of all interrupt bits in the register, then the level would
never go down for the IRQ if an unhandled interrupt bit was present like
'DP_INTR_PLL_UNLOCKED'. That would mean we would hit spurious IRQ
handling very quickly if that interrupt bit was ever seen.
But the driver is acking all interrupts, so probably trying to work
IRQ_NONE into this code is not very useful? The only thing it would
catch is DP_INTR_PLL_UNLOCKED being set over and over again, which seems
unlikely. Of course, why is this driver unmasking interrupt bits it
doesn't care about? That may be leading to useless interrupt handling in
this driver if some interrupt bit is unmasked but never looked at. Can
that be fixed in another patch?
>
> NOTE: I still like having the complete() call in
> dp_aux_native_handler() and dp_aux_i2c_handler() and, to me, that part
> of this patch is worthwhile. That makes it more obvious that the code
> is truly expecting that complete to be called for all error cases as
> well as transfer finished.
>
I think it may be required. We don't want to allow DP_INTR_PLL_UNLOCKED
to complete() the transfer.
[1]
https://lore.kernel.org/all/CAE-0n5100eGC0c09oq4B3M=ahtkw5+wglgss1jm91scyz5w...@mail.gmail.com/
Re: [PATCH] drm/panel: boe-tv101wum-nl6: Ensure DSI writes succeed during disable
Quoting Stephen Boyd (2023-01-13 12:49:43) > Quoting Sam Ravnborg (2023-01-13 06:52:09) > > Hi Stephen, > > On Tue, Jan 10, 2023 at 11:29:41AM -0800, Stephen Boyd wrote: > > > > > > After this patch the unprepare only bails out early if the bool > > > 'prepared' flag isn't set. > > OK, then everything is fine. > > > > Doug pointed out that enable isn't symmetric because it doesn't do the > DSI writes. I've updated the patch and I'll send a v2. Turns out that splitting prepare into prepare and enable breaks the display even more for me. For now this is the best patch I have and it fixes the regression I see in v6.1 kernels. Can I get your Reviewed-by on this patch?
Re: [PATCH] drm/panel: boe-tv101wum-nl6: Ensure DSI writes succeed during disable
Quoting Dave Stevenson (2023-01-16 06:11:02) > Hi Stephen > > On Fri, 13 Jan 2023 at 21:12, Stephen Boyd wrote: > > > > > > Thanks for the info! It says "Drivers that need the underlying device to > > be powered to perform these operations will first need to make sure it’s > > been properly enabled." Does that mean the panel driver needs to make > > sure the underlying dsi host device is powered? The sentence is too > > ambiguous for me to understand what 'drivers' and 'underlying device' > > are. > > The DSI host driver (ie in your case something under > drivers/gpu/drm/msm/dsi) should ensure that a transfer can be made, > regardless of state. > > I must say that this has been documented as the case, but doesn't > necessarily reflect reality in a number of drivers. Alright, thanks for the clarification. > > > > Cool. Glad that we can clean that up with your series. > > > > Is it wrong to split unprepare to a disable and unprepare phase? I'm not > > super keen on fixing 6.1 stable kernels by opting this driver into the > > ordering change. Splitting the phase into two is small and simple and > > works. I suspect changing the ordering may uncover more bugs, or be a > > larger task. I'd be glad to test that series[2] from you to get rid of > > [3]. > > Ah, I hadn't realised it was a regression in a released kernel :( > > Splitting into a disable and unprepare is totally fine. Normally > disable would normally disable the panel and backlight (probably by > drm_panel before the panel disable call), with unprepare disabling > power and clocks > > Do note that AIUI you will be telling the panel to enter sleep mode > whilst video is still being transmitted. That should be safe as the > panel has to still be partially active to handle an exit sleep mode > command, but actually powering hardware down at that point could cause > DSI bus arbitration errors as clock or data lanes could be pulled down > when the host is trying to adopt HS or LP11. > Ok. I don't think I'm running into that issue, but I have run into a different issue. I tried to split the prepare phase into enable and prepare with the DSI writes in the enable to make things symmetric but that totally failed. Now I get lots of timeouts when enabling the panel. This patch is still the best fix I have. Maybe with your series we can combine the unprepare and disable ops together again (i.e. revert this) so that power is removed immediately after sending the DSI commands? Or is that not enough to avoid the DSI bus arbitration problems you're talking about? When is the host adopting HS or LP11 with regards to the bridge ops?
Re: [PATCH] drm/panel: boe-tv101wum-nl6: Ensure DSI writes succeed during disable
Quoting Dave Stevenson (2023-01-13 08:27:29)
> Hi Stephen
>
> On Fri, 6 Jan 2023 at 03:01, Stephen Boyd wrote:
> >
> > The unprepare sequence has started to fail after moving to panel bridge
> > code in the msm drm driver (commit 007ac0262b0d ("drm/msm/dsi: switch to
> > DRM_PANEL_BRIDGE")). You'll see messages like this in the kernel logs:
> >
> >panel-boe-tv101wum-nl6 ae94000.dsi.0: failed to set panel off: -22
> >
> > This is because boe_panel_enter_sleep_mode() needs an operating DSI link
> > to set the panel into sleep mode. Performing those writes in the
> > unprepare phase of bridge ops is too late, because the link has already
> > been torn down by the DSI controller in post_disable, i.e. the PHY has
> > been disabled, etc. See dsi_mgr_bridge_post_disable() for more details
> > on the DSI .
> >
> > Split the unprepare function into a disable part and an unprepare part.
> > For now, just the DSI writes to enter sleep mode are put in the disable
> > function. This fixes the panel off routine and keeps the panel happy.
>
> It is documented that the mipi_dsi_host_ops transfer function should
> be called with the host in any state [1], so the host driver is
> failing there.
Thanks for the info! It says "Drivers that need the underlying device to
be powered to perform these operations will first need to make sure it’s
been properly enabled." Does that mean the panel driver needs to make
sure the underlying dsi host device is powered? The sentence is too
ambiguous for me to understand what 'drivers' and 'underlying device'
are.
>
> This sounds like the same issue I was addressing in adding the
> prepare_prev_first flag to drm_panel, and pre_enable_prev_first to
> drm_bridge via [2].
> Defining prepare_prev_first for your panel would result in the host
> pre_enable being called before the panel prepare, and therefore the
> transfer calls from boe_panel_init_dcs_cmd boe_panel_prepare won't be
> relying on the DSI host powering up early. It will also call the panel
> unprepare before the DSI host bridge post_disable is called, and
> therefore the DSI host will still be powered up and the transfer won't
> fail.
>
> Actually I've just noted the comment at [3]. [2] is that framework fix
> that means that the magic workaround isn't required, but it will
> require this panel to opt in to the ordering change.
Cool. Glad that we can clean that up with your series.
Is it wrong to split unprepare to a disable and unprepare phase? I'm not
super keen on fixing 6.1 stable kernels by opting this driver into the
ordering change. Splitting the phase into two is small and simple and
works. I suspect changing the ordering may uncover more bugs, or be a
larger task. I'd be glad to test that series[2] from you to get rid of
[3].
>
>
> [1]
> https://www.kernel.org/doc/html/latest/gpu/drm-kms-helpers.html#c.mipi_dsi_host_ops
> [2] https://patchwork.freedesktop.org/series/100252/
> [3]
> https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/msm/dsi/dsi_manager.c#L47
>
Re: [PATCH] drm/panel: boe-tv101wum-nl6: Ensure DSI writes succeed during disable
Quoting Sam Ravnborg (2023-01-13 06:52:09) > Hi Stephen, > On Tue, Jan 10, 2023 at 11:29:41AM -0800, Stephen Boyd wrote: > > Quoting Sam Ravnborg (2023-01-07 12:28:41) > > > > > > > > For this case we could ask ourself if the display needs to enter sleep > > > mode right before we disable the regulator. But if the regulator is > > > fixed, so the disable has no effect, this seems OK. > > > > What do you mean by fixed? > What I tried to say here is if we have a fixed regulator - or in others > words a supply voltage we cannot turn off, then entering sleep mode is > important to reduce power consumption. > But any sane design where power consumption is a concern will have the > possibility to turn off the power anyway. Ok got it! > > > > > > > > > Please fix the unprepare to not jump out early, on top of or together > > > with the other fix. > > > > After this patch the unprepare only bails out early if the bool > > 'prepared' flag isn't set. > OK, then everything is fine. > Doug pointed out that enable isn't symmetric because it doesn't do the DSI writes. I've updated the patch and I'll send a v2.
Re: [PATCH] drm/panel: boe-tv101wum-nl6: Ensure DSI writes succeed during disable
Quoting Sam Ravnborg (2023-01-07 12:28:41)
> On Thu, Jan 05, 2023 at 07:01:08PM -0800, Stephen Boyd wrote:
> > The unprepare sequence has started to fail after moving to panel bridge
> > code in the msm drm driver (commit 007ac0262b0d ("drm/msm/dsi: switch to
> > DRM_PANEL_BRIDGE")). You'll see messages like this in the kernel logs:
> >
> >panel-boe-tv101wum-nl6 ae94000.dsi.0: failed to set panel off: -22
> >
> > This is because boe_panel_enter_sleep_mode() needs an operating DSI link
> > to set the panel into sleep mode. Performing those writes in the
> > unprepare phase of bridge ops is too late, because the link has already
> > been torn down by the DSI controller in post_disable, i.e. the PHY has
> > been disabled, etc. See dsi_mgr_bridge_post_disable() for more details
> > on the DSI .
> >
> > Split the unprepare function into a disable part and an unprepare part.
> > For now, just the DSI writes to enter sleep mode are put in the disable
> > function. This fixes the panel off routine and keeps the panel happy.
> >
> > My Wormdingler has an integrated touchscreen that stops responding to
> > touch if the panel is only half disabled too. This patch fixes it. And
> > finally, this saves power when the screen is off because without this
> > fix the regulators for the panel are left enabled when nothing is being
> > displayed on the screen.
>
> The pattern we use in several (but not all) panel drivers are that
> errors in unprepare/disable are logged but the function do not skip
> the remainder of the function. This is to avoid that we do not disable
> power supplies etc.
Ah that would have made it so I didn't see a problem. But I wonder if
the panel gets borked if you don't disable it via DSI writes before
yanking the power?
>
> For this case we could ask ourself if the display needs to enter sleep
> mode right before we disable the regulator. But if the regulator is
> fixed, so the disable has no effect, this seems OK.
What do you mean by fixed?
>
> Please fix the unprepare to not jump out early, on top of or together
> with the other fix.
After this patch the unprepare only bails out early if the bool
'prepared' flag isn't set. Are you suggesting to get rid of that flag
and unconditionally disable the regulators? Is it possible for the
unprepare to be called multiple times without a balanced call to
prepare?
[PATCH] drm/panel: boe-tv101wum-nl6: Ensure DSI writes succeed during disable
The unprepare sequence has started to fail after moving to panel bridge
code in the msm drm driver (commit 007ac0262b0d ("drm/msm/dsi: switch to
DRM_PANEL_BRIDGE")). You'll see messages like this in the kernel logs:
panel-boe-tv101wum-nl6 ae94000.dsi.0: failed to set panel off: -22
This is because boe_panel_enter_sleep_mode() needs an operating DSI link
to set the panel into sleep mode. Performing those writes in the
unprepare phase of bridge ops is too late, because the link has already
been torn down by the DSI controller in post_disable, i.e. the PHY has
been disabled, etc. See dsi_mgr_bridge_post_disable() for more details
on the DSI .
Split the unprepare function into a disable part and an unprepare part.
For now, just the DSI writes to enter sleep mode are put in the disable
function. This fixes the panel off routine and keeps the panel happy.
My Wormdingler has an integrated touchscreen that stops responding to
touch if the panel is only half disabled too. This patch fixes it. And
finally, this saves power when the screen is off because without this
fix the regulators for the panel are left enabled when nothing is being
displayed on the screen.
Fixes: 007ac0262b0d ("drm/msm/dsi: switch to DRM_PANEL_BRIDGE")
Fixes: a869b9db7adf ("drm/panel: support for boe tv101wum-nl6 wuxga dsi video
mode panel")
Cc: yangcong
Cc: Douglas Anderson
Cc: Jitao Shi
Cc: Sam Ravnborg
Cc: Rob Clark
Cc: Dmitry Baryshkov
Signed-off-by: Stephen Boyd
---
drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c | 16
1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c
b/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c
index 857a2f0420d7..c924f1124ebc 100644
--- a/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c
+++ b/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c
@@ -1193,14 +1193,11 @@ static int boe_panel_enter_sleep_mode(struct boe_panel
*boe)
return 0;
}
-static int boe_panel_unprepare(struct drm_panel *panel)
+static int boe_panel_disable(struct drm_panel *panel)
{
struct boe_panel *boe = to_boe_panel(panel);
int ret;
- if (!boe->prepared)
- return 0;
-
ret = boe_panel_enter_sleep_mode(boe);
if (ret < 0) {
dev_err(panel->dev, "failed to set panel off: %d\n", ret);
@@ -1209,6 +1206,16 @@ static int boe_panel_unprepare(struct drm_panel *panel)
msleep(150);
+ return 0;
+}
+
+static int boe_panel_unprepare(struct drm_panel *panel)
+{
+ struct boe_panel *boe = to_boe_panel(panel);
+
+ if (!boe->prepared)
+ return 0;
+
if (boe->desc->discharge_on_disable) {
regulator_disable(boe->avee);
regulator_disable(boe->avdd);
@@ -1528,6 +1535,7 @@ static enum drm_panel_orientation
boe_panel_get_orientation(struct drm_panel *pa
}
static const struct drm_panel_funcs boe_panel_funcs = {
+ .disable = boe_panel_disable,
.unprepare = boe_panel_unprepare,
.prepare = boe_panel_prepare,
.enable = boe_panel_enable,
base-commit: 1b929c02afd37871d5afb9d498426f83432e71c2
--
https://chromeos.dev
Re: [PATCH 2/5] clk: qcom: gdsc: Support 'synced_poweroff' genpd flag
Quoting Akhil P Oommen (2022-12-15 07:10:58) > Add support for the newly added 'synced_poweroff' genpd flag. This allows > some clients (like adreno gpu driver) to request gdsc driver to ensure > a votable gdsc (like gpucc cx gdsc) has collapsed at hardware. > > Signed-off-by: Akhil P Oommen > --- > > drivers/clk/qcom/gdsc.c | 11 ++- > 1 file changed, 6 insertions(+), 5 deletions(-) > > diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c > index 9e4d6ce891aa..575019ba4768 100644 > --- a/drivers/clk/qcom/gdsc.c > +++ b/drivers/clk/qcom/gdsc.c > @@ -136,7 +136,8 @@ static int gdsc_update_collapse_bit(struct gdsc *sc, bool > val) > return 0; > } > > -static int gdsc_toggle_logic(struct gdsc *sc, enum gdsc_status status) > +static int gdsc_toggle_logic(struct gdsc *sc, enum gdsc_status status, > + bool force_sync) Maybe just 'wait'? That matches kernel style and is short.
Re: [PATCH v12 2/5] dt-bindings: msm/dp: add data-lanes and link-frequencies property
Quoting Dmitry Baryshkov (2022-12-15 13:12:49) > On 15/12/2022 02:38, Stephen Boyd wrote: > > Quoting Kuogee Hsieh (2022-12-14 14:56:23) > >> > >> Once link training start, then there are no any interactions between > >> controller and phy during link training session. > > > > What do you mean? The DP controller calls phy_configure() and changes > > the link rate. The return value from phy_configure() should be checked > > and link training should skip link rates that aren't supported and/or > > number of lanes that aren't supported. > > I'd toss another coin into the argument. We have previously discussed > using the link-frequencies property in the context of limiting link > speeds for the DSI. There we have both hardware (SoC) limitations and > the board limitations as in some cases the DSI lanes can not sustain > some high rate. I still hope for these patches to materialize at some point. > > For the DP this is more or less the same story. We have the hardware > (SoC, PHY, etc) limitations, but also we have the board/device > limitations. For example some of the board might not be able to support > HBR3 e.g. because of the PCB design. And while it might be logical to > also add the 'max bit rate' support to the eDP & combo PHYs, it > definitely makes sense to be able to limit the rate on the DP <-> > `something' link. Honestly I don't think the PHY even makes sense to put the link rate property. In the case of Trogdor, the DP controller and DP PHY both support all DP link frequencies. The limiting factor is the TCPC redriver that is only rated to support HBR2. We don't describe the TCPC in DT because the EC controls it. This means we have to put the limit *somewhere*, and putting it in the DP output node is the only place we have right now. I would really prefer we put it wherever the limit is, in this case either in the EC node or on the type-c ports. Another nice to have feature would be to support different TCPCs connected to the same DP port. We were considering doing this on Trogdor, where we would have a TCPC rated for HBR2 and another TCPC rated for HBR3 and then detect which TCPC was in use to adjust the supported link rates. We didn't do this though, so the idea got back-burnered. When the SoC is directly wired to a DP connector, I'd expect the connector to have the link rate property. That's because the connector or the traces outside of the SoC will be the part that's limiting the supported frequencies, not the SoC. The graph would need to be walked to find the link rate of course. The PHY could do this just as much as the DP controller could. > > Now, for all the practical purposes this `something' for the DP is the > DP connector, the eDP panel or the USB-C mux (with the possible > redrivers in the middle). > > Thus I'd support Kuogee's proposal to have link-frequencies in the DP's > outbound endpoint. This is the link which will be driven by the data > stream from the Linux point of view. The PHY is linked through the > 'phys' property, but it doesn't participate in the USB-C (or in the > connector/panel) graph. Why doesn't the PHY participate in the graph? The eDP panel could just as easily be connected to the eDP PHY if the PHY participated in the graph. > > Now let's discuss the data lanes. Currently we have them in the DP > property itself. Please correct me if I'm wrong, but I think that we can > drop it for all the practical purposes. I vaguely recall that the driver was checking data-lanes to figure out how many lanes are usable. This is another shortcut taken on Trogdor to work around a lack of complete DP bindings. We only support two lanes of DP on Trogdor. > Judging by the DP compat string > the driver can determine if it uses 2 lanes (eDP) or 4 lanes > (full-featured DP). In case of USB-C when the altmode dictates whether > to use 2 or 4 lanes, the TCPM (Type-C Port Manager) will negotiate the > mode and pin configuration, then inform the DP controller about the > selected amount of lanes. Then DP informs the PHY about the selection > (note, PHY doesn't have control at all in this scenario). > > The only problematic case is the mixed mode ports, which if I understand > correctly, can be configured either to eDP or DP modes. I'm not sure who > specifies and limits the amount of lanes available to the DP controller. > This would depend on where we send the type-c message in the kernel. It really gets to the heart of the question too. Should the PHY be "dumb" and do whatever the controller tells it to do? Or should the PHY be aware of what's going on and take action itself? Note that the data-lanes property is also used to remap lanes. On sc7180 the lane remapping happens in the DP PHY, and then the type-c PHY can flip that too, so if we don't involve the PHY(s) in th
Re: [PATCH v12 2/5] dt-bindings: msm/dp: add data-lanes and link-frequencies property
Quoting Kuogee Hsieh (2022-12-15 09:08:04) > > On 12/14/2022 4:38 PM, Stephen Boyd wrote: > > Quoting Kuogee Hsieh (2022-12-14 14:56:23) > >> On 12/13/2022 3:06 PM, Stephen Boyd wrote: > >>> Quoting Kuogee Hsieh (2022-12-13 13:44:05) > > > >> Therefore I think add data-lanes and link-frequencies properties in the > >> DP PHY binding directly will not helps. > >> > > I didn't follow your logic. Sorry. > > Sorry, probably i did not understand your proposal clearly. > > 1) move both data-lanes and link-frequencies property from dp controller > endpoint to phy > > 2) phy_configure() return succeed if both data-lanes and link > frequencies are supported. otherwise return failed. > > is above two summary items correct? Yes. > > Currently phy_configure() is part of link training process and called > if link lanes or rate changes. > > however, since current phy_configure() implementation always return 0, > the return value is not checking. > > This proposal is new, can we discuss more detail at meeting and decide > to implement it or not. > > Meanwhile can we merge current implementation (both data-lanes and > link-frequqncies at dp controller end point) first? > I don't think we can merge this patch because it depends on a DT binding change. If the PHY approach works then I'd prefer we just go with that.
Re: [PATCH v2] drm/msm/dp: do not complete dp_aux_cmd_fifo_tx() if irq is not for aux transfer
Quoting Dmitry Baryshkov (2022-12-15 10:46:42) > On 15/12/2022 20:32, Kuogee Hsieh wrote: > > if (!aux->cmd_busy) > > return; > > > > if (aux->native) > > - dp_aux_native_handler(aux, isr); > > + ret = dp_aux_native_handler(aux, isr); > > else > > - dp_aux_i2c_handler(aux, isr); > > + ret = dp_aux_i2c_handler(aux, isr); > > > > - complete(>comp); > > + if (ret == IRQ_HANDLED) > > + complete(>comp); > > Can you just move the complete() into the individual handling functions? > Then you won't have to return the error code from dp_aux_*_handler() at > all. You can check `isr' in that function and call complete if there was > any error. I'd prefer we apply my patch and pass the irqreturn_t variable to the caller of this function so spurious irqs are shutdown. Should I send it as a proper patch?
Re: [PATCH v12 2/5] dt-bindings: msm/dp: add data-lanes and link-frequencies property
Quoting Kuogee Hsieh (2022-12-14 14:56:23)
>
> On 12/13/2022 3:06 PM, Stephen Boyd wrote:
> > Quoting Kuogee Hsieh (2022-12-13 13:44:05)
> >> Add both data-lanes and link-frequencies property into endpoint
> > Why do we care? Please tell us why it's important.
Any response?
> >> @@ -193,6 +217,8 @@ examples:
> >> reg = <1>;
> >> endpoint {
> >> remote-endpoint = <>;
> >> +data-lanes = <0 1>;
> >> +link-frequencies = /bits/ 64 <162000 27
> >> 54 81>;
> >> };
> > So far we haven't used the output port on the DP controller in DT.
> >
> > I'm still not clear on what we should do in general for DP because
> > there's a PHY that actually controls a lane count and lane mapping. In
> > my mental model of the SoC, this DP controller's output port is
> > connected to the DP PHY, which then sends the DP lanes out of the SoC to
> > the next downstream device (i.e. a DP connector or type-c muxer). Having
> > a remote-endpoint property with a phandle to typec doesn't fit my mental
> > model. I'd expect it to be the typec PHY.
> ack
> >
> > That brings up the question: when we have 2 lanes vs. 4 lanes will we
> > duplicate the data-lanes property in the PHY binding? I suspect we'll
> > have to. Hopefully that sort of duplication is OK?
> Current we have limitation by reserve 2 data lanes for usb2, i am not
> sure duplication to 4 lanes will work automatically.
> >
> > Similarly, we may have a redriver that limits the link-frequencies
> > property further (e.g. only support <= 2.7GHz). Having multiple
> > link-frequencies along the graph is OK, right? And isn't the
> > link-frequencies property known here by fact that the DP controller
> > tells us which SoC this controller is for, and thus we already know the
> > supported link frequencies?
> >
> > Finally, I wonder if we should put any of this in the DP controller's
> > output endpoint, or if we can put these sorts of properties in the DP
> > PHY binding directly? Can't we do that and then when the DP controller
> > tries to set 4 lanes, the PHY immediately fails the call and the link
> > training algorithm does its thing and tries fewer lanes? And similarly,
> > if link-frequencies were in the PHY's binding, the PHY could fail to set
> > those frequencies during link training, returning an error to the DP
> > controller, letting the training move on to a lower frequency. If we did
> > that this patch series would largely be about modifying the PHY binding,
> > updating the PHY driver to enforce constraints, and handling errors
> > during link training in the DP controller (which may already be done? I
> > didn't check).
>
>
> phy/pll have different configuration base on link lanes and rate.
>
> it has to be set up before link training can start.
>
> Once link training start, then there are no any interactions between
> controller and phy during link training session.
What do you mean? The DP controller calls phy_configure() and changes
the link rate. The return value from phy_configure() should be checked
and link training should skip link rates that aren't supported and/or
number of lanes that aren't supported.
>
> Link training only happen between dp controller and sink since link
> status is reported by sink (read back from sink's dpcd register directly).
>
> T achieve link symbol locked, link training will start from reduce link
> rate until lowest rate, if it still failed, then it will reduce lanes
> with highest rate and start training again.
>
> it will repeat same process until lowest lane (one lane), if it still
> failed, then it will give up and declare link training failed.
Yes, that describes the link training algorithm. I don't see why
phy_configure() return value can't be checked and either number of lanes
or link frequencies be checked. If only two lanes are supported, then
phy_configure() will fail for the 4 link rates and the algorithm will
reduce the number of lanes and go back to the highest rate. Then when
the highest rate isn't supported it will drop link rate until the link
rate is supported.
>
> Therefore I think add data-lanes and link-frequencies properties in the
> DP PHY binding directly will not helps.
>
I didn't follow your logic. Sorry.
Re: [PATCH] drm/msm/dp: do not complete dp_aux_cmd_fifo_tx() if irq is not for aux transfer
Quoting Doug Anderson (2022-12-14 16:14:42) > Hi, > > On Wed, Dec 14, 2022 at 3:46 PM Abhinav Kumar > wrote: > > > > Hi Doug > > > > On 12/14/2022 2:29 PM, Doug Anderson wrote: > > > Hi, > > > > > > On Wed, Dec 14, 2022 at 1:21 PM Kuogee Hsieh > > > wrote: > > >> > > >> There are 3 possible interrupt sources are handled by DP controller, > > >> HPDstatus, Controller state changes and Aux read/write transaction. > > >> At every irq, DP controller have to check isr status of every interrupt > > >> sources and service the interrupt if its isr status bits shows interrupts > > >> are pending. There is potential race condition may happen at current aux > > >> isr handler implementation since it is always complete > > >> dp_aux_cmd_fifo_tx() > > >> even irq is not for aux read or write transaction. This may cause aux > > >> read > > >> transaction return premature if host aux data read is in the middle of > > >> waiting for sink to complete transferring data to host while irq happen. > > >> This will cause host's receiving buffer contains unexpected data. This > > >> patch fixes this problem by checking aux isr and return immediately at > > >> aux isr handler if there are no any isr status bits set. > > >> > > >> Follows are the signature at kernel logs when problem happen, > > >> EDID has corrupt header > > >> panel-simple-dp-aux aux-aea.edp: Couldn't identify panel via EDID > > >> panel-simple-dp-aux aux-aea.edp: error -EIO: Couldn't detect panel > > >> nor find a fallback > > >> > > >> Signed-off-by: Kuogee Hsieh > > >> --- > > >> drivers/gpu/drm/msm/dp/dp_aux.c | 7 +++ > > >> 1 file changed, 7 insertions(+) > > >> > > >> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c > > >> b/drivers/gpu/drm/msm/dp/dp_aux.c > > >> index d030a93..8f8b12a 100644 > > >> --- a/drivers/gpu/drm/msm/dp/dp_aux.c > > >> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c > > >> @@ -423,6 +423,13 @@ void dp_aux_isr(struct drm_dp_aux *dp_aux) > > >> > > >> isr = dp_catalog_aux_get_irq(aux->catalog); > > >> > > >> + /* > > >> +* if this irq is not for aux transfer, > > >> +* then return immediately > > >> +*/ > > > > > > Why do you need 4 lines for a comment that fits on one line? > > Yes, we can fit this to one line. > > > > > >> + if (!isr) > > >> + return; > > > > > > I can confirm that this works for me. I could reproduce the EDID > > > problems in the past and I can't after this patch. ...so I could give > > > a: > > > > > > Tested-by: Douglas Anderson > > > > > > I'm not an expert on this part of the code, so feel free to ignore my > > > other comments if everyone else thinks this patch is fine as-is, but > > > to me something here feels a little fragile. It feels a little weird > > > that we'll "complete" for _any_ interrupt that comes through now > > > rather than relying on dp_aux_native_handler() / dp_aux_i2c_handler() > > > to specifically identify interrupts that caused the end of the > > > transfer. I guess that idea is that every possible interrupt we get > > > causes the end of the transfer? > > > > > > -Doug > > > > So this turned out to be more tricky and was a good finding from kuogee. > > > > In the bad EDID case, it was technically not bad EDID. > > > > What was happening was, the VIDEO_READY interrupt was continuously > > firing. Ideally, this should fire only once but due to some error > > condition it kept firing. We dont exactly know why yet what was the > > error condition making it continuously fire. This is a great detail that is missing from the commit text. > > > > In the DP ISR, the dp_aux_isr() gets called even if it was not an aux > > interrupt which fired (so the call flow in this case was > > dp_display_irq_handler (triggered for VIDEO_READY) ---> dp_aux_isr() > > So we should certainly have some protection to return early from this > > routine if there was no aux interrupt which fired. I'm not sure that's a race condition though, more like a problem where the completion is called unconditionally? > > > > Which is what this fix is doing. > > > > Its not completing any interrupt, its just returning early if no aux > > interrupt fired. > > ...but the whole problem was that it was doing the complete() at the > end, right? Kuogee even mentioned that in the commit message. > Specifically, I checked dp_aux_native_handler() and > dp_aux_i2c_handler(), both of which are passed the "isr". Unless I > messed up, both functions already were no-ops if the ISR was 0, even > before Kuogee's patch. That means that the only thing Kuogee's patch > does is to prevent the call to "complete(>comp)" at the end of > "dp_aux_isr()". > > ...and it makes sense not to call the complete() if no "isr" is 0. > ...but what I'm saying is that _any_ non-zero value of ISR will still > cause the complete() to be called after Kuogee's patch. That means > that if any of the 32-bits in the "isr" variable are set, that we will > call complete(). I'm asking if you're
