Hi, On Wed, 2026-01-07 at 08:46 +0100, Thomas Zimmermann wrote: > 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 I may have misunderstood the drm docs on this, if having crtc and fb be null and returning 0 then is ok, I am happy for this patch to be applied. I have tested it and it indeed works, and removes the oops present in the driver before this. > > I think the patch is fine and IIRC we have similar logic in other drivers. Reviewed-by: Ruben Wauters <[email protected]> I believe Shenghao mentioned another oops that is present? if so it may be best to submit that in a separate patch rather than a v2 of this one. Ruben > > Best regards > Thomas > > > > > Ruben > > > Would a more explicit check be preferred? > > > > > > Thanks, > > > > > > Shenghao
signature.asc
Description: This is a digitally signed message part
