[Intel-gfx] [PATCH 6/6] drm/i915: Switch to unified plane cursor handling (v4)
G, Pallavi
pallavi.g at intel.com
Thu Jun 12 13:47:18 CEST 2014
On Tue, 2014-06-10 at 08:28 -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.
>
> v4:
> - Drop drm_gem_object_unreference() that is no longer needed now that
> we receive the GEM obj directly rather than looking up the ID.
> v3:
> - Pass cursor obj to intel_crtc_cursor_set_obj() if cursor fb changes,
> even if 'visible' is false. intel_crtc_cursor_set_obj() will notice
> that the cursor isn't visible and disable it properly, but we still
> need to get intel_crtc->cursor_addr set properly so that we behave
> properly if the cursor becomes visible again in the future without
> changing the cursor buffer (noted by Chris Wilson and verified
> via i-g-t kms_cursor_crc).
> - s/drm_plane_init/drm_universal_plane_init/. Due to type
> compatibility between enum and bool, everything actually works
> correctly with the wrong init call, except for the type of plane that
> gets exposed to userspace (it shows up as type 'primary' rather than
> type 'cursor').
> v2:
> - Remove duplicate dimension checks on cursor
> - Drop explicit cursor disable from crtc destroy (fb & plane
> destruction will take care of that now)
> - Use DRM plane helper to check update parameters
>
> Cc: intel-gfx at lists.freedesktop.org
> Signed-off-by: Matt Roper <matthew.d.roper at intel.com>
> ---
> drivers/gpu/drm/i915/intel_display.c | 166 +++++++++++++++++++++++++----------
> drivers/gpu/drm/i915/intel_drv.h | 1 -
> 2 files changed, 118 insertions(+), 49 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 1beeb2a..1880c18 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; })
>
> @@ -8044,8 +8049,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;
>
> if (on)
> @@ -8180,7 +8185,6 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
> if (intel_crtc->cursor_bo) {
> if (!INTEL_INFO(dev)->cursor_needs_physical)
> i915_gem_object_unpin_from_display_plane(intel_crtc->cursor_bo);
> - drm_gem_object_unreference(&intel_crtc->cursor_bo->base);
> }
>
> mutex_unlock(&dev->struct_mutex);
> @@ -8208,38 +8212,6 @@ fail:
> return ret;
> }
>
> -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)
> {
> @@ -8885,8 +8857,6 @@ static void intel_crtc_destroy(struct drm_crtc *crtc)
> kfree(work);
> }
>
> - intel_crtc_cursor_set(crtc, NULL, 0, 0, 0)
Please help me to understand how the cursor enable/disable will
handled in the legacy path if we remove the cursor disable from
intel_crtc_destroy
> -
> drm_crtc_cleanup(crtc);
>
> kfree(intel_crtc);
> @@ -10942,8 +10912,6 @@ out_config:
> }
>
> static const struct drm_crtc_funcs intel_crtc_funcs = {
> - .cursor_set = intel_crtc_cursor_set,
> - .cursor_move = intel_crtc_cursor_move,
I don't find the corresponding changes in the drm layer related to the
cursor_set and cursor_move removal. Even in the patch 3 only for the
universal plane drm_mode_cursor_universal is called what about the
legacy path?
> .gamma_set = intel_crtc_gamma_set,
> .set_config = intel_crtc_set_config,
> .destroy = intel_crtc_destroy,
> -11196,7 +11164,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)
> {
> struct intel_plane *intel_plane = to_intel_plane(plane);
> drm_plane_cleanup(plane);
> @@ -11206,7 +11175,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,
> @@ -11242,11 +11211,100 @@ 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 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,
> + };
> + bool visible;
> + int ret;
> +
> + ret = drm_plane_helper_check_update(plane, crtc, fb,
> + &src, &dest, &clip,
> + DRM_PLANE_HELPER_NO_SCALING,
> + DRM_PLANE_HELPER_NO_SCALING,
> + true, true, &visible);
> + if (ret)
> + return ret;
> +
> + crtc->cursor_x = crtc_x;
> + crtc->cursor_y = crtc_y;
> + if (fb != crtc->cursor->fb) {
> + return intel_crtc_cursor_set_obj(crtc, obj, crtc_w, crtc_h);
> + } else {
> + intel_crtc_update_cursor(crtc, visible);
> + 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_universal_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);
> @@ -11254,13 +11312,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++) {
> @@ -11293,6 +11355,14 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
> drm_crtc_helper_add(&intel_crtc->base, &intel_helper_funcs);
>
> WARN_ON(drm_crtc_index(&intel_crtc->base) != intel_crtc->pipe);
> + 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 78d4124..57a36ab 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -384,7 +384,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;
> uint32_t cursor_cntl;
> uint32_t cursor_base;
More information about the Intel-gfx
mailing list