* Re: [PATCH] drm/kms/crtc: Improving the func drm_mode_setcrtc
2018-07-27 10:12 ` [PATCH] drm/kms/crtc: Improving the func drm_mode_setcrtc Satendra Singh Thakur
@ 2018-07-27 12:05 ` Maarten Lankhorst
[not found] ` <CGME20180803114404epcas5p1c5aaec7ca27e520b5cba52b0d7f09124@epcas5p1.samsung.com>
2018-08-24 13:50 ` [PATCH] " Jani Nikula
2018-08-24 13:58 ` Daniel Stone
2 siblings, 1 reply; 6+ messages in thread
From: Maarten Lankhorst @ 2018-07-27 12:05 UTC (permalink / raw)
To: Satendra Singh Thakur, Gustavo Padovan, Sean Paul, David Airlie,
dri-devel, linux-kernel
Cc: vineet.j, hemanshu.s, sst2005
Hey,
Op 27-07-18 om 12:12 schreef Satendra Singh Thakur:
> Following changes are done to this func:
> 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;
> + }
It is allowed to pass crtc_id, mode_valid = false and count_connectors == 0, you're missing this check now.
It's used to disable a crtc:
https://cgit.freedesktop.org/drm/igt-gpu-tools/tree/lib/igt_kms.c#n1452
> + /* 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);
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] drm/kms/crtc: Improving the func drm_mode_setcrtc
2018-07-27 10:12 ` [PATCH] drm/kms/crtc: Improving the func drm_mode_setcrtc Satendra Singh Thakur
2018-07-27 12:05 ` Maarten Lankhorst
@ 2018-08-24 13:50 ` Jani Nikula
2018-08-24 13:58 ` Daniel Stone
2 siblings, 0 replies; 6+ messages in thread
From: Jani Nikula @ 2018-08-24 13:50 UTC (permalink / raw)
To: Satendra Singh Thakur, Gustavo Padovan, Maarten Lankhorst,
Sean Paul, David Airlie, dri-devel, linux-kernel
Cc: hemanshu.s, vineet.j, sst2005, Satendra Singh Thakur
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] drm/kms/crtc: Improving the func drm_mode_setcrtc
2018-07-27 10:12 ` [PATCH] drm/kms/crtc: Improving the func drm_mode_setcrtc Satendra Singh Thakur
2018-07-27 12:05 ` Maarten Lankhorst
2018-08-24 13:50 ` [PATCH] " Jani Nikula
@ 2018-08-24 13:58 ` Daniel Stone
2 siblings, 0 replies; 6+ messages in thread
From: Daniel Stone @ 2018-08-24 13:58 UTC (permalink / raw)
To: Satendra Singh Thakur
Cc: Gustavo Padovan, Maarten Lankhorst, Sean Paul, David Airlie,
dri-devel, Linux Kernel Mailing List, vineet.j, hemanshu.s,
sst2005
Hi Satendra,
On Fri, 27 Jul 2018 at 11:13, Satendra Singh Thakur
<satendra.t@samsung.com> wrote:
> Following changes are done to this func:
I would certainly agree with Sean, Martin, and Jani's comments. This
patch was difficult to follow as it made so many changes at once.
Being a crucial function which has many subtle details, it is
important to keep the _changes_ as readable as possible: not just the
final result, but the intermediate patches.
Another thing you might consider is running the igt test suite after
your patches, particularly when touching core code. Running igt
would've flagged the below:
> + /* 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;
> + }
This is a change from the previous behaviour, and not correct.
mode_valid is allowed to be NULL if there are also no connectors and
there is no FB: this disables the CRTC.
> + /* 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;
> + }
Same comment applies here.
Cheers,
Daniel
^ permalink raw reply [flat|nested] 6+ messages in thread