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