| Message ID | 1402414093-13401-7-git-send-email-matthew.d.roper@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@lists.freedesktop.org > Signed-off-by: Matt Roper <matthew.d.roper@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;
On Thu, Jun 12, 2014 at 04:47:18AM -0700, G, Pallavi wrote: > On Tue, 2014-06-10 at 08:28 -0700, Matt Roper wrote: ... > > @@ -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 Good question. When the driver is shutting down the DRM core tears down all the KMS stuff associated with the driver. One of those steps is destroying the CRTC which, as you note, previously would take care of turning off the cursor plane. However now that the cursor exists as its own drm_plane, and whatever is being scanned out by the cursor is a real drm_framebuffer that the DRM core knows about, the cursor's FB should get destroyed by the DRM core, which will trigger the cursor disable entrypoint on the driver. The call sequence is drm_framebuffer_remove() -> drm_force_plane_disable() -> intel_cursor_plane_disable() > > - > > 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? Right, the driver entrypoints remain in the drm_crtc_funcs structure because other non-i915 drivers may not have implemented universal cursor planes, so the DRM core will still call into their .cursor_set and .cursor_move entrypoints when they issue a legacy cursor ioctl. We don't want to force other driver authors to update to universal planes until they're ready, so we want to keep the old code paths working until everyone has updated. Drivers like i915 that do get updated to have universal cursor planes will no longer receive calls into these legacy entrypoints anymore (since everything will come into the universal entrypoint), so there's no need for us to keep the legacy .cursor_set and .cursor_move around in our own driver. Matt
On Thu, 2014-06-12 at 07:57 -0700, Matt Roper wrote: > On Thu, Jun 12, 2014 at 04:47:18AM -0700, G, Pallavi wrote: > > On Tue, 2014-06-10 at 08:28 -0700, Matt Roper wrote: > ... > > > @@ -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 > > Good question. When the driver is shutting down the DRM core tears down > all the KMS stuff associated with the driver. One of those steps is > destroying the CRTC which, as you note, previously would take care of > turning off the cursor plane. However now that the cursor exists as its > own drm_plane, and whatever is being scanned out by the cursor is a real > drm_framebuffer that the DRM core knows about, the cursor's FB should > get destroyed by the DRM core, which will trigger the cursor disable > entrypoint on the driver. The call sequence is > > drm_framebuffer_remove() -> drm_force_plane_disable() > -> intel_cursor_plane_disable() > > > > > - > > > 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? > > Right, the driver entrypoints remain in the drm_crtc_funcs structure > because other non-i915 drivers may not have implemented universal cursor > planes, so the DRM core will still call into their .cursor_set and > .cursor_move entrypoints when they issue a legacy cursor ioctl. We > don't want to force other driver authors to update to universal planes > until they're ready, so we want to keep the old code paths working until > everyone has updated. > > Drivers like i915 that do get updated to have universal cursor planes > will no longer receive calls into these legacy entrypoints anymore > (since everything will come into the universal entrypoint), so there's > no need for us to keep the legacy .cursor_set and .cursor_move around in > our own driver. > > > Matt > Thanks for the clarification. with this my doubts are clear. I reviewed the entire patch series with respect to the legacy support as well as the universal cursor plane support in general. I dont find any issues and design gaps. I hope Matt is working on the igt based test for the universal plane support (seen some patches related to that in the mail thread). Reviewed-by: Pallavi G <pallavi.g@intel.com>
On Fri, Jun 13, 2014 at 04:35:38AM +0000, G, Pallavi wrote: > On Thu, 2014-06-12 at 07:57 -0700, Matt Roper wrote: > > On Thu, Jun 12, 2014 at 04:47:18AM -0700, G, Pallavi wrote: > > > On Tue, 2014-06-10 at 08:28 -0700, Matt Roper wrote: > > ... > > > > @@ -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 > > > > Good question. When the driver is shutting down the DRM core tears down > > all the KMS stuff associated with the driver. One of those steps is > > destroying the CRTC which, as you note, previously would take care of > > turning off the cursor plane. However now that the cursor exists as its > > own drm_plane, and whatever is being scanned out by the cursor is a real > > drm_framebuffer that the DRM core knows about, the cursor's FB should > > get destroyed by the DRM core, which will trigger the cursor disable > > entrypoint on the driver. The call sequence is > > > > drm_framebuffer_remove() -> drm_force_plane_disable() > > -> intel_cursor_plane_disable() > > > > > > > > - > > > > 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? > > > > Right, the driver entrypoints remain in the drm_crtc_funcs structure > > because other non-i915 drivers may not have implemented universal cursor > > planes, so the DRM core will still call into their .cursor_set and > > .cursor_move entrypoints when they issue a legacy cursor ioctl. We > > don't want to force other driver authors to update to universal planes > > until they're ready, so we want to keep the old code paths working until > > everyone has updated. > > > > Drivers like i915 that do get updated to have universal cursor planes > > will no longer receive calls into these legacy entrypoints anymore > > (since everything will come into the universal entrypoint), so there's > > no need for us to keep the legacy .cursor_set and .cursor_move around in > > our own driver. > > > > > > Matt > > > Thanks for the clarification. > with this my doubts are clear. I reviewed the entire patch series with > respect to the legacy support as well as the universal cursor plane > support in general. I dont find any issues and design gaps. I hope Matt > is working on the igt based test for the universal plane support (seen > some patches related to that in the mail thread). > > Reviewed-by: Pallavi G <pallavi.g@intel.com> Thanks a lot for the review and patches, all merged to dinq. -Daniel
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); - 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, .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;
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@lists.freedesktop.org Signed-off-by: Matt Roper <matthew.d.roper@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(-)