Hi Am 30.09.22 um 12:26 schrieb Ville Syrjälä:
On Fri, Sep 30, 2022 at 12:12:09PM +0300, Ville Syrjälä wrote:On Fri, Sep 30, 2022 at 11:01:52AM +0200, Thomas Zimmermann wrote:Hi, thanks for reviewing. Am 29.09.22 um 21:03 schrieb Ville Syrjälä:On Thu, Sep 29, 2022 at 04:07:14PM +0200, Thomas Zimmermann wrote:In drm_atomic_helper_check_crtc_state(), do not add a new plane state to the global state if it does not exist already. Adding a new plane state will results in overhead for the plane during the atomic-commit step. For the test in drm_atomic_helper_check_crtc_state() to succeed, it is important that the CRTC has an enabled primary plane after the commit. This can be a newly enabled plane or an already enabled plane. So if a plane is not part of the commit, use the plane's existing state. The new helper drm_atomic_get_next_plane_state() returns the correct instance. Signed-off-by: Thomas Zimmermann <tzimmermann@xxxxxxx> Fixes: d6b9af1097fe ("drm/atomic-helper: Add helper drm_atomic_helper_check_crtc_state()") Cc: Thomas Zimmermann <tzimmermann@xxxxxxx> Cc: Jocelyn Falempe <jfalempe@xxxxxxxxxx> Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx> Cc: Maxime Ripard <mripard@xxxxxxxxxx> Cc: David Airlie <airlied@xxxxxxxx> Cc: Daniel Vetter <daniel@xxxxxxxx> Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx --- drivers/gpu/drm/drm_atomic_helper.c | 4 +--- include/drm/drm_atomic.h | 20 ++++++++++++++++++++ 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 98cc3137c062..463d4f3fa443 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -960,9 +960,7 @@ int drm_atomic_helper_check_crtc_state(struct drm_crtc_state *crtc_state,if (plane->type != DRM_PLANE_TYPE_PRIMARY)continue; - plane_state = drm_atomic_get_plane_state(state, plane); - if (IS_ERR(plane_state)) - return PTR_ERR(plane_state); + plane_state = drm_atomic_get_next_plane_state(state, plane); if (plane_state->fb && plane_state->crtc) {Hmm. Why this is even looking at these. If the plane is in the crtc's plane_mask then surely plane_state->crtc must already point to this crtc? And I don't think ->fb and ->crtc are allowed to disagree, so if one is set then surely the other one must be as well or we'd return an error at some point somewhere?Yeah, the crtc test is done for keeping consistency. Other places also sometimes validate that these fields don't disagree. I'll remove the crtc test in the next version. The fb test is the important one.What I'm asking how can you have crtc!=NULL && fb==NULL at all here? Some other plane state check function (can't remember which one specifically) should have rejected that. So either you're checking for impossible things, or there is a bug somewhere else.Oh and btw, fb != NULL doesn't guarantee the plane is actually visible (could have been fully clipped), if that is what you're trying to check here.
No, it's really just about having a primary plane enabled on the CRTC. Best regards Thomas -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Ivo Totev
Attachment:
OpenPGP_signature
Description: OpenPGP digital signature
