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@xxxxxxxx> 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@xxxxxxxx> 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@xxxxxxxxx> > >> > > >> > 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@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel