linux-kernel - Re: [PATCH 1/2] drm/tidss: Restructure dispc_vp_prepare() and dispc_vp

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <f4b35661-24c8-4d1e-8267-6b39c0521a4a@ti.com>
Date: Wed, 22 Oct 2025 18:41:59 +0530
From: Devarsh Thakkar <devarsht@...com>
To: Tomi Valkeinen <tomi.valkeinen@...asonboard.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>
CC: Pekka Paalanen <pekka.paalanen@...labora.com>,
        <dri-devel@...ts.freedesktop.org>, <linux-kernel@...r.kernel.org>,
        Laurent
 Pinchart <laurent.pinchart@...asonboard.com>
Subject: Re: [PATCH 1/2] drm/tidss: Restructure dispc_vp_prepare() and
 dispc_vp_enable()

On 05/09/25 19:28, Tomi Valkeinen wrote:
> tidss_crtc.c calls dispc_vp_prepare() and dispc_vp_enable() in that
> order, next to each other. dispc_vp_prepare() does preparations for
> enabling the crtc, by writing some registers, and dispc_vp_enable() does
> more preparations. As the last thing, dispc_vp_enable() enables the CRTC
> by writing the enable bit.
> 
> There might have been a reason at some point in the history for this
> split, but I can't find any point to it. They also do a bit of
> overlapping work: both call dispc_vp_find_bus_fmt(). They could as well
> be a single function.
> 
> But instead of combining them, this patch moves everything from
> dispc_vp_enable() to dispc_vp_prepare(), except the actual CRTC enable
> bit write. The reason for this is that unlike all the preparatory
> register writes, CRTC enable has an immediate effect, starting the
> timing generator and the CRTC as a whole. Thus it may be important to
> time the enable just right (as we do in the next patch).
> 
> No functional changes.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>

Reviewed-by: Devarsh Thakkar <devarsht@...com>

Regards
Devarsh

> ---
>   drivers/gpu/drm/tidss/tidss_crtc.c  |  2 +-
>   drivers/gpu/drm/tidss/tidss_dispc.c | 22 ++++++----------------
>   drivers/gpu/drm/tidss/tidss_dispc.h |  3 +--
>   3 files changed, 8 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tidss/tidss_crtc.c b/drivers/gpu/drm/tidss/tidss_crtc.c
> index da89fd01c337..1b767af8e1f6 100644
> --- a/drivers/gpu/drm/tidss/tidss_crtc.c
> +++ b/drivers/gpu/drm/tidss/tidss_crtc.c
> @@ -244,7 +244,7 @@ static void tidss_crtc_atomic_enable(struct drm_crtc *crtc,
>   
>   	dispc_vp_prepare(tidss->dispc, tcrtc->hw_videoport, crtc->state);
>   
> -	dispc_vp_enable(tidss->dispc, tcrtc->hw_videoport, crtc->state);
> +	dispc_vp_enable(tidss->dispc, tcrtc->hw_videoport);
>   
>   	spin_lock_irqsave(&ddev->event_lock, flags);
>   
> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
> index 7c8c15a5c39b..d4762410d262 100644
> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
> @@ -1161,6 +1161,9 @@ void dispc_vp_prepare(struct dispc_device *dispc, u32 hw_videoport,
>   {
>   	const struct tidss_crtc_state *tstate = to_tidss_crtc_state(state);
>   	const struct dispc_bus_format *fmt;
> +	const struct drm_display_mode *mode = &state->adjusted_mode;
> +	bool align, onoff, rf, ieo, ipc, ihs, ivs;
> +	u32 hsw, hfp, hbp, vsw, vfp, vbp;
>   
>   	fmt = dispc_vp_find_bus_fmt(dispc, hw_videoport, tstate->bus_format,
>   				    tstate->bus_flags);
> @@ -1173,22 +1176,6 @@ void dispc_vp_prepare(struct dispc_device *dispc, u32 hw_videoport,
>   
>   		dispc_enable_am65x_oldi(dispc, hw_videoport, fmt);
>   	}
> -}
> -
> -void dispc_vp_enable(struct dispc_device *dispc, u32 hw_videoport,
> -		     const struct drm_crtc_state *state)
> -{
> -	const struct drm_display_mode *mode = &state->adjusted_mode;
> -	const struct tidss_crtc_state *tstate = to_tidss_crtc_state(state);
> -	bool align, onoff, rf, ieo, ipc, ihs, ivs;
> -	const struct dispc_bus_format *fmt;
> -	u32 hsw, hfp, hbp, vsw, vfp, vbp;
> -
> -	fmt = dispc_vp_find_bus_fmt(dispc, hw_videoport, tstate->bus_format,
> -				    tstate->bus_flags);
> -
> -	if (WARN_ON(!fmt))
> -		return;
>   
>   	dispc_set_num_datalines(dispc, hw_videoport, fmt->data_width);
>   
> @@ -1244,7 +1231,10 @@ void dispc_vp_enable(struct dispc_device *dispc, u32 hw_videoport,
>   				  mode->crtc_hdisplay - 1) |
>   		       FIELD_PREP(DISPC_VP_SIZE_SCREEN_VDISPLAY_MASK,
>   				  mode->crtc_vdisplay - 1));
> +}
>   
> +void dispc_vp_enable(struct dispc_device *dispc, u32 hw_videoport)
> +{
>   	VP_REG_FLD_MOD(dispc, hw_videoport, DISPC_VP_CONTROL, 1,
>   		       DISPC_VP_CONTROL_ENABLE_MASK);
>   }
> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.h b/drivers/gpu/drm/tidss/tidss_dispc.h
> index 60c1b400eb89..f38493a70122 100644
> --- a/drivers/gpu/drm/tidss/tidss_dispc.h
> +++ b/drivers/gpu/drm/tidss/tidss_dispc.h
> @@ -119,8 +119,7 @@ void dispc_ovr_enable_layer(struct dispc_device *dispc,
>   
>   void dispc_vp_prepare(struct dispc_device *dispc, u32 hw_videoport,
>   		      const struct drm_crtc_state *state);
> -void dispc_vp_enable(struct dispc_device *dispc, u32 hw_videoport,
> -		     const struct drm_crtc_state *state);
> +void dispc_vp_enable(struct dispc_device *dispc, u32 hw_videoport);
>   void dispc_vp_disable(struct dispc_device *dispc, u32 hw_videoport);
>   void dispc_vp_unprepare(struct dispc_device *dispc, u32 hw_videoport);
>   bool dispc_vp_go_busy(struct dispc_device *dispc, u32 hw_videoport);
>