On Mon, Jul 07, 2014 at 06:21:48PM -0700, Matt Roper wrote: > This should hopefully simplify the display code slightly and also > solves at least one mistake in intel_pipe_set_base() where > to_intel_framebuffer(fb)->obj is referenced during local variable > initialization, before 'if (!fb)' gets checked. > > Potential uses of this macro were identified via the following > Coccinelle patch: > > @@ > expression E; > @@ > * to_intel_framebuffer(E)->obj > > @@ > expression E; > identifier I; > @@ > I = to_intel_framebuffer(E); > ... > * I->obj > > Signed-off-by: Matt Roper <matthew.d.roper@xxxxxxxxx> The only drawback is that I am suddenly nervous of potential NULL objs... > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 8c3dcdf..bc2519f 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -2356,7 +2356,7 @@ static void intel_find_plane_obj(struct intel_crtc *intel_crtc, > struct drm_device *dev = intel_crtc->base.dev; > struct drm_crtc *c; > struct intel_crtc *i; > - struct intel_framebuffer *fb; > + struct drm_i915_gem_object *obj; > > if (!intel_crtc->base.primary->fb) > return; > @@ -2380,11 +2380,11 @@ static void intel_find_plane_obj(struct intel_crtc *intel_crtc, > if (!i->active || !c->primary->fb) > continue; > > - fb = to_intel_framebuffer(c->primary->fb); > - if (i915_gem_obj_ggtt_offset(fb->obj) == plane_config->base) { > + obj = intel_fb_obj(c->primary->fb); I would move this before the c->primary->fb test and rewrite the check in terms of obj. if (!i->active) continue; obj = intel_fb_obj(c->primary->fb); if (obj == NULL) continue; > + if (i915_gem_obj_ggtt_offset(obj) == plane_config->base) { > drm_framebuffer_reference(c->primary->fb); > intel_crtc->base.primary->fb = c->primary->fb; > - fb->obj->frontbuffer_bits |= INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe); > + obj->frontbuffer_bits |= INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe); > break; > } > } > > @@ -9613,7 +9601,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc, > > work->event = event; > work->crtc = crtc; > - work->old_fb_obj = to_intel_framebuffer(old_fb)->obj; > + work->old_fb_obj = intel_fb_obj(old_fb); > INIT_WORK(&work->work, intel_unpin_work_fn); Here I would appreciate an if (WARN_ON(intel_fb_obj(old_fb) == NULL)) return -EBUSY; at the start of intel_crtc_page_flip. > > ret = drm_crtc_vblank_get(crtc); > @@ -12971,8 +12952,8 @@ void intel_modeset_gem_init(struct drm_device *dev) > if (!c->primary->fb) > continue; > > - fb = to_intel_framebuffer(c->primary->fb); > - if (intel_pin_and_fence_fb_obj(dev, fb->obj, NULL)) { > + obj = intel_fb_obj(c->primary->fb); Same again, change the check on primary->fb to be a check on obj. > + if (intel_pin_and_fence_fb_obj(dev, obj, NULL)) { > DRM_ERROR("failed to pin boot fb on pipe %d\n", > to_intel_crtc(c)->pipe); > drm_framebuffer_unreference(c->primary->fb); Elsewhere a GPF for a NULL pointer is reasonable. -Chris -- Chris Wilson, Intel Open Source Technology Centre _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx