* [PATCH] drm/i915/skl: Correct other-pipe watermark update condition check
@ 2015-09-21 18:02 Kumar, Mahesh
2015-10-16 16:45 ` Ville Syrjälä
2015-10-19 9:29 ` Daniel Vetter
0 siblings, 2 replies; 9+ messages in thread
From: Kumar, Mahesh @ 2015-09-21 18:02 UTC (permalink / raw)
To: intel-gfx
If ddb allocation for planes in current CRTC is changed, that doesn't
lead to ddb allocation change for other CRTCs, because our DDB allocation
is not dynamic according to plane parameters, ddb is allocated according
to number of CRTC enabled, & divided equally among CTRC's.
In current condition check during Watermark calculation, if number of
plane/ddb allocation changes for current CRTC, Watermark for other pipes
are recalculated. But there is no change in DDB allocation of other pipe
so watermark is also not changed, This leads to warning messages.
WARN_ON(!wm_changed)
This patch corrects this and check if DDB allocation for pipes is changed,
then only recalculate watermarks.
Signed-off-by: Kumar, Mahesh <mahesh1.kumar@intel.com>
---
drivers/gpu/drm/i915/intel_pm.c | 12 +++++-------
1 file changed, 5 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 62de97e..a1ed920 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3127,14 +3127,12 @@ static bool skl_ddb_allocation_changed(const struct skl_ddb_allocation *new_ddb,
struct drm_device *dev = intel_crtc->base.dev;
struct drm_i915_private *dev_priv = dev->dev_private;
const struct skl_ddb_allocation *cur_ddb = &dev_priv->wm.skl_hw.ddb;
- enum pipe pipe = intel_crtc->pipe;
-
- if (memcmp(new_ddb->plane[pipe], cur_ddb->plane[pipe],
- sizeof(new_ddb->plane[pipe])))
- return true;
- if (memcmp(&new_ddb->cursor[pipe], &cur_ddb->cursor[pipe],
- sizeof(new_ddb->cursor[pipe])))
+ /*
+ * If ddb allocation of pipes chenged, it may require recalculation of
+ * watermarks
+ */
+ if (memcmp(new_ddb->pipe, cur_ddb->pipe, sizeof(new_ddb->pipe)))
return true;
return false;
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/i915/skl: Correct other-pipe watermark update condition check
2015-09-21 18:02 [PATCH] drm/i915/skl: Correct other-pipe watermark update condition check Kumar, Mahesh
@ 2015-10-16 16:45 ` Ville Syrjälä
2015-10-19 9:29 ` Daniel Vetter
1 sibling, 0 replies; 9+ messages in thread
From: Ville Syrjälä @ 2015-10-16 16:45 UTC (permalink / raw)
To: Kumar, Mahesh; +Cc: intel-gfx
On Mon, Sep 21, 2015 at 11:32:44PM +0530, Kumar, Mahesh wrote:
> If ddb allocation for planes in current CRTC is changed, that doesn't
> lead to ddb allocation change for other CRTCs, because our DDB allocation
> is not dynamic according to plane parameters, ddb is allocated according
> to number of CRTC enabled, & divided equally among CTRC's.
>
> In current condition check during Watermark calculation, if number of
> plane/ddb allocation changes for current CRTC, Watermark for other pipes
> are recalculated. But there is no change in DDB allocation of other pipe
> so watermark is also not changed, This leads to warning messages.
> WARN_ON(!wm_changed)
>
> This patch corrects this and check if DDB allocation for pipes is changed,
> then only recalculate watermarks.
>
> Signed-off-by: Kumar, Mahesh <mahesh1.kumar@intel.com>
> ---
> drivers/gpu/drm/i915/intel_pm.c | 12 +++++-------
> 1 file changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 62de97e..a1ed920 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3127,14 +3127,12 @@ static bool skl_ddb_allocation_changed(const struct skl_ddb_allocation *new_ddb,
> struct drm_device *dev = intel_crtc->base.dev;
> struct drm_i915_private *dev_priv = dev->dev_private;
> const struct skl_ddb_allocation *cur_ddb = &dev_priv->wm.skl_hw.ddb;
> - enum pipe pipe = intel_crtc->pipe;
> -
> - if (memcmp(new_ddb->plane[pipe], cur_ddb->plane[pipe],
> - sizeof(new_ddb->plane[pipe])))
> - return true;
>
> - if (memcmp(&new_ddb->cursor[pipe], &cur_ddb->cursor[pipe],
> - sizeof(new_ddb->cursor[pipe])))
> + /*
> + * If ddb allocation of pipes chenged, it may require recalculation of
> + * watermarks
> + */
> + if (memcmp(new_ddb->pipe, cur_ddb->pipe, sizeof(new_ddb->pipe)))
> return true;
Hmm. Yeah we start from the previously calculated values for all pioes,
and just rewrite the entry for the current pipe, so doing the memset()
over the whole array is fine.
This function looks to have been this way from the start, and supposedly
I even reviewed it. Oh well, can't catch everything I guess.
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> return false;
> --
> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/i915/skl: Correct other-pipe watermark update condition check
2015-09-21 18:02 [PATCH] drm/i915/skl: Correct other-pipe watermark update condition check Kumar, Mahesh
2015-10-16 16:45 ` Ville Syrjälä
@ 2015-10-19 9:29 ` Daniel Vetter
2015-10-19 13:00 ` Ville Syrjälä
1 sibling, 1 reply; 9+ messages in thread
From: Daniel Vetter @ 2015-10-19 9:29 UTC (permalink / raw)
To: Kumar, Mahesh; +Cc: intel-gfx
On Mon, Sep 21, 2015 at 11:32:44PM +0530, Kumar, Mahesh wrote:
> If ddb allocation for planes in current CRTC is changed, that doesn't
> lead to ddb allocation change for other CRTCs, because our DDB allocation
> is not dynamic according to plane parameters, ddb is allocated according
> to number of CRTC enabled, & divided equally among CTRC's.
>
> In current condition check during Watermark calculation, if number of
> plane/ddb allocation changes for current CRTC, Watermark for other pipes
> are recalculated. But there is no change in DDB allocation of other pipe
> so watermark is also not changed, This leads to warning messages.
> WARN_ON(!wm_changed)
>
> This patch corrects this and check if DDB allocation for pipes is changed,
> then only recalculate watermarks.
>
> Signed-off-by: Kumar, Mahesh <mahesh1.kumar@intel.com>
> ---
> drivers/gpu/drm/i915/intel_pm.c | 12 +++++-------
> 1 file changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 62de97e..a1ed920 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3127,14 +3127,12 @@ static bool skl_ddb_allocation_changed(const struct skl_ddb_allocation *new_ddb,
> struct drm_device *dev = intel_crtc->base.dev;
> struct drm_i915_private *dev_priv = dev->dev_private;
> const struct skl_ddb_allocation *cur_ddb = &dev_priv->wm.skl_hw.ddb;
> - enum pipe pipe = intel_crtc->pipe;
> -
> - if (memcmp(new_ddb->plane[pipe], cur_ddb->plane[pipe],
> - sizeof(new_ddb->plane[pipe])))
> - return true;
>
> - if (memcmp(&new_ddb->cursor[pipe], &cur_ddb->cursor[pipe],
> - sizeof(new_ddb->cursor[pipe])))
> + /*
> + * If ddb allocation of pipes chenged, it may require recalculation of
> + * watermarks
> + */
> + if (memcmp(new_ddb->pipe, cur_ddb->pipe, sizeof(new_ddb->pipe)))
> return true;
I still have the cursor code here, this patch needs to be rebased.
-Daniel
>
> return false;
> --
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/i915/skl: Correct other-pipe watermark update condition check
2015-10-19 9:29 ` Daniel Vetter
@ 2015-10-19 13:00 ` Ville Syrjälä
2015-10-19 13:22 ` Daniel Vetter
0 siblings, 1 reply; 9+ messages in thread
From: Ville Syrjälä @ 2015-10-19 13:00 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx
On Mon, Oct 19, 2015 at 11:29:59AM +0200, Daniel Vetter wrote:
> On Mon, Sep 21, 2015 at 11:32:44PM +0530, Kumar, Mahesh wrote:
> > If ddb allocation for planes in current CRTC is changed, that doesn't
> > lead to ddb allocation change for other CRTCs, because our DDB allocation
> > is not dynamic according to plane parameters, ddb is allocated according
> > to number of CRTC enabled, & divided equally among CTRC's.
> >
> > In current condition check during Watermark calculation, if number of
> > plane/ddb allocation changes for current CRTC, Watermark for other pipes
> > are recalculated. But there is no change in DDB allocation of other pipe
> > so watermark is also not changed, This leads to warning messages.
> > WARN_ON(!wm_changed)
> >
> > This patch corrects this and check if DDB allocation for pipes is changed,
> > then only recalculate watermarks.
> >
> > Signed-off-by: Kumar, Mahesh <mahesh1.kumar@intel.com>
> > ---
> > drivers/gpu/drm/i915/intel_pm.c | 12 +++++-------
> > 1 file changed, 5 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 62de97e..a1ed920 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -3127,14 +3127,12 @@ static bool skl_ddb_allocation_changed(const struct skl_ddb_allocation *new_ddb,
> > struct drm_device *dev = intel_crtc->base.dev;
> > struct drm_i915_private *dev_priv = dev->dev_private;
> > const struct skl_ddb_allocation *cur_ddb = &dev_priv->wm.skl_hw.ddb;
> > - enum pipe pipe = intel_crtc->pipe;
> > -
> > - if (memcmp(new_ddb->plane[pipe], cur_ddb->plane[pipe],
> > - sizeof(new_ddb->plane[pipe])))
> > - return true;
> >
> > - if (memcmp(&new_ddb->cursor[pipe], &cur_ddb->cursor[pipe],
> > - sizeof(new_ddb->cursor[pipe])))
> > + /*
> > + * If ddb allocation of pipes chenged, it may require recalculation of
> > + * watermarks
> > + */
> > + if (memcmp(new_ddb->pipe, cur_ddb->pipe, sizeof(new_ddb->pipe)))
> > return true;
>
> I still have the cursor code here, this patch needs to be rebased.
I suppose you mean you have the other cursor code there, cause this one
has it too, just looking a bit different.
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/i915/skl: Correct other-pipe watermark update condition check
2015-10-19 13:00 ` Ville Syrjälä
@ 2015-10-19 13:22 ` Daniel Vetter
2015-10-23 16:41 ` [PATCH] drm/i915/skl: Correct other-pipe watermark update condition check (v2) Matt Roper
0 siblings, 1 reply; 9+ messages in thread
From: Daniel Vetter @ 2015-10-19 13:22 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: intel-gfx
On Mon, Oct 19, 2015 at 04:00:18PM +0300, Ville Syrjälä wrote:
> On Mon, Oct 19, 2015 at 11:29:59AM +0200, Daniel Vetter wrote:
> > On Mon, Sep 21, 2015 at 11:32:44PM +0530, Kumar, Mahesh wrote:
> > > If ddb allocation for planes in current CRTC is changed, that doesn't
> > > lead to ddb allocation change for other CRTCs, because our DDB allocation
> > > is not dynamic according to plane parameters, ddb is allocated according
> > > to number of CRTC enabled, & divided equally among CTRC's.
> > >
> > > In current condition check during Watermark calculation, if number of
> > > plane/ddb allocation changes for current CRTC, Watermark for other pipes
> > > are recalculated. But there is no change in DDB allocation of other pipe
> > > so watermark is also not changed, This leads to warning messages.
> > > WARN_ON(!wm_changed)
> > >
> > > This patch corrects this and check if DDB allocation for pipes is changed,
> > > then only recalculate watermarks.
> > >
> > > Signed-off-by: Kumar, Mahesh <mahesh1.kumar@intel.com>
> > > ---
> > > drivers/gpu/drm/i915/intel_pm.c | 12 +++++-------
> > > 1 file changed, 5 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > > index 62de97e..a1ed920 100644
> > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > @@ -3127,14 +3127,12 @@ static bool skl_ddb_allocation_changed(const struct skl_ddb_allocation *new_ddb,
> > > struct drm_device *dev = intel_crtc->base.dev;
> > > struct drm_i915_private *dev_priv = dev->dev_private;
> > > const struct skl_ddb_allocation *cur_ddb = &dev_priv->wm.skl_hw.ddb;
> > > - enum pipe pipe = intel_crtc->pipe;
> > > -
> > > - if (memcmp(new_ddb->plane[pipe], cur_ddb->plane[pipe],
> > > - sizeof(new_ddb->plane[pipe])))
> > > - return true;
> > >
> > > - if (memcmp(&new_ddb->cursor[pipe], &cur_ddb->cursor[pipe],
> > > - sizeof(new_ddb->cursor[pipe])))
> > > + /*
> > > + * If ddb allocation of pipes chenged, it may require recalculation of
> > > + * watermarks
> > > + */
> > > + if (memcmp(new_ddb->pipe, cur_ddb->pipe, sizeof(new_ddb->pipe)))
> > > return true;
> >
> > I still have the cursor code here, this patch needs to be rebased.
>
> I suppose you mean you have the other cursor code there, cause this one
> has it too, just looking a bit different.
Hm yeah right, just shows how little I understand from the wm code details
;-)
Anyway, please rebase since I can't make this one fit without too much
risk of me fumbling things.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] drm/i915/skl: Correct other-pipe watermark update condition check (v2)
2015-10-19 13:22 ` Daniel Vetter
@ 2015-10-23 16:41 ` Matt Roper
2015-10-27 6:16 ` Kumar, Mahesh
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Matt Roper @ 2015-10-23 16:41 UTC (permalink / raw)
To: intel-gfx
From: "Kumar, Mahesh" <mahesh1.kumar@intel.com>
If ddb allocation for planes in current CRTC is changed, that doesn't
lead to ddb allocation change for other CRTCs, because our DDB allocation
is not dynamic according to plane parameters, ddb is allocated according
to number of CRTC enabled, & divided equally among CTRC's.
In current condition check during Watermark calculation, if number of
plane/ddb allocation changes for current CRTC, Watermark for other pipes
are recalculated. But there is no change in DDB allocation of other pipe
so watermark is also not changed, This leads to warning messages.
WARN_ON(!wm_changed)
This patch corrects this and check if DDB allocation for pipes is changed,
then only recalculate watermarks.
v2 (by Matt): Rebased to latest -nightly and fixed a typo
Signed-off-by: Kumar, Mahesh <mahesh1.kumar@intel.com>
Reviewed-by(v1): Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
I don't think this solves all the !wm_changed warnings (I still see one during
fbdev restore following various igt tests), but it seems like a move in the
right direction so I figured I'd go ahead and rebase Mahesh' patch so it can
get merged.
drivers/gpu/drm/i915/intel_pm.c | 12 +++++-------
1 file changed, 5 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 0fb0459..0467e34 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3064,14 +3064,12 @@ static bool skl_ddb_allocation_changed(const struct skl_ddb_allocation *new_ddb,
struct drm_device *dev = intel_crtc->base.dev;
struct drm_i915_private *dev_priv = dev->dev_private;
const struct skl_ddb_allocation *cur_ddb = &dev_priv->wm.skl_hw.ddb;
- enum pipe pipe = intel_crtc->pipe;
-
- if (memcmp(new_ddb->plane[pipe], cur_ddb->plane[pipe],
- sizeof(new_ddb->plane[pipe])))
- return true;
- if (memcmp(&new_ddb->plane[pipe][PLANE_CURSOR], &cur_ddb->plane[pipe][PLANE_CURSOR],
- sizeof(new_ddb->plane[pipe][PLANE_CURSOR])))
+ /*
+ * If ddb allocation of pipes changed, it may require recalculation of
+ * watermarks
+ */
+ if (memcmp(new_ddb->pipe, cur_ddb->pipe, sizeof(new_ddb->pipe)))
return true;
return false;
--
2.1.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/i915/skl: Correct other-pipe watermark update condition check (v2)
2015-10-23 16:41 ` [PATCH] drm/i915/skl: Correct other-pipe watermark update condition check (v2) Matt Roper
@ 2015-10-27 6:16 ` Kumar, Mahesh
2015-11-17 14:16 ` Daniel Vetter
2016-02-22 16:42 ` Lyude
2 siblings, 0 replies; 9+ messages in thread
From: Kumar, Mahesh @ 2015-10-27 6:16 UTC (permalink / raw)
To: Matt Roper, intel-gfx
Yes, it doesn't solve all warnings.
Other than fbdev restore, there is one warning which I faced during
DPMS/Hot-plug, which is due to calculation of watermarks twice. Once for
first CRTC "Pipe-A" (during which it consider/calculates watermark for
other pipe "Pipe-B" as well & stores in structure
"intel_crtc->wm.active.skl") & Now in same atomic_commit, when it
calculate watermark for second CRTC "Pipe-B" , that time watermark for
other-pipe (in this case pipe-A) already have calculated values so it
gives !wm_changed warning.
Hoping 2-stage watermark programming will take care of this, as in that
case, If I'm not misunderstood final results should be stored after
calculation for all pipes.
Regards,
-Mahesh
On 10/23/2015 10:11 PM, Matt Roper wrote:
> From: "Kumar, Mahesh" <mahesh1.kumar@intel.com>
>
> If ddb allocation for planes in current CRTC is changed, that doesn't
> lead to ddb allocation change for other CRTCs, because our DDB allocation
> is not dynamic according to plane parameters, ddb is allocated according
> to number of CRTC enabled, & divided equally among CTRC's.
>
> In current condition check during Watermark calculation, if number of
> plane/ddb allocation changes for current CRTC, Watermark for other pipes
> are recalculated. But there is no change in DDB allocation of other pipe
> so watermark is also not changed, This leads to warning messages.
> WARN_ON(!wm_changed)
>
> This patch corrects this and check if DDB allocation for pipes is changed,
> then only recalculate watermarks.
>
> v2 (by Matt): Rebased to latest -nightly and fixed a typo
>
> Signed-off-by: Kumar, Mahesh <mahesh1.kumar@intel.com>
> Reviewed-by(v1): Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
> I don't think this solves all the !wm_changed warnings (I still see one during
> fbdev restore following various igt tests), but it seems like a move in the
> right direction so I figured I'd go ahead and rebase Mahesh' patch so it can
> get merged.
>
> drivers/gpu/drm/i915/intel_pm.c | 12 +++++-------
> 1 file changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 0fb0459..0467e34 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3064,14 +3064,12 @@ static bool skl_ddb_allocation_changed(const struct skl_ddb_allocation *new_ddb,
> struct drm_device *dev = intel_crtc->base.dev;
> struct drm_i915_private *dev_priv = dev->dev_private;
> const struct skl_ddb_allocation *cur_ddb = &dev_priv->wm.skl_hw.ddb;
> - enum pipe pipe = intel_crtc->pipe;
> -
> - if (memcmp(new_ddb->plane[pipe], cur_ddb->plane[pipe],
> - sizeof(new_ddb->plane[pipe])))
> - return true;
>
> - if (memcmp(&new_ddb->plane[pipe][PLANE_CURSOR], &cur_ddb->plane[pipe][PLANE_CURSOR],
> - sizeof(new_ddb->plane[pipe][PLANE_CURSOR])))
> + /*
> + * If ddb allocation of pipes changed, it may require recalculation of
> + * watermarks
> + */
> + if (memcmp(new_ddb->pipe, cur_ddb->pipe, sizeof(new_ddb->pipe)))
> return true;
>
> return false;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/i915/skl: Correct other-pipe watermark update condition check (v2)
2015-10-23 16:41 ` [PATCH] drm/i915/skl: Correct other-pipe watermark update condition check (v2) Matt Roper
2015-10-27 6:16 ` Kumar, Mahesh
@ 2015-11-17 14:16 ` Daniel Vetter
2016-02-22 16:42 ` Lyude
2 siblings, 0 replies; 9+ messages in thread
From: Daniel Vetter @ 2015-11-17 14:16 UTC (permalink / raw)
To: Matt Roper; +Cc: intel-gfx
On Fri, Oct 23, 2015 at 09:41:34AM -0700, Matt Roper wrote:
> From: "Kumar, Mahesh" <mahesh1.kumar@intel.com>
>
> If ddb allocation for planes in current CRTC is changed, that doesn't
> lead to ddb allocation change for other CRTCs, because our DDB allocation
> is not dynamic according to plane parameters, ddb is allocated according
> to number of CRTC enabled, & divided equally among CTRC's.
>
> In current condition check during Watermark calculation, if number of
> plane/ddb allocation changes for current CRTC, Watermark for other pipes
> are recalculated. But there is no change in DDB allocation of other pipe
> so watermark is also not changed, This leads to warning messages.
> WARN_ON(!wm_changed)
>
> This patch corrects this and check if DDB allocation for pipes is changed,
> then only recalculate watermarks.
>
> v2 (by Matt): Rebased to latest -nightly and fixed a typo
>
> Signed-off-by: Kumar, Mahesh <mahesh1.kumar@intel.com>
> Reviewed-by(v1): Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
> I don't think this solves all the !wm_changed warnings (I still see one during
> fbdev restore following various igt tests), but it seems like a move in the
> right direction so I figured I'd go ahead and rebase Mahesh' patch so it can
> get merged.
Merged to cut down on the noise (hopefully ...).
-Daniel
>
> drivers/gpu/drm/i915/intel_pm.c | 12 +++++-------
> 1 file changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 0fb0459..0467e34 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3064,14 +3064,12 @@ static bool skl_ddb_allocation_changed(const struct skl_ddb_allocation *new_ddb,
> struct drm_device *dev = intel_crtc->base.dev;
> struct drm_i915_private *dev_priv = dev->dev_private;
> const struct skl_ddb_allocation *cur_ddb = &dev_priv->wm.skl_hw.ddb;
> - enum pipe pipe = intel_crtc->pipe;
> -
> - if (memcmp(new_ddb->plane[pipe], cur_ddb->plane[pipe],
> - sizeof(new_ddb->plane[pipe])))
> - return true;
>
> - if (memcmp(&new_ddb->plane[pipe][PLANE_CURSOR], &cur_ddb->plane[pipe][PLANE_CURSOR],
> - sizeof(new_ddb->plane[pipe][PLANE_CURSOR])))
> + /*
> + * If ddb allocation of pipes changed, it may require recalculation of
> + * watermarks
> + */
> + if (memcmp(new_ddb->pipe, cur_ddb->pipe, sizeof(new_ddb->pipe)))
> return true;
>
> return false;
> --
> 2.1.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: drm/i915/skl: Correct other-pipe watermark update condition check (v2)
2015-10-23 16:41 ` [PATCH] drm/i915/skl: Correct other-pipe watermark update condition check (v2) Matt Roper
2015-10-27 6:16 ` Kumar, Mahesh
2015-11-17 14:16 ` Daniel Vetter
@ 2016-02-22 16:42 ` Lyude
2 siblings, 0 replies; 9+ messages in thread
From: Lyude @ 2016-02-22 16:42 UTC (permalink / raw)
To: Matt Roper, intel-gfx
Hey! I figured I should let you guys know while this fixes all of the issues
with normal display connectors, you still get WARN_ON(!wm_changed) warnings if
you try using DisplayPort MST, even with this patch.
Cheers,
Lyude
On Fri, 2015-10-23 at 09:41 -0700, Matt Roper wrote:
> From: "Kumar, Mahesh" <mahesh1.kumar@intel.com>
>
> If ddb allocation for planes in current CRTC is changed, that doesn't
> lead to ddb allocation change for other CRTCs, because our DDB allocation
> is not dynamic according to plane parameters, ddb is allocated according
> to number of CRTC enabled, & divided equally among CTRC's.
>
> In current condition check during Watermark calculation, if number of
> plane/ddb allocation changes for current CRTC, Watermark for other pipes
> are recalculated. But there is no change in DDB allocation of other pipe
> so watermark is also not changed, This leads to warning messages.
> WARN_ON(!wm_changed)
>
> This patch corrects this and check if DDB allocation for pipes is changed,
> then only recalculate watermarks.
>
> v2 (by Matt): Rebased to latest -nightly and fixed a typo
>
> Signed-off-by: Kumar, Mahesh <mahesh1.kumar@intel.com>
> Reviewed-by(v1): Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
> I don't think this solves all the !wm_changed warnings (I still see one during
> fbdev restore following various igt tests), but it seems like a move in the
> right direction so I figured I'd go ahead and rebase Mahesh' patch so it can
> get merged.
>
> drivers/gpu/drm/i915/intel_pm.c | 12 +++++-------
> 1 file changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 0fb0459..0467e34 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3064,14 +3064,12 @@ static bool skl_ddb_allocation_changed(const struct
> skl_ddb_allocation *new_ddb,
> struct drm_device *dev = intel_crtc->base.dev;
> struct drm_i915_private *dev_priv = dev->dev_private;
> const struct skl_ddb_allocation *cur_ddb = &dev_priv->wm.skl_hw.ddb;
> - enum pipe pipe = intel_crtc->pipe;
> -
> - if (memcmp(new_ddb->plane[pipe], cur_ddb->plane[pipe],
> - sizeof(new_ddb->plane[pipe])))
> - return true;
>
> - if (memcmp(&new_ddb->plane[pipe][PLANE_CURSOR], &cur_ddb-
> >plane[pipe][PLANE_CURSOR],
> - sizeof(new_ddb->plane[pipe][PLANE_CURSOR])))
> + /*
> + * If ddb allocation of pipes changed, it may require recalculation
> of
> + * watermarks
> + */
> + if (memcmp(new_ddb->pipe, cur_ddb->pipe, sizeof(new_ddb->pipe)))
> return true;
>
> return false;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2016-02-22 16:42 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-09-21 18:02 [PATCH] drm/i915/skl: Correct other-pipe watermark update condition check Kumar, Mahesh
2015-10-16 16:45 ` Ville Syrjälä
2015-10-19 9:29 ` Daniel Vetter
2015-10-19 13:00 ` Ville Syrjälä
2015-10-19 13:22 ` Daniel Vetter
2015-10-23 16:41 ` [PATCH] drm/i915/skl: Correct other-pipe watermark update condition check (v2) Matt Roper
2015-10-27 6:16 ` Kumar, Mahesh
2015-11-17 14:16 ` Daniel Vetter
2016-02-22 16:42 ` Lyude
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.