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