Messages in this thread |  | | Date | Tue, 14 Jan 2025 14:47:33 +0200 | | Subject | Re: [PATCH v7 10/12] drm/bridge: cdns-dsi: Move DSI mode check to _atomic_check() | | From | Tomi Valkeinen <> |
| |
Hi,
On 14/01/2025 07:56, Aradhya Bhatia wrote: > From: Aradhya Bhatia <a-bhatia1@ti.com> > > At present, the DSI mode configuration check happens during the > _atomic_enable() phase, which is not really the best place for this. > Moreover, if the mode is not valid, the driver gives a warning and > continues the hardware configuration. > > Move the DSI mode configuration check to _atomic_check() instead, which > can properly report back any invalid mode, before the _enable phase even > begins. > > Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com> > Signed-off-by: Aradhya Bhatia <aradhya.bhatia@linux.dev> > --- > .../gpu/drm/bridge/cadence/cdns-dsi-core.c | 87 +++++++++++++++++-- > 1 file changed, 82 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c > index b8db984ea9fa..d60254e1270c 100644 > --- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c > +++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c > @@ -425,6 +425,17 @@ > #define DSI_NULL_FRAME_OVERHEAD 6 > #define DSI_EOT_PKT_SIZE 4 > > +struct cdns_dsi_bridge_state { > + struct drm_bridge_state base; > + struct cdns_dsi_cfg dsi_cfg; > +}; > + > +static inline struct cdns_dsi_bridge_state * > +to_cdns_dsi_bridge_state(struct drm_bridge_state *bridge_state) > +{ > + return container_of(bridge_state, struct cdns_dsi_bridge_state, base); > +} > + > static inline struct cdns_dsi *input_to_dsi(struct cdns_dsi_input *input) > { > return container_of(input, struct cdns_dsi, input); > @@ -768,6 +779,9 @@ static void cdns_dsi_bridge_atomic_enable(struct drm_bridge *bridge, > struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge); > struct cdns_dsi *dsi = input_to_dsi(input); > struct cdns_dsi_output *output = &dsi->output; > + struct drm_atomic_state *state = old_bridge_state->base.state; > + struct cdns_dsi_bridge_state *dsi_state; > + struct drm_bridge_state *new_bridge_state; > struct drm_display_mode *mode; > struct phy_configure_opts_mipi_dphy *phy_cfg = &output->phy_opts.mipi_dphy; > unsigned long tx_byte_period; > @@ -778,14 +792,19 @@ static void cdns_dsi_bridge_atomic_enable(struct drm_bridge *bridge, > if (WARN_ON(pm_runtime_get_sync(dsi->base.dev) < 0)) > return; > > + new_bridge_state = drm_atomic_get_new_bridge_state(state, bridge); > + if (WARN_ON(!new_bridge_state)) > + return; > + > + dsi_state = to_cdns_dsi_bridge_state(new_bridge_state); > + dsi_cfg = dsi_state->dsi_cfg; > + > if (dsi->platform_ops && dsi->platform_ops->enable) > dsi->platform_ops->enable(dsi); > > mode = &bridge->encoder->crtc->state->adjusted_mode; > nlanes = output->dev->lanes; > > - WARN_ON_ONCE(cdns_dsi_check_conf(dsi, mode, &dsi_cfg, false)); > - > cdns_dsi_init_link(dsi); > cdns_dsi_hs_init(dsi); > > @@ -956,6 +975,63 @@ static u32 *cdns_dsi_bridge_get_input_bus_fmts(struct drm_bridge *bridge, > return input_fmts; > } > > +static int cdns_dsi_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 cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge); > + struct cdns_dsi *dsi = input_to_dsi(input); > + struct cdns_dsi_bridge_state *dsi_state = to_cdns_dsi_bridge_state(bridge_state); > + struct drm_display_mode *mode = &crtc_state->mode;
const
> + struct cdns_dsi_cfg *dsi_cfg = &dsi_state->dsi_cfg; > + > + return cdns_dsi_check_conf(dsi, mode, dsi_cfg, false); > +} > + > +static struct drm_bridge_state * > +cdns_dsi_bridge_atomic_duplicate_state(struct drm_bridge *bridge) > +{ > + struct cdns_dsi_bridge_state *dsi_state; > + > + if (WARN_ON(!bridge->base.state)) > + return NULL; > + > + dsi_state = kzalloc(sizeof(*dsi_state), GFP_KERNEL); > + if (!dsi_state) > + return NULL; > + > + __drm_atomic_helper_bridge_duplicate_state(bridge, &dsi_state->base);
Hmm, if I'm not mistaken, you should copy the private state here (cdns_dsi_cfg).
> + return &dsi_state->base; > +} > + > +static void > +cdns_dsi_bridge_atomic_destroy_state(struct drm_bridge *bridge, > + struct drm_bridge_state *state) > +{ > + struct cdns_dsi_bridge_state *dsi_state; > + > + dsi_state = to_cdns_dsi_bridge_state(state); > + > + kfree(dsi_state); > +} > + > +static struct drm_bridge_state * > +cdns_dsi_bridge_atomic_reset(struct drm_bridge *bridge) > +{ > + struct cdns_dsi_bridge_state *dsi_state; > + > + dsi_state = kzalloc(sizeof(*dsi_state), GFP_KERNEL); > + if (!dsi_state) > + return NULL; > + > + memset(dsi_state, 0, sizeof(*dsi_state)); > + dsi_state->base.bridge = bridge; > + > + return &dsi_state->base; > +} > + > static const struct drm_bridge_funcs cdns_dsi_bridge_funcs = { > .attach = cdns_dsi_bridge_attach, > .mode_valid = cdns_dsi_bridge_mode_valid, > @@ -963,9 +1039,10 @@ static const struct drm_bridge_funcs cdns_dsi_bridge_funcs = { > .atomic_pre_enable = cdns_dsi_bridge_atomic_pre_enable, > .atomic_enable = cdns_dsi_bridge_atomic_enable, > .atomic_post_disable = cdns_dsi_bridge_atomic_post_disable, > - .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state, > - .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state, > - .atomic_reset = drm_atomic_helper_bridge_reset, > + .atomic_check = cdns_dsi_bridge_atomic_check, > + .atomic_duplicate_state = cdns_dsi_bridge_atomic_duplicate_state, > + .atomic_destroy_state = cdns_dsi_bridge_atomic_destroy_state, > + .atomic_reset = cdns_dsi_bridge_atomic_reset, > .atomic_get_input_bus_fmts = cdns_dsi_bridge_get_input_bus_fmts, > }; >
Other than the two small comments:
Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Tomi
|  |