| Message ID | 1395967478-30549-7-git-send-email-matthew.d.roper@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Thu, Mar 27, 2014 at 05:44:31PM -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 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. > > v2: > - Move plane helpers to a new file (drm_plane_helper.c) > - Tighten checks on update handler (check for scaling, CRTC coverage, > subpixel positioning) > - Pass proper panning parameters to modeset interface > - Disallow disabling primary plane (and thus CRTC) if other planes are > still active on the CRTC. > - Use a minimal format list that should work on all hardware/drivers. > Drivers may call this function with a more accurate plane list to > enable additional formats they can support. > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > --- > drivers/gpu/drm/Makefile | 3 +- > drivers/gpu/drm/drm_plane_helper.c | 312 +++++++++++++++++++++++++++++++++++++ > include/drm/drm_plane_helper.h | 49 ++++++ DocBook integration is missing for all the great kerneldoc you've written. That boils down to adding a new section next to the other helper libraries in the drm docbook to pull the kerneldoc in and running make htmldocs to make sure the kerneldoc checks out. If you want you can add a DOC: overview section and pull that into the docbook too, see e.g. how the drm prime helpers are integrated. > 3 files changed, 363 insertions(+), 1 deletion(-) > create mode 100644 drivers/gpu/drm/drm_plane_helper.c > create mode 100644 include/drm/drm_plane_helper.h > > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile > index 5e792b0..9d25dbb 100644 > --- a/drivers/gpu/drm/Makefile > +++ b/drivers/gpu/drm/Makefile > @@ -13,7 +13,8 @@ drm-y := drm_auth.o drm_buffer.o drm_bufs.o drm_cache.o \ > drm_crtc.o drm_modes.o drm_edid.o \ > drm_info.o drm_debugfs.o drm_encoder_slave.o \ > drm_trace_points.o drm_global.o drm_prime.o \ > - drm_rect.o drm_vma_manager.o drm_flip_work.o > + drm_rect.o drm_vma_manager.o drm_flip_work.o \ > + drm_plane_helper.o > > drm-$(CONFIG_COMPAT) += drm_ioc32.o > drm-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_gem_cma_helper.o > diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c > new file mode 100644 > index 0000000..374a98d > --- /dev/null > +++ b/drivers/gpu/drm/drm_plane_helper.c > @@ -0,0 +1,312 @@ > +/* > + * Copyright (C) 2014 Intel Corporation > + * > + * DRM universal plane helper functions > + * > + * 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. > + */ > + > +#include <linux/list.h> > +#include <drm/drmP.h> > +#include <drm/drm_rect.h> > + > +#define SUBPIXEL_MASK 0xffff > + > +/* > + * This is the minimal list of formats that seem to be safe for modeset use > + * with all current DRM drivers. Most hardware can actually support more > + * formats than this and drivers may specify a more accurate list when > + * creating the primary plane. However drivers that still call > + * drm_plane_init() will use this minimal format list as the default. > + */ > +const static uint32_t safe_modeset_formats[] = { > + DRM_FORMAT_XRGB8888, > + DRM_FORMAT_ARGB8888, > +}; Yeah, I think this is safe enough ;-) And it's really not a big change for drivers to enable primary planes properly with these helpers here. So even if we decide to switch to them fully (e.g. to make the fbdev helpers a test-vehicle for atomic modesets) it shouldn't be a lot of fuzz to get this going. Since for fbdev support we definitely want 15/16 and C8 support back for some especially low-powered chips. > + > +/* > + * Returns the connectors currently associated with a CRTC. This function > + * should be called twice: once with a NULL connector list to retrieve > + * the list size, and once with the properly allocated list to be filled in. > + */ > +static int get_connectors_for_crtc(struct drm_crtc *crtc, > + struct drm_connector **connector_list, > + int num_connectors) > +{ > + struct drm_device *dev = crtc->dev; > + struct drm_connector *connector; > + int count = 0; > + > + list_for_each_entry(connector, &dev->mode_config.connector_list, head) > + if (connector->encoder && connector->encoder->crtc == crtc) { > + if (connector_list != NULL && count < num_connectors) > + *(connector_list++) = connector; > + > + count++; > + } > + > + return count; > +} > + > +/** > + * drm_primary_helper_update() - Helper for primary plane update > + * @plane: plane object to update > + * @crtc: owning CRTC of owning plane > + * @fb: framebuffer to flip onto plane > + * @crtc_x: x offset of primary plane on crtc > + * @crtc_y: y offset of primary plane on crtc > + * @crtc_w: width of primary plane rectangle on crtc > + * @crtc_h: height of primary plane rectangle on crtc > + * @src_x: x offset of @fb for panning > + * @src_y: y offset of @fb for panning > + * @src_w: width of source rectangle in @fb > + * @src_h: height of source rectangle in @fb > + * > + * Provides a default plane update handler for primary planes. This is handler > + * is called in response to a userspace SetPlane operation on the plane with a > + * non-NULL framebuffer. We call the driver's modeset handler to update the > + * framebuffer. > + * > + * SetPlane() on a primary plane of a disabled CRTC is not supported, and will > + * return an error. > + * > + * Note that we make some assumptions about hardware limitations that may not be > + * true for all hardware: > + * - primary plane cannot be repositioned > + * - primary plane cannot be scaled > + * - primary plane must cover the entire CRTC > + * - subpixel positioning is not supported > + * Drivers for hardware that don't have these restrictions can provide their > + * own implementation rather than using this helper. > + * > + * RETURNS: > + * Zero on success, error code on failure > + */ > +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) > +{ > + struct drm_mode_set set = { > + .crtc = crtc, > + .fb = fb, > + .mode = &crtc->mode, > + .x = src_x >> 16, > + .y = src_y >> 16, > + }; > + struct drm_rect dest = { > + .x1 = crtc_x, > + .y1 = crtc_y, > + .x2 = crtc_x + crtc_w, > + .y2 = crtc_y + crtc_h, > + }; > + struct drm_rect clip = { > + .x2 = crtc->mode.hdisplay, > + .y2 = crtc->mode.vdisplay, > + }; > + struct drm_connector **connector_list; > + struct drm_framebuffer *tmpfb; > + int num_connectors, ret; > + > + if (!crtc->enabled) { > + DRM_DEBUG_KMS("Cannot update primary plane of a disabled CRTC.\n"); > + return -EINVAL; > + } > + > + /* Disallow subpixel positioning */ > + if ((src_x | src_y | src_w | src_h) & SUBPIXEL_MASK) { > + DRM_DEBUG_KMS("Primary plane does not support subpixel positioning\n"); > + return -EINVAL; > + } > + > + /* Primary planes are locked to their owning CRTC */ > + if (plane->possible_crtcs != drm_crtc_mask(crtc)) { > + DRM_DEBUG_KMS("Cannot change primary plane CRTC\n"); > + return -EINVAL; > + } > + > + /* Disallow scaling */ > + if (crtc_w != src_w || crtc_h != src_h) { > + DRM_DEBUG_KMS("Can't scale primary plane\n"); > + return -EINVAL; > + } > + > + /* Make sure primary plane covers entire CRTC */ > + drm_rect_intersect(&dest, &clip); > + if (dest.x1 != 0 || dest.y1 != 0 || > + dest.x2 != crtc->mode.hdisplay || dest.y2 != crtc->mode.vdisplay) { > + DRM_DEBUG_KMS("Primary plane must cover entire CRTC\n"); > + return -EINVAL; > + } > + > + /* Framebuffer must be big enough to cover entire plane */ > + ret = drm_crtc_check_viewport(crtc, crtc_x, crtc_y, &crtc->mode, fb); > + if (ret) > + return ret; > + > + /* Find current connectors for CRTC */ > + num_connectors = get_connectors_for_crtc(crtc, NULL, 0); > + BUG_ON(num_connectors == 0); > + connector_list = kzalloc(num_connectors * sizeof(*connector_list), > + GFP_KERNEL); > + if (!connector_list) > + return -ENOMEM; > + get_connectors_for_crtc(crtc, connector_list, num_connectors); > + > + set.connectors = connector_list; > + set.num_connectors = num_connectors; > + > + /* > + * set_config() adjusts crtc->primary->fb; however the DRM setplane > + * code that called us expects to handle the framebuffer update and > + * reference counting; save and restore the current fb before > + * calling it. > + * > + * 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. > + */ > + 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
On Friday 28 March 2014 09:32:06 Daniel Vetter wrote: > On Thu, Mar 27, 2014 at 05:44:31PM -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 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. > > > > v2: > > - Move plane helpers to a new file (drm_plane_helper.c) > > - Tighten checks on update handler (check for scaling, CRTC coverage, > > subpixel positioning) > > - Pass proper panning parameters to modeset interface > > - Disallow disabling primary plane (and thus CRTC) if other planes are > > still active on the CRTC. > > - Use a minimal format list that should work on all hardware/drivers. > > Drivers may call this function with a more accurate plane list to > > enable additional formats they can support. > > > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > > --- > > > > drivers/gpu/drm/Makefile | 3 +- > > drivers/gpu/drm/drm_plane_helper.c | 312 ++++++++++++++++++++++++++++++++ > > include/drm/drm_plane_helper.h | 49 ++++++ > > DocBook integration is missing for all the great kerneldoc you've written. I'd go a bit further than that, there's a chapter on planes in the DocBook documentation that should also be updated, otherwise it would get outdated. > That boils down to adding a new section next to the other helper libraries > in the drm docbook to pull the kerneldoc in and running make htmldocs to > make sure the kerneldoc checks out. > > If you want you can add a DOC: overview section and pull that into the > docbook too, see e.g. how the drm prime helpers are integrated. > > > 3 files changed, 363 insertions(+), 1 deletion(-) > > create mode 100644 drivers/gpu/drm/drm_plane_helper.c > > create mode 100644 include/drm/drm_plane_helper.h
On Fri, Mar 28, 2014 at 04:48:42PM +0100, Laurent Pinchart wrote: > On Friday 28 March 2014 09:32:06 Daniel Vetter wrote: > > On Thu, Mar 27, 2014 at 05:44:31PM -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 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. > > > > > > v2: > > > - Move plane helpers to a new file (drm_plane_helper.c) > > > - Tighten checks on update handler (check for scaling, CRTC coverage, > > > subpixel positioning) > > > - Pass proper panning parameters to modeset interface > > > - Disallow disabling primary plane (and thus CRTC) if other planes are > > > still active on the CRTC. > > > - Use a minimal format list that should work on all hardware/drivers. > > > Drivers may call this function with a more accurate plane list to > > > enable additional formats they can support. > > > > > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > > > --- > > > > > > drivers/gpu/drm/Makefile | 3 +- > > > drivers/gpu/drm/drm_plane_helper.c | 312 ++++++++++++++++++++++++++++++++ > > > include/drm/drm_plane_helper.h | 49 ++++++ > > > > DocBook integration is missing for all the great kerneldoc you've written. > > I'd go a bit further than that, there's a chapter on planes in the DocBook > documentation that should also be updated, otherwise it would get outdated. Probably better done as part of the patch which actually adds primary planes, since this here is just the convenience helpers for easier transition ... -Daniel > > > That boils down to adding a new section next to the other helper libraries > > in the drm docbook to pull the kerneldoc in and running make htmldocs to > > make sure the kerneldoc checks out. > > > > If you want you can add a DOC: overview section and pull that into the > > docbook too, see e.g. how the drm prime helpers are integrated. > > > > > 3 files changed, 363 insertions(+), 1 deletion(-) > > > create mode 100644 drivers/gpu/drm/drm_plane_helper.c > > > create mode 100644 include/drm/drm_plane_helper.h > > -- > Regards, > > Laurent Pinchart >
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
On Fri, Mar 28, 2014 at 4:32 AM, Daniel Vetter <daniel@ffwll.ch> wrote: > >> + /* >> + * set_config() adjusts crtc->primary->fb; however the DRM setplane >> + * code that called us expects to handle the framebuffer update and >> + * reference counting; save and restore the current fb before >> + * calling it. >> + * >> + * 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. Hmm, I'm not really seeing the problem. The worst setplane could do is enable a crtc which otherwise already has an encoder+connector pointing to it. Which would mean that encoder+connector could not be pointing to another crtc. So yeah, we'll end up calling dpms(OFF) on a bunch of stuff that is already off.. is that a problem? > > See drm_helper_disable_unused_functions. > >> + */ >> + 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? actually, again, I think calling .set_config() directly here is the correct thing. There should be no connector/encoder changes. and the drm_mode_set_config_internal() refcounting would be wrong for drm_mode_setplane(). In the helpers he undoes the plane->fb update done by the driver in .set_config() so that drm_mode_setplane() dtrt.. As an aside, that inconsistency between who updates the fb ptr between setcrtc and setplane has been bothering me for a while now with the atomic stuff. Maybe it's just the OCD talking, but I'd *really* like to clean that up. The rough thinking I have, with the atomic stuff we introduce a second fb pointer (and reference) in the state object. So initially we have: plane->fb --- current rules plane->state->fb --- immutable, set to new incoming fb before calling in to driver.. drm core manages the refcnt, read-only access for the driver what I'd like to do is request that all drivers add their own internal scanout references, etc. And we eventually remove plane->fb when no one still references it. The outgoing state object will hold a ref to old_fb until after we return from calling in to the driver. The only thing left is for each driver to (if it cannot cancel scanout mid-frame) hold a reference until next vblank. BR, -R
On Mon, Mar 31, 2014 at 06:03:24PM -0700, Matt Roper wrote: > 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? Looking at drm_helper_disable_unused_functions we'll spot list_for_each_entry(connector, &dev->mode_config.connector_list, head) { if (!connector->encoder) continue; if (connector->status == connector_status_disconnected) connector->encoder = NULL; } So as soon as a connector is disconnected and you do _any_ kind of ->set_config call with modesetting helpers that crtc gets killed. Even if it's completely unrelated. Dave originally changed this with an imo rather thin justification: commit a3a0544b2c84e1d7a2022b558ecf66d8c6a8dd93 Author: Dave Airlie <airlied@redhat.com> Date: Mon Aug 31 15:16:30 2009 +1000 drm/kms: add explicit encoder disable function and detach harder. For shared tv-out and VGA encoders, we really need to know if the encoder is just being switched off temporarily in blanking or if we are really disabling it hard. Also we need to try harder to disconnect encoders from unused connectors so we can share more efficently. (shared encoders stuff is coming in radeon tv-out support) Signed-off-by: Dave Airlie <airlied@redhat.com> To me this always smelled like papering over broken userspace. I've killed this in the i915 modeset rewrite and we didn't really have angry users scaling our walls. But I'm not sure what'll happen if we do this for all other drivers. -Daniel
On Tue, Apr 1, 2014 at 3:45 AM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Mon, Mar 31, 2014 at 06:03:24PM -0700, Matt Roper wrote: >> 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? > > Looking at drm_helper_disable_unused_functions we'll spot > > list_for_each_entry(connector, &dev->mode_config.connector_list, head) { > if (!connector->encoder) > continue; > if (connector->status == connector_status_disconnected) > connector->encoder = NULL; > } > > > So as soon as a connector is disconnected and you do _any_ kind of > ->set_config call with modesetting helpers that crtc gets killed. Even if > it's completely unrelated. Dave originally changed this with an imo rather > thin justification: sure, so this could change the timing of things a bit.. but it would be a bug if a HPD event or poll didn't eventually detect that and shut things off.. BR, -R > commit a3a0544b2c84e1d7a2022b558ecf66d8c6a8dd93 > Author: Dave Airlie <airlied@redhat.com> > Date: Mon Aug 31 15:16:30 2009 +1000 > > drm/kms: add explicit encoder disable function and detach harder. > > For shared tv-out and VGA encoders, we really need to know if > the encoder is just being switched off temporarily in blanking > or if we are really disabling it hard. > > Also we need to try harder to disconnect encoders from unused > connectors so we can share more efficently. > > (shared encoders stuff is coming in radeon tv-out support) > > Signed-off-by: Dave Airlie <airlied@redhat.com> > > To me this always smelled like papering over broken userspace. I've killed > this in the i915 modeset rewrite and we didn't really have angry users > scaling our walls. But I'm not sure what'll happen if we do this for all > other drivers. > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Tue, Apr 1, 2014 at 1:45 PM, Rob Clark <robdclark@gmail.com> wrote: > On Tue, Apr 1, 2014 at 3:45 AM, Daniel Vetter <daniel@ffwll.ch> wrote: >> On Mon, Mar 31, 2014 at 06:03:24PM -0700, Matt Roper wrote: >>> 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? >> >> Looking at drm_helper_disable_unused_functions we'll spot >> >> list_for_each_entry(connector, &dev->mode_config.connector_list, head) { >> if (!connector->encoder) >> continue; >> if (connector->status == connector_status_disconnected) >> connector->encoder = NULL; >> } >> >> >> So as soon as a connector is disconnected and you do _any_ kind of >> ->set_config call with modesetting helpers that crtc gets killed. Even if >> it's completely unrelated. Dave originally changed this with an imo rather >> thin justification: > > sure, so this could change the timing of things a bit.. but it would > be a bug if a HPD event or poll didn't eventually detect that and shut > things off.. Well on the semantics imo this is userspace's decision, or should be at least. And there's no driver which kills the output in the hdp handler at all. So I'm all in favour of just removing this and letting DEs handle hpd events (like they currently all do). But until we have that completely unrelated crtcs _can_ get disabled when you call ->set_config, and that means we do have to handle the refcounting correctly by doing what set_config_internal does essentially. -Daniel -- Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
Hi Daniel, On Friday 28 March 2014 18:54:49 Daniel Vetter wrote: > On Fri, Mar 28, 2014 at 04:48:42PM +0100, Laurent Pinchart wrote: > > On Friday 28 March 2014 09:32:06 Daniel Vetter wrote: > > > On Thu, Mar 27, 2014 at 05:44:31PM -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 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. > > > > > > > > v2: > > > > - Move plane helpers to a new file (drm_plane_helper.c) > > > > - Tighten checks on update handler (check for scaling, CRTC coverage, > > > > subpixel positioning) > > > > - Pass proper panning parameters to modeset interface > > > > - Disallow disabling primary plane (and thus CRTC) if other planes > > > > are still active on the CRTC. > > > > - Use a minimal format list that should work on all hardware/drivers. > > > > Drivers may call this function with a more accurate plane list to > > > > enable additional formats they can support. > > > > > > > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > > > > --- > > > > > > > > drivers/gpu/drm/Makefile | 3 +- > > > > drivers/gpu/drm/drm_plane_helper.c | 312 ++++++++++++++++++++++++++++ > > > > include/drm/drm_plane_helper.h | 49 ++++++ > > > > > > DocBook integration is missing for all the great kerneldoc you've > > > written. > > > > I'd go a bit further than that, there's a chapter on planes in the DocBook > > documentation that should also be updated, otherwise it would get > > outdated. > > Probably better done as part of the patch which actually adds primary > planes, since this here is just the convenience helpers for easier > transition ... Sure, that's perfectly fine with me. > > > That boils down to adding a new section next to the other helper > > > libraries in the drm docbook to pull the kerneldoc in and running make > > > htmldocs to make sure the kerneldoc checks out. > > > > > > If you want you can add a DOC: overview section and pull that into the > > > docbook too, see e.g. how the drm prime helpers are integrated. > > > > > > > 3 files changed, 363 insertions(+), 1 deletion(-) > > > > create mode 100644 drivers/gpu/drm/drm_plane_helper.c > > > > create mode 100644 include/drm/drm_plane_helper.h
On Tue, Apr 1, 2014 at 5:45 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Mon, Mar 31, 2014 at 06:03:24PM -0700, Matt Roper wrote: >> 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? > > Looking at drm_helper_disable_unused_functions we'll spot > > list_for_each_entry(connector, &dev->mode_config.connector_list, head) { > if (!connector->encoder) > continue; > if (connector->status == connector_status_disconnected) > connector->encoder = NULL; > } > > > So as soon as a connector is disconnected and you do _any_ kind of > ->set_config call with modesetting helpers that crtc gets killed. Even if > it's completely unrelated. Dave originally changed this with an imo rather > thin justification: > > commit a3a0544b2c84e1d7a2022b558ecf66d8c6a8dd93 > Author: Dave Airlie <airlied@redhat.com> > Date: Mon Aug 31 15:16:30 2009 +1000 > > drm/kms: add explicit encoder disable function and detach harder. > > For shared tv-out and VGA encoders, we really need to know if > the encoder is just being switched off temporarily in blanking > or if we are really disabling it hard. > > Also we need to try harder to disconnect encoders from unused > connectors so we can share more efficently. > > (shared encoders stuff is coming in radeon tv-out support) > > Signed-off-by: Dave Airlie <airlied@redhat.com> > > To me this always smelled like papering over broken userspace. I've killed > this in the i915 modeset rewrite and we didn't really have angry users > scaling our walls. But I'm not sure what'll happen if we do this for all > other drivers. I've had to look at this again recently, and while I still don't like my commit, its not papering over userspace, it might be papering over fbcon :-) You don't have any hw that operates like this, so I'd be surprised if you had users falling over, The problem is if I have a single DAC encoder, with tv-out and VGA connectors, and I unplug the VGA connector, and plug in the tv connector, how do I get fbcon to pop-up, Though that said this commit caused a regression that I'm not sure I liked either, since I think we used to allow you to force a mode on disconnected outputs, and this stops that from working, I noticed in a virtual env. Dave.
On Tue, Apr 1, 2014 at 9:46 PM, Dave Airlie <airlied@gmail.com> wrote: > On Tue, Apr 1, 2014 at 5:45 PM, Daniel Vetter <daniel@ffwll.ch> wrote: >> commit a3a0544b2c84e1d7a2022b558ecf66d8c6a8dd93 >> Author: Dave Airlie <airlied@redhat.com> >> Date: Mon Aug 31 15:16:30 2009 +1000 >> >> drm/kms: add explicit encoder disable function and detach harder. >> >> For shared tv-out and VGA encoders, we really need to know if >> the encoder is just being switched off temporarily in blanking >> or if we are really disabling it hard. >> >> Also we need to try harder to disconnect encoders from unused >> connectors so we can share more efficently. >> >> (shared encoders stuff is coming in radeon tv-out support) >> >> Signed-off-by: Dave Airlie <airlied@redhat.com> >> >> To me this always smelled like papering over broken userspace. I've killed >> this in the i915 modeset rewrite and we didn't really have angry users >> scaling our walls. But I'm not sure what'll happen if we do this for all >> other drivers. > > I've had to look at this again recently, and while I still don't like > my commit, its > not papering over userspace, it might be papering over fbcon :-) > > You don't have any hw that operates like this, so I'd be surprised if > you had users falling over, > > The problem is if I have a single DAC encoder, with tv-out and VGA > connectors, and I unplug the VGA connector, and plug in the tv > connector, how do I get fbcon to pop-up, > > Though that said this commit caused a regression that I'm not sure I > liked either, since I think we used to allow you to force a mode on > disconnected outputs, and this stops that from working, I noticed in a > virtual env. As discussed on irc I think we should be able to fix the issue with fbcon by using an atomic modeset. The bug only happens when fbcon tries to enable the newly plugged-in connector before it disabled the old one and then can't get at the shared encoder needed to enable either. If we have atomic modesets the crtc helpers can always first disable all the unused stuff and then light up the new things, which should make this work. So imo disabling this hunk as in the patch I've just sent out looks like the way forward for primary planes and atomic modeset. -Daniel
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile index 5e792b0..9d25dbb 100644 --- a/drivers/gpu/drm/Makefile +++ b/drivers/gpu/drm/Makefile @@ -13,7 +13,8 @@ drm-y := drm_auth.o drm_buffer.o drm_bufs.o drm_cache.o \ drm_crtc.o drm_modes.o drm_edid.o \ drm_info.o drm_debugfs.o drm_encoder_slave.o \ drm_trace_points.o drm_global.o drm_prime.o \ - drm_rect.o drm_vma_manager.o drm_flip_work.o + drm_rect.o drm_vma_manager.o drm_flip_work.o \ + drm_plane_helper.o drm-$(CONFIG_COMPAT) += drm_ioc32.o drm-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_gem_cma_helper.o diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c new file mode 100644 index 0000000..374a98d --- /dev/null +++ b/drivers/gpu/drm/drm_plane_helper.c @@ -0,0 +1,312 @@ +/* + * Copyright (C) 2014 Intel Corporation + * + * DRM universal plane helper functions + * + * 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. + */ + +#include <linux/list.h> +#include <drm/drmP.h> +#include <drm/drm_rect.h> + +#define SUBPIXEL_MASK 0xffff + +/* + * This is the minimal list of formats that seem to be safe for modeset use + * with all current DRM drivers. Most hardware can actually support more + * formats than this and drivers may specify a more accurate list when + * creating the primary plane. However drivers that still call + * drm_plane_init() will use this minimal format list as the default. + */ +const static uint32_t safe_modeset_formats[] = { + DRM_FORMAT_XRGB8888, + DRM_FORMAT_ARGB8888, +}; + +/* + * Returns the connectors currently associated with a CRTC. This function + * should be called twice: once with a NULL connector list to retrieve + * the list size, and once with the properly allocated list to be filled in. + */ +static int get_connectors_for_crtc(struct drm_crtc *crtc, + struct drm_connector **connector_list, + int num_connectors) +{ + struct drm_device *dev = crtc->dev; + struct drm_connector *connector; + int count = 0; + + list_for_each_entry(connector, &dev->mode_config.connector_list, head) + if (connector->encoder && connector->encoder->crtc == crtc) { + if (connector_list != NULL && count < num_connectors) + *(connector_list++) = connector; + + count++; + } + + return count; +} + +/** + * drm_primary_helper_update() - Helper for primary plane update + * @plane: plane object to update + * @crtc: owning CRTC of owning plane + * @fb: framebuffer to flip onto plane + * @crtc_x: x offset of primary plane on crtc + * @crtc_y: y offset of primary plane on crtc + * @crtc_w: width of primary plane rectangle on crtc + * @crtc_h: height of primary plane rectangle on crtc + * @src_x: x offset of @fb for panning + * @src_y: y offset of @fb for panning + * @src_w: width of source rectangle in @fb + * @src_h: height of source rectangle in @fb + * + * Provides a default plane update handler for primary planes. This is handler + * is called in response to a userspace SetPlane operation on the plane with a + * non-NULL framebuffer. We call the driver's modeset handler to update the + * framebuffer. + * + * SetPlane() on a primary plane of a disabled CRTC is not supported, and will + * return an error. + * + * Note that we make some assumptions about hardware limitations that may not be + * true for all hardware: + * - primary plane cannot be repositioned + * - primary plane cannot be scaled + * - primary plane must cover the entire CRTC + * - subpixel positioning is not supported + * Drivers for hardware that don't have these restrictions can provide their + * own implementation rather than using this helper. + * + * RETURNS: + * Zero on success, error code on failure + */ +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) +{ + struct drm_mode_set set = { + .crtc = crtc, + .fb = fb, + .mode = &crtc->mode, + .x = src_x >> 16, + .y = src_y >> 16, + }; + struct drm_rect dest = { + .x1 = crtc_x, + .y1 = crtc_y, + .x2 = crtc_x + crtc_w, + .y2 = crtc_y + crtc_h, + }; + struct drm_rect clip = { + .x2 = crtc->mode.hdisplay, + .y2 = crtc->mode.vdisplay, + }; + struct drm_connector **connector_list; + struct drm_framebuffer *tmpfb; + int num_connectors, ret; + + if (!crtc->enabled) { + DRM_DEBUG_KMS("Cannot update primary plane of a disabled CRTC.\n"); + return -EINVAL; + } + + /* Disallow subpixel positioning */ + if ((src_x | src_y | src_w | src_h) & SUBPIXEL_MASK) { + DRM_DEBUG_KMS("Primary plane does not support subpixel positioning\n"); + return -EINVAL; + } + + /* Primary planes are locked to their owning CRTC */ + if (plane->possible_crtcs != drm_crtc_mask(crtc)) { + DRM_DEBUG_KMS("Cannot change primary plane CRTC\n"); + return -EINVAL; + } + + /* Disallow scaling */ + if (crtc_w != src_w || crtc_h != src_h) { + DRM_DEBUG_KMS("Can't scale primary plane\n"); + return -EINVAL; + } + + /* Make sure primary plane covers entire CRTC */ + drm_rect_intersect(&dest, &clip); + if (dest.x1 != 0 || dest.y1 != 0 || + dest.x2 != crtc->mode.hdisplay || dest.y2 != crtc->mode.vdisplay) { + DRM_DEBUG_KMS("Primary plane must cover entire CRTC\n"); + return -EINVAL; + } + + /* Framebuffer must be big enough to cover entire plane */ + ret = drm_crtc_check_viewport(crtc, crtc_x, crtc_y, &crtc->mode, fb); + if (ret) + return ret; + + /* Find current connectors for CRTC */ + num_connectors = get_connectors_for_crtc(crtc, NULL, 0); + BUG_ON(num_connectors == 0); + connector_list = kzalloc(num_connectors * sizeof(*connector_list), + GFP_KERNEL); + if (!connector_list) + return -ENOMEM; + get_connectors_for_crtc(crtc, connector_list, num_connectors); + + set.connectors = connector_list; + set.num_connectors = num_connectors; + + /* + * set_config() adjusts crtc->primary->fb; however the DRM setplane + * code that called us expects to handle the framebuffer update and + * reference counting; save and restore the current fb before + * calling it. + * + * 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. + */ + 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). + */ + 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
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 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. v2: - Move plane helpers to a new file (drm_plane_helper.c) - Tighten checks on update handler (check for scaling, CRTC coverage, subpixel positioning) - Pass proper panning parameters to modeset interface - Disallow disabling primary plane (and thus CRTC) if other planes are still active on the CRTC. - Use a minimal format list that should work on all hardware/drivers. Drivers may call this function with a more accurate plane list to enable additional formats they can support. Signed-off-by: Matt Roper <matthew.d.roper@intel.com> --- drivers/gpu/drm/Makefile | 3 +- drivers/gpu/drm/drm_plane_helper.c | 312 +++++++++++++++++++++++++++++++++++++ include/drm/drm_plane_helper.h | 49 ++++++ 3 files changed, 363 insertions(+), 1 deletion(-) create mode 100644 drivers/gpu/drm/drm_plane_helper.c create mode 100644 include/drm/drm_plane_helper.h