| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 7 Jan 2019 13:41:52 +0000
From: Brian Starkey <Brian.Starkey@....com>
To: "james qian wang (Arm Technology China)" <james.qian.wang@....com>
CC: Liviu Dudau <Liviu.Dudau@....com>,
"Jonathan Chai (Arm Technology China)" <Jonathan.Chai@....com>,
"Julien Yin (Arm Technology China)" <Julien.Yin@....com>,
"thomas Sun (Arm Technology China)" <thomas.Sun@....com>,
Alexandru-Cosmin Gheorghe <Alexandru-Cosmin.Gheorghe@....com>,
"Lowry Li (Arm Technology China)" <Lowry.Li@....com>,
Ayan Halder <Ayan.Halder@....com>,
"Tiannan Zhu (Arm Technology China)" <Tiannan.Zhu@....com>,
"Jin Gao (Arm Technology China)" <Jin.Gao@....com>,
"Yiqi Kang (Arm Technology China)" <Yiqi.Kang@....com>,
nd <nd@....com>, "malidp@...s.arm.com" <malidp@...s.arm.com>,
"airlied@...ux.ie" <airlied@...ux.ie>,
"yamada.masahiro@...ionext.com" <yamada.masahiro@...ionext.com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>
Subject: Re: [PATCH 1/7] drm/komeda: Add d71_enum_resources and d71_cleanup
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@....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@....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 --
>
> 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 --
> 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
Powered by blists - more mailing lists