Re: [PATCH 4/4] drm/i915: Intel-specific primary plane handling (v6)

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

On Thu, May 15, 2014 at 06:52:28PM +0300, Ville Syrjälä wrote:
...
> 
> > +	};
> > +	bool visible;
> > +	int ret;
> > +
> > +	ret = drm_primary_helper_check_update(plane, crtc, fb,
> > +					      &src, &dest, &clip,
> > +					      DRM_PLANE_HELPER_NO_SCALING,
> > +					      DRM_PLANE_HELPER_NO_SCALING,
> > +					      false, true, &visible);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (!visible)
> > +		return intel_primary_plane_disable(plane);
> 
> Here we unpin the old fb...
> 
> > +
> > +	/*
> > +	 * If the CRTC isn't enabled, we're just pinning the framebuffer,
> > +	 * updating the fb pointer, and returning without touching the
> > +	 * hardware.  This allows us to later do a drmModeSetCrtc with fb=-1 to
> > +	 * turn on the display with all planes setup as desired.
> > +	 */
> > +	if (!crtc->enabled)
> > +		/* Pin and return without programming hardware */
> > +		return intel_pin_and_fence_fb_obj(dev,
> > +						  to_intel_framebuffer(fb)->obj,
> > +						  NULL);
> 
> ...but here we just pin the new fb and leave the old also pinned?
> 
> Something's a bit fishy here. Also a non-enabled crtc but visible primary
> plane doesn't seem to make sense. We also need to remember that set_base
> will always unpin the old_fb. So if something can result in set_base
> being called w/ old_fb!=NULL when it's not pinned, we'll be in deep
> doodoo.

Right, I guess we need to unpin the old fb, if any, here as well in case
they perform several setplane() calls while the crtc is disabled.

Eventually the crtc will get reenabled by a drmModeSetCrtc call.  If we
do setcrtc(fb = -1), then it should keep whatever fb we've already
pinned via the setplane.  If they provide a new fb, then the pinning
we're doing here will get unpinned by the set_base that gets called.

I don't see a way that you can hit set_base with an unpinned
old_fb!=NULL (since the disable plane case farther up also clears
primary->fb).


Matt

> 
> > +
> > +	intel_crtc_wait_for_pending_flips(crtc);
> > +	ret = intel_pipe_set_base(crtc, src.x1, src.y1, fb);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (!intel_crtc->primary_enabled)
> > +		intel_enable_primary_hw_plane(dev_priv, intel_crtc->plane,
> > +					      intel_crtc->pipe);
> > +
> > +	return 0;
> > +}
> > +
> > +static void intel_primary_plane_destroy(struct drm_plane *plane)
> > +{
> > +	struct intel_plane *intel_plane = to_intel_plane(plane);
> > +	drm_plane_cleanup(plane);
> > +	kfree(intel_plane);
> > +}
> > +
> > +static const struct drm_plane_funcs intel_primary_plane_funcs = {
> > +	.update_plane = intel_primary_plane_setplane,
> > +	.disable_plane = intel_primary_plane_disable,
> > +	.destroy = intel_primary_plane_destroy,
> > +};
> > +
> > +static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
> > +						    int pipe)
> > +{
> > +	struct intel_plane *primary;
> > +	const uint32_t *intel_primary_formats;
> > +	int num_formats;
> > +
> > +	primary = kzalloc(sizeof(*primary), GFP_KERNEL);
> > +	if (primary == NULL)
> > +		return NULL;
> > +
> > +	primary->can_scale = false;
> > +	primary->max_downscale = 1;
> > +	primary->pipe = pipe;
> > +	primary->plane = pipe;
> > +	if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4)
> > +		primary->plane = !pipe;
> > +
> > +	if (INTEL_INFO(dev)->gen <= 3) {
> > +		intel_primary_formats = intel_primary_formats_gen3;
> > +		num_formats = ARRAY_SIZE(intel_primary_formats_gen3);
> > +	} else {
> > +		intel_primary_formats = intel_primary_formats_gen4;
> > +		num_formats = ARRAY_SIZE(intel_primary_formats_gen4);
> > +	}
> > +
> > +	drm_plane_init(dev, &primary->base, 0,
> > +		       &intel_primary_plane_funcs, intel_primary_formats,
> > +		       num_formats, DRM_PLANE_TYPE_PRIMARY);
> > +	return &primary->base;
> > +}
> > +
> >  static void intel_crtc_init(struct drm_device *dev, int pipe)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	struct intel_crtc *intel_crtc;
> > -	int i;
> > +	struct drm_plane *primary;
> > +	int i, ret;
> >  
> >  	intel_crtc = kzalloc(sizeof(*intel_crtc), GFP_KERNEL);
> >  	if (intel_crtc == NULL)
> >  		return;
> >  
> > -	drm_crtc_init(dev, &intel_crtc->base, &intel_crtc_funcs);
> > +	primary = intel_primary_plane_create(dev, pipe);
> > +	ret = drm_crtc_init_with_planes(dev, &intel_crtc->base, primary,
> > +					NULL, &intel_crtc_funcs);
> > +	if (ret) {
> > +		drm_plane_cleanup(primary);
> > +		kfree(intel_crtc);
> > +		return;
> > +	}
> >  
> >  	drm_mode_crtc_set_gamma_size(&intel_crtc->base, 256);
> >  	for (i = 0; i < 256; i++) {
> > -- 
> > 1.8.5.1
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel OTC

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795