On Thu, Feb 27, 2025 at 12:31:43PM +0100, Luca Ceresoli wrote:
> On Thu, 27 Feb 2025 10:32:20 +0100
> Maxime Ripard <mripard@xxxxxxxxxx> wrote:
> > On Wed, Feb 26, 2025 at 03:28:13PM +0100, Luca Ceresoli wrote:
> > > On Tue, 11 Feb 2025 14:10:50 +0100
> > > Maxime Ripard <mripard@xxxxxxxxxx> wrote:
> > > > On Mon, Feb 10, 2025 at 06:12:52PM +0100, Luca Ceresoli wrote:
> > > > > On Fri, 7 Feb 2025 12:47:51 +0100
> > > > > Maxime Ripard <mripard@xxxxxxxxxx> wrote:
> > > > >
> > > > > > > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> > > > > > > index ad7ba444a13e5ecf16f996de3742e4ac67dc21f1..43cef0f6ccd36034f64ad2babfebea62db1d9e43 100644
> > > > > > > --- a/include/drm/drm_bridge.h
> > > > > > > +++ b/include/drm/drm_bridge.h
> > > > > > > @@ -31,6 +31,7 @@
> > > > > > > #include <drm/drm_encoder.h>
> > > > > > > #include <drm/drm_mode_object.h>
> > > > > > > #include <drm/drm_modes.h>
> > > > > > > +#include <drm/drm_print.h>
> > > > > > >
> > > > > > > struct device_node;
> > > > > > >
> > > > > > > @@ -863,6 +864,22 @@ struct drm_bridge {
> > > > > > > const struct drm_bridge_timings *timings;
> > > > > > > /** @funcs: control functions */
> > > > > > > const struct drm_bridge_funcs *funcs;
> > > > > > > +
> > > > > > > + /**
> > > > > > > + * @container_offset: Offset of this struct within the container
> > > > > > > + * struct embedding it. Used for refcounted bridges to free the
> > > > > > > + * embeddeing struct when the refcount drops to zero. Unused on
> > > > > > > + * legacy bridges.
> > > > > > > + */
> > > > > > > + size_t container_offset;
> > > > > >
> > > > > > This shouldn't be in there. You can create an intermediate structure and
> > > > > > store both pointers for the action to consume.
> > > > >
> > > > > You mean to store container_offset + refcount + is_refcounted?
> > > >
> > > > No, I meant for the private structure pointer and the drm_bridge
> > > > pointer. refcount should be in drm_bridge, and I think is_refcounted
> > > > should be dropped.
> > >
> > > Storing the container pointer instead of the offset is a good idea, it
> > > will allow to get rid of is_refcounted: drm_bridge_is_refcounted() can
> > > just return "container != NULL" instead of "bridge->is_refcounted". So
> > > far so good.
> >
> > Again, I don't think the whole is_refcounted thing is a good idea. Once
> > we have the right API, we should convert all bridges to the new
> > allocation and assume that they are refcounted.
>
> Ah, thanks for clarifying, now I understand the reason you'd remove
> is_refecounted while I didn't. In my plan it's for a transition phase
> where not all bridges are converted yet. I should have added a note
> about that, indeed.
>
> While I obviously think all bridges should be converted to dynamic
> lifetime, I'm not sure it can happen all in a single run, however.
> Converting bridges to refcounting is mostly easy, but before we should
> switch all bridge users to put the pointers they have, or the bridges
> will never be freed. But the users are more in number and harder to
> convert. However I still haven't tried a real conversion of all of
> them, so it I'm going to reconsider this after I'll have tried.
I mean, you're going to have that issue anyway. There's several calls
that can get a bridge to a consumer:
- of_drm_find_bridge
- drm_bridge_get_prev_bridge / drm_bridge_get_next_bridge
- drm_bridge_chain_get_first_bridge
- drm_for_each_bridge_in_chain
- devm_drm_of_get_bridge
- drmm_of_get_bridge
The last two are easy to deal with: just add an action that will put the
reference, and you're done. devm_drm_of_get_bridge() still is completely
broken and you should deprecate it as well, but the semantics are what
they are already so you're not going to break it more than it already
is.
For all the others though, you do change the semantics of the API, and
you will need to add drm_bridge_put or switch to drmm_of_get_bridge.
Also, is_refcounted doesn't really help. Your problem is that *callers*
might not give back the reference, but the way you set it is how you
allocate the provider.
Even assuming we're not doing the mass-conversion, how do you ensure
that you fixed all the callers that would use the bridge you just
converted?
> Generally speaking, would you be OK with having is_refcounted in a
> transition phase, or do you think we absolutely must convert all bridge
> drivers and users at once?
No, see above.
> > > I'm not sure about the intermediate struct you have in mind though.
> > >
> > > Do you mean:
> > >
> > > struct drm_bridge_pointers {
> > > struct drm_bridge *bridge;
> > > void *container;
> > > }
> > >
> > > ?
> >
> > Yes
> >
> > > If that's what you mean, should it be embedded in drm_struct or
> > > allocated separately?
> >
> > Separately, but still as part of the bridge allocation function.
> >
> > > If you mean to embed that struct in drm_bridge, then I the drm_bridge
> > > pointer inside the intermediate struct would be useless.
> > >
> > > If instead you mean to embed it in drm_struct: I'm not sure I see much
> ^^^^^^^^^^^^^^^^^^^^^^^^^
> For the records, I (obviously?) meant "allocated separately" here.
>
> > > benefit except maybe not exposing the container pointer to drm_bridge
> > > users, but I see a drawbacks: at the last put we need to find the
> > > container pointer to free from a struct kref pointer, which can work
> > > only if the container pointer is in the same struct as struct kref.
> >
> > Yeah, that's true. Storing the container pointer in drm_bridge makes
> > sense to solve this.
>
> OK, so when moving the container pointer to drm_bridge, the
> drm_bridge_pointers struct will be left with the drm_bridge pointer
> only:
>
> struct drm_bridge_pointer {
> struct drm_bridge *bridge;
> }
>
> So while it would work, I still don't see the added value. We'd have
> one more allocation, we'd need to free both structs at the same time
> (correct?) and drm_bridge_put_void() would have an extra indirection
> step:
>
> static void drm_bridge_put_void(void *data)
> {
> struct drm_bridge_pointer *bridge_pointer = (struct drm_bridge_pointers *)data;
> struct drm_bridge *bridge = bridge_pointer->bridge;
>
> drm_bridge_put(bridge);
> }
>
> Can you elaborate on the gain in having such struct, or point me to
> some code using the same pattern?
There's none, if we're going to have a single pointer it's not useful
indeed.
> > I'm still not sure why we need the container offset though: if we have a
> > bridge and container pointer, then the offset is bridge - container, so
> > there's no point in storing it, right?
>
> We need either the container_offset or the container pointer, not both.
> I had chosen the offset in v6, I'm going to convert to the pointer in
> v7.
Sounds good
Maxime
Attachment:
signature.asc
Description: PGP signature
