Re: [Freedreno] [PATCH 07/25] drm/msm/dpu: reserve using crtc state
On 2018-10-09 14:06, Sean Paul wrote: On Mon, Oct 08, 2018 at 09:27:24PM -0700, Jeykumar Sankaran wrote: DPU maintained reservation lists to cache assigned HW blocks for the display and a retrieval mechanism for the individual DRM components to query their respective HW blocks. This patch uses the sub-classed CRTC state to store and track HW blocks assigned for different components of the display pipeline. It helps the driver: - to get rid of unwanted store and retrieval RM API's - to preserve HW resources assigned in atomic_check through atomic swap/duplicate. Separate patch is submitted to remove resource reservation in atomic_commit path. Signed-off-by: Jeykumar Sankaran --- drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 65 +++--- drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h | 14 + drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c| 28 +++--- .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 20 ++- drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 58 --- 5 files changed, 72 insertions(+), 113 deletions(-) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c index 4960641..0625f56 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c @@ -421,69 +421,20 @@ void dpu_crtc_complete_commit(struct drm_crtc *crtc, trace_dpu_crtc_complete_commit(DRMID(crtc)); } -static void _dpu_crtc_setup_mixer_for_encoder( - struct drm_crtc *crtc, - struct drm_encoder *enc) +static void _dpu_crtc_setup_mixers(struct drm_crtc *crtc) { struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc->state); - struct dpu_kms *dpu_kms = _dpu_crtc_get_kms(crtc); - struct dpu_rm *rm = &dpu_kms->rm; struct dpu_crtc_mixer *mixer; - struct dpu_hw_ctl *last_valid_ctl = NULL; - int i; - struct dpu_rm_hw_iter lm_iter, ctl_iter; - - dpu_rm_init_hw_iter(&lm_iter, enc->base.id, DPU_HW_BLK_LM); - dpu_rm_init_hw_iter(&ctl_iter, enc->base.id, DPU_HW_BLK_CTL); + int i, ctl_index; /* Set up all the mixers and ctls reserved by this encoder */ - for (i = cstate->num_mixers; i < ARRAY_SIZE(cstate->mixers); i++) { + for (i = 0; i < cstate->num_mixers; i++) { mixer = &cstate->mixers[i]; - if (!dpu_rm_get_hw(rm, &lm_iter)) - break; - mixer->hw_lm = (struct dpu_hw_mixer *)lm_iter.hw; - /* CTL may be <= LMs, if <, multiple LMs controlled by 1 CTL */ - if (!dpu_rm_get_hw(rm, &ctl_iter)) { - DPU_DEBUG("no ctl assigned to lm %d, using previous\n", - mixer->hw_lm->idx - LM_0); - mixer->lm_ctl = last_valid_ctl; - } else { - mixer->lm_ctl = (struct dpu_hw_ctl *)ctl_iter.hw; - last_valid_ctl = mixer->lm_ctl; - } - - /* Shouldn't happen, mixers are always >= ctls */ - if (!mixer->lm_ctl) { - DPU_ERROR("no valid ctls found for lm %d\n", - mixer->hw_lm->idx - LM_0); - return; - } - - cstate->num_mixers++; - DPU_DEBUG("setup mixer %d: lm %d\n", - i, mixer->hw_lm->idx - LM_0); - DPU_DEBUG("setup mixer %d: ctl %d\n", - i, mixer->lm_ctl->idx - CTL_0); - } -} - -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; - - _dpu_crtc_setup_mixer_for_encoder(crtc, enc); + ctl_index = min(i, cstate->num_ctls - 1); This is another one of those places I mentioned where we're just assuming a value is going to be in a certain range. If num_ctls/num_intfs/num_phys_encs (all the same value afaict) is 0, we end up in a bad place. Even though all these variables have the same value, they are representing the sizes of logically seperate components. At a minimum, there should be a WARN_ON/BUG_ON somewhere ensuring this can never drop below 0. Isn't RM guaranteeing that? I can add the WARN_ON checks on these num_xxx when the HW blocks are allocated. Thanks, Jeykumar S. + mixer->lm_ctl = cstate->hw_ctls[ctl_index]; } - - mutex_unlock(&dpu_crtc->crtc_lock); } static void _dpu_crtc_setup_lm_bounds(struct drm_crtc *crtc, @@ -536,10 +487,8 @@ static void dpu_crtc_atomic_begin(struct drm_crtc *cr
Re: [Freedreno] [PATCH 22/25] drm/msm/dpu: make crtc and encoder specific HW reservation
On 2018-10-09 13:41, Sean Paul wrote: On Mon, Oct 08, 2018 at 09:27:39PM -0700, Jeykumar Sankaran wrote: Instead of letting encoder make a centralized reservation for all of its display DRM components, this path splits the responsibility between CRTC and Encoder, each requesting RM for the HW mapping of its own domain. Signed-off-by: Jeykumar Sankaran --- drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c| 31 + drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 14 ++ drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 69 - drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h | 36 +++ 4 files changed, 119 insertions(+), 31 deletions(-) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c index 0625f56..0536b8a 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c @@ -47,6 +47,8 @@ #define LEFT_MIXER 0 #define RIGHT_MIXER 1 +#define MAX_VDISPLAY_SPLIT 1080 + static inline int _dpu_crtc_get_mixer_width(struct dpu_crtc_state *cstate, struct drm_display_mode *mode) { @@ -448,6 +450,7 @@ static void _dpu_crtc_setup_lm_bounds(struct drm_crtc *crtc, for (i = 0; i < cstate->num_mixers; i++) { struct drm_rect *r = &cstate->lm_bounds[i]; + r->x1 = crtc_split_width * i; r->y1 = 0; r->x2 = r->x1 + crtc_split_width; @@ -885,6 +888,7 @@ static void dpu_crtc_disable(struct drm_crtc *crtc) struct drm_display_mode *mode; struct drm_encoder *encoder; struct msm_drm_private *priv; + struct dpu_kms *dpu_kms; unsigned long flags; if (!crtc || !crtc->dev || !crtc->dev->dev_private || !crtc->state) { @@ -895,6 +899,7 @@ static void dpu_crtc_disable(struct drm_crtc *crtc) cstate = to_dpu_crtc_state(crtc->state); mode = &cstate->base.adjusted_mode; priv = crtc->dev->dev_private; + dpu_kms = to_dpu_kms(priv->kms); DRM_DEBUG_KMS("crtc%d\n", crtc->base.id); @@ -953,6 +958,8 @@ static void dpu_crtc_disable(struct drm_crtc *crtc) crtc->state->event = NULL; spin_unlock_irqrestore(&crtc->dev->event_lock, flags); } + + dpu_rm_crtc_release(&dpu_kms->rm, crtc->state); } static void dpu_crtc_enable(struct drm_crtc *crtc, @@ -1004,6 +1011,21 @@ struct plane_state { u32 pipe_id; }; +static void _dpu_crtc_get_topology( + struct drm_crtc_state *crtc_state, + struct drm_display_mode *mode) +{ + struct dpu_crtc_state *dpu_cstate = to_dpu_crtc_state(crtc_state); + + dpu_cstate->num_mixers = (mode->vdisplay > MAX_VDISPLAY_SPLIT) ? 2 : 1; + + /** +* encoder->atomic_check is invoked before crtc->atomic_check. +* so dpu_cstate->num_intfs should have a non-zero value. +*/ + dpu_cstate->num_ctls = dpu_cstate->num_intfs; Why do we need num_ctls? Can't we just use dpu_cstate->num_intfs directly? Also, you don't really need these in their own function, especially if num_ctls goes away. Yes. I can live with just that. But since dpu_cstate maintains HW arrays for each type, I thought it would be more readable if I could use separate variables to track their counts instead of iterating over ctl arrays over dpu_cstate->num_intfs and leaving comments that both will be same for this version of hardware. Also, the counts need not be the same for all the Snapdragon variants. Thanks, Jeykumar S. +} + static int dpu_crtc_atomic_check(struct drm_crtc *crtc, struct drm_crtc_state *state) { @@ -1014,6 +1036,8 @@ static int dpu_crtc_atomic_check(struct drm_crtc *crtc, const struct drm_plane_state *pstate; struct drm_plane *plane; struct drm_display_mode *mode; + struct msm_drm_private *priv; + struct dpu_kms *dpu_kms; int cnt = 0, rc = 0, mixer_width, i, z_pos; @@ -1039,6 +1063,9 @@ static int dpu_crtc_atomic_check(struct drm_crtc *crtc, goto end; } + priv = crtc->dev->dev_private; + dpu_kms = to_dpu_kms(priv->kms); + mode = &state->adjusted_mode; DPU_DEBUG("%s: check", dpu_crtc->name); @@ -1229,6 +1256,10 @@ static int dpu_crtc_atomic_check(struct drm_crtc *crtc, } } + _dpu_crtc_get_topology(state, mode); + if (drm_atomic_crtc_needs_modeset(state)) + rc = dpu_rm_crtc_reserve(&dpu_kms->rm, state); + end: kfree(pstates); return rc; diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c index 5d501c8..ce66309 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c @@ -67,8 +67,6 @@ #define IDLE_SHORT_TIMEOUT 1 -#define MAX_VDISPLAY_SPLIT 1080 - /** * enum dpu_enc_rc
Re: [Freedreno] [PATCH 24/25] drm/msm/dpu: remove mutex locking for RM interfaces
On 2018-10-09 12:57, Sean Paul wrote: On Mon, Oct 08, 2018 at 09:27:41PM -0700, Jeykumar Sankaran wrote: Since HW reservations are happening through atomic_check and all the display commits are catered by a single commit thread, it is not necessary to protect the interfaces by a separate mutex. Signed-off-by: Jeykumar Sankaran --- drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 24 drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h | 2 -- 2 files changed, 26 deletions(-) /snip diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h index 8676fa5..9acbeba 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h @@ -24,11 +24,9 @@ * struct dpu_rm - DPU dynamic hardware resource manager * @hw_blks: array of lists of hardware resources present in the system, one * list per type of hardware block - * @rm_lock: resource manager mutex */ struct dpu_rm { struct list_head hw_blks[DPU_HW_BLK_MAX]; At this point, there's really not much point to even having the rm. It's just another level of indirection that IMO complicates the code. If you look at the usage of hw_blks, the code is always looking at a specific type of hw_blk, so the array is unnecessary. dpu_kms could just keep a few arrays/lists of the hw types, and the crtc and encoder reserve functions can just go in crtc/encoder. Sean RM has been reduced to its current form to manage only LM/PP, CTL and interfaces. Our eventual plan is to support all the advanced HW blocks and its features in an upstream friendly way. When RM grows to manage all its subblocks, iteration logic may get heavy since the chipset have HW chain restrictions on various hw blocks. To provide room for the growth, I suggest keeping the allocation helpers in a separate file. But I can see why you want to maintain the HW block lists in the KMS. Thanks, Jeykumar S. - struct mutex rm_lock; }; /** -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno -- Jeykumar S ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH 01/25] drm/msm/dpu: fix hw ctl retrieval for mixer muxing
On 2018-10-09 11:07, Sean Paul wrote: On Mon, Oct 08, 2018 at 09:27:18PM -0700, Jeykumar Sankaran wrote: Layer mixer/pingpong block counts and hw ctl block counts will not be same for all the topologies (e.g. layer mixer muxing to single interface) Use the encoder's split_role info to retrieve the respective control path for programming. Signed-off-by: Jeykumar Sankaran --- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 9 ++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c index 96cdf06..d12f896 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c @@ -1060,6 +1060,7 @@ static void dpu_encoder_virt_mode_set(struct drm_encoder *drm_enc, for (i = 0; i < dpu_enc->num_phys_encs; i++) { struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i]; + int ctl_index; if (phys) { if (!dpu_enc->hw_pp[i]) { @@ -1068,14 +1069,16 @@ static void dpu_encoder_virt_mode_set(struct drm_encoder *drm_enc, return; } - if (!hw_ctl[i]) { + ctl_index = phys->split_role == ENC_ROLE_SLAVE ? 1 : 0; + What if MAX_CHANNELS_PER_ENC isn't 2? Similarly, what if num_phys_encs > MAX_CHANNELS_PER_ENC? It seems like there should be a more formal relationship between all of these verious values (num_of_h_tiles assumed to be <= 2 as well). If one of them changes beyond the assumed bound, the rest of the driver falls over pretty hard. MAX_CHANNELS_PER_ENC is set to 2 to represent HW limitation on the chipset as we cannot gang up more than 2 LM chain to an interface. Supporting more than 2 might demand much larger changes than validating for boundaries. num_phys_enc is the max no of phys encoders we create as we are looping through num_of_h_tiles which cannot be more than priv->dsi array size. So its very unlikely we would expect these loops to go out of bound! Thanks, Jeykumar S. + if (!hw_ctl[ctl_index]) { DPU_ERROR_ENC(dpu_enc, "no ctl block assigned" -"at idx: %d\n", i); +"at idx: %d\n", ctl_index); return; When you return on error here, should you give back the resources that you've already provisioned? } phys->hw_pp = dpu_enc->hw_pp[i]; - phys->hw_ctl = hw_ctl[i]; + phys->hw_ctl = hw_ctl[ctl_index]; phys->connector = conn->state->connector; if (phys->ops.mode_set) -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno -- Jeykumar S ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH 1/3] drm/msm/dpu: clean up references of DPU custom bus scaling
Hi Sravanthi, Thank you for the patch! Yet something to improve: [auto build test ERROR on robclark/msm-next] [also build test ERROR on next-20181009] [cannot apply to v4.19-rc7] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Sravanthi-Kollukuduru/Use-interconnect-API-in-MDSS-on-SDM845/20181008-203309 base: git://people.freedesktop.org/~robclark/linux msm-next config: arm-multi_v7_defconfig (attached as .config) compiler: arm-linux-gnueabi-gcc (Debian 7.2.0-11) 7.2.0 reproduce: wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree GCC_VERSION=7.2.0 make.cross ARCH=arm All errors (new ones prefixed by >>): drivers/gpu//drm/msm/disp/dpu1/dpu_crtc.c: In function 'dpu_crtc_debugfs_state_show': >> drivers/gpu//drm/msm/disp/dpu1/dpu_crtc.c:1458:7: error: 'i' undeclared >> (first use in this function) for (i = DPU_POWER_HANDLE_DBUS_ID_MNOC; ^ drivers/gpu//drm/msm/disp/dpu1/dpu_crtc.c:1458:7: note: each undeclared identifier is reported only once for each function it appears in >> drivers/gpu//drm/msm/disp/dpu1/dpu_crtc.c:1458:11: error: >> 'DPU_POWER_HANDLE_DBUS_ID_MNOC' undeclared (first use in this function); did >> you mean '_DPU_POWER_HANDLE_H_'? for (i = DPU_POWER_HANDLE_DBUS_ID_MNOC; ^ _DPU_POWER_HANDLE_H_ >> drivers/gpu//drm/msm/disp/dpu1/dpu_crtc.c:1459:8: error: >> 'DPU_POWER_HANDLE_DBUS_ID_MAX' undeclared (first use in this function); did >> you mean 'DPU_POWER_HANDLE_DBUS_ID_MNOC'? i < DPU_POWER_HANDLE_DBUS_ID_MAX; i++) { ^~~~ DPU_POWER_HANDLE_DBUS_ID_MNOC >> drivers/gpu//drm/msm/disp/dpu1/dpu_crtc.c:1461:5: error: implicit >> declaration of function 'dpu_power_handle_get_dbus_name'; did you mean >> 'dpu_power_handle_register_event'? [-Werror=implicit-function-declaration] dpu_power_handle_get_dbus_name(i), ^~ dpu_power_handle_register_event cc1: some warnings being treated as errors vim +1458 drivers/gpu//drm/msm/disp/dpu1/dpu_crtc.c 25fdd593 Jeykumar Sankaran 2018-06-27 1448 25fdd593 Jeykumar Sankaran 2018-06-27 1449 static int dpu_crtc_debugfs_state_show(struct seq_file *s, void *v) 25fdd593 Jeykumar Sankaran 2018-06-27 1450 { 25fdd593 Jeykumar Sankaran 2018-06-27 1451 struct drm_crtc *crtc = (struct drm_crtc *) s->private; 25fdd593 Jeykumar Sankaran 2018-06-27 1452 struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc); 25fdd593 Jeykumar Sankaran 2018-06-27 1453 25fdd593 Jeykumar Sankaran 2018-06-27 1454 seq_printf(s, "client type: %d\n", dpu_crtc_get_client_type(crtc)); 25fdd593 Jeykumar Sankaran 2018-06-27 1455 seq_printf(s, "intf_mode: %d\n", dpu_crtc_get_intf_mode(crtc)); 25fdd593 Jeykumar Sankaran 2018-06-27 1456 seq_printf(s, "core_clk_rate: %llu\n", 25fdd593 Jeykumar Sankaran 2018-06-27 1457 dpu_crtc->cur_perf.core_clk_rate); 25fdd593 Jeykumar Sankaran 2018-06-27 @1458 for (i = DPU_POWER_HANDLE_DBUS_ID_MNOC; 25fdd593 Jeykumar Sankaran 2018-06-27 @1459 i < DPU_POWER_HANDLE_DBUS_ID_MAX; i++) { 25fdd593 Jeykumar Sankaran 2018-06-27 1460 seq_printf(s, "bw_ctl[%s]: %llu\n", 25fdd593 Jeykumar Sankaran 2018-06-27 @1461 dpu_power_handle_get_dbus_name(i), 25fdd593 Jeykumar Sankaran 2018-06-27 1462 dpu_crtc->cur_perf.bw_ctl[i]); 25fdd593 Jeykumar Sankaran 2018-06-27 1463 seq_printf(s, "max_per_pipe_ib[%s]: %llu\n", 25fdd593 Jeykumar Sankaran 2018-06-27 1464 dpu_power_handle_get_dbus_name(i), 25fdd593 Jeykumar Sankaran 2018-06-27 1465 dpu_crtc->cur_perf.max_per_pipe_ib[i]); 25fdd593 Jeykumar Sankaran 2018-06-27 1466 } 25fdd593 Jeykumar Sankaran 2018-06-27 1467 25fdd593 Jeykumar Sankaran 2018-06-27 1468 return 0; 25fdd593 Jeykumar Sankaran 2018-06-27 1469 } 25fdd593 Jeykumar Sankaran 2018-06-27 1470 DEFINE_DPU_DEBUGFS_SEQ_FOPS(dpu_crtc_debugfs_state); 25fdd593 Jeykumar Sankaran 2018-06-27 1471 :: The code at line 1458 was first introduced by commit :: 25fdd5933e4c0f5fe2ea5cd59994f8ac5fbe90ef drm/msm: Add SDM845 DPU support :: TO: Jeykumar Sankaran :: CC: Sean Paul --- 0-DAY kernel test infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation .config.gz Description: application/gzip ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH 06/25] drm/msm/dpu: clean up redundant hw type
Hi Jeykumar, Thank you for the patch! Yet something to improve: [auto build test ERROR on robclark/msm-next] [also build test ERROR on next-20181009] [cannot apply to v4.19-rc7] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Jeykumar-Sankaran/reserve-RM-resources-in-CRTC-state/20181010-031051 base: git://people.freedesktop.org/~robclark/linux msm-next config: arm64-allmodconfig (attached as .config) compiler: aarch64-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0 reproduce: wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree GCC_VERSION=7.2.0 make.cross ARCH=arm64 Note: the linux-review/Jeykumar-Sankaran/reserve-RM-resources-in-CRTC-state/20181010-031051 HEAD c098339a791502c4a3732a304890f36240874372 builds fine. It only hurts bisectibility. All error/warnings (new ones prefixed by >>): In file included from include/drm/drm_mm.h:49:0, from include/drm/drmP.h:73, from drivers/gpu/drm/msm/msm_drv.h:39, from drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h:22, from drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c:16: drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c: In function '_dpu_rm_release_reservation': >> drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c:622:11: error: 'struct dpu_rm_hw_blk' >> has no member named 'type' blk->type, blk->id); ^ include/drm/drm_print.h:352:30: note: in definition of macro 'DRM_DEBUG' drm_dbg(DRM_UT_CORE, fmt, ##__VA_ARGS__) ^~~ >> drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c:621:5: note: in expansion of macro >> 'DPU_DEBUG' DPU_DEBUG("rel enc %d %d %d\n", enc_id, ^ In file included from include/linux/printk.h:336:0, from include/linux/kernel.h:14, from drivers/gpu/drm/msm/msm_drv.h:22, from drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h:22, from drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c:16: >> drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c:622:11: error: 'struct dpu_rm_hw_blk' >> has no member named 'type' blk->type, blk->id); ^ include/linux/dynamic_debug.h:128:10: note: in definition of macro 'dynamic_pr_debug' ##__VA_ARGS__); \ ^~~ >> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h:48:4: note: in expansion of macro >> 'pr_debug' pr_debug(fmt, ##__VA_ARGS__); \ ^~~~ >> drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c:621:5: note: in expansion of macro >> 'DPU_DEBUG' DPU_DEBUG("rel enc %d %d %d\n", enc_id, ^ vim +622 drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c 25fdd593 Jeykumar Sankaran 2018-06-27 611 0ff27752 Jeykumar Sankaran 2018-10-08 612 static void _dpu_rm_release_reservation(struct dpu_rm *rm, uint32_t enc_id) 25fdd593 Jeykumar Sankaran 2018-06-27 613 { 25fdd593 Jeykumar Sankaran 2018-06-27 614 struct dpu_rm_hw_blk *blk; 25fdd593 Jeykumar Sankaran 2018-06-27 615 enum dpu_hw_blk_type type; 25fdd593 Jeykumar Sankaran 2018-06-27 616 25fdd593 Jeykumar Sankaran 2018-06-27 617 for (type = 0; type < DPU_HW_BLK_MAX; type++) { 25fdd593 Jeykumar Sankaran 2018-06-27 618 list_for_each_entry(blk, &rm->hw_blks[type], list) { 0ff27752 Jeykumar Sankaran 2018-10-08 619 if (blk->enc_id == enc_id) { 0ff27752 Jeykumar Sankaran 2018-10-08 620 blk->enc_id = 0; 0ff27752 Jeykumar Sankaran 2018-10-08 @621 DPU_DEBUG("rel enc %d %d %d\n", enc_id, 25fdd593 Jeykumar Sankaran 2018-06-27 @622 blk->type, blk->id); 25fdd593 Jeykumar Sankaran 2018-06-27 623 } 25fdd593 Jeykumar Sankaran 2018-06-27 624 } 25fdd593 Jeykumar Sankaran 2018-06-27 625 } 25fdd593 Jeykumar Sankaran 2018-06-27 626 } 25fdd593 Jeykumar Sankaran 2018-06-27 627 :: The code at line 622 was first introduced by commit :: 25fdd5933e4c0f5fe2ea5cd59994f8ac5fbe90ef drm/msm: Add SDM845 DPU support :: TO: Jeykumar Sankaran :: CC: Sean Paul --- 0-DAY kernel test infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation .config.gz Description: application/gzip ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH 07/25] drm/msm/dpu: reserve using crtc state
Hi Jeykumar, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on robclark/msm-next] [also build test WARNING on next-20181009] [cannot apply to v4.19-rc7] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Jeykumar-Sankaran/reserve-RM-resources-in-CRTC-state/20181010-031051 base: git://people.freedesktop.org/~robclark/linux msm-next config: arm-allmodconfig (attached as .config) compiler: arm-linux-gnueabi-gcc (Debian 7.2.0-11) 7.2.0 reproduce: wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree GCC_VERSION=7.2.0 make.cross ARCH=arm All warnings (new ones prefixed by >>): In file included from include/linux/list.h:9:0, from include/linux/wait.h:7, from include/linux/wait_bit.h:8, from include/linux/fs.h:6, from include/linux/debugfs.h:15, from drivers/gpu//drm/msm/disp/dpu1/dpu_crtc.c:21: drivers/gpu//drm/msm/disp/dpu1/dpu_crtc.c: In function '_dpu_crtc_setup_mixers': include/linux/kernel.h:845:29: warning: comparison of distinct pointer types lacks a cast (!!(sizeof((typeof(x) *)1 == (typeof(y) *)1))) ^ include/linux/kernel.h:859:4: note: in expansion of macro '__typecheck' (__typecheck(x, y) && __no_side_effects(x, y)) ^~~ include/linux/kernel.h:869:24: note: in expansion of macro '__safe_cmp' __builtin_choose_expr(__safe_cmp(x, y), \ ^~ include/linux/kernel.h:878:19: note: in expansion of macro '__careful_cmp' #define min(x, y) __careful_cmp(x, y, <) ^ >> drivers/gpu//drm/msm/disp/dpu1/dpu_crtc.c:435:15: note: in expansion of >> macro 'min' ctl_index = min(i, cstate->num_ctls - 1); ^~~ vim +/min +435 drivers/gpu//drm/msm/disp/dpu1/dpu_crtc.c 423 424 static void _dpu_crtc_setup_mixers(struct drm_crtc *crtc) 425 { 426 struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc->state); 427 struct dpu_crtc_mixer *mixer; 428 int i, ctl_index; 429 430 /* Set up all the mixers and ctls reserved by this encoder */ 431 for (i = 0; i < cstate->num_mixers; i++) { 432 mixer = &cstate->mixers[i]; 433 434 /* CTL may be <= LMs, if <, multiple LMs controlled by 1 CTL */ > 435 ctl_index = min(i, cstate->num_ctls - 1); 436 mixer->lm_ctl = cstate->hw_ctls[ctl_index]; 437 } 438 } 439 --- 0-DAY kernel test infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation .config.gz Description: application/gzip ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH 07/25] drm/msm/dpu: reserve using crtc state
On Mon, Oct 08, 2018 at 09:27:24PM -0700, Jeykumar Sankaran wrote: > DPU maintained reservation lists to cache assigned > HW blocks for the display and a retrieval mechanism for > the individual DRM components to query their respective > HW blocks. > > This patch uses the sub-classed CRTC state to store > and track HW blocks assigned for different components > of the display pipeline. It helps the driver: > - to get rid of unwanted store and retrieval RM API's > - to preserve HW resources assigned in atomic_check > through atomic swap/duplicate. > > Separate patch is submitted to remove resource > reservation in atomic_commit path. > > Signed-off-by: Jeykumar Sankaran > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 65 > +++--- > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h | 14 + > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c| 28 +++--- > .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 20 ++- > drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 58 --- > 5 files changed, 72 insertions(+), 113 deletions(-) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > index 4960641..0625f56 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > @@ -421,69 +421,20 @@ void dpu_crtc_complete_commit(struct drm_crtc *crtc, > trace_dpu_crtc_complete_commit(DRMID(crtc)); > } > > -static void _dpu_crtc_setup_mixer_for_encoder( > - struct drm_crtc *crtc, > - struct drm_encoder *enc) > +static void _dpu_crtc_setup_mixers(struct drm_crtc *crtc) > { > struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc->state); > - struct dpu_kms *dpu_kms = _dpu_crtc_get_kms(crtc); > - struct dpu_rm *rm = &dpu_kms->rm; > struct dpu_crtc_mixer *mixer; > - struct dpu_hw_ctl *last_valid_ctl = NULL; > - int i; > - struct dpu_rm_hw_iter lm_iter, ctl_iter; > - > - dpu_rm_init_hw_iter(&lm_iter, enc->base.id, DPU_HW_BLK_LM); > - dpu_rm_init_hw_iter(&ctl_iter, enc->base.id, DPU_HW_BLK_CTL); > + int i, ctl_index; > > /* Set up all the mixers and ctls reserved by this encoder */ > - for (i = cstate->num_mixers; i < ARRAY_SIZE(cstate->mixers); i++) { > + for (i = 0; i < cstate->num_mixers; i++) { > mixer = &cstate->mixers[i]; > > - if (!dpu_rm_get_hw(rm, &lm_iter)) > - break; > - mixer->hw_lm = (struct dpu_hw_mixer *)lm_iter.hw; > - > /* CTL may be <= LMs, if <, multiple LMs controlled by 1 CTL */ > - if (!dpu_rm_get_hw(rm, &ctl_iter)) { > - DPU_DEBUG("no ctl assigned to lm %d, using previous\n", > - mixer->hw_lm->idx - LM_0); > - mixer->lm_ctl = last_valid_ctl; > - } else { > - mixer->lm_ctl = (struct dpu_hw_ctl *)ctl_iter.hw; > - last_valid_ctl = mixer->lm_ctl; > - } > - > - /* Shouldn't happen, mixers are always >= ctls */ > - if (!mixer->lm_ctl) { > - DPU_ERROR("no valid ctls found for lm %d\n", > - mixer->hw_lm->idx - LM_0); > - return; > - } > - > - cstate->num_mixers++; > - DPU_DEBUG("setup mixer %d: lm %d\n", > - i, mixer->hw_lm->idx - LM_0); > - DPU_DEBUG("setup mixer %d: ctl %d\n", > - i, mixer->lm_ctl->idx - CTL_0); > - } > -} > - > -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; > - > - _dpu_crtc_setup_mixer_for_encoder(crtc, enc); > + ctl_index = min(i, cstate->num_ctls - 1); This is another one of those places I mentioned where we're just assuming a value is going to be in a certain range. If num_ctls/num_intfs/num_phys_encs (all the same value afaict) is 0, we end up in a bad place. At a minimum, there should be a WARN_ON/BUG_ON somewhere ensuring this can never drop below 0. > + mixer->lm_ctl = cstate->hw_ctls[ctl_index]; > } > - > - mutex_unlock(&dpu_crtc->crtc_lock); > } > > static void _dpu_crtc_setup_lm_bounds(struct drm_crtc *crtc, > @@ -536,10 +487,8 @@ static void dpu_crtc_atomic_begin(struct drm_crtc *crtc, > dev = crtc->dev; > smmu_state = &dpu_crtc->smmu_state; > > - if (!cstate->num_mixers) { > - _dpu_crtc_setup_mixers(crtc); > - _dpu_crtc_setup_lm_bounds(crtc,
Re: [Freedreno] [PATCH] drm/msm: Grab a vblank reference when waiting for commit_done
On 2018-10-09 06:59, Sean Paul wrote: On Mon, Oct 08, 2018 at 06:38:11PM -0700, Abhinav Kumar wrote: On 2018-10-03 13:22, Sean Paul wrote: > From: Sean Paul > > Similar to the atomic helpers, we should enable vblank while we're > waiting for the commit to finish. DPU needs this, MDP5 seems to work > fine without it. > > Signed-off-by: Sean Paul > --- As such I dont see any issue with this patch but I have a question overall on chrome vblank handling. For a video mode panel, we will keep the HW resources enabled (including vsync related clocks) till device is suspended. The vblank_get and vblank_put are only controlling the register/deregister of the vblank IRQ callback which send the events to the userspace and anything pending on vsync completion. Does the chrome userspace turn ON/OFF the vblank using vblank_ctrl IOCTL Or does it guarantee it to be? No not explicitly. The issue I was seeing is that when userspace (this issue is not specific to chrome) does a commit we get a frame_done timeout. So while the hardware might still be active, the worker to signal frame_done is not unless the vblank_ref is > 0. If so, is this patch just more of a safety check to make sure that vsync events remain ON till the frame is done? Because HW wise it should be and this shouldnt be needed. It is definitely needed, unless you want 60ms (the frame_done timeout) between frames :-) Sean Alright, in that case Reviewed-by: Abhinav Kumar > drivers/gpu/drm/msm/msm_atomic.c | 5 + > 1 file changed, 5 insertions(+) > > diff --git a/drivers/gpu/drm/msm/msm_atomic.c > b/drivers/gpu/drm/msm/msm_atomic.c > index c1f1779c980f..2b7bb6e166d3 100644 > --- a/drivers/gpu/drm/msm/msm_atomic.c > +++ b/drivers/gpu/drm/msm/msm_atomic.c > @@ -32,7 +32,12 @@ static void msm_atomic_wait_for_commit_done(struct > drm_device *dev, >if (!new_crtc_state->active) >continue; > > + if (drm_crtc_vblank_get(crtc)) > + continue; > + >kms->funcs->wait_for_crtc_commit_done(kms, crtc); > + > + drm_crtc_vblank_put(crtc); >} > } ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH 22/25] drm/msm/dpu: make crtc and encoder specific HW reservation
On Mon, Oct 08, 2018 at 09:27:39PM -0700, Jeykumar Sankaran wrote: > Instead of letting encoder make a centralized reservation for > all of its display DRM components, this path splits the > responsibility between CRTC and Encoder, each requesting > RM for the HW mapping of its own domain. > > Signed-off-by: Jeykumar Sankaran > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c| 31 + > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 14 ++ > drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 69 > - > drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h | 36 +++ > 4 files changed, 119 insertions(+), 31 deletions(-) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > index 0625f56..0536b8a 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > @@ -47,6 +47,8 @@ > #define LEFT_MIXER 0 > #define RIGHT_MIXER 1 > > +#define MAX_VDISPLAY_SPLIT 1080 > + > static inline int _dpu_crtc_get_mixer_width(struct dpu_crtc_state *cstate, > struct drm_display_mode *mode) > { > @@ -448,6 +450,7 @@ static void _dpu_crtc_setup_lm_bounds(struct drm_crtc > *crtc, > > for (i = 0; i < cstate->num_mixers; i++) { > struct drm_rect *r = &cstate->lm_bounds[i]; > + > r->x1 = crtc_split_width * i; > r->y1 = 0; > r->x2 = r->x1 + crtc_split_width; > @@ -885,6 +888,7 @@ static void dpu_crtc_disable(struct drm_crtc *crtc) > struct drm_display_mode *mode; > struct drm_encoder *encoder; > struct msm_drm_private *priv; > + struct dpu_kms *dpu_kms; > unsigned long flags; > > if (!crtc || !crtc->dev || !crtc->dev->dev_private || !crtc->state) { > @@ -895,6 +899,7 @@ static void dpu_crtc_disable(struct drm_crtc *crtc) > cstate = to_dpu_crtc_state(crtc->state); > mode = &cstate->base.adjusted_mode; > priv = crtc->dev->dev_private; > + dpu_kms = to_dpu_kms(priv->kms); > > DRM_DEBUG_KMS("crtc%d\n", crtc->base.id); > > @@ -953,6 +958,8 @@ static void dpu_crtc_disable(struct drm_crtc *crtc) > crtc->state->event = NULL; > spin_unlock_irqrestore(&crtc->dev->event_lock, flags); > } > + > + dpu_rm_crtc_release(&dpu_kms->rm, crtc->state); > } > > static void dpu_crtc_enable(struct drm_crtc *crtc, > @@ -1004,6 +1011,21 @@ struct plane_state { > u32 pipe_id; > }; > > +static void _dpu_crtc_get_topology( > + struct drm_crtc_state *crtc_state, > + struct drm_display_mode *mode) > +{ > + struct dpu_crtc_state *dpu_cstate = to_dpu_crtc_state(crtc_state); > + > + dpu_cstate->num_mixers = (mode->vdisplay > MAX_VDISPLAY_SPLIT) ? 2 : 1; > + > + /** > + * encoder->atomic_check is invoked before crtc->atomic_check. > + * so dpu_cstate->num_intfs should have a non-zero value. > + */ > + dpu_cstate->num_ctls = dpu_cstate->num_intfs; Why do we need num_ctls? Can't we just use dpu_cstate->num_intfs directly? Also, you don't really need these in their own function, especially if num_ctls goes away. > +} > + > static int dpu_crtc_atomic_check(struct drm_crtc *crtc, > struct drm_crtc_state *state) > { > @@ -1014,6 +1036,8 @@ static int dpu_crtc_atomic_check(struct drm_crtc *crtc, > const struct drm_plane_state *pstate; > struct drm_plane *plane; > struct drm_display_mode *mode; > + struct msm_drm_private *priv; > + struct dpu_kms *dpu_kms; > > int cnt = 0, rc = 0, mixer_width, i, z_pos; > > @@ -1039,6 +1063,9 @@ static int dpu_crtc_atomic_check(struct drm_crtc *crtc, > goto end; > } > > + priv = crtc->dev->dev_private; > + dpu_kms = to_dpu_kms(priv->kms); > + > mode = &state->adjusted_mode; > DPU_DEBUG("%s: check", dpu_crtc->name); > > @@ -1229,6 +1256,10 @@ static int dpu_crtc_atomic_check(struct drm_crtc *crtc, > } > } > > + _dpu_crtc_get_topology(state, mode); > + if (drm_atomic_crtc_needs_modeset(state)) > + rc = dpu_rm_crtc_reserve(&dpu_kms->rm, state); > + > end: > kfree(pstates); > return rc; > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > index 5d501c8..ce66309 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > @@ -67,8 +67,6 @@ > > #define IDLE_SHORT_TIMEOUT 1 > > -#define MAX_VDISPLAY_SPLIT 1080 > - > /** > * enum dpu_enc_rc_events - events for resource control state machine > * @DPU_ENC_RC_EVENT_KICKOFF: > @@ -557,14 +555,10 @@ static void _dpu_encoder_adjust_mode(struct > drm_connector *connector, > > static void _dpu_encoder_get_topology( > struct dpu_encoder_virt *dpu_enc, > - struct
Re: [Freedreno] [PATCH 06/25] drm/msm/dpu: clean up redundant hw type
On Mon, Oct 08, 2018 at 09:27:23PM -0700, Jeykumar Sankaran wrote: > struct dpu_hw_blk has hw block type info. Remove duplicate > type tracking in struct dpu_rm_hw_blk. > > Signed-off-by: Jeykumar Sankaran > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 17 - > 1 file changed, 4 insertions(+), 13 deletions(-) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c > b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c > index 5ce89b9..377def7 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c > @@ -38,14 +38,12 @@ struct dpu_rm_requirements { > /** > * struct dpu_rm_hw_blk - hardware block tracking list member > * @list:List head for list of all hardware blocks tracking items > - * @type:Type of hardware block this structure tracks > * @id: Hardware ID number, within it's own space, ie. LM_X > * @enc_id: Encoder id to which this blk is binded > * @hw: Pointer to the hardware register access object for this > block > */ > struct dpu_rm_hw_blk { > struct list_head list; > - enum dpu_hw_blk_type type; > uint32_t id; > uint32_t enc_id; > struct dpu_hw_blk *hw; > @@ -86,12 +84,6 @@ static bool _dpu_rm_get_hw_locked(struct dpu_rm *rm, > struct dpu_rm_hw_iter *i) > i->blk = list_prepare_entry(i->blk, blk_list, list); > > list_for_each_entry_continue(i->blk, blk_list, list) { > - if (i->blk->type != i->type) { > - DPU_ERROR("found incorrect block type %d on %d list\n", > - i->blk->type, i->type); > - return false; > - } > - > if (i->enc_id == i->blk->enc_id) { > i->hw = i->blk->hw; > DPU_DEBUG("found type %d id %d for enc %d\n", > @@ -151,7 +143,7 @@ int dpu_rm_destroy(struct dpu_rm *rm) > list_for_each_entry_safe(hw_cur, hw_nxt, &rm->hw_blks[type], > list) { > list_del(&hw_cur->list); > - _dpu_rm_hw_destroy(hw_cur->type, hw_cur->hw); > + _dpu_rm_hw_destroy(type, hw_cur->hw); > kfree(hw_cur); > } > } > @@ -213,7 +205,6 @@ static int _dpu_rm_hw_blk_create( > return -ENOMEM; > } > > - blk->type = type; > blk->id = id; > blk->hw = hw; > blk->enc_id = 0; > @@ -458,7 +449,7 @@ static int _dpu_rm_reserve_lms(struct dpu_rm *rm, > uint32_t enc_id, > lm[i]->enc_id = enc_id; > pp[i]->enc_id = enc_id; > > - trace_dpu_rm_reserve_lms(lm[i]->id, lm[i]->type, enc_id, > + trace_dpu_rm_reserve_lms(lm[i]->id, DPU_HW_BLK_LM, enc_id, >pp[i]->id); > } > > @@ -510,7 +501,7 @@ static int _dpu_rm_reserve_ctls( > > for (i = 0; i < ARRAY_SIZE(ctls) && i < num_ctls; i++) { > ctls[i]->enc_id = enc_id; > - trace_dpu_rm_reserve_ctls(ctls[i]->id, ctls[i]->type, > + trace_dpu_rm_reserve_ctls(ctls[i]->id, DPU_HW_BLK_CTL, > enc_id); > } > > @@ -538,7 +529,7 @@ static int _dpu_rm_reserve_intf( > } > > iter.blk->enc_id = enc_id; > - trace_dpu_rm_reserve_intf(iter.blk->id, iter.blk->type, > + trace_dpu_rm_reserve_intf(iter.blk->id, DPU_HW_BLK_INTF, You should probably just remove the argument from the trace statements since it's redundant. Sean > enc_id); > break; > } > -- > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > a Linux Foundation Collaborative Project > -- Sean Paul, Software Engineer, Google / Chromium OS ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
[Freedreno] [PATCH] drm/msm: dpu: Simplify conn->state->connector to conn
From: Sean Paul state->connector is just a backpointer to conn with the added risk of state being NULL. Signed-off-by: Sean Paul --- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c index 82c55efb500f..05015d5ae3d2 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c @@ -1044,7 +1044,7 @@ static void dpu_encoder_virt_mode_set(struct drm_encoder *drm_enc, phys->hw_pp = dpu_enc->hw_pp[i]; phys->hw_ctl = hw_ctl[i]; - phys->connector = conn->state->connector; + phys->connector = conn; if (phys->ops.mode_set) phys->ops.mode_set(phys, mode, adj_mode); } -- Sean Paul, Software Engineer, Google / Chromium OS ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH 05/25] drm/msm/dpu: remove encoder from crtc mixer struct
On Mon, Oct 08, 2018 at 09:27:22PM -0700, Jeykumar Sankaran wrote: > Not actively used. Clean up the crtc mixer struct. > > Signed-off-by: Jeykumar Sankaran Reviewed-by: Sean Paul > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 2 -- > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h | 2 -- > 2 files changed, 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 d4530d6..4960641 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > @@ -461,8 +461,6 @@ static void _dpu_crtc_setup_mixer_for_encoder( > return; > } > > - mixer->encoder = enc; > - > cstate->num_mixers++; > DPU_DEBUG("setup mixer %d: lm %d\n", > i, mixer->hw_lm->idx - LM_0); > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h > b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h > index 3723b48..75fdd3c 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h > @@ -84,14 +84,12 @@ struct dpu_crtc_smmu_state_data { > * struct dpu_crtc_mixer: stores the map for each virtual pipeline in the > CRTC > * @hw_lm: LM HW Driver context > * @lm_ctl: CTL Path HW driver context > - * @encoder: Encoder attached to this lm & ctl > * @mixer_op_mode: mixer blending operation mode > * @flush_mask: mixer flush mask for ctl, mixer and pipe > */ > struct dpu_crtc_mixer { > struct dpu_hw_mixer *hw_lm; > struct dpu_hw_ctl *lm_ctl; > - struct drm_encoder *encoder; > u32 mixer_op_mode; > u32 flush_mask; > }; > -- > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > a Linux Foundation Collaborative Project > -- Sean Paul, Software Engineer, Google / Chromium OS ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH 04/25] drm/msm/dpu: clean up dpu_rm_check_property_topctl declaration
On Mon, Oct 08, 2018 at 09:27:21PM -0700, Jeykumar Sankaran wrote: > Definition was removed already. Clean up header declaration. > > Signed-off-by: Jeykumar Sankaran Reviewed-by: Sean Paul > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h | 8 > 1 file changed, 8 deletions(-) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h > b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h > index f41fd19..eb6a6ac 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h > @@ -136,12 +136,4 @@ void dpu_rm_init_hw_iter( > * @Return: true on match found, false on no match found > */ > bool dpu_rm_get_hw(struct dpu_rm *rm, struct dpu_rm_hw_iter *iter); > - > -/** > - * dpu_rm_check_property_topctl - validate property bitmask before it is set > - * @val: user's proposed topology control bitmask > - * @Return: 0 on success or error > - */ > -int dpu_rm_check_property_topctl(uint64_t val); > - > #endif /* __DPU_RM_H__ */ > -- > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > a Linux Foundation Collaborative Project > -- Sean Paul, Software Engineer, Google / Chromium OS ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH 03/25] drm/msm/dpu: remove dev from RM
On Mon, Oct 08, 2018 at 09:27:20PM -0700, Jeykumar Sankaran wrote: > Not used. Remove from RM. > > Signed-off-by: Jeykumar Sankaran Reviewed-by: Sean Paul > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 3 +-- > drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 7 ++- > drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h | 6 +- > 3 files changed, 4 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > index 0a683e6..8309850 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > @@ -1075,8 +1075,7 @@ static int dpu_kms_hw_init(struct msm_kms *kms) > goto power_error; > } > > - rc = dpu_rm_init(&dpu_kms->rm, dpu_kms->catalog, dpu_kms->mmio, > - dpu_kms->dev); > + rc = dpu_rm_init(&dpu_kms->rm, dpu_kms->catalog, dpu_kms->mmio); > if (rc) { > DPU_ERROR("rm init failed: %d\n", rc); > goto power_error; > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c > b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c > index 36a929b..5ce89b9 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c > @@ -224,13 +224,12 @@ static int _dpu_rm_hw_blk_create( > > int dpu_rm_init(struct dpu_rm *rm, > struct dpu_mdss_cfg *cat, > - void __iomem *mmio, > - struct drm_device *dev) > + void __iomem *mmio) > { > int rc, i; > enum dpu_hw_blk_type type; > > - if (!rm || !cat || !mmio || !dev) { > + if (!rm || !cat || !mmio) { > DPU_ERROR("invalid kms\n"); > return -EINVAL; > } > @@ -243,8 +242,6 @@ int dpu_rm_init(struct dpu_rm *rm, > for (type = 0; type < DPU_HW_BLK_MAX; type++) > INIT_LIST_HEAD(&rm->hw_blks[type]); > > - rm->dev = dev; > - > /* Some of the sub-blocks require an mdptop to be created */ > rm->hw_mdp = dpu_hw_mdptop_init(MDP_TOP, mmio, cat); > if (IS_ERR_OR_NULL(rm->hw_mdp)) { > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h > b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h > index 0dd3c21..f41fd19 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h > @@ -22,7 +22,6 @@ > > /** > * struct dpu_rm - DPU dynamic hardware resource manager > - * @dev: device handle for event logging purposes > * @hw_blks: array of lists of hardware resources present in the system, one > * list per type of hardware block > * @hw_mdp: hardware object for mdp_top > @@ -30,7 +29,6 @@ > * @rm_lock: resource manager mutex > */ > struct dpu_rm { > - struct drm_device *dev; > struct list_head hw_blks[DPU_HW_BLK_MAX]; > struct dpu_hw_mdp *hw_mdp; > uint32_t lm_max_width; > @@ -63,13 +61,11 @@ struct dpu_rm_hw_iter { > * @rm: DPU Resource Manager handle > * @cat: Pointer to hardware catalog > * @mmio: mapped register io address of MDP > - * @dev: device handle for event logging purposes > * @Return: 0 on Success otherwise -ERROR > */ > int dpu_rm_init(struct dpu_rm *rm, > struct dpu_mdss_cfg *cat, > - void __iomem *mmio, > - struct drm_device *dev); > + void __iomem *mmio); > > /** > * dpu_rm_destroy - Free all memory allocated by dpu_rm_init > -- > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > a Linux Foundation Collaborative Project > -- Sean Paul, Software Engineer, Google / Chromium OS ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH 02/25] drm/msm/dpu: avoid tracking reservations in RM
On Mon, Oct 08, 2018 at 09:27:19PM -0700, Jeykumar Sankaran wrote: > RM was equipped with reservation tracking structure RSVP > to cache HW reservation of displays for certain clients > where atomic_checks (atomic commit with TEST_ONLY) for all > the displays are called before their respective atomic_commits. > Since DPU doesn't support the sequence anymore, clean up > the support from RM. Replace rsvp with the corresponding > encoder id to tag the HW blocks reserved. > Can you put something to the effect of "This is temporary and removed in a future patch" in the commit message? Reviewed-by: Sean Paul > Signed-off-by: Jeykumar Sankaran > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 284 > + > drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h | 4 - > 2 files changed, 43 insertions(+), 245 deletions(-) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c > b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c > index bdb1177..36a929b 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c > @@ -21,8 +21,8 @@ > #include "dpu_encoder.h" > #include "dpu_trace.h" > > -#define RESERVED_BY_OTHER(h, r) \ > - ((h)->rsvp && ((h)->rsvp->enc_id != (r)->enc_id)) > +#define RESERVED_BY_OTHER(h, r) \ > + ((h)->enc_id && (h)->enc_id != r) > > /** > * struct dpu_rm_requirements - Reservation requirements parameter bundle > @@ -34,85 +34,23 @@ struct dpu_rm_requirements { > struct dpu_encoder_hw_resources hw_res; > }; > > -/** > - * struct dpu_rm_rsvp - Use Case Reservation tagging structure > - * Used to tag HW blocks as reserved by a CRTC->Encoder->Connector chain > - * By using as a tag, rather than lists of pointers to HW blocks used > - * we can avoid some list management since we don't know how many blocks > - * of each type a given use case may require. > - * @list:List head for list of all reservations > - * @seq: Global RSVP sequence number for debugging, especially for > - * differentiating differenct allocations for same encoder. > - * @enc_id: Reservations are tracked by Encoder DRM object ID. > - * CRTCs may be connected to multiple Encoders. > - * An encoder or connector id identifies the display path. > - */ > -struct dpu_rm_rsvp { > - struct list_head list; > - uint32_t seq; > - uint32_t enc_id; > -}; > > /** > * struct dpu_rm_hw_blk - hardware block tracking list member > * @list:List head for list of all hardware blocks tracking items > - * @rsvp:Pointer to use case reservation if reserved by a client > - * @rsvp_nxt:Temporary pointer used during reservation to the > incoming > - * request. Will be swapped into rsvp if proposal is accepted > * @type:Type of hardware block this structure tracks > * @id: Hardware ID number, within it's own space, ie. LM_X > - * @catalog: Pointer to the hardware catalog entry for this block > + * @enc_id: Encoder id to which this blk is binded > * @hw: Pointer to the hardware register access object for this > block > */ > struct dpu_rm_hw_blk { > struct list_head list; > - struct dpu_rm_rsvp *rsvp; > - struct dpu_rm_rsvp *rsvp_nxt; > enum dpu_hw_blk_type type; > uint32_t id; > + uint32_t enc_id; > struct dpu_hw_blk *hw; > }; > > -/** > - * dpu_rm_dbg_rsvp_stage - enum of steps in making reservation for event > logging > - */ > -enum dpu_rm_dbg_rsvp_stage { > - DPU_RM_STAGE_BEGIN, > - DPU_RM_STAGE_AFTER_CLEAR, > - DPU_RM_STAGE_AFTER_RSVPNEXT, > - DPU_RM_STAGE_FINAL > -}; > - > -static void _dpu_rm_print_rsvps( > - struct dpu_rm *rm, > - enum dpu_rm_dbg_rsvp_stage stage) > -{ > - struct dpu_rm_rsvp *rsvp; > - struct dpu_rm_hw_blk *blk; > - enum dpu_hw_blk_type type; > - > - DPU_DEBUG("%d\n", stage); > - > - list_for_each_entry(rsvp, &rm->rsvps, list) { > - DRM_DEBUG_KMS("%d rsvp[s%ue%u]\n", stage, rsvp->seq, > - rsvp->enc_id); > - } > - > - for (type = 0; type < DPU_HW_BLK_MAX; type++) { > - list_for_each_entry(blk, &rm->hw_blks[type], list) { > - if (!blk->rsvp && !blk->rsvp_nxt) > - continue; > - > - DRM_DEBUG_KMS("%d rsvp[s%ue%u->s%ue%u] %d %d\n", stage, > - (blk->rsvp) ? blk->rsvp->seq : 0, > - (blk->rsvp) ? blk->rsvp->enc_id : 0, > - (blk->rsvp_nxt) ? blk->rsvp_nxt->seq : 0, > - (blk->rsvp_nxt) ? blk->rsvp_nxt->enc_id : 0, > - blk->type, blk->id); > - } > - } > -} > - > struct dpu_hw_mdp *dpu_rm_get_mdp(struct dpu_rm *rm) > { > return rm->hw_mdp; > @@ -148,15 +86,13 @@ static bool _dpu_rm_get_hw_locked(struct dpu_rm *rm, > struct dpu_rm_hw_iter *i) >
[Freedreno] [pull] drm/msm: msm-fixes-2018-10-09 for 4.20
Hi Dave, Fix for armv7 u64 division The following changes since commit 3ce36b4542b585ed0231b175aee31020b2f289c2: drm/msm/a6xx: Remove CP perfcounter selects from the protected list (2018-10-07 14:40:28 -0400) are available in the Git repository at: git://people.freedesktop.org/~robclark/linux drm-msm-fixes-2018-10-09 for you to fetch changes up to 16f37102181e23ec4bec88fe1cfdd6c3c24dd1ed: drm/msm: a6xx: Fix improper u64 division (2018-10-08 20:16:01 -0400) Sean Paul (2): drm/msm: a5xx: Remove unneeded parens drm/msm: a6xx: Fix improper u64 division drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 4 ++-- drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 11 +++ 2 files changed, 9 insertions(+), 6 deletions(-) ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH 24/25] drm/msm/dpu: remove mutex locking for RM interfaces
On Mon, Oct 08, 2018 at 09:27:41PM -0700, Jeykumar Sankaran wrote: > Since HW reservations are happening through atomic_check > and all the display commits are catered by a single commit thread, > it is not necessary to protect the interfaces by a separate > mutex. > > Signed-off-by: Jeykumar Sankaran > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 24 > drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h | 2 -- > 2 files changed, 26 deletions(-) > /snip > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h > b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h > index 8676fa5..9acbeba 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h > @@ -24,11 +24,9 @@ > * struct dpu_rm - DPU dynamic hardware resource manager > * @hw_blks: array of lists of hardware resources present in the system, one > * list per type of hardware block > - * @rm_lock: resource manager mutex > */ > struct dpu_rm { > struct list_head hw_blks[DPU_HW_BLK_MAX]; At this point, there's really not much point to even having the rm. It's just another level of indirection that IMO complicates the code. If you look at the usage of hw_blks, the code is always looking at a specific type of hw_blk, so the array is unnecessary. dpu_kms could just keep a few arrays/lists of the hw types, and the crtc and encoder reserve functions can just go in crtc/encoder. Sean > - struct mutex rm_lock; > }; > > /** > -- > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > a Linux Foundation Collaborative Project > > ___ > Freedreno mailing list > Freedreno@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/freedreno -- Sean Paul, Software Engineer, Google / Chromium OS ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH 00/25] reserve RM resources in CRTC state
On Mon, Oct 08, 2018 at 09:27:17PM -0700, Jeykumar Sankaran wrote: > Submitting series of patches to clean up DPU resource manager (RM) > of complicated hw iterations, redundant data maintenence and eventually > modifying the DPU to reserve display HW blocks only in atomic check > and caching the assigned HW blocks in atomic CRTC state. > I get the following build error with this series: In file included from ../include/linux/list.h:9:0, from ../include/linux/wait.h:7, from ../include/linux/wait_bit.h:8, from ../include/linux/fs.h:6, from ../include/linux/debugfs.h:15, from ../drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c:21: ../drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c: In function ‘_dpu_crtc_setup_mixers’: ../include/linux/kernel.h:845:29: warning: comparison of distinct pointer types lacks a cast (!!(sizeof((typeof(x) *)1 == (typeof(y) *)1))) ^ ../include/linux/kernel.h:859:4: note: in expansion of macro ‘__typecheck’ (__typecheck(x, y) && __no_side_effects(x, y)) ^~~ ../include/linux/kernel.h:869:24: note: in expansion of macro ‘__safe_cmp’ __builtin_choose_expr(__safe_cmp(x, y), \ ^~ ../include/linux/kernel.h:878:19: note: in expansion of macro ‘__careful_cmp’ #define min(x, y) __careful_cmp(x, y, <) ^ ../drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c:437:15: note: in expansion of macro ‘min’ ctl_index = min(i, cstate->num_ctls - 1); ^~~ > Thanks, > Jeykumar S. > > Jeykumar Sankaran (25): > drm/msm/dpu: fix hw ctl retrieval for mixer muxing > drm/msm/dpu: avoid tracking reservations in RM > drm/msm/dpu: remove dev from RM > drm/msm/dpu: clean up dpu_rm_check_property_topctl declaration > drm/msm/dpu: remove encoder from crtc mixer struct > drm/msm/dpu: clean up redundant hw type > drm/msm/dpu: reserve using crtc state > drm/msm/dpu: release reservation using crtc state > drm/msm/dpu: make RM iterator static > drm/msm/dpu: maintain hw_mdp in kms > drm/msm/dpu: remove reserve in encoder mode_set > drm/msm/dpu: remove mode_set_complete > drm/msm/dpu: make RM iterator hw type specific > drm/msm/dpu: remove enc_id tagging for hw blocks > drm/msm/dpu: avoid redundant hw blk reference > drm/msm/dpu: clean up test_only flag for RM reservation > drm/msm/dpu: remove RM HW block list iterator > drm/msm/dpu: merge RM interface reservation helpers > drm/msm/dpu: remove msm_display_topology > drm/msm/dpu: refine layer mixer reservations > drm/msm/dpu: merge RM reservation helpers > drm/msm/dpu: make crtc and encoder specific HW reservation > drm/msm/dpu: remove max_width from RM > drm/msm/dpu: remove mutex locking for RM interfaces > drm/msm/dpu: maintain RM init check internally > > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 98 ++- > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h | 16 +- > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c| 96 +-- > .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 20 +- > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c| 16 +- > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h| 1 - > drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 728 > ++--- > drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h | 107 +-- > drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h | 28 +- > drivers/gpu/drm/msm/msm_drv.h | 12 - > 10 files changed, 322 insertions(+), 800 deletions(-) > > -- > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > a Linux Foundation Collaborative Project > -- Sean Paul, Software Engineer, Google / Chromium OS ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH 1/2] drm/msm: dpu: Fix memory leak caused by dropped reference
On Tue, Oct 09, 2018 at 11:43:25AM -0700, Jeykumar Sankaran wrote: > On 2018-10-04 11:09, Sean Paul wrote: > > From: Sean Paul > > > > We are currently leaking a drm_crtc_commit struct for every atomic > > commit containing plane state. The dpu plane destroy function cleans up > dpu plane destroy -> dpu_plane_destroy_state > > the fb reference manually, but fails to release the commit ref. As a > > result, we just keep allocating drm_crtc_commits without ever freeing > > them. Fortunately there's a helper function which will clean up all of > > our mess at once, so use that. > > > > Thanks to Doug Anderson for reporting the memory leak (and leaving > > breadcrumbs from kmemleak!). > > > > Reported-by: Doug Anderson > > Signed-off-by: Sean Paul > > --- > With the nit addressed: FYI, this already picked up in msm-next last week. > > Reviewed-by: Jeykumar Sankaran > > > drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 4 +--- > > 1 file changed, 1 insertion(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c > > index fc59a69aa832..f549daf30fe6 100644 > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c > > @@ -1203,9 +1203,7 @@ static void dpu_plane_destroy_state(struct > > drm_plane > > *plane, > > > > pstate = to_dpu_plane_state(state); > > > > - /* remove ref count for frame buffers */ > > - if (state->fb) > > - drm_framebuffer_put(state->fb); > > + __drm_atomic_helper_plane_destroy_state(state); > > > > kfree(pstate); > > } > > -- > Jeykumar S -- Sean Paul, Software Engineer, Google / Chromium OS ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH 2/2] drm/msm: dpu: Remove checks from dpu_plane_destroy_state()
On 2018-10-04 11:09, Sean Paul wrote: From: Sean Paul They're not needed. Signed-off-by: Sean Paul --- Reviewed-by: Jeykumar Sankaran drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 13 + 1 file changed, 1 insertion(+), 12 deletions(-) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c index f549daf30fe6..4213e7a8e525 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c @@ -1193,19 +1193,8 @@ static void dpu_plane_destroy(struct drm_plane *plane) static void dpu_plane_destroy_state(struct drm_plane *plane, struct drm_plane_state *state) { - struct dpu_plane_state *pstate; - - if (!plane || !state) { - DPU_ERROR("invalid arg(s), plane %d state %d\n", - plane != 0, state != 0); - return; - } - - pstate = to_dpu_plane_state(state); - __drm_atomic_helper_plane_destroy_state(state); - - kfree(pstate); + kfree(to_dpu_plane_state(state)); } static struct drm_plane_state * -- Jeykumar S ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH 1/2] drm/msm: dpu: Fix memory leak caused by dropped reference
On 2018-10-04 11:09, Sean Paul wrote: From: Sean Paul We are currently leaking a drm_crtc_commit struct for every atomic commit containing plane state. The dpu plane destroy function cleans up dpu plane destroy -> dpu_plane_destroy_state the fb reference manually, but fails to release the commit ref. As a result, we just keep allocating drm_crtc_commits without ever freeing them. Fortunately there's a helper function which will clean up all of our mess at once, so use that. Thanks to Doug Anderson for reporting the memory leak (and leaving breadcrumbs from kmemleak!). Reported-by: Doug Anderson Signed-off-by: Sean Paul --- With the nit addressed: Reviewed-by: Jeykumar Sankaran drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c index fc59a69aa832..f549daf30fe6 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c @@ -1203,9 +1203,7 @@ static void dpu_plane_destroy_state(struct drm_plane *plane, pstate = to_dpu_plane_state(state); - /* remove ref count for frame buffers */ - if (state->fb) - drm_framebuffer_put(state->fb); + __drm_atomic_helper_plane_destroy_state(state); kfree(pstate); } -- Jeykumar S ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH 18/25] drm/msm/dpu: merge RM interface reservation helpers
On 2018-10-09 09:50, Jordan Crouse wrote: On Mon, Oct 08, 2018 at 09:27:35PM -0700, Jeykumar Sankaran wrote: we don't have enough reasons why the HW block looping's cannot happen in the same function. So merge them. looping's -> looping. So there are reasons one might break them out but not interesting ones? Not just yet. Once we start supporting different type of connectors such as writeback & DP and the parsing logic for the respective type of INTF grows up, we *may* want to split this up. Thanks Jeykumar S. Signed-off-by: Jeykumar Sankaran --- drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 63 ++ 1 file changed, 26 insertions(+), 37 deletions(-) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c index a79456c..bb59250 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c @@ -435,52 +435,39 @@ static int _dpu_rm_reserve_ctls( return 0; } -static struct dpu_rm_hw_blk *_dpu_rm_reserve_intf( - struct dpu_rm *rm, - uint32_t id, - enum dpu_hw_blk_type type) -{ - struct dpu_rm_hw_blk *iter; - struct list_head *blk_list = &rm->hw_blks[DPU_HW_BLK_INTF]; - - /* Find the block entry in the rm, and note the reservation */ - list_for_each_entry(iter, blk_list, list) { - if (iter->hw->id != id || iter->in_use) - continue; - - trace_dpu_rm_reserve_intf(iter->hw->id, DPU_HW_BLK_INTF); - - break; - } - - /* Shouldn't happen since intfs are fixed at probe */ - if (!iter) { - DPU_ERROR("couldn't find type %d id %d\n", type, id); - return NULL; - } - - return iter; -} - -static int _dpu_rm_reserve_intf_related_hw( +static int _dpu_rm_reserve_intfs( struct dpu_rm *rm, struct dpu_crtc_state *dpu_cstate, struct dpu_encoder_hw_resources *hw_res) { - struct dpu_rm_hw_blk *blk; + struct dpu_rm_hw_blk *iter; + struct list_head *blk_list = &rm->hw_blks[DPU_HW_BLK_INTF]; int i, num_intfs = 0; for (i = 0; i < ARRAY_SIZE(hw_res->intfs); i++) { + struct dpu_rm_hw_blk *intf_blk = NULL; + if (hw_res->intfs[i] == INTF_MODE_NONE) continue; - blk = _dpu_rm_reserve_intf(rm, i + INTF_0, - DPU_HW_BLK_INTF); - if (!blk) - return -ENAVAIL; + list_for_each_entry(iter, blk_list, list) { + if (iter->in_use) + continue; + + if (iter->hw->id == (INTF_0 + i)) { + intf_blk = iter; + break; + } + } + + if (!intf_blk) + return -EINVAL; - blk->in_use = true; - dpu_cstate->hw_intfs[num_intfs++] = to_dpu_hw_intf(blk->hw); + intf_blk->in_use = true; + dpu_cstate->hw_intfs[num_intfs++] = + to_dpu_hw_intf(intf_blk->hw); + + trace_dpu_rm_reserve_intf(intf_blk->hw->id, DPU_HW_BLK_INTF); } dpu_cstate->num_intfs = num_intfs; @@ -507,9 +494,11 @@ static int _dpu_rm_make_reservation( return ret; } - ret = _dpu_rm_reserve_intf_related_hw(rm, dpu_cstate, &reqs->hw_res); - if (ret) + ret = _dpu_rm_reserve_intfs(rm, dpu_cstate, &reqs->hw_res); + if (ret) { + DPU_ERROR("unable to find appropriate INTF\n"); Since there is only once consumer of this function, I would move this error message down into the sub-function and provide more debug information - like which INTF wasn't found. return ret; + } And you don't need to return ret in this block - you can just drop out to the bottom. return ret; } -- Jeykumar S ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v3] drm/msm: validate display and event threads
On 2018-10-09 07:24, Sean Paul wrote: On Mon, Oct 08, 2018 at 04:55:45PM -0700, Jeykumar Sankaran wrote: While creating display and event threads per crtc, validate them before setting their priorities. changes in v2: - use dev_warn (Abhinav Kumar) changes in v3: - fix compilation error Change-Id: I1dda805286df981c0f0e2b26507d089d3a21ff6c No Change-Id's please! FWIW, checkpatch.pl would have caught this and the nits I found below. It might be worth running patches through it before sending. Signed-off-by: Jeykumar Sankaran --- drivers/gpu/drm/msm/msm_drv.c | 49 ++- 1 file changed, 16 insertions(+), 33 deletions(-) diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c index 4904d0d..ab1b0a9 100644 --- a/drivers/gpu/drm/msm/msm_drv.c +++ b/drivers/gpu/drm/msm/msm_drv.c @@ -553,17 +553,18 @@ static int msm_drm_init(struct device *dev, struct drm_driver *drv) kthread_run(kthread_worker_fn, &priv->disp_thread[i].worker, "crtc_commit:%d", priv->disp_thread[i].crtc_id); - ret = sched_setscheduler(priv->disp_thread[i].thread, - SCHED_FIFO, ¶m); - if (ret) - pr_warn("display thread priority update failed: %d\n", - ret); - if (IS_ERR(priv->disp_thread[i].thread)) { dev_err(dev, "failed to create crtc_commit kthread\n"); priv->disp_thread[i].thread = NULL; + goto err_msm_uninit; } + ret = sched_setscheduler(priv->disp_thread[i].thread, +SCHED_FIFO, ¶m); + if (ret) + dev_warn(dev, "display thread priority update failed: %d\n", Although this is wrapped, the line still exceeds 80 chars. Perhaps: dev_warn(dev, "disp_thread set priority failed %d\n", ret); I did run the checkpath.pl on this patch. Check patch usually will not complain if the >80 exceeding line is the logging string. https://01.org/linuxgraphics/gfx-docs/drm/process/coding-style.html#breaking-long-lines-and-strings https://gitlab.freedesktop.org/seanpaul/dpu-staging/blob/for-next/scripts/checkpatch.pl#L3058 Anyway, rewording will be a good idea to avoid nits! + ret); + /* initialize event thread */ priv->event_thread[i].crtc_id = priv->crtcs[i]->base.id; kthread_init_worker(&priv->event_thread[i].worker); @@ -572,6 +573,12 @@ static int msm_drm_init(struct device *dev, struct drm_driver *drv) kthread_run(kthread_worker_fn, &priv->event_thread[i].worker, "crtc_event:%d", priv->event_thread[i].crtc_id); + if (IS_ERR(priv->event_thread[i].thread)) { + dev_err(dev, "failed to create crtc_event kthread\n"); + priv->event_thread[i].thread = NULL; + goto err_msm_uninit; + } + /** * event thread should also run at same priority as disp_thread * because it is handling frame_done events. A lower priority @@ -580,34 +587,10 @@ static int msm_drm_init(struct device *dev, struct drm_driver *drv) * failure at crtc commit level. */ ret = sched_setscheduler(priv->event_thread[i].thread, - SCHED_FIFO, ¶m); +SCHED_FIFO, ¶m); if (ret) - pr_warn("display event thread priority update failed: %d\n", - ret); - - if (IS_ERR(priv->event_thread[i].thread)) { - dev_err(dev, "failed to create crtc_event kthread\n"); - priv->event_thread[i].thread = NULL; - } - - if ((!priv->disp_thread[i].thread) || - !priv->event_thread[i].thread) { - /* clean up previously created threads if any */ - for ( ; i >= 0; i--) { - if (priv->disp_thread[i].thread) { - kthread_stop( - priv->disp_thread[i].thread); - priv->disp_thread[i].thread = NULL; - } - - if (priv->event_thread[i].thread) { - kthread_stop( - priv->event_thread[i].thread); - priv->event_thread[i].thread = NULL; - } - } -
Re: [Freedreno] [PATCH 01/25] drm/msm/dpu: fix hw ctl retrieval for mixer muxing
On Mon, Oct 08, 2018 at 09:27:18PM -0700, Jeykumar Sankaran wrote: > Layer mixer/pingpong block counts and hw ctl block counts > will not be same for all the topologies (e.g. layer > mixer muxing to single interface) > > Use the encoder's split_role info to retrieve the > respective control path for programming. > > Signed-off-by: Jeykumar Sankaran > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 9 ++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > index 96cdf06..d12f896 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > @@ -1060,6 +1060,7 @@ static void dpu_encoder_virt_mode_set(struct > drm_encoder *drm_enc, > > for (i = 0; i < dpu_enc->num_phys_encs; i++) { > struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i]; > + int ctl_index; > > if (phys) { > if (!dpu_enc->hw_pp[i]) { > @@ -1068,14 +1069,16 @@ static void dpu_encoder_virt_mode_set(struct > drm_encoder *drm_enc, > return; > } > > - if (!hw_ctl[i]) { > + ctl_index = phys->split_role == ENC_ROLE_SLAVE ? 1 : 0; > + What if MAX_CHANNELS_PER_ENC isn't 2? Similarly, what if num_phys_encs > MAX_CHANNELS_PER_ENC? It seems like there should be a more formal relationship between all of these verious values (num_of_h_tiles assumed to be <= 2 as well). If one of them changes beyond the assumed bound, the rest of the driver falls over pretty hard. > + if (!hw_ctl[ctl_index]) { > DPU_ERROR_ENC(dpu_enc, "no ctl block assigned" > - "at idx: %d\n", i); > + "at idx: %d\n", ctl_index); > return; When you return on error here, should you give back the resources that you've already provisioned? > } > > phys->hw_pp = dpu_enc->hw_pp[i]; > - phys->hw_ctl = hw_ctl[i]; > + phys->hw_ctl = hw_ctl[ctl_index]; > > phys->connector = conn->state->connector; > if (phys->ops.mode_set) > -- > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > a Linux Foundation Collaborative Project > > ___ > Freedreno mailing list > Freedreno@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/freedreno -- Sean Paul, Software Engineer, Google / Chromium OS ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH 18/25] drm/msm/dpu: merge RM interface reservation helpers
On Mon, Oct 08, 2018 at 09:27:35PM -0700, Jeykumar Sankaran wrote: > we don't have enough reasons why the HW block looping's > cannot happen in the same function. So merge them. looping's -> looping. So there are reasons one might break them out but not interesting ones? > Signed-off-by: Jeykumar Sankaran > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 63 > ++ > 1 file changed, 26 insertions(+), 37 deletions(-) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c > b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c > index a79456c..bb59250 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c > @@ -435,52 +435,39 @@ static int _dpu_rm_reserve_ctls( > return 0; > } > > -static struct dpu_rm_hw_blk *_dpu_rm_reserve_intf( > - struct dpu_rm *rm, > - uint32_t id, > - enum dpu_hw_blk_type type) > -{ > - struct dpu_rm_hw_blk *iter; > - struct list_head *blk_list = &rm->hw_blks[DPU_HW_BLK_INTF]; > - > - /* Find the block entry in the rm, and note the reservation */ > - list_for_each_entry(iter, blk_list, list) { > - if (iter->hw->id != id || iter->in_use) > - continue; > - > - trace_dpu_rm_reserve_intf(iter->hw->id, DPU_HW_BLK_INTF); > - > - break; > - } > - > - /* Shouldn't happen since intfs are fixed at probe */ > - if (!iter) { > - DPU_ERROR("couldn't find type %d id %d\n", type, id); > - return NULL; > - } > - > - return iter; > -} > - > -static int _dpu_rm_reserve_intf_related_hw( > +static int _dpu_rm_reserve_intfs( > struct dpu_rm *rm, > struct dpu_crtc_state *dpu_cstate, > struct dpu_encoder_hw_resources *hw_res) > { > - struct dpu_rm_hw_blk *blk; > + struct dpu_rm_hw_blk *iter; > + struct list_head *blk_list = &rm->hw_blks[DPU_HW_BLK_INTF]; > int i, num_intfs = 0; > > for (i = 0; i < ARRAY_SIZE(hw_res->intfs); i++) { > + struct dpu_rm_hw_blk *intf_blk = NULL; > + > if (hw_res->intfs[i] == INTF_MODE_NONE) > continue; > > - blk = _dpu_rm_reserve_intf(rm, i + INTF_0, > - DPU_HW_BLK_INTF); > - if (!blk) > - return -ENAVAIL; > + list_for_each_entry(iter, blk_list, list) { > + if (iter->in_use) > + continue; > + > + if (iter->hw->id == (INTF_0 + i)) { > + intf_blk = iter; > + break; > + } > + } > + > + if (!intf_blk) > + return -EINVAL; > > - blk->in_use = true; > - dpu_cstate->hw_intfs[num_intfs++] = to_dpu_hw_intf(blk->hw); > + intf_blk->in_use = true; > + dpu_cstate->hw_intfs[num_intfs++] = > + to_dpu_hw_intf(intf_blk->hw); > + > + trace_dpu_rm_reserve_intf(intf_blk->hw->id, DPU_HW_BLK_INTF); > } > > dpu_cstate->num_intfs = num_intfs; > @@ -507,9 +494,11 @@ static int _dpu_rm_make_reservation( > return ret; > } > > - ret = _dpu_rm_reserve_intf_related_hw(rm, dpu_cstate, &reqs->hw_res); > - if (ret) > + ret = _dpu_rm_reserve_intfs(rm, dpu_cstate, &reqs->hw_res); > + if (ret) { > + DPU_ERROR("unable to find appropriate INTF\n"); Since there is only once consumer of this function, I would move this error message down into the sub-function and provide more debug information - like which INTF wasn't found. > return ret; > + } And you don't need to return ret in this block - you can just drop out to the bottom. > > return ret; > } -- The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH 10/25] drm/msm/dpu: maintain hw_mdp in kms
On Mon, Oct 08, 2018 at 09:27:27PM -0700, Jeykumar Sankaran wrote: > hw_mdp block is common for displays. No need > to reserve per display. > > Signed-off-by: Jeykumar Sankaran > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 7 ++- > drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 20 > drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h | 10 -- > 3 files changed, 6 insertions(+), 31 deletions(-) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > index 8309850..fdc89a8 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > @@ -689,6 +689,10 @@ static void _dpu_kms_hw_destroy(struct dpu_kms *dpu_kms) > devm_iounmap(&dpu_kms->pdev->dev, dpu_kms->vbif[VBIF_RT]); > dpu_kms->vbif[VBIF_RT] = NULL; > > + if (dpu_kms->hw_mdp) > + dpu_hw_mdp_destroy(dpu_kms->hw_mdp); > + dpu_kms->hw_mdp = NULL; > + > if (dpu_kms->mmio) > devm_iounmap(&dpu_kms->pdev->dev, dpu_kms->mmio); > dpu_kms->mmio = NULL; > @@ -1083,7 +1087,8 @@ static int dpu_kms_hw_init(struct msm_kms *kms) > > dpu_kms->rm_init = true; > > - dpu_kms->hw_mdp = dpu_rm_get_mdp(&dpu_kms->rm); > + dpu_kms->hw_mdp = dpu_hw_mdptop_init(MDP_TOP, dpu_kms->mmio, > + dpu_kms->catalog); > if (IS_ERR_OR_NULL(dpu_kms->hw_mdp)) { dpu_hw_mdptop_init() only returns ERR_PTR so you can change this over to IS_ERR(). > rc = PTR_ERR(dpu_kms->hw_mdp); > if (!dpu_kms->hw_mdp) And then you don't need this and whatever comes along below it. > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c > b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c > index 24fc1c7..561120d 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c > @@ -63,11 +63,6 @@ struct dpu_rm_hw_iter { > enum dpu_hw_blk_type type; > }; > > -struct dpu_hw_mdp *dpu_rm_get_mdp(struct dpu_rm *rm) > -{ > - return rm->hw_mdp; > -} > - > static void _dpu_rm_init_hw_iter( > struct dpu_rm_hw_iter *iter, > uint32_t enc_id, > @@ -151,9 +146,6 @@ int dpu_rm_destroy(struct dpu_rm *rm) > } > } > > - dpu_hw_mdp_destroy(rm->hw_mdp); > - rm->hw_mdp = NULL; > - > mutex_destroy(&rm->rm_lock); > > return 0; > @@ -168,11 +160,8 @@ static int _dpu_rm_hw_blk_create( > void *hw_catalog_info) > { > struct dpu_rm_hw_blk *blk; > - struct dpu_hw_mdp *hw_mdp; > void *hw; > > - hw_mdp = rm->hw_mdp; > - > switch (type) { > case DPU_HW_BLK_LM: > hw = dpu_hw_lm_init(id, mmio, cat); > @@ -236,15 +225,6 @@ int dpu_rm_init(struct dpu_rm *rm, > for (type = 0; type < DPU_HW_BLK_MAX; type++) > INIT_LIST_HEAD(&rm->hw_blks[type]); > > - /* Some of the sub-blocks require an mdptop to be created */ > - rm->hw_mdp = dpu_hw_mdptop_init(MDP_TOP, mmio, cat); > - if (IS_ERR_OR_NULL(rm->hw_mdp)) { > - rc = PTR_ERR(rm->hw_mdp); > - rm->hw_mdp = NULL; > - DPU_ERROR("failed: mdp hw not available\n"); > - goto fail; > - } > - > /* Interrogate HW catalog and create tracking items for hw blocks */ > for (i = 0; i < cat->mixer_count; i++) { > struct dpu_lm_cfg *lm = &cat->mixer[i]; > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h > b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h > index c7e3b2b..7ac1553 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h > @@ -24,13 +24,11 @@ > * struct dpu_rm - DPU dynamic hardware resource manager > * @hw_blks: array of lists of hardware resources present in the system, one > * list per type of hardware block > - * @hw_mdp: hardware object for mdp_top > * @lm_max_width: cached layer mixer maximum width > * @rm_lock: resource manager mutex > */ > struct dpu_rm { > struct list_head hw_blks[DPU_HW_BLK_MAX]; > - struct dpu_hw_mdp *hw_mdp; > uint32_t lm_max_width; > struct mutex rm_lock; > }; > @@ -82,12 +80,4 @@ int dpu_rm_reserve(struct dpu_rm *rm, > * @Return: 0 on Success otherwise -ERROR > */ > void dpu_rm_release(struct dpu_rm *rm, struct drm_crtc_state *crtc_state); > - > -/** > - * dpu_rm_get_mdp - Retrieve HW block for MDP TOP. > - * This is never reserved, and is usable by any display. > - * @rm: DPU Resource Manager handle > - * @Return: Pointer to hw block or NULL > - */ > -struct dpu_hw_mdp *dpu_rm_get_mdp(struct dpu_rm *rm); > #endif /* __DPU_RM_H__ */ -- The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH 01/25] drm/msm/dpu: fix hw ctl retrieval for mixer muxing
On Mon, Oct 08, 2018 at 09:27:18PM -0700, Jeykumar Sankaran wrote: > Layer mixer/pingpong block counts and hw ctl block counts > will not be same for all the topologies (e.g. layer > mixer muxing to single interface) > > Use the encoder's split_role info to retrieve the > respective control path for programming. > > Signed-off-by: Jeykumar Sankaran > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 9 ++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > index 96cdf06..d12f896 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > @@ -1060,6 +1060,7 @@ static void dpu_encoder_virt_mode_set(struct > drm_encoder *drm_enc, > > for (i = 0; i < dpu_enc->num_phys_encs; i++) { > struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i]; > + int ctl_index; > > if (phys) { > if (!dpu_enc->hw_pp[i]) { > @@ -1068,14 +1069,16 @@ static void dpu_encoder_virt_mode_set(struct > drm_encoder *drm_enc, > return; > } > > - if (!hw_ctl[i]) { > + ctl_index = phys->split_role == ENC_ROLE_SLAVE ? 1 : 0; > + > + if (!hw_ctl[ctl_index]) { > DPU_ERROR_ENC(dpu_enc, "no ctl block assigned" > - "at idx: %d\n", i); > + "at idx: %d\n", ctl_index); I know you are only updating the previous code but we shouldn't be splitting the string here for grep purposes. > return; > } > > phys->hw_pp = dpu_enc->hw_pp[i]; > - phys->hw_ctl = hw_ctl[i]; > + phys->hw_ctl = hw_ctl[ctl_index]; > > phys->connector = conn->state->connector; > if (phys->ops.mode_set) -- The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v2] drm/msm: validate display and event threads
Hi Jeykumar, Thank you for the patch! Yet something to improve: [auto build test ERROR on robclark/msm-next] [also build test ERROR on v4.19-rc7 next-20181009] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Jeykumar-Sankaran/drm-msm-validate-display-and-event-threads/20181009-203659 base: git://people.freedesktop.org/~robclark/linux msm-next config: arm-allmodconfig (attached as .config) compiler: arm-linux-gnueabi-gcc (Debian 7.2.0-11) 7.2.0 reproduce: wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree GCC_VERSION=7.2.0 make.cross ARCH=arm All error/warnings (new ones prefixed by >>): In file included from include/linux/hdmi.h:28:0, from include/drm/drm_modes.h:30, from include/drm/drm_bridge.h:29, from include/drm/drm_of.h:7, from drivers/gpu//drm/msm/msm_drv.c:21: drivers/gpu//drm/msm/msm_drv.c: In function 'msm_drm_init': >> drivers/gpu//drm/msm/msm_drv.c:565:13: error: passing argument 1 of >> '_dev_warn' from incompatible pointer type >> [-Werror=incompatible-pointer-types] dev_warn("display thread priority update failed: %d\n", ^ include/linux/device.h:1420:12: note: in definition of macro 'dev_warn' _dev_warn(dev, dev_fmt(fmt), ##__VA_ARGS__) ^~~ include/linux/device.h:1358:6: note: expected 'const struct device *' but argument is of type 'char *' void _dev_warn(const struct device *dev, const char *fmt, ...); ^ >> drivers/gpu//drm/msm/msm_drv.c:566:5: warning: passing argument 2 of >> '_dev_warn' makes pointer from integer without a cast [-Wint-conversion] ret); ^ include/linux/device.h:1335:22: note: in definition of macro 'dev_fmt' #define dev_fmt(fmt) fmt ^~~ >> drivers/gpu//drm/msm/msm_drv.c:565:4: note: in expansion of macro 'dev_warn' dev_warn("display thread priority update failed: %d\n", ^~~~ include/linux/device.h:1358:6: note: expected 'const char *' but argument is of type 'int' void _dev_warn(const struct device *dev, const char *fmt, ...); ^ drivers/gpu//drm/msm/msm_drv.c:592:13: error: passing argument 1 of '_dev_warn' from incompatible pointer type [-Werror=incompatible-pointer-types] dev_warn("display event thread priority update failed:%d\n", ^ include/linux/device.h:1420:12: note: in definition of macro 'dev_warn' _dev_warn(dev, dev_fmt(fmt), ##__VA_ARGS__) ^~~ include/linux/device.h:1358:6: note: expected 'const struct device *' but argument is of type 'char *' void _dev_warn(const struct device *dev, const char *fmt, ...); ^ drivers/gpu//drm/msm/msm_drv.c:593:5: warning: passing argument 2 of '_dev_warn' makes pointer from integer without a cast [-Wint-conversion] ret); ^ include/linux/device.h:1335:22: note: in definition of macro 'dev_fmt' #define dev_fmt(fmt) fmt ^~~ drivers/gpu//drm/msm/msm_drv.c:592:4: note: in expansion of macro 'dev_warn' dev_warn("display event thread priority update failed:%d\n", ^~~~ include/linux/device.h:1358:6: note: expected 'const char *' but argument is of type 'int' void _dev_warn(const struct device *dev, const char *fmt, ...); ^ cc1: some warnings being treated as errors vim +/dev_warn +565 drivers/gpu//drm/msm/msm_drv.c 433 434 static int msm_drm_init(struct device *dev, struct drm_driver *drv) 435 { 436 struct platform_device *pdev = to_platform_device(dev); 437 struct drm_device *ddev; 438 struct msm_drm_private *priv; 439 struct msm_kms *kms; 440 struct msm_mdss *mdss; 441 int ret, i; 442 struct sched_param param; 443 444 ddev = drm_dev_alloc(drv, dev); 445 if (IS_ERR(ddev)) { 446 dev_err(dev, "failed to allocate drm_device\n"); 447 return PTR_ERR(ddev); 448 } 449 450 platform_set_drvdata(pdev, ddev); 451 452 priv = kzalloc(sizeof(*priv), GFP_KERNEL); 453 if (!priv) { 454 ret = -ENOMEM; 455 goto err_put_drm_dev; 456 } 457 458 ddev->dev_private = priv; 459 priv->dev = ddev
Re: [Freedreno] [PATCH v3] drm/msm: validate display and event threads
On Mon, Oct 08, 2018 at 04:55:45PM -0700, Jeykumar Sankaran wrote: > While creating display and event threads per crtc, validate > them before setting their priorities. > > changes in v2: > - use dev_warn (Abhinav Kumar) > changes in v3: > - fix compilation error > > Change-Id: I1dda805286df981c0f0e2b26507d089d3a21ff6c No Change-Id's please! FWIW, checkpatch.pl would have caught this and the nits I found below. It might be worth running patches through it before sending. > Signed-off-by: Jeykumar Sankaran > --- > drivers/gpu/drm/msm/msm_drv.c | 49 > ++- > 1 file changed, 16 insertions(+), 33 deletions(-) > > diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c > index 4904d0d..ab1b0a9 100644 > --- a/drivers/gpu/drm/msm/msm_drv.c > +++ b/drivers/gpu/drm/msm/msm_drv.c > @@ -553,17 +553,18 @@ static int msm_drm_init(struct device *dev, struct > drm_driver *drv) > kthread_run(kthread_worker_fn, > &priv->disp_thread[i].worker, > "crtc_commit:%d", priv->disp_thread[i].crtc_id); > - ret = sched_setscheduler(priv->disp_thread[i].thread, > - SCHED_FIFO, ¶m); > - if (ret) > - pr_warn("display thread priority update failed: %d\n", > - ret); > - > if (IS_ERR(priv->disp_thread[i].thread)) { > dev_err(dev, "failed to create crtc_commit kthread\n"); > priv->disp_thread[i].thread = NULL; > + goto err_msm_uninit; > } > > + ret = sched_setscheduler(priv->disp_thread[i].thread, > + SCHED_FIFO, ¶m); > + if (ret) > + dev_warn(dev, "display thread priority update failed: > %d\n", Although this is wrapped, the line still exceeds 80 chars. Perhaps: dev_warn(dev, "disp_thread set priority failed %d\n", ret); > + ret); > + > /* initialize event thread */ > priv->event_thread[i].crtc_id = priv->crtcs[i]->base.id; > kthread_init_worker(&priv->event_thread[i].worker); > @@ -572,6 +573,12 @@ static int msm_drm_init(struct device *dev, struct > drm_driver *drv) > kthread_run(kthread_worker_fn, > &priv->event_thread[i].worker, > "crtc_event:%d", priv->event_thread[i].crtc_id); > + if (IS_ERR(priv->event_thread[i].thread)) { > + dev_err(dev, "failed to create crtc_event kthread\n"); > + priv->event_thread[i].thread = NULL; > + goto err_msm_uninit; > + } > + > /** >* event thread should also run at same priority as disp_thread >* because it is handling frame_done events. A lower priority > @@ -580,34 +587,10 @@ static int msm_drm_init(struct device *dev, struct > drm_driver *drv) >* failure at crtc commit level. >*/ > ret = sched_setscheduler(priv->event_thread[i].thread, > - SCHED_FIFO, ¶m); > + SCHED_FIFO, ¶m); > if (ret) > - pr_warn("display event thread priority update failed: > %d\n", > - ret); > - > - if (IS_ERR(priv->event_thread[i].thread)) { > - dev_err(dev, "failed to create crtc_event kthread\n"); > - priv->event_thread[i].thread = NULL; > - } > - > - if ((!priv->disp_thread[i].thread) || > - !priv->event_thread[i].thread) { > - /* clean up previously created threads if any */ > - for ( ; i >= 0; i--) { > - if (priv->disp_thread[i].thread) { > - kthread_stop( > - priv->disp_thread[i].thread); > - priv->disp_thread[i].thread = NULL; > - } > - > - if (priv->event_thread[i].thread) { > - kthread_stop( > - priv->event_thread[i].thread); > - priv->event_thread[i].thread = NULL; > - } > - } > - goto err_msm_uninit; > - } > + dev_warn(dev, "display event thread priority update > failed:%d\n", Same problem here,
Re: [Freedreno] [PATCH] drm/msm: Grab a vblank reference when waiting for commit_done
On Mon, Oct 08, 2018 at 06:38:11PM -0700, Abhinav Kumar wrote: > On 2018-10-03 13:22, Sean Paul wrote: > > From: Sean Paul > > > > Similar to the atomic helpers, we should enable vblank while we're > > waiting for the commit to finish. DPU needs this, MDP5 seems to work > > fine without it. > > > > Signed-off-by: Sean Paul > > --- > As such I dont see any issue with this patch but I have a question overall > on chrome vblank handling. > For a video mode panel, we will keep the HW resources enabled (including > vsync related clocks) till device > is suspended. > > The vblank_get and vblank_put are only controlling the register/deregister > of the vblank IRQ callback which > send the events to the userspace and anything pending on vsync completion. > > Does the chrome userspace turn ON/OFF the vblank using vblank_ctrl IOCTL Or > does it guarantee it to be? > No not explicitly. The issue I was seeing is that when userspace (this issue is not specific to chrome) does a commit we get a frame_done timeout. So while the hardware might still be active, the worker to signal frame_done is not unless the vblank_ref is > 0. > If so, is this patch just more of a safety check to make sure that vsync > events remain ON till the frame is done? > > Because HW wise it should be and this shouldnt be needed. It is definitely needed, unless you want 60ms (the frame_done timeout) between frames :-) Sean > > > drivers/gpu/drm/msm/msm_atomic.c | 5 + > > 1 file changed, 5 insertions(+) > > > > diff --git a/drivers/gpu/drm/msm/msm_atomic.c > > b/drivers/gpu/drm/msm/msm_atomic.c > > index c1f1779c980f..2b7bb6e166d3 100644 > > --- a/drivers/gpu/drm/msm/msm_atomic.c > > +++ b/drivers/gpu/drm/msm/msm_atomic.c > > @@ -32,7 +32,12 @@ static void msm_atomic_wait_for_commit_done(struct > > drm_device *dev, > > if (!new_crtc_state->active) > > continue; > > > > + if (drm_crtc_vblank_get(crtc)) > > + continue; > > + > > kms->funcs->wait_for_crtc_commit_done(kms, crtc); > > + > > + drm_crtc_vblank_put(crtc); > > } > > } -- Sean Paul, Software Engineer, Google / Chromium OS ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH 1/3] drm/msm/dpu: clean up references of DPU custom bus scaling
On 2018-10-08 14:57, Sravanthi Kollukuduru wrote: Since the upstream interconnect bus framework has landed upstream, the existing references of custom bus scaling needs to be cleaned up. Signed-off-by: Sravanthi Kollukuduru --- drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c| 157 ++- drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h| 4 +- drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 1 - drivers/gpu/drm/msm/disp/dpu1/dpu_power_handle.c | 47 ++- drivers/gpu/drm/msm/disp/dpu1/dpu_power_handle.h | 68 -- drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h| 21 +-- 6 files changed, 83 insertions(+), 215 deletions(-) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c index 41c5191f9056..4ee6f0dd14f7 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c @@ -90,7 +90,6 @@ static void _dpu_core_perf_calc_crtc(struct dpu_kms *kms, struct dpu_core_perf_params *perf) { struct dpu_crtc_state *dpu_cstate; - int i; if (!kms || !kms->catalog || !crtc || !state || !perf) { DPU_ERROR("invalid parameters\n"); @@ -101,35 +100,24 @@ static void _dpu_core_perf_calc_crtc(struct dpu_kms *kms, memset(perf, 0, sizeof(struct dpu_core_perf_params)); if (!dpu_cstate->bw_control) { - for (i = 0; i < DPU_POWER_HANDLE_DBUS_ID_MAX; i++) { - perf->bw_ctl[i] = kms->catalog->perf.max_bw_high * + perf->bw_ctl = kms->catalog->perf.max_bw_high * 1000ULL; - perf->max_per_pipe_ib[i] = perf->bw_ctl[i]; - } + perf->max_per_pipe_ib = perf->bw_ctl; perf->core_clk_rate = kms->perf.max_core_clk_rate; } else if (kms->perf.perf_tune.mode == DPU_PERF_MODE_MINIMUM) { - for (i = 0; i < DPU_POWER_HANDLE_DBUS_ID_MAX; i++) { - perf->bw_ctl[i] = 0; - perf->max_per_pipe_ib[i] = 0; - } + perf->bw_ctl = 0; + perf->max_per_pipe_ib = 0; perf->core_clk_rate = 0; } else if (kms->perf.perf_tune.mode == DPU_PERF_MODE_FIXED) { - for (i = 0; i < DPU_POWER_HANDLE_DBUS_ID_MAX; i++) { - perf->bw_ctl[i] = kms->perf.fix_core_ab_vote; - perf->max_per_pipe_ib[i] = kms->perf.fix_core_ib_vote; - } + perf->bw_ctl = kms->perf.fix_core_ab_vote; + perf->max_per_pipe_ib = kms->perf.fix_core_ib_vote; perf->core_clk_rate = kms->perf.fix_core_clk_rate; } DPU_DEBUG( - "crtc=%d clk_rate=%llu core_ib=%llu core_ab=%llu llcc_ib=%llu llcc_ab=%llu mem_ib=%llu mem_ab=%llu\n", + "crtc=%d clk_rate=%llu core_ib=%llu core_ab=%llu\n", crtc->base.id, perf->core_clk_rate, - perf->max_per_pipe_ib[DPU_POWER_HANDLE_DBUS_ID_MNOC], - perf->bw_ctl[DPU_POWER_HANDLE_DBUS_ID_MNOC], - perf->max_per_pipe_ib[DPU_POWER_HANDLE_DBUS_ID_LLCC], - perf->bw_ctl[DPU_POWER_HANDLE_DBUS_ID_LLCC], - perf->max_per_pipe_ib[DPU_POWER_HANDLE_DBUS_ID_EBI], - perf->bw_ctl[DPU_POWER_HANDLE_DBUS_ID_EBI]); + perf->max_per_pipe_ib, perf->bw_ctl); } int dpu_core_perf_crtc_check(struct drm_crtc *crtc, @@ -142,7 +130,6 @@ int dpu_core_perf_crtc_check(struct drm_crtc *crtc, struct dpu_crtc_state *dpu_cstate; struct drm_crtc *tmp_crtc; struct dpu_kms *kms; - int i; if (!crtc || !state) { DPU_ERROR("invalid crtc\n"); @@ -164,31 +151,28 @@ int dpu_core_perf_crtc_check(struct drm_crtc *crtc, /* obtain new values */ _dpu_core_perf_calc_crtc(kms, crtc, state, &dpu_cstate->new_perf); - for (i = DPU_POWER_HANDLE_DBUS_ID_MNOC; - i < DPU_POWER_HANDLE_DBUS_ID_MAX; i++) { - bw_sum_of_intfs = dpu_cstate->new_perf.bw_ctl[i]; - curr_client_type = dpu_crtc_get_client_type(crtc); + bw_sum_of_intfs = dpu_cstate->new_perf.bw_ctl; + curr_client_type = dpu_crtc_get_client_type(crtc); - drm_for_each_crtc(tmp_crtc, crtc->dev) { - if (_dpu_core_perf_crtc_is_power_on(tmp_crtc) && - (dpu_crtc_get_client_type(tmp_crtc) == - curr_client_type) && - (tmp_crtc != crtc)) { - struct dpu_crtc_state *tmp_cstate = - to_dpu_crtc_state(tmp_crtc->state); - - DPU_DEBUG("crtc:%d bw:%llu ctrl:%d\n", - tmp_crtc->base.id,