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

[skolluku-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org]

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 <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 />

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
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno