Re: [PATCH v2 5/8] drm/gm12u320: replace struct drm_simple_display_pipe with regular atomic helpers

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