diff mbox series

[10/13] drm/i915/vrr: Avoid sending PUSH when VRR TG is used with Fixed refresh rate

Message ID 20240902080635.2946858-11-ankit.k.nautiyal@intel.com (mailing list archive)
State New, archived
Headers show
Series Use VRR timing generator for fixed refresh rate modes | expand

Commit Message

Nautiyal, Ankit K Sept. 2, 2024, 8:06 a.m. UTC 
As per Bspec:68925: Push enable must be set if not configuring for a
fixed refresh rate (i.e Vmin == Flipline == Vmax is not true).

Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
Reviewed-by: Mitul Golani <mitulkumar.ajitkumar.golani@intel.com>
---
 drivers/gpu/drm/i915/display/intel_vrr.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

Comments

Ville Syrjala Sept. 3, 2024, 1:02 p.m. UTC | #1 
On Mon, Sep 02, 2024 at 01:36:31PM +0530, Ankit Nautiyal wrote:
> As per Bspec:68925: Push enable must be set if not configuring for a
> fixed refresh rate (i.e Vmin == Flipline == Vmax is not true).
> 
> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> Reviewed-by: Mitul Golani <mitulkumar.ajitkumar.golani@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_vrr.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c b/drivers/gpu/drm/i915/display/intel_vrr.c
> index 03af50b9f9eb..5e947465c6e0 100644
> --- a/drivers/gpu/drm/i915/display/intel_vrr.c
> +++ b/drivers/gpu/drm/i915/display/intel_vrr.c
> @@ -313,7 +313,7 @@ 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 (!crtc_state->vrr.enable)
> +	if (!crtc_state->vrr.enable || crtc_state->vrr.fixed_rr)

Hmm. I wonder if we should keep vrr.enable meaning VRR actually
enabled... Maybe not as that would complicate the readout/check
a bit too much perhaps.

But we could have some kind of helper for these "do we need to use
push?" checks (eg. intel_vrr_use_push()).

