Re: [PATCH 1/7] drm/komeda: Add d71_enum_resources and d71_cleanup

["james qian wang (Arm Technology China)" <james.qian.wang@arm.com>]

On 2019/1/7, 9:41 PM, "Brian Starkey" <Brian.Starkey@arm.com> wrote:

    Hi James,
    
    A few minor comments.
    
    On Mon, Dec 24, 2018 at 08:58:46AM +0000, james qian wang (Arm Technology China) wrote:
    > D71 consists of a number of Register Blocks, every Block controls a
    > specific HW function, every block has a common block_header to represent
    > its type and pipeline information.
    > 
    > GCU (Global Control Unit) is the first Block which describe the global
    > information of D71 HW, Like number of block contained and the number of
    > pipeline supported.
    > 
    > So the d71_enum_resources parsed GCU and create pipeline according
    > the GCU configuration, and then iterate and detect the blocks that
    > indicated by the GCU and block_header.
    > 
    > And this change also added two struct d71_dev/d71_pipeline to extend
    > komeda_dev/komeda_pipeline to add some d71 only members.
    > 
    > Signed-off-by: James (Qian) Wang <james.qian.wang@arm.com>
    > ---
    
    -- snip --
    
    > diff --git a/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c b/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
    > new file mode 100644
    > index 000000000000..a43a2410159f
    > --- /dev/null
    > +++ b/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
    > @@ -0,0 +1,120 @@
    > +// SPDX-License-Identifier: GPL-2.0
    > +/*
    > + * (C) COPYRIGHT 2018 ARM Limited. All rights reserved.
    > + * Author: James.Qian.Wang <james.qian.wang@arm.com>
    > + *
    > + */
    > +#include "d71_dev.h"
    > +#include "komeda_kms.h"
    > +#include "malidp_io.h"
    > +
    > +static int d71_layer_init(struct d71_dev *d71,
    > +			  struct block_header *blk, u32 __iomem *reg)
    > +{
    > +	DRM_INFO("Detect D71_Layer.\n");
    
    I think all of these can be DRM_DEBUG.
    
    -- snip --

Will fix it on the next version.
    
    >  
    >  static int d71_enum_resources(struct komeda_dev *mdev)
    >  {
    > -	/* TODO add enum resources */
    > +	struct d71_dev *d71;
    > +	struct komeda_pipeline *pipe;
    > +	struct block_header blk;
    > +	u32 __iomem *blk_base;
    > +	u32 i, value, offset;
    > +
    > +	d71 = devm_kzalloc(mdev->dev, sizeof(*d71), GFP_KERNEL);
    > +	if (!d71)
    > +		return -ENOMEM;
    > +
    > +	mdev->chip_data = d71;
    > +	d71->mdev = mdev;
    > +	d71->gcu_addr = mdev->reg_base;
    > +	d71->periph_addr = mdev->reg_base + (D71_BLOCK_OFFSET_PERIPH >> 2);
    > +
    > +	if (d71_reset(d71)) {
    > +		DRM_ERROR("Fail to reset d71 device.\n");
    > +		goto err_cleanup;
    > +	}
    > +
    > +	/* probe GCU */
    > +	value = malidp_read32(d71->gcu_addr, GLB_CORE_INFO);
    > +	d71->num_blocks = value & 0xFF;
    > +	d71->num_pipelines = (value >> 8) & 0x7;
    > +
    > +	if (d71->num_pipelines > D71_MAX_PIPELINE) {
    > +		DRM_ERROR("d71 supports %d pipelines, but got: %d.\n",
    > +			  D71_MAX_PIPELINE, d71->num_pipelines);
    > +		goto err_cleanup;
    > +	}
    > +
    > +	/* probe PERIPH */
    > +	value = malidp_read32(d71->periph_addr, BLK_BLOCK_INFO);
    > +	if (BLOCK_INFO_BLK_TYPE(value) != D71_BLK_TYPE_PERIPH) {
    > +		DRM_ERROR("access blk periph but got blk: %d.\n",
    > +			  BLOCK_INFO_BLK_TYPE(value));
    > +		goto err_cleanup;
    > +	}
    > +
    > +	value = malidp_read32(d71->periph_addr, PERIPH_CONFIGURATION_ID);
    > +
    > +	d71->max_line_size	= value & PERIPH_MAX_LINE_SIZE ? 4096 : 2048;
    > +	d71->max_vsize		= 4096;
    > +	d71->num_rich_layers	= value & PERIPH_NUM_RICH_LAYERS ? 2 : 1;
    > +	d71->supports_dual_link	= value & PERIPH_SPLIT_EN ? true : false;
    > +	d71->integrates_tbu	= value & PERIPH_TBU_EN ? true : false;
    > +
    > +	for (i = 0; i < d71->num_pipelines; i++) {
    > +		pipe = komeda_pipeline_add(mdev, sizeof(struct d71_pipeline),
    > +					   NULL);
    > +		if (!pipe)
    > +			goto err_cleanup;
    > +
    > +		d71->pipes[i] = to_d71_pipeline(pipe);
    > +	}
    > +
    > +	/* loop the register blks and probe */
    > +	i = 2; /* exclude GCU and PERIPH */
    > +	offset = D71_BLOCK_SIZE; /* skip GCU */
    > +	while (i < d71->num_blocks) {
    > +		blk_base = mdev->reg_base + (offset >> 2);
    > +
    > +		d71_read_block_header(blk_base, &blk);
    > +		if (BLOCK_INFO_BLK_TYPE(blk.block_info) != D71_BLK_TYPE_RESERVED) {
    > +			if (d71_probe_block(d71, &blk, blk_base))
    > +				goto err_cleanup;
    > +			i++;
    > +		}
    > +
    > +		offset += D71_BLOCK_SIZE;
    > +	}
    > +
    > +	DRM_INFO("total %d (out of %d) blocks are found.\n",
    > +		 i, d71->num_blocks);
    > +
    > +	return 0;
    > +
    > +err_cleanup:
    > +	d71_cleanup(mdev);
    >  	return -1;
    
    -1 isn't a useful error code, and you return -ENOMEM if allocation
    fails. It would probably be better to return proper codes everywhere
    (e.g. you might get -EINVAL back from d71_probe_block, but you don't
    propagate it).
    
    -- snip --

Ok, will fix it in the next version.
    
    > diff --git a/drivers/gpu/drm/arm/display/komeda/d71/d71_regs.h b/drivers/gpu/drm/arm/display/komeda/d71/d71_regs.h
    > new file mode 100644
    > index 000000000000..2d5e6d00b42c
    > --- /dev/null
    > +++ b/drivers/gpu/drm/arm/display/komeda/d71/d71_regs.h
    
    ...
    
    > +
    > +/* Image process registers */
    > +#define IPS_DEPTH		0x0D8
    > +#define IPS_RGB_RGB_COEFF0	0x130
    > +#define IPS_RGB_YUV_COEFF0	0x170
    > +
    > +#define IPS_DEPTH_MARK		0xF
    
    s/MARK/MASK/
    
    Cheers,
    -Brian