[PATCH 1/3] drm: Add the optional .fb_modifier() hook

[PATCH 1/3] drm: Add the optional .fb_modifier() hook

[Ville Syrjala]

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

To make it possible for the core to check the fb pixel format and
modifier, we need to first ask the driver to deduce the modifier
when the request does not explicitly specify one.

Add a new .fb_modifier() hook for that purpose and convert i915
and vc4 to make use if it. All other drivers seem to currently
assume linear when the request does not specify anything else,
hence we make the core use that as the fallback strategy when
the hook is not provided.

Since the two hooks are now separate it is of course possible
to race set_tiling against addfb. But since that's just userspace
intentially shooting itself in the foot I don't think we should
hve to worry about it. The addfb will simply fail in the
.fb_create() hook if the tiling has changed since
.fb_mofifier() was called. Well, that's the case for i915.
vc4 never did any cross checks between the tiling and modifier.

framebuffer_check() now checks the request against the deduced
modifier, and since we now know the final modifier we can
convert the request to one with modifiers. This means that a
driver will never even have to look at a request without
modifiers outside of the .fb_modifuer() hook, should it need
to provide one.

Cc: Eric Anholt <eric@anholt.net>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_framebuffer.c    | 61 +++++++++++++++++++++--------
 drivers/gpu/drm/i915/intel_display.c | 74 ++++++++++++++++++++++++------------
 drivers/gpu/drm/i915/intel_drv.h     |  2 +-
 drivers/gpu/drm/i915/intel_fbdev.c   |  2 +
 drivers/gpu/drm/vc4/vc4_kms.c        | 57 +++++++++++----------------
 include/drm/drm_mode_config.h        | 29 ++++++++++++++
 6 files changed, 150 insertions(+), 75 deletions(-)

diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
index 7df025669067..21d3d51eb261 100644
--- a/drivers/gpu/drm/drm_framebuffer.c
+++ b/drivers/gpu/drm/drm_framebuffer.c
@@ -153,7 +153,8 @@ static int fb_plane_height(int height,
 }
 
 static int framebuffer_check(struct drm_device *dev,
-			     const struct drm_mode_fb_cmd2 *r)
+			     struct drm_mode_fb_cmd2 *r,
+			     u64 modifier)
 {
 	const struct drm_format_info *info;
 	int i;
@@ -210,14 +211,14 @@ static int framebuffer_check(struct drm_device *dev,
 		}
 
 		if (r->flags & DRM_MODE_FB_MODIFIERS &&
-		    r->modifier[i] != r->modifier[0]) {
+		    r->modifier[i] != modifier) {
 			DRM_DEBUG_KMS("bad fb modifier %llu for plane %d\n",
 				      r->modifier[i], i);
 			return -EINVAL;
 		}
 
 		/* modifier specific checks: */
-		switch (r->modifier[i]) {
+		switch (modifier) {
 		case DRM_FORMAT_MOD_SAMSUNG_64_32_TILE:
 			/* NOTE: the pitch restriction may be lifted later if it turns
 			 * out that no hw has this restriction:
@@ -261,45 +262,73 @@ static int framebuffer_check(struct drm_device *dev,
 		}
 	}
 
+	/*
+	 * Finally convert the request into one with
+	 * modifiers to make life easier for drivers.
+	 */
+	if (dev->mode_config.allow_fb_modifiers) {
+		r->flags |= DRM_MODE_FB_MODIFIERS;
+
+		for (i = 0; i < info->num_planes; i++)
+			r->modifier[i] = modifier;
+	}
+
 	return 0;
 }
 
 struct drm_framebuffer *
 drm_internal_framebuffer_create(struct drm_device *dev,
-				const struct drm_mode_fb_cmd2 *r,
+				const struct drm_mode_fb_cmd2 *user_r,
 				struct drm_file *file_priv)
 {
 	struct drm_mode_config *config = &dev->mode_config;
+	struct drm_mode_fb_cmd2 r = *user_r;
 	struct drm_framebuffer *fb;
+	u64 modifier = 0;
 	int ret;
 
-	if (r->flags & ~(DRM_MODE_FB_INTERLACED | DRM_MODE_FB_MODIFIERS)) {
-		DRM_DEBUG_KMS("bad framebuffer flags 0x%08x\n", r->flags);
+	if (r.flags & ~(DRM_MODE_FB_INTERLACED | DRM_MODE_FB_MODIFIERS)) {
+		DRM_DEBUG_KMS("bad framebuffer flags 0x%08x\n", r.flags);
 		return ERR_PTR(-EINVAL);
 	}
 
-	if ((config->min_width > r->width) || (r->width > config->max_width)) {
+	if (config->min_width > r.width || r.width > config->max_width) {
 		DRM_DEBUG_KMS("bad framebuffer width %d, should be >= %d && <= %d\n",
-			  r->width, config->min_width, config->max_width);
+			  r.width, config->min_width, config->max_width);
 		return ERR_PTR(-EINVAL);
 	}
-	if ((config->min_height > r->height) || (r->height > config->max_height)) {
+	if (config->min_height > r.height || r.height > config->max_height) {
 		DRM_DEBUG_KMS("bad framebuffer height %d, should be >= %d && <= %d\n",
-			  r->height, config->min_height, config->max_height);
+			  r.height, config->min_height, config->max_height);
 		return ERR_PTR(-EINVAL);
 	}
 
-	if (r->flags & DRM_MODE_FB_MODIFIERS &&
-	    !dev->mode_config.allow_fb_modifiers) {
-		DRM_DEBUG_KMS("driver does not support fb modifiers\n");
-		return ERR_PTR(-EINVAL);
+	if (r.flags & DRM_MODE_FB_MODIFIERS) {
+		if (!dev->mode_config.allow_fb_modifiers) {
+			DRM_DEBUG_KMS("driver does not support fb modifiers\n");
+			return ERR_PTR(-EINVAL);
+		}
+
+		modifier = r.modifier[0];
+	} else {
+		if (dev->mode_config.funcs->fb_modifier) {
+			ret = dev->mode_config.funcs->fb_modifier(dev, file_priv,
+								  &r, &modifier);
+			if (ret) {
+				DRM_DEBUG_KMS("driver did not deduce the fb modifier\n");
+				return ERR_PTR(ret);
+			}
+		} else {
+			/* assume linear if the driver doesn't say otherwise */
+			modifier = DRM_FORMAT_MOD_LINEAR;
+		}
 	}
 
-	ret = framebuffer_check(dev, r);
+	ret = framebuffer_check(dev, &r, modifier);
 	if (ret)
 		return ERR_PTR(ret);
 
-	fb = dev->mode_config.funcs->fb_create(dev, file_priv, r);
+	fb = dev->mode_config.funcs->fb_create(dev, file_priv, &r);
 	if (IS_ERR(fb)) {
 		DRM_DEBUG_KMS("could not create framebuffer\n");
 		return fb;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 2933ad38094f..51d8d0c40674 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -123,7 +123,7 @@ static void ironlake_pch_clock_get(struct intel_crtc *crtc,
 
 static int intel_framebuffer_init(struct intel_framebuffer *ifb,
 				  struct drm_i915_gem_object *obj,
-				  struct drm_mode_fb_cmd2 *mode_cmd);
+				  const struct drm_mode_fb_cmd2 *mode_cmd);
 static void i9xx_set_pipeconf(struct intel_crtc *intel_crtc);
 static void intel_set_pipe_timings(struct intel_crtc *intel_crtc);
 static void intel_set_pipe_src_size(struct intel_crtc *intel_crtc);
@@ -9807,7 +9807,7 @@ static const struct drm_display_mode load_detect_mode = {
 
 struct drm_framebuffer *
 intel_framebuffer_create(struct drm_i915_gem_object *obj,
-			 struct drm_mode_fb_cmd2 *mode_cmd)
+			 const struct drm_mode_fb_cmd2 *mode_cmd)
 {
 	struct intel_framebuffer *intel_fb;
 	int ret;
@@ -13985,7 +13985,7 @@ u32 intel_fb_pitch_limit(struct drm_i915_private *dev_priv,
 
 static int intel_framebuffer_init(struct intel_framebuffer *intel_fb,
 				  struct drm_i915_gem_object *obj,
-				  struct drm_mode_fb_cmd2 *mode_cmd)
+				  const struct drm_mode_fb_cmd2 *mode_cmd)
 {
 	struct drm_i915_private *dev_priv = to_i915(obj->base.dev);
 	struct drm_framebuffer *fb = &intel_fb->base;
@@ -13995,29 +13995,23 @@ static int intel_framebuffer_init(struct intel_framebuffer *intel_fb,
 	int ret = -EINVAL;
 	int i;
 
+	if (WARN_ON((mode_cmd->flags & DRM_MODE_FB_MODIFIERS) == 0))
+		return -EINVAL;
+
 	i915_gem_object_lock(obj);
 	obj->framebuffer_references++;
 	tiling = i915_gem_object_get_tiling(obj);
 	stride = i915_gem_object_get_stride(obj);
 	i915_gem_object_unlock(obj);
 
-	if (mode_cmd->flags & DRM_MODE_FB_MODIFIERS) {
-		/*
-		 * If there's a fence, enforce that
-		 * the fb modifier and tiling mode match.
-		 */
-		if (tiling != I915_TILING_NONE &&
-		    tiling != intel_fb_modifier_to_tiling(mode_cmd->modifier[0])) {
-			DRM_DEBUG_KMS("tiling_mode doesn't match fb modifier\n");
-			goto err;
-		}
-	} else {
-		if (tiling == I915_TILING_X) {
-			mode_cmd->modifier[0] = I915_FORMAT_MOD_X_TILED;
-		} else if (tiling == I915_TILING_Y) {
-			DRM_DEBUG_KMS("No Y tiling for legacy addfb\n");
-			goto err;
-		}
+	/*
+	 * If there's a fence, enforce that
+	 * the fb modifier and tiling mode match.
+	 */
+	if (tiling != I915_TILING_NONE &&
+	    tiling != intel_fb_modifier_to_tiling(mode_cmd->modifier[0])) {
+		DRM_DEBUG_KMS("tiling_mode doesn't match fb modifier\n");
+		goto err;
 	}
 
 	/* Passed in modifier sanity checking. */
@@ -14193,20 +14187,51 @@ static int intel_framebuffer_init(struct intel_framebuffer *intel_fb,
 	return ret;
 }
 
+static int
+intel_user_framebuffer_modifier(struct drm_device *dev,
+				struct drm_file *filp,
+				const struct drm_mode_fb_cmd2 *mode_cmd,
+				u64 *modifier)
+{
+	struct drm_i915_gem_object *obj;
+	unsigned int tiling;
+
+	obj = i915_gem_object_lookup(filp, mode_cmd->handles[0]);
+	if (!obj)
+		return -ENOENT;
+
+	i915_gem_object_lock(obj);
+	tiling = i915_gem_object_get_tiling(obj);
+	i915_gem_object_unlock(obj);
+
+	i915_gem_object_put(obj);
+
+	if (tiling == I915_TILING_Y) {
+		DRM_DEBUG_KMS("No Y tiling for legacy addfb\n");
+		return -EINVAL;
+	}
+
+	if (tiling == I915_TILING_X)
+		*modifier = I915_FORMAT_MOD_X_TILED;
+	else
+		*modifier = DRM_FORMAT_MOD_LINEAR;
+
+	return 0;
+}
+
 static struct drm_framebuffer *
 intel_user_framebuffer_create(struct drm_device *dev,
 			      struct drm_file *filp,
-			      const struct drm_mode_fb_cmd2 *user_mode_cmd)
+			      const struct drm_mode_fb_cmd2 *mode_cmd)
 {
 	struct drm_framebuffer *fb;
 	struct drm_i915_gem_object *obj;
-	struct drm_mode_fb_cmd2 mode_cmd = *user_mode_cmd;
 
-	obj = i915_gem_object_lookup(filp, mode_cmd.handles[0]);
+	obj = i915_gem_object_lookup(filp, mode_cmd->handles[0]);
 	if (!obj)
 		return ERR_PTR(-ENOENT);
 
-	fb = intel_framebuffer_create(obj, &mode_cmd);
+	fb = intel_framebuffer_create(obj, mode_cmd);
 	if (IS_ERR(fb))
 		i915_gem_object_put(obj);
 
@@ -14251,6 +14276,7 @@ intel_mode_valid(struct drm_device *dev,
 }
 
 static const struct drm_mode_config_funcs intel_mode_funcs = {
+	.fb_modifier = intel_user_framebuffer_modifier,
 	.fb_create = intel_user_framebuffer_create,
 	.get_format_info = intel_get_format_info,
 	.output_poll_changed = intel_fbdev_output_poll_changed,
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index fce5d3072d97..678755297814 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1517,7 +1517,7 @@ intel_pin_and_fence_fb_obj(struct drm_framebuffer *fb,
 void intel_unpin_fb_vma(struct i915_vma *vma, unsigned long flags);
 struct drm_framebuffer *
 intel_framebuffer_create(struct drm_i915_gem_object *obj,
-			 struct drm_mode_fb_cmd2 *mode_cmd);
+			 const struct drm_mode_fb_cmd2 *mode_cmd);
 int intel_prepare_plane_fb(struct drm_plane *plane,
 			   struct drm_plane_state *new_state);
 void intel_cleanup_plane_fb(struct drm_plane *plane,
diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 6f12adc06365..777f5bfbe8ef 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -131,6 +131,8 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
 				    DIV_ROUND_UP(sizes->surface_bpp, 8), 64);
 	mode_cmd.pixel_format = drm_mode_legacy_fb_format(sizes->surface_bpp,
 							  sizes->surface_depth);
+	mode_cmd.modifier[0] = DRM_FORMAT_MOD_LINEAR;
+	mode_cmd.flags = DRM_MODE_FB_MODIFIERS;
 
 	size = mode_cmd.pitches[0] * mode_cmd.height;
 	size = PAGE_ALIGN(size);
diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
index ba60153dddb5..a8aed12db578 100644
--- a/drivers/gpu/drm/vc4/vc4_kms.c
+++ b/drivers/gpu/drm/vc4/vc4_kms.c
@@ -148,50 +148,39 @@ static int vc4_atomic_commit(struct drm_device *dev,
 	return 0;
 }
 
-static struct drm_framebuffer *vc4_fb_create(struct drm_device *dev,
-					     struct drm_file *file_priv,
-					     const struct drm_mode_fb_cmd2 *mode_cmd)
+static int vc4_fb_modifier(struct drm_device *dev,
+			   struct drm_file *file_priv,
+			   const struct drm_mode_fb_cmd2 *mode_cmd,
+			   u64 *modifier)
 {
-	struct drm_mode_fb_cmd2 mode_cmd_local;
-
-	/* If the user didn't specify a modifier, use the
-	 * vc4_set_tiling_ioctl() state for the BO.
-	 */
-	if (!(mode_cmd->flags & DRM_MODE_FB_MODIFIERS)) {
-		struct drm_gem_object *gem_obj;
-		struct vc4_bo *bo;
-
-		gem_obj = drm_gem_object_lookup(file_priv,
-						mode_cmd->handles[0]);
-		if (!gem_obj) {
-			DRM_DEBUG("Failed to look up GEM BO %d\n",
-				  mode_cmd->handles[0]);
-			return ERR_PTR(-ENOENT);
-		}
-		bo = to_vc4_bo(gem_obj);
-
-		mode_cmd_local = *mode_cmd;
-
-		if (bo->t_format) {
-			mode_cmd_local.modifier[0] =
-				DRM_FORMAT_MOD_BROADCOM_VC4_T_TILED;
-		} else {
-			mode_cmd_local.modifier[0] = DRM_FORMAT_MOD_NONE;
-		}
+	struct drm_gem_object *gem_obj;
+	struct vc4_bo *bo;
+
+	gem_obj = drm_gem_object_lookup(file_priv,
+					mode_cmd->handles[0]);
+	if (!gem_obj) {
+		DRM_DEBUG("Failed to look up GEM BO %d\n",
+			  mode_cmd->handles[0]);
+		return -ENOENT;
+	}
+	bo = to_vc4_bo(gem_obj);
 
-		drm_gem_object_put_unlocked(gem_obj);
+	if (bo->t_format)
+		*modifier = DRM_FORMAT_MOD_BROADCOM_VC4_T_TILED;
+	else
+		*modifier = DRM_FORMAT_MOD_LINEAR;
 
-		mode_cmd = &mode_cmd_local;
-	}
+	drm_gem_object_put_unlocked(gem_obj);
 
-	return drm_gem_fb_create(dev, file_priv, mode_cmd);
+	return 0;
 }
 
 static const struct drm_mode_config_funcs vc4_mode_funcs = {
 	.output_poll_changed = drm_fb_helper_output_poll_changed,
 	.atomic_check = drm_atomic_helper_check,
 	.atomic_commit = vc4_atomic_commit,
-	.fb_create = vc4_fb_create,
+	.fb_modifier = vc4_fb_modifier,
+	.fb_create = drm_gem_fb_create,
 };
 
 int vc4_kms_load(struct drm_device *dev)
diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
index 7569f22ffef6..c74aed292b58 100644
--- a/include/drm/drm_mode_config.h
+++ b/include/drm/drm_mode_config.h
@@ -45,6 +45,30 @@ struct drm_display_mode;
  * involve drivers.
  */
 struct drm_mode_config_funcs {
+	/**
+	 * @fb_modifier:
+	 *
+	 * Deduce the format modifier using a driver specific method. This
+	 * gets called when the request does not explicitly state the
+	 * modifier, ie. (@mode_cmd->flags & DRM_MODE_FB_MODIFIERS) == 0.
+	 * The deduced modifier is returned via @modifier.
+	 *
+	 * Once the modifier is known several sanity checks will be performed
+	 * and if all is deemed valid @fb_create() will be called to have
+	 * the driver to actually create the framebuffer.
+	 *
+	 * This hook is optional. The core will assume DRM_FORMAT_MOD_LINEAR
+	 * if the hook is not provided by the driver.
+	 *
+	 * RETURNS:
+	 *
+	 * Zero on success, negative error code on failure.
+	 */
+	int (*fb_modifier)(struct drm_device *dev,
+			   struct drm_file *file_priv,
+			   const struct drm_mode_fb_cmd2 *mode_cmd,
+			   u64 *modifier);
+
 	/**
 	 * @fb_create:
 	 *
@@ -52,6 +76,11 @@ struct drm_mode_config_funcs {
 	 * requested metadata, but most of that is left to the driver. See
 	 * &struct drm_mode_fb_cmd2 for details.
 	 *
+	 * Note that for drivers that support modifiers, the core will convert
+	 * all requests to one with modifiers. So the driver does not need to
+	 * worry about requests without modifiers (apart from having to provide
+	 * the @fb_modifier() hook if necessary).
+	 *
 	 * If the parameters are deemed valid and the backing storage objects in
 	 * the underlying memory manager all exist, then the driver allocates
 	 * a new &drm_framebuffer structure, subclassed to contain
-- 
2.16.1

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

[PATCH 2/3] drm: Make sure at least one plane supports the fb format

[Ville Syrjala]

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

To make life easier for drivers, let's have the core check that the
requested pixel format is supported by at least one plane when creating
a new framebuffer.

This eases the burden on drivers by making sure they'll never get
requests to create fbs with unsupported pixel formats. Thanks to the
new .fb_modifier() hook this check can now be done whether the request
specifies the modifier directly or driver has to deduce it from the
gem bo tiling (or via any other method really).

v0: Accept anyformat if the driver doesn't do planes (Eric)
    s/planes_have_format/any_plane_has_format/ (Eric)
    Check the modifier as well since we already have a function
    that does both
v3: Don't do the check in the core since we may not know the
    modifier yet, instead export the function and let drivers
    call it themselves
v4: Unexport the functiona and put the format_default check back
    since this will again be called by the core, ie. undo v3 ;)

Cc: Eric Anholt <eric@anholt.net>
Testcase: igt/kms_addfb_basic/expected-formats
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_framebuffer.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
index 21d3d51eb261..e618a6b728d4 100644
--- a/drivers/gpu/drm/drm_framebuffer.c
+++ b/drivers/gpu/drm/drm_framebuffer.c
@@ -152,6 +152,26 @@ static int fb_plane_height(int height,
 	return DIV_ROUND_UP(height, format->vsub);
 }
 
+static bool any_plane_has_format(struct drm_device *dev,
+				 u32 format, u64 modifier)
+{
+	struct drm_plane *plane;
+
+	drm_for_each_plane(plane, dev) {
+		/*
+		 * In case the driver doesn't really do
+		 * planes we have to accept any format here.
+		 */
+		if (plane->format_default)
+			return true;
+
+		if (drm_plane_check_pixel_format(plane, format, modifier) == 0)
+			return true;
+	}
+
+	return false;
+}
+
 static int framebuffer_check(struct drm_device *dev,
 			     struct drm_mode_fb_cmd2 *r,
 			     u64 modifier)
@@ -183,6 +203,16 @@ static int framebuffer_check(struct drm_device *dev,
 		return -EINVAL;
 	}
 
+	if (!any_plane_has_format(dev, r->pixel_format, modifier)) {
+		struct drm_format_name_buf format_name;
+
+		DRM_DEBUG_KMS("unsupported pixel format %s / modifier 0x%llx\n",
+			      drm_get_format_name(r->pixel_format,
+						  &format_name),
+			      modifier);
+		return -EINVAL;
+	}
+
 	for (i = 0; i < info->num_planes; i++) {
 		unsigned int width = fb_plane_width(r->width, info, i);
 		unsigned int height = fb_plane_height(r->height, info, i);
-- 
2.16.1

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

[PATCH 3/3] drm/i915: Eliminate the horrendous format check code

[Ville Syrjala]

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

Now that the core makes sure that the pixel format is supported
by at least one plane, we can eliminate the hand rolled code for the
same thing in i915. The way we were doing was extremely inconvenient
because not only did you have to add the format to the plane's format
list, but you also had to come up with the correct platform checks
for this code.

v2: Nuke the modifier checks as well since the core does that too now
v3: Call drm_any_plane_has_format() from the driver code
v4: Go back to letting the core do everything

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 86 ------------------------------------
 1 file changed, 86 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 51d8d0c40674..028a791889c6 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13989,7 +13989,6 @@ static int intel_framebuffer_init(struct intel_framebuffer *intel_fb,
 {
 	struct drm_i915_private *dev_priv = to_i915(obj->base.dev);
 	struct drm_framebuffer *fb = &intel_fb->base;
-	struct drm_format_name_buf format_name;
 	u32 pitch_limit;
 	unsigned int tiling, stride;
 	int ret = -EINVAL;
@@ -14014,37 +14013,6 @@ static int intel_framebuffer_init(struct intel_framebuffer *intel_fb,
 		goto err;
 	}
 
-	/* Passed in modifier sanity checking. */
-	switch (mode_cmd->modifier[0]) {
-	case I915_FORMAT_MOD_Y_TILED_CCS:
-	case I915_FORMAT_MOD_Yf_TILED_CCS:
-		switch (mode_cmd->pixel_format) {
-		case DRM_FORMAT_XBGR8888:
-		case DRM_FORMAT_ABGR8888:
-		case DRM_FORMAT_XRGB8888:
-		case DRM_FORMAT_ARGB8888:
-			break;
-		default:
-			DRM_DEBUG_KMS("RC supported only with RGB8888 formats\n");
-			goto err;
-		}
-		/* fall through */
-	case I915_FORMAT_MOD_Y_TILED:
-	case I915_FORMAT_MOD_Yf_TILED:
-		if (INTEL_GEN(dev_priv) < 9) {
-			DRM_DEBUG_KMS("Unsupported tiling 0x%llx!\n",
-				      mode_cmd->modifier[0]);
-			goto err;
-		}
-	case DRM_FORMAT_MOD_LINEAR:
-	case I915_FORMAT_MOD_X_TILED:
-		break;
-	default:
-		DRM_DEBUG_KMS("Unsupported fb modifier 0x%llx!\n",
-			      mode_cmd->modifier[0]);
-		goto err;
-	}
-
 	/*
 	 * gen2/3 display engine uses the fence if present,
 	 * so the tiling mode must match the fb modifier exactly.
@@ -14075,60 +14043,6 @@ static int intel_framebuffer_init(struct intel_framebuffer *intel_fb,
 		goto err;
 	}
 
-	/* Reject formats not supported by any plane early. */
-	switch (mode_cmd->pixel_format) {
-	case DRM_FORMAT_C8:
-	case DRM_FORMAT_RGB565:
-	case DRM_FORMAT_XRGB8888:
-	case DRM_FORMAT_ARGB8888:
-		break;
-	case DRM_FORMAT_XRGB1555:
-		if (INTEL_GEN(dev_priv) > 3) {
-			DRM_DEBUG_KMS("unsupported pixel format: %s\n",
-				      drm_get_format_name(mode_cmd->pixel_format, &format_name));
-			goto err;
-		}
-		break;
-	case DRM_FORMAT_ABGR8888:
-		if (!IS_VALLEYVIEW(dev_priv) && !IS_CHERRYVIEW(dev_priv) &&
-		    INTEL_GEN(dev_priv) < 9) {
-			DRM_DEBUG_KMS("unsupported pixel format: %s\n",
-				      drm_get_format_name(mode_cmd->pixel_format, &format_name));
-			goto err;
-		}
-		break;
-	case DRM_FORMAT_XBGR8888:
-	case DRM_FORMAT_XRGB2101010:
-	case DRM_FORMAT_XBGR2101010:
-		if (INTEL_GEN(dev_priv) < 4) {
-			DRM_DEBUG_KMS("unsupported pixel format: %s\n",
-				      drm_get_format_name(mode_cmd->pixel_format, &format_name));
-			goto err;
-		}
-		break;
-	case DRM_FORMAT_ABGR2101010:
-		if (!IS_VALLEYVIEW(dev_priv) && !IS_CHERRYVIEW(dev_priv)) {
-			DRM_DEBUG_KMS("unsupported pixel format: %s\n",
-				      drm_get_format_name(mode_cmd->pixel_format, &format_name));
-			goto err;
-		}
-		break;
-	case DRM_FORMAT_YUYV:
-	case DRM_FORMAT_UYVY:
-	case DRM_FORMAT_YVYU:
-	case DRM_FORMAT_VYUY:
-		if (INTEL_GEN(dev_priv) < 5 && !IS_G4X(dev_priv)) {
-			DRM_DEBUG_KMS("unsupported pixel format: %s\n",
-				      drm_get_format_name(mode_cmd->pixel_format, &format_name));
-			goto err;
-		}
-		break;
-	default:
-		DRM_DEBUG_KMS("unsupported pixel format: %s\n",
-			      drm_get_format_name(mode_cmd->pixel_format, &format_name));
-		goto err;
-	}
-
 	/* FIXME need to adjust LINOFF/TILEOFF accordingly. */
 	if (mode_cmd->offsets[0] != 0)
 		goto err;
-- 
2.16.1

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

✗ Fi.CI.BAT: failure for series starting with [1/3] drm: Add the optional .fb_modifier() hook

[Patchwork]

== Series Details ==

Series: series starting with [1/3] drm: Add the optional .fb_modifier() hook
URL   : https://patchwork.freedesktop.org/series/39813/
State : failure

== Summary ==

Series 39813v1 series starting with [1/3] drm: Add the optional .fb_modifier() hook
https://patchwork.freedesktop.org/api/1.0/series/39813/revisions/1/mbox/

---- Possible new issues:

Test kms_addfb_basic:
        Subgroup no-handle:
                pass       -> FAIL       (fi-bdw-5557u)
                pass       -> FAIL       (fi-bdw-gvtdvm)
                pass       -> FAIL       (fi-blb-e6850)
                pass       -> FAIL       (fi-bsw-n3050)
                pass       -> FAIL       (fi-bwr-2160)
                pass       -> FAIL       (fi-bxt-dsi)
                pass       -> FAIL       (fi-bxt-j4205)
                pass       -> FAIL       (fi-byt-j1900)
                pass       -> FAIL       (fi-byt-n2820)
                pass       -> FAIL       (fi-cfl-8700k)
                pass       -> FAIL       (fi-cfl-s2)
                pass       -> FAIL       (fi-elk-e7500)
                pass       -> FAIL       (fi-gdg-551)
                pass       -> FAIL       (fi-glk-1)
                pass       -> FAIL       (fi-hsw-4770)
                pass       -> FAIL       (fi-ilk-650)
                pass       -> FAIL       (fi-ivb-3520m)
                pass       -> FAIL       (fi-ivb-3770)
                pass       -> FAIL       (fi-kbl-7500u)
                pass       -> FAIL       (fi-kbl-7567u)
                pass       -> FAIL       (fi-kbl-r)
                pass       -> FAIL       (fi-pnv-d510)
                pass       -> FAIL       (fi-skl-6260u)
                pass       -> FAIL       (fi-skl-6600u)
                pass       -> FAIL       (fi-skl-6700hq)
                pass       -> FAIL       (fi-skl-6700k2)
                pass       -> FAIL       (fi-skl-6770hq)
                pass       -> FAIL       (fi-skl-guc)
                pass       -> FAIL       (fi-skl-gvtdvm)
                pass       -> FAIL       (fi-snb-2520m)
                pass       -> FAIL       (fi-snb-2600)

---- Known issues:

Test gem_mmap_gtt:
        Subgroup basic-small-bo-tiledx:
                fail       -> PASS       (fi-gdg-551) fdo#102575
Test kms_chamelium:
        Subgroup hdmi-hpd-fast:
                skip       -> FAIL       (fi-kbl-7500u) fdo#102672

fdo#102575 https://bugs.freedesktop.org/show_bug.cgi?id=102575
fdo#102672 https://bugs.freedesktop.org/show_bug.cgi?id=102672

fi-bdw-5557u     total:288  pass:266  dwarn:0   dfail:0   fail:1   skip:21  time:429s
fi-bdw-gvtdvm    total:288  pass:263  dwarn:0   dfail:0   fail:1   skip:24  time:433s
fi-blb-e6850     total:288  pass:222  dwarn:1   dfail:0   fail:1   skip:64  time:382s
fi-bsw-n3050     total:288  pass:241  dwarn:0   dfail:0   fail:1   skip:46  time:536s
fi-bwr-2160      total:288  pass:182  dwarn:0   dfail:0   fail:1   skip:105 time:300s
fi-bxt-dsi       total:288  pass:257  dwarn:0   dfail:0   fail:1   skip:30  time:507s
fi-bxt-j4205     total:288  pass:258  dwarn:0   dfail:0   fail:1   skip:29  time:510s
fi-byt-j1900     total:288  pass:252  dwarn:0   dfail:0   fail:1   skip:35  time:509s
fi-byt-n2820     total:288  pass:248  dwarn:0   dfail:0   fail:1   skip:39  time:499s
fi-cfl-8700k     total:288  pass:259  dwarn:0   dfail:0   fail:1   skip:28  time:412s
fi-cfl-s2        total:288  pass:261  dwarn:0   dfail:0   fail:1   skip:26  time:584s
fi-elk-e7500     total:288  pass:228  dwarn:0   dfail:0   fail:1   skip:59  time:427s
fi-gdg-551       total:288  pass:179  dwarn:0   dfail:0   fail:1   skip:108 time:314s
fi-glk-1         total:288  pass:259  dwarn:0   dfail:0   fail:1   skip:28  time:531s
fi-hsw-4770      total:288  pass:260  dwarn:0   dfail:0   fail:1   skip:27  time:408s
fi-ilk-650       total:288  pass:227  dwarn:0   dfail:0   fail:1   skip:60  time:423s
fi-ivb-3520m     total:288  pass:258  dwarn:0   dfail:0   fail:1   skip:29  time:486s
fi-ivb-3770      total:288  pass:254  dwarn:0   dfail:0   fail:1   skip:33  time:428s
fi-kbl-7500u     total:288  pass:262  dwarn:1   dfail:0   fail:2   skip:23  time:478s
fi-kbl-7567u     total:288  pass:267  dwarn:0   dfail:0   fail:1   skip:20  time:471s
fi-kbl-r         total:288  pass:260  dwarn:0   dfail:0   fail:1   skip:27  time:520s
fi-pnv-d510      total:288  pass:221  dwarn:1   dfail:0   fail:1   skip:65  time:645s
fi-skl-6260u     total:288  pass:267  dwarn:0   dfail:0   fail:1   skip:20  time:439s
fi-skl-6600u     total:288  pass:260  dwarn:0   dfail:0   fail:1   skip:27  time:529s
fi-skl-6700hq    total:288  pass:261  dwarn:0   dfail:0   fail:1   skip:26  time:539s
fi-skl-6700k2    total:288  pass:263  dwarn:0   dfail:0   fail:1   skip:24  time:507s
fi-skl-6770hq    total:288  pass:267  dwarn:0   dfail:0   fail:1   skip:20  time:492s
fi-skl-guc       total:288  pass:259  dwarn:0   dfail:0   fail:1   skip:28  time:424s
fi-skl-gvtdvm    total:288  pass:264  dwarn:0   dfail:0   fail:1   skip:23  time:440s
fi-snb-2520m     total:288  pass:247  dwarn:0   dfail:0   fail:1   skip:40  time:540s
fi-snb-2600      total:288  pass:247  dwarn:0   dfail:0   fail:1   skip:40  time:399s
Blacklisted hosts:
fi-cfl-u         total:288  pass:261  dwarn:0   dfail:0   fail:1   skip:26  time:509s
fi-cnl-drrs      total:288  pass:256  dwarn:3   dfail:0   fail:1   skip:28  time:518s
fi-glk-j5005 failed to collect. IGT log at Patchwork_8313/fi-glk-j5005/run0.log

1b5fa1f85d1cc65aec1229aeef67835af5d15864 drm-tip: 2018y-03m-12d-20h-30m-34s UTC integration manifest
e080c4bc0a11 drm/i915: Eliminate the horrendous format check code
0ce29dba9e89 drm: Make sure at least one plane supports the fb format
5f2580c91b7f drm: Add the optional .fb_modifier() hook

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8313/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 1/3] drm: Add the optional .fb_modifier() hook

[Ville Syrjala]

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

To make it possible for the core to check the fb pixel format and
modifier, we need to first ask the driver to deduce the modifier
when the request does not explicitly specify one.

Add a new .fb_modifier() hook for that purpose and convert i915
and vc4 to make use if it. All other drivers seem to currently
assume linear when the request does not specify anything else,
hence we make the core use that as the fallback strategy when
the hook is not provided.

Since the two hooks are now separate it is of course possible
to race set_tiling against addfb. But since that's just userspace
intentially shooting itself in the foot I don't think we should
hve to worry about it. The addfb will simply fail in the
.fb_create() hook if the tiling has changed since
.fb_mofifier() was called. Well, that's the case for i915.
vc4 never did any cross checks between the tiling and modifier.

framebuffer_check() now checks the request against the deduced
modifier, and since we now know the final modifier we can
convert the request to one with modifiers. This means that a
driver will never even have to look at a request without
modifiers outside of the .fb_modifuer() hook, should it need
to provide one.

v2: return -EINVAL for handles[0]==0 from i915/vc4 .fb_modifier() hook
    to keep igt/kms_addfb_basic/no-handle happy

Cc: Eric Anholt <eric@anholt.net>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_framebuffer.c    | 61 ++++++++++++++++++++--------
 drivers/gpu/drm/i915/intel_display.c | 79 +++++++++++++++++++++++++-----------
 drivers/gpu/drm/i915/intel_drv.h     |  2 +-
 drivers/gpu/drm/i915/intel_fbdev.c   |  2 +
 drivers/gpu/drm/vc4/vc4_kms.c        | 58 ++++++++++++--------------
 include/drm/drm_mode_config.h        | 29 +++++++++++++
 6 files changed, 158 insertions(+), 73 deletions(-)

diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
index 7df025669067..21d3d51eb261 100644
--- a/drivers/gpu/drm/drm_framebuffer.c
+++ b/drivers/gpu/drm/drm_framebuffer.c
@@ -153,7 +153,8 @@ static int fb_plane_height(int height,
 }
 
 static int framebuffer_check(struct drm_device *dev,
-			     const struct drm_mode_fb_cmd2 *r)
+			     struct drm_mode_fb_cmd2 *r,
+			     u64 modifier)
 {
 	const struct drm_format_info *info;
 	int i;
@@ -210,14 +211,14 @@ static int framebuffer_check(struct drm_device *dev,
 		}
 
 		if (r->flags & DRM_MODE_FB_MODIFIERS &&
-		    r->modifier[i] != r->modifier[0]) {
+		    r->modifier[i] != modifier) {
 			DRM_DEBUG_KMS("bad fb modifier %llu for plane %d\n",
 				      r->modifier[i], i);
 			return -EINVAL;
 		}
 
 		/* modifier specific checks: */
-		switch (r->modifier[i]) {
+		switch (modifier) {
 		case DRM_FORMAT_MOD_SAMSUNG_64_32_TILE:
 			/* NOTE: the pitch restriction may be lifted later if it turns
 			 * out that no hw has this restriction:
@@ -261,45 +262,73 @@ static int framebuffer_check(struct drm_device *dev,
 		}
 	}
 
+	/*
+	 * Finally convert the request into one with
+	 * modifiers to make life easier for drivers.
+	 */
+	if (dev->mode_config.allow_fb_modifiers) {
+		r->flags |= DRM_MODE_FB_MODIFIERS;
+
+		for (i = 0; i < info->num_planes; i++)
+			r->modifier[i] = modifier;
+	}
+
 	return 0;
 }
 
 struct drm_framebuffer *
 drm_internal_framebuffer_create(struct drm_device *dev,
-				const struct drm_mode_fb_cmd2 *r,
+				const struct drm_mode_fb_cmd2 *user_r,
 				struct drm_file *file_priv)
 {
 	struct drm_mode_config *config = &dev->mode_config;
+	struct drm_mode_fb_cmd2 r = *user_r;
 	struct drm_framebuffer *fb;
+	u64 modifier = 0;
 	int ret;
 
-	if (r->flags & ~(DRM_MODE_FB_INTERLACED | DRM_MODE_FB_MODIFIERS)) {
-		DRM_DEBUG_KMS("bad framebuffer flags 0x%08x\n", r->flags);
+	if (r.flags & ~(DRM_MODE_FB_INTERLACED | DRM_MODE_FB_MODIFIERS)) {
+		DRM_DEBUG_KMS("bad framebuffer flags 0x%08x\n", r.flags);
 		return ERR_PTR(-EINVAL);
 	}
 
-	if ((config->min_width > r->width) || (r->width > config->max_width)) {
+	if (config->min_width > r.width || r.width > config->max_width) {
 		DRM_DEBUG_KMS("bad framebuffer width %d, should be >= %d && <= %d\n",
-			  r->width, config->min_width, config->max_width);
+			  r.width, config->min_width, config->max_width);
 		return ERR_PTR(-EINVAL);
 	}
-	if ((config->min_height > r->height) || (r->height > config->max_height)) {
+	if (config->min_height > r.height || r.height > config->max_height) {
 		DRM_DEBUG_KMS("bad framebuffer height %d, should be >= %d && <= %d\n",
-			  r->height, config->min_height, config->max_height);
+			  r.height, config->min_height, config->max_height);
 		return ERR_PTR(-EINVAL);
 	}
 
-	if (r->flags & DRM_MODE_FB_MODIFIERS &&
-	    !dev->mode_config.allow_fb_modifiers) {
-		DRM_DEBUG_KMS("driver does not support fb modifiers\n");
-		return ERR_PTR(-EINVAL);
+	if (r.flags & DRM_MODE_FB_MODIFIERS) {
+		if (!dev->mode_config.allow_fb_modifiers) {
+			DRM_DEBUG_KMS("driver does not support fb modifiers\n");
+			return ERR_PTR(-EINVAL);
+		}
+
+		modifier = r.modifier[0];
+	} else {
+		if (dev->mode_config.funcs->fb_modifier) {
+			ret = dev->mode_config.funcs->fb_modifier(dev, file_priv,
+								  &r, &modifier);
+			if (ret) {
+				DRM_DEBUG_KMS("driver did not deduce the fb modifier\n");
+				return ERR_PTR(ret);
+			}
+		} else {
+			/* assume linear if the driver doesn't say otherwise */
+			modifier = DRM_FORMAT_MOD_LINEAR;
+		}
 	}
 
-	ret = framebuffer_check(dev, r);
+	ret = framebuffer_check(dev, &r, modifier);
 	if (ret)
 		return ERR_PTR(ret);
 
-	fb = dev->mode_config.funcs->fb_create(dev, file_priv, r);
+	fb = dev->mode_config.funcs->fb_create(dev, file_priv, &r);
 	if (IS_ERR(fb)) {
 		DRM_DEBUG_KMS("could not create framebuffer\n");
 		return fb;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 2933ad38094f..7ffe425b26ac 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -123,7 +123,7 @@ static void ironlake_pch_clock_get(struct intel_crtc *crtc,
 
 static int intel_framebuffer_init(struct intel_framebuffer *ifb,
 				  struct drm_i915_gem_object *obj,
-				  struct drm_mode_fb_cmd2 *mode_cmd);
+				  const struct drm_mode_fb_cmd2 *mode_cmd);
 static void i9xx_set_pipeconf(struct intel_crtc *intel_crtc);
 static void intel_set_pipe_timings(struct intel_crtc *intel_crtc);
 static void intel_set_pipe_src_size(struct intel_crtc *intel_crtc);
@@ -9807,7 +9807,7 @@ static const struct drm_display_mode load_detect_mode = {
 
 struct drm_framebuffer *
 intel_framebuffer_create(struct drm_i915_gem_object *obj,
-			 struct drm_mode_fb_cmd2 *mode_cmd)
+			 const struct drm_mode_fb_cmd2 *mode_cmd)
 {
 	struct intel_framebuffer *intel_fb;
 	int ret;
@@ -13985,7 +13985,7 @@ u32 intel_fb_pitch_limit(struct drm_i915_private *dev_priv,
 
 static int intel_framebuffer_init(struct intel_framebuffer *intel_fb,
 				  struct drm_i915_gem_object *obj,
-				  struct drm_mode_fb_cmd2 *mode_cmd)
+				  const struct drm_mode_fb_cmd2 *mode_cmd)
 {
 	struct drm_i915_private *dev_priv = to_i915(obj->base.dev);
 	struct drm_framebuffer *fb = &intel_fb->base;
@@ -13995,29 +13995,23 @@ static int intel_framebuffer_init(struct intel_framebuffer *intel_fb,
 	int ret = -EINVAL;
 	int i;
 
+	if (WARN_ON((mode_cmd->flags & DRM_MODE_FB_MODIFIERS) == 0))
+		return -EINVAL;
+
 	i915_gem_object_lock(obj);
 	obj->framebuffer_references++;
 	tiling = i915_gem_object_get_tiling(obj);
 	stride = i915_gem_object_get_stride(obj);
 	i915_gem_object_unlock(obj);
 
-	if (mode_cmd->flags & DRM_MODE_FB_MODIFIERS) {
-		/*
-		 * If there's a fence, enforce that
-		 * the fb modifier and tiling mode match.
-		 */
-		if (tiling != I915_TILING_NONE &&
-		    tiling != intel_fb_modifier_to_tiling(mode_cmd->modifier[0])) {
-			DRM_DEBUG_KMS("tiling_mode doesn't match fb modifier\n");
-			goto err;
-		}
-	} else {
-		if (tiling == I915_TILING_X) {
-			mode_cmd->modifier[0] = I915_FORMAT_MOD_X_TILED;
-		} else if (tiling == I915_TILING_Y) {
-			DRM_DEBUG_KMS("No Y tiling for legacy addfb\n");
-			goto err;
-		}
+	/*
+	 * If there's a fence, enforce that
+	 * the fb modifier and tiling mode match.
+	 */
+	if (tiling != I915_TILING_NONE &&
+	    tiling != intel_fb_modifier_to_tiling(mode_cmd->modifier[0])) {
+		DRM_DEBUG_KMS("tiling_mode doesn't match fb modifier\n");
+		goto err;
 	}
 
 	/* Passed in modifier sanity checking. */
@@ -14193,20 +14187,56 @@ static int intel_framebuffer_init(struct intel_framebuffer *intel_fb,
 	return ret;
 }
 
+static int
+intel_user_framebuffer_modifier(struct drm_device *dev,
+				struct drm_file *filp,
+				const struct drm_mode_fb_cmd2 *mode_cmd,
+				u64 *modifier)
+{
+	struct drm_i915_gem_object *obj;
+	unsigned int tiling;
+
+	if (!mode_cmd->handles[0]) {
+		DRM_DEBUG_KMS("no buffer object handle for plane 0\n");
+		return -EINVAL;
+	}
+
+	obj = i915_gem_object_lookup(filp, mode_cmd->handles[0]);
+	if (!obj)
+		return -ENOENT;
+
+	i915_gem_object_lock(obj);
+	tiling = i915_gem_object_get_tiling(obj);
+	i915_gem_object_unlock(obj);
+
+	i915_gem_object_put(obj);
+
+	if (tiling == I915_TILING_Y) {
+		DRM_DEBUG_KMS("No Y tiling for legacy addfb\n");
+		return -EINVAL;
+	}
+
+	if (tiling == I915_TILING_X)
+		*modifier = I915_FORMAT_MOD_X_TILED;
+	else
+		*modifier = DRM_FORMAT_MOD_LINEAR;
+
+	return 0;
+}
+
 static struct drm_framebuffer *
 intel_user_framebuffer_create(struct drm_device *dev,
 			      struct drm_file *filp,
-			      const struct drm_mode_fb_cmd2 *user_mode_cmd)
+			      const struct drm_mode_fb_cmd2 *mode_cmd)
 {
 	struct drm_framebuffer *fb;
 	struct drm_i915_gem_object *obj;
-	struct drm_mode_fb_cmd2 mode_cmd = *user_mode_cmd;
 
-	obj = i915_gem_object_lookup(filp, mode_cmd.handles[0]);
+	obj = i915_gem_object_lookup(filp, mode_cmd->handles[0]);
 	if (!obj)
 		return ERR_PTR(-ENOENT);
 
-	fb = intel_framebuffer_create(obj, &mode_cmd);
+	fb = intel_framebuffer_create(obj, mode_cmd);
 	if (IS_ERR(fb))
 		i915_gem_object_put(obj);
 
@@ -14251,6 +14281,7 @@ intel_mode_valid(struct drm_device *dev,
 }
 
 static const struct drm_mode_config_funcs intel_mode_funcs = {
+	.fb_modifier = intel_user_framebuffer_modifier,
 	.fb_create = intel_user_framebuffer_create,
 	.get_format_info = intel_get_format_info,
 	.output_poll_changed = intel_fbdev_output_poll_changed,
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index fce5d3072d97..678755297814 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1517,7 +1517,7 @@ intel_pin_and_fence_fb_obj(struct drm_framebuffer *fb,
 void intel_unpin_fb_vma(struct i915_vma *vma, unsigned long flags);
 struct drm_framebuffer *
 intel_framebuffer_create(struct drm_i915_gem_object *obj,
-			 struct drm_mode_fb_cmd2 *mode_cmd);
+			 const struct drm_mode_fb_cmd2 *mode_cmd);
 int intel_prepare_plane_fb(struct drm_plane *plane,
 			   struct drm_plane_state *new_state);
 void intel_cleanup_plane_fb(struct drm_plane *plane,
diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 6f12adc06365..777f5bfbe8ef 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -131,6 +131,8 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
 				    DIV_ROUND_UP(sizes->surface_bpp, 8), 64);
 	mode_cmd.pixel_format = drm_mode_legacy_fb_format(sizes->surface_bpp,
 							  sizes->surface_depth);
+	mode_cmd.modifier[0] = DRM_FORMAT_MOD_LINEAR;
+	mode_cmd.flags = DRM_MODE_FB_MODIFIERS;
 
 	size = mode_cmd.pitches[0] * mode_cmd.height;
 	size = PAGE_ALIGN(size);
diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
index ba60153dddb5..25a2dd6acc61 100644
--- a/drivers/gpu/drm/vc4/vc4_kms.c
+++ b/drivers/gpu/drm/vc4/vc4_kms.c
@@ -148,50 +148,44 @@ static int vc4_atomic_commit(struct drm_device *dev,
 	return 0;
 }
 
-static struct drm_framebuffer *vc4_fb_create(struct drm_device *dev,
-					     struct drm_file *file_priv,
-					     const struct drm_mode_fb_cmd2 *mode_cmd)
+static int vc4_fb_modifier(struct drm_device *dev,
+			   struct drm_file *file_priv,
+			   const struct drm_mode_fb_cmd2 *mode_cmd,
+			   u64 *modifier)
 {
-	struct drm_mode_fb_cmd2 mode_cmd_local;
+	struct drm_gem_object *gem_obj;
+	struct vc4_bo *bo;
 
-	/* If the user didn't specify a modifier, use the
-	 * vc4_set_tiling_ioctl() state for the BO.
-	 */
-	if (!(mode_cmd->flags & DRM_MODE_FB_MODIFIERS)) {
-		struct drm_gem_object *gem_obj;
-		struct vc4_bo *bo;
-
-		gem_obj = drm_gem_object_lookup(file_priv,
-						mode_cmd->handles[0]);
-		if (!gem_obj) {
-			DRM_DEBUG("Failed to look up GEM BO %d\n",
-				  mode_cmd->handles[0]);
-			return ERR_PTR(-ENOENT);
-		}
-		bo = to_vc4_bo(gem_obj);
-
-		mode_cmd_local = *mode_cmd;
+	if (!mode_cmd->handles[0]) {
+		DRM_DEBUG_KMS("no buffer object handle for plane 0\n");
+		return -EINVAL;
+	}
 
-		if (bo->t_format) {
-			mode_cmd_local.modifier[0] =
-				DRM_FORMAT_MOD_BROADCOM_VC4_T_TILED;
-		} else {
-			mode_cmd_local.modifier[0] = DRM_FORMAT_MOD_NONE;
-		}
+	gem_obj = drm_gem_object_lookup(file_priv,
+					mode_cmd->handles[0]);
+	if (!gem_obj) {
+		DRM_DEBUG("Failed to look up GEM BO %d\n",
+			  mode_cmd->handles[0]);
+		return -ENOENT;
+	}
+	bo = to_vc4_bo(gem_obj);
 
-		drm_gem_object_put_unlocked(gem_obj);
+	if (bo->t_format)
+		*modifier = DRM_FORMAT_MOD_BROADCOM_VC4_T_TILED;
+	else
+		*modifier = DRM_FORMAT_MOD_LINEAR;
 
-		mode_cmd = &mode_cmd_local;
-	}
+	drm_gem_object_put_unlocked(gem_obj);
 
-	return drm_gem_fb_create(dev, file_priv, mode_cmd);
+	return 0;
 }
 
 static const struct drm_mode_config_funcs vc4_mode_funcs = {
 	.output_poll_changed = drm_fb_helper_output_poll_changed,
 	.atomic_check = drm_atomic_helper_check,
 	.atomic_commit = vc4_atomic_commit,
-	.fb_create = vc4_fb_create,
+	.fb_modifier = vc4_fb_modifier,
+	.fb_create = drm_gem_fb_create,
 };
 
 int vc4_kms_load(struct drm_device *dev)
diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
index 7569f22ffef6..c74aed292b58 100644
--- a/include/drm/drm_mode_config.h
+++ b/include/drm/drm_mode_config.h
@@ -45,6 +45,30 @@ struct drm_display_mode;
  * involve drivers.
  */
 struct drm_mode_config_funcs {
+	/**
+	 * @fb_modifier:
+	 *
+	 * Deduce the format modifier using a driver specific method. This
+	 * gets called when the request does not explicitly state the
+	 * modifier, ie. (@mode_cmd->flags & DRM_MODE_FB_MODIFIERS) == 0.
+	 * The deduced modifier is returned via @modifier.
+	 *
+	 * Once the modifier is known several sanity checks will be performed
+	 * and if all is deemed valid @fb_create() will be called to have
+	 * the driver to actually create the framebuffer.
+	 *
+	 * This hook is optional. The core will assume DRM_FORMAT_MOD_LINEAR
+	 * if the hook is not provided by the driver.
+	 *
+	 * RETURNS:
+	 *
+	 * Zero on success, negative error code on failure.
+	 */
+	int (*fb_modifier)(struct drm_device *dev,
+			   struct drm_file *file_priv,
+			   const struct drm_mode_fb_cmd2 *mode_cmd,
+			   u64 *modifier);
+
 	/**
 	 * @fb_create:
 	 *
@@ -52,6 +76,11 @@ struct drm_mode_config_funcs {
 	 * requested metadata, but most of that is left to the driver. See
 	 * &struct drm_mode_fb_cmd2 for details.
 	 *
+	 * Note that for drivers that support modifiers, the core will convert
+	 * all requests to one with modifiers. So the driver does not need to
+	 * worry about requests without modifiers (apart from having to provide
+	 * the @fb_modifier() hook if necessary).
+	 *
 	 * If the parameters are deemed valid and the backing storage objects in
 	 * the underlying memory manager all exist, then the driver allocates
 	 * a new &drm_framebuffer structure, subclassed to contain
-- 
2.16.1

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

Re: [PATCH v2 1/3] drm: Add the optional .fb_modifier() hook

[Michel Dänzer]

On 2018-03-13 03:28 PM, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> To make it possible for the core to check the fb pixel format and
> modifier, we need to first ask the driver to deduce the modifier
> when the request does not explicitly specify one.
> 
> Add a new .fb_modifier() hook for that purpose and convert i915
> and vc4 to make use if it. All other drivers seem to currently
> assume linear when the request does not specify anything else,
> [...]

That's not true at least for the amdgpu and radeon drivers. The tiling
mode is communicated via BO metadata.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 1/3] drm: Add the optional .fb_modifier() hook

[Ville Syrjälä]

On Tue, Mar 13, 2018 at 03:38:38PM +0100, Michel Dänzer wrote:
> On 2018-03-13 03:28 PM, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > To make it possible for the core to check the fb pixel format and
> > modifier, we need to first ask the driver to deduce the modifier
> > when the request does not explicitly specify one.
> > 
> > Add a new .fb_modifier() hook for that purpose and convert i915
> > and vc4 to make use if it. All other drivers seem to currently
> > assume linear when the request does not specify anything else,
> > [...]
> 
> That's not true at least for the amdgpu and radeon drivers. The tiling
> mode is communicated via BO metadata.

Well, at least those driver don't do any bo tiling->modifier conversion.
Radeon doesn't do modifiers I suppose, so that part is probably OK.
amggpu might just be broken, not sure.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 1/3] drm: Add the optional .fb_modifier() hook

[Michel Dänzer]

On 2018-03-13 04:12 PM, Ville Syrjälä wrote:
> On Tue, Mar 13, 2018 at 03:38:38PM +0100, Michel Dänzer wrote:
>> On 2018-03-13 03:28 PM, Ville Syrjala wrote:
>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>
>>> To make it possible for the core to check the fb pixel format and
>>> modifier, we need to first ask the driver to deduce the modifier
>>> when the request does not explicitly specify one.
>>>
>>> Add a new .fb_modifier() hook for that purpose and convert i915
>>> and vc4 to make use if it. All other drivers seem to currently
>>> assume linear when the request does not specify anything else,
>>> [...]
>>
>> That's not true at least for the amdgpu and radeon drivers. The tiling
>> mode is communicated via BO metadata.
> 
> Well, at least those driver don't do any bo tiling->modifier conversion.
> Radeon doesn't do modifiers I suppose, so that part is probably OK.
> amggpu might just be broken, not sure.

amdgpu certainly wasn't broken before modifiers came along. If something
broke with those, I'd say it's more accurate to say that's broken and
needs to be fixed, rather than adding more incorrect assumptions based
on it.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 1/3] drm: Add the optional .fb_modifier() hook

[Daniel Vetter]

On Tue, Mar 13, 2018 at 03:38:38PM +0100, Michel Dänzer wrote:
> On 2018-03-13 03:28 PM, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > To make it possible for the core to check the fb pixel format and
> > modifier, we need to first ask the driver to deduce the modifier
> > when the request does not explicitly specify one.
> > 
> > Add a new .fb_modifier() hook for that purpose and convert i915
> > and vc4 to make use if it. All other drivers seem to currently
> > assume linear when the request does not specify anything else,
> > [...]
> 
> That's not true at least for the amdgpu and radeon drivers. The tiling
> mode is communicated via BO metadata.

But atm amdgpu and radeon also don't support explicit modifiers in the
kernel driver, so it again all checks out. Or should at least.

Once you add modifier support, you need to wire up all the bits and the
rigth default selection, and it should again pan out.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 1/3] drm: Add the optional .fb_modifier() hook

[Michel Dänzer]

On 2018-03-13 04:20 PM, Daniel Vetter wrote:
> On Tue, Mar 13, 2018 at 03:38:38PM +0100, Michel Dänzer wrote:
>> On 2018-03-13 03:28 PM, Ville Syrjala wrote:
>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>
>>> To make it possible for the core to check the fb pixel format and
>>> modifier, we need to first ask the driver to deduce the modifier
>>> when the request does not explicitly specify one.
>>>
>>> Add a new .fb_modifier() hook for that purpose and convert i915
>>> and vc4 to make use if it. All other drivers seem to currently
>>> assume linear when the request does not specify anything else,
>>> [...]
>>
>> That's not true at least for the amdgpu and radeon drivers. The tiling
>> mode is communicated via BO metadata.
> 
> But atm amdgpu and radeon also don't support explicit modifiers in the
> kernel driver, so it again all checks out. Or should at least.

Sounds like I misunderstood that this is trying to guess modifiers for
all drivers. So far, so good if so.


> Once you add modifier support, you need to wire up all the bits and the
> rigth default selection, and it should again pan out.

Well, this change allows a driver not to wire up the fb_modifier hook,
and just assumes linear in that case. Seems like an accident waiting to
happen, but I'll leave it to you guys.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 1/3] drm: Add the optional .fb_modifier() hook

[Ville Syrjälä]

On Tue, Mar 13, 2018 at 04:35:02PM +0100, Michel Dänzer wrote:
> On 2018-03-13 04:20 PM, Daniel Vetter wrote:
> > On Tue, Mar 13, 2018 at 03:38:38PM +0100, Michel Dänzer wrote:
> >> On 2018-03-13 03:28 PM, Ville Syrjala wrote:
> >>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>>
> >>> To make it possible for the core to check the fb pixel format and
> >>> modifier, we need to first ask the driver to deduce the modifier
> >>> when the request does not explicitly specify one.
> >>>
> >>> Add a new .fb_modifier() hook for that purpose and convert i915
> >>> and vc4 to make use if it. All other drivers seem to currently
> >>> assume linear when the request does not specify anything else,
> >>> [...]
> >>
> >> That's not true at least for the amdgpu and radeon drivers. The tiling
> >> mode is communicated via BO metadata.
> > 
> > But atm amdgpu and radeon also don't support explicit modifiers in the
> > kernel driver, so it again all checks out. Or should at least.
> 
> Sounds like I misunderstood that this is trying to guess modifiers for
> all drivers. So far, so good if so.

Somehow I convinced myself that amdgpu already had some modifier
support. But looks like I was mistaken.

> > Once you add modifier support, you need to wire up all the bits and the
> > rigth default selection, and it should again pan out.
> 
> Well, this change allows a driver not to wire up the fb_modifier hook,
> and just assumes linear in that case. Seems like an accident waiting to
> happen, but I'll leave it to you guys.

That's already the case with the current code. mode_cmd->modifier[0]
will be 0 when when DRM_MODE_FB_MODIFIERS isn't specified, which is the
same thing as DRM_FORMAT_MOD_LINEAR. So if the driver doesn't change
it before calling drm_helper_mode_fill_fb_struct() you end up with
fb->modifier == DRM_FORMAT_MOD_LINEAR.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

✓ Fi.CI.BAT: success for series starting with [v2,1/3] drm: Add the optional .fb_modifier() hook (rev2)

[Patchwork]

== Series Details ==

Series: series starting with [v2,1/3] drm: Add the optional .fb_modifier() hook (rev2)
URL   : https://patchwork.freedesktop.org/series/39813/
State : success

== Summary ==

Series 39813v2 series starting with [v2,1/3] drm: Add the optional .fb_modifier() hook
https://patchwork.freedesktop.org/api/1.0/series/39813/revisions/2/mbox/

---- Known issues:

Test kms_chamelium:
        Subgroup dp-crc-fast:
                fail       -> PASS       (fi-kbl-7500u) fdo#103841
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-c:
                pass       -> INCOMPLETE (fi-skl-6700k2) fdo#104108

fdo#103841 https://bugs.freedesktop.org/show_bug.cgi?id=103841
fdo#104108 https://bugs.freedesktop.org/show_bug.cgi?id=104108

fi-bdw-5557u     total:288  pass:267  dwarn:0   dfail:0   fail:0   skip:21  time:428s
fi-bdw-gvtdvm    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:432s
fi-blb-e6850     total:288  pass:223  dwarn:1   dfail:0   fail:0   skip:64  time:382s
fi-bsw-n3050     total:288  pass:242  dwarn:0   dfail:0   fail:0   skip:46  time:538s
fi-bwr-2160      total:288  pass:183  dwarn:0   dfail:0   fail:0   skip:105 time:298s
fi-bxt-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:508s
fi-bxt-j4205     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:505s
fi-byt-j1900     total:288  pass:253  dwarn:0   dfail:0   fail:0   skip:35  time:507s
fi-byt-n2820     total:288  pass:249  dwarn:0   dfail:0   fail:0   skip:39  time:499s
fi-cfl-8700k     total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:408s
fi-cfl-s2        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:579s
fi-cnl-y3        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:582s
fi-elk-e7500     total:288  pass:229  dwarn:0   dfail:0   fail:0   skip:59  time:425s
fi-gdg-551       total:288  pass:179  dwarn:0   dfail:0   fail:1   skip:108 time:313s
fi-glk-1         total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:530s
fi-hsw-4770      total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:404s
fi-ilk-650       total:288  pass:228  dwarn:0   dfail:0   fail:0   skip:60  time:415s
fi-ivb-3520m     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:474s
fi-kbl-7500u     total:288  pass:263  dwarn:1   dfail:0   fail:0   skip:24  time:471s
fi-kbl-7567u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:467s
fi-kbl-r         total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:517s
fi-pnv-d510      total:288  pass:222  dwarn:1   dfail:0   fail:0   skip:65  time:644s
fi-skl-6260u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:438s
fi-skl-6600u     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:524s
fi-skl-6700hq    total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:538s
fi-skl-6700k2    total:246  pass:225  dwarn:0   dfail:0   fail:0   skip:20 
fi-skl-6770hq    total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:493s
fi-skl-guc       total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:429s
fi-skl-gvtdvm    total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  time:432s
fi-snb-2520m     total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:547s
fi-snb-2600      total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:399s
Blacklisted hosts:
fi-cfl-u         total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:508s
fi-cnl-drrs      total:288  pass:257  dwarn:3   dfail:0   fail:0   skip:28  time:533s

874b86a759851707e26286c22062f6ccc526e46f drm-tip: 2018y-03m-13d-12h-36m-17s UTC integration manifest
fc4f4d11665c drm/i915: Eliminate the horrendous format check code
5caf98a38622 drm: Make sure at least one plane supports the fb format
9b1e973a1ddb drm: Add the optional .fb_modifier() hook

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8329/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.IGT: failure for series starting with [v2,1/3] drm: Add the optional .fb_modifier() hook (rev2)

[Patchwork]

== Series Details ==

Series: series starting with [v2,1/3] drm: Add the optional .fb_modifier() hook (rev2)
URL   : https://patchwork.freedesktop.org/series/39813/
State : failure

== Summary ==

---- Possible new issues:

Test kms_cursor_crc:
        Subgroup cursor-64x64-suspend:
                skip       -> PASS       (shard-snb)
Test kms_frontbuffer_tracking:
        Subgroup fbc-rgb565-draw-mmap-gtt:
                pass       -> FAIL       (shard-apl)
Test kms_properties:
        Subgroup crtc-properties-legacy:
                skip       -> PASS       (shard-snb)

---- Known issues:

Test gem_eio:
        Subgroup in-flight-external:
                pass       -> INCOMPLETE (shard-hsw) fdo#105341 +1
Test kms_flip:
        Subgroup 2x-plain-flip-fb-recreate-interruptible:
                pass       -> FAIL       (shard-hsw) fdo#100368 +2
        Subgroup flip-vs-expired-vblank:
                pass       -> FAIL       (shard-hsw) fdo#102887
Test kms_frontbuffer_tracking:
        Subgroup fbc-1p-offscren-pri-shrfb-draw-render:
                pass       -> DMESG-FAIL (shard-apl) fdo#101623 +2
        Subgroup fbc-1p-pri-indfb-multidraw:
                pass       -> FAIL       (shard-apl) fdo#103167
        Subgroup fbc-1p-primscrn-shrfb-msflip-blt:
                pass       -> FAIL       (shard-apl) fdo#104727
Test kms_rotation_crc:
        Subgroup cursor-rotation-180:
                pass       -> FAIL       (shard-snb) fdo#105185
Test perf:
        Subgroup polling:
                pass       -> FAIL       (shard-hsw) fdo#102252

fdo#105341 https://bugs.freedesktop.org/show_bug.cgi?id=105341
fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
fdo#102887 https://bugs.freedesktop.org/show_bug.cgi?id=102887
fdo#101623 https://bugs.freedesktop.org/show_bug.cgi?id=101623
fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
fdo#104727 https://bugs.freedesktop.org/show_bug.cgi?id=104727
fdo#105185 https://bugs.freedesktop.org/show_bug.cgi?id=105185
fdo#102252 https://bugs.freedesktop.org/show_bug.cgi?id=102252

shard-apl        total:3371 pass:1770 dwarn:1   dfail:1   fail:12  skip:1585 time:12277s
shard-hsw        total:3381 pass:1727 dwarn:1   dfail:0   fail:5   skip:1646 time:11578s
shard-snb        total:3444 pass:1358 dwarn:1   dfail:0   fail:4   skip:2081 time:7143s
Blacklisted hosts:
shard-kbl        total:3371 pass:1817 dwarn:14  dfail:0   fail:8   skip:1531 time:8622s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8329/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: ✗ Fi.CI.IGT: failure for series starting with [v2,1/3] drm: Add the optional .fb_modifier() hook (rev2)

[Ville Syrjälä]

On Tue, Mar 13, 2018 at 04:56:52PM -0000, Patchwork wrote:
> == Series Details ==
> 
> Series: series starting with [v2,1/3] drm: Add the optional .fb_modifier() hook (rev2)
> URL   : https://patchwork.freedesktop.org/series/39813/
> State : failure
> 
> == Summary ==
> 
> ---- Possible new issues:
> 
> Test kms_cursor_crc:
>         Subgroup cursor-64x64-suspend:
>                 skip       -> PASS       (shard-snb)
> Test kms_frontbuffer_tracking:
>         Subgroup fbc-rgb565-draw-mmap-gtt:
>                 pass       -> FAIL       (shard-apl)

Unrelated.

(kms_frontbuffer_tracking:1620) WARNING: fbc_is_enabled()?
FBC disabled: underrun detected
(kms_frontbuffer_tracking:1620) CRITICAL: Test assertion failure function do_status_assertions, file ../tests/kms_frontbuffer_tracking.c:1733:
(kms_frontbuffer_tracking:1620) CRITICAL: Failed assertion: fbc_is_enabled(IGT_LOG_WARN)
(kms_frontbuffer_tracking:1620) CRITICAL: FBC disabled

> Test kms_properties:
>         Subgroup crtc-properties-legacy:
>                 skip       -> PASS       (shard-snb)
> 
> ---- Known issues:
> 
> Test gem_eio:
>         Subgroup in-flight-external:
>                 pass       -> INCOMPLETE (shard-hsw) fdo#105341 +1
> Test kms_flip:
>         Subgroup 2x-plain-flip-fb-recreate-interruptible:
>                 pass       -> FAIL       (shard-hsw) fdo#100368 +2
>         Subgroup flip-vs-expired-vblank:
>                 pass       -> FAIL       (shard-hsw) fdo#102887
> Test kms_frontbuffer_tracking:
>         Subgroup fbc-1p-offscren-pri-shrfb-draw-render:
>                 pass       -> DMESG-FAIL (shard-apl) fdo#101623 +2
>         Subgroup fbc-1p-pri-indfb-multidraw:
>                 pass       -> FAIL       (shard-apl) fdo#103167
>         Subgroup fbc-1p-primscrn-shrfb-msflip-blt:
>                 pass       -> FAIL       (shard-apl) fdo#104727
> Test kms_rotation_crc:
>         Subgroup cursor-rotation-180:
>                 pass       -> FAIL       (shard-snb) fdo#105185
> Test perf:
>         Subgroup polling:
>                 pass       -> FAIL       (shard-hsw) fdo#102252
> 
> fdo#105341 https://bugs.freedesktop.org/show_bug.cgi?id=105341
> fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
> fdo#102887 https://bugs.freedesktop.org/show_bug.cgi?id=102887
> fdo#101623 https://bugs.freedesktop.org/show_bug.cgi?id=101623
> fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
> fdo#104727 https://bugs.freedesktop.org/show_bug.cgi?id=104727
> fdo#105185 https://bugs.freedesktop.org/show_bug.cgi?id=105185
> fdo#102252 https://bugs.freedesktop.org/show_bug.cgi?id=102252
> 
> shard-apl        total:3371 pass:1770 dwarn:1   dfail:1   fail:12  skip:1585 time:12277s
> shard-hsw        total:3381 pass:1727 dwarn:1   dfail:0   fail:5   skip:1646 time:11578s
> shard-snb        total:3444 pass:1358 dwarn:1   dfail:0   fail:4   skip:2081 time:7143s
> Blacklisted hosts:
> shard-kbl        total:3371 pass:1817 dwarn:14  dfail:0   fail:8   skip:1531 time:8622s
> 
> == Logs ==
> 
> For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8329/shards.html

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/3] drm: Make sure at least one plane supports the fb format

[Eric Anholt]

[-- Attachment #1.1: Type: text/plain, Size: 2524 bytes --]

Ville Syrjala <ville.syrjala@linux.intel.com> writes:

> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> To make life easier for drivers, let's have the core check that the
> requested pixel format is supported by at least one plane when creating
> a new framebuffer.
>
> This eases the burden on drivers by making sure they'll never get
> requests to create fbs with unsupported pixel formats. Thanks to the
> new .fb_modifier() hook this check can now be done whether the request
> specifies the modifier directly or driver has to deduce it from the
> gem bo tiling (or via any other method really).
>
> v0: Accept anyformat if the driver doesn't do planes (Eric)
>     s/planes_have_format/any_plane_has_format/ (Eric)
>     Check the modifier as well since we already have a function
>     that does both
> v3: Don't do the check in the core since we may not know the
>     modifier yet, instead export the function and let drivers
>     call it themselves
> v4: Unexport the functiona and put the format_default check back
>     since this will again be called by the core, ie. undo v3 ;)
>
> Cc: Eric Anholt <eric@anholt.net>
> Testcase: igt/kms_addfb_basic/expected-formats
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_framebuffer.c | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
> index 21d3d51eb261..e618a6b728d4 100644
> --- a/drivers/gpu/drm/drm_framebuffer.c
> +++ b/drivers/gpu/drm/drm_framebuffer.c
> @@ -152,6 +152,26 @@ static int fb_plane_height(int height,
>  	return DIV_ROUND_UP(height, format->vsub);
>  }
>  
> +static bool any_plane_has_format(struct drm_device *dev,
> +				 u32 format, u64 modifier)
> +{
> +	struct drm_plane *plane;
> +
> +	drm_for_each_plane(plane, dev) {
> +		/*
> +		 * In case the driver doesn't really do
> +		 * planes we have to accept any format here.
> +		 */
> +		if (plane->format_default)
> +			return true;
> +
> +		if (drm_plane_check_pixel_format(plane, format, modifier) == 0)
> +			return true;
> +	}
> +
> +	return false;
> +}

This drm_plane_check_pixel_format() will always fail for VC4's SAND
modifiers or VC5's UIF modifiers, where we're using the middle 48 bits
as a bit of metadata (like how we have horizontal stride passed outside
of the modifier) and you can't list all of the possible values in an
array on the plane.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

Re: [PATCH 2/3] drm: Make sure at least one plane supports the fb format

[Ville Syrjälä]

On Thu, Mar 15, 2018 at 10:42:17AM -0700, Eric Anholt wrote:
> Ville Syrjala <ville.syrjala@linux.intel.com> writes:
> 
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > To make life easier for drivers, let's have the core check that the
> > requested pixel format is supported by at least one plane when creating
> > a new framebuffer.
> >
> > This eases the burden on drivers by making sure they'll never get
> > requests to create fbs with unsupported pixel formats. Thanks to the
> > new .fb_modifier() hook this check can now be done whether the request
> > specifies the modifier directly or driver has to deduce it from the
> > gem bo tiling (or via any other method really).
> >
> > v0: Accept anyformat if the driver doesn't do planes (Eric)
> >     s/planes_have_format/any_plane_has_format/ (Eric)
> >     Check the modifier as well since we already have a function
> >     that does both
> > v3: Don't do the check in the core since we may not know the
> >     modifier yet, instead export the function and let drivers
> >     call it themselves
> > v4: Unexport the functiona and put the format_default check back
> >     since this will again be called by the core, ie. undo v3 ;)
> >
> > Cc: Eric Anholt <eric@anholt.net>
> > Testcase: igt/kms_addfb_basic/expected-formats
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/drm_framebuffer.c | 30 ++++++++++++++++++++++++++++++
> >  1 file changed, 30 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
> > index 21d3d51eb261..e618a6b728d4 100644
> > --- a/drivers/gpu/drm/drm_framebuffer.c
> > +++ b/drivers/gpu/drm/drm_framebuffer.c
> > @@ -152,6 +152,26 @@ static int fb_plane_height(int height,
> >  	return DIV_ROUND_UP(height, format->vsub);
> >  }
> >  
> > +static bool any_plane_has_format(struct drm_device *dev,
> > +				 u32 format, u64 modifier)
> > +{
> > +	struct drm_plane *plane;
> > +
> > +	drm_for_each_plane(plane, dev) {
> > +		/*
> > +		 * In case the driver doesn't really do
> > +		 * planes we have to accept any format here.
> > +		 */
> > +		if (plane->format_default)
> > +			return true;
> > +
> > +		if (drm_plane_check_pixel_format(plane, format, modifier) == 0)
> > +			return true;
> > +	}
> > +
> > +	return false;
> > +}
> 
> This drm_plane_check_pixel_format() will always fail for VC4's SAND
> modifiers or VC5's UIF modifiers, where we're using the middle 48 bits
> as a bit of metadata (like how we have horizontal stride passed outside
> of the modifier) and you can't list all of the possible values in an
> array on the plane.

Hmm. drm_atomic_plane_check() etc. call this thing as well. How does
that manage to work currently?

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/3] drm: Make sure at least one plane supports the fb format

[Ville Syrjälä]

On Thu, Mar 15, 2018 at 07:48:02PM +0200, Ville Syrjälä wrote:
> On Thu, Mar 15, 2018 at 10:42:17AM -0700, Eric Anholt wrote:
> > Ville Syrjala <ville.syrjala@linux.intel.com> writes:
> > 
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >
> > > To make life easier for drivers, let's have the core check that the
> > > requested pixel format is supported by at least one plane when creating
> > > a new framebuffer.
> > >
> > > This eases the burden on drivers by making sure they'll never get
> > > requests to create fbs with unsupported pixel formats. Thanks to the
> > > new .fb_modifier() hook this check can now be done whether the request
> > > specifies the modifier directly or driver has to deduce it from the
> > > gem bo tiling (or via any other method really).
> > >
> > > v0: Accept anyformat if the driver doesn't do planes (Eric)
> > >     s/planes_have_format/any_plane_has_format/ (Eric)
> > >     Check the modifier as well since we already have a function
> > >     that does both
> > > v3: Don't do the check in the core since we may not know the
> > >     modifier yet, instead export the function and let drivers
> > >     call it themselves
> > > v4: Unexport the functiona and put the format_default check back
> > >     since this will again be called by the core, ie. undo v3 ;)
> > >
> > > Cc: Eric Anholt <eric@anholt.net>
> > > Testcase: igt/kms_addfb_basic/expected-formats
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/drm_framebuffer.c | 30 ++++++++++++++++++++++++++++++
> > >  1 file changed, 30 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
> > > index 21d3d51eb261..e618a6b728d4 100644
> > > --- a/drivers/gpu/drm/drm_framebuffer.c
> > > +++ b/drivers/gpu/drm/drm_framebuffer.c
> > > @@ -152,6 +152,26 @@ static int fb_plane_height(int height,
> > >  	return DIV_ROUND_UP(height, format->vsub);
> > >  }
> > >  
> > > +static bool any_plane_has_format(struct drm_device *dev,
> > > +				 u32 format, u64 modifier)
> > > +{
> > > +	struct drm_plane *plane;
> > > +
> > > +	drm_for_each_plane(plane, dev) {
> > > +		/*
> > > +		 * In case the driver doesn't really do
> > > +		 * planes we have to accept any format here.
> > > +		 */
> > > +		if (plane->format_default)
> > > +			return true;
> > > +
> > > +		if (drm_plane_check_pixel_format(plane, format, modifier) == 0)
> > > +			return true;
> > > +	}
> > > +
> > > +	return false;
> > > +}
> > 
> > This drm_plane_check_pixel_format() will always fail for VC4's SAND
> > modifiers or VC5's UIF modifiers, where we're using the middle 48 bits
> > as a bit of metadata (like how we have horizontal stride passed outside
> > of the modifier) and you can't list all of the possible values in an
> > array on the plane.
> 
> Hmm. drm_atomic_plane_check() etc. call this thing as well. How does
> that manage to work currently?

Maybe it doesn't. I added the modifier checks in

commit 23163a7d4b032489d375099d56571371c0456980
Author:     Ville Syrjälä <ville.syrjala@linux.intel.com>
AuthorDate: Fri Dec 22 21:22:30 2017 +0200
Commit:     Ville Syrjälä <ville.syrjala@linux.intel.com>
CommitDate: Mon Feb 26 16:29:47 2018 +0200

    drm: Check that the plane supports the request format+modifier combo

Maybe that broke vc4?

Hmm. So either we need to stop checking against the modifiers array and
rely purely or .format_mod_supported(), or we need to somehow get the
driver to reduce the modifier to its base form. I guess we could make
.fb_modifier() do that and call it also for addfb with modifiers. And
I'd need to undo some of the modifier[0] vs. deduced modifier changes
I did to framebuffer_check(), and we'd need to preserve the original
modifier in the request for .fb_create(). Oh, but that wouldn't allow
returning a non-base modifier from .fb_modifuer() for the !modifiers
case. This is turning slightly more tricky than I had hoped...

I guess relying on .format_mod_supported() might be what we need to 
do. Unfortunately it does mean that the .format_mod_supported()
implementations must be prepared for modifiers that were not
registered with the plane. Which does feel quite a bit more
fragile.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/3] drm: Make sure at least one plane supports the fb format

[Ville Syrjälä]

On Thu, Mar 15, 2018 at 08:03:44PM +0200, Ville Syrjälä wrote:
> On Thu, Mar 15, 2018 at 07:48:02PM +0200, Ville Syrjälä wrote:
> > On Thu, Mar 15, 2018 at 10:42:17AM -0700, Eric Anholt wrote:
> > > Ville Syrjala <ville.syrjala@linux.intel.com> writes:
> > > 
> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > >
> > > > To make life easier for drivers, let's have the core check that the
> > > > requested pixel format is supported by at least one plane when creating
> > > > a new framebuffer.
> > > >
> > > > This eases the burden on drivers by making sure they'll never get
> > > > requests to create fbs with unsupported pixel formats. Thanks to the
> > > > new .fb_modifier() hook this check can now be done whether the request
> > > > specifies the modifier directly or driver has to deduce it from the
> > > > gem bo tiling (or via any other method really).
> > > >
> > > > v0: Accept anyformat if the driver doesn't do planes (Eric)
> > > >     s/planes_have_format/any_plane_has_format/ (Eric)
> > > >     Check the modifier as well since we already have a function
> > > >     that does both
> > > > v3: Don't do the check in the core since we may not know the
> > > >     modifier yet, instead export the function and let drivers
> > > >     call it themselves
> > > > v4: Unexport the functiona and put the format_default check back
> > > >     since this will again be called by the core, ie. undo v3 ;)
> > > >
> > > > Cc: Eric Anholt <eric@anholt.net>
> > > > Testcase: igt/kms_addfb_basic/expected-formats
> > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > ---
> > > >  drivers/gpu/drm/drm_framebuffer.c | 30 ++++++++++++++++++++++++++++++
> > > >  1 file changed, 30 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
> > > > index 21d3d51eb261..e618a6b728d4 100644
> > > > --- a/drivers/gpu/drm/drm_framebuffer.c
> > > > +++ b/drivers/gpu/drm/drm_framebuffer.c
> > > > @@ -152,6 +152,26 @@ static int fb_plane_height(int height,
> > > >  	return DIV_ROUND_UP(height, format->vsub);
> > > >  }
> > > >  
> > > > +static bool any_plane_has_format(struct drm_device *dev,
> > > > +				 u32 format, u64 modifier)
> > > > +{
> > > > +	struct drm_plane *plane;
> > > > +
> > > > +	drm_for_each_plane(plane, dev) {
> > > > +		/*
> > > > +		 * In case the driver doesn't really do
> > > > +		 * planes we have to accept any format here.
> > > > +		 */
> > > > +		if (plane->format_default)
> > > > +			return true;
> > > > +
> > > > +		if (drm_plane_check_pixel_format(plane, format, modifier) == 0)
> > > > +			return true;
> > > > +	}
> > > > +
> > > > +	return false;
> > > > +}
> > > 
> > > This drm_plane_check_pixel_format() will always fail for VC4's SAND
> > > modifiers or VC5's UIF modifiers, where we're using the middle 48 bits
> > > as a bit of metadata (like how we have horizontal stride passed outside
> > > of the modifier) and you can't list all of the possible values in an
> > > array on the plane.
> > 
> > Hmm. drm_atomic_plane_check() etc. call this thing as well. How does
> > that manage to work currently?
> 
> Maybe it doesn't. I added the modifier checks in
> 
> commit 23163a7d4b032489d375099d56571371c0456980
> Author:     Ville Syrjälä <ville.syrjala@linux.intel.com>
> AuthorDate: Fri Dec 22 21:22:30 2017 +0200
> Commit:     Ville Syrjälä <ville.syrjala@linux.intel.com>
> CommitDate: Mon Feb 26 16:29:47 2018 +0200
> 
>     drm: Check that the plane supports the request format+modifier combo
> 
> Maybe that broke vc4?
> 
> Hmm. So either we need to stop checking against the modifiers array and
> rely purely or .format_mod_supported(), or we need to somehow get the
> driver to reduce the modifier to its base form. I guess we could make
> .fb_modifier() do that and call it also for addfb with modifiers. And
> I'd need to undo some of the modifier[0] vs. deduced modifier changes
> I did to framebuffer_check(), and we'd need to preserve the original
> modifier in the request for .fb_create(). Oh, but that wouldn't allow
> returning a non-base modifier from .fb_modifuer() for the !modifiers
> case. This is turning slightly more tricky than I had hoped...
> 
> I guess relying on .format_mod_supported() might be what we need to 
> do. Unfortunately it does mean that the .format_mod_supported()
> implementations must be prepared for modifiers that were not
> registered with the plane. Which does feel quite a bit more
> fragile.

And that last apporach would be annoying for i915. So I think the other
apporach is better.

Fortunately it seems your SAND modifiers aren't upstream yet so I didn't
break them :) I had a quick look at your github and it looks like the
first apporach would work.

Something like this is what I had in mind. Loosk like you could plug in
fourcc_mod_broadcom_mod() almost directly into .base_modifier().

diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index a5d1fc7e8a37..250f66d51c2c 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -552,6 +552,8 @@ int drm_mode_getplane(struct drm_device *dev, void *data,
 int drm_plane_check_pixel_format(struct drm_plane *plane,
 				 u32 format, u64 modifier)
 {
+	struct drm_device *dev = plane->dev;
+	u64 base_modifier;
 	unsigned int i;
 
 	for (i = 0; i < plane->format_count; i++) {
@@ -564,8 +566,13 @@ int drm_plane_check_pixel_format(struct drm_plane *plane,
 	if (!plane->modifier_count)
 		return 0;
 
+	if (dev->mode_config.funcs->base_modifier)
+		base_modifier = dev->mode_config.funcs->base_modifier(dev, modifier);
+	else
+		base_modifier = modifier;
+
 	for (i = 0; i < plane->modifier_count; i++) {
-		if (modifier == plane->modifiers[i])
+		if (base_modifier == plane->modifiers[i])
 			break;
 	}
 	if (i == plane->modifier_count)
diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
index c74aed292b58..2ff2438263ef 100644
--- a/include/drm/drm_mode_config.h
+++ b/include/drm/drm_mode_config.h
@@ -45,6 +45,19 @@ struct drm_display_mode;
  * involve drivers.
  */
 struct drm_mode_config_funcs {
+	/**
+	 * @base_modifier:
+	 *
+	 * Reduce the passed in modifier to its base form. This optional
+	 * hook needs to be provided by any driver that embeds extra
+	 * metadata within the format modifier.
+	 *
+	 * RETURNS:
+	 * The base form of the modifier with any extra metadata
+	 * cleared out.
+	 */
+	u64 (*base_modifier)(struct drm_device *dev, u64 modifier);
+
 	/**
 	 * @fb_modifier:
 	 *
diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
index f7bf4a48b1c3..5e26d2a5f6ab 100644
--- a/include/drm/drm_plane.h
+++ b/include/drm/drm_plane.h
@@ -489,8 +489,6 @@ enum drm_plane_type {
  * @format_types: array of formats supported by this plane
  * @format_count: number of formats supported
  * @format_default: driver hasn't supplied supported formats for the plane
- * @modifiers: array of modifiers supported by this plane
- * @modifier_count: number of modifiers supported
  * @old_fb: Temporary tracking of the old fb while a modeset is ongoing. Used by
  * 	drm_mode_set_config_internal() to implement correct refcounting.
  * @funcs: helper functions
@@ -524,7 +522,15 @@ struct drm_plane {
 	unsigned int format_count;
 	bool format_default;
 
+	/**
+	 * @modifiers: Array of modifiers supported by this plane. If
+	 * the driver embeds any extra metadata within modifiers these
+	 * modifiers must be in their base form.
+	 */
 	uint64_t *modifiers;
+	/**
+	 * @modifier_count: number of modifiers supported
+	 */
 	unsigned int modifier_count;
 
 	/**
-- 
2.16.1

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/3] drm: Make sure at least one plane supports the fb format

[Eric Anholt]

[-- Attachment #1.1: Type: text/plain, Size: 5293 bytes --]

Ville Syrjälä <ville.syrjala@linux.intel.com> writes:

> On Thu, Mar 15, 2018 at 08:03:44PM +0200, Ville Syrjälä wrote:
>> On Thu, Mar 15, 2018 at 07:48:02PM +0200, Ville Syrjälä wrote:
>> > On Thu, Mar 15, 2018 at 10:42:17AM -0700, Eric Anholt wrote:
>> > > Ville Syrjala <ville.syrjala@linux.intel.com> writes:
>> > > 
>> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> > > >
>> > > > To make life easier for drivers, let's have the core check that the
>> > > > requested pixel format is supported by at least one plane when creating
>> > > > a new framebuffer.
>> > > >
>> > > > This eases the burden on drivers by making sure they'll never get
>> > > > requests to create fbs with unsupported pixel formats. Thanks to the
>> > > > new .fb_modifier() hook this check can now be done whether the request
>> > > > specifies the modifier directly or driver has to deduce it from the
>> > > > gem bo tiling (or via any other method really).
>> > > >
>> > > > v0: Accept anyformat if the driver doesn't do planes (Eric)
>> > > >     s/planes_have_format/any_plane_has_format/ (Eric)
>> > > >     Check the modifier as well since we already have a function
>> > > >     that does both
>> > > > v3: Don't do the check in the core since we may not know the
>> > > >     modifier yet, instead export the function and let drivers
>> > > >     call it themselves
>> > > > v4: Unexport the functiona and put the format_default check back
>> > > >     since this will again be called by the core, ie. undo v3 ;)
>> > > >
>> > > > Cc: Eric Anholt <eric@anholt.net>
>> > > > Testcase: igt/kms_addfb_basic/expected-formats
>> > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> > > > ---
>> > > >  drivers/gpu/drm/drm_framebuffer.c | 30 ++++++++++++++++++++++++++++++
>> > > >  1 file changed, 30 insertions(+)
>> > > >
>> > > > diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
>> > > > index 21d3d51eb261..e618a6b728d4 100644
>> > > > --- a/drivers/gpu/drm/drm_framebuffer.c
>> > > > +++ b/drivers/gpu/drm/drm_framebuffer.c
>> > > > @@ -152,6 +152,26 @@ static int fb_plane_height(int height,
>> > > >  	return DIV_ROUND_UP(height, format->vsub);
>> > > >  }
>> > > >  
>> > > > +static bool any_plane_has_format(struct drm_device *dev,
>> > > > +				 u32 format, u64 modifier)
>> > > > +{
>> > > > +	struct drm_plane *plane;
>> > > > +
>> > > > +	drm_for_each_plane(plane, dev) {
>> > > > +		/*
>> > > > +		 * In case the driver doesn't really do
>> > > > +		 * planes we have to accept any format here.
>> > > > +		 */
>> > > > +		if (plane->format_default)
>> > > > +			return true;
>> > > > +
>> > > > +		if (drm_plane_check_pixel_format(plane, format, modifier) == 0)
>> > > > +			return true;
>> > > > +	}
>> > > > +
>> > > > +	return false;
>> > > > +}
>> > > 
>> > > This drm_plane_check_pixel_format() will always fail for VC4's SAND
>> > > modifiers or VC5's UIF modifiers, where we're using the middle 48 bits
>> > > as a bit of metadata (like how we have horizontal stride passed outside
>> > > of the modifier) and you can't list all of the possible values in an
>> > > array on the plane.
>> > 
>> > Hmm. drm_atomic_plane_check() etc. call this thing as well. How does
>> > that manage to work currently?
>> 
>> Maybe it doesn't. I added the modifier checks in
>> 
>> commit 23163a7d4b032489d375099d56571371c0456980
>> Author:     Ville Syrjälä <ville.syrjala@linux.intel.com>
>> AuthorDate: Fri Dec 22 21:22:30 2017 +0200
>> Commit:     Ville Syrjälä <ville.syrjala@linux.intel.com>
>> CommitDate: Mon Feb 26 16:29:47 2018 +0200
>> 
>>     drm: Check that the plane supports the request format+modifier combo
>> 
>> Maybe that broke vc4?
>> 
>> Hmm. So either we need to stop checking against the modifiers array and
>> rely purely or .format_mod_supported(), or we need to somehow get the
>> driver to reduce the modifier to its base form. I guess we could make
>> .fb_modifier() do that and call it also for addfb with modifiers. And
>> I'd need to undo some of the modifier[0] vs. deduced modifier changes
>> I did to framebuffer_check(), and we'd need to preserve the original
>> modifier in the request for .fb_create(). Oh, but that wouldn't allow
>> returning a non-base modifier from .fb_modifuer() for the !modifiers
>> case. This is turning slightly more tricky than I had hoped...
>> 
>> I guess relying on .format_mod_supported() might be what we need to 
>> do. Unfortunately it does mean that the .format_mod_supported()
>> implementations must be prepared for modifiers that were not
>> registered with the plane. Which does feel quite a bit more
>> fragile.
>
> And that last apporach would be annoying for i915. So I think the other
> apporach is better.
>
> Fortunately it seems your SAND modifiers aren't upstream yet so I didn't
> break them :) I had a quick look at your github and it looks like the
> first apporach would work.
>
> Something like this is what I had in mind. Loosk like you could plug in
> fourcc_mod_broadcom_mod() almost directly into .base_modifier().

I think just trusting format_mod_supported's result makes more sense.
See the patch series I just sent.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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