>  		return;
>  
>  	intel_de_write(display, TRANS_PUSH(display, cpu_transcoder),
> @@ -325,7 +325,7 @@ bool intel_vrr_is_push_sent(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 (!crtc_state->vrr.enable)
> +	if (!crtc_state->vrr.enable || crtc_state->vrr.fixed_rr)
>  		return false;
>  
>  	return intel_de_read(display, TRANS_PUSH(display, cpu_transcoder)) & TRANS_PUSH_SEND;
> @@ -339,8 +339,9 @@ void intel_vrr_enable(const struct intel_crtc_state *crtc_state)
>  	if (!crtc_state->vrr.enable)
>  		return;
>  
> -	intel_de_write(display, TRANS_PUSH(display, cpu_transcoder),
> -		       TRANS_PUSH_EN);
> +	if (!crtc_state->vrr.fixed_rr)
> +		intel_de_write(display, TRANS_PUSH(display, cpu_transcoder),
> +			       TRANS_PUSH_EN);
>  
>  	if (HAS_AS_SDP(display))
>  		intel_de_write(display,
> -- 
> 2.45.2
Nautiyal, Ankit K Sept. 4, 2024, 12:57 p.m. UTC | #2 
On 9/3/2024 6:32 PM, Ville Syrjälä wrote:
> On Mon, Sep 02, 2024 at 01:36:31PM +0530, Ankit Nautiyal wrote:
>> As per Bspec:68925: Push enable must be set if not configuring for a
>> fixed refresh rate (i.e Vmin == Flipline == Vmax is not true).
>>
>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
>> Reviewed-by: Mitul Golani <mitulkumar.ajitkumar.golani@intel.com>
>> ---
>>   drivers/gpu/drm/i915/display/intel_vrr.c | 9 +++++----
>>   1 file changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c b/drivers/gpu/drm/i915/display/intel_vrr.c
>> index 03af50b9f9eb..5e947465c6e0 100644
>> --- a/drivers/gpu/drm/i915/display/intel_vrr.c
>> +++ b/drivers/gpu/drm/i915/display/intel_vrr.c
>> @@ -313,7 +313,7 @@ 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 (!crtc_state->vrr.enable)
>> +	if (!crtc_state->vrr.enable || crtc_state->vrr.fixed_rr)
> Hmm. I wonder if we should keep vrr.enable meaning VRR actually
> enabled... Maybe not as that would complicate the readout/check
> a bit too much perhaps.

I agree this perhaps need to be streamline.

Currently with this patch series vrr.enable
represents whether VRR timing generator is enabled or not.

When VRR timing generator is enabled we can be in one of 3 modes:
1. Acutal variable refresh rate modes    (vrr.enable = true; 
vrr.fixed_rr = false)
2. Fixed refresh rate modes                (vrr.enable = true; 
vrr.fixed_rr = true)
3. Content matched refresh rate mode.    (vrr.enable = true; vrr.cmrr = 
true)

I am open regarding any modification to this or have a new scheme 
(perhaps an enum vrr.mode for the above 3)


>
> But we could have some kind of helper for these "do we need to use
> push?" checks (eg. intel_vrr_use_push()).

Makes sense. I will add these helpers.


Regards,

Ankit

>>   		return;
>>   
>>   	intel_de_write(display, TRANS_PUSH(display, cpu_transcoder),
>> @@ -325,7 +325,7 @@ bool intel_vrr_is_push_sent(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 (!crtc_state->vrr.enable)
>> +	if (!crtc_state->vrr.enable || crtc_state->vrr.fixed_rr)
>>   		return false;
>>   
>>   	return intel_de_read(display, TRANS_PUSH(display, cpu_transcoder)) & TRANS_PUSH_SEND;
>> @@ -339,8 +339,9 @@ void intel_vrr_enable(const struct intel_crtc_state *crtc_state)
>>   	if (!crtc_state->vrr.enable)
>>   		return;
>>   
>> -	intel_de_write(display, TRANS_PUSH(display, cpu_transcoder),
>> -		       TRANS_PUSH_EN);
>> +	if (!crtc_state->vrr.fixed_rr)
>> +		intel_de_write(display, TRANS_PUSH(display, cpu_transcoder),
>> +			       TRANS_PUSH_EN);
>>   
>>   	if (HAS_AS_SDP(display))
>>   		intel_de_write(display,
>> -- 
>> 2.45.2
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c b/drivers/gpu/drm/i915/display/intel_vrr.c
index 03af50b9f9eb..5e947465c6e0 100644
--- a/drivers/gpu/drm/i915/display/intel_vrr.c
+++ b/drivers/gpu/drm/i915/display/intel_vrr.c
@@ -313,7 +313,7 @@  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 (!crtc_state->vrr.enable)
+	if (!crtc_state->vrr.enable || crtc_state->vrr.fixed_rr)
 		return;
 
 	intel_de_write(display, TRANS_PUSH(display, cpu_transcoder),
@@ -325,7 +325,7 @@  bool intel_vrr_is_push_sent(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 (!crtc_state->vrr.enable)
+	if (!crtc_state->vrr.enable || crtc_state->vrr.fixed_rr)
 		return false;
 
 	return intel_de_read(display, TRANS_PUSH(display, cpu_transcoder)) & TRANS_PUSH_SEND;
@@ -339,8 +339,9 @@  void intel_vrr_enable(const struct intel_crtc_state *crtc_state)
 	if (!crtc_state->vrr.enable)
 		return;
 
-	intel_de_write(display, TRANS_PUSH(display, cpu_transcoder),
-		       TRANS_PUSH_EN);
+	if (!crtc_state->vrr.fixed_rr)
+		intel_de_write(display, TRANS_PUSH(display, cpu_transcoder),
+			       TRANS_PUSH_EN);
 
 	if (HAS_AS_SDP(display))
 		intel_de_write(display,