RE: [Patch v5 09/11] media: s5p-mfc: Load firmware for each run in MFCv12.

[Aakarsh Jain]

> -----Original Message-----
> From: Hans Verkuil <hverkuil-cisco@xxxxxxxxx>
> Sent: 06 December 2023 18:28
> To: Aakarsh Jain <aakarsh.jain@xxxxxxxxxxx>; linux-arm-
> kernel@xxxxxxxxxxxxxxxxxxx; linux-media@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx
> Cc: m.szyprowski@xxxxxxxxxxx; andrzej.hajda@xxxxxxxxx;
> mchehab@xxxxxxxxxx; krzysztof.kozlowski+dt@xxxxxxxxxx;
> robh+dt@xxxxxxxxxx; conor+dt@xxxxxxxxxx; linux-samsung-
> soc@xxxxxxxxxxxxxxx; andi@xxxxxxxxxxx; gost.dev@xxxxxxxxxxx;
> alim.akhtar@xxxxxxxxxxx; aswani.reddy@xxxxxxxxxxx;
> pankaj.dubey@xxxxxxxxxxx; ajaykumar.rs@xxxxxxxxxxx; linux-
> fsd@xxxxxxxxx; Smitha T Murthy <smithatmurthy@xxxxxxxxx>
> Subject: Re: [Patch v5 09/11] media: s5p-mfc: Load firmware for each run in
> MFCv12.
> 
> On 06/12/2023 07:30, Aakarsh Jain wrote:
> > In MFCv12, some section of firmware gets updated at each MFC run.
> > Hence we need to reload original firmware for each run at the start.
> >
> > Cc: linux-fsd@xxxxxxxxx
> > Signed-off-by: Smitha T Murthy <smithatmurthy@xxxxxxxxx>
> > Signed-off-by: Aakarsh Jain <aakarsh.jain@xxxxxxxxxxx>
> > ---
> >  drivers/media/platform/samsung/s5p-mfc/s5p_mfc_ctrl.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/media/platform/samsung/s5p-mfc/s5p_mfc_ctrl.c
> > b/drivers/media/platform/samsung/s5p-mfc/s5p_mfc_ctrl.c
> > index b49159142c53..24dd40ae71ec 100644
> > --- a/drivers/media/platform/samsung/s5p-mfc/s5p_mfc_ctrl.c
> > +++ b/drivers/media/platform/samsung/s5p-mfc/s5p_mfc_ctrl.c
> > @@ -51,8 +51,10 @@ int s5p_mfc_load_firmware(struct s5p_mfc_dev
> *dev)
> >  	 * into kernel. */
> >  	mfc_debug_enter();
> >
> > -	if (dev->fw_get_done)
> > -		return 0;
> > +	/* Load MFC v12 firmware for each run when MFC runs sequentially
> */
> 
> You had a much longer explanation in your reply to my original question.
> 
> I think it is better if that longer explanation is added here.
> 
okay sure. Will add that explanation here.

> Things that are weird and unexpected need good comments, explaining why
> it is done, and also what you know and do not know about this.
> 
> E.g. you know through trial and error that it is needed (or perhaps you got
> information on this some the fw team), but there might be open questions
> that are not yet answered.
> 
> This is all information that you can't get from the source code since it has to
> do with the black box firmware. So putting all you know in a comment is the
> best way of communicating this to future readers of the source code.
> 

Thanks for the review!

> Regards,
> 
> 	Hans
> 
> > +	if (!IS_MFCV12(dev))
> > +		if (dev->fw_get_done)
> > +			return 0;
> >
> >  	for (i = MFC_FW_MAX_VERSIONS - 1; i >= 0; i--) {
> >  		if (!dev->variant->fw_name[i])