Re: [PATCHv4 06/13] drm: Add primary plane helpers (v2)

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

On Fri, Mar 28, 2014 at 09:32:06AM +0100, Daniel Vetter wrote:
> On Thu, Mar 27, 2014 at 05:44:31PM -0700, Matt Roper wrote:
...
> > +	 * N.B., we call set_config() directly here rather than using
> > +	 * drm_mode_set_config_internal.  We're reprogramming the same
> > +	 * connectors that were already in use, so we shouldn't need the extra
> > +	 * cross-CRTC fb refcounting to accomodate stealing connectors.
> > +	 * drm_mode_setplane() already handles the basic refcounting for the
> > +	 * framebuffers involved in this operation.
> 
> Wrong. The current crtc helper logic disables all disconnected connectors
> forcefully on each set_config. Nope, I didn't make those semantics ... So
> I think we need to think a bit harder here again.
> 
> See drm_helper_disable_unused_functions.

I think I'm still missing something here; can you clarify what the
problematic case would be?

I only see a call to __drm_helper_disable_unused_functions() in the CRTC
helper in cases where mode_changed = 1, which I don't believe it ever
should be through the setplane() calls.  We should just be updating the
framebuffer (and possibly panning) without touching any of the
connectors, so I don't see how unrelated CRTC's would get disabled.  Am
I overlooking another case here that the basic refcounting in setplane
doesn't already handle?

Thanks.


Matt

> 
> > +	 */
> > +	tmpfb = plane->fb;
> > +	ret = crtc->funcs->set_config(&set);
> > +	plane->fb = tmpfb;
> > +
> > +	kfree(connector_list);
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL(drm_primary_helper_update);
> > +
> > +/**
> > + * drm_primary_helper_disable() - Helper for primary plane disable
> > + * @plane: plane to disable
> > + *
> > + * Provides a default plane disable handler for primary planes.  This is handler
> > + * is called in response to a userspace SetPlane operation on the plane with a
> > + * NULL framebuffer parameter.  We call the driver's modeset handler with a NULL
> > + * framebuffer to disable the CRTC if no other planes are currently enabled.
> > + * If other planes are still enabled on the same CRTC, we return -EBUSY.
> > + *
> > + * Note that some hardware may be able to disable the primary plane without
> > + * disabling the whole CRTC.  Drivers for such hardware should provide their
> > + * own disable handler that disables just the primary plane (and they'll likely
> > + * need to provide their own update handler as well to properly re-enable a
> > + * disabled primary plane).
> > + *
> > + * RETURNS:
> > + * Zero on success, error code on failure
> > + */
> > +int drm_primary_helper_disable(struct drm_plane *plane)
> > +{
> > +	struct drm_plane *p;
> > +	struct drm_mode_set set = {
> > +		.crtc = plane->crtc,
> > +		.fb = NULL,
> > +	};
> > +
> > +	if (plane->crtc == NULL || plane->fb == NULL)
> > +		/* Already disabled */
> > +		return 0;
> > +
> > +	list_for_each_entry(p, &plane->dev->mode_config.plane_list, head)
> > +		if (p != plane && p->fb) {
> > +			DRM_DEBUG_KMS("Cannot disable primary plane while other planes are still active on CRTC.\n");
> > +			return -EBUSY;
> > +		}
> > +
> > +	/*
> > +	 * N.B.  We call set_config() directly here rather than
> > +	 * drm_mode_set_config_internal() since drm_mode_setplane() already
> > +	 * handles the basic refcounting and we don't need the special
> > +	 * cross-CRTC refcounting (no chance of stealing connectors from
> > +	 * other CRTC's with this update).
> 
> Same comment as above applies. Calling ->set_config is considered harmful
> ... Maybe we need to add another wrapper here for the primary plane
> helpers to wrap this all up safely?
> 
> > +	 */
> > +	return plane->crtc->funcs->set_config(&set);
> > +}
> > +EXPORT_SYMBOL(drm_primary_helper_disable);
> > +
> > +/**
> > + * drm_primary_helper_destroy() - Helper for primary plane destruction
> > + * @plane: plane to destroy
> > + *
> > + * Provides a default plane destroy handler for primary planes.  This handler
> > + * is called during CRTC destruction.  We disable the primary plane, remove
> > + * it from the DRM plane list, and deallocate the plane structure.
> > + */
> > +void drm_primary_helper_destroy(struct drm_plane *plane)
> > +{
> > +	plane->funcs->disable_plane(plane);
> > +	drm_plane_cleanup(plane);
> > +	kfree(plane);
> > +}
> > +EXPORT_SYMBOL(drm_primary_helper_destroy);
> > +
> > +const struct drm_plane_funcs drm_primary_helper_funcs = {
> > +	.update_plane = drm_primary_helper_update,
> > +	.disable_plane = drm_primary_helper_disable,
> > +	.destroy = drm_primary_helper_destroy,
> > +};
> > +EXPORT_SYMBOL(drm_primary_helper_funcs);
> > +
> > +/**
> > + * drm_primary_helper_create_plane() - Create a generic primary plane
> > + * @dev: drm device
> > + * @formats: pixel formats supported, or NULL for a default safe list
> > + * @num_formats: size of @formats; ignored if @formats is NULL
> > + *
> > + * Allocates and initializes a primary plane that can be used with the primary
> > + * plane helpers.  Drivers that wish to use driver-specific plane structures or
> > + * provide custom handler functions may perform their own allocation and
> > + * initialization rather than calling this function.
> > + */
> > +struct drm_plane *drm_primary_helper_create_plane(struct drm_device *dev,
> > +						  const uint32_t *formats,
> > +						  int num_formats)
> > +{
> > +	struct drm_plane *primary;
> > +	int ret;
> > +
> > +	primary = kzalloc(sizeof(*primary), GFP_KERNEL);
> > +	if (primary == NULL) {
> > +		DRM_DEBUG_KMS("Failed to allocate primary plane\n");
> > +		return NULL;
> > +	}
> > +
> > +	if (formats == NULL) {
> > +		formats = safe_modeset_formats;
> > +		num_formats = ARRAY_SIZE(safe_modeset_formats);
> > +	}
> > +
> > +	/* possible_crtc's will be filled in later by crtc_init */
> > +	ret = drm_plane_init(dev, primary, 0, &drm_primary_helper_funcs,
> > +			     formats, num_formats,
> > +			     DRM_PLANE_TYPE_PRIMARY);
> > +	if (ret) {
> > +		kfree(primary);
> > +		primary = NULL;
> > +	}
> > +
> > +	return primary;
> > +}
> > +EXPORT_SYMBOL(drm_primary_helper_create_plane);
> > +
> > diff --git a/include/drm/drm_plane_helper.h b/include/drm/drm_plane_helper.h
> > new file mode 100644
> > index 0000000..09824be
> > --- /dev/null
> > +++ b/include/drm/drm_plane_helper.h
> > @@ -0,0 +1,49 @@
> > +/*
> > + * Copyright (C) 2011-2013 Intel Corporation
> > + *
> > + * Permission is hereby granted, free of charge, to any person obtaining a
> > + * copy of this software and associated documentation files (the "Software"),
> > + * to deal in the Software without restriction, including without limitation
> > + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> > + * and/or sell copies of the Software, and to permit persons to whom the
> > + * Software is furnished to do so, subject to the following conditions:
> > + *
> > + * The above copyright notice and this permission notice (including the next
> > + * paragraph) shall be included in all copies or substantial portions of the
> > + * Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> > + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
> > + * SOFTWARE.
> > + */
> > +
> > +#ifndef DRM_PLANE_HELPER_H
> > +#define DRM_PLANE_HELPER_H
> > +
> > +/**
> > + * DOC: plane helpers
> > + *
> > + * Helper functions to assist with creation and handling of CRTC primary
> > + * planes.
> > + */
> > +
> > +extern int drm_primary_helper_update(struct drm_plane *plane,
> > +				     struct drm_crtc *crtc,
> > +				     struct drm_framebuffer *fb,
> > +				     int crtc_x, int crtc_y,
> > +				     unsigned int crtc_w, unsigned int crtc_h,
> > +				     uint32_t src_x, uint32_t src_y,
> > +				     uint32_t src_w, uint32_t src_h);
> > +extern int drm_primary_helper_disable(struct drm_plane *plane);
> > +extern void drm_primary_helper_destroy(struct drm_plane *plane);
> > +extern const struct drm_plane_funcs drm_primary_helper_funcs;
> > +extern struct drm_plane *drm_primary_helper_create_plane(struct drm_device *dev,
> > +							 uint32_t *formats,
> > +							 int num_formats);
> > +
> > +
> > +#endif
> > -- 
> > 1.8.5.1
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

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