On Tue, Nov 19, 2019 at 11:14:43AM +0100, Daniel Vetter wrote:

On Fri, Nov 15, 2019 at 09:42:00PM +0200, Ville Syrjala wrote:

...

From: Ville Syrjälä ville.syrjala@linux.intel.com

Yank out the code for the plane->fb/old_fb/crtc handling from the page flip path into page_flip_internal(), and provide a simpler variant for atomic drivers.

We'll also move the fb vs. src viewport checks into the new functions as they are slightly different between the two paths. If the atomic .page_flip() implementations are guaranteed to call drm_atomic_plane_check() we could even drop the check entirely from page_flip_atomic(). For now toss in a FIXME.

v2: Bit more polish in page_flip_internal()

Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com

drivers/gpu/drm/drm_plane.c | 159 +++++++++++++++++++++++------------- 1 file changed, 102 insertions(+), 57 deletions(-)

diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c index 38878da5b704..6052475a20a5 100644 --- a/drivers/gpu/drm/drm_plane.c +++ b/drivers/gpu/drm/drm_plane.c @@ -1031,13 +1031,109 @@ int drm_mode_cursor2_ioctl(struct drm_device *dev, return drm_mode_cursor_common(dev, req, file_priv); }

+static int page_flip_check_fbs(const struct drm_framebuffer *fb,

• 	       const struct drm_framebuffer *old_fb)
  

+{

• /* The framebuffer is currently unbound, presumably

• * due to a hotplug event, that userspace has not
  

• * yet discovered.
  

• */
  

• if (!old_fb)

• return -EBUSY;
  

• if (old_fb->format != fb->format) {

• DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n");
  

• return -EINVAL;
  

• }
• return 0;

+}

+static int page_flip_internal(struct drm_crtc *crtc,

• 	      struct drm_framebuffer *fb,
  

• 	      struct drm_pending_vblank_event *e,
  

• 	      u32 flags,
  

• 	      u32 target_vblank,
  

• 	      struct drm_modeset_acquire_ctx *ctx)
  

+{

• struct drm_plane *plane = crtc->primary;
• int ret;
• WARN_ON(drm_drv_uses_atomic_modeset(crtc->dev));
• ret = drm_crtc_check_viewport(crtc, crtc->x, crtc->y,

• 		      &crtc->mode, fb);
  

• if (ret)

• return ret;
  

• ret = page_flip_check_fbs(fb, plane->fb);
• if (ret)

• return ret;
  

• plane->old_fb = plane->fb;
• if (crtc->funcs->page_flip_target)

• ret = crtc->funcs->page_flip_target(crtc, fb, e, flags,
  

• 				    target_vblank, ctx);
  

• else

• ret = crtc->funcs->page_flip(crtc, fb, e, flags, ctx);
  

• if (ret) {

• plane->old_fb = NULL;
  

• return ret;
  

• }
• plane->fb = fb;
• drm_framebuffer_get(plane->fb);
• drm_framebuffer_put(plane->old_fb);
• plane->old_fb = NULL;
• return 0;

+}

+static int page_flip_atomic(struct drm_crtc *crtc,

• 	    struct drm_framebuffer *fb,
  

• 	    struct drm_pending_vblank_event *e,
  

• 	    u32 flags,
  

• 	    u32 target_vblank,
  

• 	    struct drm_modeset_acquire_ctx *ctx)
  

+{

• struct drm_plane *plane = crtc->primary;
• struct drm_plane_state *plane_state = plane->state;
• int ret;
• WARN_ON(!drm_drv_uses_atomic_modeset(crtc->dev));
• /*

• * FIXME: Can we assume all drivers end up calling
  

• * drm_atomic_plane_check() in their page flip paths?
  

• * If so we could remove this.
  

• */
  

This is definitely wrong, core has to check these. Currently no driver checks this at all. I think you're thinking of drm_atomic_helper_check_plane_state().

No, I'm thinking of drm_atomic_plane_check(). That one already does the "does the src rect fit into the fb" check. It gets called from drm_atomic_commit()->drm_atomic_check_only() but I wasn't sure if some drivers might take some short circuit path past that for page_flip().

That aside I'm kinda not getting the point of the shuffling/cleanup in the first 4 patches here, so will skip them.

The point was to get that legacy plane->fb/old_fb stuff totally out from the atomic codepath. Right now it's polluting the supposedly higher level code shared by both atomic and legacy drivers.

-Daniel

...

• ret = drm_framebuffer_check_src_coords(plane_state->src_x,

• 			       plane_state->src_y,
  

• 			       plane_state->src_w,
  

• 			       plane_state->src_h,
  

• 			       fb);
  

• if (ret)

• return ret;
  

• ret = page_flip_check_fbs(fb, plane_state->fb);
• if (ret)

• return ret;
  

• if (crtc->funcs->page_flip_target)

• return crtc->funcs->page_flip_target(crtc, fb, e, flags,
  

• 				     target_vblank, ctx);
  

• else

• return crtc->funcs->page_flip(crtc, fb, e, flags, ctx);
  

+}

int drm_mode_page_flip_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv) { struct drm_mode_crtc_page_flip_target *page_flip = data; struct drm_crtc *crtc; struct drm_plane *plane;

• struct drm_framebuffer *fb, *old_fb;

• struct drm_framebuffer *fb; struct drm_pending_vblank_event *e = NULL; u32 target_vblank = page_flip->sequence; struct drm_modeset_acquire_ctx ctx;

@@ -1145,65 +1241,14 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev, if (ret) goto out;

• if (plane->state)

• old_fb = plane->state->fb;
  

• if (drm_drv_uses_atomic_modeset(dev))

• ret = page_flip_atomic(crtc, fb, e, page_flip->flags,
  

• 		       target_vblank, &ctx);
  

  else

• old_fb = plane->fb;
  

• if (old_fb == NULL) {

• /* The framebuffer is currently unbound, presumably
  

•  * due to a hotplug event, that userspace has not
  

•  * yet discovered.
  

•  */
  

• ret = -EBUSY;
  

• goto out;
  

• }
• if (plane->state) {

• const struct drm_plane_state *state = plane->state;
  

• ret = drm_framebuffer_check_src_coords(state->src_x,
  

• 				       state->src_y,
  

• 				       state->src_w,
  

• 				       state->src_h,
  

• 				       fb);
  

• } else {

• ret = drm_crtc_check_viewport(crtc, crtc->x, crtc->y,
  

• 			      &crtc->mode, fb);
  

• }
• if (ret)

• goto out;
  

• if (old_fb->format != fb->format) {

• DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n");
  

• ret = -EINVAL;
  

• goto out;
  

• }
• plane->old_fb = plane->fb;
• if (crtc->funcs->page_flip_target)

• ret = crtc->funcs->page_flip_target(crtc, fb, e,
  

• 				    page_flip->flags,
  

• 				    target_vblank,
  

• 				    &ctx);
  

• else

• ret = crtc->funcs->page_flip(crtc, fb, e, page_flip->flags,
  

• 			     &ctx);
  

• if (ret) {

• /* Keep the old fb, don't unref it. */
  

• plane->old_fb = NULL;
  

• } else {

• if (!plane->state) {
  

• 	plane->fb = fb;
  

• 	drm_framebuffer_get(fb);
  

• }
  

• }

• ret = page_flip_internal(crtc, fb, e, page_flip->flags,
  

• 			 target_vblank, &ctx);
  
  

out:

• if (plane->old_fb)

• drm_framebuffer_put(plane->old_fb);
  

• plane->old_fb = NULL;
• if (ret == -EDEADLK) { ret = drm_modeset_backoff(&ctx); if (!ret)

-- 2.23.0

---

Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

On Mon, Nov 25, 2019 at 10:02:38AM +0100, Daniel Vetter wrote:

On Fri, Nov 22, 2019 at 08:35:13PM +0200, Ville Syrjälä wrote:

...

On Tue, Nov 19, 2019 at 11:14:43AM +0100, Daniel Vetter wrote:

...

On Fri, Nov 15, 2019 at 09:42:00PM +0200, Ville Syrjala wrote:

...

From: Ville Syrjälä ville.syrjala@linux.intel.com

Yank out the code for the plane->fb/old_fb/crtc handling from the page flip path into page_flip_internal(), and provide a simpler variant for atomic drivers.

We'll also move the fb vs. src viewport checks into the new functions as they are slightly different between the two paths. If the atomic .page_flip() implementations are guaranteed to call drm_atomic_plane_check() we could even drop the check entirely from page_flip_atomic(). For now toss in a FIXME.

v2: Bit more polish in page_flip_internal()

Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com

drivers/gpu/drm/drm_plane.c | 159 +++++++++++++++++++++++------------- 1 file changed, 102 insertions(+), 57 deletions(-)

diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c index 38878da5b704..6052475a20a5 100644 --- a/drivers/gpu/drm/drm_plane.c +++ b/drivers/gpu/drm/drm_plane.c @@ -1031,13 +1031,109 @@ int drm_mode_cursor2_ioctl(struct drm_device *dev, return drm_mode_cursor_common(dev, req, file_priv); }

+static int page_flip_check_fbs(const struct drm_framebuffer *fb,

• 	       const struct drm_framebuffer *old_fb)
  

+{

• /* The framebuffer is currently unbound, presumably

• * due to a hotplug event, that userspace has not
  

• * yet discovered.
  

• */
  

• if (!old_fb)

• return -EBUSY;
  

• if (old_fb->format != fb->format) {

• DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n");
  

• return -EINVAL;
  

• }
• return 0;

+}

+static int page_flip_internal(struct drm_crtc *crtc,

• 	      struct drm_framebuffer *fb,
  

• 	      struct drm_pending_vblank_event *e,
  

• 	      u32 flags,
  

• 	      u32 target_vblank,
  

• 	      struct drm_modeset_acquire_ctx *ctx)
  

+{

• struct drm_plane *plane = crtc->primary;
• int ret;
• WARN_ON(drm_drv_uses_atomic_modeset(crtc->dev));
• ret = drm_crtc_check_viewport(crtc, crtc->x, crtc->y,

• 		      &crtc->mode, fb);
  

• if (ret)

• return ret;
  

• ret = page_flip_check_fbs(fb, plane->fb);
• if (ret)

• return ret;
  

• plane->old_fb = plane->fb;
• if (crtc->funcs->page_flip_target)

• ret = crtc->funcs->page_flip_target(crtc, fb, e, flags,
  

• 				    target_vblank, ctx);
  

• else

• ret = crtc->funcs->page_flip(crtc, fb, e, flags, ctx);
  

• if (ret) {

• plane->old_fb = NULL;
  

• return ret;
  

• }
• plane->fb = fb;
• drm_framebuffer_get(plane->fb);
• drm_framebuffer_put(plane->old_fb);
• plane->old_fb = NULL;
• return 0;

+}

+static int page_flip_atomic(struct drm_crtc *crtc,

• 	    struct drm_framebuffer *fb,
  

• 	    struct drm_pending_vblank_event *e,
  

• 	    u32 flags,
  

• 	    u32 target_vblank,
  

• 	    struct drm_modeset_acquire_ctx *ctx)
  

+{

• struct drm_plane *plane = crtc->primary;
• struct drm_plane_state *plane_state = plane->state;
• int ret;
• WARN_ON(!drm_drv_uses_atomic_modeset(crtc->dev));
• /*

• * FIXME: Can we assume all drivers end up calling
  

• * drm_atomic_plane_check() in their page flip paths?
  

• * If so we could remove this.
  

• */
  

This is definitely wrong, core has to check these. Currently no driver checks this at all. I think you're thinking of drm_atomic_helper_check_plane_state().

No, I'm thinking of drm_atomic_plane_check(). That one already does the "does the src rect fit into the fb" check. It gets called from drm_atomic_commit()->drm_atomic_check_only() but I wasn't sure if some drivers might take some short circuit path past that for page_flip().

tbh if you're worried about this, I'm tempted to rip out the option for atomic drivers to overwrite their page_flip implementation, and force the default function onto everyone (drm_atomic_helper_page_flip_target and friends would need to be moved to drm_plane.c for that ofc).

We seem to have 0 need for hacks for old userspace in this area, which is great. So the option to overwrite could only lead to trouble.

...

...

That aside I'm kinda not getting the point of the shuffling/cleanup in the first 4 patches here, so will skip them.

The point was to get that legacy plane->fb/old_fb stuff totally out from the atomic codepath. Right now it's polluting the supposedly higher level code shared by both atomic and legacy drivers.

Hm ... I think if you sell it like that, and maybe even go one further and inline page_flip for all atomic drivers, I can be sold on this. As-is I just didn't really get why you're reworking all this.

Hmm. Inlining doesn't look like 100% trivial code shufling due to .page_flip_target. We'd need some other mechanism for the driver to indicate support for that.

OTOH I'm not sure we even support that stuff on any atomic driver. We have the helper but no one actually uses it. And as ususal I'm totally unusre what amdgpu is doing since I keep forgetting which parts of it are atomic and which are not.

On Fri, Nov 15, 2019 at 09:42:02PM +0200, Ville Syrjala wrote:

From: Ville Syrjälä ville.syrjala@linux.intel.com

Test the basics of drm_atomic_set_mode_for_crtc(), and in particular verify that the function doesn't take the shortcut incorrectly.

Cc: Daniel Vetter daniel@ffwll.ch Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com

---

drivers/gpu/drm/selftests/Makefile | 3 +- .../gpu/drm/selftests/drm_modeset_selftests.h | 2 + .../gpu/drm/selftests/test-drm_atomic_uapi.c | 110 ++++++++++++++++++ .../drm/selftests/test-drm_modeset_common.h | 2 + 4 files changed, 116 insertions(+), 1 deletion(-) create mode 100644 drivers/gpu/drm/selftests/test-drm_atomic_uapi.c

diff --git a/drivers/gpu/drm/selftests/Makefile b/drivers/gpu/drm/selftests/Makefile index d2137342b371..a5adc25bd345 100644 --- a/drivers/gpu/drm/selftests/Makefile +++ b/drivers/gpu/drm/selftests/Makefile @@ -1,6 +1,7 @@ # SPDX-License-Identifier: GPL-2.0-only test-drm_modeset-y := test-drm_modeset_common.o test-drm_plane_helper.o \ test-drm_format.o test-drm_framebuffer.o \

•       test-drm_damage_helper.o test-drm_dp_mst_helper.o
  

•       test-drm_damage_helper.o test-drm_dp_mst_helper.o \
  

•       test-drm_atomic_uapi.o
  
  

obj-$(CONFIG_DRM_DEBUG_SELFTEST) += test-drm_mm.o test-drm_modeset.o test-drm_cmdline_parser.o diff --git a/drivers/gpu/drm/selftests/drm_modeset_selftests.h b/drivers/gpu/drm/selftests/drm_modeset_selftests.h index 1898de0b4a4d..2cc4e2f43286 100644 --- a/drivers/gpu/drm/selftests/drm_modeset_selftests.h +++ b/drivers/gpu/drm/selftests/drm_modeset_selftests.h @@ -6,6 +6,8 @@

• Tests are executed in order by igt/drm_selftests_helper

*/ +selftest(atomic_set_mode_for_crtc, igt_atomic_set_mode_for_crtc) +selftest(atomic_set_zeroed_mode_for_crtc, igt_atomic_set_zeroed_mode_for_crtc) selftest(check_plane_state, igt_check_plane_state) selftest(check_drm_format_block_width, igt_check_drm_format_block_width) selftest(check_drm_format_block_height, igt_check_drm_format_block_height) diff --git a/drivers/gpu/drm/selftests/test-drm_atomic_uapi.c b/drivers/gpu/drm/selftests/test-drm_atomic_uapi.c new file mode 100644 index 000000000000..0f9a4d0d2541 --- /dev/null +++ b/drivers/gpu/drm/selftests/test-drm_atomic_uapi.c @@ -0,0 +1,110 @@ +// SPDX-License-Identifier: GPL-2.0 +/*

• • Test cases for the drm_atomic_uapi functions
• */

+#define pr_fmt(fmt) "drm_atomic_uapi: " fmt

+#include <drm/drm_atomic_uapi.h> +#include <drm/drm_drv.h> +#include <drm/drm_crtc.h> +#include <drm/drm_modes.h>

+#include "test-drm_modeset_common.h"

+static const struct drm_display_mode zeroed_mode;

+static struct drm_driver mock_driver; +static struct drm_device mock_device; +static struct drm_crtc mock_crtc;

+static void init(void) +{

• memset(&mock_driver, 0, sizeof(mock_driver));
• memset(&mock_device, 0, sizeof(mock_device));
• memset(&mock_crtc, 0, sizeof(mock_crtc));
• mock_device.driver = &mock_driver;
• mock_crtc.dev = &mock_device;
• drm_mode_config_init(&mock_device);

+}

Hm some todof for a real mock_driver would be nice. I think the static stuff is a bit too icky, I think we should go a proper mock_drm_device a la mock_gem_device in i915 selftests. and mock_drm_crtc. But meh ...

+static void cleanup(void) +{

• drm_mode_config_cleanup(&mock_device);

+}

+int igt_atomic_set_mode_for_crtc(void *ignored) +{

• static const struct drm_display_mode mode1 = {

• DRM_MODE("640x480", DRM_MODE_TYPE_DRIVER, 25175,
  

• 	 640, 656, 752, 800, 0, 480, 490, 492, 525, 0,
  

• 	 DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC)
  

• };
• static const struct drm_display_mode mode2 = {

• DRM_MODE("1024x768", DRM_MODE_TYPE_DRIVER, 65000,
  

• 	 1024, 1048, 1184, 1344, 0, 768, 771, 777, 806, 0,
  

• 	 DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC)
  

• };
• struct drm_crtc_state crtc_state = {

• .crtc = &mock_crtc,
  

• };
• int ret;
• init();
• /*

• * Make	sure drm_atomic_set_mode_for_crtc()
  

• * updates the crtc state correctly.
  

• */
  

• ret = drm_atomic_set_mode_for_crtc(&crtc_state, &mode1);
• FAIL(ret < 0, "Unable to set mode on crtc\n");
• FAIL_ON(!crtc_state.enable);
• FAIL_ON(memcmp(&crtc_state.mode, &mode1, sizeof(mode1)));
• ret = drm_atomic_set_mode_for_crtc(&crtc_state, &mode2);
• FAIL(ret < 0, "Unable to set mode on crtc\n");
• FAIL_ON(!crtc_state.enable);
• FAIL_ON(memcmp(&crtc_state.mode, &mode2, sizeof(mode2)));
• ret = drm_atomic_set_mode_for_crtc(&crtc_state, NULL);
• FAIL(ret < 0, "Unable to unset mode on crtc\n");
• FAIL_ON(crtc_state.enable);
• FAIL_ON(memcmp(&crtc_state.mode, &zeroed_mode, sizeof(zeroed_mode)));
• cleanup();
• return 0;

+}

+int igt_atomic_set_zeroed_mode_for_crtc(void *ignored) +{

• struct drm_crtc_state crtc_state = {

• .crtc = &mock_crtc,
  

• };
• int ret;
• init();
• /*

• * Make sure drm_atomic_set_mode_for_crtc() doesn't
  

• * take the shortcut and fail to update crtc_state.enable
  

• * when passed a zeroed mode (which is technically not
  

• * valid, but as drm_atomic_set_mode_for_crtc() does no
  

• * mode validation it should still work).
  

• */
  

• ret = drm_atomic_set_mode_for_crtc(&crtc_state, &zeroed_mode);
• FAIL(ret < 0, "Unable to set mode on crtc\n");
• FAIL_ON(!crtc_state.enable);
• FAIL_ON(memcmp(&crtc_state.mode, &zeroed_mode, sizeof(zeroed_mode)));
• ret = drm_atomic_set_mode_for_crtc(&crtc_state, NULL);
• FAIL(ret < 0, "Unable to unset mode on crtc\n");
• FAIL_ON(crtc_state.enable);
• FAIL_ON(memcmp(&crtc_state.mode, &zeroed_mode, sizeof(zeroed_mode)));

Not really seeing the point of why we have to be careful here, but I guess silly stuff can happen, and better to not be accidentally inconsistent.

Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch

• cleanup();
• return 0;

+} diff --git a/drivers/gpu/drm/selftests/test-drm_modeset_common.h b/drivers/gpu/drm/selftests/test-drm_modeset_common.h index 0fcb8bbc6a1b..a1248abf86f9 100644 --- a/drivers/gpu/drm/selftests/test-drm_modeset_common.h +++ b/drivers/gpu/drm/selftests/test-drm_modeset_common.h @@ -13,6 +13,8 @@

#define FAIL_ON(x) FAIL((x), "%s", "FAIL_ON(" __stringify(x) ")\n")

+int igt_atomic_set_mode_for_crtc(void *ignored); +int igt_atomic_set_zeroed_mode_for_crtc(void *ignored); int igt_check_plane_state(void *ignored); int igt_check_drm_format_block_width(void *ignored); int igt_check_drm_format_block_height(void *ignored); -- 2.23.0

On Fri, Nov 15, 2019 at 09:42:03PM +0200, Ville Syrjala wrote:

From: Ville Syrjälä ville.syrjala@linux.intel.com

The early return in drm_atomic_set_mode_for_crtc() isn't quite right. It would mistakenly return and fail to update crtc_state->enable if someone actually tried to set a zeroed mode on a currently disabled crtc. That should never actually happen in response to any userspace request as the zeroed mode would get rejected earlier. However there is some chance of this happening internally (eg. during hw state readout) so it seems best to not let the state become totally inconsistent.

Additionally the early return will not be taken if we're trying to disable an already disabled crtc. While that is not actually harmful it is inconsistent, so let's handle that case as well.

Testcase: igt/kms_selftest/check_atomic_set_mode_for_crtc Testcase: igt/kms_selftest/check_atomic_set_zeroed_mode_fort_crtc Reviewed-by: Daniel Vetter daniel@ffwll.ch Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com

---

drivers/gpu/drm/drm_atomic_uapi.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c index 0d466d3b0809..a3a6a8137af4 100644 --- a/drivers/gpu/drm/drm_atomic_uapi.c +++ b/drivers/gpu/drm/drm_atomic_uapi.c @@ -68,8 +68,13 @@ int drm_atomic_set_mode_for_crtc(struct drm_crtc_state *state, struct drm_mode_modeinfo umode;

/* Early return for no change. */

• if (mode && memcmp(&state->mode, mode, sizeof(*mode)) == 0)

• return 0;
  

• if (state->enable) {

Hm I think this would be clearer if you go with

if (state->enable == !!mode && (!mode || memcmp(&state->mode, mode, sizeof(*mode)) == 0)) return 0;

But also somewhat a bikeshed. I'm also wondering whether we shouldn't just declare this a driver bug (it means enable and mode are already out of sync), but I guess hw state readout is special sometimes.

Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch

• if (mode && memcmp(&state->mode, mode, sizeof(*mode)) == 0)
  

• 	return 0;
  

• } else {

• if (!mode)
  

• 	return 0;
  

• }

  drm_property_blob_put(state->mode_blob); state->mode_blob = NULL;

-- 2.23.0

On Fri, Nov 15, 2019 at 09:42:04PM +0200, Ville Syrjala wrote:

From: Ville Syrjälä ville.syrjala@linux.intel.com

Currently setplane grabs all modeset locks, which seems a bit excessive. Let's reduce that to just the locks we really need on atomic drivers. For non-atomic drivers let's stick to the current scheme for now.

Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com

drivers/gpu/drm/drm_plane.c | 56 +++++++++++++++++++------------------ 1 file changed, 29 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c index ef0cc33b43ce..e40d8a93294b 100644 --- a/drivers/gpu/drm/drm_plane.c +++ b/drivers/gpu/drm/drm_plane.c @@ -680,10 +680,14 @@ static int __setplane_internal(struct drm_plane *plane, uint32_t src_w, uint32_t src_h, struct drm_modeset_acquire_ctx *ctx) {

• int ret = 0;

• int ret;

  WARN_ON(drm_drv_uses_atomic_modeset(plane->dev));

• ret = drm_modeset_lock_all_ctx(plane->dev, ctx);

• if (ret)

• return ret;
  

There's pre-nv50 nouveau and shmob which have planes and aren't atomic (if I checked correctly), so we could probably forgo this, but also feels like totally not worth the audit trouble.

Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch

• /* No fb means shut it down */ if (!fb) { plane->old_fb = plane->fb;

@@ -767,32 +771,24 @@ static int setplane_internal(struct drm_plane *plane, uint32_t crtc_w, uint32_t crtc_h, /* src_{x,y,w,h} values are 16.16 fixed point */ uint32_t src_x, uint32_t src_y,

• 	     uint32_t src_w, uint32_t src_h)
  

• 	     uint32_t src_w, uint32_t src_h,
  

• 	     struct drm_modeset_acquire_ctx *ctx)
  

{

• struct drm_modeset_acquire_ctx ctx;
• int ret;
• DRM_MODESET_LOCK_ALL_BEGIN(plane->dev, ctx,

• 		   DRM_MODESET_ACQUIRE_INTERRUPTIBLE, ret);
  

• if (drm_drv_uses_atomic_modeset(plane->dev))

• ret = __setplane_atomic(plane, crtc, fb,
  

• 			crtc_x, crtc_y, crtc_w, crtc_h,
  

• 			src_x, src_y, src_w, src_h, &ctx);
  

• return __setplane_atomic(plane, crtc, fb,
  

• 			 crtc_x, crtc_y, crtc_w, crtc_h,
  

• 			 src_x, src_y, src_w, src_h, ctx);
  

  else

• ret = __setplane_internal(plane, crtc, fb,
  

• 			  crtc_x, crtc_y, crtc_w, crtc_h,
  

• 			  src_x, src_y, src_w, src_h, &ctx);
  

• DRM_MODESET_LOCK_ALL_END(ctx, ret);
• return ret;

• return __setplane_internal(plane, crtc, fb,
  

• 			   crtc_x, crtc_y, crtc_w, crtc_h,
  

• 			   src_x, src_y, src_w, src_h, ctx);
  

}

int drm_mode_setplane(struct drm_device *dev, void *data, struct drm_file *file_priv) { struct drm_mode_set_plane *plane_req = data;

• struct drm_modeset_acquire_ctx ctx; struct drm_plane *plane; struct drm_crtc *crtc = NULL; struct drm_framebuffer *fb = NULL;

@@ -829,11 +825,22 @@ int drm_mode_setplane(struct drm_device *dev, void *data, } }

• drm_modeset_acquire_init(&ctx, DRM_MODESET_ACQUIRE_INTERRUPTIBLE);

+retry: ret = setplane_internal(plane, crtc, fb, plane_req->crtc_x, plane_req->crtc_y, plane_req->crtc_w, plane_req->crtc_h, plane_req->src_x, plane_req->src_y,

• 		plane_req->src_w, plane_req->src_h);
  

• 		plane_req->src_w, plane_req->src_h, &ctx);
  

• if (ret == -EDEADLK) {

• ret = drm_modeset_backoff(&ctx);
  

• if (!ret)
  

• 	goto retry;
  

• }

• drm_modeset_drop_locks(&ctx);

• drm_modeset_acquire_fini(&ctx);

  if (fb) drm_framebuffer_put(fb);

@@ -907,14 +914,9 @@ static int drm_mode_cursor_universal(struct drm_crtc *crtc, src_h = fb->height << 16; }

• if (drm_drv_uses_atomic_modeset(dev))

• ret = __setplane_atomic(plane, crtc, fb,
  

• 			crtc_x, crtc_y, crtc_w, crtc_h,
  

• 			0, 0, src_w, src_h, ctx);
  

• else

• ret = __setplane_internal(plane, crtc, fb,
  

• 			  crtc_x, crtc_y, crtc_w, crtc_h,
  

• 			  0, 0, src_w, src_h, ctx);
  

• ret = setplane_internal(plane, crtc, fb,

• 		crtc_x, crtc_y, crtc_w, crtc_h,
  

• 		0, 0, src_w, src_h, ctx);
  
  

  if (fb) drm_framebuffer_put(fb);

-- 2.23.0

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel