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

["Yunfei Dong (董云飞)" <Yunfei.Dong@mediatek.com>]

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