On Wed, Nov 13, 2019 at 02:01:53AM +0000, james qian wang (Arm Technology China) wrote: > On Fri, Nov 08, 2019 at 04:09:54PM +0000, Ayan Halder wrote: > > On Mon, Nov 04, 2019 at 11:12:27PM +0100, Andrzej Pietrasiewicz wrote: > > > There are afbc helpers available. > > > > > > Signed-off-by: Andrzej Pietrasiewicz <andrze...@collabora.com> > > > --- > > > .../arm/display/komeda/komeda_format_caps.h | 1 - > > > .../arm/display/komeda/komeda_framebuffer.c | 44 +++++++------------ > > > 2 files changed, 17 insertions(+), 28 deletions(-) > > > > > > 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 32273cf18f7c..607eea80e60c 100644 > > > --- a/drivers/gpu/drm/arm/display/komeda/komeda_format_caps.h > > > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_format_caps.h > > > @@ -33,7 +33,6 @@ > > > > > > #define AFBC_TH_LAYOUT_ALIGNMENT 8 > > > #define AFBC_HEADER_SIZE 16 > > > -#define AFBC_SUPERBLK_ALIGNMENT 128 > > > #define AFBC_SUPERBLK_PIXELS 256 > > > #define AFBC_BODY_START_ALIGNMENT 1024 > > > #define AFBC_TH_BODY_START_ALIGNMENT 4096 > > > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_framebuffer.c > > > b/drivers/gpu/drm/arm/display/komeda/komeda_framebuffer.c > > > index 1b01a625f40e..e9c87551a5b8 100644 > > > --- a/drivers/gpu/drm/arm/display/komeda/komeda_framebuffer.c > > > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_framebuffer.c > > > @@ -4,6 +4,7 @@ > > > * Author: James.Qian.Wang <james.qian.w...@arm.com> > > > * > > > */ > > > +#include <drm/drm_afbc.h> > > > #include <drm/drm_device.h> > > > #include <drm/drm_fb_cma_helper.h> > > > #include <drm/drm_gem.h> > > > @@ -43,8 +44,7 @@ komeda_fb_afbc_size_check(struct komeda_fb *kfb, struct > > > drm_file *file, > > > 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, n_blocks, bpp; > > > - u64 min_size; > > > + u32 alignment_w = 0, alignment_h = 0, alignment_header, bpp; > > > > > > obj = drm_gem_object_lookup(file, mode_cmd->handles[0]); > > > if (!obj) { > > > @@ -52,19 +52,15 @@ komeda_fb_afbc_size_check(struct komeda_fb *kfb, > > > struct drm_file *file, > > > 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: > > > - WARN(1, "Invalid AFBC_FORMAT_MOD_BLOCK_SIZE: %lld.\n", > > > - fb->modifier & AFBC_FORMAT_MOD_BLOCK_SIZE_MASK); > > > - break; > > > + if (!drm_afbc_get_superblk_wh(fb->modifier, &alignment_w, &alignment_h)) > > > + return -EINVAL; > > > + > > > + if ((alignment_w != 16 || alignment_h != 16) && > > > + (alignment_w != 32 || alignment_h != 8)) { > > > + DRM_DEBUG_KMS("Unsupported afbc tile w/h [%d/%d]\n", > > > + alignment_w, alignment_h); > > > + > > > + return -EINVAL; > > To be honest, the previous code looks much more readable > > > } > > > > > > /* tiled header afbc */ > > > @@ -84,20 +80,14 @@ komeda_fb_afbc_size_check(struct komeda_fb *kfb, > > > struct drm_file *file, > > > goto check_failed; > > > } > > > > > > - n_blocks = (kfb->aligned_w * kfb->aligned_h) / AFBC_SUPERBLK_PIXELS; > > > - kfb->offset_payload = ALIGN(n_blocks * AFBC_HEADER_SIZE, > > > - alignment_header); > > > - > > > bpp = komeda_get_afbc_format_bpp(info, fb->modifier); > > > - kfb->afbc_size = kfb->offset_payload + n_blocks * > > > - ALIGN(bpp * AFBC_SUPERBLK_PIXELS / 8, > > > - 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%zx. > > > min_size 0x%llx.\n", > > > - obj->size, min_size); > > We need kfb->offset_payload and kfb->afbc_size to set some registers > > in d71_layer_update(). At this moment I feel like punching myself for > > making the suggestion to consider abstracting some of the komeda's afbc > > checks. To me it does not look like komeda(in the current shape) can take > > much advantage of the generic _afbc_fb_check() function (as was suggested > > previously by Danvet). > > > > However, I will let james.qian.w...@arm.com, > > mihail.atanas...@arm.com, ben.da...@arm.com comment here to see if > > there could be a way of abstracting the afbc bits from komeda. > > > > Hi all: > > Since the current generic drm_afbc helpers only support afbc_1.1, but > komeda needs support both afbc1.1/1.2, so I think we can: > - Add afbc1.2 support to drm afbc helpers. > - for the afbc_payload_offset, can we add this member to > drm_framebuffer ? > - The aligned_w/h are important for afbc, so can we have them in > drm_framebuffer ?
How expensive is it to recompute these from a struct drm_framebuffer? I'd just go with one function which computes all these derived values, and stuffs them into a struct drm_afbc. Then drivers call that once or so. For reworking drm_framebuffer itself I think it's probably a bit too earlier. And if we do it, we should maybe go a bit more bold, aiming to property-fy all the framebuffer settings, maybe even making it extensible, and have drivers handle a corresponding drm_framebuffer_state. A third option would be to create a drm_afbc_framebuffer which embeds drm_framebuffer and which drivers would need to use. But also feels a bit like too much churn. Recomputing should be quick enough and much easier. -Daniel > > Thanks > James > > > Thanks anyways for taking a stab at this. > > -Ayan > > > + > > > + if (!drm_afbc_check_fb_size(mode_cmd->pitches[0], bpp, > > > + mode_cmd->width, mode_cmd->height, > > > + alignment_w, alignment_h, > > > + obj->size, mode_cmd->offsets[0], > > > + AFBC_SUPERBLK_ALIGNMENT)) > > > goto check_failed; > > > - } > > > > > > fb->obj[0] = obj; > > > return 0; > > > -- > > > 2.17.1 -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel