| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <5e5cd09b-a292-4af7-a124-4eced94e1bdf@ideasonboard.com>
Date: Tue, 14 Jan 2025 14:47:33 +0200
From: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
To: Aradhya Bhatia <aradhya.bhatia@...ux.dev>
Cc: Nishanth Menon <nm@...com>, Vignesh Raghavendra <vigneshr@...com>,
Devarsh Thakkar <devarsht@...com>, Praneeth Bajjuri <praneeth@...com>,
Udit Kumar <u-kumar1@...com>, Jayesh Choudhary <j-choudhary@...com>,
DRI Development List <dri-devel@...ts.freedesktop.org>,
Linux Kernel List <linux-kernel@...r.kernel.org>,
Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
Laurent Pinchart <laurent.pinchart@...asonboard.com>,
Andrzej Hajda <andrzej.hajda@...el.com>,
Neil Armstrong <neil.armstrong@...aro.org>, Robert Foss <rfoss@...nel.org>,
Jonas Karlman <jonas@...boo.se>, Jernej Skrabec <jernej.skrabec@...il.com>,
Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>,
David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>
Subject: Re: [PATCH v7 10/12] drm/bridge: cdns-dsi: Move DSI mode check to
_atomic_check()
Hi,
On 14/01/2025 07:56, Aradhya Bhatia wrote:
> From: Aradhya Bhatia <a-bhatia1@...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@...com>
> Signed-off-by: Aradhya Bhatia <aradhya.bhatia@...ux.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@...asonboard.com>
Tomi
Powered by blists - more mailing lists