[RFCv3 03/14] drm: Add primary plane helpers

[Daniel Vetter]

On Tue, Mar 18, 2014 at 05:22:48PM -0700, Matt Roper wrote:
> When we expose non-overlay planes to userspace, they will become
> accessible via standard userspace plane API's.  We should be able to
> handle the standard plane operations against primary planes in a generic
> way via the page flip handler and modeset handler.
> 
> Drivers that can program primary planes more efficiently, that want to
> use their own primary plane structure to track additional information,
> or that don't have the limitations assumed by the helpers are free to
> provide their own implementation of some or all of these handlers.
> 
> Signed-off-by: Matt Roper <matthew.d.roper at intel.com>
> ---

One more below ...

> +/**
> + * 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.
> + *
> + * 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_mode_set set = {
> +		.crtc = plane->crtc,
> +		.fb = NULL,
> +	};
> +
> +	if (plane->crtc == NULL || plane->fb == NULL)
> +		/* Already disabled */
> +		return 0;

I think we should have a check here if any other plane is enabled
(including the cursor plane), and fail the plane disabling with -EBUSY.
Otherwise new userspace has no way to figure out whether the driver is
updated already or not.

> +
> +	return plane->crtc->funcs->set_config(&set);

Again I think you need to use set_config_internal to have correct fb
refcounting.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch