From: Daniel Vetter <daniel@xxxxxxxx> Date: 2020-11-02 18:17:24 To: Bernard Zhao <bernard@xxxxxxxx> Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>,Maxime Ripard <mripard@xxxxxxxxxx>,Thomas Zimmermann <tzimmermann@xxxxxxx>,David Airlie <airlied@xxxxxxxx>,Daniel Vetter <daniel@xxxxxxxx>,dri-devel@xxxxxxxxxxxxxxxxxxxxx,linux-kernel@xxxxxxxxxxxxxxx,opensource.kernel@xxxxxxxx Subject: Re: [PATCH] gpu/drm: make crtc check before new_connector circle>On Sun, Nov 01, 2020 at 06:58:51PM -0800, Bernard Zhao wrote: >> In function prepare_signaling, crtc check (c==0) is not related >> with the next new_connector circle, maybe we can put the crtc >> check just after the crtc circle and before new_connector circle. >> This change is to make the code to run a bit first. > >I'm a bit confused here with your explanation, I'm not understanding why >you do this change ... Can you pls elaborate? Maybe give an example or >something of the problem this patch solves, that often helps. > >Thanks, Daniel Hi: This change is to make the function return error earlier when run into the error branch: if (c == 0 && (arg->flags & DRM_MODE_PAGE_FLIP_EVENT)) return -EINVAL; There two main FOR circles in this function, and the second FOR circle never changes the if condition("(c == 0 && (arg->flags & DRM_MODE_PAGE_FLIP_EVENT))") variable`s value, like c & arg->flags. So I think maybe we can check this condition before the second for circle, and return the error earlier when run into this error branch. BR//Bernard >> >> Signed-off-by: Bernard Zhao <bernard@xxxxxxxx> >> --- >> drivers/gpu/drm/drm_atomic_uapi.c | 13 ++++++------- >> 1 file changed, 6 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c >> index 25c269bc4681..566110996474 100644 >> --- a/drivers/gpu/drm/drm_atomic_uapi.c >> +++ b/drivers/gpu/drm/drm_atomic_uapi.c >> @@ -1182,6 +1182,12 @@ static int prepare_signaling(struct drm_device *dev, >> >> c++; >> } >> + /* >> + * Having this flag means user mode pends on event which will never >> + * reach due to lack of at least one CRTC for signaling >> + */ >> + if (c == 0 && (arg->flags & DRM_MODE_PAGE_FLIP_EVENT)) >> + return -EINVAL; >> >> for_each_new_connector_in_state(state, conn, conn_state, i) { >> struct drm_writeback_connector *wb_conn; >> @@ -1220,13 +1226,6 @@ static int prepare_signaling(struct drm_device *dev, >> conn_state->writeback_job->out_fence = fence; >> } >> >> - /* >> - * Having this flag means user mode pends on event which will never >> - * reach due to lack of at least one CRTC for signaling >> - */ >> - if (c == 0 && (arg->flags & DRM_MODE_PAGE_FLIP_EVENT)) >> - return -EINVAL; >> - >> return 0; >> } >> >> -- >> 2.29.0 >> > >-- >Daniel Vetter >Software Engineer, Intel Corporation >http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel