linux-kernel - Re: [PATCH v5 5/9] drm/msm/hdmi: turn mode_set into atomic

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] [thread-next>] [day] [month] [year] [list]

Date: Fri, 21 Jun 2024 01:17:01 +0300
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Abhinav Kumar <quic_abhinavk@...cinc.com>
Cc: 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>, Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, 
	Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>, 
	David Airlie <airlied@...il.com>, Daniel Vetter <daniel@...ll.ch>, Rob Clark <robdclark@...il.com>, 
	Sean Paul <sean@...rly.run>, Marijn Suijten <marijn.suijten@...ainline.org>, 
	dri-devel@...ts.freedesktop.org, linux-arm-msm@...r.kernel.org, freedreno@...ts.freedesktop.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 5/9] drm/msm/hdmi: turn mode_set into atomic_enable

On Thu, Jun 20, 2024 at 03:05:16PM GMT, Abhinav Kumar wrote:
> 
> 
> On 6/20/2024 2:24 PM, Dmitry Baryshkov wrote:
> > On Thu, Jun 20, 2024 at 01:49:33PM GMT, Abhinav Kumar wrote:
> > > 
> > > 
> > > On 6/20/2024 1:32 PM, Dmitry Baryshkov wrote:
> > > > On Thu, Jun 20, 2024 at 01:27:15PM GMT, Abhinav Kumar wrote:
> > > > > 
> > > > > 
> > > > > On 6/7/2024 6:23 AM, Dmitry Baryshkov wrote:
> > > > > > The mode_set callback is deprecated, it doesn't get the
> > > > > > drm_bridge_state, just mode-related argumetns. Turn it into the
> > > > > > atomic_enable callback as suggested by the documentation.
> > > > > > 
> > > > > 
> > > > > mode_set is deprecated but atomic_mode_set is not.
> > > > 
> > > > There is no atomic_mode_set() in drm_bridge_funcs. Also:
> > > > 
> > > 
> > > Please excuse me. I thought since encoder has atomic_mode_set(), bridge has
> > > one too.
> > > 
> > > > * This is deprecated, do not use!
> > > > * New drivers shall set their mode in the
> > > > * &drm_bridge_funcs.atomic_enable operation.
> > > > 
> > > 
> > > Yes I saw this note but it also says "new drivers" and not really enforcing
> > > migrating existing ones which are using modeset to atomic_enable.
> > 
> > Well, deprecated functions are supposed to be migrated.
> > 
> 
> Along with rest of the pieces of the driver :)

Step by step :-)

> 
> > > My concern is that today the timing engine setup happens in encoder's
> > > enable() and the hdmi's timing is programmed in mode_set().
> > > 
> > > Ideally, we should program hdmi's timing registers first before the
> > > encoder's timing.
> > > 
> > > Although timing engine is not enabled yet, till post_kickoff, this is
> > > changing the sequence.
> > > 
> > > If this really required for rest of this series?
> > 
> > No, it was just worth doing it as I was doing conversion to atomic_*
> > anyway. I can delay this patch for now.
> > 
> > Two questions:
> > 
> > 1) Are you sure regarding the HDMI timing registers vs INTF order? I
> > observe the underrun issues while changing modes on db820c.
> > 
> 
> Yes this is the order I see in the docs:
> 
> 1) setup HDMI PHY and PLL
> 2) setup HDMI video path correctly (HDMI timing registers)
> 3) setup timing generator to match the HDMI video in (2)
> 4) Enable timing engine

Thanks!

> 
> This change is mixing up the order of (2) and (3).
> 
> > 2) What should be the order between programming of the HDMI timing
> > engine and HDMI PHY?
> > 
> 
> Mentioned above.
> 
> > What would be your opinion on moving HDMI timing programming to
> > atomic_pre_enable? (the exact place depends on the answer on the second
> > question).
> > 
> 
> So
> 
> -> bridge->pre_enable() : Do HDMI timing programming
> -> encoder->atomic_enable() : Do timing engine programming
> -> post_kickoff() does timing engine enable
> 
> This matches the steps I wrote above.
> 
> Hence this is fine from my PoV.

Ack, I'll skip this patch for now and repost it separately (moving the
code to pre_enable function).

> 
> > > 
> > > > > 
> > > > > I would rather use atomic_mode_set because moving to atomic_enable() would
> > > > > be incorrect.
> > > > > 
> > > > > That would be called after encoder's enable and hence changes the sequence.
> > > > > That was not the intention of this patch.
> > > > > 
> > > > > NAK.
> > > > > 
> > > > > > Acked-by: Maxime Ripard <mripard@...nel.org>
> > > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
> > > > > > ---
> > > > > >     drivers/gpu/drm/msm/hdmi/hdmi_bridge.c | 33 ++++++++++++++++++++++++++-------
> > > > > >     1 file changed, 26 insertions(+), 7 deletions(-)
> > > > 
> > > > 
> > 

-- 
With best wishes
Dmitry