Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH] drm/kms/plane: Avoiding redundant checks in case of
 EDEADLK retries
To:     Satendra Singh Thakur <satendra.t@samsung.com>,
        Gustavo Padovan <gustavo@padovan.org>,
        Sean Paul <seanpaul@chromium.org>,
        David Airlie <airlied@linux.ie>,
        dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org
Cc:     vineet.j@samsung.com, hemanshu.s@samsung.com,
        nishant.y08@samsung.com, sst2005@gmail.com
References: <CGME20180730055515epcas5p3358ce6d5e80b66256ea92aa5659f74bb@epcas5p3.samsung.com>
 <20180730055515epcas5p3358ce6d5e80b66256ea92aa5659f74bb~GD9F1P8zb2847928479epcas5p3D@epcas5p3.samsung.com>
From:   Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Message-ID: <9fa529a6-8ad0-9e9d-99a4-1db8284ecefd@linux.intel.com>
Date:   Mon, 30 Jul 2018 09:53:00 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.9.1
MIME-Version: 1.0
In-Reply-To: <20180730055515epcas5p3358ce6d5e80b66256ea92aa5659f74bb~GD9F1P8zb2847928479epcas5p3D@epcas5p3.samsung.com>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
Content-Language: en-US
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Op 30-07-18 om 07:18 schreef Satendra Singh Thakur:
> 1. Currently, if modeset_lock is re-tried many times in case of EDEADLK
> error, then this will be the code flow
>
> retry:
> 	ret = drm_modeset_lock();
>
> if (ret)-->this is true
> 		goto out;
>
> out:
> 	if (fb)
> 	if (plane->old_fb)
> 	if (ret == -EDEADLK)
> 		goto retry;
> It can be observed that checks on fb, old_fb are redundant
> in retry-case.
> If we keep if (ret == -EDEADLK) right after the out label,
> that will avoid redundant checks.
> It won't affect normal scenario (non-retry case).
> 2. Moved NULL assignment inside if statement related to NULL check
>
> Signed-off-by: Satendra Singh Thakur <satendra.t@samsung.com>
> ---
>  drivers/gpu/drm/drm_plane.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index 0350544..ed42cd4 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -658,9 +658,10 @@ static int __setplane_internal(struct drm_plane *plane,
>  	}
>  
>  out:
> -	if (plane->old_fb)
> +	if (plane->old_fb) {
>  		drm_framebuffer_put(plane->old_fb);
> -	plane->old_fb = NULL;
> +		plane->old_fb = NULL;
> +	}
>  
>  	return ret;
>  }
> @@ -1097,17 +1098,17 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
>  	}
>  
>  out:
> -	if (fb)
> -		drm_framebuffer_put(fb);
> -	if (plane->old_fb)
> -		drm_framebuffer_put(plane->old_fb);
> -	plane->old_fb = NULL;
> -
>  	if (ret == -EDEADLK) {
>  		ret = drm_modeset_backoff(&ctx);
>  		if (!ret)
>  			goto retry;
>  	}
> +	if (fb)
> +		drm_framebuffer_put(fb);
> +	if (plane->old_fb) {
> +		drm_framebuffer_put(plane->old_fb);
> +		plane->old_fb = NULL;
> +	}
>  
>  	drm_modeset_drop_locks(&ctx);
>  	drm_modeset_acquire_fini(&ctx);

It's not redundant. plane->old_fb is protected by the plane_mutex lock and should be
null before dropping the locks in drm_modeset_backoff(). Else other code will trip
with a WARN_ON(plane->old_fb != NULL);

Before sending any more patches I would strongly suggest setting
CONFIG_DEBUG_WW_MUTEX_SLOWPATH and CONFIG_PROVE_LOCKING in your kernel, and try to
run the IGT testsuite.

~Maarten