| Message ID | 1442858564-27931-1-git-send-email-mahesh1.kumar@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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.
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
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;
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(-)