2019-04-04 11:07:25

by James Qian Wang

[permalink] [raw]
Subject: [PATCH] drm/komeda: Added AFBC support for komeda driver

For supporting AFBC:
1. Check if the user requested modifier can be supported by display HW.
2. Check the obj->size with AFBC's requirement.
3. Configure HW according to the modifier (afbc features)

This patch depends on:
- https://patchwork.freedesktop.org/series/54448/
- https://patchwork.freedesktop.org/series/54449/
- https://patchwork.freedesktop.org/series/54450/
- https://patchwork.freedesktop.org/series/58976/
- https://patchwork.freedesktop.org/series/59000/

Signed-off-by: James Qian Wang (Arm Technology China) <[email protected]>
---
.../arm/display/komeda/d71/d71_component.c | 39 ++++++++++
.../arm/display/komeda/komeda_format_caps.c | 53 +++++++++++++
.../arm/display/komeda/komeda_format_caps.h | 5 ++
.../arm/display/komeda/komeda_framebuffer.c | 74 ++++++++++++++++++-
.../arm/display/komeda/komeda_framebuffer.h | 4 +
.../gpu/drm/arm/display/komeda/komeda_kms.c | 2 +-
.../drm/arm/display/komeda/komeda_pipeline.h | 4 +
.../display/komeda/komeda_pipeline_state.c | 18 ++++-
.../gpu/drm/arm/display/komeda/komeda_plane.c | 15 +++-
9 files changed, 209 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c b/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
index b582afcf9210..33ca1718b5cd 100644
--- a/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
+++ b/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
@@ -134,6 +134,27 @@ static u32 to_rot_ctrl(u32 rot)
return lr_ctrl;
}

+static u32 to_ad_ctrl(u64 modifier)
+{
+ u32 afbc_ctrl = AD_AEN;
+
+ if (!modifier)
+ return 0;
+
+ if ((modifier & AFBC_FORMAT_MOD_BLOCK_SIZE_MASK) ==
+ AFBC_FORMAT_MOD_BLOCK_SIZE_32x8)
+ afbc_ctrl |= AD_WB;
+
+ if (modifier & AFBC_FORMAT_MOD_YTR)
+ afbc_ctrl |= AD_YT;
+ if (modifier & AFBC_FORMAT_MOD_SPLIT)
+ afbc_ctrl |= AD_BS;
+ if (modifier & AFBC_FORMAT_MOD_TILED)
+ afbc_ctrl |= AD_TH;
+
+ return afbc_ctrl;
+}
+
static inline u32 to_d71_input_id(struct komeda_component_output *output)
{
struct komeda_component *comp = output->component;
@@ -173,6 +194,24 @@ static void d71_layer_update(struct komeda_component *c,
fb->pitches[i] & 0xFFFF);
}

+ malidp_write32(reg, AD_CONTROL, to_ad_ctrl(fb->modifier));
+ if (fb->modifier) {
+ u64 addr;
+
+ malidp_write32(reg, LAYER_AD_H_CROP, HV_CROP(st->afbc_crop_l,
+ st->afbc_crop_r));
+ malidp_write32(reg, LAYER_AD_V_CROP, HV_CROP(st->afbc_crop_t,
+ st->afbc_crop_b));
+ /* afbc 1.2 wants payload, afbc 1.0/1.1 wants end_addr */
+ if (fb->modifier & AFBC_FORMAT_MOD_TILED)
+ addr = st->addr[0] + kfb->offset_payload;
+ else
+ addr = st->addr[0] + kfb->afbc_size - 1;
+
+ malidp_write32(reg, BLK_P1_PTR_LOW, lower_32_bits(addr));
+ malidp_write32(reg, BLK_P1_PTR_HIGH, upper_32_bits(addr));
+ }
+
malidp_write32(reg, LAYER_FMT, kfb->format_caps->hw_id);
malidp_write32(reg, BLK_IN_SIZE, HV_SIZE(st->hsize, st->vsize));

diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_format_caps.c b/drivers/gpu/drm/arm/display/komeda/komeda_format_caps.c
index 1e17bd6107a4..b2195142e3f3 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_format_caps.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_format_caps.c
@@ -35,6 +35,59 @@ komeda_get_format_caps(struct komeda_format_caps_table *table,
return NULL;
}

