Re: [PATCH v11 2/3] drm/bridge: imx: Add i.MX93 parallel display format configuration support

[Frank Li <Frank.li@nxp.com>]

On Wed, Mar 04, 2026 at 11:29:03AM +0800, Liu Ying wrote:
> On Tue, Mar 03, 2026 at 05:20:09PM -0500, Frank Li wrote:
> > On Tue, Mar 03, 2026 at 10:22:02PM +0100, Marco Felsch wrote:
> >> On 26-03-03, Frank Li wrote:
> >>> From: Frank Li (AI-BOT) <frank.li@nxp.com>
> >>>
> >>> AI bot review and may be useless.
> >>
> >> Hi Frank,
> >>
> >> albeit I'm very open to new technology, I would appreciate it if your
> >> AI-BOT is used internally first till you're convinced that it reports
> >> real issues instead of false-positives.
> >>
> >> Regards,
> >>   Marco
> >>
> >>>> +static u32 *
> >>>> +imx93_pdfc_bridge_atomic_get_input_bus_fmts(struct drm_bridge *bridge,
> >>>> +					    struct drm_bridge_state *bridge_state,
> >>>> +					    struct drm_crtc_state *crtc_state,
> >>>> +					    struct drm_connector_state *conn_state,
> >>>> +					    u32 output_fmt,
> >>>> +					    unsigned int *num_input_fmts)
> >>>> +{
> >>>> +	struct imx93_pdfc *pdfc = bridge_to_imx93_pdfc(bridge);
> >>>> +	u32 *input_fmts;
> >>>> +
> >>>> +	*num_input_fmts = 0;
> >>>> +
> >>>> +	input_fmts = kmalloc_obj(*input_fmts);
> >>>> +	if (!input_fmts)
> >>>> +		return NULL;
> >>>
> >
> >> +     input_fmts = kmalloc_obj(*input_fmts);
> >
> > AI:
> >
> > Actually, this looks incorrect. kmalloc_obj() allocates based on the type
> > of the pointer argument. Here you're passing *input_fmts (a u32), not
> > input_fmts (a u32 *).
>
> This comment is false.  Per kmalloc_obj()'s kerneldoc, it's first argument
> is "Variable or type to allocate".  In this particular case, the argument
> should be "*input_fmts (a u32)".

Yes, I wrongly think it is num_input_fmts and return array point for multi
formats, actually it is just 1 items for format.

imx93_pdfc_bridge_atomic_get_input_bus_fmts() is quite likely return many
format array especially has "s" after fmt.

Frank

>
> /**
>  * kmalloc_obj - Allocate a single instance of the given type
>  * @VAR_OR_TYPE: Variable or type to allocate.
>  * @GFP: GFP flags for the allocation.
>  *
>  * Returns: newly allocated pointer to a @VAR_OR_TYPE on success, or NULL
>  * on failure.
>  */
> #define kmalloc_obj(VAR_OR_TYPE, ...) \
>         __alloc_objs(kmalloc, default_gfp(__VA_ARGS__), typeof(VAR_OR_TYPE), 1)
>
> > Should be:
> >
> > +       input_fmts = kmalloc_array(*num_input_fmts, sizeof(*input_fmts),
> > +                                  GFP_KERNEL);
>
> No.  "input_fmts = kmalloc_obj(*input_fmts);" is just fine.
>
> >
> > suppose you use kmalloc_objs()
>
> No. We should use kmalloc_obj(), not kmalloc_objs().
>
> >
> > Frank
> >
> >>> Missing kfree(input_fmts) in error path if the switch statement
> >>> or subsequent logic fails. Consider allocating a fixed-size array
> >>> or using devm_kzalloc() instead.
> >>>
> >>>> +	*num_input_fmts = 1;
> >>>> +
> >>>> +	if (!imx93_pdfc_bus_output_fmt_supported(output_fmt)) {
> >>>> +		dev_dbg(pdfc->dev, "No valid output bus-fmt detected, fallback to MEDIA_BUS_FMT_RGB888_1X24\n");
> >>>
> >>> Line exceeds 80 characters (97 chars). Break into two lines.
> >>>
> >>>> +		input_fmts[0] = MEDIA_BUS_FMT_RGB888_1X24;
> >>>> +		return input_fmts;
> >>>> +	}
> >>>> +
> >>>> +	switch (output_fmt) {
> >>>> +	case MEDIA_BUS_FMT_RGB888_1X24:
> >>>> +	case MEDIA_BUS_FMT_RGB565_1X16:
> >>>> +		input_fmts[0] = output_fmt;
> >>>> +		break;
> >>>> +	case MEDIA_BUS_FMT_RGB666_1X18:
> >>>> +	case MEDIA_BUS_FMT_FIXED:
> >>>> +		input_fmts[0] = MEDIA_BUS_FMT_RGB888_1X24;
> >>>> +		break;
> >>>> +	}
> >>>
> >>> Switch statement lacks default case. Add default case to handle
> >>> unexpected format values explicitly.
> >>>
> >>>> +static int imx93_pdfc_bridge_atomic_enable(struct drm_bridge *bridge,
> >>>> +					    struct drm_atomic_state *state)
> >>>> +{
> >>>> +	struct imx93_pdfc *pdfc = bridge_to_imx93_pdfc(bridge);
> >>>> +	const struct drm_bridge_state *bridge_state;
> >>>> +	unsigned int mask = PARALLEL_DISP_FORMAT;
> >>>> +	unsigned int val;
> >>>> +
> >>>> +	bridge_state = drm_atomic_get_new_bridge_state(state, bridge);
> >>>> +
> >>>> +	switch (bridge_state->output_bus_cfg.format) {
> >>>> +	case MEDIA_BUS_FMT_RGB888_1X24:
> >>>> +	case MEDIA_BUS_FMT_FIXED:
> >>>> +		val = FORMAT_RGB888_TO_RGB888;
> >>>> +		if (pdfc->phy_bus_width == 18) {
> >>>> +			/*
> >>>> +			 * Can be valid if physical bus limitation exists,
> >>>> +			 * therefore use dev_dbg().
> >>>> +			 */
> >>>> +			dev_dbg(pdfc->dev, "Truncate two LSBs from each color\n");
> >>>> +			val = FORMAT_RGB888_TO_RGB666;
> >>>> +		}
> >>>> +		break;
> >>>> +	case MEDIA_BUS_FMT_RGB666_1X18:
> >>>> +		val = FORMAT_RGB888_TO_RGB666;
> >>>> +
> >>>
> >>
> >> --
> >> #gernperDu
> >> #CallMeByMyFirstName
> >>
> >> Pengutronix e.K.                           |                             |
> >> Steuerwalder Str. 21                       | https://www.pengutronix.de/ |
> >> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> >> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-9    |
>
> --
> Regards,
> Liu Ying