Hi Sam, Thanks for you review.
-----Original Message----- From: Sam Ravnborg sam@ravnborg.org Sent: Wednesday, July 28, 2021 12:16 AM To: Chrisanthus, Anitha anitha.chrisanthus@intel.com Cc: dri-devel@lists.freedesktop.org; Dea, Edmund J edmund.j.dea@intel.com 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@intel.com Signed-off-by: Anitha Chrisanthus anitha.chrisanthus@intel.com
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]);LCD_LAYERn_DMA_CB_LINE_VSTRIDE(plane_id),
width * fb->format->cpp[0]);
fb->pitches[1]);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:
- offsets is no logner needed
That's a mistake, will put back the offsets
- 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.
- 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