On Wed, Oct 13, 2021 at 11:17:04PM +0300, Ville Syrjälä wrote:
> On Fri, Oct 08, 2021 at 03:19:09AM +0300, Imre Deak wrote:
> > Move the function retrieving the format override information for a given
> > format/modifier to intel_fb.c. We can store a pointer to the format list
> > in each modifier's descriptor instead of the corresponding switch/case
> > logic, avoiding the listing of the modifiers twice.
> >
> > v2: Handle invalid modifiers in intel_fb_get_format_info() passed from
> > userspace.
>
> Do we have any tests for that btw?
Yes, that's how CI caught it: igt/kms_addfb_basic/addfb25-bad-modifier
>
> > Signed-off-by: Imre Deak <imre.d...@intel.com>
> > ---
> > drivers/gpu/drm/i915/display/intel_display.c | 132 +--------------
> > drivers/gpu/drm/i915/display/intel_fb.c | 163 +++++++++++++++++++
> > drivers/gpu/drm/i915/display/intel_fb.h | 3 +
> > 3 files changed, 167 insertions(+), 131 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > b/drivers/gpu/drm/i915/display/intel_display.c
> > index 4f0badb11bbba..90802d16fbf91 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -1087,136 +1087,6 @@ void intel_add_fb_offsets(int *x, int *y,
> > *y += state->view.color_plane[color_plane].y;
> > }
> >
> > -/*
> > - * From the Sky Lake PRM:
> > - * "The Color Control Surface (CCS) contains the compression status of
> > - * the cache-line pairs. The compression state of the cache-line pair
> > - * is specified by 2 bits in the CCS. Each CCS cache-line represents
> > - * an area on the main surface of 16 x16 sets of 128 byte Y-tiled
> > - * cache-line-pairs. CCS is always Y tiled."
> > - *
> > - * Since cache line pairs refers to horizontally adjacent cache lines,
> > - * each cache line in the CCS corresponds to an area of 32x16 cache
> > - * lines on the main surface. Since each pixel is 4 bytes, this gives
> > - * us a ratio of one byte in the CCS for each 8x16 pixels in the
> > - * main surface.
> > - */
> > -static const struct drm_format_info skl_ccs_formats[] = {
> > - { .format = DRM_FORMAT_XRGB8888, .depth = 24, .num_planes = 2,
> > - .cpp = { 4, 1, }, .hsub = 8, .vsub = 16, },
> > - { .format = DRM_FORMAT_XBGR8888, .depth = 24, .num_planes = 2,
> > - .cpp = { 4, 1, }, .hsub = 8, .vsub = 16, },
> > - { .format = DRM_FORMAT_ARGB8888, .depth = 32, .num_planes = 2,
> > - .cpp = { 4, 1, }, .hsub = 8, .vsub = 16, .has_alpha = true, },
> > - { .format = DRM_FORMAT_ABGR8888, .depth = 32, .num_planes = 2,
> > - .cpp = { 4, 1, }, .hsub = 8, .vsub = 16, .has_alpha = true, },
> > -};
> > -
> > -/*
> > - * Gen-12 compression uses 4 bits of CCS data for each cache line pair in
> > the
> > - * main surface. And each 64B CCS cache line represents an area of 4x1
> > Y-tiles
> > - * in the main surface. With 4 byte pixels and each Y-tile having
> > dimensions of
> > - * 32x32 pixels, the ratio turns out to 1B in the CCS for every 2x32
> > pixels in
> > - * the main surface.
> > - */
> > -static const struct drm_format_info gen12_ccs_formats[] = {
> > - { .format = DRM_FORMAT_XRGB8888, .depth = 24, .num_planes = 2,
> > - .char_per_block = { 4, 1 }, .block_w = { 1, 2 }, .block_h = { 1, 1 },
> > - .hsub = 1, .vsub = 1, },
> > - { .format = DRM_FORMAT_XBGR8888, .depth = 24, .num_planes = 2,
> > - .char_per_block = { 4, 1 }, .block_w = { 1, 2 }, .block_h = { 1, 1 },
> > - .hsub = 1, .vsub = 1, },
> > - { .format = DRM_FORMAT_ARGB8888, .depth = 32, .num_planes = 2,
> > - .char_per_block = { 4, 1 }, .block_w = { 1, 2 }, .block_h = { 1, 1 },
> > - .hsub = 1, .vsub = 1, .has_alpha = true },
> > - { .format = DRM_FORMAT_ABGR8888, .depth = 32, .num_planes = 2,
> > - .char_per_block = { 4, 1 }, .block_w = { 1, 2 }, .block_h = { 1, 1 },
> > - .hsub = 1, .vsub = 1, .has_alpha = true },
> > - { .format = DRM_FORMAT_YUYV, .num_planes = 2,
> > - .char_per_block = { 2, 1 }, .block_w = { 1, 2 }, .block_h = { 1, 1 },
> > - .hsub = 2, .vsub = 1, .is_yuv = true },
> > - { .format = DRM_FORMAT_YVYU, .num_planes = 2,
> > - .char_per_block = { 2, 1 }, .block_w = { 1, 2 }, .block_h = { 1, 1 },
> > - .hsub = 2, .vsub = 1, .is_yuv = true },
> > - { .format = DRM_FORMAT_UYVY, .num_planes = 2,
> > - .char_per_block = { 2, 1 }, .block_w = { 1, 2 }, .block_h = { 1, 1 },
> > - .hsub = 2, .vsub = 1, .is_yuv = true },
> > - { .format = DRM_FORMAT_VYUY, .num_planes = 2,
> > - .char_per_block = { 2, 1 }, .block_w = { 1, 2 }, .block_h = { 1, 1 },
> > - .hsub = 2, .vsub = 1, .is_yuv = true },
> > - { .format = DRM_FORMAT_XYUV8888, .num_planes = 2,
> > - .char_per_block = { 4, 1 }, .block_w = { 1, 2 }, .block_h = { 1, 1 },
> > - .hsub = 1, .vsub = 1, .is_yuv = true },
> > - { .format = DRM_FORMAT_NV12, .num_planes = 4,
> > - .char_per_block = { 1, 2, 1, 1 }, .block_w = { 1, 1, 4, 4 }, .block_h
> > = { 1, 1, 1, 1 },
> > - .hsub = 2, .vsub = 2, .is_yuv = true },
> > - { .format = DRM_FORMAT_P010, .num_planes = 4,
> > - .char_per_block = { 2, 4, 1, 1 }, .block_w = { 1, 1, 2, 2 }, .block_h
> > = { 1, 1, 1, 1 },
> > - .hsub = 2, .vsub = 2, .is_yuv = true },
> > - { .format = DRM_FORMAT_P012, .num_planes = 4,
> > - .char_per_block = { 2, 4, 1, 1 }, .block_w = { 1, 1, 2, 2 }, .block_h
> > = { 1, 1, 1, 1 },
> > - .hsub = 2, .vsub = 2, .is_yuv = true },
> > - { .format = DRM_FORMAT_P016, .num_planes = 4,
> > - .char_per_block = { 2, 4, 1, 1 }, .block_w = { 1, 1, 2, 2 }, .block_h
> > = { 1, 1, 1, 1 },
> > - .hsub = 2, .vsub = 2, .is_yuv = true },
> > -};
> > -
> > -/*
> > - * Same as gen12_ccs_formats[] above, but with additional surface used
> > - * to pass Clear Color information in plane 2 with 64 bits of data.
> > - */
> > -static const struct drm_format_info gen12_ccs_cc_formats[] = {
> > - { .format = DRM_FORMAT_XRGB8888, .depth = 24, .num_planes = 3,
> > - .char_per_block = { 4, 1, 0 }, .block_w = { 1, 2, 2 }, .block_h = {
> > 1, 1, 1 },
> > - .hsub = 1, .vsub = 1, },
> > - { .format = DRM_FORMAT_XBGR8888, .depth = 24, .num_planes = 3,
> > - .char_per_block = { 4, 1, 0 }, .block_w = { 1, 2, 2 }, .block_h = {
> > 1, 1, 1 },
> > - .hsub = 1, .vsub = 1, },
> > - { .format = DRM_FORMAT_ARGB8888, .depth = 32, .num_planes = 3,
> > - .char_per_block = { 4, 1, 0 }, .block_w = { 1, 2, 2 }, .block_h = {
> > 1, 1, 1 },
> > - .hsub = 1, .vsub = 1, .has_alpha = true },
> > - { .format = DRM_FORMAT_ABGR8888, .depth = 32, .num_planes = 3,
> > - .char_per_block = { 4, 1, 0 }, .block_w = { 1, 2, 2 }, .block_h = {
> > 1, 1, 1 },
> > - .hsub = 1, .vsub = 1, .has_alpha = true },
> > -};
> > -
> > -static const struct drm_format_info *
> > -lookup_format_info(const struct drm_format_info formats[],
> > - int num_formats, u32 format)
> > -{
> > - int i;
> > -
> > - for (i = 0; i < num_formats; i++) {
> > - if (formats[i].format == format)
> > - return &formats[i];
> > - }
> > -
> > - return NULL;
> > -}
> > -
> > -static const struct drm_format_info *
> > -intel_get_format_info(const struct drm_mode_fb_cmd2 *cmd)
> > -{
> > - switch (cmd->modifier[0]) {
> > - case I915_FORMAT_MOD_Y_TILED_CCS:
> > - case I915_FORMAT_MOD_Yf_TILED_CCS:
> > - return lookup_format_info(skl_ccs_formats,
> > - ARRAY_SIZE(skl_ccs_formats),
> > - cmd->pixel_format);
> > - case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS:
> > - case I915_FORMAT_MOD_Y_TILED_GEN12_MC_CCS:
> > - return lookup_format_info(gen12_ccs_formats,
> > - ARRAY_SIZE(gen12_ccs_formats),
> > - cmd->pixel_format);
> > - case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC:
> > - return lookup_format_info(gen12_ccs_cc_formats,
> > - ARRAY_SIZE(gen12_ccs_cc_formats),
> > - cmd->pixel_format);
> > - default:
> > - return NULL;
> > - }
> > -}
> > -
> > u32 intel_plane_fb_max_stride(struct drm_i915_private *dev_priv,
> > u32 pixel_format, u64 modifier)
> > {
> > @@ -11270,7 +11140,7 @@ intel_mode_valid_max_plane_size(struct
> > drm_i915_private *dev_priv,
> >
> > static const struct drm_mode_config_funcs intel_mode_funcs = {
> > .fb_create = intel_user_framebuffer_create,
> > - .get_format_info = intel_get_format_info,
> > + .get_format_info = intel_fb_get_format_info,
> > .output_poll_changed = intel_fbdev_output_poll_changed,
> > .mode_valid = intel_mode_valid,
> > .atomic_check = intel_atomic_check,
> > diff --git a/drivers/gpu/drm/i915/display/intel_fb.c
> > b/drivers/gpu/drm/i915/display/intel_fb.c
> > index 11a4c3e81cead..920de857ffa28 100644
> > --- a/drivers/gpu/drm/i915/display/intel_fb.c
> > +++ b/drivers/gpu/drm/i915/display/intel_fb.c
> > @@ -13,9 +13,108 @@
> >
> > #define check_array_bounds(i915, a, i) drm_WARN_ON(&(i915)->drm, (i) >=
> > ARRAY_SIZE(a))
> >
> > +/*
> > + * From the Sky Lake PRM:
> > + * "The Color Control Surface (CCS) contains the compression status of
> > + * the cache-line pairs. The compression state of the cache-line pair
> > + * is specified by 2 bits in the CCS. Each CCS cache-line represents
> > + * an area on the main surface of 16 x16 sets of 128 byte Y-tiled
> > + * cache-line-pairs. CCS is always Y tiled."
> > + *
> > + * Since cache line pairs refers to horizontally adjacent cache lines,
> > + * each cache line in the CCS corresponds to an area of 32x16 cache
> > + * lines on the main surface. Since each pixel is 4 bytes, this gives
> > + * us a ratio of one byte in the CCS for each 8x16 pixels in the
> > + * main surface.
> > + */
> > +static const struct drm_format_info skl_ccs_formats[] = {
> > + { .format = DRM_FORMAT_XRGB8888, .depth = 24, .num_planes = 2,
> > + .cpp = { 4, 1, }, .hsub = 8, .vsub = 16, },
> > + { .format = DRM_FORMAT_XBGR8888, .depth = 24, .num_planes = 2,
> > + .cpp = { 4, 1, }, .hsub = 8, .vsub = 16, },
> > + { .format = DRM_FORMAT_ARGB8888, .depth = 32, .num_planes = 2,
> > + .cpp = { 4, 1, }, .hsub = 8, .vsub = 16, .has_alpha = true, },
> > + { .format = DRM_FORMAT_ABGR8888, .depth = 32, .num_planes = 2,
> > + .cpp = { 4, 1, }, .hsub = 8, .vsub = 16, .has_alpha = true, },
> > +};
> > +
> > +/*
> > + * Gen-12 compression uses 4 bits of CCS data for each cache line pair in
> > the
> > + * main surface. And each 64B CCS cache line represents an area of 4x1
> > Y-tiles
> > + * in the main surface. With 4 byte pixels and each Y-tile having
> > dimensions of
> > + * 32x32 pixels, the ratio turns out to 1B in the CCS for every 2x32
> > pixels in
> > + * the main surface.
> > + */
> > +static const struct drm_format_info gen12_ccs_formats[] = {
> > + { .format = DRM_FORMAT_XRGB8888, .depth = 24, .num_planes = 2,
> > + .char_per_block = { 4, 1 }, .block_w = { 1, 2 }, .block_h = { 1, 1 },
> > + .hsub = 1, .vsub = 1, },
> > + { .format = DRM_FORMAT_XBGR8888, .depth = 24, .num_planes = 2,
> > + .char_per_block = { 4, 1 }, .block_w = { 1, 2 }, .block_h = { 1, 1 },
> > + .hsub = 1, .vsub = 1, },
> > + { .format = DRM_FORMAT_ARGB8888, .depth = 32, .num_planes = 2,
> > + .char_per_block = { 4, 1 }, .block_w = { 1, 2 }, .block_h = { 1, 1 },
> > + .hsub = 1, .vsub = 1, .has_alpha = true },
> > + { .format = DRM_FORMAT_ABGR8888, .depth = 32, .num_planes = 2,
> > + .char_per_block = { 4, 1 }, .block_w = { 1, 2 }, .block_h = { 1, 1 },
> > + .hsub = 1, .vsub = 1, .has_alpha = true },
> > + { .format = DRM_FORMAT_YUYV, .num_planes = 2,
> > + .char_per_block = { 2, 1 }, .block_w = { 1, 2 }, .block_h = { 1, 1 },
> > + .hsub = 2, .vsub = 1, .is_yuv = true },
> > + { .format = DRM_FORMAT_YVYU, .num_planes = 2,
> > + .char_per_block = { 2, 1 }, .block_w = { 1, 2 }, .block_h = { 1, 1 },
> > + .hsub = 2, .vsub = 1, .is_yuv = true },
> > + { .format = DRM_FORMAT_UYVY, .num_planes = 2,
> > + .char_per_block = { 2, 1 }, .block_w = { 1, 2 }, .block_h = { 1, 1 },
> > + .hsub = 2, .vsub = 1, .is_yuv = true },
> > + { .format = DRM_FORMAT_VYUY, .num_planes = 2,
> > + .char_per_block = { 2, 1 }, .block_w = { 1, 2 }, .block_h = { 1, 1 },
> > + .hsub = 2, .vsub = 1, .is_yuv = true },
> > + { .format = DRM_FORMAT_XYUV8888, .num_planes = 2,
> > + .char_per_block = { 4, 1 }, .block_w = { 1, 2 }, .block_h = { 1, 1 },
> > + .hsub = 1, .vsub = 1, .is_yuv = true },
> > + { .format = DRM_FORMAT_NV12, .num_planes = 4,
> > + .char_per_block = { 1, 2, 1, 1 }, .block_w = { 1, 1, 4, 4 }, .block_h
> > = { 1, 1, 1, 1 },
> > + .hsub = 2, .vsub = 2, .is_yuv = true },
> > + { .format = DRM_FORMAT_P010, .num_planes = 4,
> > + .char_per_block = { 2, 4, 1, 1 }, .block_w = { 1, 1, 2, 2 }, .block_h
> > = { 1, 1, 1, 1 },
> > + .hsub = 2, .vsub = 2, .is_yuv = true },
> > + { .format = DRM_FORMAT_P012, .num_planes = 4,
> > + .char_per_block = { 2, 4, 1, 1 }, .block_w = { 1, 1, 2, 2 }, .block_h
> > = { 1, 1, 1, 1 },
> > + .hsub = 2, .vsub = 2, .is_yuv = true },
> > + { .format = DRM_FORMAT_P016, .num_planes = 4,
> > + .char_per_block = { 2, 4, 1, 1 }, .block_w = { 1, 1, 2, 2 }, .block_h
> > = { 1, 1, 1, 1 },
> > + .hsub = 2, .vsub = 2, .is_yuv = true },
> > +};
> > +
> > +/*
> > + * Same as gen12_ccs_formats[] above, but with additional surface used
> > + * to pass Clear Color information in plane 2 with 64 bits of data.
> > + */
> > +static const struct drm_format_info gen12_ccs_cc_formats[] = {
> > + { .format = DRM_FORMAT_XRGB8888, .depth = 24, .num_planes = 3,
> > + .char_per_block = { 4, 1, 0 }, .block_w = { 1, 2, 2 }, .block_h = {
> > 1, 1, 1 },
> > + .hsub = 1, .vsub = 1, },
> > + { .format = DRM_FORMAT_XBGR8888, .depth = 24, .num_planes = 3,
> > + .char_per_block = { 4, 1, 0 }, .block_w = { 1, 2, 2 }, .block_h = {
> > 1, 1, 1 },
> > + .hsub = 1, .vsub = 1, },
> > + { .format = DRM_FORMAT_ARGB8888, .depth = 32, .num_planes = 3,
> > + .char_per_block = { 4, 1, 0 }, .block_w = { 1, 2, 2 }, .block_h = {
> > 1, 1, 1 },
> > + .hsub = 1, .vsub = 1, .has_alpha = true },
> > + { .format = DRM_FORMAT_ABGR8888, .depth = 32, .num_planes = 3,
> > + .char_per_block = { 4, 1, 0 }, .block_w = { 1, 2, 2 }, .block_h = {
> > 1, 1, 1 },
> > + .hsub = 1, .vsub = 1, .has_alpha = true },
> > +};
> > +
> > +#define FORMAT_OVERRIDE(format_list) \
> > + .formats = format_list, \
> > + .format_count = ARRAY_SIZE(format_list)
> > +
> > const struct intel_modifier_desc {
> > u64 id;
> > u64 display_versions;
> > + const struct drm_format_info *formats;
> > + int format_count;
> > u8 is_linear:1;
> >
> > struct {
> > @@ -49,33 +148,97 @@ const struct intel_modifier_desc {
> > .display_versions = DISPLAY_VER_MASK(9, 11),
> >
> > .ccs.type = INTEL_CCS_RC,
> > +
> > + FORMAT_OVERRIDE(skl_ccs_formats),
> > },
> > {
> > .id = I915_FORMAT_MOD_Yf_TILED_CCS,
> > .display_versions = DISPLAY_VER_MASK(9, 11),
> >
> > .ccs.type = INTEL_CCS_RC,
> > +
> > + FORMAT_OVERRIDE(skl_ccs_formats),
> > },
> > {
> > .id = I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS,
> > .display_versions = DISPLAY_VER_MASK(12, 13),
> >
> > .ccs.type = INTEL_CCS_RC,
> > +
> > + FORMAT_OVERRIDE(gen12_ccs_formats),
> > },
> > {
> > .id = I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC,
> > .display_versions = DISPLAY_VER_MASK(12, 13),
> >
> > .ccs.type = INTEL_CCS_RC_CC,
> > +
> > + FORMAT_OVERRIDE(gen12_ccs_cc_formats),
> > },
> > {
> > .id = I915_FORMAT_MOD_Y_TILED_GEN12_MC_CCS,
> > .display_versions = DISPLAY_VER_MASK(12, 13),
> >
> > .ccs.type = INTEL_CCS_MC,
> > +
> > + FORMAT_OVERRIDE(gen12_ccs_formats),
> > },
> > };
> >
> > +static const struct intel_modifier_desc *lookup_modifier_or_null(u64
> > modifier)
> > +{
> > + int i;
> > +
> > + for (i = 0; i < ARRAY_SIZE(intel_modifiers); i++)
> > + if (intel_modifiers[i].id == modifier)
> > + return &intel_modifiers[i];
> > +
> > + return NULL;
> > +}
> > +
> > +static const struct intel_modifier_desc *lookup_modifier(u64 modifier)
> > +{
> > + const struct intel_modifier_desc *md =
> > lookup_modifier_or_null(modifier);
> > +
> > + if (WARN_ON(!md))
> > + return &intel_modifiers[0];
> > +
> > + return md;
> > +}
> > +
> > +static const struct drm_format_info *
> > +lookup_format_info(const struct drm_format_info formats[],
> > + int num_formats, u32 format)
> > +{
> > + int i;
> > +
> > + for (i = 0; i < num_formats; i++) {
> > + if (formats[i].format == format)
> > + return &formats[i];
> > + }
> > +
> > + return NULL;
> > +}
> > +
> > +/**
> > + * intel_fb_get_format_info: Get a modifier specific format information
> > + * @cmd: FB add command structure
> > + *
> > + * Returns:
> > + * Returns the format information for @cmd->pixel_format specific to
> > @cmd->modifier[0],
> > + * or %NULL if the modifier doesn't override the format.
> > + */
> > +const struct drm_format_info *
> > +intel_fb_get_format_info(const struct drm_mode_fb_cmd2 *cmd)
> > +{
> > + const struct intel_modifier_desc *md =
> > lookup_modifier_or_null(cmd->modifier[0]);
> > +
> > + if (!md || !md->formats)
> > + return NULL;
> > +
> > + return lookup_format_info(md->formats, md->format_count,
> > cmd->pixel_format);
> > +}
> > +
> > static bool is_ccs_type_modifier(const struct intel_modifier_desc *md, u8
> > ccs_type)
> > {
> > return md->ccs.type & ccs_type;
> > diff --git a/drivers/gpu/drm/i915/display/intel_fb.h
> > b/drivers/gpu/drm/i915/display/intel_fb.h
> > index 5bff88ccb9372..a87c58a3219cd 100644
> > --- a/drivers/gpu/drm/i915/display/intel_fb.h
> > +++ b/drivers/gpu/drm/i915/display/intel_fb.h
> > @@ -39,6 +39,9 @@ u64 *intel_fb_plane_get_modifiers(struct drm_i915_private
> > *i915,
> > enum intel_plane_caps plane_caps);
> > bool intel_fb_plane_supports_modifier(struct intel_plane *plane, u64
> > modifier);
> >
> > +const struct drm_format_info *
> > +intel_fb_get_format_info(const struct drm_mode_fb_cmd2 *cmd);
> > +
> > bool is_surface_linear(const struct drm_framebuffer *fb, int color_plane);
> >
> > int main_to_ccs_plane(const struct drm_framebuffer *fb, int main_plane);
> > --
> > 2.27.0
>
> --
> Ville Syrjälä
> Intel
