| Message ID | 1395188579-17191-4-git-send-email-matthew.d.roper@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
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@intel.com> > --- > drivers/gpu/drm/drm_crtc.c | 288 +++++++++++++++++++++++++++++++++++++++------ Given that this is helper code to transition from the current interfaces to universal planes I think we should shovel this into a new drm_planes_helper.c library or something. drm_crtc.c is reserved for core interface code between drm ioctl and the main driver entry points. Note that the legacy ioctl -> new universal plane code should probably live here since long-term we want to phase out the legacy drm core -> driver interfaces for cursors and primary planes. > include/drm/drm_crtc.h | 81 +++++++++++++ > 2 files changed, 330 insertions(+), 39 deletions(-) > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > index 0983996..db54ae9 100644 > --- a/drivers/gpu/drm/drm_crtc.c > +++ b/drivers/gpu/drm/drm_crtc.c > @@ -1118,6 +1118,255 @@ void drm_plane_force_disable(struct drm_plane *plane) > } > EXPORT_SYMBOL(drm_plane_force_disable); > > +/* > + * Checks that the framebuffer is big enough for the CRTC viewport > + * (x, y, hdisplay, vdisplay) > + */ > +static int drm_crtc_check_viewport(const struct drm_crtc *crtc, > + int x, int y, > + const struct drm_display_mode *mode, > + const struct drm_framebuffer *fb) > + > +{ > + int hdisplay, vdisplay; > + > + hdisplay = mode->hdisplay; > + vdisplay = mode->vdisplay; > + > + if (drm_mode_is_stereo(mode)) { > + struct drm_display_mode adjusted = *mode; > + > + drm_mode_set_crtcinfo(&adjusted, CRTC_STEREO_DOUBLE); > + hdisplay = adjusted.crtc_hdisplay; > + vdisplay = adjusted.crtc_vdisplay; > + } > + > + if (crtc->invert_dimensions) > + swap(hdisplay, vdisplay); > + > + if (hdisplay > fb->width || > + vdisplay > fb->height || > + x > fb->width - hdisplay || > + y > fb->height - vdisplay) { > + DRM_DEBUG_KMS("Invalid fb size %ux%u for CRTC viewport %ux%u+%d+%d%s.\n", > + fb->width, fb->height, hdisplay, vdisplay, x, y, > + crtc->invert_dimensions ? " (inverted)" : ""); > + return -ENOSPC; > + } > + > + return 0; > +} > + > +/* > + * 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 pageflip 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 assume most hardware can't reposition or scale the primary > + * plane, so we require that crtc_x = crtc_y = 0 and that src_w/src_h match the > + * current mode. 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 = crtc_x, > + .y = crtc_y, > + }; > + struct drm_connector **connector_list; > + struct drm_framebuffer *tmpfb; > + int num_connectors, ret; > + > + /* setplane API takes shifted source rectangle values; unshift them */ > + src_x >>= 16; > + src_y >>= 16; > + src_w >>= 16; > + src_h >>= 16; I think you need to check for 0 in the lower bits since set_config doesn't support sub-pixel positioning. Also we need to check for scaling and reject it and we need to check that crtc_w/crtc_h will fill the entire crtc (otherwise set_config won't do the right thing). > + > + /* 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; > + } > + > + if (!crtc->enabled) { > + DRM_DEBUG_KMS("Cannot update primary plane of a disabled CRTC.\n"); > + return -EINVAL; > + } > + > + 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. > + */ > + tmpfb = plane->fb; > + ret = crtc->funcs->set_config(&set); I wonder whether we should have an oppportunistic path using the page_flip interface here. Otoh we must have a fallback to ->set_config anyway since the drivers currently only allow pageflips on identical buffers. E.g. i915 rejects tiling changes and stride changes and position changes. So I think having a pageflip fastpath isn't worth the trouble. Also I think you need to use set_config_internal here to get the refcounting right. > + 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. > + * > + * 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; > + > + 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 > + * > + * 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) > +{ > + 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; > + } > + > + /* possible_crtc's will be filled in later by crtc_init */ > + ret = drm_plane_init(dev, primary, 0, &drm_primary_helper_funcs, > + legacy_modeset_formats, > + ARRAY_SIZE(legacy_modeset_formats), > + DRM_PLANE_TYPE_PRIMARY); > + if (ret) { > + kfree(primary); > + primary = NULL; > + } > + > + return primary; > +} > +EXPORT_SYMBOL(drm_primary_helper_create_plane); > + > static int drm_mode_create_standard_connector_properties(struct drm_device *dev) > { > struct drm_property *edid; > @@ -2185,45 +2434,6 @@ int drm_mode_set_config_internal(struct drm_mode_set *set) > } > EXPORT_SYMBOL(drm_mode_set_config_internal); > > -/* > - * Checks that the framebuffer is big enough for the CRTC viewport > - * (x, y, hdisplay, vdisplay) > - */ > -static int drm_crtc_check_viewport(const struct drm_crtc *crtc, > - int x, int y, > - const struct drm_display_mode *mode, > - const struct drm_framebuffer *fb) > - > -{ > - int hdisplay, vdisplay; > - > - hdisplay = mode->hdisplay; > - vdisplay = mode->vdisplay; > - > - if (drm_mode_is_stereo(mode)) { > - struct drm_display_mode adjusted = *mode; > - > - drm_mode_set_crtcinfo(&adjusted, CRTC_STEREO_DOUBLE); > - hdisplay = adjusted.crtc_hdisplay; > - vdisplay = adjusted.crtc_vdisplay; > - } > - > - if (crtc->invert_dimensions) > - swap(hdisplay, vdisplay); > - > - if (hdisplay > fb->width || > - vdisplay > fb->height || > - x > fb->width - hdisplay || > - y > fb->height - vdisplay) { > - DRM_DEBUG_KMS("Invalid fb size %ux%u for CRTC viewport %ux%u+%d+%d%s.\n", > - fb->width, fb->height, hdisplay, vdisplay, x, y, > - crtc->invert_dimensions ? " (inverted)" : ""); > - return -ENOSPC; > - } > - > - return 0; > -} > - > /** > * drm_mode_setcrtc - set CRTC configuration > * @dev: drm device for the ioctl > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h > index e69ada8..f43fa92 100644 > --- a/include/drm/drm_crtc.h > +++ b/include/drm/drm_crtc.h > @@ -581,6 +581,87 @@ struct drm_plane { > enum drm_plane_type type; > }; > > +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); > + > +/* > + * This is the list of formats that have historically been accepted by the > + * modeset API. The primary plane helpers use this list by default, but > + * individual drivers may provide their own primary plane initialization > + * that provides a more hw-specific format list. > + */ > +const static uint32_t legacy_modeset_formats[] = { What is this giant array doing in a header file? > + DRM_FORMAT_C8, > + DRM_FORMAT_RGB332, > + DRM_FORMAT_BGR233, > + DRM_FORMAT_XRGB4444, > + DRM_FORMAT_XBGR4444, > + DRM_FORMAT_RGBX4444, > + DRM_FORMAT_BGRX4444, > + DRM_FORMAT_ARGB4444, > + DRM_FORMAT_ABGR4444, > + DRM_FORMAT_RGBA4444, > + DRM_FORMAT_BGRA4444, > + DRM_FORMAT_XRGB1555, > + DRM_FORMAT_XBGR1555, > + DRM_FORMAT_RGBX5551, > + DRM_FORMAT_BGRX5551, > + DRM_FORMAT_ARGB1555, > + DRM_FORMAT_ABGR1555, > + DRM_FORMAT_RGBA5551, > + DRM_FORMAT_BGRA5551, > + DRM_FORMAT_RGB565, > + DRM_FORMAT_BGR565, > + DRM_FORMAT_RGB888, > + DRM_FORMAT_BGR888, > + DRM_FORMAT_XRGB8888, > + DRM_FORMAT_XBGR8888, > + DRM_FORMAT_RGBX8888, > + DRM_FORMAT_BGRX8888, > + DRM_FORMAT_ARGB8888, > + DRM_FORMAT_ABGR8888, > + DRM_FORMAT_RGBA8888, > + DRM_FORMAT_BGRA8888, > + DRM_FORMAT_XRGB2101010, > + DRM_FORMAT_XBGR2101010, > + DRM_FORMAT_RGBX1010102, > + DRM_FORMAT_BGRX1010102, > + DRM_FORMAT_ARGB2101010, > + DRM_FORMAT_ABGR2101010, > + DRM_FORMAT_RGBA1010102, > + DRM_FORMAT_BGRA1010102, > + DRM_FORMAT_YUYV, > + DRM_FORMAT_YVYU, > + DRM_FORMAT_UYVY, > + DRM_FORMAT_VYUY, > + DRM_FORMAT_AYUV, > + DRM_FORMAT_NV12, > + DRM_FORMAT_NV21, > + DRM_FORMAT_NV16, > + DRM_FORMAT_NV61, > + DRM_FORMAT_NV24, > + DRM_FORMAT_NV42, > + DRM_FORMAT_YUV410, > + DRM_FORMAT_YVU410, > + DRM_FORMAT_YUV411, > + DRM_FORMAT_YVU411, > + DRM_FORMAT_YUV420, > + DRM_FORMAT_YVU420, > + DRM_FORMAT_YUV422, > + DRM_FORMAT_YVU422, > + DRM_FORMAT_YUV444, > + DRM_FORMAT_YVU444, Hm, I'm not sure whether we have any driver or user actually supporing/using yuv buffers on primary planes. It might be potentially confusing for userspace if the format list for primary planes is nowhere near the reality. Maybe we should give drivers the option to specify a suitable one here? > +}; > + > /** > * drm_bridge_funcs - drm_bridge control functions > * @mode_fixup: Try to fixup (or reject entirely) proposed mode for this bridge > -- > 1.8.5.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
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@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
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@intel.com> One thing I've noticed with planes is that we don't have any per-plane size limits. Which will be annoying for cursors at least, but looking at intel's hw history there are other planes with limits smaller than dev->mode_config.max_widht/height. I think as part of the universal plane work here we should add this to the getplane ioctl (we can extend at the end with full backwards compat) and use the max_width/height as default for primary/overlay planes and cursor_width/height for cursor planes. Probably there's more stuff userspace might be interested in (like stride limits and alignment maybe), but that'd be a start at least. -Daniel > --- > drivers/gpu/drm/drm_crtc.c | 288 +++++++++++++++++++++++++++++++++++++++------ > include/drm/drm_crtc.h | 81 +++++++++++++ > 2 files changed, 330 insertions(+), 39 deletions(-) > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > index 0983996..db54ae9 100644 > --- a/drivers/gpu/drm/drm_crtc.c > +++ b/drivers/gpu/drm/drm_crtc.c > @@ -1118,6 +1118,255 @@ void drm_plane_force_disable(struct drm_plane *plane) > } > EXPORT_SYMBOL(drm_plane_force_disable); > > +/* > + * Checks that the framebuffer is big enough for the CRTC viewport > + * (x, y, hdisplay, vdisplay) > + */ > +static int drm_crtc_check_viewport(const struct drm_crtc *crtc, > + int x, int y, > + const struct drm_display_mode *mode, > + const struct drm_framebuffer *fb) > + > +{ > + int hdisplay, vdisplay; > + > + hdisplay = mode->hdisplay; > + vdisplay = mode->vdisplay; > + > + if (drm_mode_is_stereo(mode)) { > + struct drm_display_mode adjusted = *mode; > + > + drm_mode_set_crtcinfo(&adjusted, CRTC_STEREO_DOUBLE); > + hdisplay = adjusted.crtc_hdisplay; > + vdisplay = adjusted.crtc_vdisplay; > + } > + > + if (crtc->invert_dimensions) > + swap(hdisplay, vdisplay); > + > + if (hdisplay > fb->width || > + vdisplay > fb->height || > + x > fb->width - hdisplay || > + y > fb->height - vdisplay) { > + DRM_DEBUG_KMS("Invalid fb size %ux%u for CRTC viewport %ux%u+%d+%d%s.\n", > + fb->width, fb->height, hdisplay, vdisplay, x, y, > + crtc->invert_dimensions ? " (inverted)" : ""); > + return -ENOSPC; > + } > + > + return 0; > +} > + > +/* > + * 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 pageflip 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 assume most hardware can't reposition or scale the primary > + * plane, so we require that crtc_x = crtc_y = 0 and that src_w/src_h match the > + * current mode. 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 = crtc_x, > + .y = crtc_y, > + }; > + struct drm_connector **connector_list; > + struct drm_framebuffer *tmpfb; > + int num_connectors, ret; > + > + /* setplane API takes shifted source rectangle values; unshift them */ > + src_x >>= 16; > + src_y >>= 16; > + src_w >>= 16; > + src_h >>= 16; > + > + /* 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; > + } > + > + if (!crtc->enabled) { > + DRM_DEBUG_KMS("Cannot update primary plane of a disabled CRTC.\n"); > + return -EINVAL; > + } > + > + 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. > + */ > + 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. > + * > + * 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; > + > + 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 > + * > + * 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) > +{ > + 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; > + } > + > + /* possible_crtc's will be filled in later by crtc_init */ > + ret = drm_plane_init(dev, primary, 0, &drm_primary_helper_funcs, > + legacy_modeset_formats, > + ARRAY_SIZE(legacy_modeset_formats), > + DRM_PLANE_TYPE_PRIMARY); > + if (ret) { > + kfree(primary); > + primary = NULL; > + } > + > + return primary; > +} > +EXPORT_SYMBOL(drm_primary_helper_create_plane); > + > static int drm_mode_create_standard_connector_properties(struct drm_device *dev) > { > struct drm_property *edid; > @@ -2185,45 +2434,6 @@ int drm_mode_set_config_internal(struct drm_mode_set *set) > } > EXPORT_SYMBOL(drm_mode_set_config_internal); > > -/* > - * Checks that the framebuffer is big enough for the CRTC viewport > - * (x, y, hdisplay, vdisplay) > - */ > -static int drm_crtc_check_viewport(const struct drm_crtc *crtc, > - int x, int y, > - const struct drm_display_mode *mode, > - const struct drm_framebuffer *fb) > - > -{ > - int hdisplay, vdisplay; > - > - hdisplay = mode->hdisplay; > - vdisplay = mode->vdisplay; > - > - if (drm_mode_is_stereo(mode)) { > - struct drm_display_mode adjusted = *mode; > - > - drm_mode_set_crtcinfo(&adjusted, CRTC_STEREO_DOUBLE); > - hdisplay = adjusted.crtc_hdisplay; > - vdisplay = adjusted.crtc_vdisplay; > - } > - > - if (crtc->invert_dimensions) > - swap(hdisplay, vdisplay); > - > - if (hdisplay > fb->width || > - vdisplay > fb->height || > - x > fb->width - hdisplay || > - y > fb->height - vdisplay) { > - DRM_DEBUG_KMS("Invalid fb size %ux%u for CRTC viewport %ux%u+%d+%d%s.\n", > - fb->width, fb->height, hdisplay, vdisplay, x, y, > - crtc->invert_dimensions ? " (inverted)" : ""); > - return -ENOSPC; > - } > - > - return 0; > -} > - > /** > * drm_mode_setcrtc - set CRTC configuration > * @dev: drm device for the ioctl > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h > index e69ada8..f43fa92 100644 > --- a/include/drm/drm_crtc.h > +++ b/include/drm/drm_crtc.h > @@ -581,6 +581,87 @@ struct drm_plane { > enum drm_plane_type type; > }; > > +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); > + > +/* > + * This is the list of formats that have historically been accepted by the > + * modeset API. The primary plane helpers use this list by default, but > + * individual drivers may provide their own primary plane initialization > + * that provides a more hw-specific format list. > + */ > +const static uint32_t legacy_modeset_formats[] = { > + DRM_FORMAT_C8, > + DRM_FORMAT_RGB332, > + DRM_FORMAT_BGR233, > + DRM_FORMAT_XRGB4444, > + DRM_FORMAT_XBGR4444, > + DRM_FORMAT_RGBX4444, > + DRM_FORMAT_BGRX4444, > + DRM_FORMAT_ARGB4444, > + DRM_FORMAT_ABGR4444, > + DRM_FORMAT_RGBA4444, > + DRM_FORMAT_BGRA4444, > + DRM_FORMAT_XRGB1555, > + DRM_FORMAT_XBGR1555, > + DRM_FORMAT_RGBX5551, > + DRM_FORMAT_BGRX5551, > + DRM_FORMAT_ARGB1555, > + DRM_FORMAT_ABGR1555, > + DRM_FORMAT_RGBA5551, > + DRM_FORMAT_BGRA5551, > + DRM_FORMAT_RGB565, > + DRM_FORMAT_BGR565, > + DRM_FORMAT_RGB888, > + DRM_FORMAT_BGR888, > + DRM_FORMAT_XRGB8888, > + DRM_FORMAT_XBGR8888, > + DRM_FORMAT_RGBX8888, > + DRM_FORMAT_BGRX8888, > + DRM_FORMAT_ARGB8888, > + DRM_FORMAT_ABGR8888, > + DRM_FORMAT_RGBA8888, > + DRM_FORMAT_BGRA8888, > + DRM_FORMAT_XRGB2101010, > + DRM_FORMAT_XBGR2101010, > + DRM_FORMAT_RGBX1010102, > + DRM_FORMAT_BGRX1010102, > + DRM_FORMAT_ARGB2101010, > + DRM_FORMAT_ABGR2101010, > + DRM_FORMAT_RGBA1010102, > + DRM_FORMAT_BGRA1010102, > + DRM_FORMAT_YUYV, > + DRM_FORMAT_YVYU, > + DRM_FORMAT_UYVY, > + DRM_FORMAT_VYUY, > + DRM_FORMAT_AYUV, > + DRM_FORMAT_NV12, > + DRM_FORMAT_NV21, > + DRM_FORMAT_NV16, > + DRM_FORMAT_NV61, > + DRM_FORMAT_NV24, > + DRM_FORMAT_NV42, > + DRM_FORMAT_YUV410, > + DRM_FORMAT_YVU410, > + DRM_FORMAT_YUV411, > + DRM_FORMAT_YVU411, > + DRM_FORMAT_YUV420, > + DRM_FORMAT_YVU420, > + DRM_FORMAT_YUV422, > + DRM_FORMAT_YVU422, > + DRM_FORMAT_YUV444, > + DRM_FORMAT_YVU444, > +}; > + > /** > * drm_bridge_funcs - drm_bridge control functions > * @mode_fixup: Try to fixup (or reject entirely) proposed mode for this bridge > -- > 1.8.5.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Wed, Mar 19, 2014 at 7:28 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. >> + */ >> + tmpfb = plane->fb; >> + ret = crtc->funcs->set_config(&set); > > I wonder whether we should have an oppportunistic path using the page_flip > interface here. Otoh we must have a fallback to ->set_config anyway since > the drivers currently only allow pageflips on identical buffers. E.g. i915 > rejects tiling changes and stride changes and position changes. So I think > having a pageflip fastpath isn't worth the trouble. note that you'd get that opportunistic page_flip() path w/ the addition of atomic series. Although depending on when the two different series are merged, the .page_flip() fast path might be a good idea
On Wed, Mar 19, 2014 at 12:28:43PM +0100, Daniel Vetter wrote: > On Tue, Mar 18, 2014 at 05:22:48PM -0700, Matt Roper 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. > > + */ > > + tmpfb = plane->fb; > > + ret = crtc->funcs->set_config(&set); > > I wonder whether we should have an oppportunistic path using the page_flip > interface here. Otoh we must have a fallback to ->set_config anyway since > the drivers currently only allow pageflips on identical buffers. E.g. i915 > rejects tiling changes and stride changes and position changes. So I think > having a pageflip fastpath isn't worth the trouble. > > Also I think you need to use set_config_internal here to get the > refcounting right. Hmm. I specifically didn't use set_config_internal here because I thought drm_mode_setplane() (and presumably the future atomic ioctl which may also call into this) will already handle the refcounting for us. Am I overlooking something? Matt
On Wed, Mar 19, 2014 at 11:15:36AM -0700, Matt Roper wrote: > On Wed, Mar 19, 2014 at 12:28:43PM +0100, Daniel Vetter wrote: > > On Tue, Mar 18, 2014 at 05:22:48PM -0700, Matt Roper 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. > > > + */ > > > + tmpfb = plane->fb; > > > + ret = crtc->funcs->set_config(&set); > > > > I wonder whether we should have an oppportunistic path using the page_flip > > interface here. Otoh we must have a fallback to ->set_config anyway since > > the drivers currently only allow pageflips on identical buffers. E.g. i915 > > rejects tiling changes and stride changes and position changes. So I think > > having a pageflip fastpath isn't worth the trouble. > > > > Also I think you need to use set_config_internal here to get the > > refcounting right. > > Hmm. I specifically didn't use set_config_internal here because I > thought drm_mode_setplane() (and presumably the future atomic ioctl > which may also call into this) will already handle the refcounting for > us. Am I overlooking something? Hm, I need to take a closer look again, but set_config has some really funny rules about who updates which pointer and who grabs references for which fb. Mostly since this has all grown rather add-hoc. But you're right and the update_plane code already has some refcounting and frobbing of its own, so I'm not sure what we really need to do here. I think it at least needs a really big comment explaining what's going on. Note that iirc the crtc helper code can still disable connectors in _any_ setcrtc call, so you might accidentally end up in a full modeset, with all the fun this entails. Definitely need to think more about this here. I hope we don't need to rework the semantics of all drivers, since that would be major pain. -Daniel
On Wed, Mar 19, 2014 at 01:24:23PM +0100, Daniel Vetter wrote: > 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@intel.com> > > One thing I've noticed with planes is that we don't have any per-plane > size limits. Which will be annoying for cursors at least, but looking at > intel's hw history there are other planes with limits smaller than > dev->mode_config.max_widht/height. > > I think as part of the universal plane work here we should add this to the > getplane ioctl (we can extend at the end with full backwards compat) and > use the max_width/height as default for primary/overlay planes and > cursor_width/height for cursor planes. I'm not sure I understand how to (cleanly) extend the existing ioctl safely. Userspace (libdrm) allocates a drm_mode_get_plane structure on the stack with a specific size. If we try to extend this structure to return more information, and have the kernel write into the new fields, aren't we just going to be spilling over into other userspace stack variables if we run an old libdrm on the new kernel? The only approaches I see that could make this work would be huge ugly hacks: * Reclaiming the top few bits of plane_id to use as "I sent you the new, larger structure" capability flags (which assumes plane ID's are always small enough to leave those bits 0 on current kernels; this would effectively reduce our plane ID address space). * Figure out a way to encode extra information as bogus pixel formats and shove it into the format list returned to userspace. Presumably userspace would just ignore/skip the bogus formats. I suppose we could add a new GetPlane2 ioctl or something that returned more info, but I figured it was probably easier to just shove max plane size (and a bunch of other plane capabilities / limitations) into some new read-only plane properties. Read-only properties are easy to extend if we find other pieces of information we want to return in the future, so that seems like the most natural interface to me. Matt > > Probably there's more stuff userspace might be interested in (like stride > limits and alignment maybe), but that'd be a start at least. > -Daniel > > > --- > > drivers/gpu/drm/drm_crtc.c | 288 +++++++++++++++++++++++++++++++++++++++------ > > include/drm/drm_crtc.h | 81 +++++++++++++ > > 2 files changed, 330 insertions(+), 39 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > > index 0983996..db54ae9 100644 > > --- a/drivers/gpu/drm/drm_crtc.c > > +++ b/drivers/gpu/drm/drm_crtc.c > > @@ -1118,6 +1118,255 @@ void drm_plane_force_disable(struct drm_plane *plane) > > } > > EXPORT_SYMBOL(drm_plane_force_disable); > > > > +/* > > + * Checks that the framebuffer is big enough for the CRTC viewport > > + * (x, y, hdisplay, vdisplay) > > + */ > > +static int drm_crtc_check_viewport(const struct drm_crtc *crtc, > > + int x, int y, > > + const struct drm_display_mode *mode, > > + const struct drm_framebuffer *fb) > > + > > +{ > > + int hdisplay, vdisplay; > > + > > + hdisplay = mode->hdisplay; > > + vdisplay = mode->vdisplay; > > + > > + if (drm_mode_is_stereo(mode)) { > > + struct drm_display_mode adjusted = *mode; > > + > > + drm_mode_set_crtcinfo(&adjusted, CRTC_STEREO_DOUBLE); > > + hdisplay = adjusted.crtc_hdisplay; > > + vdisplay = adjusted.crtc_vdisplay; > > + } > > + > > + if (crtc->invert_dimensions) > > + swap(hdisplay, vdisplay); > > + > > + if (hdisplay > fb->width || > > + vdisplay > fb->height || > > + x > fb->width - hdisplay || > > + y > fb->height - vdisplay) { > > + DRM_DEBUG_KMS("Invalid fb size %ux%u for CRTC viewport %ux%u+%d+%d%s.\n", > > + fb->width, fb->height, hdisplay, vdisplay, x, y, > > + crtc->invert_dimensions ? " (inverted)" : ""); > > + return -ENOSPC; > > + } > > + > > + return 0; > > +} > > + > > +/* > > + * 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 pageflip 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 assume most hardware can't reposition or scale the primary > > + * plane, so we require that crtc_x = crtc_y = 0 and that src_w/src_h match the > > + * current mode. 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 = crtc_x, > > + .y = crtc_y, > > + }; > > + struct drm_connector **connector_list; > > + struct drm_framebuffer *tmpfb; > > + int num_connectors, ret; > > + > > + /* setplane API takes shifted source rectangle values; unshift them */ > > + src_x >>= 16; > > + src_y >>= 16; > > + src_w >>= 16; > > + src_h >>= 16; > > + > > + /* 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; > > + } > > + > > + if (!crtc->enabled) { > > + DRM_DEBUG_KMS("Cannot update primary plane of a disabled CRTC.\n"); > > + return -EINVAL; > > + } > > + > > + 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. > > + */ > > + 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. > > + * > > + * 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; > > + > > + 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 > > + * > > + * 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) > > +{ > > + 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; > > + } > > + > > + /* possible_crtc's will be filled in later by crtc_init */ > > + ret = drm_plane_init(dev, primary, 0, &drm_primary_helper_funcs, > > + legacy_modeset_formats, > > + ARRAY_SIZE(legacy_modeset_formats), > > + DRM_PLANE_TYPE_PRIMARY); > > + if (ret) { > > + kfree(primary); > > + primary = NULL; > > + } > > + > > + return primary; > > +} > > +EXPORT_SYMBOL(drm_primary_helper_create_plane); > > + > > static int drm_mode_create_standard_connector_properties(struct drm_device *dev) > > { > > struct drm_property *edid; > > @@ -2185,45 +2434,6 @@ int drm_mode_set_config_internal(struct drm_mode_set *set) > > } > > EXPORT_SYMBOL(drm_mode_set_config_internal); > > > > -/* > > - * Checks that the framebuffer is big enough for the CRTC viewport > > - * (x, y, hdisplay, vdisplay) > > - */ > > -static int drm_crtc_check_viewport(const struct drm_crtc *crtc, > > - int x, int y, > > - const struct drm_display_mode *mode, > > - const struct drm_framebuffer *fb) > > - > > -{ > > - int hdisplay, vdisplay; > > - > > - hdisplay = mode->hdisplay; > > - vdisplay = mode->vdisplay; > > - > > - if (drm_mode_is_stereo(mode)) { > > - struct drm_display_mode adjusted = *mode; > > - > > - drm_mode_set_crtcinfo(&adjusted, CRTC_STEREO_DOUBLE); > > - hdisplay = adjusted.crtc_hdisplay; > > - vdisplay = adjusted.crtc_vdisplay; > > - } > > - > > - if (crtc->invert_dimensions) > > - swap(hdisplay, vdisplay); > > - > > - if (hdisplay > fb->width || > > - vdisplay > fb->height || > > - x > fb->width - hdisplay || > > - y > fb->height - vdisplay) { > > - DRM_DEBUG_KMS("Invalid fb size %ux%u for CRTC viewport %ux%u+%d+%d%s.\n", > > - fb->width, fb->height, hdisplay, vdisplay, x, y, > > - crtc->invert_dimensions ? " (inverted)" : ""); > > - return -ENOSPC; > > - } > > - > > - return 0; > > -} > > - > > /** > > * drm_mode_setcrtc - set CRTC configuration > > * @dev: drm device for the ioctl > > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h > > index e69ada8..f43fa92 100644 > > --- a/include/drm/drm_crtc.h > > +++ b/include/drm/drm_crtc.h > > @@ -581,6 +581,87 @@ struct drm_plane { > > enum drm_plane_type type; > > }; > > > > +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); > > + > > +/* > > + * This is the list of formats that have historically been accepted by the > > + * modeset API. The primary plane helpers use this list by default, but > > + * individual drivers may provide their own primary plane initialization > > + * that provides a more hw-specific format list. > > + */ > > +const static uint32_t legacy_modeset_formats[] = { > > + DRM_FORMAT_C8, > > + DRM_FORMAT_RGB332, > > + DRM_FORMAT_BGR233, > > + DRM_FORMAT_XRGB4444, > > + DRM_FORMAT_XBGR4444, > > + DRM_FORMAT_RGBX4444, > > + DRM_FORMAT_BGRX4444, > > + DRM_FORMAT_ARGB4444, > > + DRM_FORMAT_ABGR4444, > > + DRM_FORMAT_RGBA4444, > > + DRM_FORMAT_BGRA4444, > > + DRM_FORMAT_XRGB1555, > > + DRM_FORMAT_XBGR1555, > > + DRM_FORMAT_RGBX5551, > > + DRM_FORMAT_BGRX5551, > > + DRM_FORMAT_ARGB1555, > > + DRM_FORMAT_ABGR1555, > > + DRM_FORMAT_RGBA5551, > > + DRM_FORMAT_BGRA5551, > > + DRM_FORMAT_RGB565, > > + DRM_FORMAT_BGR565, > > + DRM_FORMAT_RGB888, > > + DRM_FORMAT_BGR888, > > + DRM_FORMAT_XRGB8888, > > + DRM_FORMAT_XBGR8888, > > + DRM_FORMAT_RGBX8888, > > + DRM_FORMAT_BGRX8888, > > + DRM_FORMAT_ARGB8888, > > + DRM_FORMAT_ABGR8888, > > + DRM_FORMAT_RGBA8888, > > + DRM_FORMAT_BGRA8888, > > + DRM_FORMAT_XRGB2101010, > > + DRM_FORMAT_XBGR2101010, > > + DRM_FORMAT_RGBX1010102, > > + DRM_FORMAT_BGRX1010102, > > + DRM_FORMAT_ARGB2101010, > > + DRM_FORMAT_ABGR2101010, > > + DRM_FORMAT_RGBA1010102, > > + DRM_FORMAT_BGRA1010102, > > + DRM_FORMAT_YUYV, > > + DRM_FORMAT_YVYU, > > + DRM_FORMAT_UYVY, > > + DRM_FORMAT_VYUY, > > + DRM_FORMAT_AYUV, > > + DRM_FORMAT_NV12, > > + DRM_FORMAT_NV21, > > + DRM_FORMAT_NV16, > > + DRM_FORMAT_NV61, > > + DRM_FORMAT_NV24, > > + DRM_FORMAT_NV42, > > + DRM_FORMAT_YUV410, > > + DRM_FORMAT_YVU410, > > + DRM_FORMAT_YUV411, > > + DRM_FORMAT_YVU411, > > + DRM_FORMAT_YUV420, > > + DRM_FORMAT_YVU420, > > + DRM_FORMAT_YUV422, > > + DRM_FORMAT_YVU422, > > + DRM_FORMAT_YUV444, > > + DRM_FORMAT_YVU444, > > +}; > > + > > /** > > * drm_bridge_funcs - drm_bridge control functions > > * @mode_fixup: Try to fixup (or reject entirely) proposed mode for this bridge > > -- > > 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 Thu, Mar 20, 2014 at 12:01 AM, Matt Roper <matthew.d.roper@intel.com> wrote: > On Wed, Mar 19, 2014 at 01:24:23PM +0100, Daniel Vetter wrote: >> 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@intel.com> >> >> One thing I've noticed with planes is that we don't have any per-plane >> size limits. Which will be annoying for cursors at least, but looking at >> intel's hw history there are other planes with limits smaller than >> dev->mode_config.max_widht/height. >> >> I think as part of the universal plane work here we should add this to the >> getplane ioctl (we can extend at the end with full backwards compat) and >> use the max_width/height as default for primary/overlay planes and >> cursor_width/height for cursor planes. > > I'm not sure I understand how to (cleanly) extend the existing ioctl > safely. Userspace (libdrm) allocates a drm_mode_get_plane structure on > the stack with a specific size. If we try to extend this structure to > return more information, and have the kernel write into the new fields, > aren't we just going to be spilling over into other userspace stack > variables if we run an old libdrm on the new kernel? The only > approaches I see that could make this work would be huge ugly hacks: > * Reclaiming the top few bits of plane_id to use as "I sent you the > new, larger structure" capability flags (which assumes plane ID's are > always small enough to leave those bits 0 on current kernels; this > would effectively reduce our plane ID address space). > * Figure out a way to encode extra information as bogus pixel formats > and shove it into the format list returned to userspace. Presumably > userspace would just ignore/skip the bogus formats. > > I suppose we could add a new GetPlane2 ioctl or something that returned > more info, but I figured it was probably easier to just shove max plane > size (and a bunch of other plane capabilities / limitations) into some > new read-only plane properties. Read-only properties are easy to extend > if we find other pieces of information we want to return in the future, > so that seems like the most natural interface to me. The size userspace expects is passed in through the ioctl number. It then zero-extends the structure correctly for both forward and backwards compatibility. Which means that yeah, we can extend the struct at the end, recompile libdrm and as long as the kernel treats 0 in the new fields sanely. -Daniel
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 0983996..db54ae9 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -1118,6 +1118,255 @@ void drm_plane_force_disable(struct drm_plane *plane) } EXPORT_SYMBOL(drm_plane_force_disable); +/* + * Checks that the framebuffer is big enough for the CRTC viewport + * (x, y, hdisplay, vdisplay) + */ +static int drm_crtc_check_viewport(const struct drm_crtc *crtc, + int x, int y, + const struct drm_display_mode *mode, + const struct drm_framebuffer *fb) + +{ + int hdisplay, vdisplay; + + hdisplay = mode->hdisplay; + vdisplay = mode->vdisplay; + + if (drm_mode_is_stereo(mode)) { + struct drm_display_mode adjusted = *mode; + + drm_mode_set_crtcinfo(&adjusted, CRTC_STEREO_DOUBLE); + hdisplay = adjusted.crtc_hdisplay; + vdisplay = adjusted.crtc_vdisplay; + } + + if (crtc->invert_dimensions) + swap(hdisplay, vdisplay); + + if (hdisplay > fb->width || + vdisplay > fb->height || + x > fb->width - hdisplay || + y > fb->height - vdisplay) { + DRM_DEBUG_KMS("Invalid fb size %ux%u for CRTC viewport %ux%u+%d+%d%s.\n", + fb->width, fb->height, hdisplay, vdisplay, x, y, + crtc->invert_dimensions ? " (inverted)" : ""); + return -ENOSPC; + } + + return 0; +} + +/* + * 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 pageflip 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 assume most hardware can't reposition or scale the primary + * plane, so we require that crtc_x = crtc_y = 0 and that src_w/src_h match the + * current mode. 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 = crtc_x, + .y = crtc_y, + }; + struct drm_connector **connector_list; + struct drm_framebuffer *tmpfb; + int num_connectors, ret; + + /* setplane API takes shifted source rectangle values; unshift them */ + src_x >>= 16; + src_y >>= 16; + src_w >>= 16; + src_h >>= 16; + + /* 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; + } + + if (!crtc->enabled) { + DRM_DEBUG_KMS("Cannot update primary plane of a disabled CRTC.\n"); + return -EINVAL; + } + + 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. + */ + 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. + * + * 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; + + 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 + * + * 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) +{ + 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; + } + + /* possible_crtc's will be filled in later by crtc_init */ + ret = drm_plane_init(dev, primary, 0, &drm_primary_helper_funcs, + legacy_modeset_formats, + ARRAY_SIZE(legacy_modeset_formats), + DRM_PLANE_TYPE_PRIMARY); + if (ret) { + kfree(primary); + primary = NULL; + } + + return primary; +} +EXPORT_SYMBOL(drm_primary_helper_create_plane); + static int drm_mode_create_standard_connector_properties(struct drm_device *dev) { struct drm_property *edid; @@ -2185,45 +2434,6 @@ int drm_mode_set_config_internal(struct drm_mode_set *set) } EXPORT_SYMBOL(drm_mode_set_config_internal); -/* - * Checks that the framebuffer is big enough for the CRTC viewport - * (x, y, hdisplay, vdisplay) - */ -static int drm_crtc_check_viewport(const struct drm_crtc *crtc, - int x, int y, - const struct drm_display_mode *mode, - const struct drm_framebuffer *fb) - -{ - int hdisplay, vdisplay; - - hdisplay = mode->hdisplay; - vdisplay = mode->vdisplay; - - if (drm_mode_is_stereo(mode)) { - struct drm_display_mode adjusted = *mode; - - drm_mode_set_crtcinfo(&adjusted, CRTC_STEREO_DOUBLE); - hdisplay = adjusted.crtc_hdisplay; - vdisplay = adjusted.crtc_vdisplay; - } - - if (crtc->invert_dimensions) - swap(hdisplay, vdisplay); - - if (hdisplay > fb->width || - vdisplay > fb->height || - x > fb->width - hdisplay || - y > fb->height - vdisplay) { - DRM_DEBUG_KMS("Invalid fb size %ux%u for CRTC viewport %ux%u+%d+%d%s.\n", - fb->width, fb->height, hdisplay, vdisplay, x, y, - crtc->invert_dimensions ? " (inverted)" : ""); - return -ENOSPC; - } - - return 0; -} - /** * drm_mode_setcrtc - set CRTC configuration * @dev: drm device for the ioctl diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index e69ada8..f43fa92 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -581,6 +581,87 @@ struct drm_plane { enum drm_plane_type type; }; +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); + +/* + * This is the list of formats that have historically been accepted by the + * modeset API. The primary plane helpers use this list by default, but + * individual drivers may provide their own primary plane initialization + * that provides a more hw-specific format list. + */ +const static uint32_t legacy_modeset_formats[] = { + DRM_FORMAT_C8, + DRM_FORMAT_RGB332, + DRM_FORMAT_BGR233, + DRM_FORMAT_XRGB4444, + DRM_FORMAT_XBGR4444, + DRM_FORMAT_RGBX4444, + DRM_FORMAT_BGRX4444, + DRM_FORMAT_ARGB4444, + DRM_FORMAT_ABGR4444, + DRM_FORMAT_RGBA4444, + DRM_FORMAT_BGRA4444, + DRM_FORMAT_XRGB1555, + DRM_FORMAT_XBGR1555, + DRM_FORMAT_RGBX5551, + DRM_FORMAT_BGRX5551, + DRM_FORMAT_ARGB1555, + DRM_FORMAT_ABGR1555, + DRM_FORMAT_RGBA5551, + DRM_FORMAT_BGRA5551, + DRM_FORMAT_RGB565, + DRM_FORMAT_BGR565, + DRM_FORMAT_RGB888, + DRM_FORMAT_BGR888, + DRM_FORMAT_XRGB8888, + DRM_FORMAT_XBGR8888, + DRM_FORMAT_RGBX8888, + DRM_FORMAT_BGRX8888, + DRM_FORMAT_ARGB8888, + DRM_FORMAT_ABGR8888, + DRM_FORMAT_RGBA8888, + DRM_FORMAT_BGRA8888, + DRM_FORMAT_XRGB2101010, + DRM_FORMAT_XBGR2101010, + DRM_FORMAT_RGBX1010102, + DRM_FORMAT_BGRX1010102, + DRM_FORMAT_ARGB2101010, + DRM_FORMAT_ABGR2101010, + DRM_FORMAT_RGBA1010102, + DRM_FORMAT_BGRA1010102, + DRM_FORMAT_YUYV, + DRM_FORMAT_YVYU, + DRM_FORMAT_UYVY, + DRM_FORMAT_VYUY, + DRM_FORMAT_AYUV, + DRM_FORMAT_NV12, + DRM_FORMAT_NV21, + DRM_FORMAT_NV16, + DRM_FORMAT_NV61, + DRM_FORMAT_NV24, + DRM_FORMAT_NV42, + DRM_FORMAT_YUV410, + DRM_FORMAT_YVU410, + DRM_FORMAT_YUV411, + DRM_FORMAT_YVU411, + DRM_FORMAT_YUV420, + DRM_FORMAT_YVU420, + DRM_FORMAT_YUV422, + DRM_FORMAT_YVU422, + DRM_FORMAT_YUV444, + DRM_FORMAT_YVU444, +}; + /** * drm_bridge_funcs - drm_bridge control functions * @mode_fixup: Try to fixup (or reject entirely) proposed mode for this bridge
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@intel.com> --- drivers/gpu/drm/drm_crtc.c | 288 +++++++++++++++++++++++++++++++++++++++------ include/drm/drm_crtc.h | 81 +++++++++++++ 2 files changed, 330 insertions(+), 39 deletions(-)