From 165e5ffaffc024f77163972ed68993203aeaf77a Mon Sep 17 00:00:00 2001 From: Rahul Sharma Date: Tue, 22 Jan 2019 14:52:24 +0530 Subject: [PATCH 1/4] drm/msm: support release/retire fence through set prop Multiple drm clients are allowed to call the get_properties on drm device node. If sde driver creates the fence for each get_property call then it may leave fd leak in client process context because might not be expecting fences. Supporting get_property for only master device may not solve the issue because master client may still call the get_properties multiple times between two commit cycles. This patch supports release/retire fence through set property to avoid fence leak. Change-Id: I07fe63fe84901d7f96b522ca6309cfdd90a25c40 Signed-off-by: Dhaval Patel Signed-off-by: Rahul Sharma --- drivers/gpu/drm/msm/sde/sde_connector.c | 22 +++- drivers/gpu/drm/msm/sde/sde_crtc.c | 129 ++++++++++++++++-------- 2 files changed, 106 insertions(+), 45 deletions(-) diff --git a/drivers/gpu/drm/msm/sde/sde_connector.c b/drivers/gpu/drm/msm/sde/sde_connector.c index 7930cc29f7f4..3e8b14b6acd7 100644 --- a/drivers/gpu/drm/msm/sde/sde_connector.c +++ b/drivers/gpu/drm/msm/sde/sde_connector.c @@ -428,6 +428,7 @@ static int sde_connector_atomic_set_property(struct drm_connector *connector, struct sde_connector *c_conn; struct sde_connector_state *c_state; int idx, rc; + uint64_t fence_fd = 0; if (!connector || !state || !property) { SDE_ERROR("invalid argument(s), conn %pK, state %pK, prp %pK\n", @@ -472,6 +473,23 @@ static int sde_connector_atomic_set_property(struct drm_connector *connector, SDE_ERROR("prep fb failed, %d\n", rc); } break; + case CONNECTOR_PROP_RETIRE_FENCE: + rc = sde_fence_create(&c_conn->retire_fence, &fence_fd, 0); + if (rc) { + SDE_ERROR("fence create failed rc:%d\n", rc); + goto end; + } + + rc = copy_to_user((uint64_t __user *)val, &fence_fd, + sizeof(uint64_t)); + if (rc) { + SDE_ERROR("copy to user failed rc:%d\n", rc); + /* fence will be released with timeline update */ + put_unused_fd(fence_fd); + rc = -EFAULT; + goto end; + } + break; case CONNECTOR_PROP_TOPOLOGY_CONTROL: rc = sde_rm_check_property_topctl(val); if (rc) @@ -931,8 +949,8 @@ struct drm_connector *sde_connector_init(struct drm_device *dev, "hdr_control", 0x0, 0, ~0, 0, CONNECTOR_PROP_HDR_CONTROL); - msm_property_install_range(&c_conn->property_info, "RETIRE_FENCE", - 0x0, 0, INR_OPEN_MAX, 0, CONNECTOR_PROP_RETIRE_FENCE); + msm_property_install_volatile_range(&c_conn->property_info, + "RETIRE_FENCE", 0x0, 0, ~0, 0, CONNECTOR_PROP_RETIRE_FENCE); msm_property_install_volatile_signed_range(&c_conn->property_info, "PLL_DELTA", 0x0, INT_MIN, INT_MAX, 0, diff --git a/drivers/gpu/drm/msm/sde/sde_crtc.c b/drivers/gpu/drm/msm/sde/sde_crtc.c index 733ff5f686c0..fe72149c40c0 100644 --- a/drivers/gpu/drm/msm/sde/sde_crtc.c +++ b/drivers/gpu/drm/msm/sde/sde_crtc.c @@ -1,5 +1,5 @@ /* - * Copyright (c) 2014-2018 The Linux Foundation. All rights reserved. + * Copyright (c) 2014-2019 The Linux Foundation. All rights reserved. * Copyright (C) 2013 Red Hat * Author: Rob Clark * @@ -1642,8 +1642,8 @@ static void sde_crtc_install_properties(struct drm_crtc *crtc, "input_fence_timeout", 0x0, 0, SDE_CRTC_MAX_INPUT_FENCE_TIMEOUT, SDE_CRTC_INPUT_FENCE_TIMEOUT, CRTC_PROP_INPUT_FENCE_TIMEOUT); - msm_property_install_range(&sde_crtc->property_info, "output_fence", - 0x0, 0, INR_OPEN_MAX, 0x0, CRTC_PROP_OUTPUT_FENCE); + msm_property_install_volatile_range(&sde_crtc->property_info, + "output_fence", 0x0, 0, ~0, 0, CRTC_PROP_OUTPUT_FENCE); msm_property_install_range(&sde_crtc->property_info, "output_fence_offset", 0x0, 0, 1, 0, @@ -1708,6 +1708,21 @@ static void sde_crtc_install_properties(struct drm_crtc *crtc, kfree(info); } +static int _sde_crtc_get_output_fence(struct drm_crtc *crtc, + const struct drm_crtc_state *state, uint64_t *val) +{ + struct sde_crtc *sde_crtc; + struct sde_crtc_state *cstate; + uint32_t offset; + + sde_crtc = to_sde_crtc(crtc); + cstate = to_sde_crtc_state(state); + + offset = sde_crtc_get_property(cstate, CRTC_PROP_OUTPUT_FENCE_OFFSET); + + return sde_fence_create(&sde_crtc->output_fence, val, offset); +} + /** * sde_crtc_atomic_set_property - atomically set a crtc drm property * @crtc: Pointer to drm crtc structure @@ -1724,28 +1739,58 @@ static int sde_crtc_atomic_set_property(struct drm_crtc *crtc, struct sde_crtc *sde_crtc; struct sde_crtc_state *cstate; int idx, ret = -EINVAL; + uint64_t fence_fd = 0; if (!crtc || !state || !property) { SDE_ERROR("invalid argument(s)\n"); - } else { - sde_crtc = to_sde_crtc(crtc); - cstate = to_sde_crtc_state(state); - ret = msm_property_atomic_set(&sde_crtc->property_info, - cstate->property_values, cstate->property_blobs, - property, val); - if (!ret) { - idx = msm_property_index(&sde_crtc->property_info, - property); - if (idx == CRTC_PROP_INPUT_FENCE_TIMEOUT) - _sde_crtc_set_input_fence_timeout(cstate); - } else { - ret = sde_cp_crtc_set_property(crtc, - property, val); - } - if (ret) - DRM_ERROR("failed to set the property\n"); + return -EINVAL; } + sde_crtc = to_sde_crtc(crtc); + cstate = to_sde_crtc_state(state); + + ret = msm_property_atomic_set(&sde_crtc->property_info, + cstate->property_values, cstate->property_blobs, + property, val); + + if (!ret) { + idx = msm_property_index(&sde_crtc->property_info, + property); + switch (idx) { + case CRTC_PROP_INPUT_FENCE_TIMEOUT: + _sde_crtc_set_input_fence_timeout(cstate); + break; + case CRTC_PROP_OUTPUT_FENCE: + ret = _sde_crtc_get_output_fence(crtc, + state, &fence_fd); + if (ret) { + SDE_ERROR("fence create failed rc:%d\n", ret); + goto exit; + } + + ret = copy_to_user((uint64_t __user *)val, &fence_fd, + sizeof(uint64_t)); + + if (ret) { + SDE_ERROR("copy to user failed rc:%d\n", ret); + put_unused_fd(fence_fd); + ret = -EFAULT; + goto exit; + } + break; + default: + /* nothing to do */ + break; + } + } else { + ret = sde_cp_crtc_set_property(crtc, + property, val); + } + +exit: + if (ret) + DRM_ERROR("failed to set the property\n"); + return ret; } @@ -1783,30 +1828,28 @@ static int sde_crtc_atomic_get_property(struct drm_crtc *crtc, if (!crtc || !state) { SDE_ERROR("invalid argument(s)\n"); - } else { - sde_crtc = to_sde_crtc(crtc); - cstate = to_sde_crtc_state(state); - - i = msm_property_index(&sde_crtc->property_info, property); - if (i == CRTC_PROP_OUTPUT_FENCE) { - int offset = sde_crtc_get_property(cstate, - CRTC_PROP_OUTPUT_FENCE_OFFSET); - - ret = sde_fence_create(&sde_crtc->output_fence, val, - offset); - if (ret) - SDE_ERROR("fence create failed\n"); - } else { - ret = msm_property_atomic_get(&sde_crtc->property_info, - cstate->property_values, - cstate->property_blobs, property, val); - if (ret) - ret = sde_cp_crtc_get_property(crtc, - property, val); - } - if (ret) - DRM_ERROR("get property failed\n"); + return -EINVAL; } + + sde_crtc = to_sde_crtc(crtc); + cstate = to_sde_crtc_state(state); + + i = msm_property_index(&sde_crtc->property_info, property); + if (i == CRTC_PROP_OUTPUT_FENCE) { + ret = _sde_crtc_get_output_fence(crtc, state, val); + if (ret) + SDE_ERROR("fence create failed\n"); + } else { + ret = msm_property_atomic_get(&sde_crtc->property_info, + cstate->property_values, + cstate->property_blobs, property, val); + if (ret) + ret = sde_cp_crtc_get_property(crtc, + property, val); + } + if (ret) + DRM_ERROR("get property failed\n"); + return ret; } From d9f7a5cfa7e0c516914e0c403fcfc64235a56db6 Mon Sep 17 00:00:00 2001 From: Rahul Sharma Date: Tue, 22 Jan 2019 15:17:59 +0530 Subject: [PATCH 2/4] drm/msm/sde: remove fence support through get_property Remove release and retire fence support through get property call. Change-Id: Ib5e3643a8cb10e6bb1ffa45dd78b7a31fbba8cc9 Signed-off-by: Dhaval Patel Signed-off-by: Rahul Sharma --- drivers/gpu/drm/msm/sde/sde_connector.c | 8 +++++--- drivers/gpu/drm/msm/sde/sde_crtc.c | 5 ++--- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/msm/sde/sde_connector.c b/drivers/gpu/drm/msm/sde/sde_connector.c index 3e8b14b6acd7..3f65b1fef26a 100644 --- a/drivers/gpu/drm/msm/sde/sde_connector.c +++ b/drivers/gpu/drm/msm/sde/sde_connector.c @@ -562,12 +562,14 @@ static int sde_connector_atomic_get_property(struct drm_connector *connector, c_state = to_sde_connector_state(state); idx = msm_property_index(&c_conn->property_info, property); - if (idx == CONNECTOR_PROP_RETIRE_FENCE) - rc = sde_fence_create(&c_conn->retire_fence, val, 0); - else + if (idx == CONNECTOR_PROP_RETIRE_FENCE) { + *val = ~0; + rc = 0; + } else { /* get cached property value */ rc = msm_property_atomic_get(&c_conn->property_info, c_state->property_values, 0, property, val); + } /* allow for custom override */ if (c_conn->ops.get_property) diff --git a/drivers/gpu/drm/msm/sde/sde_crtc.c b/drivers/gpu/drm/msm/sde/sde_crtc.c index fe72149c40c0..50ae398e319c 100644 --- a/drivers/gpu/drm/msm/sde/sde_crtc.c +++ b/drivers/gpu/drm/msm/sde/sde_crtc.c @@ -1836,9 +1836,8 @@ static int sde_crtc_atomic_get_property(struct drm_crtc *crtc, i = msm_property_index(&sde_crtc->property_info, property); if (i == CRTC_PROP_OUTPUT_FENCE) { - ret = _sde_crtc_get_output_fence(crtc, state, val); - if (ret) - SDE_ERROR("fence create failed\n"); + *val = ~0; + ret = 0; } else { ret = msm_property_atomic_get(&sde_crtc->property_info, cstate->property_values, From 6e84efb6ff902cb99af71f6f087ba4e69e82f73f Mon Sep 17 00:00:00 2001 From: Rahul Sharma Date: Tue, 22 Jan 2019 15:22:25 +0530 Subject: [PATCH 3/4] drm/msm/sde: avoid fence creation if property reset Avoid creating fence on crtc and connector if property reset value set. This will avoid creating get_unsed_fd on dying process. Change-Id: Id4e898c55167b3568962384cade5e60b38c30468 Signed-off-by: Dhaval Patel Signed-off-by: Rahul Sharma --- drivers/gpu/drm/msm/sde/sde_connector.c | 3 +++ drivers/gpu/drm/msm/sde/sde_crtc.c | 3 +++ 2 files changed, 6 insertions(+) diff --git a/drivers/gpu/drm/msm/sde/sde_connector.c b/drivers/gpu/drm/msm/sde/sde_connector.c index 3f65b1fef26a..5da02221103c 100644 --- a/drivers/gpu/drm/msm/sde/sde_connector.c +++ b/drivers/gpu/drm/msm/sde/sde_connector.c @@ -474,6 +474,9 @@ static int sde_connector_atomic_set_property(struct drm_connector *connector, } break; case CONNECTOR_PROP_RETIRE_FENCE: + if (!val) + goto end; + rc = sde_fence_create(&c_conn->retire_fence, &fence_fd, 0); if (rc) { SDE_ERROR("fence create failed rc:%d\n", rc); diff --git a/drivers/gpu/drm/msm/sde/sde_crtc.c b/drivers/gpu/drm/msm/sde/sde_crtc.c index 50ae398e319c..02b344410cfc 100644 --- a/drivers/gpu/drm/msm/sde/sde_crtc.c +++ b/drivers/gpu/drm/msm/sde/sde_crtc.c @@ -1761,6 +1761,9 @@ static int sde_crtc_atomic_set_property(struct drm_crtc *crtc, _sde_crtc_set_input_fence_timeout(cstate); break; case CRTC_PROP_OUTPUT_FENCE: + if (!val) + goto exit; + ret = _sde_crtc_get_output_fence(crtc, state, &fence_fd); if (ret) { From c021cd0f307bb604a8244dd6e494ce701b4eae65 Mon Sep 17 00:00:00 2001 From: Rahul Sharma Date: Tue, 22 Jan 2019 15:27:02 +0530 Subject: [PATCH 4/4] drm/msm/sde: set correct timeline at fence create This change adds the offset to the associated timeline when the retire and release fences are created. The DRM client queries the fences using the atomic commit instead of set_property ioctl. So the sync point should contain the updated timeline before sending the FDs to client. Change-Id: I1ac9507934223bd1091be6960805c63cb4aacfb1 Signed-off-by: Abhijit Kulkarni Signed-off-by: Rahul Sharma --- drivers/gpu/drm/msm/sde/sde_connector.c | 5 ++++- drivers/gpu/drm/msm/sde/sde_crtc.c | 7 +++++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/msm/sde/sde_connector.c b/drivers/gpu/drm/msm/sde/sde_connector.c index 5da02221103c..83aaeee13d73 100644 --- a/drivers/gpu/drm/msm/sde/sde_connector.c +++ b/drivers/gpu/drm/msm/sde/sde_connector.c @@ -477,7 +477,10 @@ static int sde_connector_atomic_set_property(struct drm_connector *connector, if (!val) goto end; - rc = sde_fence_create(&c_conn->retire_fence, &fence_fd, 0); + /* + * update the the offset to a timeline for commit completion + */ + rc = sde_fence_create(&c_conn->retire_fence, &fence_fd, 1); if (rc) { SDE_ERROR("fence create failed rc:%d\n", rc); goto end; diff --git a/drivers/gpu/drm/msm/sde/sde_crtc.c b/drivers/gpu/drm/msm/sde/sde_crtc.c index 02b344410cfc..9805c8e8acb4 100644 --- a/drivers/gpu/drm/msm/sde/sde_crtc.c +++ b/drivers/gpu/drm/msm/sde/sde_crtc.c @@ -1720,6 +1720,13 @@ static int _sde_crtc_get_output_fence(struct drm_crtc *crtc, offset = sde_crtc_get_property(cstate, CRTC_PROP_OUTPUT_FENCE_OFFSET); + /* + * Hwcomposer now queries the fences using the commit list in atomic + * commit ioctl. The offset should be set to next timeline + * which will be incremented during the prepare commit phase + */ + offset++; + return sde_fence_create(&sde_crtc->output_fence, val, offset); }