Received-SPF: pass (google.com: domain of linux-kernel+bounces-88203-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Fri, 1 Mar 2024 10:52:17 +0100
From: Sebastian Fricke <sebastian.fricke@collabora.com>
To: Yunfei Dong <yunfei.dong@mediatek.com>
Cc: =?utf-8?B?TsOtY29sYXMgRiAuIFIgLiBBIC4=?= Prado <nfraprado@collabora.com>,
	Nicolas Dufresne <nicolas.dufresne@collabora.com>,
	Hans Verkuil <hverkuil-cisco@xs4all.nl>,
	AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>,
	Benjamin Gaignard <benjamin.gaignard@collabora.com>,
	Nathan Hebert <nhebert@chromium.org>,
	Hsin-Yi Wang <hsinyi@chromium.org>,
	Fritz Koenig <frkoenig@chromium.org>,
	Daniel Vetter <daniel@ffwll.ch>, Steve Cho <stevecho@chromium.org>,
	linux-media@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-mediatek@lists.infradead.org,
	Project_Global_Chrome_Upstream_Group@mediatek.com
Subject: Re: [PATCH] media: mediatek: vcodec: support 36bit physical address
Message-ID: <20240301095217.bmsnizobpb736fgg@basti-XPS-13-9310>
References: <20240301020126.11539-1-yunfei.dong@mediatek.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Disposition: inline
In-Reply-To: <20240301020126.11539-1-yunfei.dong@mediatek.com>

Hey Yunfei,

On 01.03.2024 10:01, Yunfei Dong wrote:
>The physical address is beyond 32bit for mt8188 platform, need
>to change the type from unsigned int to unsigned long in case of
>the high bit missing.

I would reword this a bit, the address is not beyond 32 bit, which would
could be interpret as if the address starts after 32nd bit, instead it
is larger than 32bits. Secondly, we don't change the type in case the
high bit is missing, we change the type unconditionally, we do change
the type so that the high bit isn't missing.

My suggestion:

The physical address on the MT8188 platform is larger than 32 bits,
change the type from unsigned int to unsigned long to be able to access
the high bits of the address.

One more question below...

>
>Signed-off-by: Yunfei Dong <yunfei.dong@mediatek.com>
>---
> .../mediatek/vcodec/decoder/vdec/vdec_vp9_req_lat_if.c        | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_vp9_req_lat_if.c b/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_vp9_req_lat_if.c
>index cf48d09b78d7..85df3e7c2983 100644
>--- a/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_vp9_req_lat_if.c
>+++ b/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_vp9_req_lat_if.c
>@@ -1074,7 +1074,7 @@ static int vdec_vp9_slice_setup_tile_buffer(struct vdec_vp9_slice_instance *inst
> 	unsigned int mi_row;
> 	unsigned int mi_col;
> 	unsigned int offset;
>-	unsigned int pa;
>+	unsigned long pa;
> 	unsigned int size;
> 	struct vdec_vp9_slice_tiles *tiles;
> 	unsigned char *pos;
>@@ -1109,7 +1109,7 @@ static int vdec_vp9_slice_setup_tile_buffer(struct vdec_vp9_slice_instance *inst
> 	pos = va + offset;
> 	end = va + bs->size;
> 	/* truncated */
>-	pa = (unsigned int)bs->dma_addr + offset;
>+	pa = (unsigned long)bs->dma_addr + offset;

I can see in other parts of the driver that bs->dma_addr is converted to
u64 or uint64_t. Is unsigned long always 64-bit on the different
Mediatek platforms? If so, why do you prefer unsigned long over u64?
(Which describes the type more precisely)

(Same applies for:
drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_vp8_if.c:452)

Greetings,
Sebastian

> 	tb = instance->tile.va;
> 	for (i = 0; i < rows; i++) {
> 		for (j = 0; j < cols; j++) {
>-- 
>2.18.0
>
>