| Message ID | 20240819223834.2049862-3-swboyd@chromium.org (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | platform/chrome: Add DT USB/DP muxing/topology support | expand |
On Mon, Aug 19, 2024 at 03:38:16PM -0700, Stephen Boyd wrote: > 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 <andrzej.hajda@intel.com> > Cc: Neil Armstrong <neil.armstrong@linaro.org> > Cc: Robert Foss <rfoss@kernel.org> > Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com> > Cc: Jonas Karlman <jonas@kwiboo.se> > Cc: Jernej Skrabec <jernej.skrabec@gmail.com> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: Maxime Ripard <mripard@kernel.org> > Cc: Thomas Zimmermann <tzimmermann@suse.de> > Cc: David Airlie <airlied@gmail.com> > Cc: Daniel Vetter <daniel@ffwll.ch> > Cc: <dri-devel@lists.freedesktop.org> > Cc: Pin-yen Lin <treapking@chromium.org> > Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> Yeah, I really think that the appearance of this thousandth time in the Git history has almost no value and just pollutes the commit message makes it not very well readable. The only outcome is exercising the compression algo used by Git. ... > + int i; unsigned? ... > + /* > + * 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; Besides missing {} this code is internal to the Linux kernel. Is it okay?
Quoting Andy Shevchenko (2024-08-20 03:09:29) > On Mon, Aug 19, 2024 at 03:38:16PM -0700, Stephen Boyd wrote: > > 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 <andrzej.hajda@intel.com> > > Cc: Neil Armstrong <neil.armstrong@linaro.org> > > Cc: Robert Foss <rfoss@kernel.org> > > Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com> > > Cc: Jonas Karlman <jonas@kwiboo.se> > > Cc: Jernej Skrabec <jernej.skrabec@gmail.com> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > Cc: Maxime Ripard <mripard@kernel.org> > > Cc: Thomas Zimmermann <tzimmermann@suse.de> > > Cc: David Airlie <airlied@gmail.com> > > Cc: Daniel Vetter <daniel@ffwll.ch> > > Cc: <dri-devel@lists.freedesktop.org> > > Cc: Pin-yen Lin <treapking@chromium.org> > > Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > Yeah, I really think that the appearance of this thousandth time in the Git > history has almost no value and just pollutes the commit message makes it not > very well readable. The only outcome is exercising the compression algo used > by Git. I'll leave the decision up to the maintainers. > > ... > > > + /* > > + * 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; > > Besides missing {} this code is internal to the Linux kernel. Is it okay? > ENOTSUPP is used by select_bus_fmt_recursive() so I simply followed that style.
On Tue, Aug 20, 2024 at 10:12:55AM -0700, Stephen Boyd wrote: > Quoting Andy Shevchenko (2024-08-20 03:09:29) > > On Mon, Aug 19, 2024 at 03:38:16PM -0700, Stephen Boyd wrote: ... > > Yeah, I really think that the appearance of this thousandth time in the Git > > history has almost no value and just pollutes the commit message makes it not > > very well readable. The only outcome is exercising the compression algo used > > by Git. > > I'll leave the decision up to the maintainers. Sure! ... > > > + /* > > > + * 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; > > > > Besides missing {} this code is internal to the Linux kernel. Is it okay? > > ENOTSUPP is used by select_bus_fmt_recursive() so I simply followed that > style. Okay, just be aware of that side effect of that code, also checkpatch may complain (however it might be false positive).
Quoting Andy Shevchenko (2024-08-20 10:17:46) > On Tue, Aug 20, 2024 at 10:12:55AM -0700, Stephen Boyd wrote: > > Quoting Andy Shevchenko (2024-08-20 03:09:29) > > > On Mon, Aug 19, 2024 at 03:38:16PM -0700, Stephen Boyd wrote: > > > > + /* > > > > + * 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; > > > > > > Besides missing {} this code is internal to the Linux kernel. Is it okay? > > > > ENOTSUPP is used by select_bus_fmt_recursive() so I simply followed that > > style. > > Okay, just be aware of that side effect of that code, also checkpatch may > complain (however it might be false positive). Yes checkpatch complained but didn't enlighten me. Please tell me the side effect as I'm unaware!
On Tue, Aug 20, 2024 at 10:24:47AM -0700, Stephen Boyd wrote: > Quoting Andy Shevchenko (2024-08-20 10:17:46) > > On Tue, Aug 20, 2024 at 10:12:55AM -0700, Stephen Boyd wrote: > > > Quoting Andy Shevchenko (2024-08-20 03:09:29) > > > > On Mon, Aug 19, 2024 at 03:38:16PM -0700, Stephen Boyd wrote: ... > > > > > + /* > > > > > + * 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; > > > > > > > > Besides missing {} this code is internal to the Linux kernel. Is it okay? > > > > > > ENOTSUPP is used by select_bus_fmt_recursive() so I simply followed that > > > style. > > > > Okay, just be aware of that side effect of that code, also checkpatch may > > complain (however it might be false positive). > > Yes checkpatch complained but didn't enlighten me. Please tell me the > side effect as I'm unaware! I already told you above, if this code ever appears in user space it will be printed as a number and very much confuse the user! That's why usage of this code either has to be documented or be subsystem _known_ practice (GPIO library comes to my mind as it uses it internally,\ but filters for user space).
Quoting Andy Shevchenko (2024-08-20 10:29:41) > On Tue, Aug 20, 2024 at 10:24:47AM -0700, Stephen Boyd wrote: > > Quoting Andy Shevchenko (2024-08-20 10:17:46) > > > On Tue, Aug 20, 2024 at 10:12:55AM -0700, Stephen Boyd wrote: > > > > Quoting Andy Shevchenko (2024-08-20 03:09:29) > > > > > On Mon, Aug 19, 2024 at 03:38:16PM -0700, Stephen Boyd wrote: > > ... > > > > > > > + /* > > > > > > + * 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; > > > > > > > > > > Besides missing {} this code is internal to the Linux kernel. Is it okay? > > > > > > > > ENOTSUPP is used by select_bus_fmt_recursive() so I simply followed that > > > > style. > > > > > > Okay, just be aware of that side effect of that code, also checkpatch may > > > complain (however it might be false positive). > > > > Yes checkpatch complained but didn't enlighten me. Please tell me the > > side effect as I'm unaware! > > I already told you above, if this code ever appears in user space it will be > printed as a number and very much confuse the user! > > That's why usage of this code either has to be documented or be subsystem > _known_ practice (GPIO library comes to my mind as it uses it internally,\ > but filters for user space). > Ok, got it. Thanks!
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index bd18c1e91dee..68c7a321b9b3 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -860,6 +860,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; @@ -873,12 +877,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, &crtc_state->mode, &crtc_state->adjusted_mode)) return -EINVAL; } + input_lanes = drm_bridge_next_bridge_lane_cfg(bridge, + crtc_state->state, + &num_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; }
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 <andrzej.hajda@intel.com> Cc: Neil Armstrong <neil.armstrong@linaro.org> Cc: Robert Foss <rfoss@kernel.org> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com> Cc: Jonas Karlman <jonas@kwiboo.se> Cc: Jernej Skrabec <jernej.skrabec@gmail.com> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Cc: Maxime Ripard <mripard@kernel.org> Cc: Thomas Zimmermann <tzimmermann@suse.de> Cc: David Airlie <airlied@gmail.com> Cc: Daniel Vetter <daniel@ffwll.ch> Cc: <dri-devel@lists.freedesktop.org> Cc: Pin-yen Lin <treapking@chromium.org> Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> Signed-off-by: Stephen Boyd <swboyd@chromium.org> --- drivers/gpu/drm/drm_bridge.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)