* 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; 9+ 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] 9+ messages in thread* Re: [PATCH] drm/kms/crtc: Improving the func drm_mode_setcrtc
@ 2018-07-27 12:05 ` Maarten Lankhorst
0 siblings, 0 replies; 9+ 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: hemanshu.s, vineet.j, 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);
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 9+ messages in thread[parent not found: <CGME20180803114404epcas5p1c5aaec7ca27e520b5cba52b0d7f09124@epcas5p1.samsung.com>]
* [PATCH v1] drm/kms/crtc: Improving the func drm_mode_setcrtc
[not found] ` <CGME20180803114404epcas5p1c5aaec7ca27e520b5cba52b0d7f09124@epcas5p1.samsung.com>
@ 2018-08-03 11:43 ` Satendra Singh Thakur
2018-08-03 12:24 ` Maarten Lankhorst
0 siblings, 1 reply; 9+ messages in thread
From: Satendra Singh Thakur @ 2018-08-03 11:43 UTC (permalink / raw)
To: Gustavo Padovan, Maarten Lankhorst, Sean Paul, David Airlie,
dri-devel, linux-kernel
Cc: vineet.j, hemanshu.s, sst2005, Satendra Singh Thakur
Following changes are done to this func:
1. The declaration of plane and it's assignment plane = crtc->primary
are only used when mode_valid is set. Therefore, moved it inside
the if(mode_valid) statement.
2. The declaration of connector and set_connectors_ptr and out_id
are moved inside the for loop, as their scope is limited within
that block.
3. Currently, there are 3 checks on count_connectors
and 4 checks on mode related params (mode_valid, mode, fb).
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) {
In the modified code, there are just 1 check on mode_valid and
2 checks count_connectors.
Checks on mode and fb are not needed as these variables will
be non-NULL by the end of if(mode_valid) statement if mode_valid is set.
If mode_valid is clear, mode and fb will be NULL.
Therefore, we just check mode_valid and NOT mode or fb.
4. Moved kfree inside if statement
Signed-off-by: Satendra Singh Thakur <satendra.t@samsung.com>
---
v1: Hi Mr Maarten, Thanks for the comments.
I have fixed some of them and done more modifications to the patch.
Please review.
drivers/gpu/drm/drm_crtc.c | 57 ++++++++++++++++++++++++----------------------
1 file changed, 30 insertions(+), 27 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 98a36e6..9842985 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -559,12 +559,10 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
struct drm_mode_config *config = &dev->mode_config;
struct drm_mode_crtc *crtc_req = data;
struct drm_crtc *crtc;
- struct drm_plane *plane;
- struct drm_connector **connector_set = NULL, *connector;
+ struct drm_connector **connector_set = NULL;
struct drm_framebuffer *fb = NULL;
struct drm_display_mode *mode = NULL;
struct drm_mode_set set;
- uint32_t __user *set_connectors_ptr;
struct drm_modeset_acquire_ctx ctx;
int ret;
int i;
@@ -586,7 +584,6 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
}
DRM_DEBUG_KMS("[CRTC:%d:%s]\n", crtc->base.id, crtc->name);
- plane = crtc->primary;
mutex_lock(&crtc->dev->mode_config.mutex);
drm_modeset_acquire_init(&ctx, DRM_MODESET_ACQUIRE_INTERRUPTIBLE);
@@ -596,6 +593,8 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
goto out;
if (crtc_req->mode_valid) {
+ struct drm_plane *plane = crtc->primary;
+ /* Handle framebuffer and mode here*/
/* 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) {
@@ -636,8 +635,6 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
ret = -EINVAL;
goto out;
}
-
-
ret = drm_mode_convert_umode(dev, mode, &crtc_req->mode);
if (ret) {
DRM_DEBUG_KMS("Invalid mode\n");
@@ -669,31 +666,22 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
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;
- 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;
- goto out;
- }
-
- if (crtc_req->count_connectors > 0) {
- u32 out_id;
-
+ /* Handle connector here
+ * crtc_req->mode_valid is set at this point
+ * and we have mode and fb non-NULL.
+ * We have already checked mode_valid
+ * hence, we don't check mode and fb here.
+ */
+ if (!crtc_req->count_connectors) {
+ DRM_DEBUG_KMS("Mode_valid flag is set but connectors' count is 0\n");
+ ret = -EINVAL;
+ goto out;
+ }
/* Avoid unbounded kernel memory allocation */
if (crtc_req->count_connectors > config->num_connector) {
ret = -EINVAL;
goto out;
}
-
connector_set = kmalloc_array(crtc_req->count_connectors,
sizeof(struct drm_connector *),
GFP_KERNEL);
@@ -703,6 +691,9 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
}
for (i = 0; i < crtc_req->count_connectors; i++) {
+ struct drm_connector *connector;
+ uint32_t __user *set_connectors_ptr;
+ u32 out_id;
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])) {
@@ -723,6 +714,18 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
connector_set[i] = connector;
}
+
+ } else {
+ /* crtc_req->mode_valid is clear at this point
+ * if mode_valid is clear, mode and fb will be NULL
+ * hence, we don't check mode and fb here.
+ */
+ if (crtc_req->count_connectors) {
+ DRM_DEBUG_KMS("Connectors's count is %u but mode_valid flag is clear\n",
+ crtc_req->count_connectors);
+ ret = -EINVAL;
+ goto out;
+ }
}
set.crtc = crtc;
@@ -743,8 +746,8 @@ 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);
--
2.7.4
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH v1] drm/kms/crtc: Improving the func drm_mode_setcrtc
2018-08-03 11:43 ` [PATCH v1] " Satendra Singh Thakur
@ 2018-08-03 12:24 ` Maarten Lankhorst
0 siblings, 0 replies; 9+ messages in thread
From: Maarten Lankhorst @ 2018-08-03 12:24 UTC (permalink / raw)
To: Satendra Singh Thakur, Gustavo Padovan, Sean Paul, David Airlie,
dri-devel, linux-kernel
Cc: vineet.j, hemanshu.s, sst2005
Op 03-08-18 om 13:43 schreef Satendra Singh Thakur:
> Following changes are done to this func:
> 1. The declaration of plane and it's assignment plane = crtc->primary
> are only used when mode_valid is set. Therefore, moved it inside
> the if(mode_valid) statement.
>
> 2. The declaration of connector and set_connectors_ptr and out_id
> are moved inside the for loop, as their scope is limited within
> that block.
>
> 3. Currently, there are 3 checks on count_connectors
> and 4 checks on mode related params (mode_valid, mode, fb).
> 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) {
>
> In the modified code, there are just 1 check on mode_valid and
> 2 checks count_connectors.
> Checks on mode and fb are not needed as these variables will
> be non-NULL by the end of if(mode_valid) statement if mode_valid is set.
> If mode_valid is clear, mode and fb will be NULL.
> Therefore, we just check mode_valid and NOT mode or fb.
>
> 4. Moved kfree inside if statement
>
> Signed-off-by: Satendra Singh Thakur <satendra.t@samsung.com>
>
> ---
>
> v1: Hi Mr Maarten, Thanks for the comments.
> I have fixed some of them and done more modifications to the patch.
> Please review.
Could you read the suggestions on https://patchwork.freedesktop.org/patch/241508/ ?
I'm not saying this code is incorrect, I think that if you want to improve drm then you should tackle something bigger than just function readability. :)
Cheers,
~Maarten
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v1] drm/kms/crtc: Improving the func drm_mode_setcrtc
@ 2018-08-03 12:24 ` Maarten Lankhorst
0 siblings, 0 replies; 9+ messages in thread
From: Maarten Lankhorst @ 2018-08-03 12:24 UTC (permalink / raw)
To: Satendra Singh Thakur, Gustavo Padovan, Sean Paul, David Airlie,
dri-devel, linux-kernel
Cc: hemanshu.s, vineet.j, sst2005
Op 03-08-18 om 13:43 schreef Satendra Singh Thakur:
> Following changes are done to this func:
> 1. The declaration of plane and it's assignment plane = crtc->primary
> are only used when mode_valid is set. Therefore, moved it inside
> the if(mode_valid) statement.
>
> 2. The declaration of connector and set_connectors_ptr and out_id
> are moved inside the for loop, as their scope is limited within
> that block.
>
> 3. Currently, there are 3 checks on count_connectors
> and 4 checks on mode related params (mode_valid, mode, fb).
> 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) {
>
> In the modified code, there are just 1 check on mode_valid and
> 2 checks count_connectors.
> Checks on mode and fb are not needed as these variables will
> be non-NULL by the end of if(mode_valid) statement if mode_valid is set.
> If mode_valid is clear, mode and fb will be NULL.
> Therefore, we just check mode_valid and NOT mode or fb.
>
> 4. Moved kfree inside if statement
>
> Signed-off-by: Satendra Singh Thakur <satendra.t@samsung.com>
>
> ---
>
> v1: Hi Mr Maarten, Thanks for the comments.
> I have fixed some of them and done more modifications to the patch.
> Please review.
Could you read the suggestions on https://patchwork.freedesktop.org/patch/241508/ ?
I'm not saying this code is incorrect, I think that if you want to improve drm then you should tackle something bigger than just function readability. :)
Cheers,
~Maarten
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 9+ 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-08-24 13:50 ` Jani Nikula
2018-08-24 13:50 ` Jani Nikula
2018-08-24 13:58 ` Daniel Stone
2 siblings, 0 replies; 9+ 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] 9+ messages in thread* Re: [PATCH] drm/kms/crtc: Improving the func drm_mode_setcrtc
@ 2018-08-24 13:50 ` Jani Nikula
0 siblings, 0 replies; 9+ messages in thread
From: Jani Nikula @ 2018-08-24 13:50 UTC (permalink / raw)
To: 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] 9+ 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; 9+ 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] 9+ messages in thread