* Re: [PATCH v11 2/3] drm/bridge: imx: Add i.MX93 parallel display format configuration support
2026-03-03 10:34 ` [PATCH v11 2/3] drm/bridge: imx: Add i.MX93 parallel display format configuration support Marco Felsch
@ 2026-03-03 21:00 ` Frank Li
2026-03-03 21:22 ` Marco Felsch
2026-03-04 3:32 ` Liu Ying
` (2 subsequent siblings)
3 siblings, 1 reply; 25+ messages in thread
From: Frank Li @ 2026-03-03 21:00 UTC (permalink / raw)
To: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Shawn Guo,
Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam, Peng Fan,
Liu Ying, Andrzej Hajda, Neil Armstrong, Robert Foss,
Laurent Pinchart, Jonas Karlman, Jernej Skrabec,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
Simona Vetter, luca.ceresoli, Frank Li
Cc: Frank Li, devicetree, imx, linux-arm-kernel, linux-kernel,
dri-devel, Marco Felsch
From: Frank Li (AI-BOT) <frank.li@nxp.com>
AI bot review and may be useless.
---
> +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;
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;
> +
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH v11 2/3] drm/bridge: imx: Add i.MX93 parallel display format configuration support
2026-03-03 21:00 ` Frank Li
@ 2026-03-03 21:22 ` Marco Felsch
2026-03-03 22:07 ` Frank Li
2026-03-03 22:20 ` Frank Li
0 siblings, 2 replies; 25+ messages in thread
From: Marco Felsch @ 2026-03-03 21:22 UTC (permalink / raw)
To: Frank Li
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Shawn Guo,
Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam, Peng Fan,
Liu Ying, Andrzej Hajda, Neil Armstrong, Robert Foss,
Laurent Pinchart, Jonas Karlman, Jernej Skrabec,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
Simona Vetter, luca.ceresoli, devicetree, imx, linux-arm-kernel,
linux-kernel, dri-devel
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;
>
> 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 |
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH v11 2/3] drm/bridge: imx: Add i.MX93 parallel display format configuration support
2026-03-03 21:22 ` Marco Felsch
@ 2026-03-03 22:07 ` Frank Li
2026-03-03 22:20 ` Frank Li
1 sibling, 0 replies; 25+ messages in thread
From: Frank Li @ 2026-03-03 22:07 UTC (permalink / raw)
To: Marco Felsch
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Shawn Guo,
Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam, Peng Fan,
Liu Ying, Andrzej Hajda, Neil Armstrong, Robert Foss,
Laurent Pinchart, Jonas Karlman, Jernej Skrabec,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
Simona Vetter, luca.ceresoli, devicetree, imx, linux-arm-kernel,
linux-kernel, dri-devel
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.
I tested 22 patches, which sent to imx mail list only. Need test script.
Impact should be limited.
Frank
>
> 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;
> >
> > 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 |
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH v11 2/3] drm/bridge: imx: Add i.MX93 parallel display format configuration support
2026-03-03 21:22 ` Marco Felsch
2026-03-03 22:07 ` Frank Li
@ 2026-03-03 22:20 ` Frank Li
2026-03-04 3:29 ` Liu Ying
1 sibling, 1 reply; 25+ messages in thread
From: Frank Li @ 2026-03-03 22:20 UTC (permalink / raw)
To: Marco Felsch
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Shawn Guo,
Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam, Peng Fan,
Liu Ying, Andrzej Hajda, Neil Armstrong, Robert Foss,
Laurent Pinchart, Jonas Karlman, Jernej Skrabec,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
Simona Vetter, luca.ceresoli, devicetree, imx, linux-arm-kernel,
linux-kernel, dri-devel
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 *). Should be:
+ input_fmts = kmalloc_array(*num_input_fmts, sizeof(*input_fmts),
+ GFP_KERNEL);
suppose you use 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 |
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH v11 2/3] drm/bridge: imx: Add i.MX93 parallel display format configuration support
2026-03-03 22:20 ` Frank Li
@ 2026-03-04 3:29 ` Liu Ying
2026-03-04 3:53 ` Frank Li
0 siblings, 1 reply; 25+ messages in thread
From: Liu Ying @ 2026-03-04 3:29 UTC (permalink / raw)
To: Frank Li, Marco Felsch
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Shawn Guo,
Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam, Peng Fan,
Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart,
Jonas Karlman, Jernej Skrabec, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, luca.ceresoli,
devicetree, imx, linux-arm-kernel, linux-kernel, dri-devel
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)".
/**
* 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
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH v11 2/3] drm/bridge: imx: Add i.MX93 parallel display format configuration support
2026-03-04 3:29 ` Liu Ying
@ 2026-03-04 3:53 ` Frank Li
0 siblings, 0 replies; 25+ messages in thread
From: Frank Li @ 2026-03-04 3:53 UTC (permalink / raw)
To: Liu Ying
Cc: Marco Felsch, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
Peng Fan, Andrzej Hajda, Neil Armstrong, Robert Foss,
Laurent Pinchart, Jonas Karlman, Jernej Skrabec,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
Simona Vetter, luca.ceresoli, devicetree, imx, linux-arm-kernel,
linux-kernel, dri-devel
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
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v11 2/3] drm/bridge: imx: Add i.MX93 parallel display format configuration support
2026-03-03 10:34 ` [PATCH v11 2/3] drm/bridge: imx: Add i.MX93 parallel display format configuration support Marco Felsch
2026-03-03 21:00 ` Frank Li
@ 2026-03-04 3:32 ` Liu Ying
2026-03-05 14:37 ` Alexander Stein
2026-03-10 2:57 ` Liu Ying
3 siblings, 0 replies; 25+ messages in thread
From: Liu Ying @ 2026-03-04 3:32 UTC (permalink / raw)
To: Marco Felsch, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
Peng Fan, Andrzej Hajda, Neil Armstrong, Robert Foss,
Laurent Pinchart, Jonas Karlman, Jernej Skrabec,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
Simona Vetter, luca.ceresoli, Frank Li
Cc: devicetree, imx, linux-arm-kernel, linux-kernel, dri-devel
On Tue, Mar 03, 2026 at 11:34:27AM +0100, Marco Felsch wrote:
> From: Liu Ying <victor.liu@nxp.com>
>
> NXP i.MX93 mediamix blk-ctrl contains one DISPLAY_MUX register which
> configures parallel display format by using the "PARALLEL_DISP_FORMAT"
> field. Add a DRM bridge driver to support the display format configuration.
>
> Reviewed-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> Signed-off-by: Liu Ying <victor.liu@nxp.com>
> [m.felsch@pengutronix.de: port to v7.0-rc1]
> [m.felsch@pengutronix.de: add review feedback (Alexander)]
> [m.felsch@pengutronix.de: fix to short Kconfig description (checkpath)]
> [m.felsch@pengutronix.de: use "GPL" instead of "GPL v2" (checkpatch)]
> [m.felsch@pengutronix.de: add bus-width support]
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
> drivers/gpu/drm/bridge/imx/Kconfig | 11 ++
> drivers/gpu/drm/bridge/imx/Makefile | 1 +
> drivers/gpu/drm/bridge/imx/imx93-pdfc.c | 225 ++++++++++++++++++++++++++++++++
> 3 files changed, 237 insertions(+)
Reviewed-by: Liu Ying <victor.liu@nxp.com>
Thanks!
I'll wait for a week or so to apply this if no objections or someone else
applies it sooner.
--
Regards,
Liu Ying
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v11 2/3] drm/bridge: imx: Add i.MX93 parallel display format configuration support
2026-03-03 10:34 ` [PATCH v11 2/3] drm/bridge: imx: Add i.MX93 parallel display format configuration support Marco Felsch
2026-03-03 21:00 ` Frank Li
2026-03-04 3:32 ` Liu Ying
@ 2026-03-05 14:37 ` Alexander Stein
2026-03-10 2:57 ` Liu Ying
3 siblings, 0 replies; 25+ messages in thread
From: Alexander Stein @ 2026-03-05 14:37 UTC (permalink / raw)
To: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Shawn Guo,
Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam, Peng Fan,
Liu Ying, Andrzej Hajda, Neil Armstrong, Robert Foss,
Laurent Pinchart, Jonas Karlman, Jernej Skrabec,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
Simona Vetter, luca.ceresoli, Frank Li, dri-devel, Marco Felsch
Cc: devicetree, imx, linux-arm-kernel, linux-kernel, dri-devel,
kernel, Marco Felsch
[-- Attachment #1: Type: text/plain, Size: 10304 bytes --]
Am Dienstag, 3. März 2026, 11:34:27 CET schrieb Marco Felsch:
> From: Liu Ying <victor.liu@nxp.com>
>
> NXP i.MX93 mediamix blk-ctrl contains one DISPLAY_MUX register which
> configures parallel display format by using the "PARALLEL_DISP_FORMAT"
> field. Add a DRM bridge driver to support the display format configuration.
>
> Reviewed-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> Signed-off-by: Liu Ying <victor.liu@nxp.com>
> [m.felsch@pengutronix.de: port to v7.0-rc1]
> [m.felsch@pengutronix.de: add review feedback (Alexander)]
> [m.felsch@pengutronix.de: fix to short Kconfig description (checkpath)]
> [m.felsch@pengutronix.de: use "GPL" instead of "GPL v2" (checkpatch)]
> [m.felsch@pengutronix.de: add bus-width support]
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
Tested-by: Alexander Stein <alexander.stein@ew.tq-group.com>
on TQMa93xxCA on MBa91xxCA
> ---
> drivers/gpu/drm/bridge/imx/Kconfig | 11 ++
> drivers/gpu/drm/bridge/imx/Makefile | 1 +
> drivers/gpu/drm/bridge/imx/imx93-pdfc.c | 225 ++++++++++++++++++++++++++++++++
> 3 files changed, 237 insertions(+)
>
> diff --git a/drivers/gpu/drm/bridge/imx/Kconfig b/drivers/gpu/drm/bridge/imx/Kconfig
> index b9028a5e5a065c3237b404111d8df57e8e017f9d..47829300a4486a090514ebe914b7667a703039a9 100644
> --- a/drivers/gpu/drm/bridge/imx/Kconfig
> +++ b/drivers/gpu/drm/bridge/imx/Kconfig
> @@ -99,4 +99,15 @@ config DRM_IMX93_MIPI_DSI
> Choose this to enable MIPI DSI controller found in Freescale i.MX93
> processor.
>
> +config DRM_IMX93_PARALLEL_DISP_FMT_CONVERTER
> + tristate "NXP i.MX91/i.MX93 parallel display format converter"
> + depends on OF
> + select DRM_KMS_HELPER
> + help
> + On i.MX93 and i.MX91 SoCs the parallel display format output is
> + controlled via the MEDIAMIX BLK-CTRL DISPLAY_MUX.
> +
> + Say 'Y' or 'M' if you use the parallel display output path on a
> + i.MX93 or i.MX91 SoC.
> +
> endif # ARCH_MXC || COMPILE_TEST
> diff --git a/drivers/gpu/drm/bridge/imx/Makefile b/drivers/gpu/drm/bridge/imx/Makefile
> index 8d01fda25451aaa1bf51a068da18948094327116..da57fde2d813b88cdde89115ca801b4cfc69afbd 100644
> --- a/drivers/gpu/drm/bridge/imx/Makefile
> +++ b/drivers/gpu/drm/bridge/imx/Makefile
> @@ -9,3 +9,4 @@ obj-$(CONFIG_DRM_IMX8QXP_PIXEL_COMBINER) += imx8qxp-pixel-combiner.o
> obj-$(CONFIG_DRM_IMX8QXP_PIXEL_LINK) += imx8qxp-pixel-link.o
> obj-$(CONFIG_DRM_IMX8QXP_PIXEL_LINK_TO_DPI) += imx8qxp-pxl2dpi.o
> obj-$(CONFIG_DRM_IMX93_MIPI_DSI) += imx93-mipi-dsi.o
> +obj-$(CONFIG_DRM_IMX93_PARALLEL_DISP_FMT_CONVERTER) += imx93-pdfc.o
> diff --git a/drivers/gpu/drm/bridge/imx/imx93-pdfc.c b/drivers/gpu/drm/bridge/imx/imx93-pdfc.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..27a54424319838517b206ab7d7447538aa1489eb
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/imx/imx93-pdfc.c
> @@ -0,0 +1,225 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +
> +/*
> + * Copyright 2022-2025 NXP
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/media-bus-format.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_graph.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
> +#include <drm/drm_atomic_state_helper.h>
> +#include <drm/drm_bridge.h>
> +
> +#define IMX93_DISPLAY_MUX_REG 0x60
> +#define PARALLEL_DISP_FORMAT GENMASK(10, 8)
> +#define FORMAT_RGB888_TO_RGB888 FIELD_PREP(PARALLEL_DISP_FORMAT, 0)
> +#define FORMAT_RGB888_TO_RGB666 FIELD_PREP(PARALLEL_DISP_FORMAT, 1)
> +#define FORMAT_RGB565_TO_RGB565 FIELD_PREP(PARALLEL_DISP_FORMAT, 2)
> +
> +struct imx93_pdfc {
> + struct drm_bridge bridge;
> + struct device *dev;
> + struct regmap *regmap;
> + u32 phy_bus_width;
> +};
> +
> +static struct imx93_pdfc *bridge_to_imx93_pdfc(struct drm_bridge *bridge)
> +{
> + return container_of(bridge, struct imx93_pdfc, bridge);
> +}
> +
> +static int
> +imx93_pdfc_bridge_attach(struct drm_bridge *bridge, struct drm_encoder *encoder,
> + enum drm_bridge_attach_flags flags)
> +{
> + return drm_bridge_attach(bridge->encoder, bridge->next_bridge, bridge, flags);
> +}
> +
> +static void 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;
> + break;
> + case MEDIA_BUS_FMT_RGB565_1X16:
> + val = FORMAT_RGB565_TO_RGB565;
> + break;
> + }
> +
> + regmap_update_bits(pdfc->regmap, IMX93_DISPLAY_MUX_REG, mask, val);
> +}
> +
> +/* TODO: Add YUV formats */
> +static const u32 imx93_pdfc_bus_output_fmts[] = {
> + MEDIA_BUS_FMT_FIXED,
> + MEDIA_BUS_FMT_RGB888_1X24,
> + MEDIA_BUS_FMT_RGB666_1X18,
> + MEDIA_BUS_FMT_RGB565_1X16,
> +};
> +
> +static bool imx93_pdfc_bus_output_fmt_supported(u32 fmt)
> +{
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(imx93_pdfc_bus_output_fmts); i++) {
> + if (imx93_pdfc_bus_output_fmts[i] == fmt)
> + return true;
> + }
> +
> + return false;
> +}
> +
> +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;
> +
> + *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");
> + 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;
> + }
> +
> + return input_fmts;
> +}
> +
> +static int imx93_pdfc_bridge_atomic_check(struct drm_bridge *bridge,
> + struct drm_bridge_state *bridge_state,
> + struct drm_crtc_state *crtc_state,
> + struct drm_connector_state *conn_state)
> +{
> + struct imx93_pdfc *pdfc = bridge_to_imx93_pdfc(bridge);
> + u32 format = bridge_state->output_bus_cfg.format;
> +
> + if (imx93_pdfc_bus_output_fmt_supported(format))
> + return 0;
> +
> + dev_warn(pdfc->dev, "Unsupported output bus format: 0x%x\n", format);
> +
> + return -EINVAL;
> +}
> +
> +static const struct drm_bridge_funcs funcs = {
> + .attach = imx93_pdfc_bridge_attach,
> + .atomic_enable = imx93_pdfc_bridge_atomic_enable,
> + .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
> + .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
> + .atomic_get_input_bus_fmts = imx93_pdfc_bridge_atomic_get_input_bus_fmts,
> + .atomic_check = imx93_pdfc_bridge_atomic_check,
> + .atomic_reset = drm_atomic_helper_bridge_reset,
> +};
> +
> +static int imx93_pdfc_bridge_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct drm_bridge *next_bridge;
> + struct imx93_pdfc *pdfc;
> + struct device_node *ep;
> + int err;
> +
> + pdfc = devm_drm_bridge_alloc(dev, struct imx93_pdfc, bridge, &funcs);
> + if (IS_ERR(pdfc))
> + return PTR_ERR(pdfc);
> +
> + pdfc->regmap = syscon_node_to_regmap(dev->of_node->parent);
> + if (IS_ERR(pdfc->regmap))
> + return dev_err_probe(dev, PTR_ERR(pdfc->regmap),
> + "failed to get regmap\n");
> +
> + /* No limits per default */
> + pdfc->phy_bus_width = 24;
> +
> + /* Get output ep (port1/endpoint) */
> + ep = of_graph_get_endpoint_by_regs(dev->of_node, 1, -1);
> + if (ep) {
> + err = of_property_read_u32(ep, "bus-width", &pdfc->phy_bus_width);
> + of_node_put(ep);
> +
> + /* bus-width is optional but it must have valid data if present */
> + if (err && err != -EINVAL)
> + return dev_err_probe(dev, err,
> + "failed to query bus-width\n");
> + }
> +
> + next_bridge = devm_drm_of_get_bridge(dev, dev->of_node, 1, 0);
> + if (IS_ERR(next_bridge))
> + return dev_err_probe(dev, PTR_ERR(next_bridge),
> + "failed to get next bridge\n");
> + pdfc->dev = dev;
> + pdfc->bridge.of_node = dev->of_node;
> + pdfc->bridge.type = DRM_MODE_CONNECTOR_DPI;
> + pdfc->bridge.next_bridge = next_bridge;
> +
> + return devm_drm_bridge_add(dev, &pdfc->bridge);
> +}
> +
> +static const struct of_device_id imx93_pdfc_dt_ids[] = {
> + { .compatible = "nxp,imx93-pdfc", },
> + { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, imx93_pdfc_dt_ids);
> +
> +static struct platform_driver imx93_pdfc_bridge_driver = {
> + .probe = imx93_pdfc_bridge_probe,
> + .driver = {
> + .of_match_table = imx93_pdfc_dt_ids,
> + .name = "imx93_pdfc",
> + },
> +};
> +module_platform_driver(imx93_pdfc_bridge_driver);
> +
> +MODULE_DESCRIPTION("NXP i.MX93 parallel display format configuration driver");
> +MODULE_AUTHOR("Liu Ying <victor.liu@nxp.com>");
> +MODULE_LICENSE("GPL");
>
>
--
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH v11 2/3] drm/bridge: imx: Add i.MX93 parallel display format configuration support
2026-03-03 10:34 ` [PATCH v11 2/3] drm/bridge: imx: Add i.MX93 parallel display format configuration support Marco Felsch
` (2 preceding siblings ...)
2026-03-05 14:37 ` Alexander Stein
@ 2026-03-10 2:57 ` Liu Ying
2026-03-10 11:53 ` Luca Ceresoli
3 siblings, 1 reply; 25+ messages in thread
From: Liu Ying @ 2026-03-10 2:57 UTC (permalink / raw)
To: Marco Felsch, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
Peng Fan, Andrzej Hajda, Neil Armstrong, Robert Foss,
Laurent Pinchart, Jonas Karlman, Jernej Skrabec,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
Simona Vetter, luca.ceresoli, Frank Li
Cc: devicetree, imx, linux-arm-kernel, linux-kernel, dri-devel
Hi Marco, Luca,
On Tue, Mar 03, 2026 at 11:34:27AM +0100, Marco Felsch wrote:
[...]
> + next_bridge = devm_drm_of_get_bridge(dev, dev->of_node, 1, 0);
> + if (IS_ERR(next_bridge))
> + return dev_err_probe(dev, PTR_ERR(next_bridge),
> + "failed to get next bridge\n");
> + pdfc->dev = dev;
> + pdfc->bridge.of_node = dev->of_node;
> + pdfc->bridge.type = DRM_MODE_CONNECTOR_DPI;
> + pdfc->bridge.next_bridge = next_bridge;
When I was reviewing another patch[1], I was aware of the necessity of
calling drm_bridge_get() for next_bridge to balance the next bridge's
refcount put from __drm_bridge_free() for this bridge. I'd be good if
Luca may confirm this is correct. Sorry for bringing this up late.
[1] https://lore.kernel.org/all/5edd40f3-f83e-4a1b-99d4-1e595fc0e4cb@nxp.com/
--
Regards,
Liu Ying
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH v11 2/3] drm/bridge: imx: Add i.MX93 parallel display format configuration support
2026-03-10 2:57 ` Liu Ying
@ 2026-03-10 11:53 ` Luca Ceresoli
2026-03-11 3:15 ` Liu Ying
2026-03-11 19:45 ` Marco Felsch
0 siblings, 2 replies; 25+ messages in thread
From: Luca Ceresoli @ 2026-03-10 11:53 UTC (permalink / raw)
To: Liu Ying, Marco Felsch, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team,
Fabio Estevam, Peng Fan, Andrzej Hajda, Neil Armstrong,
Robert Foss, Laurent Pinchart, Jonas Karlman, Jernej Skrabec,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
Simona Vetter, Frank Li
Cc: devicetree, imx, linux-arm-kernel, linux-kernel, dri-devel
Hi Liu, Marco,
On Tue Mar 10, 2026 at 3:57 AM CET, Liu Ying wrote:
> Hi Marco, Luca,
>
> On Tue, Mar 03, 2026 at 11:34:27AM +0100, Marco Felsch wrote:
>
> [...]
>
>> + next_bridge = devm_drm_of_get_bridge(dev, dev->of_node, 1, 0);
>> + if (IS_ERR(next_bridge))
>> + return dev_err_probe(dev, PTR_ERR(next_bridge),
>> + "failed to get next bridge\n");
>> + pdfc->dev = dev;
>> + pdfc->bridge.of_node = dev->of_node;
>> + pdfc->bridge.type = DRM_MODE_CONNECTOR_DPI;
>> + pdfc->bridge.next_bridge = next_bridge;
>
> When I was reviewing another patch[1], I was aware of the necessity of
> calling drm_bridge_get() for next_bridge to balance the next bridge's
> refcount put from __drm_bridge_free() for this bridge. I'd be good if
> Luca may confirm this is correct. Sorry for bringing this up late.
Indeed you have a good point.
After re-checking devm_drm_of_get_bridge(), as I wrote on the other thread
you pointed to, you should call drm_bridge_get():
- pdfc->bridge.next_bridge = next_bridge;
+ pdfc->bridge.next_bridge = drm_bridge_get(next_bridge);
Marco, you can keep my R-by if you resend with just this change.
Sorry about the confusion here.
As mention on the other thread, devm_drm_of_get_bridge() is unable to
support bridge hotplug. So it should be deprecated, but as of now there is
no alternative.
Luca
--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v11 2/3] drm/bridge: imx: Add i.MX93 parallel display format configuration support
2026-03-10 11:53 ` Luca Ceresoli
@ 2026-03-11 3:15 ` Liu Ying
2026-03-11 19:45 ` Marco Felsch
1 sibling, 0 replies; 25+ messages in thread
From: Liu Ying @ 2026-03-11 3:15 UTC (permalink / raw)
To: Luca Ceresoli, Marco Felsch, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team,
Fabio Estevam, Peng Fan, Andrzej Hajda, Neil Armstrong,
Robert Foss, Laurent Pinchart, Jonas Karlman, Jernej Skrabec,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
Simona Vetter, Frank Li
Cc: devicetree, imx, linux-arm-kernel, linux-kernel, dri-devel
On Tue, Mar 10, 2026 at 12:53:07PM +0100, Luca Ceresoli wrote:
> Hi Liu, Marco,
>
> On Tue Mar 10, 2026 at 3:57 AM CET, Liu Ying wrote:
>> Hi Marco, Luca,
>>
>> On Tue, Mar 03, 2026 at 11:34:27AM +0100, Marco Felsch wrote:
>>
>> [...]
>>
>>> + next_bridge = devm_drm_of_get_bridge(dev, dev->of_node, 1, 0);
>>> + if (IS_ERR(next_bridge))
>>> + return dev_err_probe(dev, PTR_ERR(next_bridge),
>>> + "failed to get next bridge\n");
>>> + pdfc->dev = dev;
>>> + pdfc->bridge.of_node = dev->of_node;
>>> + pdfc->bridge.type = DRM_MODE_CONNECTOR_DPI;
>>> + pdfc->bridge.next_bridge = next_bridge;
>>
>> When I was reviewing another patch[1], I was aware of the necessity of
>> calling drm_bridge_get() for next_bridge to balance the next bridge's
>> refcount put from __drm_bridge_free() for this bridge. I'd be good if
>> Luca may confirm this is correct. Sorry for bringing this up late.
>
> Indeed you have a good point.
>
> After re-checking devm_drm_of_get_bridge(), as I wrote on the other thread
> you pointed to, you should call drm_bridge_get():
Thanks for the confirmation.
>
> - pdfc->bridge.next_bridge = next_bridge;
> + pdfc->bridge.next_bridge = drm_bridge_get(next_bridge);
>
> Marco, you can keep my R-by if you resend with just this change.
Marco, care to resend with just this change?
>
> Sorry about the confusion here.
>
> As mention on the other thread, devm_drm_of_get_bridge() is unable to
> support bridge hotplug. So it should be deprecated, but as of now there is
> no alternative.
>
> Luca
>
> --
> Luca Ceresoli, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com/
--
Regards,
Liu Ying
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v11 2/3] drm/bridge: imx: Add i.MX93 parallel display format configuration support
2026-03-10 11:53 ` Luca Ceresoli
2026-03-11 3:15 ` Liu Ying
@ 2026-03-11 19:45 ` Marco Felsch
2026-03-12 9:15 ` Liu Ying
1 sibling, 1 reply; 25+ messages in thread
From: Marco Felsch @ 2026-03-11 19:45 UTC (permalink / raw)
To: Luca Ceresoli
Cc: Liu Ying, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
Peng Fan, Andrzej Hajda, Neil Armstrong, Robert Foss,
Laurent Pinchart, Jonas Karlman, Jernej Skrabec,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
Simona Vetter, Frank Li, devicetree, imx, linux-arm-kernel,
linux-kernel, dri-devel
Hi Liu, Luca,
sorry for the delayed response, I was at the EW26.
On 26-03-10, Luca Ceresoli wrote:
> Hi Liu, Marco,
>
> On Tue Mar 10, 2026 at 3:57 AM CET, Liu Ying wrote:
> > Hi Marco, Luca,
> >
> > On Tue, Mar 03, 2026 at 11:34:27AM +0100, Marco Felsch wrote:
> >
> > [...]
> >
> >> + next_bridge = devm_drm_of_get_bridge(dev, dev->of_node, 1, 0);
> >> + if (IS_ERR(next_bridge))
> >> + return dev_err_probe(dev, PTR_ERR(next_bridge),
> >> + "failed to get next bridge\n");
> >> + pdfc->dev = dev;
> >> + pdfc->bridge.of_node = dev->of_node;
> >> + pdfc->bridge.type = DRM_MODE_CONNECTOR_DPI;
> >> + pdfc->bridge.next_bridge = next_bridge;
> >
> > When I was reviewing another patch[1], I was aware of the necessity of
> > calling drm_bridge_get() for next_bridge to balance the next bridge's
> > refcount put from __drm_bridge_free() for this bridge. I'd be good if
> > Luca may confirm this is correct. Sorry for bringing this up late.
>
> Indeed you have a good point.
At which stage did you faced this issue? During driver probe, because of
EPROBE_DEFER?
That's the reason for having the local next_bridge variable since I
faced with the same issue. In other words this driver is correct and
it's on purpose to not assign it directly. Albeit I could/should have
added a comment.
> After re-checking devm_drm_of_get_bridge(), as I wrote on the other thread
> you pointed to, you should call drm_bridge_get():
>
> - pdfc->bridge.next_bridge = next_bridge;
> + pdfc->bridge.next_bridge = drm_bridge_get(next_bridge);
>
> Marco, you can keep my R-by if you resend with just this change.
>
> Sorry about the confusion here.
>
> As mention on the other thread, devm_drm_of_get_bridge() is unable to
> support bridge hotplug. So it should be deprecated, but as of now there is
> no alternative.
Sorry I need a bit more context. What's the issue? How can I trigger the
issue? Why is bridge hotplug required at this stage? Why is only this
bridge affecte by the hotplug issue?
Regards,
Marco
>
> Luca
>
> --
> Luca Ceresoli, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
>
--
#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 |
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v11 2/3] drm/bridge: imx: Add i.MX93 parallel display format configuration support
2026-03-11 19:45 ` Marco Felsch
@ 2026-03-12 9:15 ` Liu Ying
2026-03-12 16:41 ` Luca Ceresoli
0 siblings, 1 reply; 25+ messages in thread
From: Liu Ying @ 2026-03-12 9:15 UTC (permalink / raw)
To: Marco Felsch, Luca Ceresoli
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Shawn Guo,
Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam, Peng Fan,
Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart,
Jonas Karlman, Jernej Skrabec, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Frank Li,
devicetree, imx, linux-arm-kernel, linux-kernel, dri-devel
On Wed, Mar 11, 2026 at 08:45:25PM +0100, Marco Felsch wrote:
> Hi Liu, Luca,
Hi,
>
> sorry for the delayed response, I was at the EW26.
Np at all, it's good to go out to meet people sometimes :)
>
> On 26-03-10, Luca Ceresoli wrote:
>> Hi Liu, Marco,
>>
>> On Tue Mar 10, 2026 at 3:57 AM CET, Liu Ying wrote:
>>> Hi Marco, Luca,
>>>
>>> On Tue, Mar 03, 2026 at 11:34:27AM +0100, Marco Felsch wrote:
>>>
>>> [...]
>>>
>>>> + next_bridge = devm_drm_of_get_bridge(dev, dev->of_node, 1, 0);
>>>> + if (IS_ERR(next_bridge))
>>>> + return dev_err_probe(dev, PTR_ERR(next_bridge),
>>>> + "failed to get next bridge\n");
>>>> + pdfc->dev = dev;
>>>> + pdfc->bridge.of_node = dev->of_node;
>>>> + pdfc->bridge.type = DRM_MODE_CONNECTOR_DPI;
>>>> + pdfc->bridge.next_bridge = next_bridge;
>>>
>>> When I was reviewing another patch[1], I was aware of the necessity of
>>> calling drm_bridge_get() for next_bridge to balance the next bridge's
>>> refcount put from __drm_bridge_free() for this bridge. I'd be good if
>>> Luca may confirm this is correct. Sorry for bringing this up late.
>>
>> Indeed you have a good point.
>
> At which stage did you faced this issue? During driver probe, because of
> EPROBE_DEFER?
I just want to balance the get/put for the next bridge's refcount by
calling drm_bridge_get() for next_bridge, as I said above. There could
be some way to trigger some particular Use-After-Free issues if you don't
do that. I did take some time today to try to trigger UAF, but no luck,
though I found a potential bug in encoder_bridges_show() and generated a
fix[1](Cc'ed Marco) when I played with debugfs to read the refcounts.
My idea to trigger UAF is to remove imx_lcdif/imx93_pdfc/simple_panel
modules when doing pageflips. Dunno if EPROBE_DEFER may trigger UAF.
[1] https://lore.kernel.org/all/20260312-drm-misc-next-2026-03-05-fix-encoder-bridges-refcount-v1-1-b9ba3d844732@nxp.com/
>
> That's the reason for having the local next_bridge variable since I
> faced with the same issue. In other words this driver is correct and
> it's on purpose to not assign it directly. Albeit I could/should have
> added a comment.
What's the issue your faced? How would using next_bridge variable impact?
Without knowing these info, I presume that you still need to call
drm_bridge_get() for next_bridge, since it's kind of obvious if you take
a look at __drm_bridge_free().
>
>> After re-checking devm_drm_of_get_bridge(), as I wrote on the other thread
>> you pointed to, you should call drm_bridge_get():
>>
>> - pdfc->bridge.next_bridge = next_bridge;
>> + pdfc->bridge.next_bridge = drm_bridge_get(next_bridge);
>>
>> Marco, you can keep my R-by if you resend with just this change.
>>
>> Sorry about the confusion here.
>>
>> As mention on the other thread, devm_drm_of_get_bridge() is unable to
>> support bridge hotplug. So it should be deprecated, but as of now there is
>> no alternative.
>
> Sorry I need a bit more context. What's the issue? How can I trigger the
> issue? Why is bridge hotplug required at this stage? Why is only this
> bridge affecte by the hotplug issue?
Maybe, Luca can comment here.
>
> Regards,
> Marco
>
>
>
>>
>> Luca
>>
>> --
>> Luca Ceresoli, Bootlin
>> Embedded Linux and Kernel engineering
>> https://bootlin.com/
>>
>
--
Regards,
Liu Ying
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v11 2/3] drm/bridge: imx: Add i.MX93 parallel display format configuration support
2026-03-12 9:15 ` Liu Ying
@ 2026-03-12 16:41 ` Luca Ceresoli
0 siblings, 0 replies; 25+ messages in thread
From: Luca Ceresoli @ 2026-03-12 16:41 UTC (permalink / raw)
To: Liu Ying, Marco Felsch
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Shawn Guo,
Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam, Peng Fan,
Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart,
Jonas Karlman, Jernej Skrabec, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Frank Li,
devicetree, imx, linux-arm-kernel, linux-kernel, dri-devel
Hello Marco, Liu,
On Thu Mar 12, 2026 at 10:15 AM CET, Liu Ying wrote:
> On Wed, Mar 11, 2026 at 08:45:25PM +0100, Marco Felsch wrote:
>> Hi Liu, Luca,
>
> Hi,
>
>>
>> sorry for the delayed response, I was at the EW26.
>
> Np at all, it's good to go out to meet people sometimes :)
>
>>
>> On 26-03-10, Luca Ceresoli wrote:
>>> Hi Liu, Marco,
>>>
>>> On Tue Mar 10, 2026 at 3:57 AM CET, Liu Ying wrote:
>>>> Hi Marco, Luca,
>>>>
>>>> On Tue, Mar 03, 2026 at 11:34:27AM +0100, Marco Felsch wrote:
>>>>
>>>> [...]
>>>>
>>>>> + next_bridge = devm_drm_of_get_bridge(dev, dev->of_node, 1, 0);
>>>>> + if (IS_ERR(next_bridge))
>>>>> + return dev_err_probe(dev, PTR_ERR(next_bridge),
>>>>> + "failed to get next bridge\n");
>>>>> + pdfc->dev = dev;
>>>>> + pdfc->bridge.of_node = dev->of_node;
>>>>> + pdfc->bridge.type = DRM_MODE_CONNECTOR_DPI;
>>>>> + pdfc->bridge.next_bridge = next_bridge;
>>>>
>>>> When I was reviewing another patch[1], I was aware of the necessity of
>>>> calling drm_bridge_get() for next_bridge to balance the next bridge's
>>>> refcount put from __drm_bridge_free() for this bridge. I'd be good if
>>>> Luca may confirm this is correct. Sorry for bringing this up late.
>>>
>>> Indeed you have a good point.
>>
>> At which stage did you faced this issue? During driver probe, because of
>> EPROBE_DEFER?
>
> I just want to balance the get/put for the next bridge's refcount by
> calling drm_bridge_get() for next_bridge, as I said above. There could
> be some way to trigger some particular Use-After-Free issues if you don't
> do that. I did take some time today to try to trigger UAF, but no luck,
> though I found a potential bug in encoder_bridges_show() and generated a
> fix[1](Cc'ed Marco) when I played with debugfs to read the refcounts.
The whole need for refcounting DRM bridges exists if such bridges can be
removed when other parts of the kernel still have a pointer (= a reference)
to them. This is not the case with current DRM because you cannot remove
some bridges without removing the whole pipeline (card). However I am
working since a long time to suppor hardware where the last part of the
display pipeline is hot-pluggable, causing DRM bridges to be added and
removed at runtime without removal of the initial part of the pipeline
(CRCT, encoder and 0 or more bridges).
See these links for more background info:
* ELCE 2025:
- https://www.youtube.com/watch?v=C8dEQ4OzMnc
- https://bootlin.com/pub/conferences/2025/elce/ceresoli-hotplug-status.pdf
* Lots of links in the slides about previous activity
* LPC 2024 has a general description fo the use case:
- https://lpc.events/event/18/contributions/1696/
> My idea to trigger UAF is to remove imx_lcdif/imx93_pdfc/simple_panel
> modules when doing pageflips. Dunno if EPROBE_DEFER may trigger UAF.
Perhaps there is some possible UAF on probe errors or deferrals, but the
typical case is when removing the final part of a pipeline, including 1+
bridges. This was discussed in detail in [0], but here's an excerpt with
the typical UAF scenario with hotplug:
1. pipeline: encoder --> bridge A --> bridge B --> bridge C
2. encoder takes a reference to bridge B
using devm_drm_of_find_bridge() or other means
3. bridge B takes a next_bridge reference to bridge C
using devm_drm_of_find_bridge()
4. encoder calls (bridge B)->foo(), which in turns references
next_bridge, e.g.:
b_foo() {
bar(b->next_bridge);
}
If bridges B and C are removed, bridge C can be freed but B is still
allocated because the encoder holds a ref. So when step 4 happens, 'b->c'
would be a use-after-free (or NULL deref if b.remove cleared it, which is
just as bad).
[0] https://lore.kernel.org/all/DEH2CVQV21Z2.25PJBAQAKFJSG@bootlin.com/
>> That's the reason for having the local next_bridge variable since I
>> faced with the same issue. In other words this driver is correct and
>> it's on purpose to not assign it directly. Albeit I could/should have
>> added a comment.
>
> What's the issue your faced? How would using next_bridge variable impact?
> Without knowing these info, I presume that you still need to call
> drm_bridge_get() for next_bridge, since it's kind of obvious if you take
> a look at __drm_bridge_free().
Exactly. The idea in a nutshell is: every pointer to a drm_bridge stored
somewhere is a reference to a bridge. So:
* When you take a reference (= you set a pointer to the address of a
bridge) you need to call drm_bridge_get() to increment the
refcount.
* When you clear a reference (set it to NULL) or the pointer becomes
unreachable (the device goes away, the struct holding it is not
reachable anymore, etc) you need to call drm_bridge_put() to decrement
the refcount.
This way the struct drm_bridge (or, typically, the private driver struct
embedding it) will be freed only when the refcount goes to 0, avoiding UAF.
I have been converting most of the accessors returning a drm_bridge pointer
in order to drm_bridge_get() the bridge before returning it. A few random
examples:
- https://lore.kernel.org/lkml/20250620-drm-bridge-alloc-getput-drm-bridge-c-v9-0-ca53372c9a84@bootlin.com/
- https://lore.kernel.org/lkml/20250709-drm-bridge-alloc-getput-drm_bridge_get_prev_bridge-v1-0-34ba6f395aaa@bootlin.com/
- https://lore.kernel.org/lkml/20260109-drm-bridge-alloc-getput-drm_of_find_bridge-2-v2-4-8bad3ef90b9f@bootlin.com/
I hope this clarifies the general refcount topic. Now to *_of_get_bridge().
>>> After re-checking devm_drm_of_get_bridge(), as I wrote on the other thread
>>> you pointed to, you should call drm_bridge_get():
>>>
>>> - pdfc->bridge.next_bridge = next_bridge;
>>> + pdfc->bridge.next_bridge = drm_bridge_get(next_bridge);
>>>
>>> Marco, you can keep my R-by if you resend with just this change.
>>>
>>> Sorry about the confusion here.
>>>
>>> As mention on the other thread, devm_drm_of_get_bridge() is unable to
>>> support bridge hotplug. So it should be deprecated, but as of now there is
>>> no alternative.
>>
>> Sorry I need a bit more context. What's the issue?
The issue with devm_drm_of_get_bridge(MYDEV, ...) (similarly for other
*_of_get_bridge() functions) is it can return one of:
A) an existing bridge, returned by drm_of_find_panel_or_bridge() in the
&bridge parameter
B) a panel_bridge it creates on the fly calling
devm_drm_panel_bridge_add(MYDEV) based on the panel returned by
drm_of_find_panel_or_bridge() in the &panel parameter
The caller of devm_drm_of_get_bridge() gets a drm_bridge pointer but
doesn't know whether case A or B happened.
This is not a problem if the entire card is torn down at once because
devm/drmm will take care or the removal. Even easier if the card is not
torn down at all.
Now imagine adding hotplug so you can keep MYDEV present but remove any
bridges after MYDEV and the panel. The devm action added by
devm_drm_of_get_bridge() won't trigger (because MYDEV is not going
away). The caller of devm_drm_of_get_bridge(), i.e. the MYDEV driver,
should remove the panel_bridge in case B but it has no way to know whether
case A or B happened.
So, this is tricky. The good news for you is you're not supposed to fix
it. The *_of_get_bridge() API is just unable to handle hotplug, that will
be fixed at some point. For now just ensure you get a reference for every
bridge pointer you store, and to put it when that reference goes away.
>> How can I trigger the
>> issue?
See the example above.
>> Why is bridge hotplug required at this stage?
Because there is hot-pluggable hardware we want to support, see the ELCE
and even better the LPC links above.
>> Why is only this
>> bridge affecte by the hotplug issue?
It's not only this driver. Hotplug can potentially be used on any hardware,
so I'm working to implement it in a general way in the DRM common code, but
all drviers need to at least do the refcounting on their side. I'm trying
to catch all patches involving bridge pointers and check whether they do
refcounting right. This is just one of many patches I have commented about
in the past months.
I hope this clarified a bit.
Best regards,
Luca
--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 25+ messages in thread