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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Sam,
Thanks for you review.

> -----Original Message-----
> From: Sam Ravnborg <sam@xxxxxxxxxxxx>
> Sent: Wednesday, July 28, 2021 12:16 AM
> To: Chrisanthus, Anitha <anitha.chrisanthus@xxxxxxxxx>
> Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx; Dea, Edmund J
> <edmund.j.dea@xxxxxxxxx>
> Subject: Re: [PATCH 04/14] drm/kmb : W/A for 256B cache alignment for video
> 
> 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@xxxxxxxxx>
> > Signed-off-by: Anitha Chrisanthus <anitha.chrisanthus@xxxxxxxxx>
> 
> Drop extra space in subject before ':'
ok
> 
> 
> > ---
> >  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?
Good catch, yes will remove it.
> 
> >  };
> >
> >  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
That's a mistake, will put back the offsets
> 2) If pitches is always set or only set with a hantro buffer
Pitches is set by hantro driver to a different value, I'm going to send v2 which combines this with patch 09 which reverts some of the above changes.
> 3) Why addr[U_PLANE] is assigned twice in the md != NULL case
I agree this is confusing, will make send v2 with simplified and separate calculations for addr[U and V plane} for md==NULL and !=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?
Answered above, will send V2
> 
> >
> >  			/* 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 (c) 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?
This is from hantro driver and I agree it does not belong here, but we need this struct to extract the meta data info. Hantro driver is not upstreamed yet, how should I tackle this?
> Or is this some uapi stuff?
> 
> 	Sam




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux