| Message ID | 1301995458-2699-2-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 5 Apr 2011 10:24:14 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote: > crtc = intel_get_crtc_for_plane(dev, plane); > - if (crtc->fb == NULL || !crtc->enabled) > + if (crtc->fb == NULL || !crtc->enabled) { > + *cursor_wm = *plane_wm = display->guard_size; > return false; > + } Would it be clearer to have g4x_update_wm set these instead? I'm also a bit concerned about the default value; it would be lovely to have the docs say what the value should be for disabled pipes, but I couldn't find any mention of them.
On Tue, 05 Apr 2011 13:56:37 -0700, Keith Packard <keithp@keithp.com> wrote: > On Tue, 5 Apr 2011 10:24:14 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > > crtc = intel_get_crtc_for_plane(dev, plane); > > - if (crtc->fb == NULL || !crtc->enabled) > > + if (crtc->fb == NULL || !crtc->enabled) { > > + *cursor_wm = *plane_wm = display->guard_size; > > return false; > > + } > > Would it be clearer to have g4x_update_wm set these instead? > > I'm also a bit concerned about the default value; it would be lovely to > have the docs say what the value should be for disabled pipes, but I > couldn't find any mention of them. Indeed, I started by setting them to zero in the caller. Decided that there was some precedent to use the guard_size as the minimum value for unused planes (and so perhaps the unused planes on the unused pipes) and so it was then natural to do it inside g4x_compute_wm. I guess it all depends on how many FIFOs are split between the pipes. Using guard_size, I believe, should be safest. -Chris
On Tue, 05 Apr 2011 22:12:19 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote: > Indeed, I started by setting them to zero in the caller. Decided that > there was some precedent to use the guard_size as the minimum value for > unused planes (and so perhaps the unused planes on the unused pipes) and > so it was then natural to do it inside g4x_compute_wm. I guess it all > depends on how many FIFOs are split between the pipes. Using guard_size, > I believe, should be safest. guard_size is probably better than random stack stuff. Any opinion on whether this should be done in g4x_update_wm instead of g4x_compute_wm0?
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index e1c6da1..f1798f2 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -3800,8 +3800,10 @@ static bool g4x_compute_wm0(struct drm_device *dev, int entries, tlb_miss; crtc = intel_get_crtc_for_plane(dev, plane); - if (crtc->fb == NULL || !crtc->enabled) + if (crtc->fb == NULL || !crtc->enabled) { + *cursor_wm = *plane_wm = display->guard_size; return false; + } htotal = crtc->mode.htotal; hdisplay = crtc->mode.hdisplay;
We were using uninitialised watermarks values for disabled pipes which were combined into a single WM register and so corrupting the values for the enabled pipe and upsetting the display hardware. Reported-by: Riccardo Magliocchetti <riccardo.magliocchetti@gmail.com> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=32612 Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/intel_display.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-)