| 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: <03142c89-fc5a-45cf-9233-549ec1b9a469@ideasonboard.com>
Date: Tue, 8 Apr 2025 10:09:32 +0300
From: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
To: Aradhya Bhatia <aradhya.bhatia@...ux.dev>
Cc: dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
linux-phy@...ts.infradead.org, Francesco Dolcini <francesco@...cini.it>,
Devarsh Thakkar <devarsht@...com>, Jyri Sarha <jyri.sarha@....fi>,
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>,
Vinod Koul <vkoul@...nel.org>, Kishon Vijay Abraham I <kishon@...nel.org>,
Andrzej Hajda <andrzej.hajda@...el.com>,
Neil Armstrong <neil.armstrong@...aro.org>, Robert Foss <rfoss@...nel.org>,
Laurent Pinchart <Laurent.pinchart@...asonboard.com>,
Jonas Karlman <jonas@...boo.se>, Jernej Skrabec <jernej.skrabec@...il.com>
Subject: Re: [PATCH v2 12/18] drm/bridge: cdns-dsi: Drop checks that shouldn't
be in .mode_valid()
Hi,
On 08/04/2025 09:09, Tomi Valkeinen wrote:
> Hi,
>
> On 07/04/2025 20:59, Aradhya Bhatia wrote:
>> Hi Tomi,
>>
>> On 02/04/25 19:00, Tomi Valkeinen wrote:
>>> The docs say about mode_valid():
>>>
>>> "it is not allowed to look at anything else but the passed-in mode, and
>>> validate it against configuration-invariant hardware constraints"
>>>
>>> We're doing a lot more than just looking at the mode. The main issue
>>> here is that we're doing checks based on the pixel clock, before we know
>>> what the pixel clock from the crtc actually is.
>>>
>>> So, drop the checks from .mode_valid(). This also allows us to remove
>>> the 'mode_valid_check' parameter from internal functions, and the
>>> related code.
>>>
>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
>>> ---
>>> drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 44 +++++++
>>> +------------------
>>> 1 file changed, 14 insertions(+), 30 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/
>>> drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
>>> index e85c8652c96e..cf783680b1b4 100644
>>> --- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
>>> +++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
>>> @@ -469,25 +469,17 @@ static unsigned int dpi_to_dsi_timing(unsigned
>>> int dpi_timing,
>>> static int cdns_dsi_mode2cfg(struct cdns_dsi *dsi,
>>> const struct drm_display_mode *mode,
>>> - struct cdns_dsi_cfg *dsi_cfg,
>>> - bool mode_valid_check)
>>> + struct cdns_dsi_cfg *dsi_cfg)
>>> {
>>> struct cdns_dsi_output *output = &dsi->output;
>>> u32 dpi_hsa, dpi_hbp, dpi_hfp, dpi_hact;
>>> bool sync_pulse;
>>> int bpp;
>>> - if (mode_valid_check) {
>>> - dpi_hsa = mode->hsync_end - mode->hsync_start;
>>> - dpi_hbp = mode->htotal - mode->hsync_end;
>>> - dpi_hfp = mode->hsync_start - mode->hdisplay;
>>> - dpi_hact = mode->hdisplay;
>>> - } else {
>>> - dpi_hsa = mode->crtc_hsync_end - mode->crtc_hsync_start;
>>> - dpi_hbp = mode->crtc_htotal - mode->crtc_hsync_end;
>>> - dpi_hfp = mode->crtc_hsync_start - mode->crtc_hdisplay;
>>> - dpi_hact = mode->crtc_hdisplay;
>>> - }
>>> + dpi_hsa = mode->crtc_hsync_end - mode->crtc_hsync_start;
>>> + dpi_hbp = mode->crtc_htotal - mode->crtc_hsync_end;
>>> + dpi_hfp = mode->crtc_hsync_start - mode->crtc_hdisplay;
>>> + dpi_hact = mode->crtc_hdisplay;
>>> memset(dsi_cfg, 0, sizeof(*dsi_cfg));
>>> @@ -518,8 +510,7 @@ static int cdns_dsi_mode2cfg(struct cdns_dsi *dsi,
>>> static int cdns_dsi_adjust_phy_config(struct cdns_dsi *dsi,
>>> struct cdns_dsi_cfg *dsi_cfg,
>>> struct phy_configure_opts_mipi_dphy *phy_cfg,
>>> - const struct drm_display_mode *mode,
>>> - bool mode_valid_check)
>>> + const struct drm_display_mode *mode)
>>> {
>>> struct cdns_dsi_output *output = &dsi->output;
>>> unsigned long long dlane_bps;
>>> @@ -549,11 +540,11 @@ static int cdns_dsi_adjust_phy_config(struct
>>> cdns_dsi *dsi,
>>> if (dsi_htotal % lanes)
>>> adj_dsi_htotal += lanes - (dsi_htotal % lanes);
>>> - dpi_hz = (mode_valid_check ? mode->clock : mode->crtc_clock) *
>>> 1000;
>>> + dpi_hz = mode->crtc_clock * 1000;
>>> dlane_bps = (unsigned long long)dpi_hz * adj_dsi_htotal;
>>> /* data rate in bytes/sec is not an integer, refuse the mode. */
>>> - dpi_htotal = mode_valid_check ? mode->htotal : mode->crtc_htotal;
>>> + dpi_htotal = mode->crtc_htotal;
>>> if (do_div(dlane_bps, lanes * dpi_htotal))
>>> return -EINVAL;
>>> @@ -569,27 +560,25 @@ static int cdns_dsi_adjust_phy_config(struct
>>> cdns_dsi *dsi,
>>> static int cdns_dsi_check_conf(struct cdns_dsi *dsi,
>>> const struct drm_display_mode *mode,
>>> - struct cdns_dsi_cfg *dsi_cfg,
>>> - bool mode_valid_check)
>>> + struct cdns_dsi_cfg *dsi_cfg)
>>> {
>>> struct cdns_dsi_output *output = &dsi->output;
>>> struct phy_configure_opts_mipi_dphy *phy_cfg = &output-
>>> >phy_opts.mipi_dphy;
>>> unsigned int nlanes = output->dev->lanes;
>>> - int mode_clock = (mode_valid_check ? mode->clock : mode-
>>> >crtc_clock);
>>> unsigned long req_hs_clk_rate;
>>> int ret;
>>> - ret = cdns_dsi_mode2cfg(dsi, mode, dsi_cfg, mode_valid_check);
>>> + ret = cdns_dsi_mode2cfg(dsi, mode, dsi_cfg);
>>> if (ret)
>>> return ret;
>>> - ret = phy_mipi_dphy_get_default_config(mode_clock * 1000,
>>> + ret = phy_mipi_dphy_get_default_config(mode->crtc_clock * 1000,
>>> mipi_dsi_pixel_format_to_bpp(output-
>>> >dev->format),
>>> nlanes, phy_cfg);
>>> if (ret)
>>> return ret;
>>> - ret = cdns_dsi_adjust_phy_config(dsi, dsi_cfg, phy_cfg, mode,
>>> mode_valid_check);
>>> + ret = cdns_dsi_adjust_phy_config(dsi, dsi_cfg, phy_cfg, mode);
>>> if (ret)
>>> return ret;
>>> @@ -635,8 +624,7 @@ cdns_dsi_bridge_mode_valid(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 cdns_dsi_cfg dsi_cfg;
>>> - int bpp, ret;
>>> + int bpp;
>>> /*
>>> * VFP_DSI should be less than VFP_DPI and VFP_DSI should be at
>>> @@ -654,10 +642,6 @@ cdns_dsi_bridge_mode_valid(struct drm_bridge
>>> *bridge,
>>> if ((mode->hdisplay * bpp) % 32)
>>> return MODE_H_ILLEGAL;
>>> - ret = cdns_dsi_check_conf(dsi, mode, &dsi_cfg, true);
>>> - if (ret)
>>> - return MODE_BAD;
>>> -
>>> return MODE_OK;
>>> }
>>> @@ -996,7 +980,7 @@ static int cdns_dsi_bridge_atomic_check(struct
>>> drm_bridge *bridge,
>>> adjusted_crtc_mode->flags &= ~(DRM_MODE_FLAG_PHSYNC |
>>> DRM_MODE_FLAG_PVSYNC);
>>> adjusted_crtc_mode->flags |= DRM_MODE_FLAG_NHSYNC |
>>> DRM_MODE_FLAG_NVSYNC;
>>> - return cdns_dsi_check_conf(dsi, mode, dsi_cfg, false);
>>> + return cdns_dsi_check_conf(dsi, mode, dsi_cfg);
>>
>> With this patch, the driver shifts to using the crtc_* values during the
>> check phase, and then, it is brought back to using non crtc_* values in
>> the next patch.
>>
>> Should this patch just drop the cdns_dsi_check_conf() check from
>> .mode_valid() instead, and let the next patch phase out the
>> mdoe_valid_check parameter as the driver simultaneously shifts to using
>> the non crtc_* values throughout?
>
> Yes.
Actually, this patch doesn't change the crtc_* vs non-crtc_* behavior.
After dropping the cdns_dsi_check_conf() call in mode_valid(), the
'mode_valid_check' is always false. So this patch removes the parameter,
and any code paths for the true-case.
Should the atomic_check() have been using 'true' for the
'mode_valid_check'? The atomic_check code was added in
a53d987756eab40678f241d7cd0eb7e1ca42bba7.
Tomi
Powered by blists - more mailing lists