Re: [DKIM] [PATCH v21 4/4] media: platform: mtk-mdp3: add Mediatek MDP3 driver

[moudy.ho <moudy.ho@mediatek.com>]

On Fri, 2022-07-08 at 11:20 +0300, Laurent Pinchart wrote:
> On Fri, Jul 08, 2022 at 10:08:44AM +0200, Hans Verkuil wrote:
> > Hi Moudy,
> > 
> > Some comments below:
> 
> And one more
> 
> > On 7/6/22 09:54, Moudy Ho wrote:
> > > This patch adds driver for Mediatek's Media Data Path ver.3
> > > (MDP3).
> > > It provides the following functions:
> > >   color transform, format conversion, resize, crop, rotate, flip
> > >   and additional image quality enhancement.
> > > 
> > > The MDP3 driver is mainly used for Google Chromebook products to
> > > import the new architecture to set the HW settings as shown
> > > below:
> > >   User -> V4L2 framework
> > >     -> MDP3 driver -> SCP (setting calculations)
> > >       -> MDP3 driver -> CMDQ (GCE driver) -> HW
> > > 
> > > Each modules' related operation control is sited in mtk-mdp3-
> > > comp.c
> > > Each modules' register table is defined in file with "mdp_reg_"
> > > prefix
> > > GCE related API, operation control  sited in mtk-mdp3-cmdq.c
> > > V4L2 m2m device functions are implemented in mtk-mdp3-m2m.c
> > > Probe, power, suspend/resume, system level functions are defined
> > > in
> > > mtk-mdp3-core.c
> > > 
> > > v4l2-compliance 1.22.1, 32 bits, 32-bit time_t
> > > 
> > > Compliance test for mtk-mdp3 device /dev/video2:
> > > 
> > > Driver Info:
> > > 	Driver name      : mtk-mdp3
> > > 	Card type        : 14001000.mdp3-rdma0
> > 
> > Card type is expected to be a human readable name, and that's not
> > the case
> > here.
> > 
> > > 	Bus info         : platform:mtk-mdp3
> > 
> > <snip>
> > 
> > > diff --git a/drivers/media/platform/mediatek/mdp3/mtk-mdp3-m2m.c
> > > b/drivers/media/platform/mediatek/mdp3/mtk-mdp3-m2m.c
> > > new file mode 100644

[snip]

> > > +static const struct vb2_ops mdp_m2m_qops = {
> > > +	.queue_setup	= mdp_m2m_queue_setup,
> > > +	.wait_prepare	= vb2_ops_wait_prepare,
> > > +	.wait_finish	= vb2_ops_wait_finish,
> > > +	.buf_prepare	= mdp_m2m_buf_prepare,
> > > +	.start_streaming = mdp_m2m_start_streaming,
> > > +	.stop_streaming	= mdp_m2m_stop_streaming,
> > > +	.buf_queue	= mdp_m2m_buf_queue,
> > > +	.buf_out_validate = mdp_m2m_buf_out_validate,
> > > +};
> > > +
> > > +static int mdp_m2m_querycap(struct file *file, void *fh,
> > > +			    struct v4l2_capability *cap)
> > > +{
> > > +	struct mdp_m2m_ctx *ctx = fh_to_ctx(fh);
> > > +
> > > +	strscpy(cap->driver, MDP_MODULE_NAME, sizeof(cap->driver));
> > > +	strscpy(cap->card, ctx->mdp_dev->pdev->name, sizeof(cap-
> > > >card));
> > 
> > As mentioned at the top, this is not a suitable name for cap->card.
> > 
> > > +	strscpy(cap->bus_info, "platform:mtk-mdp3", sizeof(cap-
> > > >bus_info));
> 
> And don't override bus_info, the value isn't right. The V4L2 core
> should
> do the right thing for platform devices now.
> 

Hi Laurent,

Thanks for the reminder, I'll fix it in the next version along with
what Hans mentioned earlier.

Regards,
Moudy

> > > +	return 0;
> > > +}
> > > +
> > > +static int mdp_m2m_enum_fmt_mplane(struct file *file, void *fh,
> > > +				   struct v4l2_fmtdesc *f)
> > > +{
> > > +	return mdp_enum_fmt_mplane(f);
> > > +}