[Intel-gfx] [PATCH 2/2] drm/i915: Make use of intel_fb_obj()

[Daniel Vetter]

On Tue, Jul 08, 2014 at 11:06:49AM +0100, Chris Wilson wrote:
> On Tue, Jul 08, 2014 at 11:51:18AM +0200, Daniel Vetter wrote:
> > On Tue, Jul 08, 2014 at 07:47:13AM +0100, Chris Wilson wrote:
> > > 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 at intel.com>
> > > 
> > > The only drawback is that I am suddenly nervous of potential NULL
> > > objs...
> > 
> > I concur with Chris here, I think intel_fb_obj should be a static inline
> > and first check for fb == NULL. To catch abuse we could do an
> > 
> > if (WARN_ON(!fb))
> > 	return NULL;
> > 
> > return to_intel_framebuffer(fb)->obj;
> > 
> > The most likely abuse is that we call intel_fb_obj before checking fb for
> > NULL, so the WARN is better than a BUG_ON. With that I don't think we need
> > to change the checks as Chris suggested here.
> 
> This came about because of one path where we should have expected fb to
> be NULL. Having a WARN followed by a GPF isn't any better than the GPF,
> in which case this patch is superfluous and you would rather just fix
> the single callsite where the bug occurred.

Ok, I've missed the context then. Anyway I like the idea of intel_fb_obj
and that it'll make the code more robust against misorderings of fb ==
NULL checks. Please ignore me and proceed ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch