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

[Ying Liu]

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.

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