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@xxxxxxxxxx>
>> Signed-off-by: Liu Ying <victor.liu@xxxxxxx>
>> ---
>> 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
