Re: [PATCH 3/3] drm/i915: Use plane->state->fb in watermark code (v2)

[Matt Roper <matthew.d.roper@intel.com>]

On Tue, Mar 03, 2015 at 08:21:44AM -0300, Paulo Zanoni wrote:
> Hi
> 
> 2015-02-27 15:12 GMT-03:00 Matt Roper <matthew.d.roper@intel.com>:
> > plane->fb is a legacy pointer that not always be up-to-date (or updated
> > early enough).  Make sure the watermark code uses plane->state->fb so
> > that we're always doing our calculations based on the correct
> > framebuffers.
> 
> QA reported a regression caused by this patch: Kernel NULL pointer dereference.
> 
> https://bugs.freedesktop.org/show_bug.cgi?id=89388
> 

Yeah, I just saw this.  I'll have to look more closely later today, but
I'm wondering whether I should have just done this replacement
driver-wide instead of restricted to just the watermark code.  I suspect
killing off all of our uses of the old, legacy fields and using the new
atomic state values instead will make the driver more internally
consistent so we quit running into issues like this.


Matt

> 
> >
> > This patch was generated by Coccinelle with the following semantic
> > patch:
> >
> >         @@
> >         struct drm_plane *P;
> >         @@
> >         - P->fb
> >         + P->state->fb
> >
> > v2: Rebase
> >
> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_wm.c | 22 +++++++++++-----------
> >  1 file changed, 11 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_wm.c b/drivers/gpu/drm/i915/intel_wm.c
> > index 47a5175..e877e02 100644
> > --- a/drivers/gpu/drm/i915/intel_wm.c
> > +++ b/drivers/gpu/drm/i915/intel_wm.c
> > @@ -496,7 +496,7 @@ static void pineview_update_wm(struct drm_crtc *unused_crtc)
> >         crtc = single_enabled_crtc(dev);
> >         if (crtc) {
> >                 const struct drm_display_mode *adjusted_mode;
> > -               int pixel_size = crtc->primary->fb->bits_per_pixel / 8;
> > +               int pixel_size = crtc->primary->state->fb->bits_per_pixel / 8;
> >                 int clock;
> >
> >                 adjusted_mode = &to_intel_crtc(crtc)->config->base.adjusted_mode;
> > @@ -572,7 +572,7 @@ static bool g4x_compute_wm0(struct drm_device *dev,
> >         clock = adjusted_mode->crtc_clock;
> >         htotal = adjusted_mode->crtc_htotal;
> >         hdisplay = to_intel_crtc(crtc)->config->pipe_src_w;
> > -       pixel_size = crtc->primary->fb->bits_per_pixel / 8;
> > +       pixel_size = crtc->primary->state->fb->bits_per_pixel / 8;
> >
> >         /* Use the small buffer method to calculate plane watermark */
> >         entries = ((clock * pixel_size / 1000) * display_latency_ns) / 1000;
> > @@ -659,7 +659,7 @@ static bool g4x_compute_srwm(struct drm_device *dev,
> >         clock = adjusted_mode->crtc_clock;
> >         htotal = adjusted_mode->crtc_htotal;
> >         hdisplay = to_intel_crtc(crtc)->config->pipe_src_w;
> > -       pixel_size = crtc->primary->fb->bits_per_pixel / 8;
> > +       pixel_size = crtc->primary->state->fb->bits_per_pixel / 8;
> >
> >         line_time_us = max(htotal * 1000 / clock, 1);
> >         line_count = (latency_ns / line_time_us + 1000) / 1000;
> > @@ -742,7 +742,7 @@ static void vlv_update_drain_latency(struct drm_crtc *crtc)
> >         }
> >
> >         /* Primary plane Drain Latency */
> > -       pixel_size = crtc->primary->fb->bits_per_pixel / 8;     /* BPP */
> > +       pixel_size = crtc->primary->state->fb->bits_per_pixel / 8;      /* BPP */
> >         if (vlv_compute_drain_latency(crtc, pixel_size, &prec_mult, &drain_latency)) {
> >                 plane_prec = (prec_mult == high_precision) ?
> >                                            DDL_PLANE_PRECISION_HIGH :
> > @@ -1023,7 +1023,7 @@ static void i965_update_wm(struct drm_crtc *unused_crtc)
> >                 int clock = adjusted_mode->crtc_clock;
> >                 int htotal = adjusted_mode->crtc_htotal;
> >                 int hdisplay = to_intel_crtc(crtc)->config->pipe_src_w;
> > -               int pixel_size = crtc->primary->fb->bits_per_pixel / 8;
> > +               int pixel_size = crtc->primary->state->fb->bits_per_pixel / 8;
> >                 unsigned long line_time_us;
> >                 int entries;
> >
> > @@ -1100,7 +1100,7 @@ static void i9xx_update_wm(struct drm_crtc *unused_crtc)
> >         crtc = intel_get_crtc_for_plane(dev, 0);
> >         if (intel_crtc_active(crtc)) {
> >                 const struct drm_display_mode *adjusted_mode;
> > -               int cpp = crtc->primary->fb->bits_per_pixel / 8;
> > +               int cpp = crtc->primary->state->fb->bits_per_pixel / 8;
> >                 if (IS_GEN2(dev))
> >                         cpp = 4;
> >
> > @@ -1122,7 +1122,7 @@ static void i9xx_update_wm(struct drm_crtc *unused_crtc)
> >         crtc = intel_get_crtc_for_plane(dev, 1);
> >         if (intel_crtc_active(crtc)) {
> >                 const struct drm_display_mode *adjusted_mode;
> > -               int cpp = crtc->primary->fb->bits_per_pixel / 8;
> > +               int cpp = crtc->primary->state->fb->bits_per_pixel / 8;
> >                 if (IS_GEN2(dev))
> >                         cpp = 4;
> >
> > @@ -1145,7 +1145,7 @@ static void i9xx_update_wm(struct drm_crtc *unused_crtc)
> >         if (IS_I915GM(dev) && enabled) {
> >                 struct drm_i915_gem_object *obj;
> >
> > -               obj = intel_fb_obj(enabled->primary->fb);
> > +               obj = intel_fb_obj(enabled->primary->state->fb);
> >
> >                 /* self-refresh seems busted with untiled */
> >                 if (obj->tiling_mode == I915_TILING_NONE)
> > @@ -1169,7 +1169,7 @@ static void i9xx_update_wm(struct drm_crtc *unused_crtc)
> >                 int clock = adjusted_mode->crtc_clock;
> >                 int htotal = adjusted_mode->crtc_htotal;
> >                 int hdisplay = to_intel_crtc(enabled)->config->pipe_src_w;
> > -               int pixel_size = enabled->primary->fb->bits_per_pixel / 8;
> > +               int pixel_size = enabled->primary->state->fb->bits_per_pixel / 8;
> >                 unsigned long line_time_us;
> >                 int entries;
> >
> > @@ -1874,7 +1874,7 @@ static void ilk_compute_wm_parameters(struct drm_crtc *crtc,
> >         p->active = true;
> >         p->pipe_htotal = intel_crtc->config->base.adjusted_mode.crtc_htotal;
> >         p->pixel_rate = ilk_pipe_pixel_rate(dev, crtc);
> > -       p->pri.bytes_per_pixel = crtc->primary->fb->bits_per_pixel / 8;
> > +       p->pri.bytes_per_pixel = crtc->primary->state->fb->bits_per_pixel / 8;
> >         p->cur.bytes_per_pixel = 4;
> >         p->pri.horiz_pixels = intel_crtc->config->pipe_src_w;
> >         p->cur.horiz_pixels = intel_crtc->base.cursor->state->crtc_w;
> > @@ -2659,7 +2659,7 @@ static void skl_compute_wm_pipe_parameters(struct drm_crtc *crtc,
> >                  */
> >                 p->plane[0].enabled = true;
> >                 p->plane[0].bytes_per_pixel =
> > -                       crtc->primary->fb->bits_per_pixel / 8;
> > +                       crtc->primary->state->fb->bits_per_pixel / 8;
> >                 p->plane[0].horiz_pixels = intel_crtc->config->pipe_src_w;
> >                 p->plane[0].vert_pixels = intel_crtc->config->pipe_src_h;
> >                 p->plane[0].tiling = DRM_FORMAT_MOD_NONE;
> > --
> > 1.8.5.1
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Paulo Zanoni

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx