Re: [DPU PATCH 2/2] drm/msm: Add hardware catalog file for SDM845

[Sean Paul <seanpaul@chromium.org>]

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 <skolluku@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 />
-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel