Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755004AbbESHfX (ORCPT ); Tue, 19 May 2015 03:35:23 -0400 Received: from mga01.intel.com ([192.55.52.88]:56179 "EHLO mga01.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754248AbbESHfP (ORCPT ); Tue, 19 May 2015 03:35:15 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.13,456,1427785200"; d="scan'208";a="728195146" From: Jani Nikula To: Matt Roper , Thomas Gummerer Cc: dri-devel@lists.freedesktop.org, intel-gfx@lists.freedesktop.org, linux-kernel@vger.kernel.org, David Airlie , Daniel Vetter , jan@gondor.com, Tvrtko Ursulin , Michael Leuchtenburg Subject: Re: [PATCH v2] drm/i915: fix screen flickering In-Reply-To: <20150514142836.GE25648@intel.com> Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo References: <20150514032118.GD25648@intel.com> <1431587799-5399-1-git-send-email-t.gummerer@gmail.com> <20150514142836.GE25648@intel.com> User-Agent: Notmuch/0.19+112~g77230b0 (http://notmuchmail.org) Emacs/24.4.1 (x86_64-pc-linux-gnu) Date: Tue, 19 May 2015 10:37:14 +0300 Message-ID: <87k2w5kogl.fsf@intel.com> MIME-Version: 1.0 Content-Type: text/plain Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3707 Lines: 106 On Thu, 14 May 2015, Matt Roper wrote: > On Thu, May 14, 2015 at 09:16:39AM +0200, Thomas Gummerer wrote: >> Commit c9f038a1a592 ("drm/i915: Don't assume primary & cursor are >> always on for wm calculation (v4)") fixes a null pointer dereference. >> Setting the primary and cursor panes to false in >> ilk_compute_wm_parameters to false does however give the following >> errors in the kernel log and causes the screen to flicker. >> >> [ 101.133716] [drm:intel_set_cpu_fifo_underrun_reporting [i915]] >> *ERROR* uncleared fifo underrun on pipe A >> [ 101.133725] [drm:intel_cpu_fifo_underrun_irq_handler [i915]] >> *ERROR* CPU pipe A FIFO underrun >> >> Always setting the panes to enabled fixes this error. >> >> Helped-by: Matt Roper >> Signed-off-by: Thomas Gummerer > > Seems like a reasonable short-term workaround and returns us to how the > code used to behaves. > > Reviewed-by: Matt Roper Pushed to drm-intel-fixes, thanks for the patch and review. BR, Jani. > >> --- >> >> > Sorry, I missed your patch when you first sent it. That type of fix >> > looks like an okay workaround while we do a more in-depth rework of the >> > watermark system. However I think your patch could cause a crash if we >> > disable the primary plane via the universal plane interface; if we do >> > that, p->pri.bytes_per_pixel is set to 0, but since we're now pretending >> > the primary plane is always enabled, ilk_wm_fbc() can eventually get >> > called and use that 0 in the denominator of a division operation. >> > >> > If you just squash the following change into your patch, I think it should be >> > safe: >> > [...] >> >> Thank you very much for the suggestion, here is an updated version of the >> patch. >> >> drivers/gpu/drm/i915/intel_pm.c | 24 +++++++++++------------- >> 1 file changed, 11 insertions(+), 13 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c >> index fa4ccb3..555b896 100644 >> --- a/drivers/gpu/drm/i915/intel_pm.c >> +++ b/drivers/gpu/drm/i915/intel_pm.c >> @@ -2045,22 +2045,20 @@ static void ilk_compute_wm_parameters(struct drm_crtc *crtc, >> p->pipe_htotal = intel_crtc->config->base.adjusted_mode.crtc_htotal; >> p->pixel_rate = ilk_pipe_pixel_rate(dev, crtc); >> >> - if (crtc->primary->state->fb) { >> - p->pri.enabled = true; >> + if (crtc->primary->state->fb) >> p->pri.bytes_per_pixel = >> crtc->primary->state->fb->bits_per_pixel / 8; >> - } else { >> - p->pri.enabled = false; >> - p->pri.bytes_per_pixel = 0; >> - } >> + else >> + p->pri.bytes_per_pixel = 4; >> + >> + p->cur.bytes_per_pixel = 4; >> + /* >> + * TODO: for now, assume primary and cursor planes are always enabled. >> + * Setting them to false makes the screen flicker. >> + */ >> + p->pri.enabled = true; >> + p->cur.enabled = true; >> >> - if (crtc->cursor->state->fb) { >> - p->cur.enabled = true; >> - p->cur.bytes_per_pixel = 4; >> - } else { >> - p->cur.enabled = false; >> - p->cur.bytes_per_pixel = 0; >> - } >> p->pri.horiz_pixels = intel_crtc->config->pipe_src_w; >> p->cur.horiz_pixels = intel_crtc->base.cursor->state->crtc_w; >> >> -- >> 2.4.0.184.g8e1974e >> > > -- > Matt Roper > Graphics Software Engineer > IoTG Platform Enabling & Development > Intel Corporation > (916) 356-2795 -- Jani Nikula, Intel Open Source Technology Center -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/