Message-ID: <214b2282-fc83-4a25-8cf3-ce2b57b59eae@nxp.com>
Date: Tue, 24 Dec 2024 14:47:48 +0800
From: Liu Ying <victor.liu@....com>
To: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
Cc: dri-devel@...ts.freedesktop.org, devicetree@...r.kernel.org,
 imx@...ts.linux.dev, linux-arm-kernel@...ts.infradead.org,
 linux-kernel@...r.kernel.org, linux-phy@...ts.infradead.org,
 p.zabel@...gutronix.de, airlied@...il.com, simona@...ll.ch,
 maarten.lankhorst@...ux.intel.com, mripard@...nel.org, tzimmermann@...e.de,
 robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org,
 shawnguo@...nel.org, s.hauer@...gutronix.de, kernel@...gutronix.de,
 festevam@...il.com, tglx@...utronix.de, vkoul@...nel.org, kishon@...nel.org,
 aisheng.dong@....com, agx@...xcpu.org, u.kleine-koenig@...libre.com,
 francesco@...cini.it, frank.li@....com
Subject: Re: [PATCH v7 10/19] drm/imx: Add i.MX8qxp Display Controller pixel
 engine

On 12/23/2024, Dmitry Baryshkov wrote:
> On Mon, Dec 23, 2024 at 02:41:38PM +0800, Liu Ying wrote:
>> i.MX8qxp Display Controller pixel engine consists of all processing
>> units that operate in the AXI bus clock domain.  Add drivers for
>> ConstFrame, ExtDst, FetchLayer, FetchWarp and LayerBlend units, as
>> well as a pixel engine driver, so that two displays with primary
>> planes can be supported.  The pixel engine driver and those unit
>> drivers are components to be aggregated by a master registered in
>> the upcoming DRM driver.
>>
>> Reviewed-by: Maxime Ripard <mripard@...nel.org>
>> Signed-off-by: Liu Ying <victor.liu@....com>
>> ---
>> v7:
>> * Add kernel doc for struct dc_drm_device. (Dmitry)
>> * Fix regmap_config definitions by correcting name field, correcting read
>>   ranges and setting max_register field.
>> * Get instance numbers from device data(compatible strings) instead of OF
>>   aliases.
>> * Collect Maxime's R-b tag.
>> * Trivial tweaks.
>>
>> v6:
>> * Fix build warning by expanding sizeof(fu->name) from 13 to 21.
>>   (kernel test robot)
>>
>> v5:
>> * Replace .remove_new with .remove in dc-{cf,de,fl,fw,lb,pe}.c. (Uwe)
>> * Fix commit message to state that pixel engine driver is a component driver
>>   instead of a master/aggregate driver.
>>
>> v4:
>> * Use regmap to define register map for all registers. (Dmitry)
>> * Use regmap APIs to access registers. (Dmitry)
>> * Inline some small functions. (Dmitry)
>> * Move dc_lb_blendcontrol() function call from KMS routine to initialization
>>   stage. (Dmitry)
>> * Use devm_kzalloc() to drmm_kzalloc() to allocate dc_* data strutures.
>> * Drop unnecessary private struct dc_*_priv.
>> * Set suppress_bind_attrs driver flag to true to avoid unnecessary sys
>>   interfaces to bind/unbind the drivers.
>> * Make some fetch unit operations be aware of fractional fetch unit index(0-7).
>>
>> v3:
>> * No change.
>>
>> v2:
>> * Use OF alias id to get instance id.
> 
> Carrying several comments from previous patch:
> - shdld vs shdload

Will change IRQ names from shdld to shdload.

> - use of indices in the compat strings

Maybe keep adding indices in the compatible strings since I explained in
my replies to your comments on patch 3 and 9.

> - bind() behaviour depending on the particular order of device bindings

As I explained in the my reply to patch 9, bind() behaviour is deterministic.

