Re: [Intel-gfx] [PATCH 05/11] drm/i915: Add support for disabling any CRTCs during HW readout/sanitization

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

On Fri, Apr 28, 2023 at 08:47:23PM +0300, Imre Deak wrote:
> On Fri, Apr 28, 2023 at 08:22:54PM +0300, Imre Deak wrote:
> > On Fri, Apr 28, 2023 at 05:02:35PM +0300, Ville Syrjälä wrote:
> > > On Wed, Apr 26, 2023 at 07:52:59PM +0300, Imre Deak wrote:
> > > > [...] 
> > > > +static u32 get_transcoder_pipes(struct drm_i915_private *i915, u32 transcoder_mask)
> > > > +{
> > > > +	struct intel_crtc *slave_crtc;
> > > > +	u32 pipes = 0;
> > > > +
> > > > +	for_each_intel_crtc(&i915->drm, slave_crtc) {
> > > > +		struct intel_crtc_state *slave_crtc_state =
> > > > +			to_intel_crtc_state(slave_crtc->base.state);
> > > > +
> > > > +		if (slave_crtc_state->cpu_transcoder == INVALID_TRANSCODER)
> > > > +			continue;
> > > > +
> > > > +		if (transcoder_mask & BIT(slave_crtc_state->cpu_transcoder))
> > > > +			pipes |= BIT(slave_crtc->pipe);
> > > > +	}
> > > > +
> > > > +	return pipes;
> > > > +}
> > > > +
> > > > +static u32 get_bigjoiner_slave_pipes(struct drm_i915_private *i915, u32 master_pipes)
> > > > +{
> > > > +	struct intel_crtc *master_crtc;
> > > > +	u32 pipes = 0;
> > > > +
> > > > +	for_each_intel_crtc_in_pipe_mask(&i915->drm, master_crtc, master_pipes) {
> > > > +		struct intel_crtc_state *master_crtc_state =
> > > > +			to_intel_crtc_state(master_crtc->base.state);
> > > > +
> > > > +		pipes |= intel_crtc_bigjoiner_slave_pipes(master_crtc_state);
> > > > +	}
> > > > +
> > > > +	return pipes;
> > > > +}
> > > > +
> > > > +static void kill_bigjoiner_slave_noatomic(struct intel_crtc *master_crtc)
> > > > +{
> > > > +	struct drm_i915_private *i915 = to_i915(master_crtc->base.dev);
> > > > +	struct intel_crtc_state *master_crtc_state =
> > > > +		to_intel_crtc_state(master_crtc->base.state);
> > > > +	struct intel_crtc *slave_crtc;
> > > > +
> > > > +	for_each_intel_crtc_in_pipe_mask(&i915->drm, slave_crtc,
> > > > +					 intel_crtc_bigjoiner_slave_pipes(master_crtc_state)) {
> > > > +		struct intel_crtc_state *slave_crtc_state =
> > > > +			to_intel_crtc_state(slave_crtc->base.state);
> > > > +
> > > > +		slave_crtc_state->bigjoiner_pipes = 0;
> > > > +	}
> > > > +
> > > > +	master_crtc_state->bigjoiner_pipes = 0;
> > > > +}
> > > > +
> > > > +static void disable_crtc_with_slaves(struct intel_crtc *crtc,
> > > > +				     struct drm_modeset_acquire_ctx *ctx)
> > > > +{
> > > > +	struct drm_i915_private *i915 = to_i915(crtc->base.dev);
> > > > +	struct intel_crtc_state *crtc_state = to_intel_crtc_state(crtc->base.state);
> > > > +	u32 bigjoiner_masters = BIT(crtc->pipe) |
> > > > +				get_transcoder_pipes(i915, crtc_state->sync_mode_slaves_mask);
> > > 
> > > The resulting bitmask would seem to also include the bigjoiner slaves.
> > 
> > Hrm, doesn't sync_mode_slaves_mask contain only the transcoder bits for
> > the port-synced master CRTCs? I assumed that those port-synced master
> > CRTCs would point to their slave pipes via their
> > crtc_state->bigjoiner_pipes mask.
> 
> Ah, understood the issue, the port_synced slave CRTCs need to get
> disabled before the port_sync master.
> 
> So I think the following gives the correct order:
> 
> 	u32 portsync_slaves = get_transcoder_pipes(i915,
> 						   crtc_state->sync_mode_slaves_mask);
> 	u32 bigjoiner_slaves = get_bigjoiner_slave_pipes(i915,
> 							 BIT(crtc->pipe) |
> 							 portsync_slaves);
> 
> 	for_each_intel_crtc_in_pipe_mask(&i915->drm, temp_crtc, bigjoiner_slaves)
> 		intel_crtc_disable_noatomic(temp_crtc, ctx);
> 
> 	for_each_intel_crtc_in_pipe_mask(&i915->drm, temp_crtc, portsync_slaves) {
> 		intel_crtc_disable_noatomic(temp_crtc, ctx);
> 		kill_bigjoiner_slave_noatomic(temp_crtc);
> 	}
> 
> 	intel_crtc_disable_noatomic(crtc);
> 	kill_bigjoiner_slave_noatomic(crtc);
> 
> ?

And that a bigjoiner slave pipe's transcoder points to the master
transcoder :/ so those slave pipes need to be filtered out in
get_transcoder_pipes().

> > > > +	u32 bigjoiner_slaves = get_bigjoiner_slave_pipes(i915, bigjoiner_masters);
> > > > +	struct intel_crtc *temp_crtc;
> > > > +
> > > > +	for_each_intel_crtc_in_pipe_mask(&i915->drm, temp_crtc, bigjoiner_slaves)
> > > > +		intel_crtc_disable_noatomic(temp_crtc, ctx);
> > > > +
> > > > +	for_each_intel_crtc_in_pipe_mask(&i915->drm, temp_crtc, bigjoiner_masters) {
> > > > +		intel_crtc_disable_noatomic(temp_crtc, ctx);
> > > > +		kill_bigjoiner_slave_noatomic(temp_crtc);
> > > > +	}
> > > > +}
> > > > +
> > > >  static void intel_sanitize_crtc(struct intel_crtc *crtc,
> > > >  				struct drm_modeset_acquire_ctx *ctx)
> > > >  {
> > > > @@ -299,10 +404,11 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc,
> > > >  	/*
> > > >  	 * Adjust the state of the output pipe according to whether we have
> > > >  	 * active connectors/encoders.
> > > > +	 * TODO: Add support for MST
> > > >  	 */
> > > >  	if (crtc_state->hw.active && !intel_crtc_has_encoders(crtc) &&
> > > >  	    !intel_crtc_is_bigjoiner_slave(crtc_state))
> > > > -		intel_crtc_disable_noatomic(crtc, ctx);
> > > > +		disable_crtc_with_slaves(crtc, ctx);
> > > 
> > > I'd like to keep the _noatomic() in the name.
> > 
> > Ok, will change this.
> > 
> > > 
> > > >  }
> > > >  
> > > >  static bool has_bogus_dpll_config(const struct intel_crtc_state *crtc_state)
> > > > -- 
> > > > 2.37.2
> > > 
> > > -- 
> > > Ville Syrjälä
> > > Intel