From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH 03/17] drm: convert crtc and mode_config to ww_mutex Date: Mon, 26 May 2014 10:23:33 +0200 Message-ID: <20140526082333.GW14357@phenom.ffwll.local> References: <1400956226-28053-1-git-send-email-robdclark@gmail.com> <1400956226-28053-4-git-send-email-robdclark@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-wi0-f182.google.com (mail-wi0-f182.google.com [209.85.212.182]) by gabe.freedesktop.org (Postfix) with ESMTP id 8DB156E232 for ; Mon, 26 May 2014 01:23:39 -0700 (PDT) Received: by mail-wi0-f182.google.com with SMTP id r20so3892922wiv.3 for ; Mon, 26 May 2014 01:23:38 -0700 (PDT) Content-Disposition: inline In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Rob Clark Cc: dri-devel List-Id: dri-devel@lists.freedesktop.org On Sun, May 25, 2014 at 07:16:43PM -0400, Rob Clark wrote: > On Sun, May 25, 2014 at 6:10 PM, Daniel Vetter wrote: > > On Sat, May 24, 2014 at 8:30 PM, Rob Clark wrote: > >> @@ -8002,7 +8002,7 @@ bool intel_get_load_detect_pipe(struct drm_connector *connector, > >> if (encoder->crtc) { > >> crtc = encoder->crtc; > >> > >> - mutex_lock(&crtc->mutex); > >> + drm_modeset_lock(&crtc->mutex, NULL); > > > > > > This is pretty much the reason why I think switching the > > mode_config.mutex to a ww_mutex is a bad idea: This call here nests > > within the mode_config.mutex and so must be acquired. Wiring the > > acquire context through everything is going to be fairly horrible, > > especially since you must be able to bail out when trying to lock with > > an axquire context. > > which is the call-path to here from mode_config.mutex? Is it possible > to just move the locking to a higher level for a > drm_modeset_lock_all()? Connector probing. And the entire point of crtc locks was to _not_ block all screen updates while we poke for a new edid or do load balancing. If you want to test this you need a gen3/4 with tv-out (native, not through sdvo) or a gen2 or i915g/gm with vga. > > My original design behind the crtc->mutex and mode_config.mutex split > > was that as long as the connector->crtc links didn't change you can > > get away with the crtc lock. setplane made a bit a mess out of this, > > but strictly speaking as long as you acquire all crtc locks involved > > in a potential plane switch (which ww_mtuxes can do) it'll be fine. > > Since noticing whether any connector properties change should be > > doable upfront I think we should try _really_ hard to keep the > > mode_config.mutex a plain mutex which wraps all the more fine-grained > > locks and is a catch-all for everything else but crtcs/planes. > > That is basically how it was in one of the earlier iterations of > atomic.. but didn't hold mode_config.mutex in a lot of places where it > was previously held.. and while I do want to make locking more fine > grained I didn't want to try and do it at the same time as landing all > these other changes. Hm, maybe we should land the locking stuff first? I.e. just convert crtc->mutex into a ww_mutex, and then add more fine-grained locking to e.g. setplane by only grabbing the locks of the involved crtcs with the w/w logic. We might need an additional plane mutex to make it work. Iirc Ville had some patches for just this. I'm arguing this since locking at the current interface I have a really hard time seeing how we're going to implement this in i915. Still reading around though. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch