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