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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, May 15, 2014 at 09:37:55AM -0700, Matt Roper wrote:
> 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).

But it doesn't.


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

-- 
Ville Syrjälä
Intel OTC