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

[Daniel Vetter <daniel@ffwll.ch>]

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.
-Daniel

> 
> Regards,
> Liu Ying
> 
> > -Daniel
> >
> >> ---
> >>  drivers/gpu/drm/drm_crtc_helper.c | 8 +++++---
> >>  1 file changed, 5 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
> >> index 79555d2..66ca313 100644
> >> --- a/drivers/gpu/drm/drm_crtc_helper.c
> >> +++ b/drivers/gpu/drm/drm_crtc_helper.c
> >> @@ -1053,10 +1053,12 @@ int drm_helper_crtc_mode_set_base(struct drm_crtc *crtc, int x, int y,
> >>
> >>       if (plane->funcs->atomic_duplicate_state)
> >>               plane_state = plane->funcs->atomic_duplicate_state(plane);
> >> -     else if (plane->state)
> >> +     else {
> >> +             if (!plane->state)
> >> +                     drm_atomic_helper_plane_reset(plane);
> >> +
> >>               plane_state = drm_atomic_helper_plane_duplicate_state(plane);
> >> -     else
> >> -             plane_state = kzalloc(sizeof(*plane_state), GFP_KERNEL);
> >> +     }
> >>       if (!plane_state)
> >>               return -ENOMEM;
> >>       plane_state->plane = plane;
> >> --
> >> 2.5.0
> >>
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch

-- 
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