[15/21] drm/i915/slpc: Notification of Refresh Rate change

Submitted by tom.orourke@intel.com on April 28, 2016, 1:10 a.m.

Details

Message ID 1461805865-212590-16-git-send-email-tom.orourke@intel.com
State New
Headers show
Series "Add support for GuC-based SLPC" ( rev: 4 ) in Intel GFX

Not browsing as part of any series.

Commit Message

tom.orourke@intel.com April 28, 2016, 1:10 a.m.
From: Sagar Arun Kamble <sagar.a.kamble@intel.com>

This patch will inform GuC SLPC about changes in the refresh rate
due to Seamless DRRS. Refresh rate changes due to Static DRRS will
be notified via commit path.

v2: Rebased on previous changed patch and printed error message if
H2G action fails.
v2(torourke): Updates suggested by Paulo: replace HAS_SLPC with
intel_slpc_active. return void instead of ignored error code.

Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Signed-off-by: Tom O'Rourke <Tom.O'Rourke@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c   |  2 ++
 drivers/gpu/drm/i915/intel_slpc.c | 23 +++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_slpc.h |  1 +
 3 files changed, 26 insertions(+)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index c12c414..3d41f7b 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -5379,6 +5379,8 @@  static void intel_dp_set_drrs_state(struct drm_device *dev, int refresh_rate)
 	dev_priv->drrs.refresh_rate_type = index;
 
 	DRM_DEBUG_KMS("eDP Refresh Rate set to : %dHz\n", refresh_rate);
