Re: [PATCH 12/19] drm/i915/dp: Add DP tunnel atomic state and check BW limit

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

On Mon, Feb 05, 2024 at 06:11:00PM +0200, Ville Syrjälä wrote:
> On Tue, Jan 23, 2024 at 12:28:43PM +0200, Imre Deak wrote:
> > Add the atomic state during a modeset required to enable the DP tunnel
> > BW allocation mode on links where such a tunnel was detected.
> > 
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_atomic.c  |  8 ++++++++
> >  drivers/gpu/drm/i915/display/intel_display.c | 19 +++++++++++++++++++
> >  drivers/gpu/drm/i915/display/intel_link_bw.c |  5 +++++
> >  3 files changed, 32 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c b/drivers/gpu/drm/i915/display/intel_atomic.c
> > index 96ab37e158995..4236740ede9ed 100644
> > --- a/drivers/gpu/drm/i915/display/intel_atomic.c
> > +++ b/drivers/gpu/drm/i915/display/intel_atomic.c
> > @@ -260,6 +260,10 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc)
> >  	if (crtc_state->post_csc_lut)
> >  		drm_property_blob_get(crtc_state->post_csc_lut);
> >  
> > +	if (crtc_state->dp_tunnel_ref.tunnel)
> > +		drm_dp_tunnel_ref_get(old_crtc_state->dp_tunnel_ref.tunnel,
> 
> I'd probably s/old_crtc_state/crtc_state/ here. Same pointer, but
> looks out of place given everyone else just operates on 'crtc_state' 
> here.

Ok, will change that.

> > +					&crtc_state->dp_tunnel_ref);
> 
> Shame we have to have this ref wrapper. But I guess no clean
> way to have a magic tracked pointer type that works like a
> normal pointer in C...

I suppose returning a pointer to a kmalloced drm_dp_tunnel_ref from
drm_tunnel_get() and freeing this in drm_tunnel_put() would be one way,
but that imo defeats the purpose of the tracker information being valid
even after put() (so that ref_tracker can print information about where
a particular reference was already dropped).

> 
> > +
> >  	crtc_state->update_pipe = false;
> >  	crtc_state->update_m_n = false;
> >  	crtc_state->update_lrr = false;
> > @@ -311,6 +315,8 @@ intel_crtc_destroy_state(struct drm_crtc *crtc,
> >  
> >  	__drm_atomic_helper_crtc_destroy_state(&crtc_state->uapi);
> >  	intel_crtc_free_hw_state(crtc_state);
> > +	if (crtc_state->dp_tunnel_ref.tunnel)
> > +		drm_dp_tunnel_ref_put(&crtc_state->dp_tunnel_ref);
> >  	kfree(crtc_state);
> >  }
> >  
> > @@ -346,6 +352,8 @@ void intel_atomic_state_clear(struct drm_atomic_state *s)
> >  	/* state->internal not reset on purpose */
> >  
> >  	state->dpll_set = state->modeset = false;
> > +
> > +	intel_dp_tunnel_atomic_cleanup_inherited_state(state);
> 
> This seems to be in the wrong patch?

Yes, I guess a more logical place is in

[PATCH 14/19] drm/i915/dp: Compute DP tunel BW during encoder state computation

where the state is added, will move it there.

