Re: [PATCH v6 06/12] intel-ipu3: css: Add support for firmware management

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Yong,

On Wed, Sep 19, 2018 at 10:57:55PM +0000, Zhi, Yong wrote:
...
> > > +struct imgu_abi_osys_frame_params {
> > > +       /* Output pins */
> > > +       __u32 enable;
> > > +       __u32 format;           /* enum imgu_abi_osys_format */
> > > +       __u32 flip;
> > > +       __u32 mirror;
> > > +       __u32 tiling;           /* enum imgu_abi_osys_tiling */
> > > +       __u32 width;
> > > +       __u32 height;
> > > +       __u32 stride;
> > > +       __u32 scaled;
> > > +} __packed;
> > [snip]
> > > +/* Defect pixel correction */
> > > +
> > > +struct imgu_abi_dpc_config {
> > > +       __u8 __reserved[240832];
> > > +} __packed;
> > 
> > Do we need this structure? One could just add a reserved field in the parent
> > structure. Also, just to confirm, is 240832 really the right value here?
> > Where does it come from? Please create a macro for it, possibly further
> > breaking it down into the values used to compute this number.
> > 
> 
> We can add a reserved field in the parent structure, the size is based on
> original definition of dpc config which was removed since it's not
> enabled/used.

What's your plan with the DPC? If you don't plan to add it now, you could
as well drop the configuration for that block. If there's a need to add it
later on, you can still do it by defining a new struct for the buffer. Or
simply adding it at the end of the existing struct while allowing the use
of the old size without the DPC configuration.

There would be a little extra work to do there by that time when DPC
support would be added, but OTOH it seems silly to have quarter of a
megabyte of extra stuff to pass around in a struct that's never used.

-- 
Regards,

Sakari Ailus
sakari.ailus@linux.intel.com