Re: [PATCH 06/19] drm/vmwgfx: Drop the cursor locking hack

From: Thomas Hellstrom <thellstrom@vmware.com>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: Daniel Vetter <daniel.vetter@intel.com>,
	Daniel Vetter <daniel.vetter@ffwll.ch>,
	Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
	DRI Development <dri-devel@lists.freedesktop.org>
Subject: Re: [PATCH 06/19] drm/vmwgfx: Drop the cursor locking hack
Date: Thu, 23 Mar 2017 09:35:25 +0100	[thread overview]
Message-ID: <0d331d58-0c57-1182-93ba-d41b8f5cabdd@vmware.com> (raw)
In-Reply-To: <20170323073145.bvua64dymjas7isv@phenom.ffwll.local>

Hi, Daniel,

On 03/23/2017 08:31 AM, Daniel Vetter wrote:
> On Thu, Mar 23, 2017 at 08:28:32AM +0100, Daniel Vetter wrote:
>> On Thu, Mar 23, 2017 at 07:22:31AM +0100, Thomas Hellstrom wrote:
>>> On 03/22/2017 10:50 PM, Daniel Vetter wrote:
>>>> It's been around forever, no one bothered to address the FIXME, so I
>>>> presume it's all fine.
>>>>
>>>> Cc: Sinclair Yeh <syeh@vmware.com>
>>>> Cc: Thomas Hellstrom <thellstrom@vmware.com>
>>>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
>>> NAK. We need to properly address this. Probably as part of the atomic
>>> update.
>> So could someone with vmwgfx understanding explain this? Note that the
>> FIXME was originally added by me years ago, because I wasn't sure (only
>> about 90%) that this is safe, and was essentially pleading for a vmwgfx
>> expert to review this?
>>
>> Since it didn't happen I presume it's not that terribly and probably safe
>> ...
>>
>> I'm still 90% sure that this is correct, but I'd love for a vmwgfx to
>> audit it. Replying with a NAK is kinda not the response I was hoping for
>> (and yes I guess I should have explained what's going on here better, but
>> it's just a git blame of the FIXME comment away).

So the code has been left in place because it works. Altering it now
will create unnecessary merge conflicts with the atomic code, and the
change isn't tested and audited which means we need to drop focus from
what we're doing and audit and test code that isn't going to be used
anyway for not apparent reason? But otoh put in the below context there
indeed is a reason.

From a quick audit of the existing code it seems like at least
vmw_cursor_update_position is touching global device state so I think at
a minimum we need to take a spinlock in that function. Otherwise it
seems to be safe.

But I prefer if we can do that as part of the atomic update?

Thanks,
Thomas

> Bit more context even: This lock dropping dance is _not_ safe from a drm
> core perspective. But when I've done the original kms locking rework the
> tradeoff between upsetting core state a bit and totally breaking vmwgfx
> leaned towards not breaking vmwgfx. And iirc you or Syeh promised to look
> at this and then either remove the FIXME, maybe with a vmwgfx lock/unlock
> added if there's a gap (I looked, didn't find one, but I don't understand
> vmwgfx in details really).
>
> Thanks, Daniel

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel