diff mbox series

[v2] media: mediatek: vcodec: Force capture queue format to MM21

Message ID 20230210055518.6017-1-yunfei.dong@mediatek.com (mailing list archive)
State New, archived
Headers show
Series [v2] media: mediatek: vcodec: Force capture queue format to MM21 | expand

Commit Message

Yunfei Dong Feb. 10, 2023, 5:55 a.m. UTC
In order to conver the format of capture queue from mediatek MM21 to
standard yuv420 with Libyuv, need to force capture queue format to
MM21 for Libyuv can't covert mediatek MT21 format at current period.

Fixes: 7501edef6b1f ("media: mediatek: vcodec: Different codec using different capture format")
Signed-off-by: Yunfei Dong <yunfei.dong@mediatek.org>
---
changed with v1:
- add Fixes tag.
---
 drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Nicolas Dufresne Feb. 10, 2023, 3:36 p.m. UTC | #1
Le vendredi 10 février 2023 à 13:55 +0800, Yunfei Dong a écrit :
> In order to conver the format of capture queue from mediatek MM21 to
> standard yuv420 with Libyuv, need to force capture queue format to
> MM21 for Libyuv can't covert mediatek MT21 format at current period.

Please rework this text, it is hard to understand.

> 
> Fixes: 7501edef6b1f ("media: mediatek: vcodec: Different codec using different capture format")
> Signed-off-by: Yunfei Dong <yunfei.dong@mediatek.org>
> ---
> changed with v1:
> - add Fixes tag.
> ---
>  drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c
> index 641f533c417f..4f5e9c20214f 100644
> --- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c
> +++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c
> @@ -41,7 +41,7 @@ static bool mtk_vdec_get_cap_fmt(struct mtk_vcodec_ctx *ctx, int format_index)
>  	const struct mtk_video_fmt *fmt;
>  	struct mtk_q_data *q_data;
>  	int num_frame_count = 0, i;
> -	bool ret = true;
> +	bool ret = false;
>  
>  	for (i = 0; i < *dec_pdata->num_formats; i++) {
>  		if (dec_pdata->vdec_formats[i].type != MTK_FMT_FRAME)
> @@ -63,7 +63,7 @@ static bool mtk_vdec_get_cap_fmt(struct mtk_vcodec_ctx *ctx, int format_index)
>  	case V4L2_PIX_FMT_H264_SLICE:
>  	case V4L2_PIX_FMT_VP9_FRAME:
>  		if (fmt->fourcc == V4L2_PIX_FMT_MM21)
> -			ret = false;
> +			ret = true;

This makes the VP8 and the other cases identical, leaving anything that touches
MT21 as dead code. I'm not sure, cause I cannot test it, but it should in theory
render MT8192 unusable, unless a new firmware has been submitted to linux-
firmware with MM21 support ?

>  		break;
>  	default:
>  		ret = true;
Yunfei Dong Feb. 14, 2023, 2:28 a.m. UTC | #2
Hi Nicolas,

Thanks for your suggestion.
On Fri, 2023-02-10 at 10:36 -0500, Nicolas Dufresne wrote:
> Le vendredi 10 février 2023 à 13:55 +0800, Yunfei Dong a écrit :
> > In order to conver the format of capture queue from mediatek MM21
> > to
> > standard yuv420 with Libyuv, need to force capture queue format to
> > MM21 for Libyuv can't covert mediatek MT21 format at current
> > period.
> 
> Please rework this text, it is hard to understand.
> 

Will re-write the commit message in next version as below, could you
please help to review whether it's well to be used ?

Libyuv is one software library used to covert format. Only covert
mediatek MM21 to standard yuv420 for MT21 is compressed mode and MM21
is uncompressed mode at current period. Need to set capture queue
format to MM21 in order to use Libyuv.

> > 
> > Fixes: 7501edef6b1f ("media: mediatek: vcodec: Different codec
> > using different capture format")
> > Signed-off-by: Yunfei Dong <yunfei.dong@mediatek.org>
> > ---
> > changed with v1:
> > - add Fixes tag.
> > ---
> >  drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git
> > a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c
> > b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c
> > index 641f533c417f..4f5e9c20214f 100644
> > --- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c
> > +++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c
> > @@ -41,7 +41,7 @@ static bool mtk_vdec_get_cap_fmt(struct
> > mtk_vcodec_ctx *ctx, int format_index)
> >  	const struct mtk_video_fmt *fmt;
> >  	struct mtk_q_data *q_data;
> >  	int num_frame_count = 0, i;
> > -	bool ret = true;
> > +	bool ret = false;
> >  
> >  	for (i = 0; i < *dec_pdata->num_formats; i++) {
> >  		if (dec_pdata->vdec_formats[i].type != MTK_FMT_FRAME)
> > @@ -63,7 +63,7 @@ static bool mtk_vdec_get_cap_fmt(struct
> > mtk_vcodec_ctx *ctx, int format_index)
> >  	case V4L2_PIX_FMT_H264_SLICE:
> >  	case V4L2_PIX_FMT_VP9_FRAME:
> >  		if (fmt->fourcc == V4L2_PIX_FMT_MM21)
> > -			ret = false;
> > +			ret = true;
> 
> This makes the VP8 and the other cases identical, leaving anything
> that touches
> MT21 as dead code. I'm not sure, cause I cannot test it, but it
> should in theory
> render MT8192 unusable, unless a new firmware has been submitted to
> linux-
> firmware with MM21 support ?
> 

If the firmware only support MT21 => won't exist for vp8 need to use
MM21.

If the firmware only support MM21, will using MM21.
If the firmware support MT21 & MM21, will force VP8 to use MM21,
H264/VP9/AV1 will use MT21 before this change. 

Will force all driver to use MM21 after adding this change. 

Best Regards,
Yunfei Dong
> >  		break;
> >  	default:
> >  		ret = true;
> 
>
Yunfei Dong Feb. 14, 2023, 2:29 a.m. UTC | #3
Hi Nicolas,

Thanks for your suggestion.
On Fri, 2023-02-10 at 10:36 -0500, Nicolas Dufresne wrote:
> Le vendredi 10 février 2023 à 13:55 +0800, Yunfei Dong a écrit :
> > In order to conver the format of capture queue from mediatek MM21
> > to
> > standard yuv420 with Libyuv, need to force capture queue format to
> > MM21 for Libyuv can't covert mediatek MT21 format at current
> > period.
> 
> Please rework this text, it is hard to understand.
> 

Will re-write the commit message in next version as below, could you
please help to review whether it's well to be used ?

Libyuv is one software library used to covert format. Only covert
mediatek MM21 to standard yuv420 for MT21 is compressed mode and MM21
is uncompressed mode at current period. Need to set capture queue
format to MM21 in order to use Libyuv.

> > 
> > Fixes: 7501edef6b1f ("media: mediatek: vcodec: Different codec
> > using different capture format")
> > Signed-off-by: Yunfei Dong <yunfei.dong@mediatek.org>
> > ---
> > changed with v1:
> > - add Fixes tag.
> > ---
> >  drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git
> > a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c
> > b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c
> > index 641f533c417f..4f5e9c20214f 100644
> > --- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c
> > +++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c
> > @@ -41,7 +41,7 @@ static bool mtk_vdec_get_cap_fmt(struct
> > mtk_vcodec_ctx *ctx, int format_index)
> >  	const struct mtk_video_fmt *fmt;
> >  	struct mtk_q_data *q_data;
> >  	int num_frame_count = 0, i;
> > -	bool ret = true;
> > +	bool ret = false;
> >  
> >  	for (i = 0; i < *dec_pdata->num_formats; i++) {
> >  		if (dec_pdata->vdec_formats[i].type != MTK_FMT_FRAME)
> > @@ -63,7 +63,7 @@ static bool mtk_vdec_get_cap_fmt(struct
> > mtk_vcodec_ctx *ctx, int format_index)
> >  	case V4L2_PIX_FMT_H264_SLICE:
> >  	case V4L2_PIX_FMT_VP9_FRAME:
> >  		if (fmt->fourcc == V4L2_PIX_FMT_MM21)
> > -			ret = false;
> > +			ret = true;
> 
> This makes the VP8 and the other cases identical, leaving anything
> that touches
> MT21 as dead code. I'm not sure, cause I cannot test it, but it
> should in theory
> render MT8192 unusable, unless a new firmware has been submitted to
> linux-
> firmware with MM21 support ?
> 

If the firmware only support MT21 => won't exist for vp8 need to use
MM21.

If the firmware only support MM21, will using MM21.
If the firmware support MT21 & MM21, will force VP8 to use MM21,
H264/VP9/AV1 will use MT21 before this change. 

Will force all driver to use MM21 after adding this change. 

Best Regards,
Yunfei Dong
> >  		break;
> >  	default:
> >  		ret = true;
> 
>
Nícolas F. R. A. Prado Feb. 22, 2023, 9:11 p.m. UTC | #4
Hi,

On Tue, Feb 14, 2023 at 02:28:04AM +0000, Yunfei Dong (董云飞) wrote:
> Hi Nicolas,
> 
> Thanks for your suggestion.
> On Fri, 2023-02-10 at 10:36 -0500, Nicolas Dufresne wrote:
> > Le vendredi 10 février 2023 à 13:55 +0800, Yunfei Dong a écrit :
> > > In order to conver the format of capture queue from mediatek MM21
> > > to
> > > standard yuv420 with Libyuv, need to force capture queue format to
> > > MM21 for Libyuv can't covert mediatek MT21 format at current
> > > period.
> > 
> > Please rework this text, it is hard to understand.
> > 
> 
> Will re-write the commit message in next version as below, could you
> please help to review whether it's well to be used ?
> 
> Libyuv is one software library used to covert format. Only covert
> mediatek MM21 to standard yuv420 for MT21 is compressed mode and MM21
> is uncompressed mode at current period. Need to set capture queue
> format to MM21 in order to use Libyuv.
> 
> > > 
> > > Fixes: 7501edef6b1f ("media: mediatek: vcodec: Different codec
> > > using different capture format")
> > > Signed-off-by: Yunfei Dong <yunfei.dong@mediatek.org>
> > > ---
> > > changed with v1:
> > > - add Fixes tag.
> > > ---
> > >  drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git
> > > a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c
> > > b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c
> > > index 641f533c417f..4f5e9c20214f 100644
> > > --- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c
> > > +++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c
> > > @@ -41,7 +41,7 @@ static bool mtk_vdec_get_cap_fmt(struct
> > > mtk_vcodec_ctx *ctx, int format_index)
> > >  	const struct mtk_video_fmt *fmt;
> > >  	struct mtk_q_data *q_data;
> > >  	int num_frame_count = 0, i;
> > > -	bool ret = true;
> > > +	bool ret = false;

This change doesn't do anything, so I'd drop it.

> > >  
> > >  	for (i = 0; i < *dec_pdata->num_formats; i++) {
> > >  		if (dec_pdata->vdec_formats[i].type != MTK_FMT_FRAME)
> > > @@ -63,7 +63,7 @@ static bool mtk_vdec_get_cap_fmt(struct
> > > mtk_vcodec_ctx *ctx, int format_index)
> > >  	case V4L2_PIX_FMT_H264_SLICE:
> > >  	case V4L2_PIX_FMT_VP9_FRAME:
> > >  		if (fmt->fourcc == V4L2_PIX_FMT_MM21)
> > > -			ret = false;
> > > +			ret = true;
> > 
> > This makes the VP8 and the other cases identical, leaving anything
> > that touches
> > MT21 as dead code. I'm not sure, cause I cannot test it, but it
> > should in theory
> > render MT8192 unusable, unless a new firmware has been submitted to
> > linux-
> > firmware with MM21 support ?
> > 
> 
> If the firmware only support MT21 => won't exist for vp8 need to use
> MM21.

And that's the issue, the scp.img for MT8192 on linux-firmware only supports
MT21 [1]. Can you please update it to support both MM21 and MT21?

For MT8195, only MM21 is supported in scp.img [2], but since the hardware
supports both MM21 and MT21, the firmware should also support both. So please
also update it on linux-firmware.

[1] https://lore.kernel.org/all/20230112204626.ciaff4amseoidybw@notapiano/
[2] https://lore.kernel.org/all/20230112205825.wb5qcqhh5kwvyi3y@notapiano/

Thanks,
Nícolas

> 
> If the firmware only support MM21, will using MM21.
> If the firmware support MT21 & MM21, will force VP8 to use MM21,
> H264/VP9/AV1 will use MT21 before this change. 
> 
> Will force all driver to use MM21 after adding this change. 
> 
> Best Regards,
> Yunfei Dong
> > >  		break;
> > >  	default:
> > >  		ret = true;
> > 
> >
Yunfei Dong Feb. 27, 2023, 2:17 a.m. UTC | #5
Hi Nicolas,

Thanks for your suggestion.
On Wed, 2023-02-22 at 16:11 -0500, Nícolas F. R. A. Prado wrote:
> Hi,
> 
> On Tue, Feb 14, 2023 at 02:28:04AM +0000, Yunfei Dong (董云飞) wrote:
> > Hi Nicolas,
> > 
> > Thanks for your suggestion.
> > On Fri, 2023-02-10 at 10:36 -0500, Nicolas Dufresne wrote:
> > > Le vendredi 10 février 2023 à 13:55 +0800, Yunfei Dong a écrit :
> > > > In order to conver the format of capture queue from mediatek
> > > > MM21
> > > > to
> > > > standard yuv420 with Libyuv, need to force capture queue format
> > > > to
> > > > MM21 for Libyuv can't covert mediatek MT21 format at current
> > > > period.
> > > 
> > > Please rework this text, it is hard to understand.
> > > 
> > 
> > Will re-write the commit message in next version as below, could
> > you
> > please help to review whether it's well to be used ?
> > 
> > Libyuv is one software library used to covert format. Only covert
> > mediatek MM21 to standard yuv420 for MT21 is compressed mode and
> > MM21
> > is uncompressed mode at current period. Need to set capture queue
> > format to MM21 in order to use Libyuv.
> > 
> > > > 
> > > > Fixes: 7501edef6b1f ("media: mediatek: vcodec: Different codec
> > > > using different capture format")
> > > > Signed-off-by: Yunfei Dong <yunfei.dong@mediatek.org>
> > > > ---
> > > > changed with v1:
> > > > - add Fixes tag.
> > > > ---
> > > >  drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c | 4
> > > > ++--
> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git
> > > > a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c
> > > > b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c
> > > > index 641f533c417f..4f5e9c20214f 100644
> > > > --- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c
> > > > +++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c
> > > > @@ -41,7 +41,7 @@ static bool mtk_vdec_get_cap_fmt(struct
> > > > mtk_vcodec_ctx *ctx, int format_index)
> > > >  	const struct mtk_video_fmt *fmt;
> > > >  	struct mtk_q_data *q_data;
> > > >  	int num_frame_count = 0, i;
> > > > -	bool ret = true;
> > > > +	bool ret = false;
> 
> This change doesn't do anything, so I'd drop it.
> 
This change is useful when capture fourcc is MT21 will return false,
not support even if scp support mm21 and mt21.
> > > >  
> > > >  	for (i = 0; i < *dec_pdata->num_formats; i++) {
> > > >  		if (dec_pdata->vdec_formats[i].type !=
> > > > MTK_FMT_FRAME)
> > > > @@ -63,7 +63,7 @@ static bool mtk_vdec_get_cap_fmt(struct
> > > > mtk_vcodec_ctx *ctx, int format_index)
> > > >  	case V4L2_PIX_FMT_H264_SLICE:
> > > >  	case V4L2_PIX_FMT_VP9_FRAME:
> > > >  		if (fmt->fourcc == V4L2_PIX_FMT_MM21)
> > > > -			ret = false;
> > > > +			ret = true;
> > > 
> > > This makes the VP8 and the other cases identical, leaving
> > > anything
> > > that touches
> > > MT21 as dead code. I'm not sure, cause I cannot test it, but it
> > > should in theory
> > > render MT8192 unusable, unless a new firmware has been submitted
> > > to
> > > linux-
> > > firmware with MM21 support ?
> > > 
> > 
> > If the firmware only support MT21 => won't exist for vp8 need to
> > use
> > MM21.
> 
> And that's the issue, the scp.img for MT8192 on linux-firmware only
> supports
> MT21 [1]. Can you please update it to support both MM21 and MT21?
> 
> For MT8195, only MM21 is supported in scp.img [2], but since the
> hardware
> supports both MM21 and MT21, the firmware should also support both.
> So please
> also update it on linux-firmware.
> 
> [1] 
> https://lore.kernel.org/all/20230112204626.ciaff4amseoidybw@notapiano/
> [2] 
> https://lore.kernel.org/all/20230112205825.wb5qcqhh5kwvyi3y@notapiano/
> 
> Thanks,
> Nícolas
> 
MT8192 always use MM21 from the beginning, MT21 have not been enabled.

Best Regards,
Yunfei Dong
> > 
> > If the firmware only support MM21, will using MM21.
> > If the firmware support MT21 & MM21, will force VP8 to use MM21,
> > H264/VP9/AV1 will use MT21 before this change. 
> > 
> > Will force all driver to use MM21 after adding this change. 
> > 
> > Best Regards,
> > Yunfei Dong
> > > >  		break;
> > > >  	default:
> > > >  		ret = true;
> > > 
> > >
Nícolas F. R. A. Prado Feb. 28, 2023, 10:03 p.m. UTC | #6
On Mon, Feb 27, 2023 at 02:17:08AM +0000, Yunfei Dong (董云飞) wrote:
> Hi Nicolas,
> 
> Thanks for your suggestion.
> On Wed, 2023-02-22 at 16:11 -0500, Nícolas F. R. A. Prado wrote:
> > Hi,
> > 
> > On Tue, Feb 14, 2023 at 02:28:04AM +0000, Yunfei Dong (董云飞) wrote:
> > > Hi Nicolas,
> > > 
> > > Thanks for your suggestion.
> > > On Fri, 2023-02-10 at 10:36 -0500, Nicolas Dufresne wrote:
> > > > Le vendredi 10 février 2023 à 13:55 +0800, Yunfei Dong a écrit :
[..]
> > > > > diff --git
> > > > > a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c
> > > > > b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c
> > > > > index 641f533c417f..4f5e9c20214f 100644
> > > > > --- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c
> > > > > +++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c
> > > > > @@ -41,7 +41,7 @@ static bool mtk_vdec_get_cap_fmt(struct
> > > > > mtk_vcodec_ctx *ctx, int format_index)
> > > > >  	const struct mtk_video_fmt *fmt;
> > > > >  	struct mtk_q_data *q_data;
> > > > >  	int num_frame_count = 0, i;
> > > > > -	bool ret = true;
> > > > > +	bool ret = false;
> > 
> > This change doesn't do anything, so I'd drop it.
> > 
> This change is useful when capture fourcc is MT21 will return false,
> not support even if scp support mm21 and mt21.

But you have

	default:
		ret = true;
		break;

down below, so you're handling all cases in the switch, and the original value
doesn't matter. It could even be left uninitialized.

> > > > >  
> > > > >  	for (i = 0; i < *dec_pdata->num_formats; i++) {
> > > > >  		if (dec_pdata->vdec_formats[i].type !=
> > > > > MTK_FMT_FRAME)
> > > > > @@ -63,7 +63,7 @@ static bool mtk_vdec_get_cap_fmt(struct
> > > > > mtk_vcodec_ctx *ctx, int format_index)
> > > > >  	case V4L2_PIX_FMT_H264_SLICE:
> > > > >  	case V4L2_PIX_FMT_VP9_FRAME:
> > > > >  		if (fmt->fourcc == V4L2_PIX_FMT_MM21)
> > > > > -			ret = false;
> > > > > +			ret = true;
> > > > 
> > > > This makes the VP8 and the other cases identical, leaving
> > > > anything
> > > > that touches
> > > > MT21 as dead code. I'm not sure, cause I cannot test it, but it
> > > > should in theory
> > > > render MT8192 unusable, unless a new firmware has been submitted
> > > > to
> > > > linux-
> > > > firmware with MM21 support ?
> > > > 
> > > 
> > > If the firmware only support MT21 => won't exist for vp8 need to
> > > use
> > > MM21.
> > 
> > And that's the issue, the scp.img for MT8192 on linux-firmware only
> > supports
> > MT21 [1]. Can you please update it to support both MM21 and MT21?
> > 
> > For MT8195, only MM21 is supported in scp.img [2], but since the
> > hardware
> > supports both MM21 and MT21, the firmware should also support both.
> > So please
> > also update it on linux-firmware.
> > 
> > [1] 
> > https://lore.kernel.org/all/20230112204626.ciaff4amseoidybw@notapiano/
> > [2] 
> > https://lore.kernel.org/all/20230112205825.wb5qcqhh5kwvyi3y@notapiano/
> > 
> > Thanks,
> > Nícolas
> > 
> MT8192 always use MM21 from the beginning, MT21 have not been enabled.

That's not true, and you can verify through the following:

	$ curl https://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git/plain/mediatek/mt8192/scp.img > /lib/firmware/mediatek/mt8192/scp.img

	$ sha256sum /lib/firmware/mediatek/mt8192/scp.img
	fb9b4727f5e7cd82b2c63a8a2604d02e8c0f390eae2e9fd87aa1ae0797fb01b3  /lib/firmware/mediatek/mt8192/scp.img

	$ v4l2-ctl -d1 --list-formats -D
	Driver Info:
		Driver name      : mtk-vcodec-dec
		Card type        : MT8192 video decoder
		Bus info         : platform:16000000.video-codec
		Driver version   : 6.2.0
		Capabilities     : 0x84204000
			Video Memory-to-Memory Multiplanar
			Streaming
			Extended Pix Format
			Device Capabilities
		Device Caps      : 0x04204000
			Video Memory-to-Memory Multiplanar
			Streaming
			Extended Pix Format
	Media Driver Info:
		Driver name      : mtk-vcodec-dec
		Model            : mtk-vcodec-dec
		Serial           :
		Bus info         : platform:16000000.video-codec
		Media version    : 6.2.0
		Hardware revision: 0x00000000 (0)
		Driver version   : 6.2.0
	Interface Info:
		ID               : 0x0300000c
		Type             : V4L Video
	Entity Info:
		ID               : 0x00000001 (1)
		Name             : mtk-vcodec-dec-source
		Function         : V4L2 I/O
		Pad 0x01000002   : 0: Source
		 Link 0x02000008: to remote pad 0x1000004 of entity 'mtk-vcodec-dec-proc' (Video Decoder): Data, Enabled, Immutable
	ioctl: VIDIOC_ENUM_FMT
		Type: Video Capture Multiplanar

		[0]: 'MT21' (Mediatek Compressed Format, compressed)


So the decoder video node is clearly only exposing MT21 in the capture pad.

Furthermore, with

	$ echo module mtk_vcodec_dec +pmf > /sys/kernel/debug/dynamic_debug/control

and re-running the v4l2-ctl command, you'll see

	[ 3164.998781] mtk_vcodec_dec:fops_vcodec_open: fops_vcodec_open(),212: decoder capability 740

and if you check drivers/media/platform/mediatek/vcodec/mtk_vcodec_drv.h:

	enum mtk_vdec_format_types {
		MTK_VDEC_FORMAT_MM21 = 0x20,
		MTK_VDEC_FORMAT_MT21C = 0x40,
		MTK_VDEC_FORMAT_H264_SLICE = 0x100,
		MTK_VDEC_FORMAT_VP8_FRAME = 0x200,
		MTK_VDEC_FORMAT_VP9_FRAME = 0x400,
		MTK_VCODEC_INNER_RACING = 0x20000,
	};

meaning the scp.img firmware is exposing the MT21C but not the MM21 format.

So, again, the mt8192 scp.img firmware needs to be fixed.

Thanks,
Nícolas
Yunfei Dong March 14, 2023, 2:04 a.m. UTC | #7
Hi Nicolas,

Thanks for your suggestion.
On Fri, 2023-02-10 at 10:36 -0500, Nicolas Dufresne wrote:
> Le vendredi 10 février 2023 à 13:55 +0800, Yunfei Dong a écrit :
> > In order to conver the format of capture queue from mediatek MM21
> > to
> > standard yuv420 with Libyuv, need to force capture queue format to
> > MM21 for Libyuv can't covert mediatek MT21 format at current
> > period.
> 
> Please rework this text, it is hard to understand.
> 
Will re-write the commit message in next version as below, could you
please help to review whether it's well to be used ?

Libyuv is one software library used to covert format. Only covert
mediatek MM21 to standard yuv420 for MT21 is compressed mode and MM21
is uncompressed mode at current period. Need to set capture queue
format to MM21 in order to use Libyuv.

> > 
> > Fixes: 7501edef6b1f ("media: mediatek: vcodec: Different codec
> > using different capture format")
> > Signed-off-by: Yunfei Dong <yunfei.dong@mediatek.org>
> > ---
> > changed with v1:
> > - add Fixes tag.
> > ---
> >  drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git
> > a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c
> > b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c
> > index 641f533c417f..4f5e9c20214f 100644
> > --- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c
> > +++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c
> > @@ -41,7 +41,7 @@ static bool mtk_vdec_get_cap_fmt(struct
> > mtk_vcodec_ctx *ctx, int format_index)
> >  	const struct mtk_video_fmt *fmt;
> >  	struct mtk_q_data *q_data;
> >  	int num_frame_count = 0, i;
> > -	bool ret = true;
> > +	bool ret = false;
> >  
> >  	for (i = 0; i < *dec_pdata->num_formats; i++) {
> >  		if (dec_pdata->vdec_formats[i].type != MTK_FMT_FRAME)
> > @@ -63,7 +63,7 @@ static bool mtk_vdec_get_cap_fmt(struct
> > mtk_vcodec_ctx *ctx, int format_index)
> >  	case V4L2_PIX_FMT_H264_SLICE:
> >  	case V4L2_PIX_FMT_VP9_FRAME:
> >  		if (fmt->fourcc == V4L2_PIX_FMT_MM21)
> > -			ret = false;
> > +			ret = true;
> 
> This makes the VP8 and the other cases identical, leaving anything
> that touches
> MT21 as dead code. I'm not sure, cause I cannot test it, but it
> should in theory
> render MT8192 unusable, unless a new firmware has been submitted to
> linux-
> firmware with MM21 support ?
> 
If the firmware only support MT21 => won't exist for vp8 need to use
MM21.

If the firmware only support MM21, will using MM21.
If the firmware support MT21 & MM21, will force VP8 to use MM21,
H264/VP9/AV1 will use MT21 before this change. 

Will force all driver to use MM21 after adding this change. 

Best Regards,
Yunfei Dong
> >  		break;
> >  	default:
> >  		ret = true;
> 
>
Yunfei Dong March 14, 2023, 9:20 a.m. UTC | #8
Hi Nicolas,

Thanks for your suggestion.
On Tue, 2023-02-28 at 17:03 -0500, Nícolas F. R. A. Prado wrote:
> On Mon, Feb 27, 2023 at 02:17:08AM +0000, Yunfei Dong (董云飞) wrote:
> > Hi Nicolas,
> > 
> > Thanks for your suggestion.
> > On Wed, 2023-02-22 at 16:11 -0500, Nícolas F. R. A. Prado wrote:
> > > Hi,
> > > 
> > > On Tue, Feb 14, 2023 at 02:28:04AM +0000, Yunfei Dong (董云飞)
> > > wrote:
> > > > Hi Nicolas,
> > > > 
> > > > Thanks for your suggestion.
> > > > On Fri, 2023-02-10 at 10:36 -0500, Nicolas Dufresne wrote:
> > > > > Le vendredi 10 février 2023 à 13:55 +0800, Yunfei Dong a
> > > > > écrit :
> 
> [..]
> > > > > > diff --git
> > > > > > a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c
> > > > > > b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c
> > > > > > index 641f533c417f..4f5e9c20214f 100644
> > > > > > ---
> > > > > > a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c
> > > > > > +++
> > > > > > b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c
> > > > > > @@ -41,7 +41,7 @@ static bool mtk_vdec_get_cap_fmt(struct
> > > > > > mtk_vcodec_ctx *ctx, int format_index)
> > > > > >  	const struct mtk_video_fmt *fmt;
> > > > > >  	struct mtk_q_data *q_data;
> > > > > >  	int num_frame_count = 0, i;
> > > > > > -	bool ret = true;
> > > > > > +	bool ret = false;
> > > 
> > > This change doesn't do anything, so I'd drop it.
> > > 
> > 
> > This change is useful when capture fourcc is MT21 will return
> > false,
> > not support even if scp support mm21 and mt21.
> 
> But you have
> 
> 	default:
> 		ret = true;
> 		break;
> 
> down below, so you're handling all cases in the switch, and the
> original value
> doesn't matter. It could even be left uninitialized.
> 
> > > > > >  
> > > > > >  	for (i = 0; i < *dec_pdata->num_formats; i++) {
> > > > > >  		if (dec_pdata->vdec_formats[i].type !=
> > > > > > MTK_FMT_FRAME)
> > > > > > @@ -63,7 +63,7 @@ static bool mtk_vdec_get_cap_fmt(struct
> > > > > > mtk_vcodec_ctx *ctx, int format_index)
> > > > > >  	case V4L2_PIX_FMT_H264_SLICE:
> > > > > >  	case V4L2_PIX_FMT_VP9_FRAME:
> > > > > >  		if (fmt->fourcc == V4L2_PIX_FMT_MM21)
> > > > > > -			ret = false;
> > > > > > +			ret = true;
> > > > > 
> > > > > This makes the VP8 and the other cases identical, leaving
> > > > > anything
> > > > > that touches
> > > > > MT21 as dead code. I'm not sure, cause I cannot test it, but
> > > > > it
> > > > > should in theory
> > > > > render MT8192 unusable, unless a new firmware has been
> > > > > submitted
> > > > > to
> > > > > linux-
> > > > > firmware with MM21 support ?
> > > > > 
> > > > 
> > > > If the firmware only support MT21 => won't exist for vp8 need
> > > > to
> > > > use
> > > > MM21.
> > > 
> > > And that's the issue, the scp.img for MT8192 on linux-firmware
> > > only
> > > supports
> > > MT21 [1]. Can you please update it to support both MM21 and MT21?
> > > 
> > > For MT8195, only MM21 is supported in scp.img [2], but since the
> > > hardware
> > > supports both MM21 and MT21, the firmware should also support
> > > both.
> > > So please
> > > also update it on linux-firmware.
> > > 
> > > [1] 
> > > 
https://lore.kernel.org/all/20230112204626.ciaff4amseoidybw@notapiano/
> > > [2] 
> > > 
https://lore.kernel.org/all/20230112205825.wb5qcqhh5kwvyi3y@notapiano/
> > > 
> > > Thanks,
> > > Nícolas
> > > 
> > 
> > MT8192 always use MM21 from the beginning, MT21 have not been
> > enabled.
> 
> That's not true, and you can verify through the following:
> 
> 	$ curl 
> https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git/plain/mediatek/mt8192/scp.img__;!!CTRNKA9wMg0ARbw!i1Sq64yV1SsKzkxtFsZh4pfu8YwGA7ThO2zWtGyDRC4QDlzpmKxYJRvw3toH3oVDlHDxOpT73DZVWfe9OWeyj6if$
> $   > /lib/firmware/mediatek/mt8192/scp.img
> 
I have send the patch v3 to be reviewed. And Tinghan will help to
upstream mt8192 and mt8195's firmware scp.img which support MM21 and
MT21 at the same time.

Best Regards,
Yunfei Dong
> 	$ sha256sum /lib/firmware/mediatek/mt8192/scp.img
> 	fb9b4727f5e7cd82b2c63a8a2604d02e8c0f390eae2e9fd87aa1ae0797fb01b
> 3  /lib/firmware/mediatek/mt8192/scp.img
> 
> 	$ v4l2-ctl -d1 --list-formats -D
> 	Driver Info:
> 		Driver name      : mtk-vcodec-dec
> 		Card type        : MT8192 video decoder
> 		Bus info         : platform:16000000.video-codec
> 		Driver version   : 6.2.0
> 		Capabilities     : 0x84204000
> 			Video Memory-to-Memory Multiplanar
> 			Streaming
> 			Extended Pix Format
> 			Device Capabilities
> 		Device Caps      : 0x04204000
> 			Video Memory-to-Memory Multiplanar
> 			Streaming
> 			Extended Pix Format
> 	Media Driver Info:
> 		Driver name      : mtk-vcodec-dec
> 		Model            : mtk-vcodec-dec
> 		Serial           :
> 		Bus info         : platform:16000000.video-codec
> 		Media version    : 6.2.0
> 		Hardware revision: 0x00000000 (0)
> 		Driver version   : 6.2.0
> 	Interface Info:
> 		ID               : 0x0300000c
> 		Type             : V4L Video
> 	Entity Info:
> 		ID               : 0x00000001 (1)
> 		Name             : mtk-vcodec-dec-source
> 		Function         : V4L2 I/O
> 		Pad 0x01000002   : 0: Source
> 		 Link 0x02000008: to remote pad 0x1000004 of entity
> 'mtk-vcodec-dec-proc' (Video Decoder): Data, Enabled, Immutable
> 	ioctl: VIDIOC_ENUM_FMT
> 		Type: Video Capture Multiplanar
> 
> 		[0]: 'MT21' (Mediatek Compressed Format, compressed)
> 
> 
> So the decoder video node is clearly only exposing MT21 in the
> capture pad.
> 
> Furthermore, with
> 
> 	$ echo module mtk_vcodec_dec +pmf >
> /sys/kernel/debug/dynamic_debug/control
> 
> and re-running the v4l2-ctl command, you'll see
> 
> 	[ 3164.998781] mtk_vcodec_dec:fops_vcodec_open:
> fops_vcodec_open(),212: decoder capability 740
> 
> and if you check
> drivers/media/platform/mediatek/vcodec/mtk_vcodec_drv.h:
> 
> 	enum mtk_vdec_format_types {
> 		MTK_VDEC_FORMAT_MM21 = 0x20,
> 		MTK_VDEC_FORMAT_MT21C = 0x40,
> 		MTK_VDEC_FORMAT_H264_SLICE = 0x100,
> 		MTK_VDEC_FORMAT_VP8_FRAME = 0x200,
> 		MTK_VDEC_FORMAT_VP9_FRAME = 0x400,
> 		MTK_VCODEC_INNER_RACING = 0x20000,
> 	};
> 
> meaning the scp.img firmware is exposing the MT21C but not the MM21
> format.
> 
> So, again, the mt8192 scp.img firmware needs to be fixed.
> 
> Thanks,
> Nícolas
diff mbox series

Patch

diff --git a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c
index 641f533c417f..4f5e9c20214f 100644
--- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c
+++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c
@@ -41,7 +41,7 @@  static bool mtk_vdec_get_cap_fmt(struct mtk_vcodec_ctx *ctx, int format_index)
 	const struct mtk_video_fmt *fmt;
 	struct mtk_q_data *q_data;
 	int num_frame_count = 0, i;
-	bool ret = true;
+	bool ret = false;
 
 	for (i = 0; i < *dec_pdata->num_formats; i++) {
 		if (dec_pdata->vdec_formats[i].type != MTK_FMT_FRAME)
@@ -63,7 +63,7 @@  static bool mtk_vdec_get_cap_fmt(struct mtk_vcodec_ctx *ctx, int format_index)
 	case V4L2_PIX_FMT_H264_SLICE:
 	case V4L2_PIX_FMT_VP9_FRAME:
 		if (fmt->fourcc == V4L2_PIX_FMT_MM21)
-			ret = false;
+			ret = true;
 		break;
 	default:
 		ret = true;