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

[Ying Liu]

On Wed, Apr 13, 2016 at 6:33 PM, Daniel Vetter <daniel at 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 at 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 at 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.

Regards,
Liu Ying

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