[PATCH 0/6] eDP DRRS based on frontbuffer tracking

[PATCH 0/6] eDP DRRS based on frontbuffer tracking

[Ramalingam C]

This series includes a preparation patch for drrs support across differnt
platforms in intel_dp_set_m_n along with last 5 pending patches of V3 eDP
DRRS patch series.

New series is submitted to make the review more comfortable and
to display the dependancy of the patches explicitly.

Durgadoss R (1):
  drm/i915: Enable eDP DRRS for CHV

Ramalingam C (1):
  drm/i915: Add support for DRRS in intel_dp_set_m_n

Vandana Kannan (4):
  drm/i915/bdw: Add support for DRRS to switch RR
  drm/i915: Support for RR switching on VLV
  Documentation/drm: DocBook integration for DRRS
  drm/i915: Add debugfs entry for DRRS

 Documentation/DocBook/drm.tmpl       |   11 ++++
 drivers/gpu/drm/i915/i915_debugfs.c  |   99 ++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_reg.h      |    1 +
 drivers/gpu/drm/i915/intel_display.c |   32 ++++++---
 drivers/gpu/drm/i915/intel_dp.c      |  121 ++++++++++++++++++++++++++++++++--
 drivers/gpu/drm/i915/intel_drv.h     |   22 ++++++-
 6 files changed, 273 insertions(+), 13 deletions(-)

-- 
1.7.9.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 1/6] drm/i915: Add support for DRRS in intel_dp_set_m_n

[Ramalingam C]

Till Gen 7 we have two sets of M_N registers, but Gen 8 onwards
we have only one M_N register set. To support DRRS on both scenarios
a input parameter to intel_dp_set_m_n is added.

In case of DRRS, When platform provides two set of M_N registers for dp,
we can program them with two different dividers and switch between them.
But when only one such register set is provided, we have to program
the required divider M_N value on that registers itself.

Two enum members M1_N1 and M2_N2 are defined to represent the above
scenarios.

M1_N1        :	Program dp_m_n on M1_N1 registers
			dp_m2_n2 on M2_N2 registers (If supported)

M2_N2        :	Program dp_m2_n2 on M1_N1 registers
			M2_N2 registers are not supported

Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |   30 +++++++++++++++++++++++-------
 drivers/gpu/drm/i915/intel_drv.h     |   22 +++++++++++++++++++++-
 2 files changed, 44 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 3b0fe9f..2af24a7 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4322,7 +4322,7 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
 		intel_prepare_shared_dpll(intel_crtc);
 
 	if (intel_crtc->config->has_dp_encoder)
-		intel_dp_set_m_n(intel_crtc);
+		intel_dp_set_m_n(intel_crtc, M1_N1);
 
 	intel_set_pipe_timings(intel_crtc);
 
@@ -4430,7 +4430,7 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
 		intel_enable_shared_dpll(intel_crtc);
 
 	if (intel_crtc->config->has_dp_encoder)
-		intel_dp_set_m_n(intel_crtc);
+		intel_dp_set_m_n(intel_crtc, M1_N1);
 
 	intel_set_pipe_timings(intel_crtc);
 
@@ -5044,7 +5044,7 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
 	}
 
 	if (intel_crtc->config->has_dp_encoder)
-		intel_dp_set_m_n(intel_crtc);
+		intel_dp_set_m_n(intel_crtc, M1_N1);
 
 	intel_set_pipe_timings(intel_crtc);
 
@@ -5120,7 +5120,7 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
 	i9xx_set_pll_dividers(intel_crtc);
 
 	if (intel_crtc->config->has_dp_encoder)
-		intel_dp_set_m_n(intel_crtc);
+		intel_dp_set_m_n(intel_crtc, M1_N1);
 
 	intel_set_pipe_timings(intel_crtc);
 
@@ -5895,13 +5895,29 @@ static void intel_cpu_transcoder_set_m_n(struct intel_crtc *crtc,
 	}
 }
 
-void intel_dp_set_m_n(struct intel_crtc *crtc)
+void intel_dp_set_m_n(struct intel_crtc *crtc, enum link_m_n_set m_n)
 {
+	struct intel_link_m_n *dp_m_n, *dp_m2_n2 = NULL;
+
+	if (m_n == M1_N1) {
+		dp_m_n = &crtc->config->dp_m_n;
+		dp_m2_n2 = &crtc->config->dp_m2_n2;
+	} else if (m_n == M2_N2) {
+
+		/*
+		 * M2_N2 registers are not supported. Hence m2_n2 divider value
+		 * needs to be programmed into M1_N1.
+		 */
+		dp_m_n = &crtc->config->dp_m2_n2;
+	} else {
+		DRM_ERROR("Unsupported divider value\n");
+		return;
+	}
+
 	if (crtc->config->has_pch_encoder)
 		intel_pch_transcoder_set_m_n(crtc, &crtc->config->dp_m_n);
 	else
-		intel_cpu_transcoder_set_m_n(crtc, &crtc->config->dp_m_n,
-						   &crtc->config->dp_m2_n2);
+		intel_cpu_transcoder_set_m_n(crtc, dp_m_n, dp_m2_n2);
 }
 
 static void vlv_update_pll(struct intel_crtc *crtc,
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 1de8e20..1fb1529 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -593,6 +593,26 @@ struct intel_hdmi {
 struct intel_dp_mst_encoder;
 #define DP_MAX_DOWNSTREAM_PORTS		0x10
 
+/*
+ * enum link_m_n_set:
+ *	When platform provides two set of M_N registers for dp, we can
+ *	program them and switch between them incase of DRRS.
+ *	But When only one such register is provided, we have to program the
+ *	required divider value on that registers itself based on the DRRS state.
+ *
+ * M1_N1	: Program dp_m_n on M1_N1 registers
+ *			  dp_m2_n2 on M2_N2 registers (If supported)
+ *
+ * M2_N2	: Program dp_m2_n2 on M1_N1 registers
+ *			  M2_N2 registers are not supported
+ */
+
+enum link_m_n_set {
+	/* Sets the m1_n1 and m2_n2 */
+	M1_N1 = 0,
+	M2_N2
+};
+
 struct intel_dp {
 	uint32_t output_reg;
 	uint32_t aux_ch_ctl_reg;
@@ -994,7 +1014,7 @@ void hsw_enable_pc8(struct drm_i915_private *dev_priv);
 void hsw_disable_pc8(struct drm_i915_private *dev_priv);
 void intel_dp_get_m_n(struct intel_crtc *crtc,
 		      struct intel_crtc_state *pipe_config);
-void intel_dp_set_m_n(struct intel_crtc *crtc);
+void intel_dp_set_m_n(struct intel_crtc *crtc, enum link_m_n_set m_n);
 int intel_dotclock_calculate(int link_freq, const struct intel_link_m_n *m_n);
 void
 ironlake_check_encoder_dotclock(const struct intel_crtc_state *pipe_config,
-- 
1.7.9.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/6] drm/i915: Add support for DRRS in intel_dp_set_m_n

[Rodrigo Vivi]

On Fri, Feb 13, 2015 at 2:02 AM, Ramalingam C <ramalingam.c@intel.com> wrote:
> Till Gen 7 we have two sets of M_N registers, but Gen 8 onwards
> we have only one M_N register set. To support DRRS on both scenarios
> a input parameter to intel_dp_set_m_n is added.
>
> In case of DRRS, When platform provides two set of M_N registers for dp,
> we can program them with two different dividers and switch between them.
> But when only one such register set is provided, we have to program
> the required divider M_N value on that registers itself.
>
> Two enum members M1_N1 and M2_N2 are defined to represent the above
> scenarios.
>
> M1_N1        :  Program dp_m_n on M1_N1 registers
>                         dp_m2_n2 on M2_N2 registers (If supported)
>
> M2_N2        :  Program dp_m2_n2 on M1_N1 registers
>                         M2_N2 registers are not supported

On a quickly first look this enum name confused me... "Why name M2_N2
when it isn't supported?!"
But than I remembered the logic behind, read again and saw it was
M2_N2 value on dp_m_n register...


>
> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c |   30 +++++++++++++++++++++++-------
>  drivers/gpu/drm/i915/intel_drv.h     |   22 +++++++++++++++++++++-
>  2 files changed, 44 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 3b0fe9f..2af24a7 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4322,7 +4322,7 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
>                 intel_prepare_shared_dpll(intel_crtc);
>
>         if (intel_crtc->config->has_dp_encoder)
> -               intel_dp_set_m_n(intel_crtc);
> +               intel_dp_set_m_n(intel_crtc, M1_N1);
>
>         intel_set_pipe_timings(intel_crtc);
>
> @@ -4430,7 +4430,7 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
>                 intel_enable_shared_dpll(intel_crtc);
>
>         if (intel_crtc->config->has_dp_encoder)
> -               intel_dp_set_m_n(intel_crtc);
> +               intel_dp_set_m_n(intel_crtc, M1_N1);
>
>         intel_set_pipe_timings(intel_crtc);
>
> @@ -5044,7 +5044,7 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
>         }
>
>         if (intel_crtc->config->has_dp_encoder)
> -               intel_dp_set_m_n(intel_crtc);
> +               intel_dp_set_m_n(intel_crtc, M1_N1);
>
>         intel_set_pipe_timings(intel_crtc);
>
> @@ -5120,7 +5120,7 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
>         i9xx_set_pll_dividers(intel_crtc);
>
>         if (intel_crtc->config->has_dp_encoder)
> -               intel_dp_set_m_n(intel_crtc);
> +               intel_dp_set_m_n(intel_crtc, M1_N1);
>
>         intel_set_pipe_timings(intel_crtc);
>
> @@ -5895,13 +5895,29 @@ static void intel_cpu_transcoder_set_m_n(struct intel_crtc *crtc,
>         }
>  }
>
> -void intel_dp_set_m_n(struct intel_crtc *crtc)
> +void intel_dp_set_m_n(struct intel_crtc *crtc, enum link_m_n_set m_n)
>  {
> +       struct intel_link_m_n *dp_m_n, *dp_m2_n2 = NULL;
> +
> +       if (m_n == M1_N1) {
> +               dp_m_n = &crtc->config->dp_m_n;
> +               dp_m2_n2 = &crtc->config->dp_m2_n2;
> +       } else if (m_n == M2_N2) {
> +
> +               /*
> +                * M2_N2 registers are not supported. Hence m2_n2 divider value
> +                * needs to be programmed into M1_N1.
> +                */

.. also this good comment refreshed my memory! ;)

> +               dp_m_n = &crtc->config->dp_m2_n2;
> +       } else {
> +               DRM_ERROR("Unsupported divider value\n");
> +               return;
> +       }
> +
>         if (crtc->config->has_pch_encoder)
>                 intel_pch_transcoder_set_m_n(crtc, &crtc->config->dp_m_n);
>         else
> -               intel_cpu_transcoder_set_m_n(crtc, &crtc->config->dp_m_n,
> -                                                  &crtc->config->dp_m2_n2);
> +               intel_cpu_transcoder_set_m_n(crtc, dp_m_n, dp_m2_n2);
>  }
>
>  static void vlv_update_pll(struct intel_crtc *crtc,
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 1de8e20..1fb1529 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -593,6 +593,26 @@ struct intel_hdmi {
>  struct intel_dp_mst_encoder;
>  #define DP_MAX_DOWNSTREAM_PORTS                0x10
>
> +/*
> + * enum link_m_n_set:
> + *     When platform provides two set of M_N registers for dp, we can
> + *     program them and switch between them incase of DRRS.
> + *     But When only one such register is provided, we have to program the
> + *     required divider value on that registers itself based on the DRRS state.
> + *
> + * M1_N1       : Program dp_m_n on M1_N1 registers
> + *                       dp_m2_n2 on M2_N2 registers (If supported)
> + *
> + * M2_N2       : Program dp_m2_n2 on M1_N1 registers
> + *                       M2_N2 registers are not supported
> + */
> +
> +enum link_m_n_set {
> +       /* Sets the m1_n1 and m2_n2 */
> +       M1_N1 = 0,
> +       M2_N2
> +};
> +
>  struct intel_dp {
>         uint32_t output_reg;
>         uint32_t aux_ch_ctl_reg;
> @@ -994,7 +1014,7 @@ void hsw_enable_pc8(struct drm_i915_private *dev_priv);
>  void hsw_disable_pc8(struct drm_i915_private *dev_priv);
>  void intel_dp_get_m_n(struct intel_crtc *crtc,
>                       struct intel_crtc_state *pipe_config);
> -void intel_dp_set_m_n(struct intel_crtc *crtc);
> +void intel_dp_set_m_n(struct intel_crtc *crtc, enum link_m_n_set m_n);
>  int intel_dotclock_calculate(int link_freq, const struct intel_link_m_n *m_n);
>  void
>  ironlake_check_encoder_dotclock(const struct intel_crtc_state *pipe_config,
> --
> 1.7.9.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Thanks for this change.

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/6] drm/i915/bdw: Add support for DRRS to switch RR

[Ramalingam C]

From: Vandana Kannan <vandana.kannan@intel.com>

For Broadwell, there is one instance of Transcoder MN values per transcoder.
For dynamic switching between multiple refreshr rates, M/N values may be
reprogrammed on the fly. Link N programming triggers update of all data and
link M & N registers and the new M/N values will be used in the next frame
that is output.

V2: [By Ram]: intel_dp_set_m_n() is rewritten to accommodate
	gen >= 8 [Rodrigo]
V3: Coding style correction [Ram]
V4: [By Ram] intel_dp_set_m_n modifications are moved into a
	separate patch, retaining only DRRS related changes here [Rodrigo]

Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
Signed-off-by: Pradeep Bhat <pradeep.bhat@intel.com>
Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c |   16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 868a07b..6ffbf57 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4793,12 +4793,24 @@ static void intel_dp_set_drrs_state(struct drm_device *dev, int refresh_rate)
 		return;
 	}
 
-	if (INTEL_INFO(dev)->gen > 6 && INTEL_INFO(dev)->gen < 8) {
+	if (INTEL_INFO(dev)->gen >= 8) {
+		switch (index) {
+		case DRRS_HIGH_RR:
+			intel_dp_set_m_n(intel_crtc, M1_N1);
+			break;
+		case DRRS_LOW_RR:
+			intel_dp_set_m_n(intel_crtc, M2_N2);
+			break;
+		case DRRS_MAX_RR:
+		default:
+			DRM_ERROR("Unsupported refreshrate type\n");
+		}
+	} else if (INTEL_INFO(dev)->gen > 6) {
 		reg = PIPECONF(intel_crtc->config->cpu_transcoder);
 		val = I915_READ(reg);
+
 		if (index > DRRS_HIGH_RR) {
 			val |= PIPECONF_EDP_RR_MODE_SWITCH;
-			intel_dp_set_m_n(intel_crtc);
 		} else {
 			val &= ~PIPECONF_EDP_RR_MODE_SWITCH;
 		}
-- 
1.7.9.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/6] drm/i915/bdw: Add support for DRRS to switch RR

[Rodrigo Vivi]

On Fri, Feb 13, 2015 at 2:03 AM, Ramalingam C <ramalingam.c@intel.com> wrote:
> From: Vandana Kannan <vandana.kannan@intel.com>
>
> For Broadwell, there is one instance of Transcoder MN values per transcoder.
> For dynamic switching between multiple refreshr rates, M/N values may be
> reprogrammed on the fly. Link N programming triggers update of all data and
> link M & N registers and the new M/N values will be used in the next frame
> that is output.
>
> V2: [By Ram]: intel_dp_set_m_n() is rewritten to accommodate
>         gen >= 8 [Rodrigo]
> V3: Coding style correction [Ram]
> V4: [By Ram] intel_dp_set_m_n modifications are moved into a
>         separate patch, retaining only DRRS related changes here [Rodrigo]
>
> Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
> Signed-off-by: Pradeep Bhat <pradeep.bhat@intel.com>
> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c |   16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 868a07b..6ffbf57 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4793,12 +4793,24 @@ static void intel_dp_set_drrs_state(struct drm_device *dev, int refresh_rate)
>                 return;
>         }
>
> -       if (INTEL_INFO(dev)->gen > 6 && INTEL_INFO(dev)->gen < 8) {
> +       if (INTEL_INFO(dev)->gen >= 8) {
> +               switch (index) {
> +               case DRRS_HIGH_RR:
> +                       intel_dp_set_m_n(intel_crtc, M1_N1);
> +                       break;
> +               case DRRS_LOW_RR:
> +                       intel_dp_set_m_n(intel_crtc, M2_N2);
> +                       break;
> +               case DRRS_MAX_RR:

Why to redirect this to an error insted making MAX = HIGH?

> +               default:
> +                       DRM_ERROR("Unsupported refreshrate type\n");
> +               }
> +       } else if (INTEL_INFO(dev)->gen > 6) {
>                 reg = PIPECONF(intel_crtc->config->cpu_transcoder);
>                 val = I915_READ(reg);
> +
>                 if (index > DRRS_HIGH_RR) {
>                         val |= PIPECONF_EDP_RR_MODE_SWITCH;
> -                       intel_dp_set_m_n(intel_crtc);
>                 } else {
>                         val &= ~PIPECONF_EDP_RR_MODE_SWITCH;
>                 }
> --
> 1.7.9.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/6] drm/i915/bdw: Add support for DRRS to switch RR

[Ramalingam C]

Hi,

On Thursday 19 February 2015 10:55 PM, Rodrigo Vivi wrote:
> On Fri, Feb 13, 2015 at 2:03 AM, Ramalingam C <ramalingam.c@intel.com> wrote:
>> From: Vandana Kannan <vandana.kannan@intel.com>
>>
>> For Broadwell, there is one instance of Transcoder MN values per transcoder.
>> For dynamic switching between multiple refreshr rates, M/N values may be
>> reprogrammed on the fly. Link N programming triggers update of all data and
>> link M & N registers and the new M/N values will be used in the next frame
>> that is output.
>>
>> V2: [By Ram]: intel_dp_set_m_n() is rewritten to accommodate
>>          gen >= 8 [Rodrigo]
>> V3: Coding style correction [Ram]
>> V4: [By Ram] intel_dp_set_m_n modifications are moved into a
>>          separate patch, retaining only DRRS related changes here [Rodrigo]
>>
>> Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
>> Signed-off-by: Pradeep Bhat <pradeep.bhat@intel.com>
>> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_dp.c |   16 ++++++++++++++--
>>   1 file changed, 14 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index 868a07b..6ffbf57 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -4793,12 +4793,24 @@ static void intel_dp_set_drrs_state(struct drm_device *dev, int refresh_rate)
>>                  return;
>>          }
>>
>> -       if (INTEL_INFO(dev)->gen > 6 && INTEL_INFO(dev)->gen < 8) {
>> +       if (INTEL_INFO(dev)->gen >= 8) {
>> +               switch (index) {
>> +               case DRRS_HIGH_RR:
>> +                       intel_dp_set_m_n(intel_crtc, M1_N1);
>> +                       break;
>> +               case DRRS_LOW_RR:
>> +                       intel_dp_set_m_n(intel_crtc, M2_N2);
>> +                       break;
>> +               case DRRS_MAX_RR:
> Why to redirect this to an error insted making MAX = HIGH?
This DRRS state is decided within kernel based on the vrefresh 
requested. Hence this can't be out of HIGH/LOW.
So this case can't occur. If it occurs I would like to report it as an 
error.
>
>> +               default:
>> +                       DRM_ERROR("Unsupported refreshrate type\n");
>> +               }
>> +       } else if (INTEL_INFO(dev)->gen > 6) {
>>                  reg = PIPECONF(intel_crtc->config->cpu_transcoder);
>>                  val = I915_READ(reg);
>> +
>>                  if (index > DRRS_HIGH_RR) {
>>                          val |= PIPECONF_EDP_RR_MODE_SWITCH;
>> -                       intel_dp_set_m_n(intel_crtc);
>>                  } else {
>>                          val &= ~PIPECONF_EDP_RR_MODE_SWITCH;
>>                  }
>> --
>> 1.7.9.5
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>

-- 
Ram

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/6] drm/i915/bdw: Add support for DRRS to switch RR

[Rodrigo Vivi]

On Thu, Feb 19, 2015 at 10:15 PM, Ramalingam C <ramalingam.c@intel.com> wrote:
> Hi,
>
>
> On Thursday 19 February 2015 10:55 PM, Rodrigo Vivi wrote:
>>
>> On Fri, Feb 13, 2015 at 2:03 AM, Ramalingam C <ramalingam.c@intel.com>
>> wrote:
>>>
>>> From: Vandana Kannan <vandana.kannan@intel.com>
>>>
>>> For Broadwell, there is one instance of Transcoder MN values per
>>> transcoder.
>>> For dynamic switching between multiple refreshr rates, M/N values may be
>>> reprogrammed on the fly. Link N programming triggers update of all data
>>> and
>>> link M & N registers and the new M/N values will be used in the next
>>> frame
>>> that is output.
>>>
>>> V2: [By Ram]: intel_dp_set_m_n() is rewritten to accommodate
>>>          gen >= 8 [Rodrigo]
>>> V3: Coding style correction [Ram]
>>> V4: [By Ram] intel_dp_set_m_n modifications are moved into a
>>>          separate patch, retaining only DRRS related changes here
>>> [Rodrigo]
>>>
>>> Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
>>> Signed-off-by: Pradeep Bhat <pradeep.bhat@intel.com>
>>> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/intel_dp.c |   16 ++++++++++++++--
>>>   1 file changed, 14 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c
>>> b/drivers/gpu/drm/i915/intel_dp.c
>>> index 868a07b..6ffbf57 100644
>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>> @@ -4793,12 +4793,24 @@ static void intel_dp_set_drrs_state(struct
>>> drm_device *dev, int refresh_rate)
>>>                  return;
>>>          }
>>>
>>> -       if (INTEL_INFO(dev)->gen > 6 && INTEL_INFO(dev)->gen < 8) {
>>> +       if (INTEL_INFO(dev)->gen >= 8) {
>>> +               switch (index) {
>>> +               case DRRS_HIGH_RR:
>>> +                       intel_dp_set_m_n(intel_crtc, M1_N1);
>>> +                       break;
>>> +               case DRRS_LOW_RR:
>>> +                       intel_dp_set_m_n(intel_crtc, M2_N2);
>>> +                       break;
>>> +               case DRRS_MAX_RR:
>>
>> Why to redirect this to an error insted making MAX = HIGH?
>
> This DRRS state is decided within kernel based on the vrefresh requested.
> Hence this can't be out of HIGH/LOW.
> So this case can't occur. If it occurs I would like to report it as an
> error.

Thanks for the explanation
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

>>
>>
>>> +               default:
>>> +                       DRM_ERROR("Unsupported refreshrate type\n");
>>> +               }
>>> +       } else if (INTEL_INFO(dev)->gen > 6) {
>>>                  reg = PIPECONF(intel_crtc->config->cpu_transcoder);
>>>                  val = I915_READ(reg);
>>> +
>>>                  if (index > DRRS_HIGH_RR) {
>>>                          val |= PIPECONF_EDP_RR_MODE_SWITCH;
>>> -                       intel_dp_set_m_n(intel_crtc);
>>>                  } else {
>>>                          val &= ~PIPECONF_EDP_RR_MODE_SWITCH;
>>>                  }
>>> --
>>> 1.7.9.5
>>>
>>> _______________________________________________
>>> Intel-gfx mailing list
>>> Intel-gfx@lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>
>>
>>
>
> --
> Ram
>



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/6] drm/i915: Support for RR switching on VLV

[Ramalingam C]

From: Vandana Kannan <vandana.kannan@intel.com>

Definition of VLV RR switch bit and corresponding toggling in
set_drrs function.

Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
Signed-off-by: Uma Shankar <uma.shankar@intel.com>
Reviewed-by: Jani Nikula <jani.nikula@intel.com>
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h |    1 +
 drivers/gpu/drm/i915/intel_dp.c |   10 ++++++++--
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 39bdbf9..944f788 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -3880,6 +3880,7 @@ enum skl_disp_power_wells {
 #define   PIPECONF_INTERLACE_MODE_MASK		(7 << 21)
 #define   PIPECONF_EDP_RR_MODE_SWITCH		(1 << 20)
 #define   PIPECONF_CXSR_DOWNCLOCK	(1<<16)
+#define   PIPECONF_EDP_RR_MODE_SWITCH_VLV	(1 << 14)
 #define   PIPECONF_COLOR_RANGE_SELECT	(1 << 13)
 #define   PIPECONF_BPC_MASK	(0x7 << 5)
 #define   PIPECONF_8BPC		(0<<5)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 6ffbf57..9f3da8f 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4810,9 +4810,15 @@ static void intel_dp_set_drrs_state(struct drm_device *dev, int refresh_rate)
 		val = I915_READ(reg);
 
 		if (index > DRRS_HIGH_RR) {
-			val |= PIPECONF_EDP_RR_MODE_SWITCH;
+			if (IS_VALLEYVIEW(dev))
+				val |= PIPECONF_EDP_RR_MODE_SWITCH_VLV;
+			else
+				val |= PIPECONF_EDP_RR_MODE_SWITCH;
 		} else {
-			val &= ~PIPECONF_EDP_RR_MODE_SWITCH;
+			if (IS_VALLEYVIEW(dev))
+				val &= ~PIPECONF_EDP_RR_MODE_SWITCH_VLV;
+			else
+				val &= ~PIPECONF_EDP_RR_MODE_SWITCH;
 		}
 		I915_WRITE(reg, val);
 	}
-- 
1.7.9.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 4/6] drm/i915: Enable eDP DRRS for CHV

[Ramalingam C]

From: Durgadoss R <durgadoss.r@intel.com>

This patch enables eDP DRRS for CHV by adding the
required IS_CHERRYVIEW() checks.
CHV uses the same register bit as VLV.

[Vandana]: Since CHV has 2 sets of M_N registers, it will follow the same code
path as gen < 8. Added CHV check in dp_set_m_n()

[Ram]: Rebased on top of previous patch modifications

Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |    2 +-
 drivers/gpu/drm/i915/intel_dp.c      |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 2af24a7..6548524 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5879,7 +5879,7 @@ static void intel_cpu_transcoder_set_m_n(struct intel_crtc *crtc,
 		 * for gen < 8) and if DRRS is supported (to make sure the
 		 * registers are not unnecessarily accessed).
 		 */
-		if (m2_n2 && INTEL_INFO(dev)->gen < 8 &&
+		if (m2_n2 && (IS_CHERRYVIEW(dev) || INTEL_INFO(dev)->gen < 8) &&
 			crtc->config->has_drrs) {
 			I915_WRITE(PIPE_DATA_M2(transcoder),
 					TU_SIZE(m2_n2->tu) | m2_n2->gmch_m);
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 9f3da8f..dfbe97d 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4793,7 +4793,7 @@ static void intel_dp_set_drrs_state(struct drm_device *dev, int refresh_rate)
 		return;
 	}
 
-	if (INTEL_INFO(dev)->gen >= 8) {
+	if (INTEL_INFO(dev)->gen >= 8 && !IS_CHERRYVIEW(dev)) {
 		switch (index) {
 		case DRRS_HIGH_RR:
 			intel_dp_set_m_n(intel_crtc, M1_N1);
-- 
1.7.9.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/6] drm/i915: Enable eDP DRRS for CHV

[Rodrigo Vivi]

On Fri, Feb 13, 2015 at 2:03 AM, Ramalingam C <ramalingam.c@intel.com> wrote:
> From: Durgadoss R <durgadoss.r@intel.com>
>
> This patch enables eDP DRRS for CHV by adding the
> required IS_CHERRYVIEW() checks.
> CHV uses the same register bit as VLV.
>
> [Vandana]: Since CHV has 2 sets of M_N registers, it will follow the same code
> path as gen < 8. Added CHV check in dp_set_m_n()
>
> [Ram]: Rebased on top of previous patch modifications
>
> Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
> Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c |    2 +-
>  drivers/gpu/drm/i915/intel_dp.c      |    2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 2af24a7..6548524 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5879,7 +5879,7 @@ static void intel_cpu_transcoder_set_m_n(struct intel_crtc *crtc,
>                  * for gen < 8) and if DRRS is supported (to make sure the
>                  * registers are not unnecessarily accessed).
>                  */
> -               if (m2_n2 && INTEL_INFO(dev)->gen < 8 &&
> +               if (m2_n2 && (IS_CHERRYVIEW(dev) || INTEL_INFO(dev)->gen < 8) &&
This got a bit confusing and something that looks like it will need
fix on next enablings...
>                         crtc->config->has_drrs) {
>                         I915_WRITE(PIPE_DATA_M2(transcoder),
>                                         TU_SIZE(m2_n2->tu) | m2_n2->gmch_m);
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 9f3da8f..dfbe97d 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4793,7 +4793,7 @@ static void intel_dp_set_drrs_state(struct drm_device *dev, int refresh_rate)
>                 return;
>         }
>
> -       if (INTEL_INFO(dev)->gen >= 8) {
> +       if (INTEL_INFO(dev)->gen >= 8 && !IS_CHERRYVIEW(dev)) {
>                 switch (index) {
>                 case DRRS_HIGH_RR:
>                         intel_dp_set_m_n(intel_crtc, M1_N1);
> --
> 1.7.9.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

But apparently is right, so feel free to use:
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 5/6] Documentation/drm: DocBook integration for DRRS

[Ramalingam C]

From: Vandana Kannan <vandana.kannan@intel.com>

Adding an overview of DRRS in general and the implementation for eDP DRRS.
Also, describing the functions related to eDP DRRS.

Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 Documentation/DocBook/drm.tmpl  |   11 +++++
 drivers/gpu/drm/i915/intel_dp.c |   95 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 106 insertions(+)

diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
index 249f0c9..7a45775 100644
--- a/Documentation/DocBook/drm.tmpl
+++ b/Documentation/DocBook/drm.tmpl
@@ -4053,6 +4053,17 @@ int num_ioctls;</synopsis>
 !Idrivers/gpu/drm/i915/intel_fbc.c
       </sect2>
       <sect2>
+        <title>Display Refresh Rate Switching (DRRS)</title>
+!Pdrivers/gpu/drm/i915/intel_dp.c Display Refresh Rate Switching (DRRS)
+!Fdrivers/gpu/drm/i915/intel_dp.c intel_dp_set_drrs_state
+!Fdrivers/gpu/drm/i915/intel_dp.c intel_edp_drrs_enable
+!Fdrivers/gpu/drm/i915/intel_dp.c intel_edp_drrs_disable
+!Fdrivers/gpu/drm/i915/intel_dp.c intel_edp_drrs_invalidate
+!Fdrivers/gpu/drm/i915/intel_dp.c intel_edp_drrs_flush
+!Fdrivers/gpu/drm/i915/intel_dp.c intel_dp_drrs_init
+
+      </sect2>
+      <sect2>
         <title>DPIO</title>
 !Pdrivers/gpu/drm/i915/i915_reg.h DPIO
 	<table id="dpiox2">
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index dfbe97d..e9862e7 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4736,6 +4736,18 @@ intel_dp_init_panel_power_sequencer_registers(struct drm_device *dev,
 		      I915_READ(pp_div_reg));
 }
 
+/**
+ * intel_dp_set_drrs_state - program registers for RR switch to take effect
+ * @dev: DRM device
+ * @refresh_rate: RR to be programmed
+ *
+ * This function gets called when refresh rate (RR) has to be changed from
+ * one frequency to another. Switches can be between high and low RR
+ * supported by the panel or to any other RR based on media playback (in
+ * this case, RR value needs to be passed from user space).
+ *
+ * The caller of this function needs to take a lock on dev_priv->drrs.
+ */
 static void intel_dp_set_drrs_state(struct drm_device *dev, int refresh_rate)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -4828,6 +4840,12 @@ static void intel_dp_set_drrs_state(struct drm_device *dev, int refresh_rate)
 	DRM_DEBUG_KMS("eDP Refresh Rate set to : %dHz\n", refresh_rate);
 }
 
+/**
+ * intel_edp_drrs_enable - init drrs struct if supported
+ * @intel_dp: DP struct
+ *
+ * Initializes frontbuffer_bits and drrs.dp
+ */
 void intel_edp_drrs_enable(struct intel_dp *intel_dp)
 {
 	struct drm_device *dev = intel_dp_to_dev(intel_dp);
@@ -4855,6 +4873,11 @@ unlock:
 	mutex_unlock(&dev_priv->drrs.mutex);
 }
 
+/**
+ * intel_edp_drrs_disable - Disable DRRS
+ * @intel_dp: DP struct
+ *
+ */
 void intel_edp_drrs_disable(struct intel_dp *intel_dp)
 {
 	struct drm_device *dev = intel_dp_to_dev(intel_dp);
@@ -4914,6 +4937,17 @@ unlock:
 	mutex_unlock(&dev_priv->drrs.mutex);
 }
 
+/**
+ * intel_edp_drrs_invalidate - Invalidate DRRS
+ * @dev: DRM device
+ * @frontbuffer_bits: frontbuffer plane tracking bits
+ *
+ * When there is a disturbance on screen (due to cursor movement/time
+ * update etc), DRRS needs to be invalidated, i.e. need to switch to
+ * high RR.
+ *
+ * Dirty frontbuffers relevant to DRRS are tracked in busy_frontbuffer_bits.
+ */
 void intel_edp_drrs_invalidate(struct drm_device *dev,
 		unsigned frontbuffer_bits)
 {
@@ -4941,6 +4975,17 @@ void intel_edp_drrs_invalidate(struct drm_device *dev,
 	mutex_unlock(&dev_priv->drrs.mutex);
 }
 
+/**
+ * intel_edp_drrs_flush - Flush DRRS
+ * @dev: DRM device
+ * @frontbuffer_bits: frontbuffer plane tracking bits
+ *
+ * When there is no movement on screen, DRRS work can be scheduled.
+ * This DRRS work is responsible for setting relevant registers after a
+ * timeout of 1 second.
+ *
+ * Dirty frontbuffers relevant to DRRS are tracked in busy_frontbuffer_bits.
+ */
 void intel_edp_drrs_flush(struct drm_device *dev,
 		unsigned frontbuffer_bits)
 {
@@ -4965,6 +5010,56 @@ void intel_edp_drrs_flush(struct drm_device *dev,
 	mutex_unlock(&dev_priv->drrs.mutex);
 }
 
+/**
+ * DOC: Display Refresh Rate Switching (DRRS)
+ *
+ * Display Refresh Rate Switching (DRRS) is a power conservation feature
+ * which enables swtching between low and high refresh rates,
+ * dynamically, based on the usage scenario. This feature is applicable
+ * for internal panels.
+ *
+ * Indication that the panel supports DRRS is given by the panel EDID, which
+ * would list multiple refresh rates for one resolution.
+ *
+ * DRRS is of 2 types - static and seamless.
+ * Static DRRS involves changing refresh rate (RR) by doing a full modeset
+ * (may appear as a blink on screen) and is used in dock-undock scenario.
+ * Seamless DRRS involves changing RR without any visual effect to the user
+ * and can be used during normal system usage. This is done by programming
+ * certain registers.
+ *
+ * Support for static/seamless DRRS may be indicated in the VBT based on
+ * inputs from the panel spec.
+ *
+ * DRRS saves power by switching to low RR based on usage scenarios.
+ *
+ * eDP DRRS:-
+ *        The implementation is based on frontbuffer tracking implementation.
+ * When there is a disturbance on the screen triggered by user activity or a
+ * periodic system activity, DRRS is disabled (RR is changed to high RR).
+ * When there is no movement on screen, after a timeout of 1 second, a switch
+ * to low RR is made.
+ *        For integration with frontbuffer tracking code,
+ * intel_edp_drrs_invalidate() and intel_edp_drrs_flush() are called.
+ *
+ * DRRS can be further extended to support other internal panels and also
+ * the scenario of video playback wherein RR is set based on the rate
+ * requested by userspace.
+ */
+
+/**
+ * intel_dp_drrs_init - Init basic DRRS work and mutex.
+ * @intel_connector: eDP connector
+ * @fixed_mode: preferred mode of panel
+ *
+ * This function is  called only once at driver load to initialize basic
+ * DRRS stuff.
+ *
+ * Returns:
+ * Downclock mode if panel supports it, else return NULL.
+ * DRRS support is determined by the presence of downclock mode (apart
+ * from VBT setting).
+ */
 static struct drm_display_mode *
 intel_dp_drrs_init(struct intel_connector *intel_connector,
 		struct drm_display_mode *fixed_mode)
-- 
1.7.9.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 6/6] drm/i915: Add debugfs entry for DRRS

[Ramalingam C]

From: Vandana Kannan <vandana.kannan@intel.com>

Adding a debugfs entry to determine if DRRS is supported or not

V2: [By Ram]: Following details about the active crtc will be filled
	in seq-file of the debugfs
	1. Encoder output type
	2. DRRS Support on this CRTC
	3. DRRS current state
	4. Current Vrefresh
Format is as follows:
CRTC 1:  Output: eDP, DRRS Supported: Yes (Seamless), DRRS_State: DRRS_HIGH_RR, Vrefresh: 60
CRTC 2:  Output: HDMI, DRRS Supported : No, VBT DRRS_type: Seamless
CRTC 1:  Output: eDP, DRRS Supported: Yes (Seamless), DRRS_State: DRRS_LOW_RR, Vrefresh: 40
CRTC 2:  Output: HDMI, DRRS Supported : No, VBT DRRS_type: Seamless

V3: [By Ram]: Readability is improved.
	Another error case is covered [Daniel]

V4: [By Ram]: Current status of the Idleness DRRS along with
	the Front buffer bits are added to the debugfs. [Rodrigo]

Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c |   99 +++++++++++++++++++++++++++++++++++
 1 file changed, 99 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 164fa82..e08d63f 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2869,6 +2869,104 @@ static int i915_ddb_info(struct seq_file *m, void *unused)
 	return 0;
 }
 
+static void drrs_status_per_crtc(struct seq_file *m,
+		struct drm_device *dev, struct intel_crtc *intel_crtc)
+{
+	struct intel_encoder *intel_encoder;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct i915_drrs *drrs = &dev_priv->drrs;
+	int vrefresh = 0;
+
+	for_each_encoder_on_crtc(dev, &intel_crtc->base, intel_encoder) {
+		/* Encoder connected on this CRTC */
+		switch (intel_encoder->type) {
+		case INTEL_OUTPUT_EDP:
+			seq_puts(m, "Output: eDP, ");
+			break;
+		case INTEL_OUTPUT_DSI:
+			seq_puts(m, "Output: DSI, ");
+			break;
+		case INTEL_OUTPUT_HDMI:
+			seq_puts(m, "Output: HDMI, ");
+			break;
+		case INTEL_OUTPUT_DISPLAYPORT:
+			seq_puts(m, "Output: DP, ");
+			break;
+		default:
+			seq_printf(m, "Output: Others (id=%d), ",
+						intel_encoder->type);
+		}
+	}
+
+	if (intel_crtc->config->has_drrs) {
+		struct intel_panel *panel;
+
+		panel = &drrs->dp->attached_connector->panel;
+		/* DRRS Supported */
+		seq_puts(m, "DRRS Supported: Yes (Seamless), ");
+		seq_printf(m, "busy_frontbuffer_bits: 0x%X,\n\t",
+					drrs->busy_frontbuffer_bits);
+
+		if (drrs->busy_frontbuffer_bits) {
+			seq_puts(m, "Front buffer: busy, ");
+			seq_puts(m, "Idleness Timer: Suspended, ");
+		} else {
+			seq_puts(m, "Front buffer: Idle, ");
+			if (drrs->refresh_rate_type == DRRS_HIGH_RR)
+				seq_puts(m, "Idleness Timer: Ticking, ");
+			else
+				seq_puts(m, "Idleness Timer: Suspended, ");
+		}
+
+		if (drrs->refresh_rate_type == DRRS_HIGH_RR) {
+			seq_puts(m, "DRRS_State: DRRS_HIGH_RR, ");
+			vrefresh = panel->fixed_mode->vrefresh;
+		} else if (drrs->refresh_rate_type == DRRS_LOW_RR) {
+			seq_puts(m, "DRRS_State: DRRS_LOW_RR, ");
+			vrefresh = panel->downclock_mode->vrefresh;
+		} else {
+			seq_printf(m, "DRRS_State: Unknown(%d), ",
+						drrs->refresh_rate_type);
+		}
+		seq_printf(m, "Vrefresh: %d", vrefresh);
+
+	} else {
+		/* DRRS not supported. Print the VBT parameter*/
+		seq_puts(m, "DRRS Supported : No, ");
+		if (dev_priv->vbt.drrs_type == STATIC_DRRS_SUPPORT)
+			seq_puts(m, "VBT DRRS_type: Static");
+		else if (dev_priv->vbt.drrs_type == SEAMLESS_DRRS_SUPPORT)
+			seq_puts(m, "VBT DRRS_type: Seamless");
+		else if (dev_priv->vbt.drrs_type == DRRS_NOT_SUPPORTED)
+			seq_puts(m, "VBT DRRS_type: None");
+		else
+			seq_puts(m, "VBT DRRS_type: Unrecognized Value");
+	}
+	seq_puts(m, "\n");
+}
+
+static int i915_drrs_status(struct seq_file *m, void *unused)
+{
+	struct drm_info_node *node = m->private;
+	struct drm_device *dev = node->minor->dev;
+	struct intel_crtc *intel_crtc;
+	int active_crtc_cnt = 0;
+
+	for_each_intel_crtc(dev, intel_crtc) {
+		if (intel_crtc->active) {
+			active_crtc_cnt++;
+			seq_printf(m, "CRTC %d:  ", active_crtc_cnt);
+
+			drrs_status_per_crtc(m, dev, intel_crtc);
+		}
+	}
+
+	if (!active_crtc_cnt)
+		seq_puts(m, "No active crtc found\n");
+
+	return 0;
+}
+
 struct pipe_crc_info {
 	const char *name;
 	struct drm_device *dev;
@@ -4483,6 +4581,7 @@ static const struct drm_info_list i915_debugfs_list[] = {
 	{"i915_dp_mst_info", i915_dp_mst_info, 0},
 	{"i915_wa_registers", i915_wa_registers, 0},
 	{"i915_ddb_info", i915_ddb_info, 0},
+	{"i915_drrs_status", i915_drrs_status, 0},
 };
 #define I915_DEBUGFS_ENTRIES ARRAY_SIZE(i915_debugfs_list)
 
-- 
1.7.9.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 6/6] drm/i915: Add debugfs entry for DRRS

[Rodrigo Vivi]

On Fri, Feb 13, 2015 at 2:03 AM, Ramalingam C <ramalingam.c@intel.com> wrote:
> From: Vandana Kannan <vandana.kannan@intel.com>
>
> Adding a debugfs entry to determine if DRRS is supported or not
>
> V2: [By Ram]: Following details about the active crtc will be filled
>         in seq-file of the debugfs
>         1. Encoder output type
>         2. DRRS Support on this CRTC
>         3. DRRS current state
>         4. Current Vrefresh
> Format is as follows:
> CRTC 1:  Output: eDP, DRRS Supported: Yes (Seamless), DRRS_State: DRRS_HIGH_RR, Vrefresh: 60
> CRTC 2:  Output: HDMI, DRRS Supported : No, VBT DRRS_type: Seamless
> CRTC 1:  Output: eDP, DRRS Supported: Yes (Seamless), DRRS_State: DRRS_LOW_RR, Vrefresh: 40
> CRTC 2:  Output: HDMI, DRRS Supported : No, VBT DRRS_type: Seamless
>
> V3: [By Ram]: Readability is improved.
>         Another error case is covered [Daniel]
>
> V4: [By Ram]: Current status of the Idleness DRRS along with
>         the Front buffer bits are added to the debugfs. [Rodrigo]
>
> Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c |   99 +++++++++++++++++++++++++++++++++++
>  1 file changed, 99 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 164fa82..e08d63f 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2869,6 +2869,104 @@ static int i915_ddb_info(struct seq_file *m, void *unused)
>         return 0;
>  }
>
> +static void drrs_status_per_crtc(struct seq_file *m,
> +               struct drm_device *dev, struct intel_crtc *intel_crtc)
> +{
> +       struct intel_encoder *intel_encoder;
> +       struct drm_i915_private *dev_priv = dev->dev_private;
> +       struct i915_drrs *drrs = &dev_priv->drrs;
> +       int vrefresh = 0;
> +
> +       for_each_encoder_on_crtc(dev, &intel_crtc->base, intel_encoder) {
> +               /* Encoder connected on this CRTC */

Do you really need this info here?

> +               switch (intel_encoder->type) {
> +               case INTEL_OUTPUT_EDP:
> +                       seq_puts(m, "Output: eDP, ");
> +                       break;
> +               case INTEL_OUTPUT_DSI:
> +                       seq_puts(m, "Output: DSI, ");
> +                       break;
> +               case INTEL_OUTPUT_HDMI:
> +                       seq_puts(m, "Output: HDMI, ");
> +                       break;
> +               case INTEL_OUTPUT_DISPLAYPORT:
> +                       seq_puts(m, "Output: DP, ");
> +                       break;
> +               default:
> +                       seq_printf(m, "Output: Others (id=%d), ",
> +                                               intel_encoder->type);
> +               }
> +       }
> +
> +       if (intel_crtc->config->has_drrs) {
> +               struct intel_panel *panel;
> +
> +               panel = &drrs->dp->attached_connector->panel;
> +               /* DRRS Supported */
> +               seq_puts(m, "DRRS Supported: Yes (Seamless), ");

isn't "Yes" enough? Remind that you might want to parse in the future.

> +               seq_printf(m, "busy_frontbuffer_bits: 0x%X,\n\t",
> +                                       drrs->busy_frontbuffer_bits);
> +
> +               if (drrs->busy_frontbuffer_bits) {
> +                       seq_puts(m, "Front buffer: busy, ");
> +                       seq_puts(m, "Idleness Timer: Suspended, ");
> +               } else {
> +                       seq_puts(m, "Front buffer: Idle, ");
> +                       if (drrs->refresh_rate_type == DRRS_HIGH_RR)
> +                               seq_puts(m, "Idleness Timer: Ticking, ");
> +                       else
> +                               seq_puts(m, "Idleness Timer: Suspended, ");
> +               }
> +
> +               if (drrs->refresh_rate_type == DRRS_HIGH_RR) {
> +                       seq_puts(m, "DRRS_State: DRRS_HIGH_RR, ");
> +                       vrefresh = panel->fixed_mode->vrefresh;
> +               } else if (drrs->refresh_rate_type == DRRS_LOW_RR) {
> +                       seq_puts(m, "DRRS_State: DRRS_LOW_RR, ");
> +                       vrefresh = panel->downclock_mode->vrefresh;
> +               } else {
> +                       seq_printf(m, "DRRS_State: Unknown(%d), ",
> +                                               drrs->refresh_rate_type);
> +               }

I wonder what it is printed when DRRS is supported but is disabled?

Also why not print some info like enabled/disabled?

> +               seq_printf(m, "Vrefresh: %d", vrefresh);
> +
> +       } else {
> +               /* DRRS not supported. Print the VBT parameter*/

Why? Should be better a dmesg when enabling DRRS and print why not supported?

Is VBT only reason for not being supported? Is is information useless
when drrs is supported?

> +               seq_puts(m, "DRRS Supported : No, ");
> +               if (dev_priv->vbt.drrs_type == STATIC_DRRS_SUPPORT)
> +                       seq_puts(m, "VBT DRRS_type: Static");
> +               else if (dev_priv->vbt.drrs_type == SEAMLESS_DRRS_SUPPORT)
> +                       seq_puts(m, "VBT DRRS_type: Seamless");
> +               else if (dev_priv->vbt.drrs_type == DRRS_NOT_SUPPORTED)
> +                       seq_puts(m, "VBT DRRS_type: None");
> +               else
> +                       seq_puts(m, "VBT DRRS_type: Unrecognized Value");
> +       }
> +       seq_puts(m, "\n");
> +}
> +
> +static int i915_drrs_status(struct seq_file *m, void *unused)
> +{
> +       struct drm_info_node *node = m->private;
> +       struct drm_device *dev = node->minor->dev;
> +       struct intel_crtc *intel_crtc;
> +       int active_crtc_cnt = 0;
> +
> +       for_each_intel_crtc(dev, intel_crtc) {
> +               if (intel_crtc->active) {
> +                       active_crtc_cnt++;

isn't a bool enough?
why do you need a counter?

> +                       seq_printf(m, "CRTC %d:  ", active_crtc_cnt);
> +
> +                       drrs_status_per_crtc(m, dev, intel_crtc);

Since you are doing this for all active crtcs, have you considered
indenting all other information per crtc to be more clean when having
more than one active crtc?

> +               }
> +       }
> +
> +       if (!active_crtc_cnt)
> +               seq_puts(m, "No active crtc found\n");
> +
> +       return 0;
> +}
> +
>  struct pipe_crc_info {
>         const char *name;
>         struct drm_device *dev;
> @@ -4483,6 +4581,7 @@ static const struct drm_info_list i915_debugfs_list[] = {
>         {"i915_dp_mst_info", i915_dp_mst_info, 0},
>         {"i915_wa_registers", i915_wa_registers, 0},
>         {"i915_ddb_info", i915_ddb_info, 0},
> +       {"i915_drrs_status", i915_drrs_status, 0},
>  };
>  #define I915_DEBUGFS_ENTRIES ARRAY_SIZE(i915_debugfs_list)
>
> --
> 1.7.9.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 6/6] drm/i915: Add debugfs entry for DRRS

[Ramalingam C]

Hi,

On Friday 20 February 2015 12:15 AM, Rodrigo Vivi wrote:
> On Fri, Feb 13, 2015 at 2:03 AM, Ramalingam C <ramalingam.c@intel.com> wrote:
>> From: Vandana Kannan <vandana.kannan@intel.com>
>>
>> Adding a debugfs entry to determine if DRRS is supported or not
>>
>> V2: [By Ram]: Following details about the active crtc will be filled
>>          in seq-file of the debugfs
>>          1. Encoder output type
>>          2. DRRS Support on this CRTC
>>          3. DRRS current state
>>          4. Current Vrefresh
>> Format is as follows:
>> CRTC 1:  Output: eDP, DRRS Supported: Yes (Seamless), DRRS_State: DRRS_HIGH_RR, Vrefresh: 60
>> CRTC 2:  Output: HDMI, DRRS Supported : No, VBT DRRS_type: Seamless
>> CRTC 1:  Output: eDP, DRRS Supported: Yes (Seamless), DRRS_State: DRRS_LOW_RR, Vrefresh: 40
>> CRTC 2:  Output: HDMI, DRRS Supported : No, VBT DRRS_type: Seamless
>>
>> V3: [By Ram]: Readability is improved.
>>          Another error case is covered [Daniel]
>>
>> V4: [By Ram]: Current status of the Idleness DRRS along with
>>          the Front buffer bits are added to the debugfs. [Rodrigo]
>>
>> Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
>> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_debugfs.c |   99 +++++++++++++++++++++++++++++++++++
>>   1 file changed, 99 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
>> index 164fa82..e08d63f 100644
>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>> @@ -2869,6 +2869,104 @@ static int i915_ddb_info(struct seq_file *m, void *unused)
>>          return 0;
>>   }
>>
>> +static void drrs_status_per_crtc(struct seq_file *m,
>> +               struct drm_device *dev, struct intel_crtc *intel_crtc)
>> +{
>> +       struct intel_encoder *intel_encoder;
>> +       struct drm_i915_private *dev_priv = dev->dev_private;
>> +       struct i915_drrs *drrs = &dev_priv->drrs;
>> +       int vrefresh = 0;
>> +
>> +       for_each_encoder_on_crtc(dev, &intel_crtc->base, intel_encoder) {
>> +               /* Encoder connected on this CRTC */
> Do you really need this info here?
Better to have the encoder details
>
>> +               switch (intel_encoder->type) {
>> +               case INTEL_OUTPUT_EDP:
>> +                       seq_puts(m, "Output: eDP, ");
>> +                       break;
>> +               case INTEL_OUTPUT_DSI:
>> +                       seq_puts(m, "Output: DSI, ");
>> +                       break;
>> +               case INTEL_OUTPUT_HDMI:
>> +                       seq_puts(m, "Output: HDMI, ");
>> +                       break;
>> +               case INTEL_OUTPUT_DISPLAYPORT:
>> +                       seq_puts(m, "Output: DP, ");
>> +                       break;
>> +               default:
>> +                       seq_printf(m, "Output: Others (id=%d), ",
>> +                                               intel_encoder->type);
>> +               }
>> +       }
>> +
>> +       if (intel_crtc->config->has_drrs) {
>> +               struct intel_panel *panel;
>> +
>> +               panel = &drrs->dp->attached_connector->panel;
>> +               /* DRRS Supported */
>> +               seq_puts(m, "DRRS Supported: Yes (Seamless), ");
> isn't "Yes" enough? Remind that you might want to parse in the future.
Agreed. Yes alone will be simple.
>
>> +               seq_printf(m, "busy_frontbuffer_bits: 0x%X,\n\t",
>> +                                       drrs->busy_frontbuffer_bits);
>> +
>> +               if (drrs->busy_frontbuffer_bits) {
>> +                       seq_puts(m, "Front buffer: busy, ");
>> +                       seq_puts(m, "Idleness Timer: Suspended, ");
>> +               } else {
>> +                       seq_puts(m, "Front buffer: Idle, ");
>> +                       if (drrs->refresh_rate_type == DRRS_HIGH_RR)
>> +                               seq_puts(m, "Idleness Timer: Ticking, ");
>> +                       else
>> +                               seq_puts(m, "Idleness Timer: Suspended, ");
>> +               }
>> +
>> +               if (drrs->refresh_rate_type == DRRS_HIGH_RR) {
>> +                       seq_puts(m, "DRRS_State: DRRS_HIGH_RR, ");
>> +                       vrefresh = panel->fixed_mode->vrefresh;
>> +               } else if (drrs->refresh_rate_type == DRRS_LOW_RR) {
>> +                       seq_puts(m, "DRRS_State: DRRS_LOW_RR, ");
>> +                       vrefresh = panel->downclock_mode->vrefresh;
>> +               } else {
>> +                       seq_printf(m, "DRRS_State: Unknown(%d), ",
>> +                                               drrs->refresh_rate_type);
>> +               }
> I wonder what it is printed when DRRS is supported but is disabled?
It will be printing as "Idleness Timer: Suspended". I will rename this 
to remove this confusions :)
>
> Also why not print some info like enabled/disabled?
Sure we will do it.
>
>> +               seq_printf(m, "Vrefresh: %d", vrefresh);
>> +
>> +       } else {
>> +               /* DRRS not supported. Print the VBT parameter*/
> Why? Should be better a dmesg when enabling DRRS and print why not supported?
>
> Is VBT only reason for not being supported? Is is information useless
> when drrs is supported?
VBT details and also the lower refresh rate from EDID both are 
controlling factors for DRRS.
If DRRS is not supported only then VBT details are added so that modes 
can be verified along with. Its understood that VBT supports seamless 
type when DRRS is supported

>
>> +               seq_puts(m, "DRRS Supported : No, ");
>> +               if (dev_priv->vbt.drrs_type == STATIC_DRRS_SUPPORT)
>> +                       seq_puts(m, "VBT DRRS_type: Static");
>> +               else if (dev_priv->vbt.drrs_type == SEAMLESS_DRRS_SUPPORT)
>> +                       seq_puts(m, "VBT DRRS_type: Seamless");
>> +               else if (dev_priv->vbt.drrs_type == DRRS_NOT_SUPPORTED)
>> +                       seq_puts(m, "VBT DRRS_type: None");
>> +               else
>> +                       seq_puts(m, "VBT DRRS_type: Unrecognized Value");
>> +       }
>> +       seq_puts(m, "\n");
>> +}
>> +
>> +static int i915_drrs_status(struct seq_file *m, void *unused)
>> +{
>> +       struct drm_info_node *node = m->private;
>> +       struct drm_device *dev = node->minor->dev;
>> +       struct intel_crtc *intel_crtc;
>> +       int active_crtc_cnt = 0;
>> +
>> +       for_each_intel_crtc(dev, intel_crtc) {
>> +               if (intel_crtc->active) {
>> +                       active_crtc_cnt++;
> isn't a bool enough?
> why do you need a counter?
Actually we are disabling the DRRS in case of clone mode on Android.
So I added a counter for active CRTCs. So that we can inform about 
cloned mode.
But here it doesn't make any sense.
>
>> +                       seq_printf(m, "CRTC %d:  ", active_crtc_cnt);
>> +
>> +                       drrs_status_per_crtc(m, dev, intel_crtc);
> Since you are doing this for all active crtcs, have you considered
> indenting all other information per crtc to be more clean when having
> more than one active crtc?
Sure. we can make more presentable.
>
>> +               }
>> +       }
>> +
>> +       if (!active_crtc_cnt)
>> +               seq_puts(m, "No active crtc found\n");
>> +
>> +       return 0;
>> +}
>> +
>>   struct pipe_crc_info {
>>          const char *name;
>>          struct drm_device *dev;
>> @@ -4483,6 +4581,7 @@ static const struct drm_info_list i915_debugfs_list[] = {
>>          {"i915_dp_mst_info", i915_dp_mst_info, 0},
>>          {"i915_wa_registers", i915_wa_registers, 0},
>>          {"i915_ddb_info", i915_ddb_info, 0},
>> +       {"i915_drrs_status", i915_drrs_status, 0},
>>   };
>>   #define I915_DEBUGFS_ENTRIES ARRAY_SIZE(i915_debugfs_list)
>>
>> --
>> 1.7.9.5
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>
I will submit a patch for this by monday.

-- 
Ram

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/i915: Add debugfs entry for DRRS

[Ramalingam C]

From: Vandana Kannan <vandana.kannan@intel.com>

Adding a debugfs entry to determine if DRRS is supported or not

V2: [By Ram]: Following details about the active crtc will be filled
	in seq-file of the debugfs
	1. Encoder output type
	2. DRRS Support on this CRTC
	3. DRRS current state
	4. Current Vrefresh
Format is as follows:
CRTC 1:  Output: eDP, DRRS Supported: Yes (Seamless), DRRS_State: DRRS_HIGH_RR, Vrefresh: 60
CRTC 2:  Output: HDMI, DRRS Supported : No, VBT DRRS_type: Seamless
CRTC 1:  Output: eDP, DRRS Supported: Yes (Seamless), DRRS_State: DRRS_LOW_RR, Vrefresh: 40
CRTC 2:  Output: HDMI, DRRS Supported : No, VBT DRRS_type: Seamless

V3: [By Ram]: Readability is improved.
	Another error case is covered [Daniel]

V4: [By Ram]: Current status of the Idleness DRRS along with
	the Front buffer bits are added to the debugfs. [Rodrigo]

V5: [By Ram]: Rephrased to make it easy to understand.
	And format is modified. [Rodrigo]

Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c |  113 +++++++++++++++++++++++++++++++++++
 1 file changed, 113 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 164fa82..e51001c 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2869,6 +2869,118 @@ static int i915_ddb_info(struct seq_file *m, void *unused)
 	return 0;
 }
 
+static void drrs_status_per_crtc(struct seq_file *m,
+		struct drm_device *dev, struct intel_crtc *intel_crtc)
+{
+	struct intel_encoder *intel_encoder;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct i915_drrs *drrs = &dev_priv->drrs;
+	int vrefresh = 0;
+	u32 work_status;
+
+	for_each_encoder_on_crtc(dev, &intel_crtc->base, intel_encoder) {
+		/* Encoder connected on this CRTC */
+		switch (intel_encoder->type) {
+		case INTEL_OUTPUT_EDP:
+			seq_puts(m, "eDP:\n");
+			break;
+		case INTEL_OUTPUT_DSI:
+			seq_puts(m, "DSI:\n");
+			break;
+		case INTEL_OUTPUT_HDMI:
+			seq_puts(m, "HDMI:\n");
+			break;
+		case INTEL_OUTPUT_DISPLAYPORT:
+			seq_puts(m, "DP:\n");
+			break;
+		default:
+			seq_printf(m, "Other encoder (id=%d).\n",
+						intel_encoder->type);
+			return;
+		}
+	}
+
+	if (dev_priv->vbt.drrs_type == STATIC_DRRS_SUPPORT)
+		seq_puts(m, "\tVBT: DRRS_type: Static");
+	else if (dev_priv->vbt.drrs_type == SEAMLESS_DRRS_SUPPORT)
+		seq_puts(m, "\tVBT: DRRS_type: Seamless");
+	else if (dev_priv->vbt.drrs_type == DRRS_NOT_SUPPORTED)
+		seq_puts(m, "\tVBT: DRRS_type: None");
+	else
+		seq_puts(m, "\tVBT: DRRS_type: FIXME: Unrecognized Value");
+
+	seq_puts(m, "\n\n");
+
+	if (intel_crtc->config->has_drrs) {
+		struct intel_panel *panel;
+
+		panel = &drrs->dp->attached_connector->panel;
+		/* DRRS Supported */
+		seq_puts(m, "\tDRRS Supported: Yes\n");
+		seq_printf(m, "\t\tBusy_frontbuffer_bits: 0x%X",
+					drrs->busy_frontbuffer_bits);
+
+		seq_puts(m, "\n\t\t");
+		work_status = work_busy(&drrs->work.work);
+		if (drrs->busy_frontbuffer_bits) {
+			seq_puts(m, "Front buffer: Busy.\n");
+			seq_puts(m, "\t\tIdleness DRRS: Disabled");
+		} else {
+			seq_puts(m, "Front buffer: Idle");
+			seq_puts(m, "\n\t\t");
+			if (drrs->refresh_rate_type == DRRS_HIGH_RR) {
+				if (work_status)
+					seq_puts(m, "Idleness DRRS: Enabled");
+				else
+					seq_puts(m, "Idleness DRRS: Disabled");
+			} else if (drrs->refresh_rate_type == DRRS_LOW_RR) {
+				seq_puts(m, "Idleness DRRS: Enabled");
+			}
+		}
+
+		seq_puts(m, "\n\t\t");
+		if (drrs->refresh_rate_type == DRRS_HIGH_RR) {
+			seq_puts(m, "DRRS_State: DRRS_HIGH_RR\n");
+			vrefresh = panel->fixed_mode->vrefresh;
+		} else if (drrs->refresh_rate_type == DRRS_LOW_RR) {
+			seq_puts(m, "DRRS_State: DRRS_LOW_RR\n");
+			vrefresh = panel->downclock_mode->vrefresh;
+		} else {
+			seq_printf(m, "DRRS_State: Unknown(%d)\n",
+						drrs->refresh_rate_type);
+			return;
+		}
+		seq_printf(m, "\t\tVrefresh: %d", vrefresh);
+
+	} else {
+		/* DRRS not supported. Print the VBT parameter*/
+		seq_puts(m, "\tDRRS Supported : No");
+	}
+	seq_puts(m, "\n");
+}
+
+static int i915_drrs_status(struct seq_file *m, void *unused)
+{
+	struct drm_info_node *node = m->private;
+	struct drm_device *dev = node->minor->dev;
+	struct intel_crtc *intel_crtc;
+	int active_crtc_cnt = 0;
+
+	for_each_intel_crtc(dev, intel_crtc) {
+		if (intel_crtc->active) {
+			active_crtc_cnt++;
+			seq_printf(m, "\nCRTC %d:  ", active_crtc_cnt);
+
+			drrs_status_per_crtc(m, dev, intel_crtc);
+		}
+	}
+
+	if (!active_crtc_cnt)
+		seq_puts(m, "No active crtc found\n");
+
+	return 0;
+}
+
 struct pipe_crc_info {
 	const char *name;
 	struct drm_device *dev;
@@ -4483,6 +4595,7 @@ static const struct drm_info_list i915_debugfs_list[] = {
 	{"i915_dp_mst_info", i915_dp_mst_info, 0},
 	{"i915_wa_registers", i915_wa_registers, 0},
 	{"i915_ddb_info", i915_ddb_info, 0},
+	{"i915_drrs_status", i915_drrs_status, 0},
 };
 #define I915_DEBUGFS_ENTRIES ARRAY_SIZE(i915_debugfs_list)
 
-- 
1.7.9.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Add debugfs entry for DRRS

[Rodrigo Vivi]

On Mon, Feb 23, 2015 at 4:05 AM, Ramalingam C <ramalingam.c@intel.com> wrote:
> From: Vandana Kannan <vandana.kannan@intel.com>
>
> Adding a debugfs entry to determine if DRRS is supported or not
>
> V2: [By Ram]: Following details about the active crtc will be filled
>         in seq-file of the debugfs
>         1. Encoder output type
>         2. DRRS Support on this CRTC
>         3. DRRS current state
>         4. Current Vrefresh
> Format is as follows:
> CRTC 1:  Output: eDP, DRRS Supported: Yes (Seamless), DRRS_State: DRRS_HIGH_RR, Vrefresh: 60
> CRTC 2:  Output: HDMI, DRRS Supported : No, VBT DRRS_type: Seamless
> CRTC 1:  Output: eDP, DRRS Supported: Yes (Seamless), DRRS_State: DRRS_LOW_RR, Vrefresh: 40
> CRTC 2:  Output: HDMI, DRRS Supported : No, VBT DRRS_type: Seamless
>
> V3: [By Ram]: Readability is improved.
>         Another error case is covered [Daniel]
>
> V4: [By Ram]: Current status of the Idleness DRRS along with
>         the Front buffer bits are added to the debugfs. [Rodrigo]
>
> V5: [By Ram]: Rephrased to make it easy to understand.
>         And format is modified. [Rodrigo]
>
> Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c |  113 +++++++++++++++++++++++++++++++++++
>  1 file changed, 113 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 164fa82..e51001c 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2869,6 +2869,118 @@ static int i915_ddb_info(struct seq_file *m, void *unused)
>         return 0;
>  }
>
> +static void drrs_status_per_crtc(struct seq_file *m,
> +               struct drm_device *dev, struct intel_crtc *intel_crtc)
> +{
> +       struct intel_encoder *intel_encoder;
> +       struct drm_i915_private *dev_priv = dev->dev_private;
> +       struct i915_drrs *drrs = &dev_priv->drrs;
> +       int vrefresh = 0;
> +       u32 work_status;
> +
> +       for_each_encoder_on_crtc(dev, &intel_crtc->base, intel_encoder) {
> +               /* Encoder connected on this CRTC */
> +               switch (intel_encoder->type) {
> +               case INTEL_OUTPUT_EDP:
> +                       seq_puts(m, "eDP:\n");
> +                       break;
> +               case INTEL_OUTPUT_DSI:
> +                       seq_puts(m, "DSI:\n");
> +                       break;
> +               case INTEL_OUTPUT_HDMI:
> +                       seq_puts(m, "HDMI:\n");
> +                       break;
> +               case INTEL_OUTPUT_DISPLAYPORT:
> +                       seq_puts(m, "DP:\n");
> +                       break;
> +               default:
> +                       seq_printf(m, "Other encoder (id=%d).\n",
> +                                               intel_encoder->type);
> +                       return;
> +               }
> +       }
> +
> +       if (dev_priv->vbt.drrs_type == STATIC_DRRS_SUPPORT)
> +               seq_puts(m, "\tVBT: DRRS_type: Static");
> +       else if (dev_priv->vbt.drrs_type == SEAMLESS_DRRS_SUPPORT)
> +               seq_puts(m, "\tVBT: DRRS_type: Seamless");
> +       else if (dev_priv->vbt.drrs_type == DRRS_NOT_SUPPORTED)
> +               seq_puts(m, "\tVBT: DRRS_type: None");
> +       else
> +               seq_puts(m, "\tVBT: DRRS_type: FIXME: Unrecognized Value");
> +
> +       seq_puts(m, "\n\n");
> +
> +       if (intel_crtc->config->has_drrs) {
> +               struct intel_panel *panel;
> +
> +               panel = &drrs->dp->attached_connector->panel;
> +               /* DRRS Supported */
> +               seq_puts(m, "\tDRRS Supported: Yes\n");
> +               seq_printf(m, "\t\tBusy_frontbuffer_bits: 0x%X",
> +                                       drrs->busy_frontbuffer_bits);
> +
> +               seq_puts(m, "\n\t\t");
> +               work_status = work_busy(&drrs->work.work);
> +               if (drrs->busy_frontbuffer_bits) {
> +                       seq_puts(m, "Front buffer: Busy.\n");
> +                       seq_puts(m, "\t\tIdleness DRRS: Disabled");
> +               } else {
> +                       seq_puts(m, "Front buffer: Idle");
> +                       seq_puts(m, "\n\t\t");
> +                       if (drrs->refresh_rate_type == DRRS_HIGH_RR) {
> +                               if (work_status)

Why do you need to check work_busy here?

> +                                       seq_puts(m, "Idleness DRRS: Enabled");
> +                               else
> +                                       seq_puts(m, "Idleness DRRS: Disabled");
> +                       } else if (drrs->refresh_rate_type == DRRS_LOW_RR) {
> +                               seq_puts(m, "Idleness DRRS: Enabled");
> +                       }
> +               }
> +
> +               seq_puts(m, "\n\t\t");
> +               if (drrs->refresh_rate_type == DRRS_HIGH_RR) {
> +                       seq_puts(m, "DRRS_State: DRRS_HIGH_RR\n");
> +                       vrefresh = panel->fixed_mode->vrefresh;
> +               } else if (drrs->refresh_rate_type == DRRS_LOW_RR) {
> +                       seq_puts(m, "DRRS_State: DRRS_LOW_RR\n");
> +                       vrefresh = panel->downclock_mode->vrefresh;
> +               } else {
> +                       seq_printf(m, "DRRS_State: Unknown(%d)\n",
> +                                               drrs->refresh_rate_type);
> +                       return;
> +               }
> +               seq_printf(m, "\t\tVrefresh: %d", vrefresh);
> +
> +       } else {
> +               /* DRRS not supported. Print the VBT parameter*/
> +               seq_puts(m, "\tDRRS Supported : No");
> +       }
> +       seq_puts(m, "\n");
> +}
> +
> +static int i915_drrs_status(struct seq_file *m, void *unused)
> +{
> +       struct drm_info_node *node = m->private;
> +       struct drm_device *dev = node->minor->dev;
> +       struct intel_crtc *intel_crtc;
> +       int active_crtc_cnt = 0;
> +
> +       for_each_intel_crtc(dev, intel_crtc) {
> +               if (intel_crtc->active) {
> +                       active_crtc_cnt++;
> +                       seq_printf(m, "\nCRTC %d:  ", active_crtc_cnt);
> +
> +                       drrs_status_per_crtc(m, dev, intel_crtc);
> +               }
> +       }
> +
> +       if (!active_crtc_cnt)
> +               seq_puts(m, "No active crtc found\n");
> +
> +       return 0;
> +}
> +
>  struct pipe_crc_info {
>         const char *name;
>         struct drm_device *dev;
> @@ -4483,6 +4595,7 @@ static const struct drm_info_list i915_debugfs_list[] = {
>         {"i915_dp_mst_info", i915_dp_mst_info, 0},
>         {"i915_wa_registers", i915_wa_registers, 0},
>         {"i915_ddb_info", i915_ddb_info, 0},
> +       {"i915_drrs_status", i915_drrs_status, 0},
>  };
>  #define I915_DEBUGFS_ENTRIES ARRAY_SIZE(i915_debugfs_list)
>
> --
> 1.7.9.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Thanks for the changes.
With that explained feel free to use Reviewed-by: Rodrigo Vivi
<rodrigo.vivi@intel.com>

-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Add debugfs entry for DRRS

[Ramalingam C]

On Monday 23 February 2015 11:49 PM, Rodrigo Vivi wrote:
> On Mon, Feb 23, 2015 at 4:05 AM, Ramalingam C <ramalingam.c@intel.com> wrote:
>> From: Vandana Kannan <vandana.kannan@intel.com>
>>
>> Adding a debugfs entry to determine if DRRS is supported or not
>>
>> V2: [By Ram]: Following details about the active crtc will be filled
>>          in seq-file of the debugfs
>>          1. Encoder output type
>>          2. DRRS Support on this CRTC
>>          3. DRRS current state
>>          4. Current Vrefresh
>> Format is as follows:
>> CRTC 1:  Output: eDP, DRRS Supported: Yes (Seamless), DRRS_State: DRRS_HIGH_RR, Vrefresh: 60
>> CRTC 2:  Output: HDMI, DRRS Supported : No, VBT DRRS_type: Seamless
>> CRTC 1:  Output: eDP, DRRS Supported: Yes (Seamless), DRRS_State: DRRS_LOW_RR, Vrefresh: 40
>> CRTC 2:  Output: HDMI, DRRS Supported : No, VBT DRRS_type: Seamless
>>
>> V3: [By Ram]: Readability is improved.
>>          Another error case is covered [Daniel]
>>
>> V4: [By Ram]: Current status of the Idleness DRRS along with
>>          the Front buffer bits are added to the debugfs. [Rodrigo]
>>
>> V5: [By Ram]: Rephrased to make it easy to understand.
>>          And format is modified. [Rodrigo]
>>
>> Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
>> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_debugfs.c |  113 +++++++++++++++++++++++++++++++++++
>>   1 file changed, 113 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
>> index 164fa82..e51001c 100644
>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>> @@ -2869,6 +2869,118 @@ static int i915_ddb_info(struct seq_file *m, void *unused)
>>          return 0;
>>   }
>>
>> +static void drrs_status_per_crtc(struct seq_file *m,
>> +               struct drm_device *dev, struct intel_crtc *intel_crtc)
>> +{
>> +       struct intel_encoder *intel_encoder;
>> +       struct drm_i915_private *dev_priv = dev->dev_private;
>> +       struct i915_drrs *drrs = &dev_priv->drrs;
>> +       int vrefresh = 0;
>> +       u32 work_status;
>> +
>> +       for_each_encoder_on_crtc(dev, &intel_crtc->base, intel_encoder) {
>> +               /* Encoder connected on this CRTC */
>> +               switch (intel_encoder->type) {
>> +               case INTEL_OUTPUT_EDP:
>> +                       seq_puts(m, "eDP:\n");
>> +                       break;
>> +               case INTEL_OUTPUT_DSI:
>> +                       seq_puts(m, "DSI:\n");
>> +                       break;
>> +               case INTEL_OUTPUT_HDMI:
>> +                       seq_puts(m, "HDMI:\n");
>> +                       break;
>> +               case INTEL_OUTPUT_DISPLAYPORT:
>> +                       seq_puts(m, "DP:\n");
>> +                       break;
>> +               default:
>> +                       seq_printf(m, "Other encoder (id=%d).\n",
>> +                                               intel_encoder->type);
>> +                       return;
>> +               }
>> +       }
>> +
>> +       if (dev_priv->vbt.drrs_type == STATIC_DRRS_SUPPORT)
>> +               seq_puts(m, "\tVBT: DRRS_type: Static");
>> +       else if (dev_priv->vbt.drrs_type == SEAMLESS_DRRS_SUPPORT)
>> +               seq_puts(m, "\tVBT: DRRS_type: Seamless");
>> +       else if (dev_priv->vbt.drrs_type == DRRS_NOT_SUPPORTED)
>> +               seq_puts(m, "\tVBT: DRRS_type: None");
>> +       else
>> +               seq_puts(m, "\tVBT: DRRS_type: FIXME: Unrecognized Value");
>> +
>> +       seq_puts(m, "\n\n");
>> +
>> +       if (intel_crtc->config->has_drrs) {
>> +               struct intel_panel *panel;
>> +
>> +               panel = &drrs->dp->attached_connector->panel;
>> +               /* DRRS Supported */
>> +               seq_puts(m, "\tDRRS Supported: Yes\n");
>> +               seq_printf(m, "\t\tBusy_frontbuffer_bits: 0x%X",
>> +                                       drrs->busy_frontbuffer_bits);
>> +
>> +               seq_puts(m, "\n\t\t");
>> +               work_status = work_busy(&drrs->work.work);
>> +               if (drrs->busy_frontbuffer_bits) {
>> +                       seq_puts(m, "Front buffer: Busy.\n");
>> +                       seq_puts(m, "\t\tIdleness DRRS: Disabled");
>> +               } else {
>> +                       seq_puts(m, "Front buffer: Idle");
>> +                       seq_puts(m, "\n\t\t");
>> +                       if (drrs->refresh_rate_type == DRRS_HIGH_RR) {
>> +                               if (work_status)
> Why do you need to check work_busy here?
This is to capture the DRRS disabled state due to function 
intel_edp_drrs_disable.
>
>> +                                       seq_puts(m, "Idleness DRRS: Enabled");
>> +                               else
>> +                                       seq_puts(m, "Idleness DRRS: Disabled");
>> +                       } else if (drrs->refresh_rate_type == DRRS_LOW_RR) {
>> +                               seq_puts(m, "Idleness DRRS: Enabled");
>> +                       }
>> +               }
>> +
>> +               seq_puts(m, "\n\t\t");
>> +               if (drrs->refresh_rate_type == DRRS_HIGH_RR) {
>> +                       seq_puts(m, "DRRS_State: DRRS_HIGH_RR\n");
>> +                       vrefresh = panel->fixed_mode->vrefresh;
>> +               } else if (drrs->refresh_rate_type == DRRS_LOW_RR) {
>> +                       seq_puts(m, "DRRS_State: DRRS_LOW_RR\n");
>> +                       vrefresh = panel->downclock_mode->vrefresh;
>> +               } else {
>> +                       seq_printf(m, "DRRS_State: Unknown(%d)\n",
>> +                                               drrs->refresh_rate_type);
>> +                       return;
>> +               }
>> +               seq_printf(m, "\t\tVrefresh: %d", vrefresh);
>> +
>> +       } else {
>> +               /* DRRS not supported. Print the VBT parameter*/
>> +               seq_puts(m, "\tDRRS Supported : No");
>> +       }
>> +       seq_puts(m, "\n");
>> +}
>> +
>> +static int i915_drrs_status(struct seq_file *m, void *unused)
>> +{
>> +       struct drm_info_node *node = m->private;
>> +       struct drm_device *dev = node->minor->dev;
>> +       struct intel_crtc *intel_crtc;
>> +       int active_crtc_cnt = 0;
>> +
>> +       for_each_intel_crtc(dev, intel_crtc) {
>> +               if (intel_crtc->active) {
>> +                       active_crtc_cnt++;
>> +                       seq_printf(m, "\nCRTC %d:  ", active_crtc_cnt);
>> +
>> +                       drrs_status_per_crtc(m, dev, intel_crtc);
>> +               }
>> +       }
>> +
>> +       if (!active_crtc_cnt)
>> +               seq_puts(m, "No active crtc found\n");
>> +
>> +       return 0;
>> +}
>> +
>>   struct pipe_crc_info {
>>          const char *name;
>>          struct drm_device *dev;
>> @@ -4483,6 +4595,7 @@ static const struct drm_info_list i915_debugfs_list[] = {
>>          {"i915_dp_mst_info", i915_dp_mst_info, 0},
>>          {"i915_wa_registers", i915_wa_registers, 0},
>>          {"i915_ddb_info", i915_ddb_info, 0},
>> +       {"i915_drrs_status", i915_drrs_status, 0},
>>   };
>>   #define I915_DEBUGFS_ENTRIES ARRAY_SIZE(i915_debugfs_list)
>>
>> --
>> 1.7.9.5
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> Thanks for the changes.
> With that explained feel free to use Reviewed-by: Rodrigo Vivi
> <rodrigo.vivi@intel.com>

-- 
Ram

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Add debugfs entry for DRRS

[Daniel Vetter]

On Mon, Feb 23, 2015 at 05:35:54PM +0530, Ramalingam C wrote:
> From: Vandana Kannan <vandana.kannan@intel.com>
> 
> Adding a debugfs entry to determine if DRRS is supported or not
> 
> V2: [By Ram]: Following details about the active crtc will be filled
> 	in seq-file of the debugfs
> 	1. Encoder output type
> 	2. DRRS Support on this CRTC
> 	3. DRRS current state
> 	4. Current Vrefresh
> Format is as follows:
> CRTC 1:  Output: eDP, DRRS Supported: Yes (Seamless), DRRS_State: DRRS_HIGH_RR, Vrefresh: 60
> CRTC 2:  Output: HDMI, DRRS Supported : No, VBT DRRS_type: Seamless
> CRTC 1:  Output: eDP, DRRS Supported: Yes (Seamless), DRRS_State: DRRS_LOW_RR, Vrefresh: 40
> CRTC 2:  Output: HDMI, DRRS Supported : No, VBT DRRS_type: Seamless
> 
> V3: [By Ram]: Readability is improved.
> 	Another error case is covered [Daniel]
> 
> V4: [By Ram]: Current status of the Idleness DRRS along with
> 	the Front buffer bits are added to the debugfs. [Rodrigo]
> 
> V5: [By Ram]: Rephrased to make it easy to understand.
> 	And format is modified. [Rodrigo]
> 
> Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>

Total absence of locking while walking modesetting structures is a bit
uncool.

> ---
>  drivers/gpu/drm/i915/i915_debugfs.c |  113 +++++++++++++++++++++++++++++++++++
>  1 file changed, 113 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 164fa82..e51001c 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2869,6 +2869,118 @@ static int i915_ddb_info(struct seq_file *m, void *unused)
>  	return 0;
>  }
>  
> +static void drrs_status_per_crtc(struct seq_file *m,
> +		struct drm_device *dev, struct intel_crtc *intel_crtc)
> +{
> +	struct intel_encoder *intel_encoder;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct i915_drrs *drrs = &dev_priv->drrs;
> +	int vrefresh = 0;
> +	u32 work_status;
> +
> +	for_each_encoder_on_crtc(dev, &intel_crtc->base, intel_encoder) {
> +		/* Encoder connected on this CRTC */
> +		switch (intel_encoder->type) {
> +		case INTEL_OUTPUT_EDP:
> +			seq_puts(m, "eDP:\n");
> +			break;
> +		case INTEL_OUTPUT_DSI:
> +			seq_puts(m, "DSI:\n");
> +			break;
> +		case INTEL_OUTPUT_HDMI:
> +			seq_puts(m, "HDMI:\n");
> +			break;
> +		case INTEL_OUTPUT_DISPLAYPORT:
> +			seq_puts(m, "DP:\n");
> +			break;
> +		default:
> +			seq_printf(m, "Other encoder (id=%d).\n",
> +						intel_encoder->type);
> +			return;
> +		}
> +	}
> +
> +	if (dev_priv->vbt.drrs_type == STATIC_DRRS_SUPPORT)
> +		seq_puts(m, "\tVBT: DRRS_type: Static");
> +	else if (dev_priv->vbt.drrs_type == SEAMLESS_DRRS_SUPPORT)
> +		seq_puts(m, "\tVBT: DRRS_type: Seamless");
> +	else if (dev_priv->vbt.drrs_type == DRRS_NOT_SUPPORTED)
> +		seq_puts(m, "\tVBT: DRRS_type: None");
> +	else
> +		seq_puts(m, "\tVBT: DRRS_type: FIXME: Unrecognized Value");
> +
> +	seq_puts(m, "\n\n");
> +
> +	if (intel_crtc->config->has_drrs) {
> +		struct intel_panel *panel;
> +
> +		panel = &drrs->dp->attached_connector->panel;

Same here, chasing drrs pointers without grabbing the right locks isn't
awesome either. I've merged all the other patches in this series to dinq.
-Daniel

> +		/* DRRS Supported */
> +		seq_puts(m, "\tDRRS Supported: Yes\n");
> +		seq_printf(m, "\t\tBusy_frontbuffer_bits: 0x%X",
> +					drrs->busy_frontbuffer_bits);
> +
> +		seq_puts(m, "\n\t\t");
> +		work_status = work_busy(&drrs->work.work);
> +		if (drrs->busy_frontbuffer_bits) {
> +			seq_puts(m, "Front buffer: Busy.\n");
> +			seq_puts(m, "\t\tIdleness DRRS: Disabled");
> +		} else {
> +			seq_puts(m, "Front buffer: Idle");
> +			seq_puts(m, "\n\t\t");
> +			if (drrs->refresh_rate_type == DRRS_HIGH_RR) {
> +				if (work_status)
> +					seq_puts(m, "Idleness DRRS: Enabled");
> +				else
> +					seq_puts(m, "Idleness DRRS: Disabled");
> +			} else if (drrs->refresh_rate_type == DRRS_LOW_RR) {
> +				seq_puts(m, "Idleness DRRS: Enabled");
> +			}
> +		}
> +
> +		seq_puts(m, "\n\t\t");
> +		if (drrs->refresh_rate_type == DRRS_HIGH_RR) {
> +			seq_puts(m, "DRRS_State: DRRS_HIGH_RR\n");
> +			vrefresh = panel->fixed_mode->vrefresh;
> +		} else if (drrs->refresh_rate_type == DRRS_LOW_RR) {
> +			seq_puts(m, "DRRS_State: DRRS_LOW_RR\n");
> +			vrefresh = panel->downclock_mode->vrefresh;
> +		} else {
> +			seq_printf(m, "DRRS_State: Unknown(%d)\n",
> +						drrs->refresh_rate_type);
> +			return;
> +		}
> +		seq_printf(m, "\t\tVrefresh: %d", vrefresh);
> +
> +	} else {
> +		/* DRRS not supported. Print the VBT parameter*/
> +		seq_puts(m, "\tDRRS Supported : No");
> +	}
> +	seq_puts(m, "\n");
> +}
> +
> +static int i915_drrs_status(struct seq_file *m, void *unused)
> +{
> +	struct drm_info_node *node = m->private;
> +	struct drm_device *dev = node->minor->dev;
> +	struct intel_crtc *intel_crtc;
> +	int active_crtc_cnt = 0;
> +
> +	for_each_intel_crtc(dev, intel_crtc) {
> +		if (intel_crtc->active) {
> +			active_crtc_cnt++;
> +			seq_printf(m, "\nCRTC %d:  ", active_crtc_cnt);
> +
> +			drrs_status_per_crtc(m, dev, intel_crtc);
> +		}
> +	}
> +
> +	if (!active_crtc_cnt)
> +		seq_puts(m, "No active crtc found\n");
> +
> +	return 0;
> +}
> +
>  struct pipe_crc_info {
>  	const char *name;
>  	struct drm_device *dev;
> @@ -4483,6 +4595,7 @@ static const struct drm_info_list i915_debugfs_list[] = {
>  	{"i915_dp_mst_info", i915_dp_mst_info, 0},
>  	{"i915_wa_registers", i915_wa_registers, 0},
>  	{"i915_ddb_info", i915_ddb_info, 0},
> +	{"i915_drrs_status", i915_drrs_status, 0},
>  };
>  #define I915_DEBUGFS_ENTRIES ARRAY_SIZE(i915_debugfs_list)
>  
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Add debugfs entry for DRRS

[Ramalingam C]

On Tuesday 24 February 2015 06:09 AM, Daniel Vetter wrote:
> On Mon, Feb 23, 2015 at 05:35:54PM +0530, Ramalingam C wrote:
>> From: Vandana Kannan <vandana.kannan@intel.com>
>>
>> Adding a debugfs entry to determine if DRRS is supported or not
>>
>> V2: [By Ram]: Following details about the active crtc will be filled
>> 	in seq-file of the debugfs
>> 	1. Encoder output type
>> 	2. DRRS Support on this CRTC
>> 	3. DRRS current state
>> 	4. Current Vrefresh
>> Format is as follows:
>> CRTC 1:  Output: eDP, DRRS Supported: Yes (Seamless), DRRS_State: DRRS_HIGH_RR, Vrefresh: 60
>> CRTC 2:  Output: HDMI, DRRS Supported : No, VBT DRRS_type: Seamless
>> CRTC 1:  Output: eDP, DRRS Supported: Yes (Seamless), DRRS_State: DRRS_LOW_RR, Vrefresh: 40
>> CRTC 2:  Output: HDMI, DRRS Supported : No, VBT DRRS_type: Seamless
>>
>> V3: [By Ram]: Readability is improved.
>> 	Another error case is covered [Daniel]
>>
>> V4: [By Ram]: Current status of the Idleness DRRS along with
>> 	the Front buffer bits are added to the debugfs. [Rodrigo]
>>
>> V5: [By Ram]: Rephrased to make it easy to understand.
>> 	And format is modified. [Rodrigo]
>>
>> Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
>> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> Total absence of locking while walking modesetting structures is a bit
> uncool.
>
>> ---
>>   drivers/gpu/drm/i915/i915_debugfs.c |  113 +++++++++++++++++++++++++++++++++++
>>   1 file changed, 113 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
>> index 164fa82..e51001c 100644
>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>> @@ -2869,6 +2869,118 @@ static int i915_ddb_info(struct seq_file *m, void *unused)
>>   	return 0;
>>   }
>>   
>> +static void drrs_status_per_crtc(struct seq_file *m,
>> +		struct drm_device *dev, struct intel_crtc *intel_crtc)
>> +{
>> +	struct intel_encoder *intel_encoder;
>> +	struct drm_i915_private *dev_priv = dev->dev_private;
>> +	struct i915_drrs *drrs = &dev_priv->drrs;
>> +	int vrefresh = 0;
>> +	u32 work_status;
>> +
>> +	for_each_encoder_on_crtc(dev, &intel_crtc->base, intel_encoder) {
>> +		/* Encoder connected on this CRTC */
>> +		switch (intel_encoder->type) {
>> +		case INTEL_OUTPUT_EDP:
>> +			seq_puts(m, "eDP:\n");
>> +			break;
>> +		case INTEL_OUTPUT_DSI:
>> +			seq_puts(m, "DSI:\n");
>> +			break;
>> +		case INTEL_OUTPUT_HDMI:
>> +			seq_puts(m, "HDMI:\n");
>> +			break;
>> +		case INTEL_OUTPUT_DISPLAYPORT:
>> +			seq_puts(m, "DP:\n");
>> +			break;
>> +		default:
>> +			seq_printf(m, "Other encoder (id=%d).\n",
>> +						intel_encoder->type);
>> +			return;
>> +		}
>> +	}
>> +
>> +	if (dev_priv->vbt.drrs_type == STATIC_DRRS_SUPPORT)
>> +		seq_puts(m, "\tVBT: DRRS_type: Static");
>> +	else if (dev_priv->vbt.drrs_type == SEAMLESS_DRRS_SUPPORT)
>> +		seq_puts(m, "\tVBT: DRRS_type: Seamless");
>> +	else if (dev_priv->vbt.drrs_type == DRRS_NOT_SUPPORTED)
>> +		seq_puts(m, "\tVBT: DRRS_type: None");
>> +	else
>> +		seq_puts(m, "\tVBT: DRRS_type: FIXME: Unrecognized Value");
>> +
>> +	seq_puts(m, "\n\n");
>> +
>> +	if (intel_crtc->config->has_drrs) {
>> +		struct intel_panel *panel;
>> +
>> +		panel = &drrs->dp->attached_connector->panel;
> Same here, chasing drrs pointers without grabbing the right locks isn't
> awesome either. I've merged all the other patches in this series to dinq.
> -Daniel
>
>> +		/* DRRS Supported */
>> +		seq_puts(m, "\tDRRS Supported: Yes\n");
>> +		seq_printf(m, "\t\tBusy_frontbuffer_bits: 0x%X",
>> +					drrs->busy_frontbuffer_bits);
>> +
>> +		seq_puts(m, "\n\t\t");
>> +		work_status = work_busy(&drrs->work.work);
>> +		if (drrs->busy_frontbuffer_bits) {
>> +			seq_puts(m, "Front buffer: Busy.\n");
>> +			seq_puts(m, "\t\tIdleness DRRS: Disabled");
>> +		} else {
>> +			seq_puts(m, "Front buffer: Idle");
>> +			seq_puts(m, "\n\t\t");
>> +			if (drrs->refresh_rate_type == DRRS_HIGH_RR) {
>> +				if (work_status)
>> +					seq_puts(m, "Idleness DRRS: Enabled");
>> +				else
>> +					seq_puts(m, "Idleness DRRS: Disabled");
>> +			} else if (drrs->refresh_rate_type == DRRS_LOW_RR) {
>> +				seq_puts(m, "Idleness DRRS: Enabled");
>> +			}
>> +		}
>> +
>> +		seq_puts(m, "\n\t\t");
>> +		if (drrs->refresh_rate_type == DRRS_HIGH_RR) {
>> +			seq_puts(m, "DRRS_State: DRRS_HIGH_RR\n");
>> +			vrefresh = panel->fixed_mode->vrefresh;
>> +		} else if (drrs->refresh_rate_type == DRRS_LOW_RR) {
>> +			seq_puts(m, "DRRS_State: DRRS_LOW_RR\n");
>> +			vrefresh = panel->downclock_mode->vrefresh;
>> +		} else {
>> +			seq_printf(m, "DRRS_State: Unknown(%d)\n",
>> +						drrs->refresh_rate_type);
>> +			return;
>> +		}
>> +		seq_printf(m, "\t\tVrefresh: %d", vrefresh);
>> +
>> +	} else {
>> +		/* DRRS not supported. Print the VBT parameter*/
>> +		seq_puts(m, "\tDRRS Supported : No");
>> +	}
>> +	seq_puts(m, "\n");
>> +}
>> +
>> +static int i915_drrs_status(struct seq_file *m, void *unused)
>> +{
>> +	struct drm_info_node *node = m->private;
>> +	struct drm_device *dev = node->minor->dev;
>> +	struct intel_crtc *intel_crtc;
>> +	int active_crtc_cnt = 0;
>> +
>> +	for_each_intel_crtc(dev, intel_crtc) {
>> +		if (intel_crtc->active) {
>> +			active_crtc_cnt++;
>> +			seq_printf(m, "\nCRTC %d:  ", active_crtc_cnt);
>> +
>> +			drrs_status_per_crtc(m, dev, intel_crtc);
>> +		}
>> +	}
>> +
>> +	if (!active_crtc_cnt)
>> +		seq_puts(m, "No active crtc found\n");
>> +
>> +	return 0;
>> +}
>> +
>>   struct pipe_crc_info {
>>   	const char *name;
>>   	struct drm_device *dev;
>> @@ -4483,6 +4595,7 @@ static const struct drm_info_list i915_debugfs_list[] = {
>>   	{"i915_dp_mst_info", i915_dp_mst_info, 0},
>>   	{"i915_wa_registers", i915_wa_registers, 0},
>>   	{"i915_ddb_info", i915_ddb_info, 0},
>> +	{"i915_drrs_status", i915_drrs_status, 0},
>>   };
>>   #define I915_DEBUGFS_ENTRIES ARRAY_SIZE(i915_debugfs_list)
>>   
>> -- 
>> 1.7.9.5
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Sorry for missing mutex protection. I will modify accordingly. Thanks 
for the review daniel.

-- 
Ram

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/i915: Add debugfs entry for DRRS

[Ramalingam C]

From: Vandana Kannan <vandana.kannan@intel.com>

Adding a debugfs entry to determine if DRRS is supported or not

V2: [By Ram]: Following details about the active crtc will be filled
	in seq-file of the debugfs
	1. Encoder output type
	2. DRRS Support on this CRTC
	3. DRRS current state
	4. Current Vrefresh
Format is as follows:
CRTC 1:  Output: eDP, DRRS Supported: Yes (Seamless), DRRS_State: DRRS_HIGH_RR, Vrefresh: 60
CRTC 2:  Output: HDMI, DRRS Supported : No, VBT DRRS_type: Seamless
CRTC 1:  Output: eDP, DRRS Supported: Yes (Seamless), DRRS_State: DRRS_LOW_RR, Vrefresh: 40
CRTC 2:  Output: HDMI, DRRS Supported : No, VBT DRRS_type: Seamless

V3: [By Ram]: Readability is improved.
	Another error case is covered [Daniel]

V4: [By Ram]: Current status of the Idleness DRRS along with
	the Front buffer bits are added to the debugfs. [Rodrigo]

V5: [By Ram]: Rephrased to make it easy to understand.
	And format is modified. [Rodrigo]

V6: [By Ram]: Modeset mutex are acquired for each crtc along with
	renaming the Idleness detection states  [Daniel]

Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c |  141 +++++++++++++++++++++++++++++++++++
 1 file changed, 141 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 94b3984..90e56ca 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2870,6 +2870,146 @@ static int i915_ddb_info(struct seq_file *m, void *unused)
 	return 0;
 }
 
+static void drrs_status_per_crtc(struct seq_file *m,
+		struct drm_device *dev, struct intel_crtc *intel_crtc)
+{
+	struct intel_encoder *intel_encoder;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct i915_drrs *drrs = &dev_priv->drrs;
+	int vrefresh = 0;
+	u32 work_status;
+
+	for_each_encoder_on_crtc(dev, &intel_crtc->base, intel_encoder) {
+		/* Encoder connected on this CRTC */
+		switch (intel_encoder->type) {
+		case INTEL_OUTPUT_EDP:
+			seq_puts(m, "eDP:\n");
+			break;
+		case INTEL_OUTPUT_DSI:
+			seq_puts(m, "DSI:\n");
+			break;
+		case INTEL_OUTPUT_HDMI:
+			seq_puts(m, "HDMI:\n");
+			break;
+		case INTEL_OUTPUT_DISPLAYPORT:
+			seq_puts(m, "DP:\n");
+			break;
+		default:
+			seq_printf(m, "Other encoder (id=%d).\n",
+						intel_encoder->type);
+			return;
+		}
+	}
+
+	if (dev_priv->vbt.drrs_type == STATIC_DRRS_SUPPORT)
+		seq_puts(m, "\tVBT: DRRS_type: Static");
+	else if (dev_priv->vbt.drrs_type == SEAMLESS_DRRS_SUPPORT)
+		seq_puts(m, "\tVBT: DRRS_type: Seamless");
+	else if (dev_priv->vbt.drrs_type == DRRS_NOT_SUPPORTED)
+		seq_puts(m, "\tVBT: DRRS_type: None");
+	else
+		seq_puts(m, "\tVBT: DRRS_type: FIXME: Unrecognized Value");
+
+	seq_puts(m, "\n\n");
+
+	/*
+	 * Idleness DRRS detection states:
+	 *	Enabled   :	Idleness detection is active. When system is
+	 *			Idle for the defined duration DRRS_LOW_RR
+	 *			will be set. Or Idleness is already detected
+	 *			and DRRS_LOW_RR is applied.
+	 *	Suspended :	Due to frontbuffer's busy state, Idleness
+	 *			detection is suspended.
+	 *	Disabled  :	Idleness detection is disabled until a call is
+	 *			made to enable. No encoder pointer will be
+	 *			available.
+	 */
+	if (intel_crtc->config->has_drrs) {
+		struct intel_panel *panel;
+
+		mutex_lock(&drrs->mutex);
+		/* DRRS Supported */
+		seq_puts(m, "\tDRRS Supported: Yes\n");
+
+		/* disable_drrs() will make drrs->dp NULL */
+		if (!drrs->dp) {
+			seq_puts(m, "Idleness DRRS: Disabled");
+			mutex_unlock(&drrs->mutex);
+			return;
+		}
+
+		panel = &drrs->dp->attached_connector->panel;
+		seq_printf(m, "\t\tBusy_frontbuffer_bits: 0x%X",
+					drrs->busy_frontbuffer_bits);
+
+		seq_puts(m, "\n\t\t");
+		if (drrs->refresh_rate_type == DRRS_HIGH_RR) {
+			seq_puts(m, "DRRS_State: DRRS_HIGH_RR\n");
+			vrefresh = panel->fixed_mode->vrefresh;
+		} else if (drrs->refresh_rate_type == DRRS_LOW_RR) {
+			seq_puts(m, "DRRS_State: DRRS_LOW_RR\n");
+			vrefresh = panel->downclock_mode->vrefresh;
+		} else {
+			seq_printf(m, "DRRS_State: Unknown(%d)\n",
+						drrs->refresh_rate_type);
+			mutex_unlock(&drrs->mutex);
+			return;
+		}
+		seq_printf(m, "\t\tVrefresh: %d", vrefresh);
+
+		seq_puts(m, "\n\t\t");
+		work_status = work_busy(&drrs->work.work);
+		if (drrs->busy_frontbuffer_bits) {
+			seq_puts(m, "Front buffer: Busy.\n");
+			seq_puts(m, "\t\tIdleness DRRS: Suspended");
+		} else {
+			seq_puts(m, "Front buffer: Idle");
+			seq_puts(m, "\n\t\t");
+			if (drrs->refresh_rate_type == DRRS_HIGH_RR) {
+				if (work_status) {
+					seq_puts(m, "Idleness DRRS: Enabled");
+				} else {
+					seq_puts(m, "Idleness DRRS: Suspended.");
+					seq_puts(m, " FIXME: Shouldn't be here");
+				}
+			} else if (drrs->refresh_rate_type == DRRS_LOW_RR) {
+				seq_puts(m, "Idleness DRRS: Enabled");
+			}
+		}
+		mutex_unlock(&drrs->mutex);
+	} else {
+		/* DRRS not supported. Print the VBT parameter*/
+		seq_puts(m, "\tDRRS Supported : No");
+	}
+	seq_puts(m, "\n");
+}
+
+static int i915_drrs_status(struct seq_file *m, void *unused)
+{
+	struct drm_info_node *node = m->private;
+	struct drm_device *dev = node->minor->dev;
+	struct intel_crtc *intel_crtc;
+	int active_crtc_cnt = 0;
+
+	for_each_intel_crtc(dev, intel_crtc) {
+		drm_modeset_lock(&intel_crtc->base.mutex, NULL);
+
+		if (intel_crtc->active) {
+			active_crtc_cnt++;
+			seq_printf(m, "\nCRTC %d:  ", active_crtc_cnt);
+
+			drrs_status_per_crtc(m, dev, intel_crtc);
+		}
+
+		drm_modeset_unlock(&intel_crtc->base.mutex);
+	}
+
+	if (!active_crtc_cnt)
+		seq_puts(m, "No active crtc found\n");
+
+	return 0;
+}
+
 struct pipe_crc_info {
 	const char *name;
 	struct drm_device *dev;
@@ -4548,6 +4688,7 @@ static const struct drm_info_list i915_debugfs_list[] = {
 	{"i915_wa_registers", i915_wa_registers, 0},
 	{"i915_ddb_info", i915_ddb_info, 0},
 	{"i915_sseu_status", i915_sseu_status, 0},
+	{"i915_drrs_status", i915_drrs_status, 0},
 };
 #define I915_DEBUGFS_ENTRIES ARRAY_SIZE(i915_debugfs_list)
 
-- 
1.7.9.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Add debugfs entry for DRRS

[Rodrigo Vivi]

On Tue, Mar 3, 2015 at 7:23 AM, Ramalingam C <ramalingam.c@intel.com> wrote:
> From: Vandana Kannan <vandana.kannan@intel.com>
>
> Adding a debugfs entry to determine if DRRS is supported or not
>
> V2: [By Ram]: Following details about the active crtc will be filled
>         in seq-file of the debugfs
>         1. Encoder output type
>         2. DRRS Support on this CRTC
>         3. DRRS current state
>         4. Current Vrefresh
> Format is as follows:
> CRTC 1:  Output: eDP, DRRS Supported: Yes (Seamless), DRRS_State: DRRS_HIGH_RR, Vrefresh: 60
> CRTC 2:  Output: HDMI, DRRS Supported : No, VBT DRRS_type: Seamless
> CRTC 1:  Output: eDP, DRRS Supported: Yes (Seamless), DRRS_State: DRRS_LOW_RR, Vrefresh: 40
> CRTC 2:  Output: HDMI, DRRS Supported : No, VBT DRRS_type: Seamless
>
> V3: [By Ram]: Readability is improved.
>         Another error case is covered [Daniel]
>
> V4: [By Ram]: Current status of the Idleness DRRS along with
>         the Front buffer bits are added to the debugfs. [Rodrigo]
>
> V5: [By Ram]: Rephrased to make it easy to understand.
>         And format is modified. [Rodrigo]
>
> V6: [By Ram]: Modeset mutex are acquired for each crtc along with
>         renaming the Idleness detection states  [Daniel]
>
> Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c |  141 +++++++++++++++++++++++++++++++++++
>  1 file changed, 141 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 94b3984..90e56ca 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2870,6 +2870,146 @@ static int i915_ddb_info(struct seq_file *m, void *unused)
>         return 0;
>  }
>
> +static void drrs_status_per_crtc(struct seq_file *m,
> +               struct drm_device *dev, struct intel_crtc *intel_crtc)
> +{
> +       struct intel_encoder *intel_encoder;
> +       struct drm_i915_private *dev_priv = dev->dev_private;
> +       struct i915_drrs *drrs = &dev_priv->drrs;
> +       int vrefresh = 0;
> +       u32 work_status;
> +
> +       for_each_encoder_on_crtc(dev, &intel_crtc->base, intel_encoder) {
> +               /* Encoder connected on this CRTC */
> +               switch (intel_encoder->type) {
> +               case INTEL_OUTPUT_EDP:
> +                       seq_puts(m, "eDP:\n");
> +                       break;
> +               case INTEL_OUTPUT_DSI:
> +                       seq_puts(m, "DSI:\n");
> +                       break;
> +               case INTEL_OUTPUT_HDMI:
> +                       seq_puts(m, "HDMI:\n");
> +                       break;
> +               case INTEL_OUTPUT_DISPLAYPORT:
> +                       seq_puts(m, "DP:\n");
> +                       break;
> +               default:
> +                       seq_printf(m, "Other encoder (id=%d).\n",
> +                                               intel_encoder->type);
> +                       return;
> +               }
> +       }
> +
> +       if (dev_priv->vbt.drrs_type == STATIC_DRRS_SUPPORT)
> +               seq_puts(m, "\tVBT: DRRS_type: Static");
> +       else if (dev_priv->vbt.drrs_type == SEAMLESS_DRRS_SUPPORT)
> +               seq_puts(m, "\tVBT: DRRS_type: Seamless");
> +       else if (dev_priv->vbt.drrs_type == DRRS_NOT_SUPPORTED)
> +               seq_puts(m, "\tVBT: DRRS_type: None");
> +       else
> +               seq_puts(m, "\tVBT: DRRS_type: FIXME: Unrecognized Value");
> +
> +       seq_puts(m, "\n\n");
> +
> +       /*
> +        * Idleness DRRS detection states:
> +        *      Enabled   :     Idleness detection is active. When system is
> +        *                      Idle for the defined duration DRRS_LOW_RR
> +        *                      will be set. Or Idleness is already detected
> +        *                      and DRRS_LOW_RR is applied.
> +        *      Suspended :     Due to frontbuffer's busy state, Idleness
> +        *                      detection is suspended.
> +        *      Disabled  :     Idleness detection is disabled until a call is
> +        *                      made to enable. No encoder pointer will be
> +        *                      available.
> +        */
> +       if (intel_crtc->config->has_drrs) {
> +               struct intel_panel *panel;
> +
> +               mutex_lock(&drrs->mutex);
> +               /* DRRS Supported */
> +               seq_puts(m, "\tDRRS Supported: Yes\n");
> +
> +               /* disable_drrs() will make drrs->dp NULL */
> +               if (!drrs->dp) {
> +                       seq_puts(m, "Idleness DRRS: Disabled");
> +                       mutex_unlock(&drrs->mutex);
> +                       return;
> +               }
> +
> +               panel = &drrs->dp->attached_connector->panel;
> +               seq_printf(m, "\t\tBusy_frontbuffer_bits: 0x%X",
> +                                       drrs->busy_frontbuffer_bits);
> +
> +               seq_puts(m, "\n\t\t");
> +               if (drrs->refresh_rate_type == DRRS_HIGH_RR) {
> +                       seq_puts(m, "DRRS_State: DRRS_HIGH_RR\n");
> +                       vrefresh = panel->fixed_mode->vrefresh;
> +               } else if (drrs->refresh_rate_type == DRRS_LOW_RR) {
> +                       seq_puts(m, "DRRS_State: DRRS_LOW_RR\n");
> +                       vrefresh = panel->downclock_mode->vrefresh;
> +               } else {
> +                       seq_printf(m, "DRRS_State: Unknown(%d)\n",
> +                                               drrs->refresh_rate_type);
> +                       mutex_unlock(&drrs->mutex);
> +                       return;
> +               }
> +               seq_printf(m, "\t\tVrefresh: %d", vrefresh);
> +
> +               seq_puts(m, "\n\t\t");
> +               work_status = work_busy(&drrs->work.work);
> +               if (drrs->busy_frontbuffer_bits) {
> +                       seq_puts(m, "Front buffer: Busy.\n");
> +                       seq_puts(m, "\t\tIdleness DRRS: Suspended");
> +               } else {
> +                       seq_puts(m, "Front buffer: Idle");
> +                       seq_puts(m, "\n\t\t");
> +                       if (drrs->refresh_rate_type == DRRS_HIGH_RR) {
> +                               if (work_status) {
> +                                       seq_puts(m, "Idleness DRRS: Enabled");

I couldn't understand yet why the work busy means DRRS enabled necessarily.
Isn't there a more reliable way to check for DRRS enabled?

Nothing that can't be done in a followup patch...  Also I'd like to
see drrs status now so feel free to use
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> +                               } else {
> +                                       seq_puts(m, "Idleness DRRS: Suspended.");
> +                                       seq_puts(m, " FIXME: Shouldn't be here");
> +                               }
> +                       } else if (drrs->refresh_rate_type == DRRS_LOW_RR) {
> +                               seq_puts(m, "Idleness DRRS: Enabled");
> +                       }
> +               }
> +               mutex_unlock(&drrs->mutex);
> +       } else {
> +               /* DRRS not supported. Print the VBT parameter*/
> +               seq_puts(m, "\tDRRS Supported : No");
> +       }
> +       seq_puts(m, "\n");
> +}
> +
> +static int i915_drrs_status(struct seq_file *m, void *unused)
> +{
> +       struct drm_info_node *node = m->private;
> +       struct drm_device *dev = node->minor->dev;
> +       struct intel_crtc *intel_crtc;
> +       int active_crtc_cnt = 0;
> +
> +       for_each_intel_crtc(dev, intel_crtc) {
> +               drm_modeset_lock(&intel_crtc->base.mutex, NULL);
> +
> +               if (intel_crtc->active) {
> +                       active_crtc_cnt++;
> +                       seq_printf(m, "\nCRTC %d:  ", active_crtc_cnt);
> +
> +                       drrs_status_per_crtc(m, dev, intel_crtc);
> +               }
> +
> +               drm_modeset_unlock(&intel_crtc->base.mutex);
> +       }
> +
> +       if (!active_crtc_cnt)
> +               seq_puts(m, "No active crtc found\n");
> +
> +       return 0;
> +}
> +
>  struct pipe_crc_info {
>         const char *name;
>         struct drm_device *dev;
> @@ -4548,6 +4688,7 @@ static const struct drm_info_list i915_debugfs_list[] = {
>         {"i915_wa_registers", i915_wa_registers, 0},
>         {"i915_ddb_info", i915_ddb_info, 0},
>         {"i915_sseu_status", i915_sseu_status, 0},
> +       {"i915_drrs_status", i915_drrs_status, 0},
>  };
>  #define I915_DEBUGFS_ENTRIES ARRAY_SIZE(i915_debugfs_list)
>
> --
> 1.7.9.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Add debugfs entry for DRRS

[Daniel Vetter]

On Wed, Mar 04, 2015 at 03:00:17PM -0800, Rodrigo Vivi wrote:
> On Tue, Mar 3, 2015 at 7:23 AM, Ramalingam C <ramalingam.c@intel.com> wrote:
> > From: Vandana Kannan <vandana.kannan@intel.com>
> >
> > Adding a debugfs entry to determine if DRRS is supported or not
> >
> > V2: [By Ram]: Following details about the active crtc will be filled
> >         in seq-file of the debugfs
> >         1. Encoder output type
> >         2. DRRS Support on this CRTC
> >         3. DRRS current state
> >         4. Current Vrefresh
> > Format is as follows:
> > CRTC 1:  Output: eDP, DRRS Supported: Yes (Seamless), DRRS_State: DRRS_HIGH_RR, Vrefresh: 60
> > CRTC 2:  Output: HDMI, DRRS Supported : No, VBT DRRS_type: Seamless
> > CRTC 1:  Output: eDP, DRRS Supported: Yes (Seamless), DRRS_State: DRRS_LOW_RR, Vrefresh: 40
> > CRTC 2:  Output: HDMI, DRRS Supported : No, VBT DRRS_type: Seamless
> >
> > V3: [By Ram]: Readability is improved.
> >         Another error case is covered [Daniel]
> >
> > V4: [By Ram]: Current status of the Idleness DRRS along with
> >         the Front buffer bits are added to the debugfs. [Rodrigo]
> >
> > V5: [By Ram]: Rephrased to make it easy to understand.
> >         And format is modified. [Rodrigo]
> >
> > V6: [By Ram]: Modeset mutex are acquired for each crtc along with
> >         renaming the Idleness detection states  [Daniel]
> >
> > Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
> > Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_debugfs.c |  141 +++++++++++++++++++++++++++++++++++
> >  1 file changed, 141 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> > index 94b3984..90e56ca 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -2870,6 +2870,146 @@ static int i915_ddb_info(struct seq_file *m, void *unused)
> >         return 0;
> >  }
> >
> > +static void drrs_status_per_crtc(struct seq_file *m,
> > +               struct drm_device *dev, struct intel_crtc *intel_crtc)
> > +{
> > +       struct intel_encoder *intel_encoder;
> > +       struct drm_i915_private *dev_priv = dev->dev_private;
> > +       struct i915_drrs *drrs = &dev_priv->drrs;
> > +       int vrefresh = 0;
> > +       u32 work_status;
> > +
> > +       for_each_encoder_on_crtc(dev, &intel_crtc->base, intel_encoder) {
> > +               /* Encoder connected on this CRTC */
> > +               switch (intel_encoder->type) {
> > +               case INTEL_OUTPUT_EDP:
> > +                       seq_puts(m, "eDP:\n");
> > +                       break;
> > +               case INTEL_OUTPUT_DSI:
> > +                       seq_puts(m, "DSI:\n");
> > +                       break;
> > +               case INTEL_OUTPUT_HDMI:
> > +                       seq_puts(m, "HDMI:\n");
> > +                       break;
> > +               case INTEL_OUTPUT_DISPLAYPORT:
> > +                       seq_puts(m, "DP:\n");
> > +                       break;
> > +               default:
> > +                       seq_printf(m, "Other encoder (id=%d).\n",
> > +                                               intel_encoder->type);
> > +                       return;
> > +               }
> > +       }
> > +
> > +       if (dev_priv->vbt.drrs_type == STATIC_DRRS_SUPPORT)
> > +               seq_puts(m, "\tVBT: DRRS_type: Static");
> > +       else if (dev_priv->vbt.drrs_type == SEAMLESS_DRRS_SUPPORT)
> > +               seq_puts(m, "\tVBT: DRRS_type: Seamless");
> > +       else if (dev_priv->vbt.drrs_type == DRRS_NOT_SUPPORTED)
> > +               seq_puts(m, "\tVBT: DRRS_type: None");
> > +       else
> > +               seq_puts(m, "\tVBT: DRRS_type: FIXME: Unrecognized Value");
> > +
> > +       seq_puts(m, "\n\n");
> > +
> > +       /*
> > +        * Idleness DRRS detection states:
> > +        *      Enabled   :     Idleness detection is active. When system is
> > +        *                      Idle for the defined duration DRRS_LOW_RR
> > +        *                      will be set. Or Idleness is already detected
> > +        *                      and DRRS_LOW_RR is applied.
> > +        *      Suspended :     Due to frontbuffer's busy state, Idleness
> > +        *                      detection is suspended.
> > +        *      Disabled  :     Idleness detection is disabled until a call is
> > +        *                      made to enable. No encoder pointer will be
> > +        *                      available.
> > +        */
> > +       if (intel_crtc->config->has_drrs) {
> > +               struct intel_panel *panel;
> > +
> > +               mutex_lock(&drrs->mutex);
> > +               /* DRRS Supported */
> > +               seq_puts(m, "\tDRRS Supported: Yes\n");
> > +
> > +               /* disable_drrs() will make drrs->dp NULL */
> > +               if (!drrs->dp) {
> > +                       seq_puts(m, "Idleness DRRS: Disabled");
> > +                       mutex_unlock(&drrs->mutex);
> > +                       return;
> > +               }
> > +
> > +               panel = &drrs->dp->attached_connector->panel;
> > +               seq_printf(m, "\t\tBusy_frontbuffer_bits: 0x%X",
> > +                                       drrs->busy_frontbuffer_bits);
> > +
> > +               seq_puts(m, "\n\t\t");
> > +               if (drrs->refresh_rate_type == DRRS_HIGH_RR) {
> > +                       seq_puts(m, "DRRS_State: DRRS_HIGH_RR\n");
> > +                       vrefresh = panel->fixed_mode->vrefresh;
> > +               } else if (drrs->refresh_rate_type == DRRS_LOW_RR) {
> > +                       seq_puts(m, "DRRS_State: DRRS_LOW_RR\n");
> > +                       vrefresh = panel->downclock_mode->vrefresh;
> > +               } else {
> > +                       seq_printf(m, "DRRS_State: Unknown(%d)\n",
> > +                                               drrs->refresh_rate_type);
> > +                       mutex_unlock(&drrs->mutex);
> > +                       return;
> > +               }
> > +               seq_printf(m, "\t\tVrefresh: %d", vrefresh);
> > +
> > +               seq_puts(m, "\n\t\t");
> > +               work_status = work_busy(&drrs->work.work);
> > +               if (drrs->busy_frontbuffer_bits) {
> > +                       seq_puts(m, "Front buffer: Busy.\n");
> > +                       seq_puts(m, "\t\tIdleness DRRS: Suspended");
> > +               } else {
> > +                       seq_puts(m, "Front buffer: Idle");
> > +                       seq_puts(m, "\n\t\t");
> > +                       if (drrs->refresh_rate_type == DRRS_HIGH_RR) {
> > +                               if (work_status) {
> > +                                       seq_puts(m, "Idleness DRRS: Enabled");
> 
> I couldn't understand yet why the work busy means DRRS enabled necessarily.
> Isn't there a more reliable way to check for DRRS enabled?

Yeah that seems wrong really and will just confuse people. I'll remove
this code. Also it's usually much more informative to dump all the
frontbuffer bits instead of just a busy/idle answer - if there's a bug in
the frontbuffer tracking it's good to know where the busy bits come from.

Applied the patch with those changes.
-Daniel

> Nothing that can't be done in a followup patch...  Also I'd like to
> see drrs status now so feel free to use
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> 
> > +                               } else {
> > +                                       seq_puts(m, "Idleness DRRS: Suspended.");
> > +                                       seq_puts(m, " FIXME: Shouldn't be here");
> > +                               }
> > +                       } else if (drrs->refresh_rate_type == DRRS_LOW_RR) {
> > +                               seq_puts(m, "Idleness DRRS: Enabled");
> > +                       }
> > +               }
> > +               mutex_unlock(&drrs->mutex);
> > +       } else {
> > +               /* DRRS not supported. Print the VBT parameter*/
> > +               seq_puts(m, "\tDRRS Supported : No");
> > +       }
> > +       seq_puts(m, "\n");
> > +}
> > +
> > +static int i915_drrs_status(struct seq_file *m, void *unused)
> > +{
> > +       struct drm_info_node *node = m->private;
> > +       struct drm_device *dev = node->minor->dev;
> > +       struct intel_crtc *intel_crtc;
> > +       int active_crtc_cnt = 0;
> > +
> > +       for_each_intel_crtc(dev, intel_crtc) {
> > +               drm_modeset_lock(&intel_crtc->base.mutex, NULL);
> > +
> > +               if (intel_crtc->active) {
> > +                       active_crtc_cnt++;
> > +                       seq_printf(m, "\nCRTC %d:  ", active_crtc_cnt);
> > +
> > +                       drrs_status_per_crtc(m, dev, intel_crtc);
> > +               }
> > +
> > +               drm_modeset_unlock(&intel_crtc->base.mutex);
> > +       }
> > +
> > +       if (!active_crtc_cnt)
> > +               seq_puts(m, "No active crtc found\n");
> > +
> > +       return 0;
> > +}
> > +
> >  struct pipe_crc_info {
> >         const char *name;
> >         struct drm_device *dev;
> > @@ -4548,6 +4688,7 @@ static const struct drm_info_list i915_debugfs_list[] = {
> >         {"i915_wa_registers", i915_wa_registers, 0},
> >         {"i915_ddb_info", i915_ddb_info, 0},
> >         {"i915_sseu_status", i915_sseu_status, 0},
> > +       {"i915_drrs_status", i915_drrs_status, 0},
> >  };
> >  #define I915_DEBUGFS_ENTRIES ARRAY_SIZE(i915_debugfs_list)
> >
> > --
> > 1.7.9.5
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Rodrigo Vivi
> Blog: http://blog.vivi.eng.br

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Add debugfs entry for DRRS

[Ramalingam C]

On Thursday 05 March 2015 04:48 PM, Daniel Vetter wrote:
> On Wed, Mar 04, 2015 at 03:00:17PM -0800, Rodrigo Vivi wrote:
>> On Tue, Mar 3, 2015 at 7:23 AM, Ramalingam C <ramalingam.c@intel.com> wrote:
>>> From: Vandana Kannan <vandana.kannan@intel.com>
>>>
>>> Adding a debugfs entry to determine if DRRS is supported or not
>>>
>>> V2: [By Ram]: Following details about the active crtc will be filled
>>>          in seq-file of the debugfs
>>>          1. Encoder output type
>>>          2. DRRS Support on this CRTC
>>>          3. DRRS current state
>>>          4. Current Vrefresh
>>> Format is as follows:
>>> CRTC 1:  Output: eDP, DRRS Supported: Yes (Seamless), DRRS_State: DRRS_HIGH_RR, Vrefresh: 60
>>> CRTC 2:  Output: HDMI, DRRS Supported : No, VBT DRRS_type: Seamless
>>> CRTC 1:  Output: eDP, DRRS Supported: Yes (Seamless), DRRS_State: DRRS_LOW_RR, Vrefresh: 40
>>> CRTC 2:  Output: HDMI, DRRS Supported : No, VBT DRRS_type: Seamless
>>>
>>> V3: [By Ram]: Readability is improved.
>>>          Another error case is covered [Daniel]
>>>
>>> V4: [By Ram]: Current status of the Idleness DRRS along with
>>>          the Front buffer bits are added to the debugfs. [Rodrigo]
>>>
>>> V5: [By Ram]: Rephrased to make it easy to understand.
>>>          And format is modified. [Rodrigo]
>>>
>>> V6: [By Ram]: Modeset mutex are acquired for each crtc along with
>>>          renaming the Idleness detection states  [Daniel]
>>>
>>> Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
>>> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/i915_debugfs.c |  141 +++++++++++++++++++++++++++++++++++
>>>   1 file changed, 141 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
>>> index 94b3984..90e56ca 100644
>>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>>> @@ -2870,6 +2870,146 @@ static int i915_ddb_info(struct seq_file *m, void *unused)
>>>          return 0;
>>>   }
>>>
>>> +static void drrs_status_per_crtc(struct seq_file *m,
>>> +               struct drm_device *dev, struct intel_crtc *intel_crtc)
>>> +{
>>> +       struct intel_encoder *intel_encoder;
>>> +       struct drm_i915_private *dev_priv = dev->dev_private;
>>> +       struct i915_drrs *drrs = &dev_priv->drrs;
>>> +       int vrefresh = 0;
>>> +       u32 work_status;
>>> +
>>> +       for_each_encoder_on_crtc(dev, &intel_crtc->base, intel_encoder) {
>>> +               /* Encoder connected on this CRTC */
>>> +               switch (intel_encoder->type) {
>>> +               case INTEL_OUTPUT_EDP:
>>> +                       seq_puts(m, "eDP:\n");
>>> +                       break;
>>> +               case INTEL_OUTPUT_DSI:
>>> +                       seq_puts(m, "DSI:\n");
>>> +                       break;
>>> +               case INTEL_OUTPUT_HDMI:
>>> +                       seq_puts(m, "HDMI:\n");
>>> +                       break;
>>> +               case INTEL_OUTPUT_DISPLAYPORT:
>>> +                       seq_puts(m, "DP:\n");
>>> +                       break;
>>> +               default:
>>> +                       seq_printf(m, "Other encoder (id=%d).\n",
>>> +                                               intel_encoder->type);
>>> +                       return;
>>> +               }
>>> +       }
>>> +
>>> +       if (dev_priv->vbt.drrs_type == STATIC_DRRS_SUPPORT)
>>> +               seq_puts(m, "\tVBT: DRRS_type: Static");
>>> +       else if (dev_priv->vbt.drrs_type == SEAMLESS_DRRS_SUPPORT)
>>> +               seq_puts(m, "\tVBT: DRRS_type: Seamless");
>>> +       else if (dev_priv->vbt.drrs_type == DRRS_NOT_SUPPORTED)
>>> +               seq_puts(m, "\tVBT: DRRS_type: None");
>>> +       else
>>> +               seq_puts(m, "\tVBT: DRRS_type: FIXME: Unrecognized Value");
>>> +
>>> +       seq_puts(m, "\n\n");
>>> +
>>> +       /*
>>> +        * Idleness DRRS detection states:
>>> +        *      Enabled   :     Idleness detection is active. When system is
>>> +        *                      Idle for the defined duration DRRS_LOW_RR
>>> +        *                      will be set. Or Idleness is already detected
>>> +        *                      and DRRS_LOW_RR is applied.
>>> +        *      Suspended :     Due to frontbuffer's busy state, Idleness
>>> +        *                      detection is suspended.
>>> +        *      Disabled  :     Idleness detection is disabled until a call is
>>> +        *                      made to enable. No encoder pointer will be
>>> +        *                      available.
>>> +        */
>>> +       if (intel_crtc->config->has_drrs) {
>>> +               struct intel_panel *panel;
>>> +
>>> +               mutex_lock(&drrs->mutex);
>>> +               /* DRRS Supported */
>>> +               seq_puts(m, "\tDRRS Supported: Yes\n");
>>> +
>>> +               /* disable_drrs() will make drrs->dp NULL */
>>> +               if (!drrs->dp) {
>>> +                       seq_puts(m, "Idleness DRRS: Disabled");
>>> +                       mutex_unlock(&drrs->mutex);
>>> +                       return;
>>> +               }
>>> +
>>> +               panel = &drrs->dp->attached_connector->panel;
>>> +               seq_printf(m, "\t\tBusy_frontbuffer_bits: 0x%X",
>>> +                                       drrs->busy_frontbuffer_bits);
Front buffer bits are added here.
>>> +
>>> +               seq_puts(m, "\n\t\t");
>>> +               if (drrs->refresh_rate_type == DRRS_HIGH_RR) {
>>> +                       seq_puts(m, "DRRS_State: DRRS_HIGH_RR\n");
>>> +                       vrefresh = panel->fixed_mode->vrefresh;
>>> +               } else if (drrs->refresh_rate_type == DRRS_LOW_RR) {
>>> +                       seq_puts(m, "DRRS_State: DRRS_LOW_RR\n");
>>> +                       vrefresh = panel->downclock_mode->vrefresh;
>>> +               } else {
>>> +                       seq_printf(m, "DRRS_State: Unknown(%d)\n",
>>> +                                               drrs->refresh_rate_type);
>>> +                       mutex_unlock(&drrs->mutex);
>>> +                       return;
>>> +               }
>>> +               seq_printf(m, "\t\tVrefresh: %d", vrefresh);
>>> +
>>> +               seq_puts(m, "\n\t\t");
>>> +               work_status = work_busy(&drrs->work.work);
>>> +               if (drrs->busy_frontbuffer_bits) {
>>> +                       seq_puts(m, "Front buffer: Busy.\n");
>>> +                       seq_puts(m, "\t\tIdleness DRRS: Suspended");
>>> +               } else {
>>> +                       seq_puts(m, "Front buffer: Idle");
>>> +                       seq_puts(m, "\n\t\t");
>>> +                       if (drrs->refresh_rate_type == DRRS_HIGH_RR) {
>>> +                               if (work_status) {
>>> +                                       seq_puts(m, "Idleness DRRS: Enabled");
>> I couldn't understand yet why the work busy means DRRS enabled necessarily.
>> Isn't there a more reliable way to check for DRRS enabled?
> Yeah that seems wrong really and will just confuse people. I'll remove
> this code.
When the front buffer bit indicates Idle and the DRRS state is 
DRRS_HIGH_RR then we should have the downclock_work  scheduled.
I was double checking that here. If you are removing that I am fine with 
that.
> Also it's usually much more informative to dump all the
> frontbuffer bits instead of just a busy/idle answer - if there's a bug in
> the frontbuffer tracking it's good to know where the busy bits come from.
Already we have added the front buffer bits in the string.
>
> Applied the patch with those changes.
> -Daniel
>
>> Nothing that can't be done in a followup patch...  Also I'd like to
>> see drrs status now so feel free to use
>> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>
>>> +                               } else {
>>> +                                       seq_puts(m, "Idleness DRRS: Suspended.");
>>> +                                       seq_puts(m, " FIXME: Shouldn't be here");
>>> +                               }
>>> +                       } else if (drrs->refresh_rate_type == DRRS_LOW_RR) {
>>> +                               seq_puts(m, "Idleness DRRS: Enabled");
>>> +                       }
>>> +               }
>>> +               mutex_unlock(&drrs->mutex);
>>> +       } else {
>>> +               /* DRRS not supported. Print the VBT parameter*/
>>> +               seq_puts(m, "\tDRRS Supported : No");
>>> +       }
>>> +       seq_puts(m, "\n");
>>> +}
>>> +
>>> +static int i915_drrs_status(struct seq_file *m, void *unused)
>>> +{
>>> +       struct drm_info_node *node = m->private;
>>> +       struct drm_device *dev = node->minor->dev;
>>> +       struct intel_crtc *intel_crtc;
>>> +       int active_crtc_cnt = 0;
>>> +
>>> +       for_each_intel_crtc(dev, intel_crtc) {
>>> +               drm_modeset_lock(&intel_crtc->base.mutex, NULL);
>>> +
>>> +               if (intel_crtc->active) {
>>> +                       active_crtc_cnt++;
>>> +                       seq_printf(m, "\nCRTC %d:  ", active_crtc_cnt);
>>> +
>>> +                       drrs_status_per_crtc(m, dev, intel_crtc);
>>> +               }
>>> +
>>> +               drm_modeset_unlock(&intel_crtc->base.mutex);
>>> +       }
>>> +
>>> +       if (!active_crtc_cnt)
>>> +               seq_puts(m, "No active crtc found\n");
>>> +
>>> +       return 0;
>>> +}
>>> +
>>>   struct pipe_crc_info {
>>>          const char *name;
>>>          struct drm_device *dev;
>>> @@ -4548,6 +4688,7 @@ static const struct drm_info_list i915_debugfs_list[] = {
>>>          {"i915_wa_registers", i915_wa_registers, 0},
>>>          {"i915_ddb_info", i915_ddb_info, 0},
>>>          {"i915_sseu_status", i915_sseu_status, 0},
>>> +       {"i915_drrs_status", i915_drrs_status, 0},
>>>   };
>>>   #define I915_DEBUGFS_ENTRIES ARRAY_SIZE(i915_debugfs_list)
>>>
>>> --
>>> 1.7.9.5
>>>
>>> _______________________________________________
>>> Intel-gfx mailing list
>>> Intel-gfx@lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>
>>
>> -- 
>> Rodrigo Vivi
>> Blog: http://blog.vivi.eng.br

-- 
Ram

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Add debugfs entry for DRRS

[Daniel Vetter]

On Thu, Mar 05, 2015 at 04:52:47PM +0530, Ramalingam C wrote:
> On Thursday 05 March 2015 04:48 PM, Daniel Vetter wrote:
> >Also it's usually much more informative to dump all the
> >frontbuffer bits instead of just a busy/idle answer - if there's a bug in
> >the frontbuffer tracking it's good to know where the busy bits come from.
> Already we have added the front buffer bits in the string.

Indeed, removed the additional line.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/i915: Enhancing eDP DRRS debug message

[Ramalingam C]

When Downclock mode is not found, the same info is added to the
corresponding debug log.

Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index e9862e7..8d674f4 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -5083,7 +5083,7 @@ intel_dp_drrs_init(struct intel_connector *intel_connector,
 					(dev, fixed_mode, connector);
 
 	if (!downclock_mode) {
-		DRM_DEBUG_KMS("DRRS not supported\n");
+		DRM_DEBUG_KMS("Downclock mode is not found. DRRS not supported\n");
 		return NULL;
 	}
 
-- 
1.7.9.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Enhancing eDP DRRS debug message

[Rodrigo Vivi]

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

On Mon, Feb 23, 2015 at 4:08 AM, Ramalingam C <ramalingam.c@intel.com> wrote:
> When Downclock mode is not found, the same info is added to the
> corresponding debug log.
>
> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index e9862e7..8d674f4 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -5083,7 +5083,7 @@ intel_dp_drrs_init(struct intel_connector *intel_connector,
>                                         (dev, fixed_mode, connector);
>
>         if (!downclock_mode) {
> -               DRM_DEBUG_KMS("DRRS not supported\n");
> +               DRM_DEBUG_KMS("Downclock mode is not found. DRRS not supported\n");
>                 return NULL;
>         }
>
> --
> 1.7.9.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 0/6] eDP DRRS based on frontbuffer tracking

[Daniel Vetter]

On Fri, Feb 13, 2015 at 03:32:58PM +0530, Ramalingam C wrote:
> This series includes a preparation patch for drrs support across differnt
> platforms in intel_dp_set_m_n along with last 5 pending patches of V3 eDP
> DRRS patch series.
> 
> New series is submitted to make the review more comfortable and
> to display the dependancy of the patches explicitly.
> 
> Durgadoss R (1):
>   drm/i915: Enable eDP DRRS for CHV
> 
> Ramalingam C (1):
>   drm/i915: Add support for DRRS in intel_dp_set_m_n
> 
> Vandana Kannan (4):
>   drm/i915/bdw: Add support for DRRS to switch RR
>   drm/i915: Support for RR switching on VLV
>   Documentation/drm: DocBook integration for DRRS
>   drm/i915: Add debugfs entry for DRRS

Ok I've reviewed the locking for DRRS quickly now that it's all merged and
it's deadlock-y:

intel_edp_drrs_downclock_work grabs the drrs mutex. But in the disable
function we cancel that work and wait for that to complete (cancel_sync)
while holding the lock.

Which means if the async work is running this will deadlock. The work
cancel must be moved out of the critical section, and the work must
double-check (after taking the lock) that drrs hasn't been disabled
meanwhile (by checking drrs.dp, which we already do). intel_psr.c contains
all that logic as an example.

While you do that follow-up patch could we extract the drrs code into a
new intel_drrs.c file? That would also simplify the kerneldoc includes a
bit.

Cheers, Daniel
> 
>  Documentation/DocBook/drm.tmpl       |   11 ++++
>  drivers/gpu/drm/i915/i915_debugfs.c  |   99 ++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/i915_reg.h      |    1 +
>  drivers/gpu/drm/i915/intel_display.c |   32 ++++++---
>  drivers/gpu/drm/i915/intel_dp.c      |  121 ++++++++++++++++++++++++++++++++--
>  drivers/gpu/drm/i915/intel_drv.h     |   22 ++++++-
>  6 files changed, 273 insertions(+), 13 deletions(-)
> 
> -- 
> 1.7.9.5
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 0/6] eDP DRRS based on frontbuffer tracking

[Ramalingam C]

On Tuesday 24 February 2015 06:21 AM, Daniel Vetter wrote:
> On Fri, Feb 13, 2015 at 03:32:58PM +0530, Ramalingam C wrote:
>> This series includes a preparation patch for drrs support across differnt
>> platforms in intel_dp_set_m_n along with last 5 pending patches of V3 eDP
>> DRRS patch series.
>>
>> New series is submitted to make the review more comfortable and
>> to display the dependancy of the patches explicitly.
>>
>> Durgadoss R (1):
>>    drm/i915: Enable eDP DRRS for CHV
>>
>> Ramalingam C (1):
>>    drm/i915: Add support for DRRS in intel_dp_set_m_n
>>
>> Vandana Kannan (4):
>>    drm/i915/bdw: Add support for DRRS to switch RR
>>    drm/i915: Support for RR switching on VLV
>>    Documentation/drm: DocBook integration for DRRS
>>    drm/i915: Add debugfs entry for DRRS
> Ok I've reviewed the locking for DRRS quickly now that it's all merged and
> it's deadlock-y:
>
> intel_edp_drrs_downclock_work grabs the drrs mutex. But in the disable
> function we cancel that work and wait for that to complete (cancel_sync)
> while holding the lock.
>
> Which means if the async work is running this will deadlock. The work
> cancel must be moved out of the critical section, and the work must
> double-check (after taking the lock) that drrs hasn't been disabled
> meanwhile (by checking drrs.dp, which we already do). intel_psr.c contains
> all that logic as an example.
>
> While you do that follow-up patch could we extract the drrs code into a
> new intel_drrs.c file? That would also simplify the kerneldoc includes a
> bit.
>
> Cheers, Daniel
Missed the possible deadlock window. I will upload a patch moving the 
cancellation of the deferred work out of the mutex protection asap.

As a heads up, in VPG we have implemented MIPI DRRS and also content 
based DRRS.

MIPI DRRS:
We have extended the DRRS to DSI encoder also on android codelines for 
BYT and CHV.


Content based DRRS:
We have implemented the interfaces
         to expose the DRRS capability and and the vrefresh supported
         to receive the request for the new vrefresh.

So based on required FPS for the display content to be rendered 
userspace, will place a request for the new vrefresh.

We have verified the functionality of this implementation on android 
devices for almost an year now.
I will rework on the implementation to adapt to the DRM-intel and submit 
a RFC to explain the complete design.

This RFC will have the generalized DRRS state machine for all encoders 
like eDP and DSI along with encoder support for DSI and eDP.
So as a result we will have the generic code for drrs state machine in 
intel_drrs.c totally separated from the encoder related code. Can we 
wait till then?


--Ram
>>   Documentation/DocBook/drm.tmpl       |   11 ++++
>>   drivers/gpu/drm/i915/i915_debugfs.c  |   99 ++++++++++++++++++++++++++++
>>   drivers/gpu/drm/i915/i915_reg.h      |    1 +
>>   drivers/gpu/drm/i915/intel_display.c |   32 ++++++---
>>   drivers/gpu/drm/i915/intel_dp.c      |  121 ++++++++++++++++++++++++++++++++--
>>   drivers/gpu/drm/i915/intel_drv.h     |   22 ++++++-
>>   6 files changed, 273 insertions(+), 13 deletions(-)
>>
>> -- 
>> 1.7.9.5
>>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 0/6] eDP DRRS based on frontbuffer tracking

[Daniel Vetter]

On Fri, Feb 27, 2015 at 07:59:43PM +0530, Ramalingam C wrote:
> 
> On Tuesday 24 February 2015 06:21 AM, Daniel Vetter wrote:
> >On Fri, Feb 13, 2015 at 03:32:58PM +0530, Ramalingam C wrote:
> >>This series includes a preparation patch for drrs support across differnt
> >>platforms in intel_dp_set_m_n along with last 5 pending patches of V3 eDP
> >>DRRS patch series.
> >>
> >>New series is submitted to make the review more comfortable and
> >>to display the dependancy of the patches explicitly.
> >>
> >>Durgadoss R (1):
> >>   drm/i915: Enable eDP DRRS for CHV
> >>
> >>Ramalingam C (1):
> >>   drm/i915: Add support for DRRS in intel_dp_set_m_n
> >>
> >>Vandana Kannan (4):
> >>   drm/i915/bdw: Add support for DRRS to switch RR
> >>   drm/i915: Support for RR switching on VLV
> >>   Documentation/drm: DocBook integration for DRRS
> >>   drm/i915: Add debugfs entry for DRRS
> >Ok I've reviewed the locking for DRRS quickly now that it's all merged and
> >it's deadlock-y:
> >
> >intel_edp_drrs_downclock_work grabs the drrs mutex. But in the disable
> >function we cancel that work and wait for that to complete (cancel_sync)
> >while holding the lock.
> >
> >Which means if the async work is running this will deadlock. The work
> >cancel must be moved out of the critical section, and the work must
> >double-check (after taking the lock) that drrs hasn't been disabled
> >meanwhile (by checking drrs.dp, which we already do). intel_psr.c contains
> >all that logic as an example.
> >
> >While you do that follow-up patch could we extract the drrs code into a
> >new intel_drrs.c file? That would also simplify the kerneldoc includes a
> >bit.
> >
> >Cheers, Daniel
> Missed the possible deadlock window. I will upload a patch moving the
> cancellation of the deferred work out of the mutex protection asap.
> 
> As a heads up, in VPG we have implemented MIPI DRRS and also content based
> DRRS.
> 
> MIPI DRRS:
> We have extended the DRRS to DSI encoder also on android codelines for BYT
> and CHV.
> 
> 
> Content based DRRS:
> We have implemented the interfaces

What kind of interface? Imo we should do this by adjusting the frequecy of
the mode, with a suitable modeset fastpath.
-Daniel

>         to expose the DRRS capability and and the vrefresh supported
>         to receive the request for the new vrefresh.
> 
> So based on required FPS for the display content to be rendered userspace,
> will place a request for the new vrefresh.
> 
> We have verified the functionality of this implementation on android devices
> for almost an year now.
> I will rework on the implementation to adapt to the DRM-intel and submit a
> RFC to explain the complete design.
> 
> This RFC will have the generalized DRRS state machine for all encoders like
> eDP and DSI along with encoder support for DSI and eDP.
> So as a result we will have the generic code for drrs state machine in
> intel_drrs.c totally separated from the encoder related code. Can we wait
> till then?

The deadlock fix should land asap. Or do you mean something else?
-Daniel

> 
> 
> --Ram
> >>  Documentation/DocBook/drm.tmpl       |   11 ++++
> >>  drivers/gpu/drm/i915/i915_debugfs.c  |   99 ++++++++++++++++++++++++++++
> >>  drivers/gpu/drm/i915/i915_reg.h      |    1 +
> >>  drivers/gpu/drm/i915/intel_display.c |   32 ++++++---
> >>  drivers/gpu/drm/i915/intel_dp.c      |  121 ++++++++++++++++++++++++++++++++--
> >>  drivers/gpu/drm/i915/intel_drv.h     |   22 ++++++-
> >>  6 files changed, 273 insertions(+), 13 deletions(-)
> >>
> >>-- 
> >>1.7.9.5
> >>
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 0/6] eDP DRRS based on frontbuffer tracking

[Ramalingam C]

On Friday 27 February 2015 09:07 PM, Daniel Vetter wrote:
> On Fri, Feb 27, 2015 at 07:59:43PM +0530, Ramalingam C wrote:
>> On Tuesday 24 February 2015 06:21 AM, Daniel Vetter wrote:
>>> On Fri, Feb 13, 2015 at 03:32:58PM +0530, Ramalingam C wrote:
>>>> This series includes a preparation patch for drrs support across differnt
>>>> platforms in intel_dp_set_m_n along with last 5 pending patches of V3 eDP
>>>> DRRS patch series.
>>>>
>>>> New series is submitted to make the review more comfortable and
>>>> to display the dependancy of the patches explicitly.
>>>>
>>>> Durgadoss R (1):
>>>>    drm/i915: Enable eDP DRRS for CHV
>>>>
>>>> Ramalingam C (1):
>>>>    drm/i915: Add support for DRRS in intel_dp_set_m_n
>>>>
>>>> Vandana Kannan (4):
>>>>    drm/i915/bdw: Add support for DRRS to switch RR
>>>>    drm/i915: Support for RR switching on VLV
>>>>    Documentation/drm: DocBook integration for DRRS
>>>>    drm/i915: Add debugfs entry for DRRS
>>> Ok I've reviewed the locking for DRRS quickly now that it's all merged and
>>> it's deadlock-y:
>>>
>>> intel_edp_drrs_downclock_work grabs the drrs mutex. But in the disable
>>> function we cancel that work and wait for that to complete (cancel_sync)
>>> while holding the lock.
>>>
>>> Which means if the async work is running this will deadlock. The work
>>> cancel must be moved out of the critical section, and the work must
>>> double-check (after taking the lock) that drrs hasn't been disabled
>>> meanwhile (by checking drrs.dp, which we already do). intel_psr.c contains
>>> all that logic as an example.
>>>
>>> While you do that follow-up patch could we extract the drrs code into a
>>> new intel_drrs.c file? That would also simplify the kerneldoc includes a
>>> bit.
>>>
>>> Cheers, Daniel
>> Missed the possible deadlock window. I will upload a patch moving the
>> cancellation of the deferred work out of the mutex protection asap.
>>
>> As a heads up, in VPG we have implemented MIPI DRRS and also content based
>> DRRS.
>>
>> MIPI DRRS:
>> We have extended the DRRS to DSI encoder also on android codelines for BYT
>> and CHV.
>>
>>
>> Content based DRRS:
>> We have implemented the interfaces
> What kind of interface? Imo we should do this by adjusting the frequecy of
> the mode, with a suitable modeset fastpath.
> -Daniel
We have used the specific path in modeset to request for the DRRS 
vrefresh. And used a drm property to expose the DRRS type.
>
>>          to expose the DRRS capability and and the vrefresh supported
>>          to receive the request for the new vrefresh.
>>
>> So based on required FPS for the display content to be rendered userspace,
>> will place a request for the new vrefresh.
>>
>> We have verified the functionality of this implementation on android devices
>> for almost an year now.
>> I will rework on the implementation to adapt to the DRM-intel and submit a
>> RFC to explain the complete design.
>>
>> This RFC will have the generalized DRRS state machine for all encoders like
>> eDP and DSI along with encoder support for DSI and eDP.
>> So as a result we will have the generic code for drrs state machine in
>> intel_drrs.c totally separated from the encoder related code. Can we wait
>> till then?
> The deadlock fix should land asap. Or do you mean something else?
Patch to fix deadlock will be submitted asap in early monday after 
required testing. I was asking regarding the DRRS code separation.
> -Daniel
>
>>
>> --Ram
>>>>   Documentation/DocBook/drm.tmpl       |   11 ++++
>>>>   drivers/gpu/drm/i915/i915_debugfs.c  |   99 ++++++++++++++++++++++++++++
>>>>   drivers/gpu/drm/i915/i915_reg.h      |    1 +
>>>>   drivers/gpu/drm/i915/intel_display.c |   32 ++++++---
>>>>   drivers/gpu/drm/i915/intel_dp.c      |  121 ++++++++++++++++++++++++++++++++--
>>>>   drivers/gpu/drm/i915/intel_drv.h     |   22 ++++++-
>>>>   6 files changed, 273 insertions(+), 13 deletions(-)
>>>>
>>>> -- 
>>>> 1.7.9.5
>>>>

-- 
Ram

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/i915: Fixing mutex deadlock window at eDP DRRS

[Ramalingam C]

In invalidate and flush functions of eDP DRRS, if deferred downclock
work starts execution at a time window between acquiring the drrs
mutex and cancellation of the deferred work
(intel_edp_drrs_downclock_work), then deferred work will find
drrs mutex locked and wait for the same.

Meanwhile the function that acquired mutex drrs invalidate/flush will
wait for the completion of the deferred work before releasing the mutex.
Thats a deadlock.

To avoid such deadlock scenario, this change cancels the deferred work
before acquiring the mutex at invalidate and flush functions.

Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index d1141d3..0a57763 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4966,12 +4966,13 @@ void intel_edp_drrs_invalidate(struct drm_device *dev,
 	if (!dev_priv->drrs.dp)
 		return;
 
+	cancel_delayed_work_sync(&dev_priv->drrs.work);
+
 	mutex_lock(&dev_priv->drrs.mutex);
 	crtc = dp_to_dig_port(dev_priv->drrs.dp)->base.base.crtc;
 	pipe = to_intel_crtc(crtc)->pipe;
 
 	if (dev_priv->drrs.refresh_rate_type == DRRS_LOW_RR) {
-		cancel_delayed_work_sync(&dev_priv->drrs.work);
 		intel_dp_set_drrs_state(dev_priv->dev,
 				dev_priv->drrs.dp->attached_connector->panel.
 				fixed_mode->vrefresh);
@@ -5004,13 +5005,13 @@ void intel_edp_drrs_flush(struct drm_device *dev,
 	if (!dev_priv->drrs.dp)
 		return;
 
+	cancel_delayed_work_sync(&dev_priv->drrs.work);
+
 	mutex_lock(&dev_priv->drrs.mutex);
 	crtc = dp_to_dig_port(dev_priv->drrs.dp)->base.base.crtc;
 	pipe = to_intel_crtc(crtc)->pipe;
 	dev_priv->drrs.busy_frontbuffer_bits &= ~frontbuffer_bits;
 
-	cancel_delayed_work_sync(&dev_priv->drrs.work);
-
 	if (dev_priv->drrs.refresh_rate_type != DRRS_LOW_RR &&
 			!dev_priv->drrs.busy_frontbuffer_bits)
 		schedule_delayed_work(&dev_priv->drrs.work,
-- 
1.7.9.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Fixing mutex deadlock window at eDP DRRS

[Rodrigo Vivi]

Looks enough for me...

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

On Mon, Mar 2, 2015 at 10:41 PM, Ramalingam C <ramalingam.c@intel.com> wrote:
> In invalidate and flush functions of eDP DRRS, if deferred downclock
> work starts execution at a time window between acquiring the drrs
> mutex and cancellation of the deferred work
> (intel_edp_drrs_downclock_work), then deferred work will find
> drrs mutex locked and wait for the same.
>
> Meanwhile the function that acquired mutex drrs invalidate/flush will
> wait for the completion of the deferred work before releasing the mutex.
> Thats a deadlock.
>
> To avoid such deadlock scenario, this change cancels the deferred work
> before acquiring the mutex at invalidate and flush functions.
>
> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c |    7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index d1141d3..0a57763 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4966,12 +4966,13 @@ void intel_edp_drrs_invalidate(struct drm_device *dev,
>         if (!dev_priv->drrs.dp)
>                 return;
>
> +       cancel_delayed_work_sync(&dev_priv->drrs.work);
> +
>         mutex_lock(&dev_priv->drrs.mutex);
>         crtc = dp_to_dig_port(dev_priv->drrs.dp)->base.base.crtc;
>         pipe = to_intel_crtc(crtc)->pipe;
>
>         if (dev_priv->drrs.refresh_rate_type == DRRS_LOW_RR) {
> -               cancel_delayed_work_sync(&dev_priv->drrs.work);
>                 intel_dp_set_drrs_state(dev_priv->dev,
>                                 dev_priv->drrs.dp->attached_connector->panel.
>                                 fixed_mode->vrefresh);
> @@ -5004,13 +5005,13 @@ void intel_edp_drrs_flush(struct drm_device *dev,
>         if (!dev_priv->drrs.dp)
>                 return;
>
> +       cancel_delayed_work_sync(&dev_priv->drrs.work);
> +
>         mutex_lock(&dev_priv->drrs.mutex);
>         crtc = dp_to_dig_port(dev_priv->drrs.dp)->base.base.crtc;
>         pipe = to_intel_crtc(crtc)->pipe;
>         dev_priv->drrs.busy_frontbuffer_bits &= ~frontbuffer_bits;
>
> -       cancel_delayed_work_sync(&dev_priv->drrs.work);
> -
>         if (dev_priv->drrs.refresh_rate_type != DRRS_LOW_RR &&
>                         !dev_priv->drrs.busy_frontbuffer_bits)
>                 schedule_delayed_work(&dev_priv->drrs.work,
> --
> 1.7.9.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Fixing mutex deadlock window at eDP DRRS

[Daniel Vetter]

On Wed, Mar 04, 2015 at 02:55:23PM -0800, Rodrigo Vivi wrote:
> Looks enough for me...
> 
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> 
> On Mon, Mar 2, 2015 at 10:41 PM, Ramalingam C <ramalingam.c@intel.com> wrote:
> > In invalidate and flush functions of eDP DRRS, if deferred downclock
> > work starts execution at a time window between acquiring the drrs
> > mutex and cancellation of the deferred work
> > (intel_edp_drrs_downclock_work), then deferred work will find
> > drrs mutex locked and wait for the same.
> >
> > Meanwhile the function that acquired mutex drrs invalidate/flush will
> > wait for the completion of the deferred work before releasing the mutex.
> > Thats a deadlock.
> >
> > To avoid such deadlock scenario, this change cancels the deferred work
> > before acquiring the mutex at invalidate and flush functions.
> >
> > Signed-off-by: Ramalingam C <ramalingam.c@intel.com>

Queued for -next, thanks for the patch.
-Daniel

> > ---
> >  drivers/gpu/drm/i915/intel_dp.c |    7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index d1141d3..0a57763 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -4966,12 +4966,13 @@ void intel_edp_drrs_invalidate(struct drm_device *dev,
> >         if (!dev_priv->drrs.dp)
> >                 return;
> >
> > +       cancel_delayed_work_sync(&dev_priv->drrs.work);
> > +
> >         mutex_lock(&dev_priv->drrs.mutex);
> >         crtc = dp_to_dig_port(dev_priv->drrs.dp)->base.base.crtc;
> >         pipe = to_intel_crtc(crtc)->pipe;
> >
> >         if (dev_priv->drrs.refresh_rate_type == DRRS_LOW_RR) {
> > -               cancel_delayed_work_sync(&dev_priv->drrs.work);
> >                 intel_dp_set_drrs_state(dev_priv->dev,
> >                                 dev_priv->drrs.dp->attached_connector->panel.
> >                                 fixed_mode->vrefresh);
> > @@ -5004,13 +5005,13 @@ void intel_edp_drrs_flush(struct drm_device *dev,
> >         if (!dev_priv->drrs.dp)
> >                 return;
> >
> > +       cancel_delayed_work_sync(&dev_priv->drrs.work);
> > +
> >         mutex_lock(&dev_priv->drrs.mutex);
> >         crtc = dp_to_dig_port(dev_priv->drrs.dp)->base.base.crtc;
> >         pipe = to_intel_crtc(crtc)->pipe;
> >         dev_priv->drrs.busy_frontbuffer_bits &= ~frontbuffer_bits;
> >
> > -       cancel_delayed_work_sync(&dev_priv->drrs.work);
> > -
> >         if (dev_priv->drrs.refresh_rate_type != DRRS_LOW_RR &&
> >                         !dev_priv->drrs.busy_frontbuffer_bits)
> >                 schedule_delayed_work(&dev_priv->drrs.work,
> > --
> > 1.7.9.5
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Rodrigo Vivi
> Blog: http://blog.vivi.eng.br

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx