Re: [RFC V3 2/2] drm/i915/vrr: Set drm crtc vrr_enabled property

2022-06-01 Thread Modem, Bhanuprakash

On Tue-31-05-2022 10:44 pm, Navare, Manasi wrote:

On Tue, May 17, 2022 at 12:56:36PM +0530, Bhanuprakash Modem wrote:

This function sets the vrr_enabled property for crtc based
on the platform support and the request from userspace.

V2: Check for platform support before updating the prop.
V3: Don't attach vrr_enabled prop, as it is alreay attached.

Cc: Ville Syrjälä 
Cc: Manasi Navare 
Signed-off-by: Bhanuprakash Modem 
---
  drivers/gpu/drm/i915/display/intel_vrr.c | 8 
  1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c 
b/drivers/gpu/drm/i915/display/intel_vrr.c
index 396f2f994fa0..7263b35550de 100644
--- a/drivers/gpu/drm/i915/display/intel_vrr.c
+++ b/drivers/gpu/drm/i915/display/intel_vrr.c
@@ -160,6 +160,10 @@ void intel_vrr_enable(struct intel_encoder *encoder,
enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
u32 trans_vrr_ctl;
  
+	if (HAS_VRR(dev_priv))

+   drm_mode_crtc_set_vrr_enabled_property(crtc_state->uapi.crtc,
+  crtc_state->vrr.enable);


But here if userspace has accidently set the CRTC vrr enabled prop to
true without cheking VRR capabile prop, this will be true and here the
driver will still not
reset it. If that is the purpose of adding this then we should infact
set this to false if !HAS_VRR


Yes, my intention is same as you mentioned. But this looks correct to 
me, because:


1) HAS_VRR() is a check to make sure that the driver supports VRR. If 
there is no VRR support on the driver, then there is no point to 
set/clear this prop.


2) crtc_state->vrr.enable is true If vrr_capable is set to true, else false.

So we can interpret above check as below

enable_VRR() {
IF Driver supports VRR:
IF VRR_CAPABLE:
Set VRR_ENABLED;
ELSE:
Clear VRR_ENABLED;
ELSE:
Do nothing;
}

- Bhanu



Manasi


+
if (!crtc_state->vrr.enable)
return;
  
@@ -211,6 +215,10 @@ void intel_vrr_disable(const struct intel_crtc_state *old_crtc_state)

struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
enum transcoder cpu_transcoder = old_crtc_state->cpu_transcoder;
  
+	if (HAS_VRR(dev_priv))

+   
drm_mode_crtc_set_vrr_enabled_property(old_crtc_state->uapi.crtc,
+  false);
+
if (!old_crtc_state->vrr.enable)
return;
  
--

2.35.1





Re: [RFC V3 2/2] drm/i915/vrr: Set drm crtc vrr_enabled property

2022-05-31 Thread Navare, Manasi
On Tue, May 17, 2022 at 12:56:36PM +0530, Bhanuprakash Modem wrote:
> This function sets the vrr_enabled property for crtc based
> on the platform support and the request from userspace.
> 
> V2: Check for platform support before updating the prop.
> V3: Don't attach vrr_enabled prop, as it is alreay attached.
> 
> Cc: Ville Syrjälä 
> Cc: Manasi Navare 
> Signed-off-by: Bhanuprakash Modem 
> ---
>  drivers/gpu/drm/i915/display/intel_vrr.c | 8 
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c 
> b/drivers/gpu/drm/i915/display/intel_vrr.c
> index 396f2f994fa0..7263b35550de 100644
> --- a/drivers/gpu/drm/i915/display/intel_vrr.c
> +++ b/drivers/gpu/drm/i915/display/intel_vrr.c
> @@ -160,6 +160,10 @@ void intel_vrr_enable(struct intel_encoder *encoder,
>   enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
>   u32 trans_vrr_ctl;
>  
> + if (HAS_VRR(dev_priv))
> + drm_mode_crtc_set_vrr_enabled_property(crtc_state->uapi.crtc,
> +crtc_state->vrr.enable);

But here if userspace has accidently set the CRTC vrr enabled prop to
true without cheking VRR capabile prop, this will be true and here the
driver will still not
reset it. If that is the purpose of adding this then we should infact
set this to false if !HAS_VRR

Manasi

> +
>   if (!crtc_state->vrr.enable)
>   return;
>  
> @@ -211,6 +215,10 @@ void intel_vrr_disable(const struct intel_crtc_state 
> *old_crtc_state)
>   struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>   enum transcoder cpu_transcoder = old_crtc_state->cpu_transcoder;
>  
> + if (HAS_VRR(dev_priv))
> + 
> drm_mode_crtc_set_vrr_enabled_property(old_crtc_state->uapi.crtc,
> +false);
> +
>   if (!old_crtc_state->vrr.enable)
>   return;
>  
> -- 
> 2.35.1
> 


[RFC V3 2/2] drm/i915/vrr: Set drm crtc vrr_enabled property

2022-05-17 Thread Bhanuprakash Modem
This function sets the vrr_enabled property for crtc based
on the platform support and the request from userspace.

V2: Check for platform support before updating the prop.
V3: Don't attach vrr_enabled prop, as it is alreay attached.

Cc: Ville Syrjälä 
Cc: Manasi Navare 
Signed-off-by: Bhanuprakash Modem 
---
 drivers/gpu/drm/i915/display/intel_vrr.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c 
b/drivers/gpu/drm/i915/display/intel_vrr.c
index 396f2f994fa0..7263b35550de 100644
--- a/drivers/gpu/drm/i915/display/intel_vrr.c
+++ b/drivers/gpu/drm/i915/display/intel_vrr.c
@@ -160,6 +160,10 @@ void intel_vrr_enable(struct intel_encoder *encoder,
enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
u32 trans_vrr_ctl;
 
+   if (HAS_VRR(dev_priv))
+   drm_mode_crtc_set_vrr_enabled_property(crtc_state->uapi.crtc,
+  crtc_state->vrr.enable);
+
if (!crtc_state->vrr.enable)
return;
 
@@ -211,6 +215,10 @@ void intel_vrr_disable(const struct intel_crtc_state 
*old_crtc_state)
struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
enum transcoder cpu_transcoder = old_crtc_state->cpu_transcoder;
 
+   if (HAS_VRR(dev_priv))
+   
drm_mode_crtc_set_vrr_enabled_property(old_crtc_state->uapi.crtc,
+  false);
+
if (!old_crtc_state->vrr.enable)
return;
 
-- 
2.35.1