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. > > 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() 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. Ruben > > Would a more explicit check be preferred? > > Thanks, > > Shenghao
signature.asc
Description: This is a digitally signed message part
