Re: [PATCH 04/14] drm/kmb : W/A for 256B cache alignment for video

[Sam Ravnborg <sam@ravnborg.org>]

Hi Anitha,
On Tue, Jul 27, 2021 at 05:31:16PM -0700, Anitha Chrisanthus wrote:
> For B0 silicon, the media driver pads the decoded video dmabufs for 256B
> alignment. This is the backing buffer of the framebuffer and info in the
> drm frame buffer is not correct for these buffers as this is done
> internally in the media driver. This change extracts the meta data info
> from dmabuf priv structure and uses that info for programming stride and
> offsets in kmb_plane_atomic_update().
> 
> Fixes: 7f7b96a8a0a1 ("drm/kmb: Add support for KeemBay Display")
> Signed-off-by: Edmund Dea <edmund.j.dea@intel.com>
> Signed-off-by: Anitha Chrisanthus <anitha.chrisanthus@intel.com>

Drop extra space in subject before ':'


> ---
>  drivers/gpu/drm/kmb/kmb_drv.h    |  1 +
>  drivers/gpu/drm/kmb/kmb_plane.c  | 38 ++++++++++++++++++++---
>  drivers/gpu/drm/kmb/kmb_vidmem.h | 52 ++++++++++++++++++++++++++++++++
>  3 files changed, 87 insertions(+), 4 deletions(-)
>  create mode 100644 drivers/gpu/drm/kmb/kmb_vidmem.h
> 
> diff --git a/drivers/gpu/drm/kmb/kmb_drv.h b/drivers/gpu/drm/kmb/kmb_drv.h
> index ebbaa5f422d5..0904e6eb2a09 100644
> --- a/drivers/gpu/drm/kmb/kmb_drv.h
> +++ b/drivers/gpu/drm/kmb/kmb_drv.h
> @@ -49,6 +49,7 @@ struct kmb_drm_private {
>  	int				kmb_under_flow;
>  	int				kmb_flush_done;
>  	int				layer_no;
> +	struct viv_vidmem_metadata	*md_info;
I cannot see this member used in this patch - can it be dropped?

>  };
>  
>  static inline struct kmb_drm_private *to_kmb(const struct drm_device *dev)
> diff --git a/drivers/gpu/drm/kmb/kmb_plane.c b/drivers/gpu/drm/kmb/kmb_plane.c
> index 2888dd5dcc2c..e45419d6ed96 100644
> --- a/drivers/gpu/drm/kmb/kmb_plane.c
> +++ b/drivers/gpu/drm/kmb/kmb_plane.c
> @@ -11,12 +11,16 @@
>  #include <drm/drm_fb_helper.h>
>  #include <drm/drm_fourcc.h>
>  #include <drm/drm_gem_cma_helper.h>
> +#include <drm/drm_gem_framebuffer_helper.h>
>  #include <drm/drm_managed.h>
>  #include <drm/drm_plane_helper.h>
>  
> +#include <linux/dma-buf.h>
> +
>  #include "kmb_drv.h"
>  #include "kmb_plane.h"
>  #include "kmb_regs.h"
> +#include "kmb_vidmem.h"
>  
>  const u32 layer_irqs[] = {
>  	LCD_INT_VL0,
> @@ -294,8 +298,10 @@ static void kmb_plane_atomic_update(struct drm_plane *plane,
>  	unsigned int ctrl = 0, val = 0, out_format = 0;
>  	unsigned int src_w, src_h, crtc_x, crtc_y;
>  	unsigned char plane_id;
> -	int num_planes;
> +	int num_planes, i;
>  	static dma_addr_t addr[MAX_SUB_PLANES];
> +	struct viv_vidmem_metadata *md = NULL;
> +	struct drm_gem_object *gem_obj;
>  
>  	if (!plane || !new_plane_state || !old_plane_state)
>  		return;
> @@ -325,6 +331,16 @@ static void kmb_plane_atomic_update(struct drm_plane *plane,
>  	drm_dbg(&kmb->drm,
>  		"src_w=%d src_h=%d, fb->format->format=0x%x fb->flags=0x%x\n",
>  		  src_w, src_h, fb->format->format, fb->flags);
> +	gem_obj = drm_gem_fb_get_obj(fb, plane_id);
> +	if (gem_obj && gem_obj->import_attach &&
> +	    gem_obj->import_attach->dmabuf &&
> +	    gem_obj->import_attach->dmabuf->priv) {
> +		md = gem_obj->import_attach->dmabuf->priv;
> +
> +		/* Check if metadata is coming from hantro driver */
> +		if (md->magic != HANTRO_IMAGE_VIV_META_DATA_MAGIC)
> +			md = NULL;
> +	}
>  
>  	width = fb->width;
>  	height = fb->height;
> @@ -332,6 +348,11 @@ static void kmb_plane_atomic_update(struct drm_plane *plane,
>  	drm_dbg(&kmb->drm, "dma_len=%d ", dma_len);
>  	kmb_write_lcd(kmb, LCD_LAYERn_DMA_LEN(plane_id), dma_len);
>  	kmb_write_lcd(kmb, LCD_LAYERn_DMA_LEN_SHADOW(plane_id), dma_len);
> +	if (md) {
> +		for (i = 0; i < 3; i++)
> +			fb->pitches[i] = md->plane[i].stride;
> +	}
> +
>  	kmb_write_lcd(kmb, LCD_LAYERn_DMA_LINE_VSTRIDE(plane_id),
>  		      fb->pitches[0]);
>  	kmb_write_lcd(kmb, LCD_LAYERn_DMA_LINE_WIDTH(plane_id),
> @@ -339,18 +360,22 @@ static void kmb_plane_atomic_update(struct drm_plane *plane,
>  
>  	addr[Y_PLANE] = drm_fb_cma_get_gem_addr(fb, new_plane_state, 0);
>  	kmb_write_lcd(kmb, LCD_LAYERn_DMA_START_ADDR(plane_id),
> -		      addr[Y_PLANE] + fb->offsets[0]);
> +		      addr[Y_PLANE]);
>  	val = get_pixel_format(fb->format->format);
>  	val |= get_bits_per_pixel(fb->format);
>  	/* Program Cb/Cr for planar formats */
>  	if (num_planes > 1) {
>  		kmb_write_lcd(kmb, LCD_LAYERn_DMA_CB_LINE_VSTRIDE(plane_id),
> -			      width * fb->format->cpp[0]);
> +				fb->pitches[1]);
>  		kmb_write_lcd(kmb, LCD_LAYERn_DMA_CB_LINE_WIDTH(plane_id),
>  			      (width * fb->format->cpp[0]));
>  
>  		addr[U_PLANE] = drm_fb_cma_get_gem_addr(fb, new_plane_state,
>  							U_PLANE);
> +		if (md) {
> +			addr[U_PLANE] += md->plane[1].offset -
> +					 (addr[U_PLANE] - addr[Y_PLANE]);
> +		}

I failed to follow why:
1) offsets is no logner needed
2) If pitches is always set or only set with a hantro buffer
3) Why addr[U_PLANE] is assigned twice in the md != NULL case

>  		/* check if Cb/Cr is swapped*/
>  		if (num_planes == 3 && (val & LCD_LAYER_CRCB_ORDER))
>  			kmb_write_lcd(kmb,
> @@ -364,7 +389,7 @@ static void kmb_plane_atomic_update(struct drm_plane *plane,
>  		if (num_planes == 3) {
>  			kmb_write_lcd(kmb,
>  				      LCD_LAYERn_DMA_CR_LINE_VSTRIDE(plane_id),
> -				      ((width) * fb->format->cpp[0]));
> +				      fb->pitches[2]);
>  
>  			kmb_write_lcd(kmb,
>  				      LCD_LAYERn_DMA_CR_LINE_WIDTH(plane_id),
> @@ -373,6 +398,11 @@ static void kmb_plane_atomic_update(struct drm_plane *plane,
>  			addr[V_PLANE] = drm_fb_cma_get_gem_addr(fb,
>  								new_plane_state,
>  								V_PLANE);
> +			if (md) {
> +				addr[V_PLANE] +=
> +					md->plane[2].offset -
> +					(addr[V_PLANE] - addr[Y_PLANE]);
> +			}
Likewise - is pitches always valid and why assing addr[V_PLANE] twice?

>  
>  			/* check if Cb/Cr is swapped*/
>  			if (val & LCD_LAYER_CRCB_ORDER)
> diff --git a/drivers/gpu/drm/kmb/kmb_vidmem.h b/drivers/gpu/drm/kmb/kmb_vidmem.h
> new file mode 100644
> index 000000000000..06198d413f50
> --- /dev/null
> +++ b/drivers/gpu/drm/kmb/kmb_vidmem.h
> @@ -0,0 +1,52 @@
> +/* SPDX-License-Identifier: GPL-2.0-only
> + *
> + * Copyright © 2018-2020 Intel Corporation
> + */
> +
> +#ifndef __KMB_VIDMEM_H__
> +#define __KMB_VIDMEM_H__
> +
> +#define HANTRO_MAGIC(ch0, ch1, ch2, ch3) \
> +	    ((unsigned long)(unsigned char)(ch0) | \
> +	    ((unsigned long)(unsigned char)(ch1) << 8) | \
> +	    ((unsigned long)(unsigned char)(ch2) << 16) | \
> +	    ((unsigned long)(unsigned char)(ch3) << 24))
...

This header looks like it belongs outside the drm driver - I assume the
hantro driver needs this?
Or is this some uapi stuff?

	Sam