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;
From:   Jani Nikula <jani.nikula@linux.intel.com>
To:     Satendra Singh Thakur <satendra.t@samsung.com>,
        Gustavo Padovan <gustavo@padovan.org>,
        Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
        Sean Paul <seanpaul@chromium.org>,
        David Airlie <airlied@linux.ie>,
        dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org
Cc:     hemanshu.s@samsung.com, vineet.j@samsung.com, sst2005@gmail.com,
        Satendra Singh Thakur <satendra.t@samsung.com>
Subject: Re: [PATCH] drm/kms/crtc: Improving the func drm_mode_setcrtc
In-Reply-To: <20180727101302epcas5p22561edd1d60101e611933ccff3e3d008~FMiUBZnCN2101321013epcas5p2Z@epcas5p2.samsung.com>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
References: <CGME20180727101302epcas5p22561edd1d60101e611933ccff3e3d008@epcas5p2.samsung.com> <20180727101302epcas5p22561edd1d60101e611933ccff3e3d008~FMiUBZnCN2101321013epcas5p2Z@epcas5p2.samsung.com>
Date:   Fri, 24 Aug 2018 16:50:35 +0300
Message-ID: <87wosflvpw.fsf@intel.com>
MIME-Version: 1.0
Content-Type: text/plain
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, 27 Jul 2018, Satendra Singh Thakur <satendra.t@samsung.com> wrote:
> Following changes are done to this func:

Just a drive-by observation, if your commit message lists a number of
changes, with "also", it's usually an indication they should be separate
patches.

BR,
Jani.


> 1. Currently there are many redundant error checks for
> count_connectors, mode, fb and mode_valid.
> if (crtc_req->mode_valid)
> if (crtc_req->count_connectors == 0 && mode)
> if (crtc_req->count_connectors > 0 && (!mode || !fb))
> if (crtc_req->count_connectors > 0)
> if (crtc_req->count_connectors > config->num_connector)
>
> These 5 checks are replaced by just 2 checks.
> if (!crtc_req->mode_valid)
> if (!crtc_req->count_connectors ||
> crtc_req->count_connectors > config->num_connector)
>
> 2. Also, currently, if user pass invalid args
> count_connectors=0, mode=NULL, fb=NULL, code wont throw
> any errors and eventually __drm_mode_set_config_internal
> will be called.
> With the modified code, this will be taken care.
>
> 3. Also, these error checks alongwith following
> if (!file_priv->aspect_ratio_allowed &&
> (crtc_req->mode.flags & DRM_MODE_FLAG_PIC_AR_MASK)
>
> has been  moved  before taking mutex and modeset lock
> because they don't need any lock. Moreover, after grabbing locks,
> if its found that user supplied  invalid args, it will
> be too late as getting lock may require considerable time.
>
> 4. Also, if modeset_lock is tried many times in case of EDEADLK
> error, then this will be the code flow
>
> retry:
> 	ret = drm_modeset_lock_all_ctx(crtc->dev, &ctx);
>
> if (ret)-->this is true
> 		goto out;
>
> out:
> 	if (fb)
> 	if (connector_set)
> 	drm_mode_destroy(dev, mode);
> 	if (ret == -EDEADLK)
> 		goto retry;
> It can be observed that checks on fb, connector_set and call to
> mode_destroy are redundant in retry-case.
> If we keep if (ret == -EDEADLK) just after out:,
> that will avoid redundant checks.
>
> In the normal case (non-retry), all checks will be required.
> Thus shifting of if (ret == -EDEADLK) right after out label
> won't affect normal case.
>
> 5. Also, kfree is moved inside if (connector_set).
> 6. Also, if major error checks are in the beginning of the func
> and if user supplied invalid params,  we will exit the func sooner
> without wasting much effort in finding crtc and framebuffer etc.
>
> Signed-off-by: Satendra Singh Thakur <satendra.t@samsung.com>
> ---
>  drivers/gpu/drm/drm_crtc.c | 207 +++++++++++++++++++++------------------------
>  1 file changed, 98 insertions(+), 109 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 98a36e6..15927e7 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -578,7 +578,25 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
>  	 */
>  	if (crtc_req->x & 0xffff0000 || crtc_req->y & 0xffff0000)
>  		return -ERANGE;
> -
> +	if (!file_priv->aspect_ratio_allowed &&
> +		(crtc_req->mode.flags & DRM_MODE_FLAG_PIC_AR_MASK) !=
> +		DRM_MODE_FLAG_PIC_AR_NONE) {
> +		DRM_DEBUG_KMS("Unexpected aspect-ratio flag bits\n");
> +		return -EINVAL;
> +	}
> +	/* Check if the flag is set*/
> +	if (!crtc_req->mode_valid) {
> +		DRM_DEBUG_KMS("mode_valid flag [%d] is not set\n",
> +		crtc_req->mode_valid);
> +		return -EINVAL;
> +	}
> +	/* Check the validity of count_connectors supplied by user */
> +	if (!crtc_req->count_connectors ||
> +		crtc_req->count_connectors > config->num_connector) {
> +		DRM_DEBUG_KMS("Invalid connectors' count %d\n",
> +		crtc_req->count_connectors);
> +		return -EINVAL;
> +	}
>  	crtc = drm_crtc_find(dev, file_priv, crtc_req->crtc_id);
>  	if (!crtc) {
>  		DRM_DEBUG_KMS("Unknown CRTC ID %d\n", crtc_req->crtc_id);
> @@ -595,134 +613,105 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
>  	if (ret)
>  		goto out;
>  
> -	if (crtc_req->mode_valid) {
> -		/* If we have a mode we need a framebuffer. */
> -		/* If we pass -1, set the mode with the currently bound fb */
> -		if (crtc_req->fb_id == -1) {
> -			struct drm_framebuffer *old_fb;
> +	/* If we have a mode we need a framebuffer. */
> +	/* If we pass -1, set the mode with the currently bound fb */
> +	if (crtc_req->fb_id == -1) {
> +		struct drm_framebuffer *old_fb;
>  
> -			if (plane->state)
> -				old_fb = plane->state->fb;
> -			else
> -				old_fb = plane->fb;
> +		if (plane->state)
> +			old_fb = plane->state->fb;
> +		else
> +			old_fb = plane->fb;
>  
> -			if (!old_fb) {
> -				DRM_DEBUG_KMS("CRTC doesn't have current FB\n");
> -				ret = -EINVAL;
> -				goto out;
> -			}
> -
> -			fb = old_fb;
> -			/* Make refcounting symmetric with the lookup path. */
> -			drm_framebuffer_get(fb);
> -		} else {
> -			fb = drm_framebuffer_lookup(dev, file_priv, crtc_req->fb_id);
> -			if (!fb) {
> -				DRM_DEBUG_KMS("Unknown FB ID%d\n",
> -						crtc_req->fb_id);
> -				ret = -ENOENT;
> -				goto out;
> -			}
> -		}
> -
> -		mode = drm_mode_create(dev);
> -		if (!mode) {
> -			ret = -ENOMEM;
> -			goto out;
> -		}
> -		if (!file_priv->aspect_ratio_allowed &&
> -		    (crtc_req->mode.flags & DRM_MODE_FLAG_PIC_AR_MASK) != DRM_MODE_FLAG_PIC_AR_NONE) {
> -			DRM_DEBUG_KMS("Unexpected aspect-ratio flag bits\n");
> +		if (!old_fb) {
> +			DRM_DEBUG_KMS("CRTC doesn't have current FB\n");
>  			ret = -EINVAL;
>  			goto out;
>  		}
>  
> -
> -		ret = drm_mode_convert_umode(dev, mode, &crtc_req->mode);
> -		if (ret) {
> -			DRM_DEBUG_KMS("Invalid mode\n");
> +		fb = old_fb;
> +		/* Make refcounting symmetric with the lookup path. */
> +		drm_framebuffer_get(fb);
> +	} else {
> +		fb = drm_framebuffer_lookup(dev, file_priv, crtc_req->fb_id);
> +		if (!fb) {
> +			DRM_DEBUG_KMS("Unknown FB ID%d\n",
> +					crtc_req->fb_id);
> +			ret = -ENOENT;
>  			goto out;
>  		}
> -
> -		/*
> -		 * Check whether the primary plane supports the fb pixel format.
> -		 * Drivers not implementing the universal planes API use a
> -		 * default formats list provided by the DRM core which doesn't
> -		 * match real hardware capabilities. Skip the check in that
> -		 * case.
> -		 */
> -		if (!plane->format_default) {
> -			ret = drm_plane_check_pixel_format(plane,
> -							   fb->format->format,
> -							   fb->modifier);
> -			if (ret) {
> -				struct drm_format_name_buf format_name;
> -				DRM_DEBUG_KMS("Invalid pixel format %s, modifier 0x%llx\n",
> -					      drm_get_format_name(fb->format->format,
> -								  &format_name),
> -					      fb->modifier);
> -				goto out;
> -			}
> -		}
> -
> -		ret = drm_crtc_check_viewport(crtc, crtc_req->x, crtc_req->y,
> -					      mode, fb);
> -		if (ret)
> -			goto out;
> -
>  	}
>  
> -	if (crtc_req->count_connectors == 0 && mode) {
> -		DRM_DEBUG_KMS("Count connectors is 0 but mode set\n");
> -		ret = -EINVAL;
> +	mode = drm_mode_create(dev);
> +	if (!mode) {
> +		ret = -ENOMEM;
>  		goto out;
>  	}
>  
> -	if (crtc_req->count_connectors > 0 && (!mode || !fb)) {
> -		DRM_DEBUG_KMS("Count connectors is %d but no mode or fb set\n",
> -			  crtc_req->count_connectors);
> -		ret = -EINVAL;
> +	ret = drm_mode_convert_umode(dev, mode, &crtc_req->mode);
> +	if (ret) {
> +		DRM_DEBUG_KMS("Invalid mode\n");
>  		goto out;
>  	}
>  
> -	if (crtc_req->count_connectors > 0) {
> -		u32 out_id;
> +	/*
> +	 * Check whether the primary plane supports the fb pixel format.
> +	 * Drivers not implementing the universal planes API use a
> +	 * default formats list provided by the DRM core which doesn't
> +	 * match real hardware capabilities. Skip the check in that
> +	 * case.
> +	 */
> +	if (!plane->format_default) {
> +		ret = drm_plane_check_pixel_format(plane,
> +						   fb->format->format,
> +						   fb->modifier);
> +		if (ret) {
> +			struct drm_format_name_buf format_name;
>  
> -		/* Avoid unbounded kernel memory allocation */
> -		if (crtc_req->count_connectors > config->num_connector) {
> -			ret = -EINVAL;
> +			DRM_DEBUG_KMS("Invalid pixel format %s, modifier 0x%llx\n",
> +				      drm_get_format_name(fb->format->format,
> +							  &format_name),
> +				      fb->modifier);
>  			goto out;
>  		}
> +	}
>  
> -		connector_set = kmalloc_array(crtc_req->count_connectors,
> +	ret = drm_crtc_check_viewport(crtc, crtc_req->x, crtc_req->y,
> +				      mode, fb);
> +	if (ret)
> +		goto out;
> +
> +	connector_set = kmalloc_array(crtc_req->count_connectors,
>  					      sizeof(struct drm_connector *),
>  					      GFP_KERNEL);
> -		if (!connector_set) {
> -			ret = -ENOMEM;
> -			goto out;
> -		}
> +	if (!connector_set) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
>  
> -		for (i = 0; i < crtc_req->count_connectors; i++) {
> -			connector_set[i] = NULL;
> -			set_connectors_ptr = (uint32_t __user *)(unsigned long)crtc_req->set_connectors_ptr;
> -			if (get_user(out_id, &set_connectors_ptr[i])) {
> -				ret = -EFAULT;
> -				goto out;
> -			}
> +	for (i = 0; i < crtc_req->count_connectors; i++) {
> +		u32 out_id;
>  
> -			connector = drm_connector_lookup(dev, file_priv, out_id);
> -			if (!connector) {
> -				DRM_DEBUG_KMS("Connector id %d unknown\n",
> -						out_id);
> -				ret = -ENOENT;
> -				goto out;
> -			}
> -			DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
> -					connector->base.id,
> -					connector->name);
> +		connector_set[i] = NULL;
> +		set_connectors_ptr =
> +		(uint32_t __user *)(unsigned long)crtc_req->set_connectors_ptr;
> +		if (get_user(out_id, &set_connectors_ptr[i])) {
> +			ret = -EFAULT;
> +			goto out;
> +		}
>  
> -			connector_set[i] = connector;
> +		connector = drm_connector_lookup(dev, file_priv, out_id);
> +		if (!connector) {
> +			DRM_DEBUG_KMS("Connector id %d unknown\n",
> +					out_id);
> +			ret = -ENOENT;
> +			goto out;
>  		}
> +		DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
> +				connector->base.id,
> +				connector->name);
> +
> +		connector_set[i] = connector;
>  	}
>  
>  	set.crtc = crtc;
> @@ -735,6 +724,11 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
>  	ret = __drm_mode_set_config_internal(&set, &ctx);
>  
>  out:
> +	if (ret == -EDEADLK) {
> +		ret = drm_modeset_backoff(&ctx);
> +		if (!ret)
> +			goto retry;
> +	}
>  	if (fb)
>  		drm_framebuffer_put(fb);
>  
> @@ -743,14 +737,9 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
>  			if (connector_set[i])
>  				drm_connector_put(connector_set[i]);
>  		}
> +		kfree(connector_set);
>  	}
> -	kfree(connector_set);
>  	drm_mode_destroy(dev, mode);
> -	if (ret == -EDEADLK) {
> -		ret = drm_modeset_backoff(&ctx);
> -		if (!ret)
> -			goto retry;
> -	}
>  	drm_modeset_drop_locks(&ctx);
>  	drm_modeset_acquire_fini(&ctx);
>  	mutex_unlock(&crtc->dev->mode_config.mutex);

-- 
Jani Nikula, Intel Open Source Graphics Center