+/* Two assumptions
+ * 1. RGB always has YTR
+ * 2. Tiled RGB always has SC
+ */
+u64 komeda_supported_modifiers[] = {
+ /* AFBC_16x16 + features: YUV+RGB both */
+ AFBC_16x16(0),
+ /* SPARSE */
+ AFBC_16x16(_SPARSE),
+ /* YTR + (SPARSE) */
+ AFBC_16x16(_YTR | _SPARSE),
+ AFBC_16x16(_YTR),
+ /* SPLIT + SPARSE + YTR RGB only */
+ /* split mode is only allowed for sparse mode */
+ AFBC_16x16(_SPLIT | _SPARSE | _YTR),
+ /* TILED + (SPARSE) */
+ /* TILED YUV format only */
+ AFBC_16x16(_TILED | _SPARSE),
+ AFBC_16x16(_TILED),
+ /* TILED + SC + (SPLIT+SPARSE | SPARSE) + (YTR) */
+ AFBC_16x16(_TILED | _SC | _SPLIT | _SPARSE | _YTR),
+ AFBC_16x16(_TILED | _SC | _SPARSE | _YTR),
+ AFBC_16x16(_TILED | _SC | _YTR),
+ /* AFBC_32x8 + features: which are RGB formats only */
+ /* YTR + (SPARSE) */
+ AFBC_32x8(_YTR | _SPARSE),
+ AFBC_32x8(_YTR),
+ /* SPLIT + SPARSE + (YTR) */
+ /* split mode is only allowed for sparse mode */
+ AFBC_32x8(_SPLIT | _SPARSE | _YTR),
+ /* TILED + SC + (SPLIT+SPARSE | SPARSE) + YTR */
+ AFBC_32x8(_TILED | _SC | _SPLIT | _SPARSE | _YTR),
+ AFBC_32x8(_TILED | _SC | _SPARSE | _YTR),
+ AFBC_32x8(_TILED | _SC | _YTR),
+ DRM_FORMAT_MOD_LINEAR,
+ DRM_FORMAT_MOD_INVALID
+};
+
+bool komeda_format_mod_supported(struct komeda_format_caps_table *table,
+ u32 layer_type, u32 fourcc, u64 modifier)
+{
+ const struct komeda_format_caps *caps;
+
+ caps = komeda_get_format_caps(table, fourcc, modifier);
+ if (!caps)
+ return false;
+
+ if (!(caps->supported_layer_types & layer_type))
+ return false;
+
+ return true;
+}
+
u32 *komeda_get_layer_fourcc_list(struct komeda_format_caps_table *table,
u32 layer_type, u32 *n_fmts)
{
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_format_caps.h b/drivers/gpu/drm/arm/display/komeda/komeda_format_caps.h
index 60f39e77b098..bc3b2df361b9 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_format_caps.h
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_format_caps.h
@@ -77,6 +77,8 @@ struct komeda_format_caps_table {
const struct komeda_format_caps *format_caps;
};

+extern u64 komeda_supported_modifiers[];
+
const struct komeda_format_caps *
komeda_get_format_caps(struct komeda_format_caps_table *table,
u32 fourcc, u64 modifier);
@@ -86,4 +88,7 @@ u32 *komeda_get_layer_fourcc_list(struct komeda_format_caps_table *table,

void komeda_put_fourcc_list(u32 *fourcc_list);

+bool komeda_format_mod_supported(struct komeda_format_caps_table *table,
+ u32 layer_type, u32 fourcc, u64 modifier);
+
#endif
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_framebuffer.c b/drivers/gpu/drm/arm/display/komeda/komeda_framebuffer.c
index 4d8160cf09c3..f842c886c73c 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_framebuffer.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_framebuffer.c
@@ -36,6 +36,75 @@ static const struct drm_framebuffer_funcs komeda_fb_funcs = {
.create_handle = komeda_fb_create_handle,
};

+static int
+komeda_fb_afbc_size_check(struct komeda_fb *kfb, struct drm_file *file,
+ const struct drm_mode_fb_cmd2 *mode_cmd)
+{
+ struct drm_framebuffer *fb = &kfb->base;
+ const struct drm_format_info *info = fb->format;
+ struct drm_gem_object *obj;
+ u32 alignment_w = 0, alignment_h = 0, alignment_header;
+ u32 n_blocks = 0, min_size = 0;
+
+ obj = drm_gem_object_lookup(file, mode_cmd->handles[0]);
+ if (!obj) {
+ DRM_DEBUG_KMS("Failed to lookup GEM object\n");
+ return -ENOENT;
+ }
+
+ switch (fb->modifier & AFBC_FORMAT_MOD_BLOCK_SIZE_MASK) {
+ case AFBC_FORMAT_MOD_BLOCK_SIZE_32x8:
+ alignment_w = 32;
+ alignment_h = 8;
+ break;
+ case AFBC_FORMAT_MOD_BLOCK_SIZE_16x16:
+ alignment_w = 16;
+ alignment_h = 16;
+ break;
+ default:
+ break;
+ }
+
+ /* tiled header afbc */
+ if (fb->modifier & AFBC_FORMAT_MOD_TILED) {
+ alignment_w *= AFBC_TH_LAYOUT_ALIGNMENT;
+ alignment_h *= AFBC_TH_LAYOUT_ALIGNMENT;
+ alignment_header = AFBC_TH_BODY_START_ALIGNMENT;
+ } else {
+ alignment_header = AFBC_BODY_START_ALIGNMENT;
+ }
+
+ kfb->aligned_w = ALIGN(fb->width, alignment_w);
+ kfb->aligned_h = ALIGN(fb->height, alignment_h);
+
+ if (fb->offsets[0] % alignment_header) {
+ DRM_DEBUG_KMS("afbc offset alignment check failed.\n");
+ goto afbc_unreference;
+ }
+
+ n_blocks = (kfb->aligned_w * kfb->aligned_h) / AFBC_SUPERBLK_PIXELS;
+ kfb->offset_payload = ALIGN(n_blocks * AFBC_HEADER_SIZE,
+ alignment_header);
+
+ kfb->afbc_size = kfb->offset_payload + n_blocks *
+ ALIGN(info->cpp[0] * AFBC_SUPERBLK_PIXELS,
+ AFBC_SUPERBLK_ALIGNMENT);
+ min_size = kfb->afbc_size + fb->offsets[0];
+ if (min_size > obj->size) {
+ DRM_DEBUG_KMS("afbc size check failed, obj_size: 0x%lx. min_size 0x%x.\n",
+ obj->size, min_size);
+ goto afbc_unreference;
+ }
+
+ fb->obj[0] = obj;
+ return 0;
+
+afbc_unreference:
+ drm_gem_object_put_unlocked(obj);
+ fb->obj[0] = NULL;
+ return -EINVAL;
+}
+
static int
komeda_fb_none_afbc_size_check(struct komeda_dev *mdev, struct komeda_fb *kfb,
struct drm_file *file,
@@ -118,7 +187,10 @@ komeda_fb_create(struct drm_device *dev, struct drm_file *file,

drm_helper_mode_fill_fb_struct(dev, &kfb->base, mode_cmd);

- ret = komeda_fb_none_afbc_size_check(mdev, kfb, file, mode_cmd);
+ if (kfb->base.modifier)
+ ret = komeda_fb_afbc_size_check(kfb, file, mode_cmd);
+ else
+ ret = komeda_fb_none_afbc_size_check(mdev, kfb, file, mode_cmd);
if (ret < 0)
goto err_cleanup;

diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_framebuffer.h b/drivers/gpu/drm/arm/display/komeda/komeda_framebuffer.h
index ea2fe190c1e3..e3bab0defd72 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_framebuffer.h
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_framebuffer.h
@@ -25,6 +25,10 @@ struct komeda_fb {
u32 aligned_w;
/** @aligned_h: aligned frame buffer height */
u32 aligned_h;
+ /** @afbc_size: minimum size of afbc */
+ u32 afbc_size;
+ /** @offset_payload: start of afbc body buffer */
+ u32 offset_payload;
};

#define to_kfb(dfb) container_of(dfb, struct komeda_fb, base)
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
index 3e58901fb776..306ea069a1b4 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
@@ -148,7 +148,7 @@ static void komeda_kms_mode_config_init(struct komeda_kms_dev *kms,
config->min_height = 0;
config->max_width = 4096;
config->max_height = 4096;
- config->allow_fb_modifiers = false;
+ config->allow_fb_modifiers = true;

config->funcs = &komeda_mode_config_funcs;
config->helper_private = &komeda_mode_config_helpers;
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.h b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.h
index 7998a1e456b7..ba5bc0810c81 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.h
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.h
@@ -235,6 +235,10 @@ struct komeda_layer_state {
/* layer specific configuration state */
u16 hsize, vsize;
u32 rot;
+ u16 afbc_crop_l;
+ u16 afbc_crop_r;
+ u16 afbc_crop_t;
+ u16 afbc_crop_b;
dma_addr_t addr[3];
};

diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
index 944dca2e3379..9b29e9a9f49c 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
@@ -289,8 +289,22 @@ komeda_layer_validate(struct komeda_layer *layer,
st = to_layer_st(c_st);

st->rot = dflow->rot;
- st->hsize = kfb->aligned_w;
- st->vsize = kfb->aligned_h;
+
+ if (fb->modifier) {
+ st->hsize = kfb->aligned_w;
+ st->vsize = kfb->aligned_h;
+ st->afbc_crop_l = dflow->in_x;
+ st->afbc_crop_r = kfb->aligned_w - dflow->in_x - dflow->in_w;
+ st->afbc_crop_t = dflow->in_y;
+ st->afbc_crop_b = kfb->aligned_h - dflow->in_y - dflow->in_h;
+ } else {
+ st->hsize = dflow->in_w;
+ st->vsize = dflow->in_h;
+ st->afbc_crop_l = 0;
+ st->afbc_crop_r = 0;
+ st->afbc_crop_t = 0;
+ st->afbc_crop_b = 0;
+ }

for (i = 0; i < fb->format->num_planes; i++)
st->addr[i] = komeda_fb_get_pixel_addr(kfb, dflow->in_x,
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_plane.c b/drivers/gpu/drm/arm/display/komeda/komeda_plane.c
index aae5e800bed4..14d68612052f 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_plane.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_plane.c
@@ -150,6 +150,18 @@ komeda_plane_atomic_destroy_state(struct drm_plane *plane,
kfree(to_kplane_st(state));
}

+static bool
+komeda_plane_format_mod_supported(struct drm_plane *plane,
+ u32 format, u64 modifier)
+{
+ struct komeda_dev *mdev = plane->dev->dev_private;
+ struct komeda_plane *kplane = to_kplane(plane);
+ u32 layer_type = kplane->layer->layer_type;
+
+ return komeda_format_mod_supported(&mdev->fmt_tbl, layer_type,
+ format, modifier);
+}
+
static const struct drm_plane_funcs komeda_plane_funcs = {
.update_plane = drm_atomic_helper_update_plane,
.disable_plane = drm_atomic_helper_disable_plane,
@@ -157,6 +169,7 @@ static const struct drm_plane_funcs komeda_plane_funcs = {
.reset = komeda_plane_reset,
.atomic_duplicate_state = komeda_plane_atomic_duplicate_state,
.atomic_destroy_state = komeda_plane_atomic_destroy_state,
+ .format_mod_supported = komeda_plane_format_mod_supported,
};

/* for komeda, which is pipeline can be share between crtcs */
@@ -209,7 +222,7 @@ static int komeda_plane_add(struct komeda_kms_dev *kms,
err = drm_universal_plane_init(&kms->base, plane,
get_possible_crtcs(kms, c->pipeline),
&komeda_plane_funcs,
- formats, n_formats, NULL,
+ formats, n_formats, komeda_supported_modifiers,
get_plane_type(kms, c),
"%s", c->name);

--
2.17.1


2019-05-16 14:00:31

by Ayan Halder

[permalink] [raw]
Subject: Re: [PATCH] drm/komeda: Added AFBC support for komeda driver

On Thu, Apr 04, 2019 at 12:06:14PM +0100, james qian wang (Arm Technology China) wrote:
> For supporting AFBC:
> 1. Check if the user requested modifier can be supported by display HW.
> 2. Check the obj->size with AFBC's requirement.
> 3. Configure HW according to the modifier (afbc features)
Can we have a bit more detailed commit message listing the various
constraints (about which combination of modifiers, format and block
sizes are valid) ?
>
> This patch depends on:
> - https://patchwork.freedesktop.org/series/54448/
> - https://patchwork.freedesktop.org/series/54449/
> - https://patchwork.freedesktop.org/series/54450/
> - https://patchwork.freedesktop.org/series/58976/
> - https://patchwork.freedesktop.org/series/59000/
>
> Signed-off-by: James Qian Wang (Arm Technology China) <[email protected]>
> ---
> .../arm/display/komeda/d71/d71_component.c | 39 ++++++++++
> .../arm/display/komeda/komeda_format_caps.c | 53 +++++++++++++
> .../arm/display/komeda/komeda_format_caps.h | 5 ++
> .../arm/display/komeda/komeda_framebuffer.c | 74 ++++++++++++++++++-
> .../arm/display/komeda/komeda_framebuffer.h | 4 +
> .../gpu/drm/arm/display/komeda/komeda_kms.c | 2 +-
> .../drm/arm/display/komeda/komeda_pipeline.h | 4 +
> .../display/komeda/komeda_pipeline_state.c | 18 ++++-
> .../gpu/drm/arm/display/komeda/komeda_plane.c | 15 +++-
> 9 files changed, 209 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c b/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
> index b582afcf9210..33ca1718b5cd 100644
> --- a/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
> +++ b/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
> @@ -134,6 +134,27 @@ static u32 to_rot_ctrl(u32 rot)
> return lr_ctrl;
> }
>
> +static u32 to_ad_ctrl(u64 modifier)
> +{
> + u32 afbc_ctrl = AD_AEN;
> +
> + if (!modifier)
> + return 0;
> +
> + if ((modifier & AFBC_FORMAT_MOD_BLOCK_SIZE_MASK) ==
> + AFBC_FORMAT_MOD_BLOCK_SIZE_32x8)
> + afbc_ctrl |= AD_WB;
> +
> + if (modifier & AFBC_FORMAT_MOD_YTR)
> + afbc_ctrl |= AD_YT;
> + if (modifier & AFBC_FORMAT_MOD_SPLIT)
> + afbc_ctrl |= AD_BS;
> + if (modifier & AFBC_FORMAT_MOD_TILED)
> + afbc_ctrl |= AD_TH;
> +
> + return afbc_ctrl;
> +}
> +
> static inline u32 to_d71_input_id(struct komeda_component_output *output)
> {
> struct komeda_component *comp = output->component;
> @@ -173,6 +194,24 @@ static void d71_layer_update(struct komeda_component *c,
> fb->pitches[i] & 0xFFFF);
> }
>
> + malidp_write32(reg, AD_CONTROL, to_ad_ctrl(fb->modifier));
> + if (fb->modifier) {
> + u64 addr;
> +
> + malidp_write32(reg, LAYER_AD_H_CROP, HV_CROP(st->afbc_crop_l,
> + st->afbc_crop_r));
> + malidp_write32(reg, LAYER_AD_V_CROP, HV_CROP(st->afbc_crop_t,
> + st->afbc_crop_b));
> + /* afbc 1.2 wants payload, afbc 1.0/1.1 wants end_addr */
> + if (fb->modifier & AFBC_FORMAT_MOD_TILED)
> + addr = st->addr[0] + kfb->offset_payload;
> + else
> + addr = st->addr[0] + kfb->afbc_size - 1;
> +
> + malidp_write32(reg, BLK_P1_PTR_LOW, lower_32_bits(addr));
> + malidp_write32(reg, BLK_P1_PTR_HIGH, upper_32_bits(addr));
> + }
> +
> malidp_write32(reg, LAYER_FMT, kfb->format_caps->hw_id);
> malidp_write32(reg, BLK_IN_SIZE, HV_SIZE(st->hsize, st->vsize));
>
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_format_caps.c b/drivers/gpu/drm/arm/display/komeda/komeda_format_caps.c
> index 1e17bd6107a4..b2195142e3f3 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_format_caps.c
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_format_caps.c
> @@ -35,6 +35,59 @@ komeda_get_format_caps(struct komeda_format_caps_table *table,
> return NULL;
> }
>
> +/* Two assumptions
> + * 1. RGB always has YTR
> + * 2. Tiled RGB always has SC
> + */
> +u64 komeda_supported_modifiers[] = {
> + /* AFBC_16x16 + features: YUV+RGB both */
> + AFBC_16x16(0),
> + /* SPARSE */
> + AFBC_16x16(_SPARSE),
> + /* YTR + (SPARSE) */
> + AFBC_16x16(_YTR | _SPARSE),
> + AFBC_16x16(_YTR),
> + /* SPLIT + SPARSE + YTR RGB only */
> + /* split mode is only allowed for sparse mode */
> + AFBC_16x16(_SPLIT | _SPARSE | _YTR),
> + /* TILED + (SPARSE) */
> + /* TILED YUV format only */
> + AFBC_16x16(_TILED | _SPARSE),
> + AFBC_16x16(_TILED),
> + /* TILED + SC + (SPLIT+SPARSE | SPARSE) + (YTR) */
> + AFBC_16x16(_TILED | _SC | _SPLIT | _SPARSE | _YTR),
> + AFBC_16x16(_TILED | _SC | _SPARSE | _YTR),
> + AFBC_16x16(_TILED | _SC | _YTR),
> + /* AFBC_32x8 + features: which are RGB formats only */
> + /* YTR + (SPARSE) */
> + AFBC_32x8(_YTR | _SPARSE),
> + AFBC_32x8(_YTR),
> + /* SPLIT + SPARSE + (YTR) */
> + /* split mode is only allowed for sparse mode */
> + AFBC_32x8(_SPLIT | _SPARSE | _YTR),
> + /* TILED + SC + (SPLIT+SPARSE | SPARSE) + YTR */
> + AFBC_32x8(_TILED | _SC | _SPLIT | _SPARSE | _YTR),
> + AFBC_32x8(_TILED | _SC | _SPARSE | _YTR),
> + AFBC_32x8(_TILED | _SC | _YTR),
> + DRM_FORMAT_MOD_LINEAR,
> + DRM_FORMAT_MOD_INVALID
> +};
> +
> +bool komeda_format_mod_supported(struct komeda_format_caps_table *table,
> + u32 layer_type, u32 fourcc, u64 modifier)
> +{
> + const struct komeda_format_caps *caps;
> +
Do we have a NULL check on 'table' anywhere? I see it gets
dereferenced in the function call below.
> + caps = komeda_get_format_caps(table, fourcc, modifier);
> + if (!caps)
> + return false;
> +
> + if (!(caps->supported_layer_types & layer_type))
> + return false;
> +
> + return true;
> +}
> +
> u32 *komeda_get_layer_fourcc_list(struct komeda_format_caps_table *table,
> u32 layer_type, u32 *n_fmts)
> {
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_format_caps.h b/drivers/gpu/drm/arm/display/komeda/komeda_format_caps.h
> index 60f39e77b098..bc3b2df361b9 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_format_caps.h
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_format_caps.h
> @@ -77,6 +77,8 @@ struct komeda_format_caps_table {
> const struct komeda_format_caps *format_caps;
> };
>
> +extern u64 komeda_supported_modifiers[];
> +
> const struct komeda_format_caps *
> komeda_get_format_caps(struct komeda_format_caps_table *table,
> u32 fourcc, u64 modifier);
> @@ -86,4 +88,7 @@ u32 *komeda_get_layer_fourcc_list(struct komeda_format_caps_table *table,
>
> void komeda_put_fourcc_list(u32 *fourcc_list);
>
> +bool komeda_format_mod_supported(struct komeda_format_caps_table *table,
> + u32 layer_type, u32 fourcc, u64 modifier);
> +
> #endif
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_framebuffer.c b/drivers/gpu/drm/arm/display/komeda/komeda_framebuffer.c
> index 4d8160cf09c3..f842c886c73c 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_framebuffer.c
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_framebuffer.c
> @@ -36,6 +36,75 @@ static const struct drm_framebuffer_funcs komeda_fb_funcs = {
> .create_handle = komeda_fb_create_handle,
> };
>
> +static int
> +komeda_fb_afbc_size_check(struct komeda_fb *kfb, struct drm_file *file,
> + const struct drm_mode_fb_cmd2 *mode_cmd)
> +{
> + struct drm_framebuffer *fb = &kfb->base;
> + const struct drm_format_info *info = fb->format;
> + struct drm_gem_object *obj;
> + u32 alignment_w = 0, alignment_h = 0, alignment_header;
> + u32 n_blocks = 0, min_size = 0;
> +
> + obj = drm_gem_object_lookup(file, mode_cmd->handles[0]);
> + if (!obj) {
> + DRM_DEBUG_KMS("Failed to lookup GEM object\n");
> + return -ENOENT;
> + }
> +
> + switch (fb->modifier & AFBC_FORMAT_MOD_BLOCK_SIZE_MASK) {
> + case AFBC_FORMAT_MOD_BLOCK_SIZE_32x8:
> + alignment_w = 32;
> + alignment_h = 8;
> + break;
> + case AFBC_FORMAT_MOD_BLOCK_SIZE_16x16:
> + alignment_w = 16;
> + alignment_h = 16;
> + break;
> + default:
Can we have something like a warn here ?
> + break;
> + }
> +
> + /* tiled header afbc */
> + if (fb->modifier & AFBC_FORMAT_MOD_TILED) {
> + alignment_w *= AFBC_TH_LAYOUT_ALIGNMENT;
> + alignment_h *= AFBC_TH_LAYOUT_ALIGNMENT;
> + alignment_header = AFBC_TH_BODY_START_ALIGNMENT;
> + } else {
> + alignment_header = AFBC_BODY_START_ALIGNMENT;
> + }
> +
> + kfb->aligned_w = ALIGN(fb->width, alignment_w);
> + kfb->aligned_h = ALIGN(fb->height, alignment_h);
> +
> + if (fb->offsets[0] % alignment_header) {
> + DRM_DEBUG_KMS("afbc offset alignment check failed.\n");
> + goto afbc_unreference;
> + }
> +
> + n_blocks = (kfb->aligned_w * kfb->aligned_h) / AFBC_SUPERBLK_PIXELS;
> + kfb->offset_payload = ALIGN(n_blocks * AFBC_HEADER_SIZE,
> + alignment_header);
> +
> + kfb->afbc_size = kfb->offset_payload + n_blocks *
> + ALIGN(info->cpp[0] * AFBC_SUPERBLK_PIXELS,
> + AFBC_SUPERBLK_ALIGNMENT);
> + min_size = kfb->afbc_size + fb->offsets[0];
> + if (min_size > obj->size) {
> + DRM_DEBUG_KMS("afbc size check failed, obj_size: 0x%lx. min_size 0x%x.\n",
> + obj->size, min_size);
> + goto afbc_unreference;
> + }
> +
> + fb->obj[0] = obj;
> + return 0;
> +
> +afbc_unreference:
Why not call it afbc_fail ?
> + drm_gem_object_put_unlocked(obj);
> + fb->obj[0] = NULL;
> + return -EINVAL;
> +}
> +
> static int
> komeda_fb_none_afbc_size_check(struct komeda_dev *mdev, struct komeda_fb *kfb,
> struct drm_file *file,
> @@ -118,7 +187,10 @@ komeda_fb_create(struct drm_device *dev, struct drm_file *file,
>
> drm_helper_mode_fill_fb_struct(dev, &kfb->base, mode_cmd);
>
> - ret = komeda_fb_none_afbc_size_check(mdev, kfb, file, mode_cmd);
> + if (kfb->base.modifier)
> + ret = komeda_fb_afbc_size_check(kfb, file, mode_cmd);
> + else
> + ret = komeda_fb_none_afbc_size_check(mdev, kfb, file, mode_cmd);
> if (ret < 0)
> goto err_cleanup;
>
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_framebuffer.h b/drivers/gpu/drm/arm/display/komeda/komeda_framebuffer.h
> index ea2fe190c1e3..e3bab0defd72 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_framebuffer.h
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_framebuffer.h
> @@ -25,6 +25,10 @@ struct komeda_fb {
> u32 aligned_w;
> /** @aligned_h: aligned frame buffer height */
> u32 aligned_h;
> + /** @afbc_size: minimum size of afbc */
> + u32 afbc_size;
> + /** @offset_payload: start of afbc body buffer */
> + u32 offset_payload;
> };
>
> #define to_kfb(dfb) container_of(dfb, struct komeda_fb, base)
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
> index 3e58901fb776..306ea069a1b4 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
> @@ -148,7 +148,7 @@ static void komeda_kms_mode_config_init(struct komeda_kms_dev *kms,
> config->min_height = 0;
> config->max_width = 4096;
> config->max_height = 4096;
> - config->allow_fb_modifiers = false;
> + config->allow_fb_modifiers = true;
>
> config->funcs = &komeda_mode_config_funcs;
> config->helper_private = &komeda_mode_config_helpers;
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.h b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.h
> index 7998a1e456b7..ba5bc0810c81 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.h
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.h
> @@ -235,6 +235,10 @@ struct komeda_layer_state {
> /* layer specific configuration state */
> u16 hsize, vsize;
> u32 rot;
> + u16 afbc_crop_l;
> + u16 afbc_crop_r;
> + u16 afbc_crop_t;
> + u16 afbc_crop_b;
> dma_addr_t addr[3];
> };
>
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
> index 944dca2e3379..9b29e9a9f49c 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
> @@ -289,8 +289,22 @@ komeda_layer_validate(struct komeda_layer *layer,
> st = to_layer_st(c_st);
>
> st->rot = dflow->rot;
> - st->hsize = kfb->aligned_w;
> - st->vsize = kfb->aligned_h;
> +
Can you put some comments here explaining the snippet below?
> + if (fb->modifier) {
> + st->hsize = kfb->aligned_w;
> + st->vsize = kfb->aligned_h;
> + st->afbc_crop_l = dflow->in_x;
> + st->afbc_crop_r = kfb->aligned_w - dflow->in_x - dflow->in_w;
> + st->afbc_crop_t = dflow->in_y;
> + st->afbc_crop_b = kfb->aligned_h - dflow->in_y - dflow->in_h;
> + } else {
> + st->hsize = dflow->in_w;
> + st->vsize = dflow->in_h;
> + st->afbc_crop_l = 0;
> + st->afbc_crop_r = 0;
> + st->afbc_crop_t = 0;
> + st->afbc_crop_b = 0;
> + }
>
> for (i = 0; i < fb->format->num_planes; i++)
> st->addr[i] = komeda_fb_get_pixel_addr(kfb, dflow->in_x,
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_plane.c b/drivers/gpu/drm/arm/display/komeda/komeda_plane.c
> index aae5e800bed4..14d68612052f 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_plane.c
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_plane.c
> @@ -150,6 +150,18 @@ komeda_plane_atomic_destroy_state(struct drm_plane *plane,
> kfree(to_kplane_st(state));
> }
>
> +static bool
> +komeda_plane_format_mod_supported(struct drm_plane *plane,
> + u32 format, u64 modifier)
> +{
> + struct komeda_dev *mdev = plane->dev->dev_private;
> + struct komeda_plane *kplane = to_kplane(plane);
> + u32 layer_type = kplane->layer->layer_type;
> +
> + return komeda_format_mod_supported(&mdev->fmt_tbl, layer_type,
> + format, modifier);
> +}
> +
> static const struct drm_plane_funcs komeda_plane_funcs = {
> .update_plane = drm_atomic_helper_update_plane,
> .disable_plane = drm_atomic_helper_disable_plane,
> @@ -157,6 +169,7 @@ static const struct drm_plane_funcs komeda_plane_funcs = {
> .reset = komeda_plane_reset,
> .atomic_duplicate_state = komeda_plane_atomic_duplicate_state,
> .atomic_destroy_state = komeda_plane_atomic_destroy_state,
> + .format_mod_supported = komeda_plane_format_mod_supported,
> };
>
> /* for komeda, which is pipeline can be share between crtcs */
> @@ -209,7 +222,7 @@ static int komeda_plane_add(struct komeda_kms_dev *kms,
> err = drm_universal_plane_init(&kms->base, plane,
> get_possible_crtcs(kms, c->pipeline),
> &komeda_plane_funcs,
> - formats, n_formats, NULL,
> + formats, n_formats, komeda_supported_modifiers,
> get_plane_type(kms, c),
> "%s", c->name);
>
> --
> 2.17.1

2019-05-21 08:47:27

by James Qian Wang

[permalink] [raw]
Subject: Re: [PATCH] drm/komeda: Added AFBC support for komeda driver

On Thu, May 16, 2019 at 09:57:49PM +0800, Ayan Halder wrote:
> On Thu, Apr 04, 2019 at 12:06:14PM +0100, james qian wang (Arm Technology China) wrote:
> > For supporting AFBC:
> > 1. Check if the user requested modifier can be supported by display HW.
> > 2. Check the obj->size with AFBC's requirement.
> > 3. Configure HW according to the modifier (afbc features)
> Can we have a bit more detailed commit message listing the various
> constraints (about which combination of modifiers, format and block
> sizes are valid) ?

- First for the acceptable combination of modifiers for the specific format
which is desribed by komeda format handling patch
https://patchwork.freedesktop.org/patch/274057/?series=54681&rev=1

and the struct komeda_format_caps has a detailed doc information.

> >
> > This patch depends on:
> > - https://patchwork.freedesktop.org/series/54448/
> > - https://patchwork.freedesktop.org/series/54449/
> > - https://patchwork.freedesktop.org/series/54450/
> > - https://patchwork.freedesktop.org/series/58976/
> > - https://patchwork.freedesktop.org/series/59000/
> >
> > Signed-off-by: James Qian Wang (Arm Technology China) <[email protected]>
> > ---
> > .../arm/display/komeda/d71/d71_component.c | 39 ++++++++++
> > .../arm/display/komeda/komeda_format_caps.c | 53 +++++++++++++
> > .../arm/display/komeda/komeda_format_caps.h | 5 ++
> > .../arm/display/komeda/komeda_framebuffer.c | 74 ++++++++++++++++++-
> > .../arm/display/komeda/komeda_framebuffer.h | 4 +
> > .../gpu/drm/arm/display/komeda/komeda_kms.c | 2 +-
> > .../drm/arm/display/komeda/komeda_pipeline.h | 4 +
> > .../display/komeda/komeda_pipeline_state.c | 18 ++++-
> > .../gpu/drm/arm/display/komeda/komeda_plane.c | 15 +++-
> > 9 files changed, 209 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c b/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
> > index b582afcf9210..33ca1718b5cd 100644
> > --- a/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
> > +++ b/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
> > @@ -134,6 +134,27 @@ static u32 to_rot_ctrl(u32 rot)
> > return lr_ctrl;
> > }
> >
> > +static u32 to_ad_ctrl(u64 modifier)
> > +{
> > + u32 afbc_ctrl = AD_AEN;
> > +
> > + if (!modifier)
> > + return 0;
> > +
> > + if ((modifier & AFBC_FORMAT_MOD_BLOCK_SIZE_MASK) ==
> > + AFBC_FORMAT_MOD_BLOCK_SIZE_32x8)
> > + afbc_ctrl |= AD_WB;
> > +
> > + if (modifier & AFBC_FORMAT_MOD_YTR)
> > + afbc_ctrl |= AD_YT;
> > + if (modifier & AFBC_FORMAT_MOD_SPLIT)
> > + afbc_ctrl |= AD_BS;
> > + if (modifier & AFBC_FORMAT_MOD_TILED)
> > + afbc_ctrl |= AD_TH;
> > +
> > + return afbc_ctrl;
> > +}
> > +
> > static inline u32 to_d71_input_id(struct komeda_component_output *output)
> > {
> > struct komeda_component *comp = output->component;
> > @@ -173,6 +194,24 @@ static void d71_layer_update(struct komeda_component *c,
> > fb->pitches[i] & 0xFFFF);
> > }
> >
> > + malidp_write32(reg, AD_CONTROL, to_ad_ctrl(fb->modifier));
> > + if (fb->modifier) {
> > + u64 addr;
> > +
> > + malidp_write32(reg, LAYER_AD_H_CROP, HV_CROP(st->afbc_crop_l,
> > + st->afbc_crop_r));
> > + malidp_write32(reg, LAYER_AD_V_CROP, HV_CROP(st->afbc_crop_t,
> > + st->afbc_crop_b));
> > + /* afbc 1.2 wants payload, afbc 1.0/1.1 wants end_addr */
> > + if (fb->modifier & AFBC_FORMAT_MOD_TILED)
> > + addr = st->addr[0] + kfb->offset_payload;
> > + else
> > + addr = st->addr[0] + kfb->afbc_size - 1;
> > +
> > + malidp_write32(reg, BLK_P1_PTR_LOW, lower_32_bits(addr));
> > + malidp_write32(reg, BLK_P1_PTR_HIGH, upper_32_bits(addr));
> > + }
> > +
> > malidp_write32(reg, LAYER_FMT, kfb->format_caps->hw_id);
> > malidp_write32(reg, BLK_IN_SIZE, HV_SIZE(st->hsize, st->vsize));
> >
> > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_format_caps.c b/drivers/gpu/drm/arm/display/komeda/komeda_format_caps.c
> > index 1e17bd6107a4..b2195142e3f3 100644
> > --- a/drivers/gpu/drm/arm/display/komeda/komeda_format_caps.c
> > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_format_caps.c
> > @@ -35,6 +35,59 @@ komeda_get_format_caps(struct komeda_format_caps_table *table,
> > return NULL;
> > }
> >
> > +/* Two assumptions
> > + * 1. RGB always has YTR
> > + * 2. Tiled RGB always has SC
> > + */
> > +u64 komeda_supported_modifiers[] = {
> > + /* AFBC_16x16 + features: YUV+RGB both */
> > + AFBC_16x16(0),
> > + /* SPARSE */
> > + AFBC_16x16(_SPARSE),
> > + /* YTR + (SPARSE) */
> > + AFBC_16x16(_YTR | _SPARSE),
> > + AFBC_16x16(_YTR),
> > + /* SPLIT + SPARSE + YTR RGB only */
> > + /* split mode is only allowed for sparse mode */
> > + AFBC_16x16(_SPLIT | _SPARSE | _YTR),
> > + /* TILED + (SPARSE) */
> > + /* TILED YUV format only */
> > + AFBC_16x16(_TILED | _SPARSE),
> > + AFBC_16x16(_TILED),
> > + /* TILED + SC + (SPLIT+SPARSE | SPARSE) + (YTR) */
> > + AFBC_16x16(_TILED | _SC | _SPLIT | _SPARSE | _YTR),
> > + AFBC_16x16(_TILED | _SC | _SPARSE | _YTR),
> > + AFBC_16x16(_TILED | _SC | _YTR),
> > + /* AFBC_32x8 + features: which are RGB formats only */
> > + /* YTR + (SPARSE) */
> > + AFBC_32x8(_YTR | _SPARSE),
> > + AFBC_32x8(_YTR),
> > + /* SPLIT + SPARSE + (YTR) */
> > + /* split mode is only allowed for sparse mode */
> > + AFBC_32x8(_SPLIT | _SPARSE | _YTR),
> > + /* TILED + SC + (SPLIT+SPARSE | SPARSE) + YTR */
> > + AFBC_32x8(_TILED | _SC | _SPLIT | _SPARSE | _YTR),
> > + AFBC_32x8(_TILED | _SC | _SPARSE | _YTR),
> > + AFBC_32x8(_TILED | _SC | _YTR),
> > + DRM_FORMAT_MOD_LINEAR,
> > + DRM_FORMAT_MOD_INVALID
> > +};
> > +
> > +bool komeda_format_mod_supported(struct komeda_format_caps_table *table,
> > + u32 layer_type, u32 fourcc, u64 modifier)
> > +{
> > + const struct komeda_format_caps *caps;
> > +
> Do we have a NULL check on 'table' anywhere? I see it gets
> dereferenced in the function call below.

No need check, since table is embebbed in komeda_dev, which is
komeda_dev is referenced by &mdev->table.

Any dereferenced should be a bug, can you point me the place.

> > + caps = komeda_get_format_caps(table, fourcc, modifier);
> > + if (!caps)
> > + return false;
> > +
> > + if (!(caps->supported_layer_types & layer_type))
> > + return false;
> > +
> > + return true;
> > +}
> > +
> > u32 *komeda_get_layer_fourcc_list(struct komeda_format_caps_table *table,
> > u32 layer_type, u32 *n_fmts)
> > {
> > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_format_caps.h b/drivers/gpu/drm/arm/display/komeda/komeda_format_caps.h
> > index 60f39e77b098..bc3b2df361b9 100644
> > --- a/drivers/gpu/drm/arm/display/komeda/komeda_format_caps.h
> > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_format_caps.h
> > @@ -77,6 +77,8 @@ struct komeda_format_caps_table {
> > const struct komeda_format_caps *format_caps;
> > };
> >
> > +extern u64 komeda_supported_modifiers[];
> > +
> > const struct komeda_format_caps *
> > komeda_get_format_caps(struct komeda_format_caps_table *table,
> > u32 fourcc, u64 modifier);
> > @@ -86,4 +88,7 @@ u32 *komeda_get_layer_fourcc_list(struct komeda_format_caps_table *table,
> >
> > void komeda_put_fourcc_list(u32 *fourcc_list);
> >
> > +bool komeda_format_mod_supported(struct komeda_format_caps_table *table,
> > + u32 layer_type, u32 fourcc, u64 modifier);
> > +
> > #endif
> > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_framebuffer.c b/drivers/gpu/drm/arm/display/komeda/komeda_framebuffer.c
> > index 4d8160cf09c3..f842c886c73c 100644
> > --- a/drivers/gpu/drm/arm/display/komeda/komeda_framebuffer.c
> > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_framebuffer.c
> > @@ -36,6 +36,75 @@ static const struct drm_framebuffer_funcs komeda_fb_funcs = {
> > .create_handle = komeda_fb_create_handle,
> > };
> >
> > +static int
> > +komeda_fb_afbc_size_check(struct komeda_fb *kfb, struct drm_file *file,
> > + const struct drm_mode_fb_cmd2 *mode_cmd)
> > +{
> > + struct drm_framebuffer *fb = &kfb->base;
> > + const struct drm_format_info *info = fb->format;
> > + struct drm_gem_object *obj;
> > + u32 alignment_w = 0, alignment_h = 0, alignment_header;
> > + u32 n_blocks = 0, min_size = 0;
> > +
> > + obj = drm_gem_object_lookup(file, mode_cmd->handles[0]);
> > + if (!obj) {
> > + DRM_DEBUG_KMS("Failed to lookup GEM object\n");
> > + return -ENOENT;
> > + }
> > +
> > + switch (fb->modifier & AFBC_FORMAT_MOD_BLOCK_SIZE_MASK) {
> > + case AFBC_FORMAT_MOD_BLOCK_SIZE_32x8:
> > + alignment_w = 32;
> > + alignment_h = 8;
> > + break;
> > + case AFBC_FORMAT_MOD_BLOCK_SIZE_16x16:
> > + alignment_w = 16;
> > + alignment_h = 16;
> > + break;
> > + default:
> Can we have something like a warn here ?

will add a WARN here.

> > + break;
> > + }
> > +
> > + /* tiled header afbc */
> > + if (fb->modifier & AFBC_FORMAT_MOD_TILED) {
> > + alignment_w *= AFBC_TH_LAYOUT_ALIGNMENT;
> > + alignment_h *= AFBC_TH_LAYOUT_ALIGNMENT;
> > + alignment_header = AFBC_TH_BODY_START_ALIGNMENT;
> > + } else {
> > + alignment_header = AFBC_BODY_START_ALIGNMENT;
> > + }
> > +
> > + kfb->aligned_w = ALIGN(fb->width, alignment_w);
> > + kfb->aligned_h = ALIGN(fb->height, alignment_h);
> > +
> > + if (fb->offsets[0] % alignment_header) {
> > + DRM_DEBUG_KMS("afbc offset alignment check failed.\n");
> > + goto afbc_unreference;
> > + }
> > +
> > + n_blocks = (kfb->aligned_w * kfb->aligned_h) / AFBC_SUPERBLK_PIXELS;
> > + kfb->offset_payload = ALIGN(n_blocks * AFBC_HEADER_SIZE,
> > + alignment_header);
> > +
> > + kfb->afbc_size = kfb->offset_payload + n_blocks *
> > + ALIGN(info->cpp[0] * AFBC_SUPERBLK_PIXELS,
> > + AFBC_SUPERBLK_ALIGNMENT);
> > + min_size = kfb->afbc_size + fb->offsets[0];
> > + if (min_size > obj->size) {
> > + DRM_DEBUG_KMS("afbc size check failed, obj_size: 0x%lx. min_size 0x%x.\n",
> > + obj->size, min_size);
> > + goto afbc_unreference;
> > + }
> > +
> > + fb->obj[0] = obj;
> > + return 0;
> > +
> > +afbc_unreference:
> Why not call it afbc_fail ?

no problem :)

> > + drm_gem_object_put_unlocked(obj);
> > + fb->obj[0] = NULL;
> > + return -EINVAL;
> > +}
> > +
> > static int
> > komeda_fb_none_afbc_size_check(struct komeda_dev *mdev, struct komeda_fb *kfb,
> > struct drm_file *file,
> > @@ -118,7 +187,10 @@ komeda_fb_create(struct drm_device *dev, struct drm_file *file,
> >
> > drm_helper_mode_fill_fb_struct(dev, &kfb->base, mode_cmd);
> >
> > - ret = komeda_fb_none_afbc_size_check(mdev, kfb, file, mode_cmd);
> > + if (kfb->base.modifier)
> > + ret = komeda_fb_afbc_size_check(kfb, file, mode_cmd);
> > + else
> > + ret = komeda_fb_none_afbc_size_check(mdev, kfb, file, mode_cmd);
> > if (ret < 0)
> > goto err_cleanup;
> >
> > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_framebuffer.h b/drivers/gpu/drm/arm/display/komeda/komeda_framebuffer.h
> > index ea2fe190c1e3..e3bab0defd72 100644
> > --- a/drivers/gpu/drm/arm/display/komeda/komeda_framebuffer.h
> > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_framebuffer.h
> > @@ -25,6 +25,10 @@ struct komeda_fb {
> > u32 aligned_w;
> > /** @aligned_h: aligned frame buffer height */
> > u32 aligned_h;
> > + /** @afbc_size: minimum size of afbc */
> > + u32 afbc_size;
> > + /** @offset_payload: start of afbc body buffer */
> > + u32 offset_payload;
> > };
> >
> > #define to_kfb(dfb) container_of(dfb, struct komeda_fb, base)
> > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
> > index 3e58901fb776..306ea069a1b4 100644
> > --- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
> > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
> > @@ -148,7 +148,7 @@ static void komeda_kms_mode_config_init(struct komeda_kms_dev *kms,
> > config->min_height = 0;
> > config->max_width = 4096;
> > config->max_height = 4096;
> > - config->allow_fb_modifiers = false;
> > + config->allow_fb_modifiers = true;
> >
> > config->funcs = &komeda_mode_config_funcs;
> > config->helper_private = &komeda_mode_config_helpers;
> > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.h b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.h
> > index 7998a1e456b7..ba5bc0810c81 100644
> > --- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.h
> > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.h
> > @@ -235,6 +235,10 @@ struct komeda_layer_state {
> > /* layer specific configuration state */
> > u16 hsize, vsize;
> > u32 rot;
> > + u16 afbc_crop_l;
> > + u16 afbc_crop_r;
> > + u16 afbc_crop_t;
> > + u16 afbc_crop_b;
> > dma_addr_t addr[3];
> > };
> >
> > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
> > index 944dca2e3379..9b29e9a9f49c 100644
> > --- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
> > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
> > @@ -289,8 +289,22 @@ komeda_layer_validate(struct komeda_layer *layer,
> > st = to_layer_st(c_st);
> >
> > st->rot = dflow->rot;
> > - st->hsize = kfb->aligned_w;
> > - st->vsize = kfb->aligned_h;
> > +
> Can you put some comments here explaining the snippet below?
> > + if (fb->modifier) {
> > + st->hsize = kfb->aligned_w;
> > + st->vsize = kfb->aligned_h;
> > + st->afbc_crop_l = dflow->in_x;
> > + st->afbc_crop_r = kfb->aligned_w - dflow->in_x - dflow->in_w;
> > + st->afbc_crop_t = dflow->in_y;
> > + st->afbc_crop_b = kfb->aligned_h - dflow->in_y - dflow->in_h;
> > + } else {
> > + st->hsize = dflow->in_w;
> > + st->vsize = dflow->in_h;
> > + st->afbc_crop_l = 0;
> > + st->afbc_crop_r = 0;
> > + st->afbc_crop_t = 0;
> > + st->afbc_crop_b = 0;
> > + }
> >
> > for (i = 0; i < fb->format->num_planes; i++)
> > st->addr[i] = komeda_fb_get_pixel_addr(kfb, dflow->in_x,
> > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_plane.c b/drivers/gpu/drm/arm/display/komeda/komeda_plane.c
> > index aae5e800bed4..14d68612052f 100644
> > --- a/drivers/gpu/drm/arm/display/komeda/komeda_plane.c
> > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_plane.c
> > @@ -150,6 +150,18 @@ komeda_plane_atomic_destroy_state(struct drm_plane *plane,
> > kfree(to_kplane_st(state));
> > }
> >
> > +static bool
> > +komeda_plane_format_mod_supported(struct drm_plane *plane,
> > + u32 format, u64 modifier)
> > +{
> > + struct komeda_dev *mdev = plane->dev->dev_private;
> > + struct komeda_plane *kplane = to_kplane(plane);
> > + u32 layer_type = kplane->layer->layer_type;
> > +
> > + return komeda_format_mod_supported(&mdev->fmt_tbl, layer_type,
> > + format, modifier);
> > +}
> > +
> > static const struct drm_plane_funcs komeda_plane_funcs = {
> > .update_plane = drm_atomic_helper_update_plane,
> > .disable_plane = drm_atomic_helper_disable_plane,
> > @@ -157,6 +169,7 @@ static const struct drm_plane_funcs komeda_plane_funcs = {
> > .reset = komeda_plane_reset,
> > .atomic_duplicate_state = komeda_plane_atomic_duplicate_state,
> > .atomic_destroy_state = komeda_plane_atomic_destroy_state,
> > + .format_mod_supported = komeda_plane_format_mod_supported,
> > };
> >
> > /* for komeda, which is pipeline can be share between crtcs */
> > @@ -209,7 +222,7 @@ static int komeda_plane_add(struct komeda_kms_dev *kms,
> > err = drm_universal_plane_init(&kms->base, plane,
> > get_possible_crtcs(kms, c->pipeline),
> > &komeda_plane_funcs,
> > - formats, n_formats, NULL,
> > + formats, n_formats, komeda_supported_modifiers,
> > get_plane_type(kms, c),
> > "%s", c->name);
> >
> > --
> > 2.17.1


2019-05-24 11:12:52

by Brian Starkey

[permalink] [raw]
Subject: Re: [PATCH] drm/komeda: Added AFBC support for komeda driver

Hi,

On Tue, May 21, 2019 at 09:45:58AM +0100, james qian wang (Arm Technology China) wrote:
> On Thu, May 16, 2019 at 09:57:49PM +0800, Ayan Halder wrote:
> > On Thu, Apr 04, 2019 at 12:06:14PM +0100, james qian wang (Arm Technology China) wrote:
> > >
> > > +static int
> > > +komeda_fb_afbc_size_check(struct komeda_fb *kfb, struct drm_file *file,
> > > + const struct drm_mode_fb_cmd2 *mode_cmd)
> > > +{
> > > + struct drm_framebuffer *fb = &kfb->base;
> > > + const struct drm_format_info *info = fb->format;
> > > + struct drm_gem_object *obj;
> > > + u32 alignment_w = 0, alignment_h = 0, alignment_header;
> > > + u32 n_blocks = 0, min_size = 0;
> > > +
> > > + obj = drm_gem_object_lookup(file, mode_cmd->handles[0]);
> > > + if (!obj) {
> > > + DRM_DEBUG_KMS("Failed to lookup GEM object\n");
> > > + return -ENOENT;
> > > + }
> > > +
> > > + switch (fb->modifier & AFBC_FORMAT_MOD_BLOCK_SIZE_MASK) {
> > > + case AFBC_FORMAT_MOD_BLOCK_SIZE_32x8:
> > > + alignment_w = 32;
> > > + alignment_h = 8;
> > > + break;
> > > + case AFBC_FORMAT_MOD_BLOCK_SIZE_16x16:
> > > + alignment_w = 16;
> > > + alignment_h = 16;
> > > + break;
> > > + default:
> > Can we have something like a warn here ?
>
> will add a WARN here.
>

I think it's better not to. fb->modifier comes from
userspace, so a malicious app could spam us with WARNs, effectively
dos-ing the system. -EINVAL should be sufficient.

Thanks,
-Brian

2019-05-24 12:14:14

by Ville Syrjälä

[permalink] [raw]
Subject: Re: [PATCH] drm/komeda: Added AFBC support for komeda driver

On Fri, May 24, 2019 at 11:10:09AM +0000, Brian Starkey wrote:
> Hi,
>
> On Tue, May 21, 2019 at 09:45:58AM +0100, james qian wang (Arm Technology China) wrote:
> > On Thu, May 16, 2019 at 09:57:49PM +0800, Ayan Halder wrote:
> > > On Thu, Apr 04, 2019 at 12:06:14PM +0100, james qian wang (Arm Technology China) wrote:
> > > >
> > > > +static int
> > > > +komeda_fb_afbc_size_check(struct komeda_fb *kfb, struct drm_file *file,
> > > > + const struct drm_mode_fb_cmd2 *mode_cmd)
> > > > +{
> > > > + struct drm_framebuffer *fb = &kfb->base;
> > > > + const struct drm_format_info *info = fb->format;
> > > > + struct drm_gem_object *obj;
> > > > + u32 alignment_w = 0, alignment_h = 0, alignment_header;
> > > > + u32 n_blocks = 0, min_size = 0;
> > > > +
> > > > + obj = drm_gem_object_lookup(file, mode_cmd->handles[0]);
> > > > + if (!obj) {
> > > > + DRM_DEBUG_KMS("Failed to lookup GEM object\n");
> > > > + return -ENOENT;
> > > > + }
> > > > +
> > > > + switch (fb->modifier & AFBC_FORMAT_MOD_BLOCK_SIZE_MASK) {
> > > > + case AFBC_FORMAT_MOD_BLOCK_SIZE_32x8:
> > > > + alignment_w = 32;
> > > > + alignment_h = 8;
> > > > + break;
> > > > + case AFBC_FORMAT_MOD_BLOCK_SIZE_16x16:
> > > > + alignment_w = 16;
> > > > + alignment_h = 16;
> > > > + break;
> > > > + default:
> > > Can we have something like a warn here ?
> >
> > will add a WARN here.
> >
>
> I think it's better not to. fb->modifier comes from
> userspace, so a malicious app could spam us with WARNs, effectively
> dos-ing the system. -EINVAL should be sufficient.

Should probably check that the entire modifier+format is
actually valid. Otherwise you risk passing on a bogus
modifier deeper into the driver which may trigger
interesting bugs.

Also theoretically (however unlikely) some broken userspace
might start to depend on the ability to create framebuffers
with crap modifiers, which could later break if you change
the way you handle the modifiers. Then you're stuck between
the rock and hard place because you can't break existing
userspace but you still want to change the way modifiers
are handled in the kernel.

Best not give userspace too much rope IMO. Two ways to go about
that:
1) drm_any_plane_has_format() (assumes your .format_mod_supported()
does its job properly)
2) roll your own

--
Ville Syrj?l?
Intel

2019-05-27 07:30:57

by James Qian Wang

[permalink] [raw]
Subject: Re: [PATCH] drm/komeda: Added AFBC support for komeda driver

On Fri, May 24, 2019 at 03:12:26PM +0300, Ville Syrj?l? wrote:
> On Fri, May 24, 2019 at 11:10:09AM +0000, Brian Starkey wrote:
> > Hi,
> >
> > On Tue, May 21, 2019 at 09:45:58AM +0100, james qian wang (Arm Technology China) wrote:
> > > On Thu, May 16, 2019 at 09:57:49PM +0800, Ayan Halder wrote:
> > > > On Thu, Apr 04, 2019 at 12:06:14PM +0100, james qian wang (Arm Technology China) wrote:
> > > > >
> > > > > +static int
> > > > > +komeda_fb_afbc_size_check(struct komeda_fb *kfb, struct drm_file *file,
> > > > > + const struct drm_mode_fb_cmd2 *mode_cmd)
> > > > > +{
> > > > > + struct drm_framebuffer *fb = &kfb->base;
> > > > > + const struct drm_format_info *info = fb->format;
> > > > > + struct drm_gem_object *obj;
> > > > > + u32 alignment_w = 0, alignment_h = 0, alignment_header;
> > > > > + u32 n_blocks = 0, min_size = 0;
> > > > > +
> > > > > + obj = drm_gem_object_lookup(file, mode_cmd->handles[0]);
> > > > > + if (!obj) {
> > > > > + DRM_DEBUG_KMS("Failed to lookup GEM object\n");
> > > > > + return -ENOENT;
> > > > > + }
> > > > > +
> > > > > + switch (fb->modifier & AFBC_FORMAT_MOD_BLOCK_SIZE_MASK) {
> > > > > + case AFBC_FORMAT_MOD_BLOCK_SIZE_32x8:
> > > > > + alignment_w = 32;
> > > > > + alignment_h = 8;
> > > > > + break;
> > > > > + case AFBC_FORMAT_MOD_BLOCK_SIZE_16x16:
> > > > > + alignment_w = 16;
> > > > > + alignment_h = 16;
> > > > > + break;
> > > > > + default:
> > > > Can we have something like a warn here ?
> > >
> > > will add a WARN here.
> > >
> >
> > I think it's better not to. fb->modifier comes from
> > userspace, so a malicious app could spam us with WARNs, effectively
> > dos-ing the system. -EINVAL should be sufficient.
>
> Should probably check that the entire modifier+format is
> actually valid. Otherwise you risk passing on a bogus
> modifier deeper into the driver which may trigger
> interesting bugs.
>
> Also theoretically (however unlikely) some broken userspace
> might start to depend on the ability to create framebuffers
> with crap modifiers, which could later break if you change
> the way you handle the modifiers. Then you're stuck between
> the rock and hard place because you can't break existing
> userspace but you still want to change the way modifiers
> are handled in the kernel.
>
> Best not give userspace too much rope IMO. Two ways to go about
> that:
> 1) drm_any_plane_has_format() (assumes your .format_mod_supported()
> does its job properly)
> 2) roll your own
>
> --
> Ville Syrj?l?
> Intel

Hi Brian & Ville:

komed has a format+modifier check before the fb size check.
and for komeda_fb_create, the first step is do the format+modifier
check, the size check is the furthur check after the such format
valid check. and the detailed fb_create is like:

struct drm_framebuffer *
komeda_fb_create(struct drm_device *dev, struct drm_file *file,
const struct drm_mode_fb_cmd2 *mode_cmd)
{
...
/* Step 1: format+modifier valid check, if it can not be support,
* get_format_caps will return a NULL ptr.
*/
kfb->format_caps = komeda_get_format_caps(&mdev->fmt_tbl,
mode_cmd->pixel_format,
mode_cmd->modifier[0]);
if (!kfb->format_caps) {
DRM_DEBUG_KMS("FMT %x is not supported.\n",
mode_cmd->pixel_format);
kfree(kfb);
return ERR_PTR(-EINVAL);
}

drm_helper_mode_fill_fb_struct(dev, &kfb->base, mode_cmd);

/* step 2, do the size check */
if (kfb->base.modifier)
ret = komeda_fb_afbc_size_check(kfb, file, mode_cmd);
else
ret = komeda_fb_none_afbc_size_check(mdev, kfb, file, mode_cmd);
if (ret < 0)
goto err_cleanup;

...
}

So theoretically, the WARN in step2 is redundant if get_format_caps
function has no problem. :). the WARN here is only for reporting
the kernel BUG or code inconsitent with format caps check and the
fb size check. And I agree, basically it will not happene.
@Brian, I'm Ok to remove it. :)

@Ville:
Basically komeda follow the way-1, but a little improvement for
matching komeda's requirement. for komeda which will do two level's
format+modifier check.
1). In fb_create, A roughly check to see if the format+modifier can be
supported by current HW.
2). In plane_atomic_check: to see if the format+modifier can be
supported for a specific layer and with a specific configuration (ROT)

This is a format valid check example for plane_check.
https://patchwork.freedesktop.org/patch/301140/?series=59747&rev=2

James

2019-05-27 14:45:12

by Ville Syrjälä

[permalink] [raw]
Subject: Re: [PATCH] drm/komeda: Added AFBC support for komeda driver

On Mon, May 27, 2019 at 06:51:18AM +0000, james qian wang (Arm Technology China) wrote:
> On Fri, May 24, 2019 at 03:12:26PM +0300, Ville Syrj?l? wrote:
> > On Fri, May 24, 2019 at 11:10:09AM +0000, Brian Starkey wrote:
> > > Hi,
> > >
> > > On Tue, May 21, 2019 at 09:45:58AM +0100, james qian wang (Arm Technology China) wrote:
> > > > On Thu, May 16, 2019 at 09:57:49PM +0800, Ayan Halder wrote:
> > > > > On Thu, Apr 04, 2019 at 12:06:14PM +0100, james qian wang (Arm Technology China) wrote:
> > > > > >
> > > > > > +static int
> > > > > > +komeda_fb_afbc_size_check(struct komeda_fb *kfb, struct drm_file *file,
> > > > > > + const struct drm_mode_fb_cmd2 *mode_cmd)
> > > > > > +{
> > > > > > + struct drm_framebuffer *fb = &kfb->base;
> > > > > > + const struct drm_format_info *info = fb->format;
> > > > > > + struct drm_gem_object *obj;
> > > > > > + u32 alignment_w = 0, alignment_h = 0, alignment_header;
> > > > > > + u32 n_blocks = 0, min_size = 0;
> > > > > > +
> > > > > > + obj = drm_gem_object_lookup(file, mode_cmd->handles[0]);
> > > > > > + if (!obj) {
> > > > > > + DRM_DEBUG_KMS("Failed to lookup GEM object\n");
> > > > > > + return -ENOENT;
> > > > > > + }
> > > > > > +
> > > > > > + switch (fb->modifier & AFBC_FORMAT_MOD_BLOCK_SIZE_MASK) {
> > > > > > + case AFBC_FORMAT_MOD_BLOCK_SIZE_32x8:
> > > > > > + alignment_w = 32;
> > > > > > + alignment_h = 8;
> > > > > > + break;
> > > > > > + case AFBC_FORMAT_MOD_BLOCK_SIZE_16x16:
> > > > > > + alignment_w = 16;
> > > > > > + alignment_h = 16;
> > > > > > + break;
> > > > > > + default:
> > > > > Can we have something like a warn here ?
> > > >
> > > > will add a WARN here.
> > > >
> > >
> > > I think it's better not to. fb->modifier comes from
> > > userspace, so a malicious app could spam us with WARNs, effectively
> > > dos-ing the system. -EINVAL should be sufficient.
> >
> > Should probably check that the entire modifier+format is
> > actually valid. Otherwise you risk passing on a bogus
> > modifier deeper into the driver which may trigger
> > interesting bugs.
> >
> > Also theoretically (however unlikely) some broken userspace
> > might start to depend on the ability to create framebuffers
> > with crap modifiers, which could later break if you change
> > the way you handle the modifiers. Then you're stuck between
> > the rock and hard place because you can't break existing
> > userspace but you still want to change the way modifiers
> > are handled in the kernel.
> >
> > Best not give userspace too much rope IMO. Two ways to go about
> > that:
> > 1) drm_any_plane_has_format() (assumes your .format_mod_supported()
> > does its job properly)
> > 2) roll your own
> >
> > --
> > Ville Syrj?l?
> > Intel
>
> Hi Brian & Ville:
>
> komed has a format+modifier check before the fb size check.
> and for komeda_fb_create, the first step is do the format+modifier
> check, the size check is the furthur check after the such format
> valid check. and the detailed fb_create is like:
>
> struct drm_framebuffer *
> komeda_fb_create(struct drm_device *dev, struct drm_file *file,
> const struct drm_mode_fb_cmd2 *mode_cmd)
> {
> ...
> /* Step 1: format+modifier valid check, if it can not be support,
> * get_format_caps will return a NULL ptr.
> */
> kfb->format_caps = komeda_get_format_caps(&mdev->fmt_tbl,
> mode_cmd->pixel_format,
> mode_cmd->modifier[0]);
> if (!kfb->format_caps) {
> DRM_DEBUG_KMS("FMT %x is not supported.\n",
> mode_cmd->pixel_format);
> kfree(kfb);
> return ERR_PTR(-EINVAL);
> }
>
> drm_helper_mode_fill_fb_struct(dev, &kfb->base, mode_cmd);
>
> /* step 2, do the size check */
> if (kfb->base.modifier)
> ret = komeda_fb_afbc_size_check(kfb, file, mode_cmd);
> else
> ret = komeda_fb_none_afbc_size_check(mdev, kfb, file, mode_cmd);
> if (ret < 0)
> goto err_cleanup;
>
> ...
> }
>
> So theoretically, the WARN in step2 is redundant if get_format_caps
> function has no problem. :). the WARN here is only for reporting
> the kernel BUG or code inconsitent with format caps check and the
> fb size check. And I agree, basically it will not happene.
> @Brian, I'm Ok to remove it. :)
>
> @Ville:
> Basically komeda follow the way-1, but a little improvement for
> matching komeda's requirement. for komeda which will do two level's
> format+modifier check.
> 1). In fb_create, A roughly check to see if the format+modifier can be
> supported by current HW.

Yeah, looks like it shouldn't allow any unspecfied modifiers to
sneak through. So should be good.

> 2). In plane_atomic_check: to see if the format+modifier can be
> supported for a specific layer and with a specific configuration (ROT)
>
> This is a format valid check example for plane_check.
> https://patchwork.freedesktop.org/patch/301140/?series=59747&rev=2
>
> James

--
Ville Syrj?l?
Intel

2019-05-28 15:28:10

by Brian Starkey

[permalink] [raw]
Subject: Re: [PATCH] drm/komeda: Added AFBC support for komeda driver

Hi James,

On Mon, May 27, 2019 at 07:51:18AM +0100, james qian wang (Arm Technology China) wrote:
> Hi Brian & Ville:
>
> komed has a format+modifier check before the fb size check.
> and for komeda_fb_create, the first step is do the format+modifier
> check, the size check is the furthur check after the such format
> valid check. and the detailed fb_create is like:

Thanks for the detail, it sounds good.

-Brian