Re: [PATCHv2 3/4] drm/komeda: use afbc helpers

["james qian wang (Arm Technology China)" <james.qian.wang@arm.com>]

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 <andrzej.p@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.wang@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.wang@arm.com,
> Mihail.Atanassov@arm.com, Ben.Davis@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 ?  

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel