Re: [PATCH 05/16] drm/dp_mst: Switch private_obj initialization to reset

[Imre Deak <imre.deak@intel.com>]

On Wed, Oct 15, 2025 at 01:35:47AM +0300, Dmitry Baryshkov wrote:
> On Wed, Oct 08, 2025 at 07:24:15PM +0300, Imre Deak 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 <mripard@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.
> > > 
> > > 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.
> > > 
> > > That said I haven't even thought through whether this mst and
> > > tunneling state reset might have some undesired side effects
> > > since we previously did none of it. I suspect it should be fine,
> > > but at least the mst code does some questionable things with
> > > the state so not 100% sure.
> > > 
> > > Imre, do you recall if we might somehow depend on preserving
> > > the state across drm_mode_config_reset()?
> > 
> > Yes, the stream payload info in the MST state and the stream BW info in
> > the tunnel state is computed for the active streams (i.e. CRTCs) before
> > suspend and this info is reused after resume. These active streams must
> > be restored to their pre-suspend state after resume, which will need the
> > above payload/BW info.
> > 
> > The restore should happen without recomputing the state for CRTCs, so
> > the payload/BW info should be also preserved across suspend/resume.
> > 
> > crtc/plane/connector objects which have the reset semantic added now in
> > this patch for private objects do preserve their state across
> > suspend/resume, see drm_atomic_helper_duplicate_state() and
> > drm_atomic_helper_commit_duplicated_state().
> 
> Doesn't this mean that we should implement handling of private objects
> in those functions too? E.g. we track resource allocation in the private
> object. It should also be restored to exactly the same state as it was
> before suspend.

Yes, I think so. Ville mentioned that someone has sent already patches
to that extent. Note that, things also do work atm without the
save/restore for the mst, tunnel private objects, at least for i915,
since everything does get recomputed during resume: the effectively
reset/zeroed private object state (due to the last disabling modeset
during suspend) will get recomputed from other states (crtc state, sink
capabilities) during resume. However this dependency on the other states
should be removed eventually and then saving/restoring the private obj
state would be required.

> -- 
> With best wishes
> Dmitry