From: Sean Paul seanpaul@chromium.org

It's only used for debugfs, so just output the enum value instead.

Changes in v2: - None

Reviewed-by: Jeykumar Sankaran jsanka@codeaurora.org Signed-off-by: Sean Paul seanpaul@chromium.org --- drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 6 ++---- drivers/gpu/drm/msm/disp/dpu1/dpu_power_handle.c | 14 -------------- drivers/gpu/drm/msm/disp/dpu1/dpu_power_handle.h | 7 ------- 3 files changed, 2 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c index ed84cf44a222..e09209d6c469 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c @@ -1381,11 +1381,9 @@ static int dpu_crtc_debugfs_state_show(struct seq_file *s, void *v) dpu_crtc->cur_perf.core_clk_rate); for (i = DPU_POWER_HANDLE_DBUS_ID_MNOC; i < DPU_POWER_HANDLE_DBUS_ID_MAX; i++) { - seq_printf(s, "bw_ctl[%s]: %llu\n", - dpu_power_handle_get_dbus_name(i), + seq_printf(s, "bw_ctl[%d]: %llu\n", i, dpu_crtc->cur_perf.bw_ctl[i]); - seq_printf(s, "max_per_pipe_ib[%s]: %llu\n", - dpu_power_handle_get_dbus_name(i), + seq_printf(s, "max_per_pipe_ib[%d]: %llu\n", i, dpu_crtc->cur_perf.max_per_pipe_ib[i]); }

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_power_handle.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_power_handle.c index fc14116789f2..8c6f92aaaf87 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_power_handle.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_power_handle.c @@ -24,20 +24,6 @@ #include "dpu_power_handle.h" #include "dpu_trace.h"

-static const char *data_bus_name[DPU_POWER_HANDLE_DBUS_ID_MAX] = { - [DPU_POWER_HANDLE_DBUS_ID_MNOC] = "qcom,dpu-data-bus", - [DPU_POWER_HANDLE_DBUS_ID_LLCC] = "qcom,dpu-llcc-bus", - [DPU_POWER_HANDLE_DBUS_ID_EBI] = "qcom,dpu-ebi-bus", -}; - -const char *dpu_power_handle_get_dbus_name(u32 bus_id) -{ - if (bus_id < DPU_POWER_HANDLE_DBUS_ID_MAX) - return data_bus_name[bus_id]; - - return NULL; -} - static void dpu_power_event_trigger_locked(struct dpu_power_handle *phandle, u32 event_type) { diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_power_handle.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_power_handle.h index a65b7a297f21..f627ae28ec68 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_power_handle.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_power_handle.h @@ -207,11 +207,4 @@ struct dpu_power_event *dpu_power_handle_register_event( void dpu_power_handle_unregister_event(struct dpu_power_handle *phandle, struct dpu_power_event *event);

-/** - * dpu_power_handle_get_dbus_name - get name of given data bus identifier - * @bus_id: data bus identifier - * Return: Pointer to name string if success; NULL otherwise - */ -const char *dpu_power_handle_get_dbus_name(u32 bus_id); - #endif /* _DPU_POWER_HANDLE_H_ */

From: Sean Paul seanpaul@chromium.org

There's only one client -- core, and it's only used for runtime pm which is already refcounted.

Changes in v2: - None

Reviewed-by: Jeykumar Sankaran jsanka@codeaurora.org Signed-off-by: Sean Paul seanpaul@chromium.org --- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 22 +---- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 1 - .../gpu/drm/msm/disp/dpu1/dpu_power_handle.c | 96 +------------------ .../gpu/drm/msm/disp/dpu1/dpu_power_handle.h | 86 +---------------- 4 files changed, 6 insertions(+), 199 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c index 985c855796ae..23094d108e81 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c @@ -676,11 +676,6 @@ static void _dpu_kms_hw_destroy(struct dpu_kms *dpu_kms) dpu_hw_catalog_deinit(dpu_kms->catalog); dpu_kms->catalog = NULL;

- if (dpu_kms->core_client) - dpu_power_client_destroy(&dpu_kms->phandle, - dpu_kms->core_client); - dpu_kms->core_client = NULL; - if (dpu_kms->vbif[VBIF_NRT]) devm_iounmap(&dpu_kms->pdev->dev, dpu_kms->vbif[VBIF_NRT]); dpu_kms->vbif[VBIF_NRT] = NULL; @@ -913,17 +908,6 @@ static int dpu_kms_hw_init(struct msm_kms *kms) dpu_kms->reg_dma_len = dpu_iomap_size(dpu_kms->pdev, "regdma"); }

- dpu_kms->core_client = dpu_power_client_create(&dpu_kms->phandle, - "core"); - if (IS_ERR_OR_NULL(dpu_kms->core_client)) { - rc = PTR_ERR(dpu_kms->core_client); - if (!dpu_kms->core_client) - rc = -EINVAL; - DPU_ERROR("dpu power client create failed: %d\n", rc); - dpu_kms->core_client = NULL; - goto error; - } - pm_runtime_get_sync(&dpu_kms->pdev->dev);

_dpu_kms_core_hw_rev_init(dpu_kms); @@ -1157,8 +1141,7 @@ static int __maybe_unused dpu_runtime_suspend(struct device *dev) return rc; }

- rc = dpu_power_resource_enable(&dpu_kms->phandle, - dpu_kms->core_client, false); + rc = dpu_power_resource_enable(&dpu_kms->phandle, false); if (rc) DPU_ERROR("resource disable failed: %d\n", rc);

@@ -1189,8 +1172,7 @@ static int __maybe_unused dpu_runtime_resume(struct device *dev) return rc; }

- rc = dpu_power_resource_enable(&dpu_kms->phandle, - dpu_kms->core_client, true); + rc = dpu_power_resource_enable(&dpu_kms->phandle, true); if (rc) DPU_ERROR("resource enable failed: %d\n", rc);

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h index 2a3625eef6d3..f2c78deb0854 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h @@ -114,7 +114,6 @@ struct dpu_kms { struct dpu_mdss_cfg *catalog;

struct dpu_power_handle phandle; - struct dpu_power_client *core_client; struct dpu_power_event *power_event;

/* directory entry for debugfs */ diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_power_handle.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_power_handle.c index 8c6f92aaaf87..8e64f0a52147 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_power_handle.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_power_handle.c @@ -35,59 +35,11 @@ static void dpu_power_event_trigger_locked(struct dpu_power_handle *phandle, } }

-struct dpu_power_client *dpu_power_client_create( - struct dpu_power_handle *phandle, char *client_name) -{ - struct dpu_power_client *client; - static u32 id; - - if (!client_name || !phandle) { - pr_err("client name is null or invalid power data\n"); - return ERR_PTR(-EINVAL); - } - - client = kzalloc(sizeof(struct dpu_power_client), GFP_KERNEL); - if (!client) - return ERR_PTR(-ENOMEM); - - mutex_lock(&phandle->phandle_lock); - strlcpy(client->name, client_name, MAX_CLIENT_NAME_LEN); - client->usecase_ndx = VOTE_INDEX_DISABLE; - client->id = id; - client->active = true; - pr_debug("client %s created:%pK id :%d\n", client_name, - client, id); - id++; - list_add(&client->list, &phandle->power_client_clist); - mutex_unlock(&phandle->phandle_lock); - - return client; -} - -void dpu_power_client_destroy(struct dpu_power_handle *phandle, - struct dpu_power_client *client) -{ - if (!client || !phandle) { - pr_err("reg bus vote: invalid client handle\n"); - } else if (!client->active) { - pr_err("dpu power deinit already done\n"); - kfree(client); - } else { - pr_debug("bus vote client %s destroyed:%pK id:%u\n", - client->name, client, client->id); - mutex_lock(&phandle->phandle_lock); - list_del_init(&client->list); - mutex_unlock(&phandle->phandle_lock); - kfree(client); - } -} - void dpu_power_resource_init(struct platform_device *pdev, struct dpu_power_handle *phandle) { phandle->dev = &pdev->dev;

- INIT_LIST_HEAD(&phandle->power_client_clist); INIT_LIST_HEAD(&phandle->event_list);

mutex_init(&phandle->phandle_lock); @@ -96,7 +48,6 @@ void dpu_power_resource_init(struct platform_device *pdev, void dpu_power_resource_deinit(struct platform_device *pdev, struct dpu_power_handle *phandle) { - struct dpu_power_client *curr_client, *next_client; struct dpu_power_event *curr_event, *next_event;

if (!phandle || !pdev) { @@ -105,15 +56,6 @@ void dpu_power_resource_deinit(struct platform_device *pdev, }

mutex_lock(&phandle->phandle_lock); - list_for_each_entry_safe(curr_client, next_client, - &phandle->power_client_clist, list) { - pr_err("client:%s-%d still registered with refcount:%d\n", - curr_client->name, curr_client->id, - curr_client->refcount); - curr_client->active = false; - list_del(&curr_client->list); - } - list_for_each_entry_safe(curr_event, next_event, &phandle->event_list, list) { pr_err("event:%d, client:%s still registered\n", @@ -125,53 +67,21 @@ void dpu_power_resource_deinit(struct platform_device *pdev, mutex_unlock(&phandle->phandle_lock); }

-int dpu_power_resource_enable(struct dpu_power_handle *phandle, - struct dpu_power_client *pclient, bool enable) +int dpu_power_resource_enable(struct dpu_power_handle *phandle, bool enable) { - bool changed = false; - u32 max_usecase_ndx = VOTE_INDEX_DISABLE, prev_usecase_ndx; - struct dpu_power_client *client; u32 event_type;

- if (!phandle || !pclient) { + if (!phandle) { pr_err("invalid input argument\n"); return -EINVAL; }

mutex_lock(&phandle->phandle_lock); - if (enable) - pclient->refcount++; - else if (pclient->refcount) - pclient->refcount--; - - if (pclient->refcount) - pclient->usecase_ndx = VOTE_INDEX_LOW; - else - pclient->usecase_ndx = VOTE_INDEX_DISABLE; - - list_for_each_entry(client, &phandle->power_client_clist, list) { - if (client->usecase_ndx < VOTE_INDEX_MAX && - client->usecase_ndx > max_usecase_ndx) - max_usecase_ndx = client->usecase_ndx; - } - - if (phandle->current_usecase_ndx != max_usecase_ndx) { - changed = true; - prev_usecase_ndx = phandle->current_usecase_ndx; - phandle->current_usecase_ndx = max_usecase_ndx; - } - - pr_debug("%pS: changed=%d current idx=%d request client %s id:%u enable:%d refcount:%d\n", - __builtin_return_address(0), changed, max_usecase_ndx, - pclient->name, pclient->id, enable, pclient->refcount); - - if (!changed) - goto end;

event_type = enable ? DPU_POWER_EVENT_ENABLE : DPU_POWER_EVENT_DISABLE;

dpu_power_event_trigger_locked(phandle, event_type); -end: + mutex_unlock(&phandle->phandle_lock); return 0; } diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_power_handle.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_power_handle.h index f627ae28ec68..124ebc93c877 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_power_handle.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_power_handle.h @@ -27,30 +27,6 @@ #define DPU_POWER_EVENT_DISABLE BIT(0) #define DPU_POWER_EVENT_ENABLE BIT(1)

-/** - * mdss_bus_vote_type: register bus vote type - * VOTE_INDEX_DISABLE: removes the client vote - * VOTE_INDEX_LOW: keeps the lowest vote for register bus - * VOTE_INDEX_MAX: invalid - */ -enum mdss_bus_vote_type { - VOTE_INDEX_DISABLE, - VOTE_INDEX_LOW, - VOTE_INDEX_MAX, -}; - -/** - * enum dpu_power_handle_data_bus_client - type of axi bus clients - * @DPU_POWER_HANDLE_DATA_BUS_CLIENT_RT: core real-time bus client - * @DPU_POWER_HANDLE_DATA_BUS_CLIENT_NRT: core non-real-time bus client - * @DPU_POWER_HANDLE_DATA_BUS_CLIENT_MAX: maximum number of bus client type - */ -enum dpu_power_handle_data_bus_client { - DPU_POWER_HANDLE_DATA_BUS_CLIENT_RT, - DPU_POWER_HANDLE_DATA_BUS_CLIENT_NRT, - DPU_POWER_HANDLE_DATA_BUS_CLIENT_MAX -}; - /** * enum DPU_POWER_HANDLE_DBUS_ID - data bus identifier * @DPU_POWER_HANDLE_DBUS_ID_MNOC: DPU/MNOC data bus @@ -64,31 +40,6 @@ enum DPU_POWER_HANDLE_DBUS_ID { DPU_POWER_HANDLE_DBUS_ID_MAX, };

-/** - * struct dpu_power_client: stores the power client for dpu driver - * @name: name of the client - * @usecase_ndx: current regs bus vote type - * @refcount: current refcount if multiple modules are using same - * same client for enable/disable. Power module will - * aggregate the refcount and vote accordingly for this - * client. - * @id: assigned during create. helps for debugging. - * @list: list to attach power handle master list - * @ab: arbitrated bandwidth for each bus client - * @ib: instantaneous bandwidth for each bus client - * @active: inidcates the state of dpu power handle - */ -struct dpu_power_client { - char name[MAX_CLIENT_NAME_LEN]; - short usecase_ndx; - short refcount; - u32 id; - struct list_head list; - u64 ab[DPU_POWER_HANDLE_DATA_BUS_CLIENT_MAX]; - u64 ib[DPU_POWER_HANDLE_DATA_BUS_CLIENT_MAX]; - bool active; -}; - /* * struct dpu_power_event - local event registration structure * @client_name: name of the client registering @@ -109,14 +60,12 @@ struct dpu_power_event {

/** * struct dpu_power_handle: power handle main struct - * @client_clist: master list to store all clients * @phandle_lock: lock to synchronize the enable/disable * @dev: pointer to device structure * @usecase_ndx: current usecase index * @event_list: current power handle event list */ struct dpu_power_handle { - struct list_head power_client_clist; struct mutex phandle_lock; struct device *dev; u32 current_usecase_ndx; @@ -141,47 +90,14 @@ void dpu_power_resource_init(struct platform_device *pdev, void dpu_power_resource_deinit(struct platform_device *pdev, struct dpu_power_handle *pdata);

-/** - * dpu_power_client_create() - create the client on power handle - * @pdata: power handle containing the resources - * @client_name: new client name for registration - * - * Return: error code. - */ -struct dpu_power_client *dpu_power_client_create(struct dpu_power_handle *pdata, - char *client_name); - -/** - * dpu_power_client_destroy() - destroy the client on power handle - * @pdata: power handle containing the resources - * @client_name: new client name for registration - * - * Return: none - */ -void dpu_power_client_destroy(struct dpu_power_handle *phandle, - struct dpu_power_client *client); - /** * dpu_power_resource_enable() - enable/disable the power resources * @pdata: power handle containing the resources - * @client: client information to enable/disable its vote * @enable: boolean request for enable/disable * * Return: error code. */ -int dpu_power_resource_enable(struct dpu_power_handle *pdata, - struct dpu_power_client *pclient, bool enable); - -/** - * dpu_power_data_bus_bandwidth_ctrl() - control data bus bandwidth enable - * @phandle: power handle containing the resources - * @client: client information to bandwidth control - * @enable: true to enable bandwidth for data base - * - * Return: none - */ -void dpu_power_data_bus_bandwidth_ctrl(struct dpu_power_handle *phandle, - struct dpu_power_client *pclient, int enable); +int dpu_power_resource_enable(struct dpu_power_handle *pdata, bool enable);

/** * dpu_power_handle_register_event - register a callback function for an event.

From: Sean Paul seanpaul@chromium.org

It's needed for struct dss_module_power, and is currently being pulled in by dpu_power_handle.h

Changes in v2: - None

Reviewed-by: Jeykumar Sankaran jsanka@codeaurora.org Signed-off-by: Sean Paul seanpaul@chromium.org --- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 1 + 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h index 5f08be187c86..4e5acacb3065 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h @@ -29,6 +29,7 @@ #include "dpu_hw_lm.h" #include "dpu_hw_interrupts.h" #include "dpu_hw_top.h" +#include "dpu_io_util.h" #include "dpu_rm.h" #include "dpu_power_handle.h" #include "dpu_irq.h"

From: Sean Paul seanpaul@chromium.org

Now that we don't have any event handlers, remove dpu_power_handle!

Changes in v2: - None

Reviewed-by: Jeykumar Sankaran jsanka@codeaurora.org Signed-off-by: Sean Paul seanpaul@chromium.org --- drivers/gpu/drm/msm/Makefile | 1 - drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 11 -- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 3 - .../gpu/drm/msm/disp/dpu1/dpu_power_handle.c | 136 ------------------ .../gpu/drm/msm/disp/dpu1/dpu_power_handle.h | 113 --------------- 5 files changed, 264 deletions(-) delete mode 100644 drivers/gpu/drm/msm/disp/dpu1/dpu_power_handle.c delete mode 100644 drivers/gpu/drm/msm/disp/dpu1/dpu_power_handle.h

diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile index 19ab521d4c3a..7d02ef3655b5 100644 --- a/drivers/gpu/drm/msm/Makefile +++ b/drivers/gpu/drm/msm/Makefile @@ -72,7 +72,6 @@ msm-y := \ disp/dpu1/dpu_kms.o \ disp/dpu1/dpu_mdss.o \ disp/dpu1/dpu_plane.o \ - disp/dpu1/dpu_power_handle.o \ disp/dpu1/dpu_rm.o \ disp/dpu1/dpu_vbif.o \ msm_atomic.o \ diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c index af666d917a0b..9f7da56bb453 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c @@ -1060,8 +1060,6 @@ static int dpu_bind(struct device *dev, struct device *master, void *data) return ret; }

- dpu_power_resource_init(pdev, &dpu_kms->phandle); - platform_set_drvdata(pdev, dpu_kms);

msm_kms_init(&dpu_kms->base, &kms_funcs); @@ -1081,7 +1079,6 @@ static void dpu_unbind(struct device *dev, struct device *master, void *data) struct dpu_kms *dpu_kms = platform_get_drvdata(pdev); struct dss_module_power *mp = &dpu_kms->mp;

- dpu_power_resource_deinit(pdev, &dpu_kms->phandle); msm_dss_put_clk(mp->clk_config, mp->num_clk); devm_kfree(&pdev->dev, mp->clk_config); mp->num_clk = 0; @@ -1120,10 +1117,6 @@ static int __maybe_unused dpu_runtime_suspend(struct device *dev) return rc; }

- rc = dpu_power_resource_enable(&dpu_kms->phandle, false); - if (rc) - DPU_ERROR("resource disable failed: %d\n", rc); - rc = msm_dss_enable_clk(mp->clk_config, mp->num_clk, false); if (rc) DPU_ERROR("clock disable failed rc:%d\n", rc); @@ -1157,10 +1150,6 @@ static int __maybe_unused dpu_runtime_resume(struct device *dev) drm_for_each_crtc(crtc, ddev) dpu_crtc_runtime_resume(crtc);

- rc = dpu_power_resource_enable(&dpu_kms->phandle, true); - if (rc) - DPU_ERROR("resource enable failed: %d\n", rc); - return rc; }

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h index 4e5acacb3065..59e18e2d3c59 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h @@ -31,7 +31,6 @@ #include "dpu_hw_top.h" #include "dpu_io_util.h" #include "dpu_rm.h" -#include "dpu_power_handle.h" #include "dpu_irq.h" #include "dpu_core_perf.h"

@@ -114,8 +113,6 @@ struct dpu_kms { int core_rev; struct dpu_mdss_cfg *catalog;

- struct dpu_power_handle phandle; - /* directory entry for debugfs */ struct dentry *debugfs_root; struct dentry *debugfs_danger; diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_power_handle.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_power_handle.c deleted file mode 100644 index 8e64f0a52147..000000000000 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_power_handle.c +++ /dev/null @@ -1,136 +0,0 @@ -/* Copyright (c) 2014-2018, The Linux Foundation. All rights reserved. - * - * This program is free software; you can redistribute it and/or modify - * it under the terms of the GNU General Public License version 2 and - * only version 2 as published by the Free Software Foundation. - * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - * GNU General Public License for more details. - * - */ - -#define pr_fmt(fmt) "[drm:%s:%d]: " fmt, __func__, __LINE__ - -#include <linux/kernel.h> -#include <linux/of.h> -#include <linux/string.h> -#include <linux/of_address.h> -#include <linux/slab.h> -#include <linux/mutex.h> -#include <linux/of_platform.h> - -#include "dpu_power_handle.h" -#include "dpu_trace.h" - -static void dpu_power_event_trigger_locked(struct dpu_power_handle *phandle, - u32 event_type) -{ - struct dpu_power_event *event; - - list_for_each_entry(event, &phandle->event_list, list) { - if (event->event_type & event_type) - event->cb_fnc(event_type, event->usr); - } -} - -void dpu_power_resource_init(struct platform_device *pdev, - struct dpu_power_handle *phandle) -{ - phandle->dev = &pdev->dev; - - INIT_LIST_HEAD(&phandle->event_list); - - mutex_init(&phandle->phandle_lock); -} - -void dpu_power_resource_deinit(struct platform_device *pdev, - struct dpu_power_handle *phandle) -{ - struct dpu_power_event *curr_event, *next_event; - - if (!phandle || !pdev) { - pr_err("invalid input param\n"); - return; - } - - mutex_lock(&phandle->phandle_lock); - list_for_each_entry_safe(curr_event, next_event, - &phandle->event_list, list) { - pr_err("event:%d, client:%s still registered\n", - curr_event->event_type, - curr_event->client_name); - curr_event->active = false; - list_del(&curr_event->list); - } - mutex_unlock(&phandle->phandle_lock); -} - -int dpu_power_resource_enable(struct dpu_power_handle *phandle, bool enable) -{ - u32 event_type; - - if (!phandle) { - pr_err("invalid input argument\n"); - return -EINVAL; - } - - mutex_lock(&phandle->phandle_lock); - - event_type = enable ? DPU_POWER_EVENT_ENABLE : DPU_POWER_EVENT_DISABLE; - - dpu_power_event_trigger_locked(phandle, event_type); - - mutex_unlock(&phandle->phandle_lock); - return 0; -} - -struct dpu_power_event *dpu_power_handle_register_event( - struct dpu_power_handle *phandle, - u32 event_type, void (*cb_fnc)(u32 event_type, void *usr), - void *usr, char *client_name) -{ - struct dpu_power_event *event; - - if (!phandle) { - pr_err("invalid power handle\n"); - return ERR_PTR(-EINVAL); - } else if (!cb_fnc || !event_type) { - pr_err("no callback fnc or event type\n"); - return ERR_PTR(-EINVAL); - } - - event = kzalloc(sizeof(struct dpu_power_event), GFP_KERNEL); - if (!event) - return ERR_PTR(-ENOMEM); - - event->event_type = event_type; - event->cb_fnc = cb_fnc; - event->usr = usr; - strlcpy(event->client_name, client_name, MAX_CLIENT_NAME_LEN); - event->active = true; - - mutex_lock(&phandle->phandle_lock); - list_add(&event->list, &phandle->event_list); - mutex_unlock(&phandle->phandle_lock); - - return event; -} - -void dpu_power_handle_unregister_event( - struct dpu_power_handle *phandle, - struct dpu_power_event *event) -{ - if (!phandle || !event) { - pr_err("invalid phandle or event\n"); - } else if (!event->active) { - pr_err("power handle deinit already done\n"); - kfree(event); - } else { - mutex_lock(&phandle->phandle_lock); - list_del_init(&event->list); - mutex_unlock(&phandle->phandle_lock); - kfree(event); - } -} diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_power_handle.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_power_handle.h deleted file mode 100644 index 7536624c8b20..000000000000 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_power_handle.h +++ /dev/null @@ -1,113 +0,0 @@ -/* Copyright (c) 2016-2018, The Linux Foundation. All rights reserved. - * - * This program is free software; you can redistribute it and/or modify - * it under the terms of the GNU General Public License version 2 and - * only version 2 as published by the Free Software Foundation. - * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - * GNU General Public License for more details. - * - */ - -#ifndef _DPU_POWER_HANDLE_H_ -#define _DPU_POWER_HANDLE_H_ - -#define MAX_CLIENT_NAME_LEN 128 - -#define DPU_POWER_HANDLE_ENABLE_BUS_AB_QUOTA 0 -#define DPU_POWER_HANDLE_DISABLE_BUS_AB_QUOTA 0 -#define DPU_POWER_HANDLE_ENABLE_BUS_IB_QUOTA 1600000000 -#define DPU_POWER_HANDLE_DISABLE_BUS_IB_QUOTA 0 - -#include "dpu_io_util.h" - -/* events will be triggered on power handler enable/disable */ -#define DPU_POWER_EVENT_DISABLE BIT(0) -#define DPU_POWER_EVENT_ENABLE BIT(1) - -/* - * struct dpu_power_event - local event registration structure - * @client_name: name of the client registering - * @cb_fnc: pointer to desired callback function - * @usr: user pointer to pass to callback event trigger - * @event: refer to DPU_POWER_HANDLE_EVENT_* - * @list: list to attach event master list - * @active: indicates the state of dpu power handle - */ -struct dpu_power_event { - char client_name[MAX_CLIENT_NAME_LEN]; - void (*cb_fnc)(u32 event_type, void *usr); - void *usr; - u32 event_type; - struct list_head list; - bool active; -}; - -/** - * struct dpu_power_handle: power handle main struct - * @phandle_lock: lock to synchronize the enable/disable - * @dev: pointer to device structure - * @usecase_ndx: current usecase index - * @event_list: current power handle event list - */ -struct dpu_power_handle { - struct mutex phandle_lock; - struct device *dev; - u32 current_usecase_ndx; - struct list_head event_list; -}; - -/** - * dpu_power_resource_init() - initializes the dpu power handle - * @pdev: platform device to search the power resources - * @pdata: power handle to store the power resources - */ -void dpu_power_resource_init(struct platform_device *pdev, - struct dpu_power_handle *pdata); - -/** - * dpu_power_resource_deinit() - release the dpu power handle - * @pdev: platform device for power resources - * @pdata: power handle containing the resources - * - * Return: error code. - */ -void dpu_power_resource_deinit(struct platform_device *pdev, - struct dpu_power_handle *pdata); - -/** - * dpu_power_resource_enable() - enable/disable the power resources - * @pdata: power handle containing the resources - * @enable: boolean request for enable/disable - * - * Return: error code. - */ -int dpu_power_resource_enable(struct dpu_power_handle *pdata, bool enable); - -/** - * dpu_power_handle_register_event - register a callback function for an event. - * Clients can register for multiple events with a single register. - * Any block with access to phandle can register for the event - * notification. - * @phandle: power handle containing the resources - * @event_type: event type to register; refer DPU_POWER_HANDLE_EVENT_* - * @cb_fnc: pointer to desired callback function - * @usr: user pointer to pass to callback on event trigger - * - * Return: event pointer if success, or error code otherwise - */ -struct dpu_power_event *dpu_power_handle_register_event( - struct dpu_power_handle *phandle, - u32 event_type, void (*cb_fnc)(u32 event_type, void *usr), - void *usr, char *client_name); -/** - * dpu_power_handle_unregister_event - unregister callback for event(s) - * @phandle: power handle containing the resources - * @event: event pointer returned after power handle register - */ -void dpu_power_handle_unregister_event(struct dpu_power_handle *phandle, - struct dpu_power_event *event); - -#endif /* _DPU_POWER_HANDLE_H_ */

From: Sean Paul seanpaul@chromium.org

Add a bool to dpu_encoder_virt to track whether the encoder is enabled or not. Repurpose the enc_lock mutex to ensure that it is consistent with the hw state.

Changes in v2: - None

Cc: Jeykumar Sankaran jsanka@codeaurora.org Signed-off-by: Sean Paul seanpaul@chromium.org --- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 27 +++++++++++++++++---- 1 file changed, 22 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c index 10a0676d1dcf..3daa86220d47 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c @@ -132,6 +132,7 @@ enum dpu_enc_rc_states { * @base: drm_encoder base class for registration with DRM * @enc_spinlock: Virtual-Encoder-Wide Spin Lock for IRQ purposes * @bus_scaling_client: Client handle to the bus scaling interface + * @enabled: True if the encoder is active, protected by enc_lock * @num_phys_encs: Actual number of physical encoders contained. * @phys_encs: Container of physical encoders managed. * @cur_master: Pointer to the current master in this mode. Optimization @@ -148,8 +149,8 @@ enum dpu_enc_rc_states { * all CTL paths * @crtc_kickoff_cb_data: Opaque user data given to crtc_kickoff_cb * @debugfs_root: Debug file system root file node - * @enc_lock: Lock around physical encoder create/destroy and - access. + * @enc_lock: Lock around physical encoder + * create/destroy/enable/disable * @frame_busy_mask: Bitmask tracking which phys_enc we are still * busy processing current command. * Bit0 = phys_encs[0] etc. @@ -175,6 +176,8 @@ struct dpu_encoder_virt { spinlock_t enc_spinlock; uint32_t bus_scaling_client;

+ bool enabled; + unsigned int num_phys_encs; struct dpu_encoder_phys *phys_encs[MAX_PHYS_ENCODERS_PER_VIRTUAL]; struct dpu_encoder_phys *cur_master; @@ -1121,6 +1124,8 @@ static void dpu_encoder_virt_enable(struct drm_encoder *drm_enc) return; } dpu_enc = to_dpu_encoder_virt(drm_enc); + + mutex_lock(&dpu_enc->enc_lock); cur_mode = &dpu_enc->base.crtc->state->adjusted_mode;

trace_dpu_enc_enable(DRMID(drm_enc), cur_mode->hdisplay, @@ -1137,10 +1142,15 @@ static void dpu_encoder_virt_enable(struct drm_encoder *drm_enc) if (ret) { DPU_ERROR_ENC(dpu_enc, "dpu resource control failed: %d\n", ret); - return; + goto out; }

_dpu_encoder_virt_enable_helper(drm_enc); + + dpu_enc->enabled = true; + +out: + mutex_unlock(&dpu_enc->enc_lock); }

static void dpu_encoder_virt_disable(struct drm_encoder *drm_enc) @@ -1162,11 +1172,14 @@ static void dpu_encoder_virt_disable(struct drm_encoder *drm_enc) return; }

- mode = &drm_enc->crtc->state->adjusted_mode; - dpu_enc = to_dpu_encoder_virt(drm_enc); DPU_DEBUG_ENC(dpu_enc, "\n");

+ mutex_lock(&dpu_enc->enc_lock); + dpu_enc->enabled = false; + + mode = &drm_enc->crtc->state->adjusted_mode; + priv = drm_enc->dev->dev_private; dpu_kms = to_dpu_kms(priv->kms);

@@ -1200,6 +1213,8 @@ static void dpu_encoder_virt_disable(struct drm_encoder *drm_enc) DPU_DEBUG_ENC(dpu_enc, "encoder disabled\n");

dpu_rm_release(&dpu_kms->rm, drm_enc); + + mutex_unlock(&dpu_enc->enc_lock); }

static enum dpu_intf dpu_encoder_get_intf(struct dpu_mdss_cfg *catalog, @@ -2233,6 +2248,8 @@ struct drm_encoder *dpu_encoder_init(struct drm_device *dev,

drm_encoder_helper_add(&dpu_enc->base, &dpu_encoder_helper_funcs);

+ dpu_enc->enabled = false; + return &dpu_enc->base; }

From: Sean Paul seanpaul@chromium.org

The crtc runtime resume doesn't actually operate on the crtc, but rather its encoders. The problem with this is that we need to inspect the crtc state to get the currently connected encoders. Since runtime resume isn't guaranteed to be called while holding the modeset locks (although it sometimes is), this presents a race condition.

Now that we have ->enabled on the virtual encoders, and a lock to protect it, just call resume on each encoder and only restore the ones that are enabled.

Changes in v2: - None

Cc: Jeykumar Sankaran jsanka@codeaurora.org Signed-off-by: Sean Paul seanpaul@chromium.org --- drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 24 --------------------- drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h | 6 ------ drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 24 +++++++++------------ drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 4 ++-- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 6 +++--- 5 files changed, 15 insertions(+), 49 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c index d8f58caf2772..9be24907f8c1 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c @@ -841,30 +841,6 @@ static struct drm_crtc_state *dpu_crtc_duplicate_state(struct drm_crtc *crtc) return &cstate->base; }

-void dpu_crtc_runtime_resume(struct drm_crtc *crtc) -{ - struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc); - struct drm_encoder *encoder; - - mutex_lock(&dpu_crtc->crtc_lock); - - if (!dpu_crtc->enabled) - goto end; - - trace_dpu_crtc_runtime_resume(DRMID(crtc)); - - /* restore encoder; crtc will be programmed during commit */ - drm_for_each_encoder(encoder, crtc->dev) { - if (encoder->crtc != crtc) - continue; - - dpu_encoder_virt_restore(encoder); - } - -end: - mutex_unlock(&dpu_crtc->crtc_lock); -} - static void dpu_crtc_disable(struct drm_crtc *crtc) { struct dpu_crtc *dpu_crtc; diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h index 1dca91d1210f..93d21a61a040 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h @@ -329,10 +329,4 @@ static inline bool dpu_crtc_is_enabled(struct drm_crtc *crtc) return crtc ? crtc->enabled : false; }

-/** - * dpu_crtc_runtime_resume - called by the top-level on pm_runtime_resume - * @crtc: CRTC to resume - */ -void dpu_crtc_runtime_resume(struct drm_crtc *crtc); - #endif /* _DPU_CRTC_H_ */ diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c index 3daa86220d47..d89ac520f7e6 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c @@ -1089,28 +1089,24 @@ static void _dpu_encoder_virt_enable_helper(struct drm_encoder *drm_enc) _dpu_encoder_update_vsync_source(dpu_enc, &dpu_enc->disp_info); }

-void dpu_encoder_virt_restore(struct drm_encoder *drm_enc) +void dpu_encoder_virt_runtime_resume(struct drm_encoder *drm_enc) { - struct dpu_encoder_virt *dpu_enc = NULL; - int i; - - if (!drm_enc) { - DPU_ERROR("invalid encoder\n"); - return; - } - dpu_enc = to_dpu_encoder_virt(drm_enc); + struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc);

- for (i = 0; i < dpu_enc->num_phys_encs; i++) { - struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i]; + mutex_lock(&dpu_enc->enc_lock);

- if (phys && (phys != dpu_enc->cur_master) && phys->ops.restore) - phys->ops.restore(phys); - } + if (!dpu_enc->enabled) + goto out;

+ if (dpu_enc->cur_slave && dpu_enc->cur_slave->ops.restore) + dpu_enc->cur_slave->ops.restore(dpu_enc->cur_slave); if (dpu_enc->cur_master && dpu_enc->cur_master->ops.restore) dpu_enc->cur_master->ops.restore(dpu_enc->cur_master);

_dpu_encoder_virt_enable_helper(drm_enc); + +out: + mutex_unlock(&dpu_enc->enc_lock); }

static void dpu_encoder_virt_enable(struct drm_encoder *drm_enc) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h index 9dbf38f446d9..aa4f135218fa 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h @@ -126,10 +126,10 @@ int dpu_encoder_wait_for_event(struct drm_encoder *drm_encoder, enum dpu_intf_mode dpu_encoder_get_intf_mode(struct drm_encoder *encoder);

/** - * dpu_encoder_virt_restore - restore the encoder configs + * dpu_encoder_virt_runtime_resume - pm runtime resume the encoder configs * @encoder: encoder pointer */ -void dpu_encoder_virt_restore(struct drm_encoder *encoder); +void dpu_encoder_virt_runtime_resume(struct drm_encoder *encoder);

/** * dpu_encoder_init - initialize virtual encoder object diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c index 9f7da56bb453..1bec4540f3e1 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c @@ -1129,7 +1129,7 @@ static int __maybe_unused dpu_runtime_resume(struct device *dev) int rc = -1; struct platform_device *pdev = to_platform_device(dev); struct dpu_kms *dpu_kms = platform_get_drvdata(pdev); - struct drm_crtc *crtc; + struct drm_encoder *encoder; struct drm_device *ddev; struct dss_module_power *mp = &dpu_kms->mp;

@@ -1147,8 +1147,8 @@ static int __maybe_unused dpu_runtime_resume(struct device *dev)

dpu_vbif_init_memtypes(dpu_kms);

- drm_for_each_crtc(crtc, ddev) - dpu_crtc_runtime_resume(crtc); + drm_for_each_encoder(encoder, ddev) + dpu_encoder_virt_runtime_resume(encoder);

return rc; }

From: Sean Paul seanpaul@chromium.org

Now that runtime resume is handled in encoder, we don't need to worry about crtc_lock recursion when calling pm_runtime_(get|put). So drop the lock drops in _dpu_crtc_vblank_enable_no_lock().

Changes in v2: - None

Signed-off-by: Sean Paul seanpaul@chromium.org --- drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 6 ------ 1 file changed, 6 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c index 9be24907f8c1..80de5289ada3 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c @@ -777,10 +777,7 @@ static void _dpu_crtc_vblank_enable_no_lock( struct drm_encoder *enc;

if (enable) { - /* drop lock since power crtc cb may try to re-acquire lock */ - mutex_unlock(&dpu_crtc->crtc_lock); pm_runtime_get_sync(dev->dev); - mutex_lock(&dpu_crtc->crtc_lock);

list_for_each_entry(enc, &dev->mode_config.encoder_list, head) { if (enc->crtc != crtc) @@ -805,10 +802,7 @@ static void _dpu_crtc_vblank_enable_no_lock( dpu_encoder_register_vblank_callback(enc, NULL, NULL); }

- /* drop lock since power crtc cb may try to re-acquire lock */ - mutex_unlock(&dpu_crtc->crtc_lock); pm_runtime_put_sync(dev->dev); - mutex_lock(&dpu_crtc->crtc_lock); } }

From: Sean Paul seanpaul@chromium.org

This patch wraps dpu_core_perf_crtc_release_bw() with modeset locks since it digs into the state objects.

Changes in v2: - None

Signed-off-by: Sean Paul seanpaul@chromium.org --- drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 2 ++ 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c index 80de5289ada3..156f4c77ca44 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c @@ -335,7 +335,9 @@ static void dpu_crtc_frame_event_work(struct kthread_work *work) /* release bandwidth and other resources */ trace_dpu_crtc_frame_event_done(DRMID(crtc), fevent->event); + drm_modeset_lock_all(crtc->dev); dpu_core_perf_crtc_release_bw(crtc); + drm_modeset_unlock_all(crtc->dev); } else { trace_dpu_crtc_frame_event_more_pending(DRMID(crtc), fevent->event);

From: Sean Paul seanpaul@chromium.org

It's for legacy drivers, for atomic drivers crtc->state->encoder_mask should be used to map encoder to crtc.

Changes in v2: - None

Signed-off-by: Sean Paul seanpaul@chromium.org --- drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 46 ++++++++---------------- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 19 +++++++--- 2 files changed, 29 insertions(+), 36 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c index 156f4c77ca44..a008a87a8113 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c @@ -284,9 +284,9 @@ enum dpu_intf_mode dpu_crtc_get_intf_mode(struct drm_crtc *crtc) return INTF_MODE_NONE; }

- drm_for_each_encoder(encoder, crtc->dev) - if (encoder->crtc == crtc) - return dpu_encoder_get_intf_mode(encoder); + /* TODO: Returns the first INTF_MODE, could there be multiple values? */ + drm_for_each_encoder_mask(encoder, crtc->dev, crtc->state->encoder_mask) + return dpu_encoder_get_intf_mode(encoder);

return INTF_MODE_NONE; } @@ -551,13 +551,9 @@ static void dpu_crtc_atomic_begin(struct drm_crtc *crtc, spin_unlock_irqrestore(&dev->event_lock, flags); }

- list_for_each_entry(encoder, &dev->mode_config.encoder_list, head) { - if (encoder->crtc != crtc) - continue; - - /* encoder will trigger pending mask now */ + /* encoder will trigger pending mask now */ + drm_for_each_encoder_mask(encoder, crtc->dev, crtc->state->encoder_mask) dpu_encoder_trigger_kickoff_pending(encoder); - }

/* * If no mixers have been allocated in dpu_crtc_atomic_check(), @@ -704,7 +700,6 @@ static int _dpu_crtc_wait_for_frame_done(struct drm_crtc *crtc) void dpu_crtc_commit_kickoff(struct drm_crtc *crtc) { struct drm_encoder *encoder; - struct drm_device *dev = crtc->dev; struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc); struct dpu_kms *dpu_kms = _dpu_crtc_get_kms(crtc); struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc->state); @@ -720,16 +715,13 @@ void dpu_crtc_commit_kickoff(struct drm_crtc *crtc)

DPU_ATRACE_BEGIN("crtc_commit");

- list_for_each_entry(encoder, &dev->mode_config.encoder_list, head) { + /* + * Encoder will flush/start now, unless it has a tx pending. If so, it + * may delay and flush at an irq event (e.g. ppdone) + */ + drm_for_each_encoder_mask(encoder, crtc->dev, + crtc->state->encoder_mask) { struct dpu_encoder_kickoff_params params = { 0 }; - - if (encoder->crtc != crtc) - continue; - - /* - * Encoder will flush/start now, unless it has a tx pending. - * If so, it may delay and flush at an irq event (e.g. ppdone) - */ dpu_encoder_prepare_for_kickoff(encoder, &params); }

@@ -754,12 +746,8 @@ void dpu_crtc_commit_kickoff(struct drm_crtc *crtc)

dpu_vbif_clear_errors(dpu_kms);

- list_for_each_entry(encoder, &dev->mode_config.encoder_list, head) { - if (encoder->crtc != crtc) - continue; - + drm_for_each_encoder_mask(encoder, crtc->dev, crtc->state->encoder_mask) dpu_encoder_kickoff(encoder); - }

end: reinit_completion(&dpu_crtc->frame_done_comp); @@ -883,11 +871,8 @@ static void dpu_crtc_disable(struct drm_crtc *crtc)

dpu_core_perf_crtc_update(crtc, 0, true);

- drm_for_each_encoder(encoder, crtc->dev) { - if (encoder->crtc != crtc) - continue; + drm_for_each_encoder_mask(encoder, crtc->dev, crtc->state->encoder_mask) dpu_encoder_register_frame_event_callback(encoder, NULL, NULL); - }

memset(cstate->mixers, 0, sizeof(cstate->mixers)); cstate->num_mixers = 0; @@ -922,12 +907,9 @@ static void dpu_crtc_enable(struct drm_crtc *crtc, DRM_DEBUG_KMS("crtc%d\n", crtc->base.id); dpu_crtc = to_dpu_crtc(crtc);

- drm_for_each_encoder(encoder, crtc->dev) { - if (encoder->crtc != crtc) - continue; + drm_for_each_encoder_mask(encoder, crtc->dev, crtc->state->encoder_mask) dpu_encoder_register_frame_event_callback(encoder, dpu_crtc_frame_event_cb, (void *)crtc); - }

mutex_lock(&dpu_crtc->crtc_lock); trace_dpu_crtc_enable(DRMID(crtc), true, dpu_crtc); diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c index 1bec4540f3e1..64134d619748 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c @@ -320,7 +320,10 @@ static void dpu_kms_prepare_commit(struct msm_kms *kms, struct dpu_kms *dpu_kms; struct msm_drm_private *priv; struct drm_device *dev; + struct drm_crtc *crtc; + struct drm_crtc_state *crtc_state; struct drm_encoder *encoder; + int i;

if (!kms) return; @@ -332,9 +335,13 @@ static void dpu_kms_prepare_commit(struct msm_kms *kms, priv = dev->dev_private; pm_runtime_get_sync(&dpu_kms->pdev->dev);

- list_for_each_entry(encoder, &dev->mode_config.encoder_list, head) - if (encoder->crtc != NULL) + /* Call prepare_commit for all affected encoders */ + for_each_new_crtc_in_state(state, crtc, crtc_state, i) { + drm_for_each_encoder_mask(encoder, crtc->dev, + crtc_state->encoder_mask) { dpu_encoder_prepare_commit(encoder); + } + } }

/* @@ -344,13 +351,17 @@ static void dpu_kms_prepare_commit(struct msm_kms *kms, void dpu_kms_encoder_enable(struct drm_encoder *encoder) { const struct drm_encoder_helper_funcs *funcs = encoder->helper_private; - struct drm_crtc *crtc = encoder->crtc; + struct drm_device *dev = encoder->dev; + struct drm_crtc *crtc;

/* Forward this enable call to the commit hook */ if (funcs && funcs->commit) funcs->commit(encoder);

- if (crtc && crtc->state->active) { + drm_for_each_crtc(crtc, dev) { + if (!(crtc->state->encoder_mask & drm_encoder_mask(encoder))) + continue; + trace_dpu_kms_enc_enable(DRMID(crtc)); dpu_crtc_commit_kickoff(crtc); }

From: Sean Paul seanpaul@chromium.org

Add modeset lock checks to functions that could be called outside the core atomic stack.

Changes in v2: - None

Signed-off-by: Sean Paul seanpaul@chromium.org --- drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 2 ++ drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 1 + 2 files changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c index a008a87a8113..cd0a0bea4335 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c @@ -284,6 +284,8 @@ enum dpu_intf_mode dpu_crtc_get_intf_mode(struct drm_crtc *crtc) return INTF_MODE_NONE; }

+ WARN_ON(!drm_modeset_is_locked(&crtc->mutex)); + /* TODO: Returns the first INTF_MODE, could there be multiple values? */ drm_for_each_encoder_mask(encoder, crtc->dev, crtc->state->encoder_mask) return dpu_encoder_get_intf_mode(encoder); diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c index 64134d619748..5104fc01147e 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c @@ -358,6 +358,7 @@ void dpu_kms_encoder_enable(struct drm_encoder *encoder) if (funcs && funcs->commit) funcs->commit(encoder);

+ WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex)); drm_for_each_crtc(crtc, dev) { if (!(crtc->state->encoder_mask & drm_encoder_mask(encoder))) continue;

From: Sean Paul seanpaul@chromium.org

There are 4 times that _dpu_crtc_vblank_enable_no_lock() is called:

1- crtc enable 2- crtc disable 3- crtc vblank enable 4- crtc vblank disable

When we enable or disable the crtc, we call drm_crtc_vblank_on and drm_crtc_vblank_off respectively. That will gate vblank enables and disables to only being called when the crtc is active. That means that we can just enable/disable pm runtime in crtc enable/disable. This will be beneficial in trying to eliminate blocking calls from the vblank call chain.

Changes in v2: - None

Signed-off-by: Sean Paul seanpaul@chromium.org --- drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c index cd0a0bea4335..c49aaf412b6e 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c @@ -769,8 +769,6 @@ static void _dpu_crtc_vblank_enable_no_lock( struct drm_encoder *enc;

if (enable) { - pm_runtime_get_sync(dev->dev); - list_for_each_entry(enc, &dev->mode_config.encoder_list, head) { if (enc->crtc != crtc) continue; @@ -793,8 +791,6 @@ static void _dpu_crtc_vblank_enable_no_lock(

dpu_encoder_register_vblank_callback(enc, NULL, NULL); } - - pm_runtime_put_sync(dev->dev); } }

@@ -891,6 +887,8 @@ static void dpu_crtc_disable(struct drm_crtc *crtc) crtc->state->event = NULL; spin_unlock_irqrestore(&crtc->dev->event_lock, flags); } + + pm_runtime_put_sync(crtc->dev->dev); }

static void dpu_crtc_enable(struct drm_crtc *crtc, @@ -906,6 +904,8 @@ static void dpu_crtc_enable(struct drm_crtc *crtc, } priv = crtc->dev->dev_private;

+ pm_runtime_get_sync(crtc->dev->dev); + DRM_DEBUG_KMS("crtc%d\n", crtc->base.id); dpu_crtc = to_dpu_crtc(crtc);

From: Sean Paul seanpaul@chromium.org

I think the intention here was to protect the enc->crtc access, but that's insufficient to avoid enc->crtc changing. Fortunately we're already holding the modeset lock when this is called (from atomic_check), so remove the crtc_lock and add a modeset lock check.

While we're at it, use the encoder mask from crtc state instead of legacy pointer.

Changes in v2: - None

Signed-off-by: Sean Paul seanpaul@chromium.org --- drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c index c49aaf412b6e..141ed1b0e90a 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c @@ -474,19 +474,13 @@ static void _dpu_crtc_setup_mixer_for_encoder(

static void _dpu_crtc_setup_mixers(struct drm_crtc *crtc) { - struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc); struct drm_encoder *enc;

- mutex_lock(&dpu_crtc->crtc_lock); - /* Check for mixers on all encoders attached to this crtc */ - list_for_each_entry(enc, &crtc->dev->mode_config.encoder_list, head) { - if (enc->crtc != crtc) - continue; + WARN_ON(!drm_modeset_is_locked(&crtc->mutex));

+ /* Check for mixers on all encoders attached to this crtc */ + drm_for_each_encoder_mask(enc, crtc->dev, crtc->state->encoder_mask) _dpu_crtc_setup_mixer_for_encoder(crtc, enc); - } - - mutex_unlock(&dpu_crtc->crtc_lock); }

static void _dpu_crtc_setup_lm_bounds(struct drm_crtc *crtc,

From: Sean Paul seanpaul@chromium.org

The indirection of registering a callback and opaque pointer isn't reall useful when there's only one callsite. So instead of having the vblank_cb registration, just give encoder a crtc and let it directly call the vblank handler.

In a later patch, we'll make use of this further.

Changes in v2: - None

Cc: Jeykumar Sankaran jsanka@codeaurora.org Signed-off-by: Sean Paul seanpaul@chromium.org --- drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 8 +++---- drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h | 6 +++++ drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 25 +++++++++++---------- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 10 ++++----- 4 files changed, 26 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c index 141ed1b0e90a..4d7f9ff1e9f4 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c @@ -293,9 +293,8 @@ enum dpu_intf_mode dpu_crtc_get_intf_mode(struct drm_crtc *crtc) return INTF_MODE_NONE; }

-static void dpu_crtc_vblank_cb(void *data) +void dpu_crtc_vblank_callback(struct drm_crtc *crtc) { - struct drm_crtc *crtc = (struct drm_crtc *)data; struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);

/* keep statistics on vblank callback - with auto reset via debugfs */ @@ -771,8 +770,7 @@ static void _dpu_crtc_vblank_enable_no_lock( DRMID(enc), enable, dpu_crtc);

- dpu_encoder_register_vblank_callback(enc, - dpu_crtc_vblank_cb, (void *)crtc); + dpu_encoder_assign_crtc(enc, crtc); } } else { list_for_each_entry(enc, &dev->mode_config.encoder_list, head) { @@ -783,7 +781,7 @@ static void _dpu_crtc_vblank_enable_no_lock( DRMID(enc), enable, dpu_crtc);

- dpu_encoder_register_vblank_callback(enc, NULL, NULL); + dpu_encoder_assign_crtc(enc, NULL); } } } diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h index 93d21a61a040..54595cc29be5 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h @@ -270,6 +270,12 @@ static inline int dpu_crtc_frame_pending(struct drm_crtc *crtc) */ int dpu_crtc_vblank(struct drm_crtc *crtc, bool en);

+/** + * dpu_crtc_vblank_callback - called on vblank irq, issues completion events + * @crtc: Pointer to drm crtc object + */ +void dpu_crtc_vblank_callback(struct drm_crtc *crtc); + /** * dpu_crtc_commit_kickoff - trigger kickoff of the commit for this crtc * @crtc: Pointer to drm crtc object diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c index d89ac520f7e6..fd6514f681ae 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c @@ -142,9 +142,11 @@ enum dpu_enc_rc_states { * @intfs_swapped Whether or not the phys_enc interfaces have been swapped * for partial update right-only cases, such as pingpong * split where virtual pingpong does not generate IRQs - * @crtc_vblank_cb: Callback into the upper layer / CRTC for - * notification of the VBLANK - * @crtc_vblank_cb_data: Data from upper layer for VBLANK notification + * @crtc: Pointer to the currently assigned crtc. Normally you + * would use crtc->state->encoder_mask to determine the + * link between encoder/crtc. However in this case we need + * to track crtc in the disable() hook which is called + * _after_ encoder_mask is cleared. * @crtc_kickoff_cb: Callback into CRTC that will flush & start * all CTL paths * @crtc_kickoff_cb_data: Opaque user data given to crtc_kickoff_cb @@ -186,8 +188,7 @@ struct dpu_encoder_virt {

bool intfs_swapped;

- void (*crtc_vblank_cb)(void *); - void *crtc_vblank_cb_data; + struct drm_crtc *crtc;

struct dentry *debugfs_root; struct mutex enc_lock; @@ -1241,8 +1242,8 @@ static void dpu_encoder_vblank_callback(struct drm_encoder *drm_enc, dpu_enc = to_dpu_encoder_virt(drm_enc);

spin_lock_irqsave(&dpu_enc->enc_spinlock, lock_flags); - if (dpu_enc->crtc_vblank_cb) - dpu_enc->crtc_vblank_cb(dpu_enc->crtc_vblank_cb_data); + if (dpu_enc->crtc) + dpu_crtc_vblank_callback(dpu_enc->crtc); spin_unlock_irqrestore(&dpu_enc->enc_spinlock, lock_flags);

atomic_inc(&phy_enc->vsync_cnt); @@ -1262,15 +1263,14 @@ static void dpu_encoder_underrun_callback(struct drm_encoder *drm_enc, DPU_ATRACE_END("encoder_underrun_callback"); }

-void dpu_encoder_register_vblank_callback(struct drm_encoder *drm_enc, - void (*vbl_cb)(void *), void *vbl_data) +void dpu_encoder_assign_crtc(struct drm_encoder *drm_enc, struct drm_crtc *crtc) { struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc); unsigned long lock_flags; bool enable; int i;

- enable = vbl_cb ? true : false; + enable = crtc ? true : false;

if (!drm_enc) { DPU_ERROR("invalid encoder\n"); @@ -1279,8 +1279,9 @@ void dpu_encoder_register_vblank_callback(struct drm_encoder *drm_enc, trace_dpu_enc_vblank_cb(DRMID(drm_enc), enable);

spin_lock_irqsave(&dpu_enc->enc_spinlock, lock_flags); - dpu_enc->crtc_vblank_cb = vbl_cb; - dpu_enc->crtc_vblank_cb_data = vbl_data; + /* crtc should always be cleared before re-assigning */ + WARN_ON(crtc && dpu_enc->crtc); + dpu_enc->crtc = crtc; spin_unlock_irqrestore(&dpu_enc->enc_spinlock, lock_flags);

for (i = 0; i < dpu_enc->num_phys_encs; i++) { diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h index aa4f135218fa..be1d80867834 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h @@ -55,14 +55,12 @@ void dpu_encoder_get_hw_resources(struct drm_encoder *encoder, struct dpu_encoder_hw_resources *hw_res);

/** - * dpu_encoder_register_vblank_callback - provide callback to encoder that - * will be called on the next vblank. + * dpu_encoder_assign_crtc - Link the encoder to the crtc it's assigned to * @encoder: encoder pointer - * @cb: callback pointer, provide NULL to deregister and disable IRQs - * @data: user data provided to callback + * @crtc: crtc pointer */ -void dpu_encoder_register_vblank_callback(struct drm_encoder *encoder, - void (*cb)(void *), void *data); +void dpu_encoder_assign_crtc(struct drm_encoder *encoder, + struct drm_crtc *crtc);

/** * dpu_encoder_register_frame_event_callback - provide callback to encoder that

From: Sean Paul seanpaul@chromium.org

Matches dpu_crtc_enable and we'll need the old state in a future patch

Changes in v2: - None

Signed-off-by: Sean Paul seanpaul@chromium.org --- drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c index 4d7f9ff1e9f4..9efb41c7973b 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c @@ -815,7 +815,8 @@ static struct drm_crtc_state *dpu_crtc_duplicate_state(struct drm_crtc *crtc) return &cstate->base; }

-static void dpu_crtc_disable(struct drm_crtc *crtc) +static void dpu_crtc_disable(struct drm_crtc *crtc, + struct drm_crtc_state *old_crtc_state) { struct dpu_crtc *dpu_crtc; struct dpu_crtc_state *cstate; @@ -1407,7 +1408,7 @@ static const struct drm_crtc_funcs dpu_crtc_funcs = { };

static const struct drm_crtc_helper_funcs dpu_crtc_helper_funcs = { - .disable = dpu_crtc_disable, + .atomic_disable = dpu_crtc_disable, .atomic_enable = dpu_crtc_enable, .atomic_check = dpu_crtc_atomic_check, .atomic_begin = dpu_crtc_atomic_begin,

From: Sean Paul seanpaul@chromium.org

The drm_crtc_vblank_on/off calls in enable/disable guarantee that we won't call this function when crtc is not enabled.

Changes in v2: - None

Signed-off-by: Sean Paul seanpaul@chromium.org --- drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c index 9efb41c7973b..54bb777b2d0c 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c @@ -1162,9 +1162,7 @@ int dpu_crtc_vblank(struct drm_crtc *crtc, bool en)

mutex_lock(&dpu_crtc->crtc_lock); trace_dpu_crtc_vblank(DRMID(&dpu_crtc->base), en, dpu_crtc); - if (dpu_crtc->enabled) { - _dpu_crtc_vblank_enable_no_lock(dpu_crtc, en); - } + _dpu_crtc_vblank_enable_no_lock(dpu_crtc, en); dpu_crtc->vblank_requested = en; mutex_unlock(&dpu_crtc->crtc_lock);

From: Sean Paul seanpaul@chromium.org

Instead of assigning/clearing the crtc on vblank enable/disable, we can just assign and clear the crtc on modeset. That allows us to just toggle the encoder's vblank interrupts on vblank_enable.

So why is this important? Previously the driver was using the legacy pointers to assign/clear the crtc. Legacy pointers are cleared _after_ disabling the hardware, so the legacy pointer was valid during vblank_disable, but that's not something we should rely on.

Instead of relying on the core ordering the legacy pointer assignments just so, we'll assign the crtc in dpu_crtc enable/disable. This is the only place that mapping can change, so we're covered there.

We're also taking advantage of drm_crtc_vblank_on/off. By using this, we ensure that vblank_enable/disable can never be called while the crtc is off (which means the assigned crtc will always be valid). As such, we don't need to use modeset locks or the crtc_lock in the vblank_enable/disable routine to be sure state is consistent.

...I think.

Changes in v2: - Changed crtc check in toggle_vblank to != (Jeykumar)

Cc: Jeykumar Sankaran jsanka@codeaurora.org Signed-off-by: Sean Paul seanpaul@chromium.org --- drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 77 +++++++++------------ drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 27 +++++--- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 10 +++ 3 files changed, 59 insertions(+), 55 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c index 54bb777b2d0c..725e15178cdb 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c @@ -749,43 +749,6 @@ void dpu_crtc_commit_kickoff(struct drm_crtc *crtc) DPU_ATRACE_END("crtc_commit"); }

-/** - * _dpu_crtc_vblank_enable_no_lock - update power resource and vblank request - * @dpu_crtc: Pointer to dpu crtc structure - * @enable: Whether to enable/disable vblanks - */ -static void _dpu_crtc_vblank_enable_no_lock( - struct dpu_crtc *dpu_crtc, bool enable) -{ - struct drm_crtc *crtc = &dpu_crtc->base; - struct drm_device *dev = crtc->dev; - struct drm_encoder *enc; - - if (enable) { - list_for_each_entry(enc, &dev->mode_config.encoder_list, head) { - if (enc->crtc != crtc) - continue; - - trace_dpu_crtc_vblank_enable(DRMID(&dpu_crtc->base), - DRMID(enc), enable, - dpu_crtc); - - dpu_encoder_assign_crtc(enc, crtc); - } - } else { - list_for_each_entry(enc, &dev->mode_config.encoder_list, head) { - if (enc->crtc != crtc) - continue; - - trace_dpu_crtc_vblank_enable(DRMID(&dpu_crtc->base), - DRMID(enc), enable, - dpu_crtc); - - dpu_encoder_assign_crtc(enc, NULL); - } - } -} - /** * dpu_crtc_duplicate_state - state duplicate hook * @crtc: Pointer to drm crtc structure @@ -839,6 +802,10 @@ static void dpu_crtc_disable(struct drm_crtc *crtc, /* Disable/save vblank irq handling */ drm_crtc_vblank_off(crtc);

+ drm_for_each_encoder_mask(encoder, crtc->dev, + old_crtc_state->encoder_mask) + dpu_encoder_assign_crtc(encoder, NULL); + mutex_lock(&dpu_crtc->crtc_lock);

/* wait for frame_event_done completion */ @@ -848,9 +815,6 @@ static void dpu_crtc_disable(struct drm_crtc *crtc, atomic_read(&dpu_crtc->frame_pending));

trace_dpu_crtc_disable(DRMID(crtc), false, dpu_crtc); - if (dpu_crtc->enabled && dpu_crtc->vblank_requested) { - _dpu_crtc_vblank_enable_no_lock(dpu_crtc, false); - } dpu_crtc->enabled = false;

if (atomic_read(&dpu_crtc->frame_pending)) { @@ -908,13 +872,13 @@ static void dpu_crtc_enable(struct drm_crtc *crtc,

mutex_lock(&dpu_crtc->crtc_lock); trace_dpu_crtc_enable(DRMID(crtc), true, dpu_crtc); - if (!dpu_crtc->enabled && dpu_crtc->vblank_requested) { - _dpu_crtc_vblank_enable_no_lock(dpu_crtc, true); - } dpu_crtc->enabled = true;

mutex_unlock(&dpu_crtc->crtc_lock);

+ drm_for_each_encoder_mask(encoder, crtc->dev, crtc->state->encoder_mask) + dpu_encoder_assign_crtc(encoder, crtc); + /* Enable/restore vblank irq handling */ drm_crtc_vblank_on(crtc); } @@ -1159,10 +1123,33 @@ static int dpu_crtc_atomic_check(struct drm_crtc *crtc, int dpu_crtc_vblank(struct drm_crtc *crtc, bool en) { struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc); + struct drm_encoder *enc;

- mutex_lock(&dpu_crtc->crtc_lock); trace_dpu_crtc_vblank(DRMID(&dpu_crtc->base), en, dpu_crtc); - _dpu_crtc_vblank_enable_no_lock(dpu_crtc, en); + + /* + * Normally we would iterate through encoder_mask in crtc state to find + * attached encoders. In this case, we might be disabling vblank _after_ + * encoder_mask has been cleared. + * + * Instead, we "assign" a crtc to the encoder in enable and clear it in + * disable (which is also after encoder_mask is cleared). So instead of + * using encoder mask, we'll ask the encoder to toggle itself iff it's + * currently assigned to our crtc. + * + * Note also that this function cannot be called while crtc is disabled + * since we use drm_crtc_vblank_on/off. So we don't need to worry + * about the assigned crtcs being inconsistent with the current state + * (which means no need to worry about modeset locks). + */ + list_for_each_entry(enc, &crtc->dev->mode_config.encoder_list, head) { + trace_dpu_crtc_vblank_enable(DRMID(crtc), DRMID(enc), en, + dpu_crtc); + + dpu_encoder_toggle_vblank_for_crtc(enc, crtc, en); + } + + mutex_lock(&dpu_crtc->crtc_lock); dpu_crtc->vblank_requested = en; mutex_unlock(&dpu_crtc->crtc_lock);

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c index fd6514f681ae..90b77d98dde8 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c @@ -1267,22 +1267,29 @@ void dpu_encoder_assign_crtc(struct drm_encoder *drm_enc, struct drm_crtc *crtc) { struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc); unsigned long lock_flags; - bool enable; - int i; - - enable = crtc ? true : false; - - if (!drm_enc) { - DPU_ERROR("invalid encoder\n"); - return; - } - trace_dpu_enc_vblank_cb(DRMID(drm_enc), enable);

spin_lock_irqsave(&dpu_enc->enc_spinlock, lock_flags); /* crtc should always be cleared before re-assigning */ WARN_ON(crtc && dpu_enc->crtc); dpu_enc->crtc = crtc; spin_unlock_irqrestore(&dpu_enc->enc_spinlock, lock_flags); +} + +void dpu_encoder_toggle_vblank_for_crtc(struct drm_encoder *drm_enc, + struct drm_crtc *crtc, bool enable) +{ + struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc); + unsigned long lock_flags; + int i; + + trace_dpu_enc_vblank_cb(DRMID(drm_enc), enable); + + spin_lock_irqsave(&dpu_enc->enc_spinlock, lock_flags); + if (dpu_enc->crtc != crtc) { + spin_unlock_irqrestore(&dpu_enc->enc_spinlock, lock_flags); + return; + } + spin_unlock_irqrestore(&dpu_enc->enc_spinlock, lock_flags);

for (i = 0; i < dpu_enc->num_phys_encs; i++) { struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i]; diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h index be1d80867834..6896ea2ab854 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h @@ -62,6 +62,16 @@ void dpu_encoder_get_hw_resources(struct drm_encoder *encoder, void dpu_encoder_assign_crtc(struct drm_encoder *encoder, struct drm_crtc *crtc);

+/** + * dpu_encoder_toggle_vblank_for_crtc - Toggles vblank interrupts on or off if + * the encoder is assigned to the given crtc + * @encoder: encoder pointer + * @crtc: crtc pointer + * @enable: true if vblank should be enabled + */ +void dpu_encoder_toggle_vblank_for_crtc(struct drm_encoder *encoder, + struct drm_crtc *crtc, bool enable); + /** * dpu_encoder_register_frame_event_callback - provide callback to encoder that * will be called after the request is complete, or other events.

From: Sean Paul seanpaul@chromium.org

It's just for debugfs output, we don't need it

Changes in v2: - None

Cc: Jeykumar Sankaran jsanka@codeaurora.org Signed-off-by: Sean Paul seanpaul@chromium.org --- drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 6 ------ drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h | 2 -- drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h | 14 ++++---------- 3 files changed, 4 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c index 725e15178cdb..f15cba2584a0 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c @@ -1149,10 +1149,6 @@ int dpu_crtc_vblank(struct drm_crtc *crtc, bool en) dpu_encoder_toggle_vblank_for_crtc(enc, crtc, en); }

- mutex_lock(&dpu_crtc->crtc_lock); - dpu_crtc->vblank_requested = en; - mutex_unlock(&dpu_crtc->crtc_lock); - return 0; }

@@ -1268,8 +1264,6 @@ static int _dpu_debugfs_status_show(struct seq_file *s, void *data) dpu_crtc->vblank_cb_time = ktime_set(0, 0); }

- seq_printf(s, "vblank_enable:%d\n", dpu_crtc->vblank_requested); - mutex_unlock(&dpu_crtc->crtc_lock); drm_modeset_unlock_all(crtc->dev);

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h index 54595cc29be5..2b358546af49 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h @@ -132,7 +132,6 @@ struct dpu_crtc_frame_event { * @vblank_cb_count : count of vblank callback since last reset * @play_count : frame count between crtc enable and disable * @vblank_cb_time : ktime at vblank count reset - * @vblank_requested : whether the user has requested vblank events * @enabled : whether the DPU CRTC is currently enabled. updated in the * commit-thread, not state-swap time which is earlier, so * safe to make decisions on during VBLANK on/off work @@ -166,7 +165,6 @@ struct dpu_crtc { u32 vblank_cb_count; u64 play_count; ktime_t vblank_cb_time; - bool vblank_requested; bool enabled;

struct list_head feature_list; diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h index 328df37d7580..c78b521ceda1 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h @@ -728,20 +728,17 @@ TRACE_EVENT(dpu_crtc_vblank_enable, __field( uint32_t, enc_id ) __field( bool, enable ) __field( bool, enabled ) - __field( bool, vblank_requested ) ), TP_fast_assign( __entry->drm_id = drm_id; __entry->enc_id = enc_id; __entry->enable = enable; __entry->enabled = crtc->enabled; - __entry->vblank_requested = crtc->vblank_requested; ), - TP_printk("id:%u encoder:%u enable:%s state{enabled:%s vblank_req:%s}", + TP_printk("id:%u encoder:%u enable:%s state{enabled:%s}", __entry->drm_id, __entry->enc_id, __entry->enable ? "true" : "false", - __entry->enabled ? "true" : "false", - __entry->vblank_requested ? "true" : "false") + __entry->enabled ? "true" : "false") );

DECLARE_EVENT_CLASS(dpu_crtc_enable_template, @@ -751,18 +748,15 @@ DECLARE_EVENT_CLASS(dpu_crtc_enable_template, __field( uint32_t, drm_id ) __field( bool, enable ) __field( bool, enabled ) - __field( bool, vblank_requested ) ), TP_fast_assign( __entry->drm_id = drm_id; __entry->enable = enable; __entry->enabled = crtc->enabled; - __entry->vblank_requested = crtc->vblank_requested; ), - TP_printk("id:%u enable:%s state{enabled:%s vblank_req:%s}", + TP_printk("id:%u enable:%s state{enabled:%s}", __entry->drm_id, __entry->enable ? "true" : "false", - __entry->enabled ? "true" : "false", - __entry->vblank_requested ? "true" : "false") + __entry->enabled ? "true" : "false") ); DEFINE_EVENT(dpu_crtc_enable_template, dpu_crtc_enable, TP_PROTO(uint32_t drm_id, bool enable, struct dpu_crtc *crtc),

From: Sean Paul seanpaul@chromium.org

Each time it's called we're holding the crtc modeset lock, so it's redundant.

Changes in v2: - None

Signed-off-by: Sean Paul seanpaul@chromium.org --- drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 11 ----------- drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h | 3 --- 2 files changed, 14 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c index f15cba2584a0..70b5104d1111 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c @@ -69,7 +69,6 @@ static void dpu_crtc_destroy(struct drm_crtc *crtc) return;

drm_crtc_cleanup(crtc); - mutex_destroy(&dpu_crtc->crtc_lock); kfree(dpu_crtc); }

@@ -806,8 +805,6 @@ static void dpu_crtc_disable(struct drm_crtc *crtc, old_crtc_state->encoder_mask) dpu_encoder_assign_crtc(encoder, NULL);

- mutex_lock(&dpu_crtc->crtc_lock); - /* wait for frame_event_done completion */ if (_dpu_crtc_wait_for_frame_done(crtc)) DPU_ERROR("crtc%d wait for frame done failed;frame_pending%d\n", @@ -836,8 +833,6 @@ static void dpu_crtc_disable(struct drm_crtc *crtc, cstate->bw_control = false; cstate->bw_split_vote = false;

- mutex_unlock(&dpu_crtc->crtc_lock); - if (crtc->state->event && !crtc->state->active) { spin_lock_irqsave(&crtc->dev->event_lock, flags); drm_crtc_send_vblank_event(crtc, crtc->state->event); @@ -870,12 +865,9 @@ static void dpu_crtc_enable(struct drm_crtc *crtc, dpu_encoder_register_frame_event_callback(encoder, dpu_crtc_frame_event_cb, (void *)crtc);

- mutex_lock(&dpu_crtc->crtc_lock); trace_dpu_crtc_enable(DRMID(crtc), true, dpu_crtc); dpu_crtc->enabled = true;

- mutex_unlock(&dpu_crtc->crtc_lock); - drm_for_each_encoder_mask(encoder, crtc->dev, crtc->state->encoder_mask) dpu_encoder_assign_crtc(encoder, crtc);

@@ -1177,7 +1169,6 @@ static int _dpu_debugfs_status_show(struct seq_file *s, void *data) drm_modeset_lock_all(crtc->dev); cstate = to_dpu_crtc_state(crtc->state);

- mutex_lock(&dpu_crtc->crtc_lock); mode = &crtc->state->adjusted_mode; out_width = _dpu_crtc_get_mixer_width(cstate, mode);

@@ -1264,7 +1255,6 @@ static int _dpu_debugfs_status_show(struct seq_file *s, void *data) dpu_crtc->vblank_cb_time = ktime_set(0, 0); }

- mutex_unlock(&dpu_crtc->crtc_lock); drm_modeset_unlock_all(crtc->dev);

return 0; @@ -1414,7 +1404,6 @@ struct drm_crtc *dpu_crtc_init(struct drm_device *dev, struct drm_plane *plane, crtc = &dpu_crtc->base; crtc->dev = dev;

- mutex_init(&dpu_crtc->crtc_lock); spin_lock_init(&dpu_crtc->spin_lock); atomic_set(&dpu_crtc->frame_pending, 0);

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h index 2b358546af49..34f0c4d4d774 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h @@ -140,7 +140,6 @@ struct dpu_crtc_frame_event { * @dirty_list : list of color processing features are dirty * @ad_dirty: list containing ad properties that are dirty * @ad_active: list containing ad properties that are active - * @crtc_lock : crtc lock around create, destroy and access. * @frame_pending : Whether or not an update is pending * @frame_events : static allocation of in-flight frame events * @frame_event_list : available frame event list @@ -173,8 +172,6 @@ struct dpu_crtc { struct list_head ad_dirty; struct list_head ad_active;

- struct mutex crtc_lock; - atomic_t frame_pending; struct dpu_crtc_frame_event frame_events[DPU_CRTC_FRAME_EVENT_SIZE]; struct list_head frame_event_list;