On Fri Jul 17, 2026 at 3:18 PM CST, Thomas Zimmermann wrote: > Hi > > Am 16.07.26 um 11:01 schrieb Ze Huang: >> Convert gm12u320 to direct primary plane, CRTC and encoder setup. >> >> Keep shadow-plane helper state, framebuffer access helpers and >> no-scaling plane-state check from simple-KMS path. >> >> Reviewed-by: Thomas Zimmermann <[email protected]> >> Tested-by: Thomas Zimmermann <[email protected]> >> Signed-off-by: Ze Huang <[email protected]> >> --- >> drivers/gpu/drm/tiny/gm12u320.c | 132 >> ++++++++++++++++++++++++++++++++-------- >> 1 file changed, 107 insertions(+), 25 deletions(-) >> > [...] >> >> -static void gm12u320_pipe_enable(struct drm_simple_display_pipe *pipe, >> - struct drm_crtc_state *crtc_state, >> - struct drm_plane_state *plane_state) >> +static void gm12u320_crtc_helper_atomic_enable(struct drm_crtc *crtc, >> + struct drm_atomic_commit *commit) >> { >> struct drm_rect rect = { 0, 0, GM12U320_USER_WIDTH, GM12U320_HEIGHT }; >> - struct gm12u320_device *gm12u320 = to_gm12u320(pipe->crtc.dev); >> + struct gm12u320_device *gm12u320 = to_gm12u320(crtc->dev); >> + struct drm_plane_state *plane_state = gm12u320->plane.state; > > Did you see the reply from the Sashiko bot? > > What happens is that user space can apply multiple atomic commits in a > row, but they are applied to hardware asynchronously. So if you take the > plane state here directly from the plane, it could have been replaced by > a later atomic commit already. Rather get the correct plane state with > the helper drm_atomic_get_new_plane_state(). >
I think I misunderstood this point. I had assumed that drm_atomic_get_*_state() was only needed for the state object directly matching the helper callback, for example crtc_state in *_crtc_helper_atomic_enable() and plane_state in *_plane_helper_atomic_update(), while other state could be accessed via the device-private structure. >From your explanation, I see that this is wrong. In the atomic commit path, any state associated with the current commit should be obtained through drm_atomic_get_*_state(), because the object’s ->state may already refer to a later commit. > I did not look at all of the series' patches for this problem, but the rule > applies to all of the mode-setting code. > > Best regards > Thomas This affects other patches in the series as well, and some drivers may need additional fixes in internal helpers too, e.g. arc_pgu_set_pxl_fmt() in arcpgu.c [1]. I will audit the full series and fix such cases in the next version. Best regards Ze
