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

["Nautiyal, Ankit K" <ankit.k.nautiyal@intel.com>]

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 :(


Regards,

Ankit

>>>>
>>>> #define for_each_intel_crtc_in_pipe_mask_ordered(crtc, pipe_masks, order, i) \
>>>>           for ((i) = 0; \
>>>>                (i) < ARRAY_SIZE(order) && \
>>>>                ((crtc) = intel_crtc_for_pipe(joiner_primary_pipe(pipe_mask) + (order)[(i)]), 1); \
>>>>                (i)++) \
>>>>                   for_each_if((crtc) && (pipe_mask) & BIT((crtc)->pipe))
>>>>
>>>> would let us avoid that hardcoded pipe stuff, and everything is
>>>> just based on the relative order between the pipes. The same orders
>>>> also work for bigjoiner and non-joined cases (it just skips the pipes
>>>> that are't in the mask).
>>>>
>>>>
>>>> The alternative would be to just use the bigjoiner primary+secondary masks
>>>> and come up with a a way to iterate two bitmask in either forward or reverse
>>>> order. Hmm, I suppose one might just combine the bigjoiner primary and
>>>> secondary masks into one, with one of them shifted up to some high bits,
>>>> and then iterate the combined bitmask either forward or backward.
>>>>
>>>> Something like this should work:
>>>> #define for_each_crtc_in_masks(crtc, first_pipes, second_pipes, pipes, i) \
>>>>           for ((i) = 0, (pipes) = (second_pipes) << 16 | (first_pipes); \
>>>>                (i) < 32 && ((crtc) = intel_crtc_for_pipe((i) & 15), 1); \
>>>>                (i)++) \
>>>>                   for_each_if((crtc) && (pipes) & BIT(i))
>>>>
>>>> #define for_each_crtc_in_masks_reverse(crtc, first_pipes, second_pipes, pipes, i) \
>>>>           for ((i) = 31, (pipes) = (first_pipes) << 16 | (second_pipes); \
>>>>                (i) >= 0 && ((crtc) = intel_crtc_for_pipe((i) & 15), 1); \
>>>>                (i)--) \
>>>>                   for_each_if((crtc) && (pipes) & BIT(i))
>>>>
>>>> (could reduce the constants a bit given we don't have 16 pipes).
>>> This looks good to me. changed for 4 pipes, as below:
>>>
>>>
>>> #define for_each_crtc_in_masks(crtc, first_pipes, second_pipes, pipes, i) \
>>>           for ((i) = 0, (pipes) = (first_pipes) | ((second_pipes) << 4); \
>>>                (i) < 8 && ((crtc) = intel_crtc_for_pipe((i & 3)), 1); \
>> We could probably use a single internal define for the magic
>> number to avoid things going out of sync by accident.
>>
>> Hmm, maybe even define it as something like
>> #define _INTEL_MAX_PIPES_POT roundup_power_of_two(I915_MAX_PIPES)
>> ?
>>
>> O, I suppose we don't really need it to be POT, so we could
>> just replace the '&' with '%', and then we can just use
>> I915_MAX_PIPES directly.
>>
>>>                (i)++) \
>>>                   for_each_if((crtc) && (pipes) & BIT(i))
>>>
>>> #define for_each_crtc_in_masks_reverse(crtc, first_pipes, second_pipes,
>>> pipes, i) \
>>>           for ((i) = 7, (pipes) = (first_pipes) | ((second_pipes) << 4); \
>>>                (i) >= 0 && ((crtc) = intel_crtc_for_pipe((i & 3)), 1); \
>>>                (i)--) \
>>>                   for_each_if((crtc) && (pipes) & BIT(i))
>>>
>>> But, for non joiner case, when the bigjoiner_primary/secondary_pipes are
>>> 0 so pipes will be 0.
>> Hmm. I think we just need to make bigjoiner_primary_pipes()
>> return BIT(crtc->pipe) for the non-joiner cases.
>>
>> Maybe we should rename these to something like
>> _modeset_{primary,secondary}_pipes() so that people
>> don't get tempted to use them for anything else?
>>
>> And then we could hide all this into something like
>> #define for_each_pipe_crtc_modeset_disable(...) \
>> 	for_each_crtc_in_masks(..., _modeset_primary_pipes(), \
>> 			       _modeset_secondary_pipes(), ...)
>> #define for_each_pipe_crtc_modeset_enable(...) \
>> 	for_each_crtc_in_masks_reverse(..., _modeset_secondary_pipes(), \
>> 				      _modeset_primary_pipes(), ...)
> These last two macros you could already implement right
> now using the current code, and then we can replace them
> with the ultrajoiner capable stuff in another patch and
> not touch any of the actual modeset code anymore.
>