Re: [PATCH] drm/crtc_helper: Reset empty plane state in drm_helper_crtc_mode_set_base()

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Apr 14, 2016 at 11:25:00AM +0800, Ying Liu wrote:
> On Wed, Apr 13, 2016 at 6:33 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Wed, Apr 13, 2016 at 10:52:34AM +0800, Ying Liu wrote:
> >> On Wed, Apr 13, 2016 at 12:00 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> >> > On Tue, Apr 05, 2016 at 04:50:39PM +0800, Liu Ying wrote:
> >> >> Transitional drivers might access the NULL pointer plane->state in
> >> >> drm_helper_crtc_mode_set_base(), which causes NULL pointer dereference.
> >> >> So, let's reset it before handing it over to those drivers.
> >> >> commit e4f31ad2b713 ("drm: reset empty state in transitional helpers")
> >> >> did the same thing for other transitional helpers, but it seems this one
> >> >> was missed.
> >> >>
> >> >> Signed-off-by: Liu Ying <gnuiyl@gmail.com>
> >> >
> >> > This is kinda intentionally not done, assuming that drivers (when
> >> > transitioning) can't handle the full state stuff yet.
> >>
> >> It's a question whether the assumption is correct or not.
> >> IMHO, during the transition, the old/new state door has been
> >> opened to drivers.
> >>
> >> >
> >> > I'm not entirely opposed to this, but then we should do it for both plane
> >> > and crtc states, and in all cases I think.
> >>
> >> It looks this doesn't hurt.
> >>
> >> >
> >> > Completely new drivers should never use transitional helpers, but instead
> >> > just bring up using native atomic helpers directly. So what exactly do you
> >> > need this for?
> >>
> >> This is needed for my WIP imx-drm transitional plane driver.
> >> It would access the fb pointer of the old plane state(the empty plane
> >> state in question) to do atomic check.  Also, ->atomic_update() will
> >> do something like page flip if the fb pointer is not NULL(meaning
> >> that the plane was enabled before the update).  All of this aims to
> >> make the driver behave the similar way after the transition.
> >
> > If you need this in your driver, just make sure you're calling ->reset
> > hooks yourself as needed? Also this really is just for transitioning, in
> > the end no driver should permanently end up using these functions. In the
> > end transitional helpers here are meant as guidelines/crutches, every
> > driver is slightly different. Adding hacks to make things work smoothly
> > for all of them is impossible, and imo better to just move real fast to
> > proper atomic.
> 
> Granted my driver may call ->reset, it's more appropriate to do that
> in the helper.  There are several reasons for this.  First, helper callers
> do not bother to do this.  Secondly,  drm_helper_crtc_mode_set_base
> is somewhat a counterpart of drm_helper_crtc_mode_set,
> drm_plane_helper_update and drm_plane_helper_disable.  Now that
> commit e4f31ad2b713 calls ->reset in the three helpers, it looks
> there is no reason this cannot be done for their counterpart.
> Last but not least, exposing the NULL pointer plane->state to helper
> callers is not a good behavior, even worse could be a bug. So, this
> patch is not a hack but a somewhat bug fix.

Hm ok, I was blind and didn't realize that we've done this partially
already. Thanks for insisting, I applied your patch to drm-misc now.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel