Re: [PATCH 16/19] drm/i915: Add new abstraction layer to handle pipe order for different joiners

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Sep 17, 2024 at 02:52:10PM +0530, Nautiyal, Ankit K wrote:
> 
> On 9/16/2024 8:36 PM, Ville Syrjälä wrote:
> > On Mon, Sep 16, 2024 at 05:54:12PM +0300, Ville Syrjälä wrote:
> >> On Mon, Sep 16, 2024 at 01:09:42PM +0530, Nautiyal, Ankit K wrote:
> >>> On 9/12/2024 4:08 AM, Ville Syrjälä wrote:
> >>>> On Wed, Sep 11, 2024 at 06:43:46PM +0530, Ankit Nautiyal wrote:
> >>>>> From: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> >>>>>
> >>>>> Ultrajoiner case requires special treatment where both reverse and
> >>>>> staight order iteration doesn't work(for instance disabling case requires
> >>>>> order to be: primary master, slaves, secondary master).
> >>>>>
> >>>>> Lets unify our approach by using not only pipe masks for iterating required
> >>>>> pipes based on joiner type used, but also using different "priority" arrays
> >>>>> for each of those.
> >>>>>
> >>>>> v2: Fix checkpatch warnings. (Ankit)
> >>>>>
> >>>>> Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> >>>>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> >>>>> ---
> >>>>>    drivers/gpu/drm/i915/display/intel_ddi.c     | 19 +++--
> >>>>>    drivers/gpu/drm/i915/display/intel_display.c | 83 ++++++++++++++++----
> >>>>>    drivers/gpu/drm/i915/display/intel_display.h |  7 ++
> >>>>>    drivers/gpu/drm/i915/display/intel_dp_mst.c  | 18 +++--
> >>>>>    4 files changed, 96 insertions(+), 31 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> >>>>> index 00fbe9f8c03a..2c064b6c6d01 100644
> >>>>> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> >>>>> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> >>>>> @@ -3116,10 +3116,11 @@ static void intel_ddi_post_disable_hdmi_or_sst(struct intel_atomic_state *state,
> >>>>>    					       const struct drm_connector_state *old_conn_state)
> >>>>>    {
> >>>>>    	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> >>>>> -	struct intel_crtc *pipe_crtc;
> >>>>> +	struct intel_crtc *pipe_crtc; enum pipe pipe;
> >>>>>    
> >>>>> -	for_each_intel_crtc_in_pipe_mask(&dev_priv->drm, pipe_crtc,
> >>>>> -					 intel_crtc_joined_pipe_mask(old_crtc_state)) {
> >>>>> +	for_each_intel_crtc_in_mask_priority(dev_priv, pipe_crtc, pipe,
> >>>>> +					     intel_crtc_joined_pipe_mask(old_crtc_state),
> >>>>> +					     intel_get_pipe_order_disable(old_crtc_state)) {
> >>>>>    		const struct intel_crtc_state *old_pipe_crtc_state =
> >>>>>    			intel_atomic_get_old_crtc_state(state, pipe_crtc);
> >>>>>    
> >>>>> @@ -3130,8 +3131,9 @@ static void intel_ddi_post_disable_hdmi_or_sst(struct intel_atomic_state *state,
> >>>>>    
> >>>>>    	intel_ddi_disable_transcoder_func(old_crtc_state);
> >>>>>    
> >>>>> -	for_each_intel_crtc_in_pipe_mask(&dev_priv->drm, pipe_crtc,
> >>>>> -					 intel_crtc_joined_pipe_mask(old_crtc_state)) {
> >>>>> +	for_each_intel_crtc_in_mask_priority(dev_priv, pipe_crtc, pipe,
> >>>>> +					     intel_crtc_joined_pipe_mask(old_crtc_state),
> >>>>> +					     intel_get_pipe_order_disable(old_crtc_state)) {
> >>>>>    		const struct intel_crtc_state *old_pipe_crtc_state =
> >>>>>    			intel_atomic_get_old_crtc_state(state, pipe_crtc);
> >>>>>    
> >>>>> @@ -3383,7 +3385,7 @@ static void intel_enable_ddi(struct intel_atomic_state *state,
> >>>>>    			     const struct drm_connector_state *conn_state)
> >>>>>    {
> >>>>>    	struct drm_i915_private *i915 = to_i915(encoder->base.dev);
> >>>>> -	struct intel_crtc *pipe_crtc;
> >>>>> +	struct intel_crtc *pipe_crtc; enum pipe pipe;
> >>>>>    
> >>>>>    	intel_ddi_enable_transcoder_func(encoder, crtc_state);
> >>>>>    
> >>>>> @@ -3394,8 +3396,9 @@ static void intel_enable_ddi(struct intel_atomic_state *state,
> >>>>>    
> >>>>>    	intel_ddi_wait_for_fec_status(encoder, crtc_state, true);
> >>>>>    
> >>>>> -	for_each_intel_crtc_in_pipe_mask_reverse(&i915->drm, pipe_crtc,
> >>>>> -						 intel_crtc_joined_pipe_mask(crtc_state)) {
> >>>>> +	for_each_intel_crtc_in_mask_priority(i915, pipe_crtc, pipe,
> >>>>> +					     intel_crtc_joined_pipe_mask(crtc_state),
> >>>>> +					     intel_get_pipe_order_enable(crtc_state)) {
> >>>>>    		const struct intel_crtc_state *pipe_crtc_state =
> >>>>>    			intel_atomic_get_new_crtc_state(state, pipe_crtc);
> >>>>>    
> >>>>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> >>>>> index db27850b2c36..27622d51a473 100644
> >>>>> --- a/drivers/gpu/drm/i915/display/intel_display.c
> >>>>> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> >>>>> @@ -1737,6 +1737,50 @@ static void hsw_configure_cpu_transcoder(const struct intel_crtc_state *crtc_sta
> >>>>>    	hsw_set_transconf(crtc_state);
> >>>>>    }
> >>>>>    
> >>>>> +static
> >>>>> +bool intel_crtc_is_bigjoiner(const struct intel_crtc_state *pipe_config)
> >>>>> +{
> >>>>> +	return hweight8(pipe_config->joiner_pipes) == 2;
> >>>>> +}
> >>>>> +
> >>>>> +const enum pipe *intel_get_pipe_order_enable(const struct intel_crtc_state *crtc_state)
> >>>>> +{
> >>>>> +	static const enum pipe ultrajoiner_pipe_order_enable[I915_MAX_PIPES] = {
> >>>>> +		PIPE_B, PIPE_D, PIPE_C, PIPE_A
> >>>>> +	};
> >>>>> +	static const enum pipe bigjoiner_pipe_order_enable[I915_MAX_PIPES] = {
> >>>>> +		PIPE_B, PIPE_A, PIPE_D, PIPE_C
> >>>>> +	};
> >>>>> +	static const enum pipe nojoiner_pipe_order_enable[I915_MAX_PIPES] = {
> >>>>> +		PIPE_A, PIPE_B, PIPE_C, PIPE_D
> >>>>> +	};
> >>>>> +
> >>>>> +	if (intel_crtc_is_ultrajoiner(crtc_state))
> >>>>> +		return ultrajoiner_pipe_order_enable;
> >>>>> +	else if (intel_crtc_is_bigjoiner(crtc_state))
> >>>>> +		return bigjoiner_pipe_order_enable;
> >>>>> +	return nojoiner_pipe_order_enable;
> >>>>> +}
> >>>>> +
> >>>>> +const enum pipe *intel_get_pipe_order_disable(const struct intel_crtc_state *crtc_state)
> >>>>> +{
> >>>>> +	static const enum pipe ultrajoiner_pipe_order_disable[I915_MAX_PIPES] = {
> >>>>> +		PIPE_A, PIPE_B, PIPE_D, PIPE_C
> >>>>> +	};
> >>>>> +	static const enum pipe bigjoiner_pipe_order_disable[I915_MAX_PIPES] = {
> >>>>> +		PIPE_A, PIPE_B, PIPE_C, PIPE_D
> >>>>> +	};
> >>>>> +	static const enum pipe nojoiner_pipe_order_disable[I915_MAX_PIPES] = {
> >>>>> +		PIPE_A, PIPE_B, PIPE_C, PIPE_D
> >>>>> +	};
> >>>>> +
> >>>>> +	if (intel_crtc_is_ultrajoiner(crtc_state))
> >>>>> +		return ultrajoiner_pipe_order_disable;
> >>>>> +	else if (intel_crtc_is_bigjoiner(crtc_state))
> >>>>> +		return bigjoiner_pipe_order_disable;
> >>>>> +	return nojoiner_pipe_order_disable;
> >>>> I don't think we should need all those diffrent order array. Technically
> >>>> one should do. Though having two might make sense.
> >>>>
> >>>> Another problem is the hardcoded pipes. If we eg. get hardware that
> >>>> would support ultrajoiner on pipes B-E in the future this would no
> >>>> longer  work.
> >>>>
> >>>>> +}
> >>>> <snip>
> >>>>> +#define for_each_intel_crtc_in_mask_priority(__dev_priv, intel_crtc, __p, __mask, __priolist) \
> >>>>> +	for_each_pipe(__dev_priv, __p) \
> >>>>> +		for_each_if((__mask) & BIT(__priolist[__p])) \
> >>>>> +			for_each_if(intel_crtc = intel_crtc_for_pipe(to_intel_display(&__dev_priv->drm), __priolist[__p]))
> >>>> I think something like:
> >>>>
> >>>> const u8 intel_pipe_order_enable[4] = {
> >>>>           3, 1, 2, 0,
> >>>> };
> >>>>
> >>>> const u8 intel_pipe_order_disable[4] = {
> >>>>           0, 2, 1, 3,
> >>>> };
> 
> 
> I just realized that as per bspec:54142 sequence for ultrajoiner doesnt 
> follow this.
> 
> its 1, 3, 2, 0 for enabling and 0, 1, 3, 2 for disabling :(

The bigjoiner secondary pipes are listed as one step in the sequence.
I believe that means that there is no ordering requirement between
them.

As for the funny C vs. BD order in the disable sequence, bspec:68847
has it the other way around as ACBD (a more proper mirror image of the
enable sequence). So either the hardware really has changed between
these two and the order is thus different, or one of them is simply
wrong, or the order doesn't really matter here either.

I suspect we can just always follow the more sensible order from
bspec:68847. Should probably ask for clarification though, and
test on actual hardware to confirm ofc.

-- 
Ville Syrjälä
Intel