On Thu, May 15, 2014 at 06:17:29PM -0700, Matt Roper wrote:
> The DRM core will translate calls to legacy cursor ioctls into universal
> cursor calls automatically, so there's no need to maintain the legacy
> cursor support. This greatly simplifies the transition since we don't
> have to handle reference counting differently depending on which cursor
> interface was called.
>
> The aim here is to transition to the universal plane interface with
> minimal code change. There's a lot of cleanup that can be done (e.g.,
> using state stored in crtc->cursor->fb rather than intel_crtc) that is
> left to future patches.
>
> Signed-off-by: Matt Roper <matthew.d.ro...@intel.com>
Bunch of comments below. I think for testing we have good coverage of
functional cursor tests already, and with the backwards-compat code in the
drm core we'll excerise the cursor plane code sufficiently imo.
But there's a bit of corner-case checking:
- Interactions between legacy cursor ioctl and the new stuff to check the
refcounting and correctness wrt set_bo and visible and fun like that.
- Maybe some checks for the no-upscaling and no-clipping stuff. Hopefully
you can easily copy&paste this from the primary plane tests.
But in the end you're worked on this and are the expert, so if you
think a different focus gives a better payoff then I'm all ears - we want
to write tests that have a good chance of catching real bugs after all ;-)
Cheers, Daniel
> ---
> drivers/gpu/drm/i915/intel_display.c | 194
> ++++++++++++++++++++++++-----------
> drivers/gpu/drm/i915/intel_drv.h | 1 -
> 2 files changed, 136 insertions(+), 59 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 7e75b13..d145130 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -68,6 +68,11 @@ static const uint32_t intel_primary_formats_gen4[] = {
> DRM_FORMAT_ABGR2101010,
> };
>
> +/* Cursor formats */
> +static const uint32_t intel_cursor_formats[] = {
> + DRM_FORMAT_ARGB8888,
> +};
> +
> #define DIV_ROUND_CLOSEST_ULL(ll, d) \
> ({ unsigned long long _tmp = (ll)+(d)/2; do_div(_tmp, d); _tmp; })
>
> @@ -7967,8 +7972,8 @@ static void intel_crtc_update_cursor(struct drm_crtc
> *crtc,
> struct drm_i915_private *dev_priv = dev->dev_private;
> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> int pipe = intel_crtc->pipe;
> - int x = intel_crtc->cursor_x;
> - int y = intel_crtc->cursor_y;
> + int x = crtc->cursor_x;
> + int y = crtc->cursor_y;
> u32 base = 0, pos = 0;
> bool visible;
>
> @@ -8023,8 +8028,6 @@ static void intel_crtc_update_cursor(struct drm_crtc
> *crtc,
> * @height: cursor height
> *
> * Programs a crtc's cursor plane with the specified GEM object.
> - * intel_crtc_cursor_set() should be used instead if we have a userspace
> buffer
> - * handle rather than the actual GEM object.
> */
> static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
> struct drm_i915_gem_object *obj,
> @@ -8143,48 +8146,6 @@ fail:
> return ret;
> }
>
> -/**
> - * intel_crtc_cursor_set - Update cursor buffer
> - * @crtc: crtc to set cursor for
> - * @handle: userspace handle of buffer to set cursor to
> - * @width: cursor width
> - * @height: cursor height
> - *
> - * Programs a crtc's cursor plane with the buffer referred to by userspace
> - * handle @handle.
> - */
> -static int intel_crtc_cursor_set(struct drm_crtc *crtc,
> - struct drm_file *file,
> - uint32_t handle,
> - uint32_t width, uint32_t height)
> -{
> - struct drm_device *dev = crtc->dev;
> - struct drm_i915_gem_object *obj;
> -
> - if (handle) {
> - obj = to_intel_bo(drm_gem_object_lookup(dev, file, handle));
> - if (&obj->base == NULL)
> - return -ENOENT;
> - } else {
> - obj = NULL;
> - }
> -
> - return intel_crtc_cursor_set_obj(crtc, obj, width, height);
> -}
> -
> -static int intel_crtc_cursor_move(struct drm_crtc *crtc, int x, int y)
> -{
> - struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -
> - intel_crtc->cursor_x = clamp_t(int, x, SHRT_MIN, SHRT_MAX);
> - intel_crtc->cursor_y = clamp_t(int, y, SHRT_MIN, SHRT_MAX);
> -
> - if (intel_crtc->active)
> - intel_crtc_update_cursor(crtc, intel_crtc->cursor_bo != NULL);
> -
> - return 0;
> -}
> -
> static void intel_crtc_gamma_set(struct drm_crtc *crtc, u16 *red, u16 *green,
> u16 *blue, uint32_t start, uint32_t size)
> {
> @@ -8806,7 +8767,7 @@ static void intel_crtc_destroy(struct drm_crtc *crtc)
> kfree(work);
> }
>
> - intel_crtc_cursor_set(crtc, NULL, 0, 0, 0);
> + intel_crtc_cursor_set_obj(crtc, NULL, 0, 0);
This shouldn't be needed any more with the plane cleanup code - all the
refcounting is now done with fbs.
>
> drm_crtc_cleanup(crtc);
>
> @@ -10784,8 +10745,6 @@ out_config:
> }
>
> static const struct drm_crtc_funcs intel_crtc_funcs = {
> - .cursor_set = intel_crtc_cursor_set,
> - .cursor_move = intel_crtc_cursor_move,
> .gamma_set = intel_crtc_gamma_set,
> .set_config = intel_crtc_set_config,
> .destroy = intel_crtc_destroy,
> @@ -11018,7 +10977,8 @@ intel_primary_plane_setplane(struct drm_plane *plane,
> struct drm_crtc *crtc,
> return 0;
> }
>
> -static void intel_primary_plane_destroy(struct drm_plane *plane)
> +/* Common destruction function for both primary and cursor planes */
> +static void intel_plane_destroy(struct drm_plane *plane)
Actually a review comment for your primary plane series: We have this
function as intel_destroy_plane in intel_sprite.c already ;-)
> {
> struct intel_plane *intel_plane = to_intel_plane(plane);
> drm_plane_cleanup(plane);
> @@ -11028,7 +10988,7 @@ static void intel_primary_plane_destroy(struct
> drm_plane *plane)
> static const struct drm_plane_funcs intel_primary_plane_funcs = {
> .update_plane = intel_primary_plane_setplane,
> .disable_plane = intel_primary_plane_disable,
> - .destroy = intel_primary_plane_destroy,
> + .destroy = intel_plane_destroy,
> };
>
> static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
> @@ -11064,11 +11024,116 @@ static struct drm_plane
> *intel_primary_plane_create(struct drm_device *dev,
> return &primary->base;
> }
>
> +static int
> +intel_cursor_plane_disable(struct drm_plane *plane)
> +{
> + if (!plane->fb)
> + return 0;
> +
> + BUG_ON(!plane->crtc);
> +
> + return intel_crtc_cursor_set_obj(plane->crtc, NULL, 0, 0);
> +}
> +
> +static int
> +intel_cursor_plane_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_device *dev = crtc->dev;
> + struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> + struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
> + struct drm_i915_gem_object *obj = intel_fb->obj;
> + struct drm_rect dest = {
> + /* integer pixels */
> + .x1 = crtc_x,
> + .y1 = crtc_y,
> + .x2 = crtc_x + crtc_w,
> + .y2 = crtc_y + crtc_h,
> + };
> + struct drm_rect src = {
> + /* 16.16 fixed point */
> + .x1 = src_x,
> + .y1 = src_y,
> + .x2 = src_x + src_w,
> + .y2 = src_y + src_h,
> + };
> + const struct drm_rect clip = {
> + /* integer pixels */
> + .x2 = intel_crtc->config.pipe_src_w,
> + .y2 = intel_crtc->config.pipe_src_h,
> + };
> + int hscale, vscale;
> + bool visible;
> +
> + /* Check scaling */
> + hscale = drm_rect_calc_hscale(&src, &dest,
> + DRM_PLANE_HELPER_NO_SCALING,
> + DRM_PLANE_HELPER_NO_SCALING);
> + vscale = drm_rect_calc_vscale(&src, &dest,
> + DRM_PLANE_HELPER_NO_SCALING,
> + DRM_PLANE_HELPER_NO_SCALING);
> + if (hscale < 0 || vscale < 0) {
> + DRM_DEBUG_KMS("Invalid scaling of cursor plane\n");
> + return -ERANGE;
> + }
Can't we reuse the check helpers for primary planes here? We again want to
scaling and no additional clipping (besides the viewport clipping).
> +
> + /* Check dimensions */
> + if (!((crtc_w == 64 && crtc_h == 64) ||
> + (crtc_w == 128 && crtc_h == 128 && !IS_GEN2(dev)) ||
> + (crtc_w == 256 && crtc_h == 256 && !IS_GEN2(dev)))) {
> + DRM_DEBUG_KMS("Cursor dimension not supported: %dx%d\n",
> + crtc_w, crtc_h);
> + return -EINVAL;
> + }
Isn't this check redudnant with the one in cursor_set_obj?
> +
> + /* Clip to display; if no longer visible after clipping, disable */
> + visible = drm_rect_clip_scaled(&src, &dest, &clip, hscale, vscale);
> +
> + crtc->cursor_x = crtc_x;
> + crtc->cursor_y = crtc_y;
> + if (fb != crtc->cursor->fb) {
> + return intel_crtc_cursor_set_obj(crtc, visible ? obj : NULL,
> + crtc_w, crtc_h);
> + } else {
> + intel_crtc_update_cursor(crtc, true);
> + return 0;
> + }
> +}
> +static const struct drm_plane_funcs intel_cursor_plane_funcs = {
> + .update_plane = intel_cursor_plane_update,
> + .disable_plane = intel_cursor_plane_disable,
> + .destroy = intel_plane_destroy,
> +};
> +
> +static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
> + int pipe)
> +{
> + struct intel_plane *cursor;
> +
> + cursor = kzalloc(sizeof(*cursor), GFP_KERNEL);
> + if (cursor == NULL)
> + return NULL;
> +
> + cursor->can_scale = false;
> + cursor->max_downscale = 1;
> + cursor->pipe = pipe;
> + cursor->plane = pipe;
> +
> + drm_plane_init(dev, &cursor->base, 0,
> + &intel_cursor_plane_funcs, intel_cursor_formats,
> + ARRAY_SIZE(intel_cursor_formats), DRM_PLANE_TYPE_CURSOR);
> + return &cursor->base;
> +}
> +
> static void intel_crtc_init(struct drm_device *dev, int pipe)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
> struct intel_crtc *intel_crtc;
> - struct drm_plane *primary;
> + struct drm_plane *primary = NULL;
> + struct drm_plane *cursor = NULL;
> int i, ret;
>
> intel_crtc = kzalloc(sizeof(*intel_crtc), GFP_KERNEL);
> @@ -11076,13 +11141,17 @@ static void intel_crtc_init(struct drm_device *dev,
> int pipe)
> return;
>
> primary = intel_primary_plane_create(dev, pipe);
> + if (!primary)
> + goto fail;
> +
> + cursor = intel_cursor_plane_create(dev, pipe);
> + if (!cursor)
> + goto fail;
> +
> ret = drm_crtc_init_with_planes(dev, &intel_crtc->base, primary,
> - NULL, &intel_crtc_funcs);
> - if (ret) {
> - drm_plane_cleanup(primary);
> - kfree(intel_crtc);
> - return;
> - }
> + cursor, &intel_crtc_funcs);
> + if (ret)
> + goto fail;
>
> drm_mode_crtc_set_gamma_size(&intel_crtc->base, 256);
> for (i = 0; i < 256; i++) {
> @@ -11110,6 +11179,15 @@ static void intel_crtc_init(struct drm_device *dev,
> int pipe)
> dev_priv->pipe_to_crtc_mapping[intel_crtc->pipe] = &intel_crtc->base;
>
> drm_crtc_helper_add(&intel_crtc->base, &intel_helper_funcs);
> +
> + return;
> +
> +fail:
> + if (primary)
> + drm_plane_cleanup(primary);
> + if (cursor)
> + drm_plane_cleanup(cursor);
> + kfree(intel_crtc);
> }
>
> enum pipe intel_get_pipe_from_connector(struct intel_connector *connector)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 32a74e1..8374f37a 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -378,7 +378,6 @@ struct intel_crtc {
>
> struct drm_i915_gem_object *cursor_bo;
> uint32_t cursor_addr;
> - int16_t cursor_x, cursor_y;
> int16_t cursor_width, cursor_height;
> bool cursor_visible;
>
> --
> 1.8.5.1
>
> _______________________________________________
> dri-devel mailing list
> dri-de...@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
