| Message ID | 1362172471-7643-15-git-send-email-jbarnes@virtuousgeek.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Mar 01, 2013 at 01:14:17PM -0800, Jesse Barnes wrote: > For current usage, not needed. > > Signed-off-by: Vijay Purushothaman <vijay.a.purushothaman@intel.com> > --- > drivers/gpu/drm/i915/intel_pm.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 7de8cec..b8f5a17 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -4211,7 +4211,9 @@ void intel_init_pm(struct drm_device *dev) > } else > dev_priv->display.update_wm = NULL; > } else if (IS_VALLEYVIEW(dev)) { > - dev_priv->display.update_wm = valleyview_update_wm; > +// dev_priv->display.update_wm = valleyview_update_wm; > + dev_priv->display.update_wm = NULL; > +// dev_priv->display.update_sprite_wm = valleyview_update_sprite_wm; Either kill this all (it's kzalloced so no need for NULL assignments) or add a giant comment explaining what's going on. Adding commented-out code without comment is strange ... -Daniel > dev_priv->display.init_clock_gating = > valleyview_init_clock_gating; > } else if (IS_PINEVIEW(dev)) { > -- > 1.7.9.5 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, Mar 06, 2013 at 07:56:33PM +0100, Daniel Vetter wrote: > On Fri, Mar 01, 2013 at 01:14:17PM -0800, Jesse Barnes wrote: > > For current usage, not needed. > > > > Signed-off-by: Vijay Purushothaman <vijay.a.purushothaman@intel.com> > > --- > > drivers/gpu/drm/i915/intel_pm.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > index 7de8cec..b8f5a17 100644 > > --- a/drivers/gpu/drm/i915/intel_pm.c > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > @@ -4211,7 +4211,9 @@ void intel_init_pm(struct drm_device *dev) > > } else > > dev_priv->display.update_wm = NULL; > > } else if (IS_VALLEYVIEW(dev)) { > > - dev_priv->display.update_wm = valleyview_update_wm; > > +// dev_priv->display.update_wm = valleyview_update_wm; > > + dev_priv->display.update_wm = NULL; > > +// dev_priv->display.update_sprite_wm = valleyview_update_sprite_wm; > > Either kill this all (it's kzalloced so no need for NULL assignments) or > add a giant comment explaining what's going on. Adding commented-out code > without comment is strange ... The whole premise that pondicherry magically handles things is a bit weird too. I suppose all it really means is that the default WMs were high enough to avoid underruns for the guys who tested this. Actually wasn't there a comment on an earlier version of this patch that there was a div by zero or something, and that's the real reason for this patch? > -Daniel > > > dev_priv->display.init_clock_gating = > > valleyview_init_clock_gating; > > } else if (IS_PINEVIEW(dev)) { > > -- > > 1.7.9.5 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, Mar 06, 2013 at 09:09:06PM +0200, Ville Syrjälä wrote: > On Wed, Mar 06, 2013 at 07:56:33PM +0100, Daniel Vetter wrote: > > On Fri, Mar 01, 2013 at 01:14:17PM -0800, Jesse Barnes wrote: > > > For current usage, not needed. > > > > > > Signed-off-by: Vijay Purushothaman <vijay.a.purushothaman@intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_pm.c | 4 +++- > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > > index 7de8cec..b8f5a17 100644 > > > --- a/drivers/gpu/drm/i915/intel_pm.c > > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > > @@ -4211,7 +4211,9 @@ void intel_init_pm(struct drm_device *dev) > > > } else > > > dev_priv->display.update_wm = NULL; > > > } else if (IS_VALLEYVIEW(dev)) { > > > - dev_priv->display.update_wm = valleyview_update_wm; > > > +// dev_priv->display.update_wm = valleyview_update_wm; > > > + dev_priv->display.update_wm = NULL; > > > +// dev_priv->display.update_sprite_wm = valleyview_update_sprite_wm; > > > > Either kill this all (it's kzalloced so no need for NULL assignments) or > > add a giant comment explaining what's going on. Adding commented-out code > > without comment is strange ... > > The whole premise that pondicherry magically handles things is a bit > weird too. I suppose all it really means is that the default WMs were > high enough to avoid underruns for the guys who tested this. > > Actually wasn't there a comment on an earlier version of this patch > that there was a div by zero or something, and that's the real reason > for this patch? Fyi the patch which fixed div-by-zero on other platforms is: commit 3490ea5de6ac4af309c3df8a26a5cca61306334c Author: Chris Wilson <chris@chris-wilson.co.uk> Date: Mon Jan 7 10:11:40 2013 +0000 drm/i915: Treat crtc->mode.clock == 0 as disabled Cheers, Daniel
On Wed, Mar 06, 2013 at 08:14:54PM +0100, Daniel Vetter wrote: > On Wed, Mar 06, 2013 at 09:09:06PM +0200, Ville Syrjälä wrote: > > On Wed, Mar 06, 2013 at 07:56:33PM +0100, Daniel Vetter wrote: > > > On Fri, Mar 01, 2013 at 01:14:17PM -0800, Jesse Barnes wrote: > > > > For current usage, not needed. > > > > > > > > Signed-off-by: Vijay Purushothaman <vijay.a.purushothaman@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/intel_pm.c | 4 +++- > > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > > > index 7de8cec..b8f5a17 100644 > > > > --- a/drivers/gpu/drm/i915/intel_pm.c > > > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > > > @@ -4211,7 +4211,9 @@ void intel_init_pm(struct drm_device *dev) > > > > } else > > > > dev_priv->display.update_wm = NULL; > > > > } else if (IS_VALLEYVIEW(dev)) { > > > > - dev_priv->display.update_wm = valleyview_update_wm; > > > > +// dev_priv->display.update_wm = valleyview_update_wm; > > > > + dev_priv->display.update_wm = NULL; > > > > +// dev_priv->display.update_sprite_wm = valleyview_update_sprite_wm; > > > > > > Either kill this all (it's kzalloced so no need for NULL assignments) or > > > add a giant comment explaining what's going on. Adding commented-out code > > > without comment is strange ... > > > > The whole premise that pondicherry magically handles things is a bit > > weird too. I suppose all it really means is that the default WMs were > > high enough to avoid underruns for the guys who tested this. > > > > Actually wasn't there a comment on an earlier version of this patch > > that there was a div by zero or something, and that's the real reason > > for this patch? > > Fyi the patch which fixed div-by-zero on other platforms is: > > commit 3490ea5de6ac4af309c3df8a26a5cca61306334c > Author: Chris Wilson <chris@chris-wilson.co.uk> > Date: Mon Jan 7 10:11:40 2013 +0000 > > drm/i915: Treat crtc->mode.clock == 0 as disabled Now I actually remember that Jani did try reverting this patch in his tree, and didn't get any div by zeros. So we were a bit confused about the patch at the time. If he had that fix in his tree, then that would explain it. Maybe this patch can be dropped then?
On Wed, Mar 06, 2013 at 09:19:27PM +0200, Ville Syrjälä wrote: > On Wed, Mar 06, 2013 at 08:14:54PM +0100, Daniel Vetter wrote: > > On Wed, Mar 06, 2013 at 09:09:06PM +0200, Ville Syrjälä wrote: > > > On Wed, Mar 06, 2013 at 07:56:33PM +0100, Daniel Vetter wrote: > > > > On Fri, Mar 01, 2013 at 01:14:17PM -0800, Jesse Barnes wrote: > > > > > For current usage, not needed. > > > > > > > > > > Signed-off-by: Vijay Purushothaman <vijay.a.purushothaman@intel.com> > > > > > --- > > > > > drivers/gpu/drm/i915/intel_pm.c | 4 +++- > > > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > > > > index 7de8cec..b8f5a17 100644 > > > > > --- a/drivers/gpu/drm/i915/intel_pm.c > > > > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > > > > @@ -4211,7 +4211,9 @@ void intel_init_pm(struct drm_device *dev) > > > > > } else > > > > > dev_priv->display.update_wm = NULL; > > > > > } else if (IS_VALLEYVIEW(dev)) { > > > > > - dev_priv->display.update_wm = valleyview_update_wm; > > > > > +// dev_priv->display.update_wm = valleyview_update_wm; > > > > > + dev_priv->display.update_wm = NULL; > > > > > +// dev_priv->display.update_sprite_wm = valleyview_update_sprite_wm; > > > > > > > > Either kill this all (it's kzalloced so no need for NULL assignments) or > > > > add a giant comment explaining what's going on. Adding commented-out code > > > > without comment is strange ... > > > > > > The whole premise that pondicherry magically handles things is a bit > > > weird too. I suppose all it really means is that the default WMs were > > > high enough to avoid underruns for the guys who tested this. > > > > > > Actually wasn't there a comment on an earlier version of this patch > > > that there was a div by zero or something, and that's the real reason > > > for this patch? > > > > Fyi the patch which fixed div-by-zero on other platforms is: > > > > commit 3490ea5de6ac4af309c3df8a26a5cca61306334c > > Author: Chris Wilson <chris@chris-wilson.co.uk> > > Date: Mon Jan 7 10:11:40 2013 +0000 > > > > drm/i915: Treat crtc->mode.clock == 0 as disabled > > Now I actually remember that Jani did try reverting this patch in his > tree, and didn't get any div by zeros. So we were a bit confused about > the patch at the time. If he had that fix in his tree, then that would > explain it. Maybe this patch can be dropped then? This is a fallout from the reworked modeset code. Since we now read out the bios hw state, if the new state leaves pipe B running, but enables pipe A (happens only when the bios enables pipe B) we try to recompute watermarks for pipe B, but that mode is 0. Hence boom. This patch can be killed once the hw state readout is properly extended to cover the required information (i.e. we need to move all the wm code to only look at pipe_config and ensure that all the values in there are correct). Once booted, you can't hit this any more. -Daniel
On Wed, 6 Mar 2013 20:32:22 +0100 Daniel Vetter <daniel@ffwll.ch> wrote: > On Wed, Mar 06, 2013 at 09:19:27PM +0200, Ville Syrjälä wrote: > > On Wed, Mar 06, 2013 at 08:14:54PM +0100, Daniel Vetter wrote: > > > On Wed, Mar 06, 2013 at 09:09:06PM +0200, Ville Syrjälä wrote: > > > > On Wed, Mar 06, 2013 at 07:56:33PM +0100, Daniel Vetter wrote: > > > > > On Fri, Mar 01, 2013 at 01:14:17PM -0800, Jesse Barnes wrote: > > > > > > For current usage, not needed. > > > > > > > > > > > > Signed-off-by: Vijay Purushothaman <vijay.a.purushothaman@intel.com> > > > > > > --- > > > > > > drivers/gpu/drm/i915/intel_pm.c | 4 +++- > > > > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > > > > > index 7de8cec..b8f5a17 100644 > > > > > > --- a/drivers/gpu/drm/i915/intel_pm.c > > > > > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > > > > > @@ -4211,7 +4211,9 @@ void intel_init_pm(struct drm_device *dev) > > > > > > } else > > > > > > dev_priv->display.update_wm = NULL; > > > > > > } else if (IS_VALLEYVIEW(dev)) { > > > > > > - dev_priv->display.update_wm = valleyview_update_wm; > > > > > > +// dev_priv->display.update_wm = valleyview_update_wm; > > > > > > + dev_priv->display.update_wm = NULL; > > > > > > +// dev_priv->display.update_sprite_wm = valleyview_update_sprite_wm; > > > > > > > > > > Either kill this all (it's kzalloced so no need for NULL assignments) or > > > > > add a giant comment explaining what's going on. Adding commented-out code > > > > > without comment is strange ... > > > > > > > > The whole premise that pondicherry magically handles things is a bit > > > > weird too. I suppose all it really means is that the default WMs were > > > > high enough to avoid underruns for the guys who tested this. > > > > > > > > Actually wasn't there a comment on an earlier version of this patch > > > > that there was a div by zero or something, and that's the real reason > > > > for this patch? > > > > > > Fyi the patch which fixed div-by-zero on other platforms is: > > > > > > commit 3490ea5de6ac4af309c3df8a26a5cca61306334c > > > Author: Chris Wilson <chris@chris-wilson.co.uk> > > > Date: Mon Jan 7 10:11:40 2013 +0000 > > > > > > drm/i915: Treat crtc->mode.clock == 0 as disabled > > > > Now I actually remember that Jani did try reverting this patch in his > > tree, and didn't get any div by zeros. So we were a bit confused about > > the patch at the time. If he had that fix in his tree, then that would > > explain it. Maybe this patch can be dropped then? > > This is a fallout from the reworked modeset code. Since we now read out > the bios hw state, if the new state leaves pipe B running, but enables > pipe A (happens only when the bios enables pipe B) we try to recompute > watermarks for pipe B, but that mode is 0. Hence boom. > > This patch can be killed once the hw state readout is properly extended to > cover the required information (i.e. we need to move all the wm code to > only look at pipe_config and ensure that all the values in there are > correct). > > Once booted, you can't hit this any more. I don't think this patch is necessary anymore... just tested and things work fine w/o so I'll drop.
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 7de8cec..b8f5a17 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -4211,7 +4211,9 @@ void intel_init_pm(struct drm_device *dev) } else dev_priv->display.update_wm = NULL; } else if (IS_VALLEYVIEW(dev)) { - dev_priv->display.update_wm = valleyview_update_wm; +// dev_priv->display.update_wm = valleyview_update_wm; + dev_priv->display.update_wm = NULL; +// dev_priv->display.update_sprite_wm = valleyview_update_sprite_wm; dev_priv->display.init_clock_gating = valleyview_init_clock_gating; } else if (IS_PINEVIEW(dev)) {
For current usage, not needed. Signed-off-by: Vijay Purushothaman <vijay.a.purushothaman@intel.com> --- drivers/gpu/drm/i915/intel_pm.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)