Hi Ruben
Am 03.01.26 um 20:18 schrieb Ruben Wauters:
Hi
On Sun, 2026-01-04 at 01:47 +0800, Shenghao Yang wrote:
Hi Ruben,
On 4/1/26 01:23, Ruben Wauters wrote:
With the elimination of these two WARN_ON_ONCEs, it's possible that
crtc_state may not be assigned below, and therefore may be read/passed
to functions when it is NULL (e.g. line 488). Either protection for a
null crtc_state should be added to the rest of the function, or the
function shouldn't continue if crtc is NULL.
Ruben
- crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
-
- mode = &crtc_state->mode;
+ if (crtc)
+ crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
ret = drm_atomic_helper_check_plane_state(new_plane_state, crtc_state,
DRM_PLANE_NO_SCALING,
@@ -492,6 +485,9 @@ int gud_plane_atomic_check(struct drm_plane *plane,
if (old_plane_state->rotation != new_plane_state->rotation)
crtc_state->mode_changed = true;
+ mode = &crtc_state->mode;
+ format = fb->format;
Yup - in this case I'm relying on drm_atomic_helper_check_plane_state()
bailing out early after seeing that fb is NULL (since a NULL crtc should
imply no fb) and setting plane_state->visible to false.
This is correct behavior.
That would cause an early return in gud_plane_atomic_check() without
dereferencing crtc_state.
This does work, however this ends up returning 0, which implies that
the atomic check succeded. In my opinion in this case, -EINVAL should
be returned, as both the crtc and fb don't exist, therefore the check
should not succeed. I would personally prefer a more explicit check
that does return -EINVAL instead of 0 from
drm_atomic_helper_check_planes()
If the plane has been disbabled, fb and crtc are NULL. So this is
correct. drm_atomic_helper_check_plane_state() is the place to do this
testing before doing much else in the atomic_check handler. If
drm_atomic_helper_check_plane_state() gives an error, the error should
be returns. But if it returns 0 and sets ->visible to false, the
atomic_check should return 0. That means that the plane can
successfully be disabled.
As a side note, I'm not sure if there's a reasoning as to why
drm_atomic_helper_check_planes() returns 0 if fb is NULL instead of
-EINVAL, I'm assuming it's not designed to come across this specific
case. Either way it's not too much of an issue but maybe one of the drm
maintainers can clarify why it's this way.
Disabling a plane is not an error, but a common operation.
I think the patch is fine and IIRC we have similar logic in other drivers.
Best regards
Thomas
Ruben
Would a more explicit check be preferred?
Thanks,
Shenghao
--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, Werner Knoblich, (HRB 36809, AG Nürnberg)
