Hi
Am 07.01.26 um 16:02 schrieb Ruben Wauters:
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.
No worries, DRM semantics can be murky. This is one of the cases that is
impossible to know unless you came across a patch like this one.
Best regards
Thomas
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
--
--
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)
