| Message ID | 1450321580-26269-1-git-send-email-radhakrishna.sripada@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 17 Dec 2015, Radhakrishna Sripada <radhakrishna.sripada@intel.com> wrote: > Original value of 32 blocks is not sufficient when using cursor size of > 256x256 causing FIFO underruns when the reworked wm > caluclations in > > commit 024c9045221fe45482863c47c4b4c47d37f97cbf > Author: Matt Roper <matthew.d.roper@intel.com> > Date: Thu Sep 24 15:53:11 2015 -0700 > > drm/i915/skl: Eliminate usage of pipe_wm_parameters from SKL-style WM (v4) > > are used. Increasing the number of blocks to 52 to make cursor plane tolerate > SAGV block time for the maximum possible cursor size. > > Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@intel.com> > Signed-off-by: Kalyan Kondapally <kalyan.kondapally@intel.com> Cc: drm-intel-fixes@lists.freedesktop.org > --- > drivers/gpu/drm/i915/intel_pm.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index d385d99..137fb68 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -2802,7 +2802,7 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev, > static unsigned int skl_cursor_allocation(const struct intel_wm_config *config) > { > if (config->num_pipes_active == 1) > - return 32; > + return 52; > > return 8; > }
On Wed, Dec 16, 2015 at 07:06:20PM -0800, Radhakrishna Sripada wrote: > Original value of 32 blocks is not sufficient when using cursor size of > 256x256 causing FIFO underruns when the reworked wm > caluclations in > > commit 024c9045221fe45482863c47c4b4c47d37f97cbf > Author: Matt Roper <matthew.d.roper@intel.com> > Date: Thu Sep 24 15:53:11 2015 -0700 > > drm/i915/skl: Eliminate usage of pipe_wm_parameters from SKL-style WM (v4) Well that commit is obviously incorrect. It's now using the pipe src width as the plane width for all planes. > > are used. Increasing the number of blocks to 52 to make cursor plane tolerate > SAGV block time for the maximum possible cursor size. > > Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@intel.com> > Signed-off-by: Kalyan Kondapally <kalyan.kondapally@intel.com> > --- > drivers/gpu/drm/i915/intel_pm.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index d385d99..137fb68 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -2802,7 +2802,7 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev, > static unsigned int skl_cursor_allocation(const struct intel_wm_config *config) > { > if (config->num_pipes_active == 1) > - return 32; > + return 52; > > return 8; > } > -- > 1.9.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, Dec 18, 2015 at 12:35:47PM +0200, Ville Syrjälä wrote: > On Wed, Dec 16, 2015 at 07:06:20PM -0800, Radhakrishna Sripada wrote: > > Original value of 32 blocks is not sufficient when using cursor size of > > 256x256 causing FIFO underruns when the reworked wm > > caluclations in > > > > commit 024c9045221fe45482863c47c4b4c47d37f97cbf > > Author: Matt Roper <matthew.d.roper@intel.com> > > Date: Thu Sep 24 15:53:11 2015 -0700 > > > > drm/i915/skl: Eliminate usage of pipe_wm_parameters from SKL-style WM (v4) > > Well that commit is obviously incorrect. It's now using the pipe src > width as the plane width for all planes. > Yeah, we already noted that bug in another email thread, but decided that it was unrelated to the problems Radhakrishna is facing. Radhakrishna is only using a cursor (which doesn't use that buggy function) and a full-screen primary plane, so the numbers don't actually change (for his use case). Of course that bug is still worth fixing too; I was planning on writing up a patch for it later today. Matt > > > > are used. Increasing the number of blocks to 52 to make cursor plane tolerate > > SAGV block time for the maximum possible cursor size. > > > > Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@intel.com> > > Signed-off-by: Kalyan Kondapally <kalyan.kondapally@intel.com> > > --- > > drivers/gpu/drm/i915/intel_pm.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > index d385d99..137fb68 100644 > > --- a/drivers/gpu/drm/i915/intel_pm.c > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > @@ -2802,7 +2802,7 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev, > > static unsigned int skl_cursor_allocation(const struct intel_wm_config *config) > > { > > if (config->num_pipes_active == 1) > > - return 32; > > + return 52; > > > > return 8; > > } > > -- > > 1.9.1 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Ville Syrjälä > Intel OTC
On Fri, Dec 18, 2015 at 06:58:58AM -0800, Matt Roper wrote: > On Fri, Dec 18, 2015 at 12:35:47PM +0200, Ville Syrjälä wrote: > > On Wed, Dec 16, 2015 at 07:06:20PM -0800, Radhakrishna Sripada wrote: > > > Original value of 32 blocks is not sufficient when using cursor size of > > > 256x256 causing FIFO underruns when the reworked wm > > > caluclations in > > > > > > commit 024c9045221fe45482863c47c4b4c47d37f97cbf > > > Author: Matt Roper <matthew.d.roper@intel.com> > > > Date: Thu Sep 24 15:53:11 2015 -0700 > > > > > > drm/i915/skl: Eliminate usage of pipe_wm_parameters from SKL-style WM (v4) > > > > Well that commit is obviously incorrect. It's now using the pipe src > > width as the plane width for all planes. > > > > Yeah, we already noted that bug in another email thread, but decided > that it was unrelated to the problems Radhakrishna is facing. > Radhakrishna is only using a cursor (which doesn't use that buggy > function) Pop quiz: what does it use then? > and a full-screen primary plane, so the numbers don't actually > change (for his use case). > > Of course that bug is still worth fixing too; I was planning on writing > up a patch for it later today. > > > Matt > > > > > > > > are used. Increasing the number of blocks to 52 to make cursor plane tolerate > > > SAGV block time for the maximum possible cursor size. > > > > > > Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@intel.com> > > > Signed-off-by: Kalyan Kondapally <kalyan.kondapally@intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_pm.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > > index d385d99..137fb68 100644 > > > --- a/drivers/gpu/drm/i915/intel_pm.c > > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > > @@ -2802,7 +2802,7 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev, > > > static unsigned int skl_cursor_allocation(const struct intel_wm_config *config) > > > { > > > if (config->num_pipes_active == 1) > > > - return 32; > > > + return 52; > > > > > > return 8; > > > } > > > -- > > > 1.9.1 > > > > > > _______________________________________________ > > > Intel-gfx mailing list > > > Intel-gfx@lists.freedesktop.org > > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > > Ville Syrjälä > > Intel OTC > > -- > Matt Roper > Graphics Software Engineer > IoTG Platform Enabling & Development > Intel Corporation > (916) 356-2795
On Fri, Dec 18, 2015 at 05:10:12PM +0200, Ville Syrjälä wrote: > On Fri, Dec 18, 2015 at 06:58:58AM -0800, Matt Roper wrote: > > On Fri, Dec 18, 2015 at 12:35:47PM +0200, Ville Syrjälä wrote: > > > On Wed, Dec 16, 2015 at 07:06:20PM -0800, Radhakrishna Sripada wrote: > > > > Original value of 32 blocks is not sufficient when using cursor size of > > > > 256x256 causing FIFO underruns when the reworked wm > > > > caluclations in > > > > > > > > commit 024c9045221fe45482863c47c4b4c47d37f97cbf > > > > Author: Matt Roper <matthew.d.roper@intel.com> > > > > Date: Thu Sep 24 15:53:11 2015 -0700 > > > > > > > > drm/i915/skl: Eliminate usage of pipe_wm_parameters from SKL-style WM (v4) > > > > > > Well that commit is obviously incorrect. It's now using the pipe src > > > width as the plane width for all planes. > > > > > > > Yeah, we already noted that bug in another email thread, but decided > > that it was unrelated to the problems Radhakrishna is facing. > > Radhakrishna is only using a cursor (which doesn't use that buggy > > function) > > Pop quiz: what does it use then? All non-cursor planes (i.e., primary+sprite). Cursors use a fixed DDB allocation (currently 32 blocks as suggested by bspec, but Radhakrishna's testing has found this to be too small, so his patch here is bumping that number up. Primary and sprite planes are supposed to divide up the remaining blocks proportional to their size, but the bug here causes them to all be considered full-screen size. If you're not actually using sprites and not windowing your primary plane, then the bug has no effect (which is probably why we didn't already catch and fix it). If you do use a sprite plane or window your primary plane, your proportions are probably wrong and you get non-optimal settings, although even then you usually won't have actual problems. Matt > > > and a full-screen primary plane, so the numbers don't actually > > change (for his use case). > > > > Of course that bug is still worth fixing too; I was planning on writing > > up a patch for it later today. > > > > > > Matt > > > > > > > > > > > > are used. Increasing the number of blocks to 52 to make cursor plane tolerate > > > > SAGV block time for the maximum possible cursor size. > > > > > > > > Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@intel.com> > > > > Signed-off-by: Kalyan Kondapally <kalyan.kondapally@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/intel_pm.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > > > index d385d99..137fb68 100644 > > > > --- a/drivers/gpu/drm/i915/intel_pm.c > > > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > > > @@ -2802,7 +2802,7 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev, > > > > static unsigned int skl_cursor_allocation(const struct intel_wm_config *config) > > > > { > > > > if (config->num_pipes_active == 1) > > > > - return 32; > > > > + return 52; > > > > > > > > return 8; > > > > } > > > > -- > > > > 1.9.1 > > > > > > > > _______________________________________________ > > > > Intel-gfx mailing list > > > > Intel-gfx@lists.freedesktop.org > > > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > > > -- > > > Ville Syrjälä > > > Intel OTC > > > > -- > > Matt Roper > > Graphics Software Engineer > > IoTG Platform Enabling & Development > > Intel Corporation > > (916) 356-2795 > > -- > Ville Syrjälä > Intel OTC
On Fri, Dec 18, 2015 at 07:14:17AM -0800, Matt Roper wrote: > On Fri, Dec 18, 2015 at 05:10:12PM +0200, Ville Syrjälä wrote: > > On Fri, Dec 18, 2015 at 06:58:58AM -0800, Matt Roper wrote: > > > On Fri, Dec 18, 2015 at 12:35:47PM +0200, Ville Syrjälä wrote: > > > > On Wed, Dec 16, 2015 at 07:06:20PM -0800, Radhakrishna Sripada wrote: > > > > > Original value of 32 blocks is not sufficient when using cursor size of > > > > > 256x256 causing FIFO underruns when the reworked wm > > > > > caluclations in > > > > > > > > > > commit 024c9045221fe45482863c47c4b4c47d37f97cbf > > > > > Author: Matt Roper <matthew.d.roper@intel.com> > > > > > Date: Thu Sep 24 15:53:11 2015 -0700 > > > > > > > > > > drm/i915/skl: Eliminate usage of pipe_wm_parameters from SKL-style WM (v4) > > > > > > > > Well that commit is obviously incorrect. It's now using the pipe src > > > > width as the plane width for all planes. > > > > > > > > > > Yeah, we already noted that bug in another email thread, but decided > > > that it was unrelated to the problems Radhakrishna is facing. > > > Radhakrishna is only using a cursor (which doesn't use that buggy > > > function) > > > > Pop quiz: what does it use then? > > All non-cursor planes (i.e., primary+sprite). Cursors use a fixed DDB > allocation (currently 32 blocks as suggested by bspec, but > Radhakrishna's testing has found this to be too small, so his patch here > is bumping that number up. > To elaborate on this some more, we have some suspicions about why the bspec-suggested 32 blocks might not be enough. There's some new workaround information in the bspec about sometimes needing to disable the system agent (SAGV) on non-BXT gen9, depending on the configuration and that's not something that's been implemented in our driver code yet. There's also another new workaround that checks raw system memory bandwidth and adjusts watermark programming that hasn't been implemented yet either. It could be that one of those two missing workarounds is the true culprit, but Radhakrishna's patch here looks like a reasonable short-term workaround; my main worry is that if he needs to bump the hardcoded 32 up to to 52 for the single pipe case, he might also need to bump the hardcoded 8 up as well for the multi-pipe case; I don't think anyone has tested that yet (and this seems to be a SKL-specific problem, so I can't reproduce it on my BXT). Matt > Primary and sprite planes are supposed to divide up the remaining blocks > proportional to their size, but the bug here causes them to all be > considered full-screen size. If you're not actually using sprites and > not windowing your primary plane, then the bug has no effect (which is > probably why we didn't already catch and fix it). If you do use a > sprite plane or window your primary plane, your proportions are probably > wrong and you get non-optimal settings, although even then you usually > won't have actual problems. > > > Matt > > > > > > > and a full-screen primary plane, so the numbers don't actually > > > change (for his use case). > > > > > > Of course that bug is still worth fixing too; I was planning on writing > > > up a patch for it later today. > > > > > > > > > Matt > > > > > > > > > > > > > > > > are used. Increasing the number of blocks to 52 to make cursor plane tolerate > > > > > SAGV block time for the maximum possible cursor size. > > > > > > > > > > Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@intel.com> > > > > > Signed-off-by: Kalyan Kondapally <kalyan.kondapally@intel.com> > > > > > --- > > > > > drivers/gpu/drm/i915/intel_pm.c | 2 +- > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > > > > index d385d99..137fb68 100644 > > > > > --- a/drivers/gpu/drm/i915/intel_pm.c > > > > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > > > > @@ -2802,7 +2802,7 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev, > > > > > static unsigned int skl_cursor_allocation(const struct intel_wm_config *config) > > > > > { > > > > > if (config->num_pipes_active == 1) > > > > > - return 32; > > > > > + return 52; > > > > > > > > > > return 8; > > > > > } > > > > > -- > > > > > 1.9.1 > > > > > > > > > > _______________________________________________ > > > > > Intel-gfx mailing list > > > > > Intel-gfx@lists.freedesktop.org > > > > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > > > > > -- > > > > Ville Syrjälä > > > > Intel OTC > > > > > > -- > > > Matt Roper > > > Graphics Software Engineer > > > IoTG Platform Enabling & Development > > > Intel Corporation > > > (916) 356-2795 > > > > -- > > Ville Syrjälä > > Intel OTC > > -- > Matt Roper > Graphics Software Engineer > IoTG Platform Enabling & Development > Intel Corporation > (916) 356-2795
On Fri, Dec 18, 2015 at 07:14:17AM -0800, Matt Roper wrote: > On Fri, Dec 18, 2015 at 05:10:12PM +0200, Ville Syrjälä wrote: > > On Fri, Dec 18, 2015 at 06:58:58AM -0800, Matt Roper wrote: > > > On Fri, Dec 18, 2015 at 12:35:47PM +0200, Ville Syrjälä wrote: > > > > On Wed, Dec 16, 2015 at 07:06:20PM -0800, Radhakrishna Sripada wrote: > > > > > Original value of 32 blocks is not sufficient when using cursor size of > > > > > 256x256 causing FIFO underruns when the reworked wm > > > > > caluclations in > > > > > > > > > > commit 024c9045221fe45482863c47c4b4c47d37f97cbf > > > > > Author: Matt Roper <matthew.d.roper@intel.com> > > > > > Date: Thu Sep 24 15:53:11 2015 -0700 > > > > > > > > > > drm/i915/skl: Eliminate usage of pipe_wm_parameters from SKL-style WM (v4) > > > > > > > > Well that commit is obviously incorrect. It's now using the pipe src > > > > width as the plane width for all planes. > > > > > > > > > > Yeah, we already noted that bug in another email thread, but decided > > > that it was unrelated to the problems Radhakrishna is facing. > > > Radhakrishna is only using a cursor (which doesn't use that buggy > > > function) > > > > Pop quiz: what does it use then? > > All non-cursor planes (i.e., primary+sprite). Cursors use a fixed DDB > allocation (currently 32 blocks as suggested by bspec, but > Radhakrishna's testing has found this to be too small, so his patch here > is bumping that number up. Oh I'm talking about the WM calculation, not the DDB allocation.
On Fri, Dec 18, 2015 at 07:24:19AM -0800, Matt Roper wrote: > On Fri, Dec 18, 2015 at 07:14:17AM -0800, Matt Roper wrote: > > On Fri, Dec 18, 2015 at 05:10:12PM +0200, Ville Syrjälä wrote: > > > On Fri, Dec 18, 2015 at 06:58:58AM -0800, Matt Roper wrote: > > > > On Fri, Dec 18, 2015 at 12:35:47PM +0200, Ville Syrjälä wrote: > > > > > On Wed, Dec 16, 2015 at 07:06:20PM -0800, Radhakrishna Sripada wrote: > > > > > > Original value of 32 blocks is not sufficient when using cursor size of > > > > > > 256x256 causing FIFO underruns when the reworked wm > > > > > > caluclations in > > > > > > > > > > > > commit 024c9045221fe45482863c47c4b4c47d37f97cbf > > > > > > Author: Matt Roper <matthew.d.roper@intel.com> > > > > > > Date: Thu Sep 24 15:53:11 2015 -0700 > > > > > > > > > > > > drm/i915/skl: Eliminate usage of pipe_wm_parameters from SKL-style WM (v4) > > > > > > > > > > Well that commit is obviously incorrect. It's now using the pipe src > > > > > width as the plane width for all planes. > > > > > > > > > > > > > Yeah, we already noted that bug in another email thread, but decided > > > > that it was unrelated to the problems Radhakrishna is facing. > > > > Radhakrishna is only using a cursor (which doesn't use that buggy > > > > function) > > > > > > Pop quiz: what does it use then? > > > > All non-cursor planes (i.e., primary+sprite). Cursors use a fixed DDB > > allocation (currently 32 blocks as suggested by bspec, but > > Radhakrishna's testing has found this to be too small, so his patch here > > is bumping that number up. > > > > To elaborate on this some more, we have some suspicions about why the > bspec-suggested 32 blocks might not be enough. There's some new > workaround information in the bspec about sometimes needing to disable > the system agent (SAGV) on non-BXT gen9, depending on the configuration > and that's not something that's been implemented in our driver code yet. > There's also another new workaround that checks raw system memory > bandwidth and adjusts watermark programming that hasn't been implemented > yet either. It could be that one of those two missing workarounds is > the true culprit, but Radhakrishna's patch here looks like a reasonable > short-term workaround; my main worry is that if he needs to bump the > hardcoded 32 up to to 52 for the single pipe case, he might also need to > bump the hardcoded 8 up as well for the multi-pipe case; I don't think > anyone has tested that yet (and this seems to be a SKL-specific problem, > so I can't reproduce it on my BXT). This needs at least a very big comment that we're just hacking around with duct-tape. Also allocating DDB shouldn't matter for correctness, as long as we compute the WM values correctly. I wonder how this fixes anything really (except making it a bit harder to hit perhaps due to the enlarged buffering it affords for the cursor). Imo better to just implement the other workarounds first, then see what's left. -Daniel > > > Matt > > > Primary and sprite planes are supposed to divide up the remaining blocks > > proportional to their size, but the bug here causes them to all be > > considered full-screen size. If you're not actually using sprites and > > not windowing your primary plane, then the bug has no effect (which is > > probably why we didn't already catch and fix it). If you do use a > > sprite plane or window your primary plane, your proportions are probably > > wrong and you get non-optimal settings, although even then you usually > > won't have actual problems. > > > > > > Matt > > > > > > > > > > > and a full-screen primary plane, so the numbers don't actually > > > > change (for his use case). > > > > > > > > Of course that bug is still worth fixing too; I was planning on writing > > > > up a patch for it later today. > > > > > > > > > > > > Matt > > > > > > > > > > > > > > > > > > > > are used. Increasing the number of blocks to 52 to make cursor plane tolerate > > > > > > SAGV block time for the maximum possible cursor size. > > > > > > > > > > > > Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@intel.com> > > > > > > Signed-off-by: Kalyan Kondapally <kalyan.kondapally@intel.com> > > > > > > --- > > > > > > drivers/gpu/drm/i915/intel_pm.c | 2 +- > > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > > > > > index d385d99..137fb68 100644 > > > > > > --- a/drivers/gpu/drm/i915/intel_pm.c > > > > > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > > > > > @@ -2802,7 +2802,7 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev, > > > > > > static unsigned int skl_cursor_allocation(const struct intel_wm_config *config) > > > > > > { > > > > > > if (config->num_pipes_active == 1) > > > > > > - return 32; > > > > > > + return 52; > > > > > > > > > > > > return 8; > > > > > > } > > > > > > -- > > > > > > 1.9.1 > > > > > > > > > > > > _______________________________________________ > > > > > > Intel-gfx mailing list > > > > > > Intel-gfx@lists.freedesktop.org > > > > > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > > > > > > > -- > > > > > Ville Syrjälä > > > > > Intel OTC > > > > > > > > -- > > > > Matt Roper > > > > Graphics Software Engineer > > > > IoTG Platform Enabling & Development > > > > Intel Corporation > > > > (916) 356-2795 > > > > > > -- > > > Ville Syrjälä > > > Intel OTC > > > > -- > > Matt Roper > > Graphics Software Engineer > > IoTG Platform Enabling & Development > > Intel Corporation > > (916) 356-2795 > > -- > Matt Roper > Graphics Software Engineer > IoTG Platform Enabling & Development > Intel Corporation > (916) 356-2795 > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 12/21/2015 05:39 PM, Daniel Vetter wrote: > On Fri, Dec 18, 2015 at 07:24:19AM -0800, Matt Roper wrote: >> On Fri, Dec 18, 2015 at 07:14:17AM -0800, Matt Roper wrote: >>> On Fri, Dec 18, 2015 at 05:10:12PM +0200, Ville Syrjälä wrote: >>>> On Fri, Dec 18, 2015 at 06:58:58AM -0800, Matt Roper wrote: >>>>> On Fri, Dec 18, 2015 at 12:35:47PM +0200, Ville Syrjälä wrote: >>>>>> On Wed, Dec 16, 2015 at 07:06:20PM -0800, Radhakrishna Sripada wrote: >>>>>>> Original value of 32 blocks is not sufficient when using cursor size of >>>>>>> 256x256 causing FIFO underruns when the reworked wm >>>>>>> caluclations in >>>>>>> >>>>>>> commit 024c9045221fe45482863c47c4b4c47d37f97cbf >>>>>>> Author: Matt Roper <matthew.d.roper@intel.com> >>>>>>> Date: Thu Sep 24 15:53:11 2015 -0700 >>>>>>> >>>>>>> drm/i915/skl: Eliminate usage of pipe_wm_parameters from SKL-style WM (v4) >>>>>> >>>>>> Well that commit is obviously incorrect. It's now using the pipe src >>>>>> width as the plane width for all planes. >>>>>> >>>>> >>>>> Yeah, we already noted that bug in another email thread, but decided >>>>> that it was unrelated to the problems Radhakrishna is facing. >>>>> Radhakrishna is only using a cursor (which doesn't use that buggy >>>>> function) >>>> >>>> Pop quiz: what does it use then? >>> >>> All non-cursor planes (i.e., primary+sprite). Cursors use a fixed DDB >>> allocation (currently 32 blocks as suggested by bspec, but >>> Radhakrishna's testing has found this to be too small, so his patch here >>> is bumping that number up. >>> >> >> To elaborate on this some more, we have some suspicions about why the >> bspec-suggested 32 blocks might not be enough. There's some new >> workaround information in the bspec about sometimes needing to disable >> the system agent (SAGV) on non-BXT gen9, depending on the configuration >> and that's not something that's been implemented in our driver code yet. >> There's also another new workaround that checks raw system memory >> bandwidth and adjusts watermark programming that hasn't been implemented >> yet either. It could be that one of those two missing workarounds is >> the true culprit, but Radhakrishna's patch here looks like a reasonable >> short-term workaround; my main worry is that if he needs to bump the >> hardcoded 32 up to to 52 for the single pipe case, he might also need to >> bump the hardcoded 8 up as well for the multi-pipe case; I don't think >> anyone has tested that yet (and this seems to be a SKL-specific problem, >> so I can't reproduce it on my BXT). > > This needs at least a very big comment that we're just hacking around with > duct-tape. Also allocating DDB shouldn't matter for correctness, as long > as we compute the WM values correctly. I wonder how this fixes anything > really (except making it a bit harder to hit perhaps due to the enlarged > buffering it affords for the cursor). Agree Daniel, but in our detailed testing this is found to have fixed reliably all known flickers. I recommend to have a detailed comment and *put* the duct-tape. I can have a rerun of detailed tests again to ensure that its indeed fixing everything. > > Imo better to just implement the other workarounds first, then see what's > left. I know there are few known workarounds pending. Lets continue implementing them and more. But with Matt's patch not fixing the issue, lets take this as an immediate short term fix and come back to remove the duct-tape when we have correct fix. Regards Shobhit > -Daniel > >> >> >> Matt >> >>> Primary and sprite planes are supposed to divide up the remaining blocks >>> proportional to their size, but the bug here causes them to all be >>> considered full-screen size. If you're not actually using sprites and >>> not windowing your primary plane, then the bug has no effect (which is >>> probably why we didn't already catch and fix it). If you do use a >>> sprite plane or window your primary plane, your proportions are probably >>> wrong and you get non-optimal settings, although even then you usually >>> won't have actual problems. >>> >>> >>> Matt >>> >>> >>>> >>>>> and a full-screen primary plane, so the numbers don't actually >>>>> change (for his use case). >>>>> >>>>> Of course that bug is still worth fixing too; I was planning on writing >>>>> up a patch for it later today. >>>>> >>>>> >>>>> Matt >>>>> >>>>> >>>>>>> >>>>>>> are used. Increasing the number of blocks to 52 to make cursor plane tolerate >>>>>>> SAGV block time for the maximum possible cursor size. >>>>>>> >>>>>>> Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@intel.com> >>>>>>> Signed-off-by: Kalyan Kondapally <kalyan.kondapally@intel.com> >>>>>>> --- >>>>>>> drivers/gpu/drm/i915/intel_pm.c | 2 +- >>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>>>> >>>>>>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c >>>>>>> index d385d99..137fb68 100644 >>>>>>> --- a/drivers/gpu/drm/i915/intel_pm.c >>>>>>> +++ b/drivers/gpu/drm/i915/intel_pm.c >>>>>>> @@ -2802,7 +2802,7 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev, >>>>>>> static unsigned int skl_cursor_allocation(const struct intel_wm_config *config) >>>>>>> { >>>>>>> if (config->num_pipes_active == 1) >>>>>>> - return 32; >>>>>>> + return 52; >>>>>>> >>>>>>> return 8; >>>>>>> } >>>>>>> -- >>>>>>> 1.9.1 >>>>>>> >>>>>>> _______________________________________________ >>>>>>> Intel-gfx mailing list >>>>>>> Intel-gfx@lists.freedesktop.org >>>>>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx >>>>>> >>>>>> -- >>>>>> Ville Syrjälä >>>>>> Intel OTC >>>>> >>>>> -- >>>>> Matt Roper >>>>> Graphics Software Engineer >>>>> IoTG Platform Enabling & Development >>>>> Intel Corporation >>>>> (916) 356-2795 >>>> >>>> -- >>>> Ville Syrjälä >>>> Intel OTC >>> >>> -- >>> Matt Roper >>> Graphics Software Engineer >>> IoTG Platform Enabling & Development >>> Intel Corporation >>> (916) 356-2795 >> >> -- >> Matt Roper >> Graphics Software Engineer >> IoTG Platform Enabling & Development >> Intel Corporation >> (916) 356-2795 >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx >
On 12/23/2015 08:22 AM, Kumar, Shobhit wrote: > On 12/21/2015 05:39 PM, Daniel Vetter wrote: >> On Fri, Dec 18, 2015 at 07:24:19AM -0800, Matt Roper wrote: >>> On Fri, Dec 18, 2015 at 07:14:17AM -0800, Matt Roper wrote: >>>> On Fri, Dec 18, 2015 at 05:10:12PM +0200, Ville Syrjälä wrote: >>>>> On Fri, Dec 18, 2015 at 06:58:58AM -0800, Matt Roper wrote: >>>>>> On Fri, Dec 18, 2015 at 12:35:47PM +0200, Ville Syrjälä wrote: >>>>>>> On Wed, Dec 16, 2015 at 07:06:20PM -0800, Radhakrishna Sripada >>>>>>> wrote: >>>>>>>> Original value of 32 blocks is not sufficient when using cursor >>>>>>>> size of >>>>>>>> 256x256 causing FIFO underruns when the reworked wm >>>>>>>> caluclations in >>>>>>>> >>>>>>>> commit 024c9045221fe45482863c47c4b4c47d37f97cbf >>>>>>>> Author: Matt Roper <matthew.d.roper@intel.com> >>>>>>>> Date: Thu Sep 24 15:53:11 2015 -0700 >>>>>>>> >>>>>>>> drm/i915/skl: Eliminate usage of pipe_wm_parameters from >>>>>>>> SKL-style WM (v4) >>>>>>> >>>>>>> Well that commit is obviously incorrect. It's now using the pipe src >>>>>>> width as the plane width for all planes. >>>>>>> >>>>>> >>>>>> Yeah, we already noted that bug in another email thread, but decided >>>>>> that it was unrelated to the problems Radhakrishna is facing. >>>>>> Radhakrishna is only using a cursor (which doesn't use that buggy >>>>>> function) >>>>> >>>>> Pop quiz: what does it use then? >>>> >>>> All non-cursor planes (i.e., primary+sprite). Cursors use a fixed DDB >>>> allocation (currently 32 blocks as suggested by bspec, but >>>> Radhakrishna's testing has found this to be too small, so his patch >>>> here >>>> is bumping that number up. >>>> >>> >>> To elaborate on this some more, we have some suspicions about why the >>> bspec-suggested 32 blocks might not be enough. There's some new >>> workaround information in the bspec about sometimes needing to disable >>> the system agent (SAGV) on non-BXT gen9, depending on the configuration >>> and that's not something that's been implemented in our driver code yet. >>> There's also another new workaround that checks raw system memory >>> bandwidth and adjusts watermark programming that hasn't been implemented >>> yet either. It could be that one of those two missing workarounds is >>> the true culprit, but Radhakrishna's patch here looks like a reasonable >>> short-term workaround; my main worry is that if he needs to bump the >>> hardcoded 32 up to to 52 for the single pipe case, he might also need to >>> bump the hardcoded 8 up as well for the multi-pipe case; I don't think >>> anyone has tested that yet (and this seems to be a SKL-specific problem, >>> so I can't reproduce it on my BXT). >> >> This needs at least a very big comment that we're just hacking around >> with >> duct-tape. Also allocating DDB shouldn't matter for correctness, as long >> as we compute the WM values correctly. I wonder how this fixes anything >> really (except making it a bit harder to hit perhaps due to the enlarged >> buffering it affords for the cursor). > > Agree Daniel, but in our detailed testing this is found to have fixed > reliably all known flickers. I recommend to have a detailed comment and > *put* the duct-tape. I can have a rerun of detailed tests again to > ensure that its indeed fixing everything. > >> >> Imo better to just implement the other workarounds first, then see what's >> left. > > I know there are few known workarounds pending. Lets continue > implementing them and more. But with Matt's patch not fixing the issue, > lets take this as an immediate short term fix and come back to remove > the duct-tape when we have correct fix. I experimented and found that reverting this patch but having arbitrated display bandwidth based workaround to increase latency values for all levels is fixing the flicker. Will be working on this along with Mahesh, unless Matt, you have a patch for this already. Regards Shobhit > > Regards > Shobhit > >> -Daniel >> >>> >>> >>> Matt >>> >>>> Primary and sprite planes are supposed to divide up the remaining >>>> blocks >>>> proportional to their size, but the bug here causes them to all be >>>> considered full-screen size. If you're not actually using sprites and >>>> not windowing your primary plane, then the bug has no effect (which is >>>> probably why we didn't already catch and fix it). If you do use a >>>> sprite plane or window your primary plane, your proportions are >>>> probably >>>> wrong and you get non-optimal settings, although even then you usually >>>> won't have actual problems. >>>> >>>> >>>> Matt >>>> >>>> >>>>> >>>>>> and a full-screen primary plane, so the numbers don't actually >>>>>> change (for his use case). >>>>>> >>>>>> Of course that bug is still worth fixing too; I was planning on >>>>>> writing >>>>>> up a patch for it later today. >>>>>> >>>>>> >>>>>> Matt >>>>>> >>>>>> >>>>>>>> >>>>>>>> are used. Increasing the number of blocks to 52 to make cursor >>>>>>>> plane tolerate >>>>>>>> SAGV block time for the maximum possible cursor size. >>>>>>>> >>>>>>>> Signed-off-by: Radhakrishna Sripada >>>>>>>> <radhakrishna.sripada@intel.com> >>>>>>>> Signed-off-by: Kalyan Kondapally <kalyan.kondapally@intel.com> >>>>>>>> --- >>>>>>>> drivers/gpu/drm/i915/intel_pm.c | 2 +- >>>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>>>>> >>>>>>>> diff --git a/drivers/gpu/drm/i915/intel_pm.c >>>>>>>> b/drivers/gpu/drm/i915/intel_pm.c >>>>>>>> index d385d99..137fb68 100644 >>>>>>>> --- a/drivers/gpu/drm/i915/intel_pm.c >>>>>>>> +++ b/drivers/gpu/drm/i915/intel_pm.c >>>>>>>> @@ -2802,7 +2802,7 @@ skl_ddb_get_pipe_allocation_limits(struct >>>>>>>> drm_device *dev, >>>>>>>> static unsigned int skl_cursor_allocation(const struct >>>>>>>> intel_wm_config *config) >>>>>>>> { >>>>>>>> if (config->num_pipes_active == 1) >>>>>>>> - return 32; >>>>>>>> + return 52; >>>>>>>> >>>>>>>> return 8; >>>>>>>> } >>>>>>>> -- >>>>>>>> 1.9.1 >>>>>>>> >>>>>>>> _______________________________________________ >>>>>>>> Intel-gfx mailing list >>>>>>>> Intel-gfx@lists.freedesktop.org >>>>>>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx >>>>>>> >>>>>>> -- >>>>>>> Ville Syrjälä >>>>>>> Intel OTC >>>>>> >>>>>> -- >>>>>> Matt Roper >>>>>> Graphics Software Engineer >>>>>> IoTG Platform Enabling & Development >>>>>> Intel Corporation >>>>>> (916) 356-2795 >>>>> >>>>> -- >>>>> Ville Syrjälä >>>>> Intel OTC >>>> >>>> -- >>>> Matt Roper >>>> Graphics Software Engineer >>>> IoTG Platform Enabling & Development >>>> Intel Corporation >>>> (916) 356-2795 >>> >>> -- >>> Matt Roper >>> Graphics Software Engineer >>> IoTG Platform Enabling & Development >>> Intel Corporation >>> (916) 356-2795 >>> _______________________________________________ >>> Intel-gfx mailing list >>> Intel-gfx@lists.freedesktop.org >>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx >> > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, Jan 04, 2016 at 07:18:26PM +0530, Kumar, Shobhit wrote: > On 12/23/2015 08:22 AM, Kumar, Shobhit wrote: > >On 12/21/2015 05:39 PM, Daniel Vetter wrote: > >>On Fri, Dec 18, 2015 at 07:24:19AM -0800, Matt Roper wrote: > >>>On Fri, Dec 18, 2015 at 07:14:17AM -0800, Matt Roper wrote: > >>>>On Fri, Dec 18, 2015 at 05:10:12PM +0200, Ville Syrjälä wrote: > >>>>>On Fri, Dec 18, 2015 at 06:58:58AM -0800, Matt Roper wrote: > >>>>>>On Fri, Dec 18, 2015 at 12:35:47PM +0200, Ville Syrjälä wrote: > >>>>>>>On Wed, Dec 16, 2015 at 07:06:20PM -0800, Radhakrishna Sripada > >>>>>>>wrote: > >>>>>>>>Original value of 32 blocks is not sufficient when using cursor > >>>>>>>>size of > >>>>>>>>256x256 causing FIFO underruns when the reworked wm > >>>>>>>>caluclations in > >>>>>>>> > >>>>>>>>commit 024c9045221fe45482863c47c4b4c47d37f97cbf > >>>>>>>>Author: Matt Roper <matthew.d.roper@intel.com> > >>>>>>>>Date: Thu Sep 24 15:53:11 2015 -0700 > >>>>>>>> > >>>>>>>> drm/i915/skl: Eliminate usage of pipe_wm_parameters from > >>>>>>>>SKL-style WM (v4) > >>>>>>> > >>>>>>>Well that commit is obviously incorrect. It's now using the pipe src > >>>>>>>width as the plane width for all planes. > >>>>>>> > >>>>>> > >>>>>>Yeah, we already noted that bug in another email thread, but decided > >>>>>>that it was unrelated to the problems Radhakrishna is facing. > >>>>>>Radhakrishna is only using a cursor (which doesn't use that buggy > >>>>>>function) > >>>>> > >>>>>Pop quiz: what does it use then? > >>>> > >>>>All non-cursor planes (i.e., primary+sprite). Cursors use a fixed DDB > >>>>allocation (currently 32 blocks as suggested by bspec, but > >>>>Radhakrishna's testing has found this to be too small, so his patch > >>>>here > >>>>is bumping that number up. > >>>> > >>> > >>>To elaborate on this some more, we have some suspicions about why the > >>>bspec-suggested 32 blocks might not be enough. There's some new > >>>workaround information in the bspec about sometimes needing to disable > >>>the system agent (SAGV) on non-BXT gen9, depending on the configuration > >>>and that's not something that's been implemented in our driver code yet. > >>>There's also another new workaround that checks raw system memory > >>>bandwidth and adjusts watermark programming that hasn't been implemented > >>>yet either. It could be that one of those two missing workarounds is > >>>the true culprit, but Radhakrishna's patch here looks like a reasonable > >>>short-term workaround; my main worry is that if he needs to bump the > >>>hardcoded 32 up to to 52 for the single pipe case, he might also need to > >>>bump the hardcoded 8 up as well for the multi-pipe case; I don't think > >>>anyone has tested that yet (and this seems to be a SKL-specific problem, > >>>so I can't reproduce it on my BXT). > >> > >>This needs at least a very big comment that we're just hacking around > >>with > >>duct-tape. Also allocating DDB shouldn't matter for correctness, as long > >>as we compute the WM values correctly. I wonder how this fixes anything > >>really (except making it a bit harder to hit perhaps due to the enlarged > >>buffering it affords for the cursor). > > > >Agree Daniel, but in our detailed testing this is found to have fixed > >reliably all known flickers. I recommend to have a detailed comment and > >*put* the duct-tape. I can have a rerun of detailed tests again to > >ensure that its indeed fixing everything. > > > >> > >>Imo better to just implement the other workarounds first, then see what's > >>left. > > > >I know there are few known workarounds pending. Lets continue > >implementing them and more. But with Matt's patch not fixing the issue, > >lets take this as an immediate short term fix and come back to remove > >the duct-tape when we have correct fix. > > I experimented and found that reverting this patch but having arbitrated > display bandwidth based workaround to increase latency values for all levels > is fixing the flicker. Will be working on this along with Mahesh, unless > Matt, you have a patch for this already. Excellent, really happy that we can go ahead with a reasonable looking fix right away here. Thanks, Daniel
On 01/05/2016 03:44 PM, Daniel Vetter wrote: > On Mon, Jan 04, 2016 at 07:18:26PM +0530, Kumar, Shobhit wrote: >> On 12/23/2015 08:22 AM, Kumar, Shobhit wrote: >>> On 12/21/2015 05:39 PM, Daniel Vetter wrote: >>>> On Fri, Dec 18, 2015 at 07:24:19AM -0800, Matt Roper wrote: >>>>> On Fri, Dec 18, 2015 at 07:14:17AM -0800, Matt Roper wrote: >>>>>> On Fri, Dec 18, 2015 at 05:10:12PM +0200, Ville Syrjälä wrote: >>>>>>> On Fri, Dec 18, 2015 at 06:58:58AM -0800, Matt Roper wrote: >>>>>>>> On Fri, Dec 18, 2015 at 12:35:47PM +0200, Ville Syrjälä wrote: >>>>>>>>> On Wed, Dec 16, 2015 at 07:06:20PM -0800, Radhakrishna Sripada >>>>>>>>> wrote: >>>>>>>>>> Original value of 32 blocks is not sufficient when using cursor >>>>>>>>>> size of >>>>>>>>>> 256x256 causing FIFO underruns when the reworked wm >>>>>>>>>> caluclations in >>>>>>>>>> >>>>>>>>>> commit 024c9045221fe45482863c47c4b4c47d37f97cbf >>>>>>>>>> Author: Matt Roper <matthew.d.roper@intel.com> >>>>>>>>>> Date: Thu Sep 24 15:53:11 2015 -0700 >>>>>>>>>> >>>>>>>>>> drm/i915/skl: Eliminate usage of pipe_wm_parameters from >>>>>>>>>> SKL-style WM (v4) >>>>>>>>> >>>>>>>>> Well that commit is obviously incorrect. It's now using the pipe src >>>>>>>>> width as the plane width for all planes. >>>>>>>>> >>>>>>>> >>>>>>>> Yeah, we already noted that bug in another email thread, but decided >>>>>>>> that it was unrelated to the problems Radhakrishna is facing. >>>>>>>> Radhakrishna is only using a cursor (which doesn't use that buggy >>>>>>>> function) >>>>>>> >>>>>>> Pop quiz: what does it use then? >>>>>> >>>>>> All non-cursor planes (i.e., primary+sprite). Cursors use a fixed DDB >>>>>> allocation (currently 32 blocks as suggested by bspec, but >>>>>> Radhakrishna's testing has found this to be too small, so his patch >>>>>> here >>>>>> is bumping that number up. >>>>>> >>>>> >>>>> To elaborate on this some more, we have some suspicions about why the >>>>> bspec-suggested 32 blocks might not be enough. There's some new >>>>> workaround information in the bspec about sometimes needing to disable >>>>> the system agent (SAGV) on non-BXT gen9, depending on the configuration >>>>> and that's not something that's been implemented in our driver code yet. >>>>> There's also another new workaround that checks raw system memory >>>>> bandwidth and adjusts watermark programming that hasn't been implemented >>>>> yet either. It could be that one of those two missing workarounds is >>>>> the true culprit, but Radhakrishna's patch here looks like a reasonable >>>>> short-term workaround; my main worry is that if he needs to bump the >>>>> hardcoded 32 up to to 52 for the single pipe case, he might also need to >>>>> bump the hardcoded 8 up as well for the multi-pipe case; I don't think >>>>> anyone has tested that yet (and this seems to be a SKL-specific problem, >>>>> so I can't reproduce it on my BXT). >>>> >>>> This needs at least a very big comment that we're just hacking around >>>> with >>>> duct-tape. Also allocating DDB shouldn't matter for correctness, as long >>>> as we compute the WM values correctly. I wonder how this fixes anything >>>> really (except making it a bit harder to hit perhaps due to the enlarged >>>> buffering it affords for the cursor). >>> >>> Agree Daniel, but in our detailed testing this is found to have fixed >>> reliably all known flickers. I recommend to have a detailed comment and >>> *put* the duct-tape. I can have a rerun of detailed tests again to >>> ensure that its indeed fixing everything. >>> >>>> >>>> Imo better to just implement the other workarounds first, then see what's >>>> left. >>> >>> I know there are few known workarounds pending. Lets continue >>> implementing them and more. But with Matt's patch not fixing the issue, >>> lets take this as an immediate short term fix and come back to remove >>> the duct-tape when we have correct fix. >> >> I experimented and found that reverting this patch but having arbitrated >> display bandwidth based workaround to increase latency values for all levels >> is fixing the flicker. Will be working on this along with Mahesh, unless >> Matt, you have a patch for this already. > > Excellent, really happy that we can go ahead with a reasonable looking fix > right away here. Daniel, we implemented the memory latency increase for quick test and it worked, but in reality that situation was not hitting even with a QHD eDP and 4k@60 DP with a two channel 1867 MHz memory. So this was not a solution. There have been another patch series to try to fix this that I posted - https://patchwork.freedesktop.org/patch/71758/ and we have been trying with lot of Matt's patches for watermark, and along with what I posted but all of this has not yielded perfect under run error free results though improved a little. But cursor DDB bump for both internal and external pipe is resulting in complete fix of the problem. As of now that is the only solution I can think off and has been extensively tested with QHD eDP/ 19x10 eDP + 4k DP/HDMI and working reliably. I think we should be dynamically allocating cursor DDB as well so that it will increase the DDB if cursor size is big. But for now proposal is to statically increase these cursor DDB to 52 for single pipe as in this patch and additionally have it 32 in case of multi pipe scenario. Once we have all water mark thingy sorted out, we can come back and re-look at this. Regards Shobhit > > Thanks, Daniel >
On 02/16/2016 03:44 PM, Kumar, Shobhit wrote: > On 01/05/2016 03:44 PM, Daniel Vetter wrote: >> On Mon, Jan 04, 2016 at 07:18:26PM +0530, Kumar, Shobhit wrote: >>> On 12/23/2015 08:22 AM, Kumar, Shobhit wrote: >>>> On 12/21/2015 05:39 PM, Daniel Vetter wrote: >>>>> On Fri, Dec 18, 2015 at 07:24:19AM -0800, Matt Roper wrote: >>>>>> On Fri, Dec 18, 2015 at 07:14:17AM -0800, Matt Roper wrote: >>>>>>> On Fri, Dec 18, 2015 at 05:10:12PM +0200, Ville Syrjälä wrote: >>>>>>>> On Fri, Dec 18, 2015 at 06:58:58AM -0800, Matt Roper wrote: >>>>>>>>> On Fri, Dec 18, 2015 at 12:35:47PM +0200, Ville Syrjälä wrote: >>>>>>>>>> On Wed, Dec 16, 2015 at 07:06:20PM -0800, Radhakrishna Sripada >>>>>>>>>> wrote: >>>>>>>>>>> Original value of 32 blocks is not sufficient when using cursor >>>>>>>>>>> size of >>>>>>>>>>> 256x256 causing FIFO underruns when the reworked wm >>>>>>>>>>> caluclations in >>>>>>>>>>> >>>>>>>>>>> commit 024c9045221fe45482863c47c4b4c47d37f97cbf >>>>>>>>>>> Author: Matt Roper <matthew.d.roper@intel.com> >>>>>>>>>>> Date: Thu Sep 24 15:53:11 2015 -0700 >>>>>>>>>>> >>>>>>>>>>> drm/i915/skl: Eliminate usage of pipe_wm_parameters from >>>>>>>>>>> SKL-style WM (v4) >>>>>>>>>> >>>>>>>>>> Well that commit is obviously incorrect. It's now using the >>>>>>>>>> pipe src >>>>>>>>>> width as the plane width for all planes. >>>>>>>>>> >>>>>>>>> >>>>>>>>> Yeah, we already noted that bug in another email thread, but >>>>>>>>> decided >>>>>>>>> that it was unrelated to the problems Radhakrishna is facing. >>>>>>>>> Radhakrishna is only using a cursor (which doesn't use that buggy >>>>>>>>> function) >>>>>>>> >>>>>>>> Pop quiz: what does it use then? >>>>>>> >>>>>>> All non-cursor planes (i.e., primary+sprite). Cursors use a >>>>>>> fixed DDB >>>>>>> allocation (currently 32 blocks as suggested by bspec, but >>>>>>> Radhakrishna's testing has found this to be too small, so his patch >>>>>>> here >>>>>>> is bumping that number up. >>>>>>> >>>>>> >>>>>> To elaborate on this some more, we have some suspicions about why the >>>>>> bspec-suggested 32 blocks might not be enough. There's some new >>>>>> workaround information in the bspec about sometimes needing to >>>>>> disable >>>>>> the system agent (SAGV) on non-BXT gen9, depending on the >>>>>> configuration >>>>>> and that's not something that's been implemented in our driver >>>>>> code yet. >>>>>> There's also another new workaround that checks raw system memory >>>>>> bandwidth and adjusts watermark programming that hasn't been >>>>>> implemented >>>>>> yet either. It could be that one of those two missing workarounds is >>>>>> the true culprit, but Radhakrishna's patch here looks like a >>>>>> reasonable >>>>>> short-term workaround; my main worry is that if he needs to bump the >>>>>> hardcoded 32 up to to 52 for the single pipe case, he might also >>>>>> need to >>>>>> bump the hardcoded 8 up as well for the multi-pipe case; I don't >>>>>> think >>>>>> anyone has tested that yet (and this seems to be a SKL-specific >>>>>> problem, >>>>>> so I can't reproduce it on my BXT). >>>>> >>>>> This needs at least a very big comment that we're just hacking around >>>>> with >>>>> duct-tape. Also allocating DDB shouldn't matter for correctness, as >>>>> long >>>>> as we compute the WM values correctly. I wonder how this fixes >>>>> anything >>>>> really (except making it a bit harder to hit perhaps due to the >>>>> enlarged >>>>> buffering it affords for the cursor). >>>> >>>> Agree Daniel, but in our detailed testing this is found to have fixed >>>> reliably all known flickers. I recommend to have a detailed comment and >>>> *put* the duct-tape. I can have a rerun of detailed tests again to >>>> ensure that its indeed fixing everything. >>>> >>>>> >>>>> Imo better to just implement the other workarounds first, then see >>>>> what's >>>>> left. >>>> >>>> I know there are few known workarounds pending. Lets continue >>>> implementing them and more. But with Matt's patch not fixing the issue, >>>> lets take this as an immediate short term fix and come back to remove >>>> the duct-tape when we have correct fix. >>> >>> I experimented and found that reverting this patch but having arbitrated >>> display bandwidth based workaround to increase latency values for all >>> levels >>> is fixing the flicker. Will be working on this along with Mahesh, unless >>> Matt, you have a patch for this already. >> >> Excellent, really happy that we can go ahead with a reasonable looking >> fix >> right away here. > > Daniel, we implemented the memory latency increase for quick test and it > worked, but in reality that situation was not hitting even with a QHD > eDP and 4k@60 DP with a two channel 1867 MHz memory. So this was not a > solution. > > There have been another patch series to try to fix this that I posted - > https://patchwork.freedesktop.org/patch/71758/ > > and we have been trying with lot of Matt's patches for watermark, and > along with what I posted but all of this has not yielded perfect under > run error free results though improved a little. > > But cursor DDB bump for both internal and external pipe is resulting in > complete fix of the problem. As of now that is the only solution I can > think off and has been extensively tested with QHD eDP/ 19x10 eDP + 4k > DP/HDMI and working reliably. I think we should be dynamically > allocating cursor DDB as well so that it will increase the DDB if cursor > size is big. > > But for now proposal is to statically increase these cursor DDB to 52 > for single pipe as in this patch and additionally have it 32 in case of > multi pipe scenario. Once we have all water mark thingy sorted out, we > can come back and re-look at this. Daniel, any comments or suggestions ? Regards Shobhit > > Regards > Shobhit > > >> >> Thanks, Daniel >> > _______________________________________________ > 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_pm.c b/drivers/gpu/drm/i915/intel_pm.c index d385d99..137fb68 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -2802,7 +2802,7 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev, static unsigned int skl_cursor_allocation(const struct intel_wm_config *config) { if (config->num_pipes_active == 1) - return 32; + return 52; return 8; }
Original value of 32 blocks is not sufficient when using cursor size of 256x256 causing FIFO underruns when the reworked wm caluclations in commit 024c9045221fe45482863c47c4b4c47d37f97cbf Author: Matt Roper <matthew.d.roper@intel.com> Date: Thu Sep 24 15:53:11 2015 -0700 drm/i915/skl: Eliminate usage of pipe_wm_parameters from SKL-style WM (v4) are used. Increasing the number of blocks to 52 to make cursor plane tolerate SAGV block time for the maximum possible cursor size. Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@intel.com> Signed-off-by: Kalyan Kondapally <kalyan.kondapally@intel.com> --- drivers/gpu/drm/i915/intel_pm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)