Hi, First of all, thanks for the feedback.
I will fix all the problems pointed out in the review. I just have two inline questions. On 03/12, Daniel Vetter wrote: > On Mon, Mar 11, 2019 at 06:01:20PM -0300, Rodrigo Siqueira wrote: > > The function disable_outputs() and > > drm_atomic_helper_commit_modeset_enables() tries to retrieve > > helper_private from the target CRTC, for dereferencing some operations. > > However, the current implementation does not check whether > > helper_private is null and, if not, if it has a valid pointer to a dpms > > and commit functions. This commit adds pointer validations before > > trying to dereference the dpms and commit function. > > > > Signed-off-by: Rodrigo Siqueira <rodrigosiqueiram...@gmail.com> > > Please also adjust the kerneldoc for these functions. And I think the > patch subject can be improved, e.g. "Make ->atomic_enable/disable crtc > callbacks optional". Describe what you're trying to achieve in the > summary, not how you achieve it. Do I need to add information which says that both functions are optional? I'm asking because the description related to the affected functions and struct looks good for me [1,2,3]. 1. Documentation for drm_crtc_helper_funcs: https://dri.freedesktop.org/docs/drm/gpu/drm-kms-helpers.html?highlight=drm_crtc_helper_funcs#c.drm_crtc_helper_funcs 2. Documentation for drm_atomic_helper_commit_modeset_enables(): https://dri.freedesktop.org/docs/drm/gpu/drm-kms-helpers.html?highlight=drm_atomic_helper_commit_modeset_enables#c.drm_atomic_helper_commit_modeset_enables 3. Documentation for drm_atomic_helper_commit_modeset_disables(): https://dri.freedesktop.org/docs/drm/gpu/drm-kms-helpers.html?highlight=drm_atomic_helper_commit_modeset_disables#c.drm_atomic_helper_commit_modeset_disables > > --- > > drivers/gpu/drm/drm_atomic_helper.c | 30 ++++++++++++++++------------- > > 1 file changed, 17 insertions(+), 13 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c > > b/drivers/gpu/drm/drm_atomic_helper.c > > index 540a77a2ade9..fbeef7c461fc 100644 > > --- a/drivers/gpu/drm/drm_atomic_helper.c > > +++ b/drivers/gpu/drm/drm_atomic_helper.c > > @@ -1028,14 +1028,16 @@ disable_outputs(struct drm_device *dev, struct > > drm_atomic_state *old_state) > > > > > > /* Right function depends upon target state. */ > > - if (new_crtc_state->enable && funcs->prepare) > > - funcs->prepare(crtc); > > - else if (funcs->atomic_disable) > > - funcs->atomic_disable(crtc, old_crtc_state); > > - else if (funcs->disable) > > - funcs->disable(crtc); > > - else > > - funcs->dpms(crtc, DRM_MODE_DPMS_OFF); > > + if (funcs) { > > I don't think making funcs optional is a good idea. If you have a crtc > with no functions implemented, it's not terribly useful. > > Also making functions optional just here is not going to help if we still > require it everywhere else. Should I remove the other occurrence of "if (funcs)" inside disable_outputs() and drm_atomic_helper_commit_modeset_enables()? Both functions, already had this before. Best Regards > -Daniel > > > + if (new_crtc_state->enable && funcs->prepare) > > + funcs->prepare(crtc); > > + else if (funcs->atomic_disable) > > + funcs->atomic_disable(crtc, old_crtc_state); > > + else if (funcs->disable) > > + funcs->disable(crtc); > > + else if (funcs->dpms) > > + funcs->dpms(crtc, DRM_MODE_DPMS_OFF); > > + } > > > > if (!(dev->irq_enabled && dev->num_crtcs)) > > continue; > > @@ -1277,11 +1279,13 @@ void > > drm_atomic_helper_commit_modeset_enables(struct drm_device *dev, > > if (new_crtc_state->enable) { > > DRM_DEBUG_ATOMIC("enabling [CRTC:%d:%s]\n", > > crtc->base.id, crtc->name); > > - > > - if (funcs->atomic_enable) > > - funcs->atomic_enable(crtc, old_crtc_state); > > - else > > - funcs->commit(crtc); > > + if (funcs) { > > + if (funcs->atomic_enable) > > + funcs->atomic_enable(crtc, > > + old_crtc_state); > > + else if (funcs->atomic_enable) > > + funcs->commit(crtc); > > + } > > } > > } > > > > -- > > 2.21.0 > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch -- Rodrigo Siqueira https://siqueira.tech Graduate Student Department of Computer Science University of São Paulo _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
