Hello all， At 2025-06-19 21:09:09, "Maxime Ripard" <[email protected]> wrote: >On Sat, May 24, 2025 at 11:09:48AM +0300, Dmitry Baryshkov wrote: >> On Sun, Mar 23, 2025 at 04:22:27AM +0200, Dmitry Baryshkov wrote: >> > On Fri, Mar 21, 2025 at 10:46:56AM +0100, Maxime Ripard wrote: >> > > On Tue, Mar 18, 2025 at 09:00:29PM +0200, Dmitry Baryshkov wrote: >> > > > On Tue, Mar 18, 2025 at 04:51:19PM +0100, Maxime Ripard wrote: >> > > > > On Fri, Mar 14, 2025 at 08:28:22PM +0200, Dmitry Baryshkov wrote: >> > > > > > On Fri, Mar 14, 2025 at 06:40:24PM +0100, Maxime Ripard wrote: >> > > > > > > On Fri, Mar 14, 2025 at 09:59:36AM +0200, Dmitry Baryshkov wrote: >> > > > > > > > On Fri, Mar 14, 2025 at 08:45:17AM +0100, Maxime Ripard wrote: >> > > > > > > > > On Fri, Mar 14, 2025 at 07:52:35AM +0200, Dmitry Baryshkov >> > > > > > > > > wrote: >> > > > > > > > > > On Fri, Mar 14, 2025 at 08:50:29AM +0800, Andy Yan wrote: >> > > > > > > > > > > At 2025-03-13 19:55:33, "Maxime Ripard" >> > > > > > > > > > > <[email protected]> wrote: >> > > > > > > > > > > >Hi, >> > > > > > > > > > > > >> > > > > > > > > > > >On Thu, Mar 13, 2025 at 04:09:54PM +0800, Andy Yan >> > > > > > > > > > > >wrote: >> > > > > > > > > > > >> At 2025-03-05 19:55:19, "Andy Yan" <[email protected]> >> > > > > > > > > > > >> wrote: >> > > > > > > > > > > >> >At 2025-03-04 19:10:47, "Maxime Ripard" >> > > > > > > > > > > >> ><[email protected]> wrote: >> > > > > > > > > > > >> >>With the bridges switching over to >> > > > > > > > > > > >> >>drm_bridge_connector, the direct >> > > > > > > > > > > >> >>association between a bridge driver and its >> > > > > > > > > > > >> >>connector was lost. >> > > > > > > > > > > >> >> >> > > > > > > > > > > >> >>This is mitigated for atomic bridge drivers by the >> > > > > > > > > > > >> >>fact you can access >> > > > > > > > > > > >> >>the encoder, and then call >> > > > > > > > > > > >> >>drm_atomic_get_old_connector_for_encoder() or >> > > > > > > > > > > >> >>drm_atomic_get_new_connector_for_encoder() with >> > > > > > > > > > > >> >>drm_atomic_state. >> > > > > > > > > > > >> >> >> > > > > > > > > > > >> >>This was also made easier by providing >> > > > > > > > > > > >> >>drm_atomic_state directly to all >> > > > > > > > > > > >> >>atomic hooks bridges can implement. >> > > > > > > > > > > >> >> >> > > > > > > > > > > >> >>However, bridge drivers don't have a way to access >> > > > > > > > > > > >> >>drm_atomic_state >> > > > > > > > > > > >> >>outside of the modeset path, like from the hotplug >> > > > > > > > > > > >> >>interrupt path or any >> > > > > > > > > > > >> >>interrupt handler. >> > > > > > > > > > > >> >> >> > > > > > > > > > > >> >>Let's introduce a function to retrieve the >> > > > > > > > > > > >> >>connector currently assigned >> > > > > > > > > > > >> >>to an encoder, without using drm_atomic_state, to >> > > > > > > > > > > >> >>make these drivers' >> > > > > > > > > > > >> >>life easier. >> > > > > > > > > > > >> >> >> > > > > > > > > > > >> >>Reviewed-by: Dmitry Baryshkov >> > > > > > > > > > > >> >><[email protected]> >> > > > > > > > > > > >> >>Co-developed-by: Simona Vetter >> > > > > > > > > > > >> >><[email protected]> >> > > > > > > > > > > >> >>Signed-off-by: Maxime Ripard <[email protected]> >> > > > > > > > > > > >> >>--- >> > > > > > > > > > > >> >> drivers/gpu/drm/drm_atomic.c | 45 >> > > > > > > > > > > >> >> ++++++++++++++++++++++++++++++++++++++++++++ >> > > > > > > > > > > >> >> include/drm/drm_atomic.h | 3 +++ >> > > > > > > > > > > >> >> 2 files changed, 48 insertions(+) >> > > > > > > > > > > >> >> >> > > > > > > > > > > >> >>diff --git a/drivers/gpu/drm/drm_atomic.c >> > > > > > > > > > > >> >>b/drivers/gpu/drm/drm_atomic.c >> > > > > > > > > > > >> >>index >> > > > > > > > > > > >> >>9ea2611770f43ce7ccba410406d5f2c528aab022..b926b132590e78f8d41d48eb4da4bccf170ee236 >> > > > > > > > > > > >> >> 100644 >> > > > > > > > > > > >> >>--- a/drivers/gpu/drm/drm_atomic.c >> > > > > > > > > > > >> >>+++ b/drivers/gpu/drm/drm_atomic.c >> > > > > > > > > > > >> >>@@ -985,10 +985,55 @@ >> > > > > > > > > > > >> >>drm_atomic_get_new_connector_for_encoder(const >> > > > > > > > > > > >> >>struct drm_atomic_state *state, >> > > > > > > > > > > >> >> >> > > > > > > > > > > >> >> return NULL; >> > > > > > > > > > > >> >> } >> > > > > > > > > > > >> >> EXPORT_SYMBOL(drm_atomic_get_new_connector_for_encoder); >> > > > > > > > > > > >> >> >> > > > > > > > > > > >> >>+/** >> > > > > > > > > > > >> >>+ * drm_atomic_get_connector_for_encoder - Get >> > > > > > > > > > > >> >>connector currently assigned to an encoder >> > > > > > > > > > > >> >>+ * @encoder: The encoder to find the connector of >> > > > > > > > > > > >> >>+ * @ctx: Modeset locking context >> > > > > > > > > > > >> >>+ * >> > > > > > > > > > > >> >>+ * This function finds and returns the connector >> > > > > > > > > > > >> >>currently assigned to >> > > > > > > > > > > >> >>+ * an @encoder. >> > > > > > > > > > > >> >>+ * >> > > > > > > > > > > >> >>+ * Returns: >> > > > > > > > > > > >> >>+ * The connector connected to @encoder, or an >> > > > > > > > > > > >> >>error pointer otherwise. >> > > > > > > > > > > >> >>+ * When the error is EDEADLK, a deadlock has been >> > > > > > > > > > > >> >>detected and the >> > > > > > > > > > > >> >>+ * sequence must be restarted. >> > > > > > > > > > > >> >>+ */ >> > > > > > > > > > > >> >>+struct drm_connector * >> > > > > > > > > > > >> >>+drm_atomic_get_connector_for_encoder(const struct >> > > > > > > > > > > >> >>drm_encoder *encoder, >> > > > > > > > > > > >> >>+ struct >> > > > > > > > > > > >> >>drm_modeset_acquire_ctx *ctx) >> > > > > > > > > > > >> >>+{ >> > > > > > > > > > > >> >>+ struct drm_connector_list_iter conn_iter; >> > > > > > > > > > > >> >>+ struct drm_connector *out_connector = >> > > > > > > > > > > >> >>ERR_PTR(-EINVAL); >> > > > > > > > > > > >> >>+ struct drm_connector *connector; >> > > > > > > > > > > >> >>+ struct drm_device *dev = encoder->dev; >> > > > > > > > > > > >> >>+ int ret; >> > > > > > > > > > > >> >>+ >> > > > > > > > > > > >> >>+ ret = >> > > > > > > > > > > >> >>drm_modeset_lock(&dev->mode_config.connection_mutex, >> > > > > > > > > > > >> >> ctx); >> > > > > > > > > > > >> >>+ if (ret) >> > > > > > > > > > > >> >>+ return ERR_PTR(ret); >> > > > > > > > > > > >> > >> > > > > > > > > > > >> >It seems that this will cause a deadlock when called >> > > > > > > > > > > >> >from a hotplug handling path, >> > > > > > > > > > > >> >I have a WIP DP diver[0], which suggested by Dmitry >> > > > > > > > > > > >> >to use this API from a >> > > > > > > > > > > >> >&drm_bridge_funcs.detect callback to get the >> > > > > > > > > > > >> >connector, as detect is called by >> > > > > > > > > > > >> >drm_helper_probe_detect, >> > > > > > > > > > > >> >which will hold connection_mutex first, so the >> > > > > > > > > > > >> >deaklock happens: >> > > > > > > > > > > >> > >> > > > > > > > > > > >> > >> > > > > > > > > > > >> >drm_helper_probe_detect(struct drm_connector >> > > > > > > > > > > >> >*connector, >> > > > > > > > > > > >> > struct >> > > > > > > > > > > >> > drm_modeset_acquire_ctx *ctx, >> > > > > > > > > > > >> > bool force) >> > > > > > > > > > > >> >{ >> > > > > > > > > > > >> > const struct drm_connector_helper_funcs >> > > > > > > > > > > >> > *funcs = connector->helper_private; >> > > > > > > > > > > >> > struct drm_device *dev = connector->dev; >> > > > > > > > > > > >> > int ret; >> > > > > > > > > > > >> > >> > > > > > > > > > > >> > if (!ctx) >> > > > > > > > > > > >> > return >> > > > > > > > > > > >> > drm_helper_probe_detect_ctx(connector, force); >> > > > > > > > > > > >> > >> > > > > > > > > > > >> > ret = >> > > > > > > > > > > >> > drm_modeset_lock(&dev->mode_config.connection_mutex, >> > > > > > > > > > > >> > ctx); >> > > > > > > > > > > >> > if (ret) >> > > > > > > > > > > >> > return ret; >> > > > > > > > > > > >> > >> > > > > > > > > > > >> > if (funcs->detect_ctx) >> > > > > > > > > > > >> > ret = funcs->detect_ctx(connector, >> > > > > > > > > > > >> > ctx, force); >> > > > > > > > > > > >> > else if (connector->funcs->detect) >> > > > > > > > > > > >> > ret = >> > > > > > > > > > > >> > connector->funcs->detect(connector, force); >> > > > > > > > > > > >> > else >> > > > > > > > > > > >> > ret = connector_status_connected; >> > > > > > > > > > > >> > >> > > > > > > > > > > >> > if (ret != connector->status) >> > > > > > > > > > > >> > connector->epoch_counter += 1; >> > > > > > > > > > > >> > >> > > > > > > > > > > >> >So I wonder can we let drm_bridge_funcs.detect pass >> > > > > > > > > > > >> >a connector for this case ? >> > > > > > > > > > > >> > >> > > > > > > > > > > >> > >> > > > > > > > > > > >> > >> > > > > > > > > > > >> >[0]https://lore.kernel.org/linux-rockchip/[email protected]/T/#m25bc53b79f5cc7bddfcb7aae5656f68df396f094 >> > > > > > > > > > > >> >>+ >> > > > > > > > > > > >> >>+ drm_connector_list_iter_begin(dev, &conn_iter); >> > > > > > > > > > > >> >>+ drm_for_each_connector_iter(connector, >> > > > > > > > > > > >> >>&conn_iter) { >> > > > > > > > > > > >> >>+ if (!connector->state) >> > > > > > > > > > > >> >>+ continue; >> > > > > > > > > > > >> >>+ >> > > > > > > > > > > >> >>+ if (encoder == >> > > > > > > > > > > >> >>connector->state->best_encoder) { >> > > > > > > > > > > >> >>+ out_connector = connector; >> > > > > > > > > > > >> >> > > > > > > > > > > >> >> > > > > > > > > > > >> When try to use this patch in my bridge driver, I >> > > > > > > > > > > >> found that the connector->state->best_encoder >> > > > > > > > > > > >> maybe NULL when drm_bridge_funcs.detect or >> > > > > > > > > > > >> drm_bridge_funcs.detect_ctx is called: >> > > > > > > > > > > >> >> > > > > > > > > > > >> [ 52.713030] Invalid return value -22 for connector >> > > > > > > > > > > >> detection >> > > > > > > > > > > >> [ 52.713539] WARNING: CPU: 7 PID: 288 at >> > > > > > > > > > > >> drivers/gpu/drm/drm_probe_helper.c:602 >> > > > > > > > > > > >> drm_helper_probe_single_connector_modes+0x5e0/ >> > > > > > > > > > > >> 0x63c >> > > > > > > > > > > >> [ 52.714568] Modules linked in: >> > > > > > > > > > > >> >> > > > > > > > > > > >> [ 52.724546] Call trace: >> > > > > > > > > > > >> [ 52.724762] >> > > > > > > > > > > >> drm_helper_probe_single_connector_modes+0x5e0/0x63c >> > > > > > > > > > > >> (P) >> > > > > > > > > > > >> [ 52.725319] drm_mode_getconnector+0x2a4/0x488 >> > > > > > > > > > > >> [ 52.725711] drm_ioctl_kernel+0xb4/0x11c >> > > > > > > > > > > >> [ 52.726057] drm_ioctl+0x22c/0x544 >> > > > > > > > > > > >> [ 52.726358] __arm64_sys_ioctl+0xac/0xe0 >> > > > > > > > > > > >> [ 52.726706] invoke_syscall+0x44/0x100 >> > > > > > > > > > > >> [ 52.727039] el0_svc_common.constprop.0+0x3c/0xd4 >> > > > > > > > > > > >> >> > > > > > > > > > > >> This is because best_encoder is set by >> > > > > > > > > > > >> set_best_encoder, which is called from >> > > > > > > > > > > >> drm_atomic_helper_check_modeset. When we call >> > > > > > > > > > > >> drm_mode_getconnector >> > > > > > > > > > > >> for the first time, the functions mentioned above >> > > > > > > > > > > >> have not been called yet, >> > > > > > > > > > > >> then we can't match the encoder from >> > > > > > > > > > > >> connector->state->best_encoder for this case. >> > > > > > > > > > > > >> > > > > > > > > > > >As far as I'm concerned, it's by design. Encoders and >> > > > > > > > > > > >connectors have >> > > > > > > > > > > >1:N relationship, and only once a connector has been >> > > > > > > > > > > >enabled it has an >> > > > > > > > > > > >encoder. >> > > > > > > > > > > > >> > > > > > > > > > > >If the connector is disabled, there's no associated >> > > > > > > > > > > >encoder. >> > > > > > > > > > > >> > > > > > > > > > > Does this prove that this API is not suitable for my >> > > > > > > > > > > application scenario: >> > > > > > > > > > > Get the connector in the bridge's .detect callback, so >> > > > > > > > > > > this means that I may >> > > > > > > > > > > still need to modify the bridge's connector callback so >> > > > > > > > > > > that it can pass the connector ? >> > > > > > > > > > >> > > > > > > > > > I'd say, yes, please. >> > > > > > > > > >> > > > > > > > > And I'd say no :) >> > > > > > > > >> > > > > > > > Fair enough :-) >> > > > > > > > >> > > > > > > > > There's no reason to deviate from the API other entities >> > > > > > > > > have here. It's >> > > > > > > > > just that the switch to DRM_BRIDGE_ATTACH_NO_CONNECTOR >> > > > > > > > > hasn't been >> > > > > > > > > completely thought through and it's one of the part where it >> > > > > > > > > shows. >> > > > > > > > > >> > > > > > > > > We have two alternative solutions: Either the driver creates >> > > > > > > > > the >> > > > > > > > > connector itself, since it doesn't seem to use any >> > > > > > > > > downstream bridge >> > > > > > > > > anyway, or we need a new bridge helper to find the connector >> > > > > > > > > on a bridge >> > > > > > > > > chain. >> > > > > > > > > >> > > > > > > > > We have the iterator already, we just need a new accessor to >> > > > > > > > > retrieve >> > > > > > > > > the (optional) connector of a bridge, and if there's none, >> > > > > > > > > go to the >> > > > > > > > > next bridge and try again. >> > > > > > > > >> > > > > > > > The problem is that there is no guarantee that the the created >> > > > > > > > connector >> > > > > > > > is created for or linked to any bridge. For example, for msm >> > > > > > > > driver I'm >> > > > > > > > waiting for several series to go in, but after that I plan to >> > > > > > > > work on >> > > > > > > > moving connector creation to the generic code within the msm >> > > > > > > > driver. >> > > > > > > > >> > > > > > > > In other words, with DRM_BRIDGE_ATTACH_NO_CONNECTOR in place >> > > > > > > > it is >> > > > > > > > perfectly legit not to have a bridge which has "connector of a >> > > > > > > > bridge". >> > > > > > > > It is possible to create drm_bridge_connector on the >> > > > > > > > drm_encoder's side >> > > > > > > > after the drm_bridge_attach() succeeds. >> > > > > > > >> > > > > > > Sure, but then I'd expect detect and get_modes to only be called >> > > > > > > *after* >> > > > > > > that connector has been created, right? >> > > > > > >> > > > > > Yes. But you can not get the connector by following bridge chain. >> > > > > > Well, >> > > > > > unless you include encoder into the chain. If that's what you have >> > > > > > had >> > > > > > in mind, then please excuse me, I didn't understand that from the >> > > > > > beginning. >> > > > > >> > > > > You can't include the encoder either, because the encoder doesn't >> > > > > have a >> > > > > connector assigned yet at that time. >> > > > > >> > > > > However, you can: >> > > > > >> > > > > - Store the bridge attach flags in drm_bridge >> > > > > >> > > > > - Create a hook that returns the connector a bridge creates, >> > > > > depending >> > > > > on the attach flags. >> > > > > >> > > > > - Create a helper that iterates over the next bridges until the >> > > > > previous hook returns !NULL. If it doesn't find anything, return >> > > > > NULL. >> > > > > >> > > > > AFAIK, it solves all the problems being discussed here, while dealing >> > > > > with legacy and new-style bridge drivers. >> > > > >> > > > I'm still fail to understand how does that solve the issue for >> > > > new-style >> > > > bridges. How do we find the created drm_bridge_connector for them? >> > > >> > > Sigh, for some reason I was remembering that drm_bridge_connector was a >> > > bridge itself, which it isn't. My bad. But I guess it still applies. If >> > > we make drm_bridge_connector a bridge, then it works, doesn't it? >> > >> > I'd rather not. This would complicate other bridges using >> > drm_bridge_connector (e.g. ite-it6263, ti-sn65dsi86) >> >> I should have probably explained this a bit more. >> >> Currently each bridge has a link to the next bridge, obtained by lookup. >> All bridges are attached, then we create a connector. Adding drm_bridge >> into drm_bridge_connector would mean that the chain is mutated after all >> bridges are attached. The bridge that assumes that is the last bridge in >> the chain won't be the last one anymore. >> >> Next, we get an immediate issue with DP bridge chains. In some cases >> they rely on connector's fwnode being the fwnode of the last bridge so >> that displayport AltMode driver can deliver HPD events properly via a >> call to drm_connector_oob_hotplug_event(). Pushing one extra bridge >> would raise a question, which OF node should be specified in that bridge >> (and why), how will connector receive HPD calls, etc. >> >> Last, but not least, we have bridge drivers which create >> drm_bridge_connector on their own if DRM_BRIDGE_ATTACH_NO_CONNECTOR >> wasn't specified. Adding one extra bridge might surprise them. >> >> Generally I feel that while this looks appealing, it turns the framework >> upside down and makes it more fragile. >> >> > >> > > > > > But frankly speaking, I think it might be easier to pass down the >> > > > > > connector to the detect callback (as drm_connector_funcs.detect >> > > > > > already >> > > > > > gets the connecor) rather than making bridge drivers go through the >> > > > > > chain to get the value that is already present in the caller >> > > > > > function. >> > > > > > >> > > > > > (For some other usecases I'd totally agree with you, especially if >> > > > > > the >> > > > > > connector isn't already available on the caller side). >> > > > > >> > > > > Still, we've tried to converge to the same API for all entities, it >> > > > > feels like a step backward to me. >> > > > >> > > > I'd argue here a bit. The drm_connector interface has connector here. >> > > > drm_bridge is an extension/subpart of the drm_connector, so it would be >> > > > logical to extend that interface. >> > > >> > > The drm_connector interface has the connector because it's a connector. >> > > Just like CRTC atomic_check has a crtc, but you wouldn't pass the crtc >> > > pointer to drm_bridge atomic_check. >> >> I wouldn't pass CRTC pointer, because drm_bridge isn't a part of the >> CRTC. However it is clear that bridges reside between encoder and >> connector. As I wrote later, there are enough drm_bridge calls which >> recieve connector as an argument: get_modes(), edid_read(), all >> hdmi_audio_*() and dp_audio_*() calls. Not passing connector to those >> calls would make them much more complicated, especially in spite of >> Luca's work on hot-pluggable DRM bridges. > >The counterpoints being atomic_check, or >atomic_get_input/output_bus_fmts where having the crtc_state and >connector state just don't make sense. You can get them through >drm_atomic_state, and that's what we should be doing. > >We basically have two cases: the hooks that have access to >drm_atomic_state, and the ones that don't. The former should just get >drm_bridge and drm_atomic_state. They don't need anything more. > >For the latter, I guess you've convinced me and we could indeed pass the >connector. However, I'd still like the parameters to be consistent, and
Does this mean that we have the meaningful closure to the discussion and we can go on with this PATCH[0] >hdmi_audio_* have the reversed order compared to get_modes and >edid_read. Could you make sure the audio callbacks are using the same >order doing so? And for the parameters order, Dmitry, if you agree, I can send a patch to make them consistent. [0]https://lore.kernel.org/dri-devel/[email protected]/ [0] > >Thanks > >Maxime
