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

[Chris Wilson]

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...

> 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