On 08/06/2014 03:57 PM, Daniel Vetter wrote:
> On Wed, Aug 06, 2014 at 01:51:41PM +0300, Ville Syrj?l? wrote:
>> On Wed, Aug 06, 2014 at 03:22:46AM +0200, Mario Kleiner wrote:
>>> Calling vblank_disable_fn() will cause that function to no-op
>>> if !dev->vblank_disable_allowed for some kms drivers, e.g.,
>>> on nouveau-kms. This can cause the gpu vblank irq's to not get
>>> disabled before freeing the dev->vblank array, so if a
>>> vblank irq fires and calls into drm_handle_vblank() after
>>> drm_vblank_cleanup() completes, it will cause use-after-free
>>> access to dev->vblank array.
>>>
>>> Call vblank_disable_and_save unconditionally, so vblank irqs
>>> are guaranteed to be off, before we delete the data structures
>>> on which they operate.
>>>
>>> Signed-off-by: Mario Kleiner <mario.kleiner.de at gmail.com>
>>> Cc: stable at vger.kernel.org
> Imo cc: stable isn't justified for these patches which fix stuff that
> normal users don't really see (driver load failure and module reload for
> kms drivers never tends to happen for normal users).
>
> So I've dropped that and pulled the 2 patches Ville reviewd into my
> topic/vblank-rework branch for 3.18.
>
> Thanks, Daniel
Ok, good with me, thanks. Ville, thanks for the review. I'll review and
test your vblank series next week when i have access to suitable
machines and enough time. I need to go through this in single-step mode,
vblank on/off changes always make me nervous, given how dependent my
main application is on this for its timing, so i want to move through it
in slow motion.
Btw. wrt to nouveau "No idea what games nouveau is playign with that
flag, but this patch should be fine at least for drivers that don't do
such things." (Villes comment).
Nouveau currently doesn't support hw vblank counter queries at all. The
dev->driver->get_vblank_count() is just hooked up to drm_vblank_count(),
so it's a no-op. Therefore nouveau can't allow disabling of vblank irq
during "normal" operation as it would lose all vblank counts during the
off period. That's why it leaves dev->vblank_disable_allowed = false;
Pre NV-50 apparently doesn't have any hw vblank counter register, but
NV50+ seems to have one. I'll probably give implementing this a try for
3.18 if nobody else does.
I'm not sure about all the new embedded drivers, if they have hw vblank
counters?
thanks,
-mario
>
>> No idea what games nouveau is playign with that flag, but this patch
>> should be fine at least for drivers that don't do such things.
>>
>> Reviewed-by: Ville Syrj?l? <ville.syrjala at linux.intel.com>
>>
>>> ---
>>> drivers/gpu/drm/drm_irq.c | 5 ++++-
>>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
>>> index 89e91e3..22e2bba9 100644
>>> --- a/drivers/gpu/drm/drm_irq.c
>>> +++ b/drivers/gpu/drm/drm_irq.c
>>> @@ -164,6 +164,7 @@ static void vblank_disable_fn(unsigned long arg)
>>> void drm_vblank_cleanup(struct drm_device *dev)
>>> {
>>> int crtc;
>>> + unsigned long irqflags;
>>>
>>> /* Bail if the driver didn't call drm_vblank_init() */
>>> if (dev->num_crtcs == 0)
>>> @@ -171,7 +172,9 @@ void drm_vblank_cleanup(struct drm_device *dev)
>>>
>>> for (crtc = 0; crtc < dev->num_crtcs; crtc++) {
>>> del_timer_sync(&dev->vblank[crtc].disable_timer);
>>> - vblank_disable_fn((unsigned long)&dev->vblank[crtc]);
>>> + spin_lock_irqsave(&dev->vbl_lock, irqflags);
>>> + vblank_disable_and_save(dev, crtc);
>>> + spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
>>> }
>>>
>>> kfree(dev->vblank);
>>> --
>>> 1.9.1
>>>
>>> _______________________________________________
>>> dri-devel mailing list
>>> dri-devel at lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>> --
>> Ville Syrj?l?
>> Intel OTC
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
