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