| Message ID | 1431587799-5399-1-git-send-email-t.gummerer@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <matthew.d.roper@intel.com> > Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com> Seems like a reasonable short-term workaround and returns us to how the code used to behaves. Reviewed-by: Matt Roper <matthew.d.roper@intel.com> > --- > > > 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 >
On Thu, 14 May 2015, Matt Roper <matthew.d.roper@intel.com> 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 <matthew.d.roper@intel.com> >> Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com> > > Seems like a reasonable short-term workaround and returns us to how the > code used to behaves. > > Reviewed-by: Matt Roper <matthew.d.roper@intel.com> 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
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;
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 <matthew.d.roper@intel.com> Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com> --- > 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(-)