> 
> >  }
> >  
> >  struct intel_crtc_state *
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index b9f985a5e705b..46b27a32c8640 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -33,6 +33,7 @@
> >  #include <linux/string_helpers.h>
> >  
> >  #include <drm/display/drm_dp_helper.h>
> > +#include <drm/display/drm_dp_tunnel.h>
> >  #include <drm/drm_atomic.h>
> >  #include <drm/drm_atomic_helper.h>
> >  #include <drm/drm_atomic_uapi.h>
> > @@ -73,6 +74,7 @@
> >  #include "intel_dp.h"
> >  #include "intel_dp_link_training.h"
> >  #include "intel_dp_mst.h"
> > +#include "intel_dp_tunnel.h"
> >  #include "intel_dpll.h"
> >  #include "intel_dpll_mgr.h"
> >  #include "intel_dpt.h"
> > @@ -4490,6 +4492,8 @@ copy_bigjoiner_crtc_state_modeset(struct intel_atomic_state *state,
> >  	saved_state->crc_enabled = slave_crtc_state->crc_enabled;
> >  
> >  	intel_crtc_free_hw_state(slave_crtc_state);
> > +	if (slave_crtc_state->dp_tunnel_ref.tunnel)
> > +		drm_dp_tunnel_ref_put(&slave_crtc_state->dp_tunnel_ref);
> >  	memcpy(slave_crtc_state, saved_state, sizeof(*slave_crtc_state));
> >  	kfree(saved_state);
> >  
> > @@ -4505,6 +4509,10 @@ copy_bigjoiner_crtc_state_modeset(struct intel_atomic_state *state,
> >  		      &master_crtc_state->hw.adjusted_mode);
> >  	slave_crtc_state->hw.scaling_filter = master_crtc_state->hw.scaling_filter;
> >  
> > +	if (master_crtc_state->dp_tunnel_ref.tunnel)
> > +		drm_dp_tunnel_ref_get(master_crtc_state->dp_tunnel_ref.tunnel,
> > +					&slave_crtc_state->dp_tunnel_ref);
> > +
> >  	copy_bigjoiner_crtc_state_nomodeset(state, slave_crtc);
> >  
> >  	slave_crtc_state->uapi.mode_changed = master_crtc_state->uapi.mode_changed;
> > @@ -4533,6 +4541,13 @@ intel_crtc_prepare_cleared_state(struct intel_atomic_state *state,
> >  	/* free the old crtc_state->hw members */
> >  	intel_crtc_free_hw_state(crtc_state);
> >  
> > +	if (crtc_state->dp_tunnel_ref.tunnel) {
> > +		drm_dp_tunnel_atomic_set_stream_bw(&state->base,
> > +						   crtc_state->dp_tunnel_ref.tunnel,
> > +						   crtc->pipe, 0);
> > +		drm_dp_tunnel_ref_put(&crtc_state->dp_tunnel_ref);
> > +	}
> > +
> >  	/* FIXME: before the switch to atomic started, a new pipe_config was
> >  	 * kzalloc'd. Code that depends on any field being zero should be
> >  	 * fixed, so that the crtc_state can be safely duplicated. For now,
> > @@ -5374,6 +5389,10 @@ static int intel_modeset_pipe(struct intel_atomic_state *state,
> >  	if (ret)
> >  		return ret;
> >  
> > +	ret = intel_dp_tunnel_atomic_add_state_for_crtc(state, crtc);
> > +	if (ret)
> > +		return ret;
> > +
> >  	ret = intel_dp_mst_add_topology_state_for_crtc(state, crtc);
> >  	if (ret)
> >  		return ret;
> > diff --git a/drivers/gpu/drm/i915/display/intel_link_bw.c b/drivers/gpu/drm/i915/display/intel_link_bw.c
> > index 9c6d35a405a18..5b539ba996ddf 100644
> > --- a/drivers/gpu/drm/i915/display/intel_link_bw.c
> > +++ b/drivers/gpu/drm/i915/display/intel_link_bw.c
> > @@ -8,6 +8,7 @@
> >  #include "intel_atomic.h"
> >  #include "intel_display_types.h"
> >  #include "intel_dp_mst.h"
> > +#include "intel_dp_tunnel.h"
> >  #include "intel_fdi.h"
> >  #include "intel_link_bw.h"
> >  
> > @@ -149,6 +150,10 @@ static int check_all_link_config(struct intel_atomic_state *state,
> >  	if (ret)
> >  		return ret;
> >  
> > +	ret = intel_dp_tunnel_atomic_check_link(state, limits);
> > +	if (ret)
> > +		return ret;
> > +
> >  	ret = intel_fdi_atomic_check_link(state, limits);
> >  	if (ret)
> >  		return ret;
> > -- 
> > 2.39.2
> 
> -- 
> Ville Syrjälä
> Intel