| Message ID | 1461805865-212590-16-git-send-email-tom.orourke@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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