| Message ID | 20240301020126.11539-1-yunfei.dong@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: mediatek: vcodec: support 36bit physical address | expand |
Il 01/03/24 03:01, Yunfei Dong ha scritto: > 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. > > 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; If you used the right type from the beginning, you wouldn't have to fix that ;-) Is there any reason why you didn't - and still don't use the `phys_addr_t` type for the `pa` member? Cheers, Angelo > 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; > tb = instance->tile.va; > for (i = 0; i < rows; i++) { > for (j = 0; j < cols; j++) {
Hi AngeloGioacchino, Thanks for you reviewing. On Fri, 2024-03-01 at 10:03 +0100, AngeloGioacchino Del Regno wrote: > Il 01/03/24 03:01, Yunfei Dong ha scritto: > > 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. > > > > 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; > > If you used the right type from the beginning, you wouldn't have to > fix that ;-) > Yes, you are right, thanks for your remind. > Is there any reason why you didn't - and still don't use the > `phys_addr_t` type > for the `pa` member? > pa is also iova, dma address. Change it to dma_addr_t looks much better. I will change it in next patch. > Cheers, > Angelo > Best Regards, Yunfei Dong > > 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; > > tb = instance->tile.va; > > for (i = 0; i < rows; i++) { > > for (j = 0; j < cols; j++) { > >
Il 01/03/24 10:23, Yunfei Dong (董云飞) ha scritto: > Hi AngeloGioacchino, > > Thanks for you reviewing. > On Fri, 2024-03-01 at 10:03 +0100, AngeloGioacchino Del Regno wrote: >> Il 01/03/24 03:01, Yunfei Dong ha scritto: >>> 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. >>> >>> 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; >> >> If you used the right type from the beginning, you wouldn't have to >> fix that ;-) >> > Yes, you are right, thanks for your remind. >> Is there any reason why you didn't - and still don't use the >> `phys_addr_t` type >> for the `pa` member? >> > pa is also iova, dma address. Change it to dma_addr_t looks much > better. > Ok, dma_addr_t looks good as well. Cheers! > I will change it in next patch. >> Cheers, >> Angelo >> > Best Regards, > Yunfei Dong >>> 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; >>> tb = instance->tile.va; >>> for (i = 0; i < rows; i++) { >>> for (j = 0; j < cols; j++) { >> >>
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 > >
Hey Yunfei, On 01.03.2024 10:36, AngeloGioacchino Del Regno wrote: >Il 01/03/24 10:23, Yunfei Dong (董云飞) ha scritto: >>Hi AngeloGioacchino, >> >>Thanks for you reviewing. >>On Fri, 2024-03-01 at 10:03 +0100, AngeloGioacchino Del Regno wrote: >>>Il 01/03/24 03:01, Yunfei Dong ha scritto: >>>>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. >>>> >>>>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; >>> >>>If you used the right type from the beginning, you wouldn't have to >>>fix that ;-) >>> >>Yes, you are right, thanks for your remind. >>>Is there any reason why you didn't - and still don't use the >>>`phys_addr_t` type >>>for the `pa` member? >>> >>pa is also iova, dma address. Change it to dma_addr_t looks much >>better. >> > >Ok, dma_addr_t looks good as well. Ah alright, disregard my comment about unsigned long vs u64 then, but please have a look at the other casts in the driver as well as you currently cast to either: - u64 - uint64_t - unsigned long Greetings, Sebastian > >Cheers! > >>I will change it in next patch. >>>Cheers, >>>Angelo >>> >>Best Regards, >>Yunfei Dong >>>> 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; >>>> tb = instance->tile.va; >>>> for (i = 0; i < rows; i++) { >>>> for (j = 0; j < cols; j++) { >>> >>> > >
Hi Sebastian, Thanks for your advice. On Fri, 2024-03-01 at 10:52 +0100, Sebastian Fricke wrote: > 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. > I will rewrite the commit message according to your advice. > 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) > For 64-bit system, unsigned long is u64 or uint64_t. The pa is over 32bit for mt8188(64bit system). The pa is 32bit for mt8195/mt8192/mt8186 etc(64bit system). pa = (unsigned long)bs->dma_addr + offset; -> I need to remove 'unsigned long' before bs->dma_addr. For 32bit pa, the high bit is zero, won't influence the hardware decoder. Best Regards, Yunfei Dong > (Same applies for: > drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_vp8_if.c:452 > ) > Ok, I will change it in next patch. > Greetings, > Sebastian > > > tb = instance->tile.va; > > for (i = 0; i < rows; i++) { > > for (j = 0; j < cols; j++) { > > -- > > 2.18.0 > > > >
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; tb = instance->tile.va; for (i = 0; i < rows; i++) { for (j = 0; j < cols; j++) {
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. 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(-)