+
+	intel_slpc_update_display_rr_info(dev, refresh_rate);
 }
 
 /**
diff --git a/drivers/gpu/drm/i915/intel_slpc.c b/drivers/gpu/drm/i915/intel_slpc.c
index 7f26284..9e0bc96 100644
--- a/drivers/gpu/drm/i915/intel_slpc.c
+++ b/drivers/gpu/drm/i915/intel_slpc.c
@@ -364,3 +364,26 @@  void intel_slpc_update_atomic_commit_info(struct drm_device *dev,
 	if (notify)
 		host2guc_slpc_display_mode_change(dev);
 }
+
+void intel_slpc_update_display_rr_info(struct drm_device *dev, u32 refresh_rate)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_crtc *crtc;
+	struct intel_display_pipe_info *per_pipe_info;
+	struct intel_slpc_display_mode_event_params *display_params;
+
+	if (!intel_slpc_active(dev))
+		return;
+
+	if (!refresh_rate)
+		return;
+
+	display_params = &dev_priv->guc.slpc.display_mode_params;
+	crtc = dp_to_dig_port(dev_priv->drrs.dp)->base.base.crtc;
+
+	per_pipe_info = &display_params->per_pipe_info[to_intel_crtc(crtc)->pipe];
+	per_pipe_info->refresh_rate = refresh_rate;
+	per_pipe_info->vsync_ft_usec = 1000000 / refresh_rate;
+
+	host2guc_slpc_display_mode_change(dev);
+}
diff --git a/drivers/gpu/drm/i915/intel_slpc.h b/drivers/gpu/drm/i915/intel_slpc.h
index 39b4657..0b251a1 100644
--- a/drivers/gpu/drm/i915/intel_slpc.h
+++ b/drivers/gpu/drm/i915/intel_slpc.h
@@ -153,5 +153,6 @@  void intel_slpc_reset(struct drm_device *dev);
 void intel_slpc_update_display_mode_info(struct drm_device *dev);
 void intel_slpc_update_atomic_commit_info(struct drm_device *dev,
 					  struct drm_atomic_state *state);
+void intel_slpc_update_display_rr_info(struct drm_device *dev, u32 refresh_rate);
 
 #endif

Comments

On Wed, Apr 27, 2016 at 06:10:59PM -0700, tom.orourke@intel.com wrote:
> From: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> 
> This patch will inform GuC SLPC about changes in the refresh rate
> due to Seamless DRRS. Refresh rate changes due to Static DRRS will
> be notified via commit path.
> 
> v2: Rebased on previous changed patch and printed error message if
> H2G action fails.
> v2(torourke): Updates suggested by Paulo: replace HAS_SLPC with
> intel_slpc_active. return void instead of ignored error code.
> 
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> Signed-off-by: Tom O'Rourke <Tom.O'Rourke@intel.com>

So all this display notification stuff looks real fancy, but we have it
already in the kernel. We notice when we're late in a frame, and then
aggressively ramp up.

I want to see an implementation that reuses that infrastructure and just
tells guc to hurry up, and then benchmark against this one (wrt overall
frame latency distribution in spikey workloads). All this complexity and
an entire 2nd codepath needs to be justified in a unified driver, and I
see exactly none of that going on.

Also we need to figure out what kind of exact&minimal lie we need to feed
guc in this case, that needs some collab with the firmware team.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_dp.c   |  2 ++
>  drivers/gpu/drm/i915/intel_slpc.c | 23 +++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_slpc.h |  1 +
>  3 files changed, 26 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index c12c414..3d41f7b 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -5379,6 +5379,8 @@ static void intel_dp_set_drrs_state(struct drm_device *dev, int refresh_rate)
>  	dev_priv->drrs.refresh_rate_type = index;
>  
>  	DRM_DEBUG_KMS("eDP Refresh Rate set to : %dHz\n", refresh_rate);
> +
> +	intel_slpc_update_display_rr_info(dev, refresh_rate);
>  }
>  
>  /**
> diff --git a/drivers/gpu/drm/i915/intel_slpc.c b/drivers/gpu/drm/i915/intel_slpc.c
> index 7f26284..9e0bc96 100644
> --- a/drivers/gpu/drm/i915/intel_slpc.c
> +++ b/drivers/gpu/drm/i915/intel_slpc.c
> @@ -364,3 +364,26 @@ void intel_slpc_update_atomic_commit_info(struct drm_device *dev,
>  	if (notify)
>  		host2guc_slpc_display_mode_change(dev);
>  }
> +
> +void intel_slpc_update_display_rr_info(struct drm_device *dev, u32 refresh_rate)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct drm_crtc *crtc;
> +	struct intel_display_pipe_info *per_pipe_info;
> +	struct intel_slpc_display_mode_event_params *display_params;
> +
> +	if (!intel_slpc_active(dev))
> +		return;
> +
> +	if (!refresh_rate)
> +		return;
> +
> +	display_params = &dev_priv->guc.slpc.display_mode_params;
> +	crtc = dp_to_dig_port(dev_priv->drrs.dp)->base.base.crtc;
> +
> +	per_pipe_info = &display_params->per_pipe_info[to_intel_crtc(crtc)->pipe];
> +	per_pipe_info->refresh_rate = refresh_rate;
> +	per_pipe_info->vsync_ft_usec = 1000000 / refresh_rate;
> +
> +	host2guc_slpc_display_mode_change(dev);
> +}
> diff --git a/drivers/gpu/drm/i915/intel_slpc.h b/drivers/gpu/drm/i915/intel_slpc.h
> index 39b4657..0b251a1 100644
> --- a/drivers/gpu/drm/i915/intel_slpc.h
> +++ b/drivers/gpu/drm/i915/intel_slpc.h
> @@ -153,5 +153,6 @@ void intel_slpc_reset(struct drm_device *dev);
>  void intel_slpc_update_display_mode_info(struct drm_device *dev);
>  void intel_slpc_update_atomic_commit_info(struct drm_device *dev,
>  					  struct drm_atomic_state *state);
> +void intel_slpc_update_display_rr_info(struct drm_device *dev, u32 refresh_rate);
>  
>  #endif
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, Apr 28, 2016 at 10:38:27AM +0200, Daniel Vetter wrote:
> On Wed, Apr 27, 2016 at 06:10:59PM -0700, tom.orourke@intel.com wrote:
> > From: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> > 
> > This patch will inform GuC SLPC about changes in the refresh rate
> > due to Seamless DRRS. Refresh rate changes due to Static DRRS will
> > be notified via commit path.
> > 
> > v2: Rebased on previous changed patch and printed error message if
> > H2G action fails.
> > v2(torourke): Updates suggested by Paulo: replace HAS_SLPC with
> > intel_slpc_active. return void instead of ignored error code.
> > 
> > Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> > Signed-off-by: Tom O'Rourke <Tom.O'Rourke@intel.com>
> 
> So all this display notification stuff looks real fancy, but we have it
> already in the kernel. We notice when we're late in a frame, and then
> aggressively ramp up.
> 
> I want to see an implementation that reuses that infrastructure and just
> tells guc to hurry up, and then benchmark against this one (wrt overall
> frame latency distribution in spikey workloads). All this complexity and
> an entire 2nd codepath needs to be justified in a unified driver, and I
> see exactly none of that going on.

+ power consumption benchmarks ofc, since current code also aggressively
downclocks again when the burst is over. My concern here is that
fundamentally guc just doesn't know enough to make a good decision here,
or at least it can react only much later. Whereas the kernel actually
knows what atomic display update it has pending, and what exactly it needs
to boost to get that to the screen asap.
-Daniel
On Wed, Apr 27, 2016 at 06:10:59PM -0700, tom.orourke@intel.com wrote:
> From: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> 
> This patch will inform GuC SLPC about changes in the refresh rate
> due to Seamless DRRS. Refresh rate changes due to Static DRRS will
> be notified via commit path.
> 
> v2: Rebased on previous changed patch and printed error message if
> H2G action fails.
> v2(torourke): Updates suggested by Paulo: replace HAS_SLPC with
> intel_slpc_active. return void instead of ignored error code.
> 
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> Signed-off-by: Tom O'Rourke <Tom.O'Rourke@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c   |  2 ++
>  drivers/gpu/drm/i915/intel_slpc.c | 23 +++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_slpc.h |  1 +
>  3 files changed, 26 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index c12c414..3d41f7b 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -5379,6 +5379,8 @@ static void intel_dp_set_drrs_state(struct drm_device *dev, int refresh_rate)
>  	dev_priv->drrs.refresh_rate_type = index;
>  
>  	DRM_DEBUG_KMS("eDP Refresh Rate set to : %dHz\n", refresh_rate);
> +
> +	intel_slpc_update_display_rr_info(dev, refresh_rate);
>  }
>  
>  /**
> diff --git a/drivers/gpu/drm/i915/intel_slpc.c b/drivers/gpu/drm/i915/intel_slpc.c
> index 7f26284..9e0bc96 100644
> --- a/drivers/gpu/drm/i915/intel_slpc.c
> +++ b/drivers/gpu/drm/i915/intel_slpc.c
> @@ -364,3 +364,26 @@ void intel_slpc_update_atomic_commit_info(struct drm_device *dev,
>  	if (notify)
>  		host2guc_slpc_display_mode_change(dev);
>  }
> +
> +void intel_slpc_update_display_rr_info(struct drm_device *dev, u32 refresh_rate)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct drm_crtc *crtc;
> +	struct intel_display_pipe_info *per_pipe_info;
> +	struct intel_slpc_display_mode_event_params *display_params;
> +
> +	if (!intel_slpc_active(dev))
> +		return;
> +
> +	if (!refresh_rate)
> +		return;
> +
> +	display_params = &dev_priv->guc.slpc.display_mode_params;
> +	crtc = dp_to_dig_port(dev_priv->drrs.dp)->base.base.crtc;
> +
> +	per_pipe_info = &display_params->per_pipe_info[to_intel_crtc(crtc)->pipe];
> +	per_pipe_info->refresh_rate = refresh_rate;
> +	per_pipe_info->vsync_ft_usec = 1000000 / refresh_rate;
> +
> +	host2guc_slpc_display_mode_change(dev);
> +}

Other people already commented, but I'll just add that this in any case
looks rather bogus. Only updated for a single pipe when it has drrs
enabled, non-drrs and all the other pipes left out. What's that about?

> diff --git a/drivers/gpu/drm/i915/intel_slpc.h b/drivers/gpu/drm/i915/intel_slpc.h
> index 39b4657..0b251a1 100644
> --- a/drivers/gpu/drm/i915/intel_slpc.h
> +++ b/drivers/gpu/drm/i915/intel_slpc.h
> @@ -153,5 +153,6 @@ void intel_slpc_reset(struct drm_device *dev);
>  void intel_slpc_update_display_mode_info(struct drm_device *dev);
>  void intel_slpc_update_atomic_commit_info(struct drm_device *dev,
>  					  struct drm_atomic_state *state);
> +void intel_slpc_update_display_rr_info(struct drm_device *dev, u32 refresh_rate);
>  
>  #endif
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx