On 2018-03-19 19:29, Sean Paul wrote:
On Wed, Mar 14, 2018 at 11:21:38AM +0530, Sravanthi Kollukuduru wrote:
This change adds the hardware catalog information in driver source
for SDM845. This removes the current logic of dt based parsing
of target catalog information.

Signed-off-by: Sravanthi Kollukuduru <skoll...@codeaurora.org>
---
 drivers/gpu/drm/msm/Makefile                       |    1 +
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 3071 +-------------------
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h     |   17 +-
 .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog_sdm845.c  |  744 +++++
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c            |    2 +-
 5 files changed, 767 insertions(+), 3068 deletions(-)

Hi Sravanthi,
Thank you for the patch, it's great to see diffstats like this :)

<snip />

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog_sdm845.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog_sdm845.c
new file mode 100644
index 0000000..3ca5dc7
--- /dev/null
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog_sdm845.c
@@ -0,0 +1,744 @@
+/* Copyright (c) 2018, The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.

We should be using SPDX license tags from now on.

+ */
+
+#include "dpu_hw_catalog.h"
+#include "dpu_hw_catalog_format.h"
+#include "dpu_hw_mdss.h"
+#include "dpu_hwio.h"
+
+/* VIG layer capability */
+#define VIG_40X_MASK \
+ (BIT(DPU_SSPP_SRC) | BIT(DPU_SSPP_SCALER_QSEED3) | BIT(DPU_SSPP_QOS) |\ + BIT(DPU_SSPP_CSC_10BIT) | BIT(DPU_SSPP_CDP) | BIT(DPU_SSPP_QOS_8LVL) |\
+       BIT(DPU_SSPP_TS_PREFILL) | BIT(DPU_SSPP_EXCL_RECT))
+
+/* DMA layer capability */
+#define DMA_40X_MASK \
+       (BIT(DPU_SSPP_SRC) | BIT(DPU_SSPP_QOS) | BIT(DPU_SSPP_QOS_8LVL) |\
+ BIT(DPU_SSPP_TS_PREFILL) | BIT(DPU_SSPP_CDP) | BIT(DPU_SSPP_EXCL_RECT))
+
+#define MIXER_40X_MASK \
+       (BIT(DPU_MIXER_SOURCESPLIT) | BIT(DPU_DIM_LAYER))
+
+#define DSPP_40X_MASK \
+       (BIT(DPU_DSPP_IGC) | BIT(DPU_DSPP_PCC) | BIT(DPU_DSPP_GC) |\
+       BIT(DPU_DSPP_HSIC) | BIT(DPU_DSPP_GAMUT) | BIT(DPU_DSPP_HIST) |\
+       BIT(DPU_DSPP_MEMCOLOR) | BIT(DPU_DSPP_SIXZONE) | BIT(DPU_DSPP_VLUT))
+
+#define DSPP_AD_40X_MASK \
+       (DSPP_40X_MASK | BIT(DPU_DSPP_AD))
+
+#define PINGPONG_40X_MASK BIT(DPU_PINGPONG_DITHER)
+
+#define PINGPONG_40X_SPLIT_MASK \
+ (PINGPONG_40X_MASK | BIT(DPU_PINGPONG_SPLIT) | BIT(DPU_PINGPONG_TE2))
+
+#define WB2_40X_MASK \
+ (BIT(DPU_WB_LINE_MODE) | BIT(DPU_WB_TRAFFIC_SHAPER) | BIT(DPU_WB_CDP) |\
+       BIT(DPU_WB_YUV_CONFIG) | BIT(DPU_WB_QOS_8LVL) | BIT(DPU_WB_UBWC))
+
+#define DECIMATION_40X_MAX_H   4
+#define DECIMATION_40X_MAX_V   4
+
+/*
+ * set_cfg_xxx_init(): populate dpu sub-blocks reg offsets
+ * and instance counts.
+ */
+static inline int set_cfg_40X_init(struct dpu_mdss_cfg *dpu_cfg)
+{
+       /* Layer capability */
+       static const struct dpu_sspp_sub_blks vig_sblk_0 = {
+               .maxlinewidth = 2560,
+               .pixel_ram_size = 50 * 1024,
+               .maxdwnscale = 4,
+               .maxupscale = 20,
+               .maxhdeciexp = DECIMATION_40X_MAX_H,
+               .maxvdeciexp = DECIMATION_40X_MAX_V,
+               .smart_dma_priority = 5,
+               .src_blk = {.name = "sspp_src_0", .id = DPU_SSPP_SRC,
+                       .base = 0x00, .len = 0x150,},
+               .scaler_blk = {.name = "sspp_scaler0",
+                       .id = DPU_SSPP_SCALER_QSEED3,
+                       .base = 0xa00, .len = 0xa0,},
+               .csc_blk = {.name = "sspp_csc0", .id = DPU_SSPP_CSC_10BIT,
+                       .base = 0x1a00, .len = 0x100,},
+               .format_list = plane_formats_yuv,
+               .virt_format_list = plane_formats,
+       };

Instead of locating all of these parameters in one file, these should be located in their respective driver file. It also seems like you could separate
out the common stuff such as line width, ram size, scaling, format, etc
parameters from the pipeline setup.

The same comments apply to the other blocks. Move things into the drivers, use compatibility string to determine the version, and then associate the common
parameters with of_device_id.data.

Sean

<snip />

Thanks Sean for the feedback.
The idea behind this approach is to maintain a one point access for all the
target specific information, analogous to the current dpu dtsi file.
This also ensures easy maintenance for different hardware versions, as all it takes is to add another file instead of updating across individual sub block files.

Also, i'm not quite clear on how compatibility strings is applicable to sub blocks.
Please clarify.

Thanks,
Sravanthi
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Reply via email to