Re: [PATCH 11/13] drm/i915/vrr: Handle joiner with vrr

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

On 9/3/2024 6:34 PM, Ville Syrjälä wrote:
> On Mon, Sep 02, 2024 at 01:36:32PM +0530, Ankit Nautiyal wrote:
>> Do not program transcoder registers for VRR for the secondary pipe of
>> the joiner. Remove check to skip VRR for joiner case.
>>
>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
>> ---
>>   drivers/gpu/drm/i915/display/intel_vrr.c | 19 ++++++++++++-------
>>   1 file changed, 12 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c b/drivers/gpu/drm/i915/display/intel_vrr.c
>> index 5e947465c6e0..e01d4b4b8fa7 100644
>> --- a/drivers/gpu/drm/i915/display/intel_vrr.c
>> +++ b/drivers/gpu/drm/i915/display/intel_vrr.c
>> @@ -169,13 +169,6 @@ intel_vrr_compute_config(struct intel_crtc_state *crtc_state,
>>   	const struct drm_display_info *info = &connector->base.display_info;
>>   	int vmin, vmax;
>>   
>> -	/*
>> -	 * FIXME all joined pipes share the same transcoder.
>> -	 * Need to account for that during VRR toggle/push/etc.
>> -	 */
>> -	if (crtc_state->joiner_pipes)
>> -		return;
> There's more to this than just sprinkling the secondary checks.
> Namely, we need to make sure the timing changes happen in the
> correct spot in the sequence for both primary and secondary pipes.

Yeah, I was not very confident about this.

Whether calling intel_vrr_set_transcoder_timings, intel_vrr_enable, 
intel_vrr_send_push for each joined pipe in reverse order will work.
I think currently they are part of intel_pre_update_crtc, 
intel_update_crtc and intel_pipe_update_end respectively, but called for 
each pipe not in reverse order?

I am still not very sure though.

Regards,

Ankit

>
>> -
>>   	if (adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE)
>>   		return;
>>   
>> @@ -272,6 +265,9 @@ void intel_vrr_set_transcoder_timings(const struct intel_crtc_state *crtc_state)
>>   	struct intel_display *display = to_intel_display(crtc_state);
>>   	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
>>   
>> +	if (intel_crtc_is_joiner_secondary(crtc_state))
>> +		return;
>> +
>>   	/*
>>   	 * This bit seems to have two meanings depending on the platform:
>>   	 * TGL: generate VRR "safe window" for DSB vblank waits
>> @@ -313,6 +309,9 @@ void intel_vrr_send_push(const struct intel_crtc_state *crtc_state)
>>   	struct intel_display *display = to_intel_display(crtc_state);
>>   	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
>>   
>> +	if (intel_crtc_is_joiner_secondary(crtc_state))
>> +		return;
>> +
>>   	if (!crtc_state->vrr.enable || crtc_state->vrr.fixed_rr)
>>   		return;
>>   
>> @@ -336,6 +335,9 @@ void intel_vrr_enable(const struct intel_crtc_state *crtc_state)
>>   	struct intel_display *display = to_intel_display(crtc_state);
>>   	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
>>   
>> +	if (intel_crtc_is_joiner_secondary(crtc_state))
>> +		return;
>> +
>>   	if (!crtc_state->vrr.enable)
>>   		return;
>>   
>> @@ -364,6 +366,9 @@ void intel_vrr_disable(const struct intel_crtc_state *old_crtc_state)
>>   	struct intel_display *display = to_intel_display(old_crtc_state);
>>   	enum transcoder cpu_transcoder = old_crtc_state->cpu_transcoder;
>>   
>> +	if (intel_crtc_is_joiner_secondary(old_crtc_state))
>> +		return;
>> +
>>   	if (!old_crtc_state->vrr.enable)
>>   		return;
>>   
>> -- 
>> 2.45.2