> 
>>
>> +
>> +void dc_fu_common_hw_init(struct dc_fu *fu)
>> +{
>> +	enum dc_fu_frac frac;
>> +	int i;
>> +
>> +	dc_fu_baddr_autoupdate(fu, 0x0);
>> +	dc_fu_enable_shden(fu);
>> +	dc_fu_set_linemode(fu, LINEMODE_DISPLAY);
>> +	dc_fu_set_numbuffers(fu, 16);
>> +
>> +	for (i = 0; i < ARRAY_SIZE(dc_fetchunit_all_fracs); i++) {
> 
> for (i = DC_FETCHUNIT_FRAC0 ; i < DC_FETCHUNIT_FRAC_NUM; i++) ?

Ack.

> 
>> +		frac = dc_fetchunit_all_fracs[i];
>> +
>> +		dc_fu_layeroffset(fu, frac, 0, 0);
>> +		dc_fu_clipoffset(fu, frac, 0, 0);
>> +		dc_fu_clipdimensions(fu, frac, 1, 1);
>> +		dc_fu_disable_src_buf(fu, frac);
>> +		dc_fu_set_pixel_blend_mode(fu, frac);
>> +	}
>> +}
>> +
> 
> [...]
> 
>> +enum dc_link_id dc_lb_get_link_id(struct dc_lb *lb)
>> +{
>> +	return lb->link;
>> +}
>> +
>> +void dc_lb_pec_dynamic_prim_sel(struct dc_lb *lb, enum dc_link_id prim)
>> +{
>> +	int fixed_sels_num = ARRAY_SIZE(prim_sels) - 4;
>> +	int i;
>> +
>> +	for (i = 0; i < fixed_sels_num + lb->id; i++) {
> 
> This function and the next one silently skip writing link ID if it is
> incorrect. Can it actually become incorrect? If not, I'd say, it is
> better to drop the loop and the array. If you are not sure, there should
> be some kind of dev_warn() or drm_warn().

Will add a dev_warn() in this and the next function in case prim/sec arguments
are invalid.

> 
>> +		if (prim_sels[i] == prim) {
>> +			regmap_write_bits(lb->reg_pec, PIXENGCFG_DYNAMIC,
>> +					  PIXENGCFG_DYNAMIC_PRIM_SEL_MASK,
>> +					  PIXENGCFG_DYNAMIC_PRIM_SEL(prim));
>> +			return;
>> +		}
>> +	}
>> +}
>> +
>> +void dc_lb_pec_dynamic_sec_sel(struct dc_lb *lb, enum dc_link_id sec)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i < ARRAY_SIZE(sec_sels); i++) {
>> +		if (sec_sels[i] == sec) {
>> +			regmap_write_bits(lb->reg_pec, PIXENGCFG_DYNAMIC,
>> +					  PIXENGCFG_DYNAMIC_SEC_SEL_MASK,
>> +					  PIXENGCFG_DYNAMIC_SEC_SEL(sec));
>> +			return;
>> +		}
>> +	}
>> +}
>> +
> 
> [...]
> 
>> +
>> +static int dc_lb_bind(struct device *dev, struct device *master, void *data)
>> +{
>> +	struct platform_device *pdev = to_platform_device(dev);
>> +	struct dc_drm_device *dc_drm = data;
>> +	struct dc_pe *pe = dc_drm->pe;
>> +	void __iomem *base_pec;
>> +	void __iomem *base_cfg;
>> +	struct dc_lb *lb;
>> +
>> +	lb = devm_kzalloc(dev, sizeof(*lb), GFP_KERNEL);
>> +	if (!lb)
>> +		return -ENOMEM;
>> +
>> +	lb->id = (enum dc_lb_id)(uintptr_t)device_get_match_data(dev);
>> +
>> +	base_pec = devm_platform_ioremap_resource_byname(pdev, "pec");
>> +	if (IS_ERR(base_pec))
>> +		return PTR_ERR(base_pec);
>> +
>> +	base_cfg = devm_platform_ioremap_resource_byname(pdev, "cfg");
>> +	if (IS_ERR(base_cfg))
>> +		return PTR_ERR(base_cfg);
>> +
>> +	lb->reg_pec = devm_regmap_init_mmio(dev, base_pec,
>> +					    &dc_lb_pec_regmap_config);
>> +	if (IS_ERR(lb->reg_pec))
>> +		return PTR_ERR(lb->reg_pec);
>> +
>> +	lb->reg_cfg = devm_regmap_init_mmio(dev, base_cfg,
>> +					    &dc_lb_cfg_regmap_config);
>> +	if (IS_ERR(lb->reg_cfg))
>> +		return PTR_ERR(lb->reg_cfg);
>> +
>> +	lb->link = lb_links[lb->id];
> 
> lb->link = LINK_ID_LAYERBLEND0 + lb->id ?

Ack.

> 
>> +
>> +	pe->lb[lb->id] = lb;
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct component_ops dc_lb_ops = {
>> +	.bind = dc_lb_bind,
>> +};
>> +
> 

-- 
Regards,
Liu Ying

Powered by blists - more mailing lists