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) <[email protected]>
> >>>
> >>> 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
