Re: [PATCH v2 0/8] Add AMD ISP4 driver

[Du, Bin]

Thanks, Sultan.

On 8/28/2025 1:50 PM, Sultan Alsawaf wrote:
> On Wed, Aug 27, 2025 at 06:30:14PM +0800, Du, Bin wrote:
> > On 8/22/2025 11:56 AM, Sultan Alsawaf wrote:
> > > On Fri, Aug 22, 2025 at 10:23:48AM +0800, Du, Bin wrote:
> > > > On 8/14/2025 2:53 PM, Sultan Alsawaf wrote:
> > > > > On Wed, Jun 18, 2025 at 05:19:51PM +0800, Bin Du wrote:
> > > > > > AMD ISP4 Key features:
> > > > > > - Processes bayer raw data from the connected sensor and output them to different YUV formats
> > > > > > - Downscale input image to different output image resolution
> > > > > > - Pipeline to do image processing on the input image including demosaic, denoise, 3A, etc
> > > > >
> > > > > BTW, another problem I have which I would love some help with: may I get the FW
> > > > > commands for setting basic 3A parameters?
> > > > >
> > > > > It seems like the default AE mode does frame-averaging, which is really
> > > > > unsuitable for video calls on the webcam. My face is really underexposed as a
> > > > > result during the daytime because there's a lot of ambient light in the
> > > > > background.
> > > > >
> > > > > The webcam on this laptop also has a very wide field of view, which makes my
> > > > > face appear small and shows too much of the background. This also exacerbates
> > > > > the AE problem.
> > > > >
> > > > > I'm thinking CMD_ID_SET_ZOOM would fix the FOV problem, and then either
> > > > > CMD_ID_AE_SET_MODE to change the AE mode or CMD_ID_AE_SET_REGION to set the AE
> > > > > ROI would fix the exposure problem. What do you think?
> > > > >
> > > > > Thanks,
> > > > > Sultan
> > > >
> > > > Thanks Sultan for the suggestion, sorry for the late response because we
> > > > spent some time internally to discuss the feasibility.
> > >
> > > Thanks for looking into this!
> > >
> > > > Yes, it's really good suggestion. Because current V4l2 doesn't have standard
> > > > ioctl for like region setting, to support it, besides adding FW command, new
> > > > customized ioctl also needs be added and no existing APP can benefit from
> > > > it. So our idea is not to add them to our current upstream driver, but we
> > > > would be really glad to help you to enable them locally with dedicated
> > > > thread, suppose it can help to improve the IQ with correct input setting
> > > > like the correct ROI region, but we aren't sure because we didn't do that
> > > > before on Linux and would really expect your test result and feedback.
> > >
> > > I'm happy to help develop this and even help write the code. :)
> >
> > Thank you very much for your consistent help, that's invaluable.
>
> Of course! :)
>
> > > For advanced functions that don't have a standard V4L2 control, maybe we can set
> > > some defaults in the driver to improve the experience for front-facing cameras,
> > > i.e. webcams.
> > >
> > > Depending on the features exposed by FW, maybe setting a ROI for AE isn't
> > > necessary. Is it possible for the FW to do face detection and set 3A according
> > > to the face landmarks/ROI?
> >
> > Oh, so sad, our FW doesn't embed algorithm to do face detection. :(
>
> Ah, that's unfortunate. :(
>
> > > I think a lot of useful functionality can be put upstream just through V4L2,
> > > like V4L2_CID_EXPOSURE_METERING to control the AE mode.
> >
> > Yes, that's feasible, the consideration is if we add support to control AE
> > mode, as a complete solution, then we have to add support to set again and
> > exposure time in manual mode by FW command , besides that, extra work is
> > also needed from QA to develop new testcases, all these together will cost
> > more time. So, our current plan is first to upstream a version with basic
> > functions, after that, we can submit incremental patches to add more
> > features. Does it make sense?
>
> Yes, I understand.
>
> For now, what about just changing the default AE mode to center-weighted? I
> think this would produce a big IQ improvement since the typical usecase for the
> webcam is to show your face in the center of the frame. This change could even
> be made in the firmware instead of the driver.

Many thanks, you can always come up with good suggestions and ideas, 
that's really helpful. Yes, the ideal place will be in the FW. Have 
given this suggestion to FW guys, they mentioned they will check if 
there is center-wighted AE mode if got time, so far, it is never used 
and tuned.

> Or alternatively, HDR could be enabled by default.
>
> What do you think?

Yes, HDR should be able to improve the IQ, as you know, starting from 
patch V3, sensor driver will embed in FW and ISP driver will work just 
like webcam. To add HDR, we need support from sensor vendor, on the 
other hand, complete tuning and verification are also needed.

> Thanks,
> Sultan

--
Regards,
Bin