On Thu, Oct 09, 2025 at 04:42:15PM +0200, Maxime Ripard wrote:
> On Wed, Oct 08, 2025 at 07:12:28PM +0300, Ville Syrjälä wrote:
> > On Wed, Oct 08, 2025 at 04:53:20PM +0200, Maxime Ripard wrote:
> > > On Wed, Oct 08, 2025 at 05:06:43PM +0300, Ville Syrjälä wrote:
> > > > On Wed, Oct 08, 2025 at 02:04:03PM +0200, Maxime Ripard wrote:
> > > > > The DP MST implementation relies on a drm_private_obj, that is
> > > > > initialized by allocating and initializing a state, and then passing
> > > > > it
> > > > > to drm_private_obj_init.
> > > > >
> > > > > Since we're gradually moving away from that pattern to the more
> > > > > established one relying on a reset implementation, let's migrate this
> > > > > instance to the new pattern.
> > > > >
> > > > > Signed-off-by: Maxime Ripard <mrip...@kernel.org>
> > > > > ---
> > > > > drivers/gpu/drm/display/drm_dp_mst_topology.c | 39
> > > > > ++++++++++++++++++---------
> > > > > 1 file changed, 26 insertions(+), 13 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > > > > b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > > > > index
> > > > > 64e5c176d5cce9df9314f77a0b4c97662c30c070..255fbdcea9f0b6376d15439e3da1dc02be472a20
> > > > > 100644
> > > > > --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > > > > +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > > > > @@ -5181,10 +5181,34 @@ static void drm_dp_mst_destroy_state(struct
> > > > > drm_private_obj *obj,
> > > > >
> > > > > kfree(mst_state->commit_deps);
> > > > > kfree(mst_state);
> > > > > }
> > > > >
> > > > > +static void drm_dp_mst_reset(struct drm_private_obj *obj)
> > > > > +{
> > > > > + struct drm_dp_mst_topology_mgr *mgr =
> > > > > + to_dp_mst_topology_mgr(obj);
> > > > > + struct drm_dp_mst_topology_state *mst_state;
> > > > > +
> > > > > + if (obj->state) {
> > > > > + drm_dp_mst_destroy_state(obj, obj->state);
> > > > > + obj->state = NULL;
> > > >
> > > > I'm not a big fan of this "free+reallocate without any way to handle
> > > > failures" pattern.
> > > >
> > > > Currently i915 does not do any of this, and so there are no unhandled
> > > > points of failure. But the mst and tunneling changes here force it on
> > > > i915 as well.
> > >
> > > A very theoretical point of failure. If I'm counting right,
> > > drm_dp_mst_topology_state takes 72 bytes. Any GFP_KERNEL allocation
> > > of less than 8 pages cannot fail.
> >
> > Until you start to inject failures, or you blow up the size of the
> > state. You wouldn't think to check for potential NULL pointer
> > dereferences before doing that, nor should you have to.
> >
> > I'd rather not leave potential footguns lying around intentionally.
>
> I'm not sure it's reasonable to expect a structure to grow by three
> orders of magnitude, but we can add static build checks on the size of
> the structure if it makes you more comfortable.
I'm not really talking about any specific state structures here,
but in general. This thing should be robust even if someone needs
to add a private object with a ginormouse state.
>
> > > So, sure, it's less satisfying to look at, but that's about it. It's
> > > just as safe and reliable.
> > >
> > > > I think for the common things it would be nicer to just implement
> > > > the reset as just that "a reset of the current state", and leave
> > > > the "let's play fast and loose with kmalloc() failures" to the
> > > > drivers that want it.
> > >
> > > I'm sorry, but I have no idea what you mean by that. It's using the
> > > exact same pattern we've been using since forever for every driver
> > > (except, apparently, i915).
> >
> > The reset() stuff was added specifically to deal with drivers
> > that didn't have readout. i915 has always had full readout and
> > never needed it.
> >
> > > What would you like to change to that pattern to accomodate i915
> > > requirements?
> >
> > I think the reset should be ideally done without reallocation,
> > or perhaps just don't implement reset for the things that don't
> > need it (mst and tunneling in this case).
>
> I'll be leaving aside i915 because it's quite far from actually using
> all that infrastructure. From what I understand, your main concern is
> that the drm_private_obj would be reset during a suspend, and the DP
> objs should actually persist.
With my i915 hat on, yes. That would be a functional change
you're introducing here that may or may not be good.
But in general I think simply getting rid of that unchecked
kmalloc() would be a good idea. There's no real reason to keep it
other than being lazy and not implementing the state reset without
it.
>
> Thomas has been floating the idea recently to create a new helper to
> supersede reset which would only reset the state itself, and not the
> hardware. If we're doing that split, I guess it would mean that we would
> need a new callback to allocate the initial state, since reset does both.
>
> Would you feel better about creating an atomic_create_state hook or
> something?
I suppose you could add a hook for it. Sort of the initial version
of .duplicate_state() I guess.
Doesn't really need a hook though when you could just either just:
- pass the state to the init (which I guess was exactly what private
objs did, but other object types didn't do)
- add a new function that just sets the obj->state pointer, if you
want to hide that from the drivers a bit more
- just set the obj->state pointer by hand (which we do eg. in i915
intel_crtc_alloc())
Or did you want to use this also after the object init somewhere?
.duplicate_state() is often implemented with a memdup() so wouldn't
really help there at least.
--
Ville Syrjälä
Intel
