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@xxxxxxxxxx> > > > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx> > > > > > > --- > > > > > > drivers/gpu/drm/msm/hdmi/hdmi_bridge.c | 33 ++++++++++++++++++++++++++------- > > > > > > 1 file changed, 26 insertions(+), 7 deletions(-) > > > > > > > > > > -- With best wishes Dmitry
