Re: [PATCH v7 09/19] drm/imx: Add i.MX8qxp Display Controller display engine

[Liu Ying <victor.liu@nxp.com>]

On 12/23/2024, Dmitry Baryshkov wrote:
> On Mon, Dec 23, 2024 at 02:41:37PM +0800, Liu Ying wrote:
>> i.MX8qxp Display Controller display engine consists of all processing
>> units that operate in a display clock domain.  Add minimal feature
>> support with FrameGen and TCon so that the engine can output display
>> timings.  The FrameGen driver, TCon driver and display engine driver
>> are components to be aggregated by a master registered in the upcoming
>> DRM driver.
>>
>> Reviewed-by: Maxime Ripard <mripard@kernel.org>
>> Signed-off-by: Liu Ying <victor.liu@nxp.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.
> 
> Unfortunately no. Your instances are compatible on the hardware level
> (at least they were with the previous versions, I don't think that
> there was a silicon change).

v5/v6 use OF aliases to the instance numbers, but in v6 Rob said it's
abusing aliases because the aliases contain display controller instance
number, like "dc0-display-engine0"(i.MX8QM SoC has two display controllers).
Or, use OF graph to describe all connections between the display controller's
internal devices, but it's too complex. So, I choose to add the instance
numbers to compatible strings.

> 
>> * Collect Maxime's R-b tag.
>> * Trivial tweaks.
>>
>> v6:
>> * No change.
>>
>> v5:
>> * Replace .remove_new with .remove in dc-{de,fg,tc}.c. (Uwe)
>> * Select REGMAP and REGMAP_MMIO Kconfig options.
>> * Fix commit message to state that display 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_fg_displaymode() and dc_fg_panic_displaymode() function calls 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.
>>
>> v3:
>> * No change.
>>
>> v2:
>> * Use OF alias id to get instance id.
>> * Add dev member to struct dc_tc.
>>
>>  drivers/gpu/drm/imx/Kconfig     |   1 +
>>  drivers/gpu/drm/imx/Makefile    |   1 +
>>  drivers/gpu/drm/imx/dc/Kconfig  |   7 +
>>  drivers/gpu/drm/imx/dc/Makefile |   5 +
>>  drivers/gpu/drm/imx/dc/dc-de.c  | 153 +++++++++++++
>>  drivers/gpu/drm/imx/dc/dc-de.h  |  62 ++++++
>>  drivers/gpu/drm/imx/dc/dc-drv.c |  32 +++
>>  drivers/gpu/drm/imx/dc/dc-drv.h |  29 +++
>>  drivers/gpu/drm/imx/dc/dc-fg.c  | 378 ++++++++++++++++++++++++++++++++
>>  drivers/gpu/drm/imx/dc/dc-tc.c  | 142 ++++++++++++
>>  10 files changed, 810 insertions(+)
>>  create mode 100644 drivers/gpu/drm/imx/dc/Kconfig
>>  create mode 100644 drivers/gpu/drm/imx/dc/Makefile
>>  create mode 100644 drivers/gpu/drm/imx/dc/dc-de.c
>>  create mode 100644 drivers/gpu/drm/imx/dc/dc-de.h
>>  create mode 100644 drivers/gpu/drm/imx/dc/dc-drv.c
>>  create mode 100644 drivers/gpu/drm/imx/dc/dc-drv.h
>>  create mode 100644 drivers/gpu/drm/imx/dc/dc-fg.c
>>  create mode 100644 drivers/gpu/drm/imx/dc/dc-tc.c
>>
> 
> [...]
> 
>> +
>> +static int dc_de_bind(struct device *dev, struct device *master, void *data)
>> +{
>> +	struct platform_device *pdev = to_platform_device(dev);
>> +	struct dc_drm_device *dc_drm = data;
>> +	void __iomem *base_top;
>> +	struct dc_de *de;
>> +	int ret;
>> +
>> +	de = devm_kzalloc(dev, sizeof(*de), GFP_KERNEL);
>> +	if (!de)
>> +		return -ENOMEM;
>> +
>> +	de->id = (enum dc_de_id)(uintptr_t)device_get_match_data(dev);
>> +
>> +	base_top = devm_platform_ioremap_resource_byname(pdev, "top");
>> +	if (IS_ERR(base_top))
>> +		return PTR_ERR(base_top);
>> +
>> +	de->reg_top = devm_regmap_init_mmio(dev, base_top,
>> +					    &dc_de_top_regmap_config);
>> +	if (IS_ERR(de->reg_top))
>> +		return PTR_ERR(de->reg_top);
>> +
>> +	de->irq_shdld = platform_get_irq_byname(pdev, "shdload");
> 
> Nit: shdload or shdld? Which one is used in the documentation?
> 
>> +	if (de->irq_shdld < 0)
>> +		return de->irq_shdld;
>> +
>> +	de->irq_framecomplete = platform_get_irq_byname(pdev, "framecomplete");
>> +	if (de->irq_framecomplete < 0)
>> +		return de->irq_framecomplete;
>> +
>> +	de->irq_seqcomplete = platform_get_irq_byname(pdev, "seqcomplete");
>> +	if (de->irq_seqcomplete < 0)
>> +		return de->irq_seqcomplete;
>> +
>> +	de->dev = dev;
>> +
>> +	dev_set_drvdata(dev, de);
>> +
>> +	ret = devm_pm_runtime_enable(dev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	dc_drm->de[de->id] = de;
>> +
>> +	return 0;
>> +}
>> +
> 
> [...]
> 
>> +
>> +struct dc_de {
>> +	struct device *dev;
>> +	struct regmap *reg_top;
>> +	struct dc_fg *fg;
>> +	struct dc_tc *tc;
>> +	int irq_shdld;
>> +	int irq_framecomplete;
>> +	int irq_seqcomplete;
>> +	enum dc_de_id id;
> 
> Why do you need to store it? This patch makes use of it just for the
> registration.

dc-crtc.c added in patch 12 would reference de->id. If no strong opinions,
I'd keep this as-is.

> 
>> +};
>> +
> 
> [...]
> 
>> +static int dc_fg_bind(struct device *dev, struct device *master, void *data)
>> +{
>> +	struct platform_device *pdev = to_platform_device(dev);
>> +	struct dc_drm_device *dc_drm = data;
>> +	void __iomem *base;
>> +	enum dc_fg_id id;
>> +	struct dc_fg *fg;
>> +	struct dc_de *de;
>> +
>> +	fg = devm_kzalloc(dev, sizeof(*fg), GFP_KERNEL);
>> +	if (!fg)
>> +		return -ENOMEM;
>> +
>> +	id = (enum dc_fg_id)(uintptr_t)device_get_match_data(dev);
>> +
>> +	base = devm_platform_ioremap_resource(pdev, 0);
>> +	if (IS_ERR(base))
>> +		return PTR_ERR(base);
>> +
>> +	fg->reg = devm_regmap_init_mmio(dev, base, &dc_fg_regmap_config);
>> +	if (IS_ERR(fg->reg))
>> +		return PTR_ERR(fg->reg);
>> +
>> +	fg->clk_disp = devm_clk_get(dev, NULL);
>> +	if (IS_ERR(fg->clk_disp))
>> +		return dev_err_probe(dev, PTR_ERR(fg->clk_disp),
>> +				     "failed to get display clock\n");
>> +
>> +	fg->dev = dev;
>> +
>> +	de = dc_drm->de[id];
> 
> This depends on a particular order of component's being bound. If the
> order changes for whatever reason (e.g. due to component.c
> implementation being changed) then your driver might crash here.

Nope. There is no chance to crash here, because
1) dc_drm is not NULL here
dc_drm is allocated in dc_drm_bind() and before component_bind_all() is
called.  dc_fg_bind() is called by component_bind_all().

2) dc_drm->de[id] is not NULL here
It's already set by dc_de_bind(), because component_bind_all() binds
components in match order and the component match for DE is added before
the one for FG(DE is a child device of display controller and FG is a
_grandchild_ of display controller).

component_bind_all():
        /* Bind components in match order */                                     
        for (i = 0; i < adev->match->num; i++)                                   
                if (!adev->match->compare[i].duplicate) {                        
                        c = adev->match->compare[i].component;                   
                        ret = component_bind(c, adev, data);                     

dc_add_components():
        for_each_available_child_of_node(dev->of_node, child) {                  
                ...

                drm_of_component_match_add(dev, matchptr, component_compare_of,  
                                           child);                               
                                                                                 
                for_each_available_child_of_node(child, grandchild)              
                        drm_of_component_match_add(dev, matchptr,                
                                                   component_compare_of,         
                                                   grandchild);                  
        }                                                                        

> 
> This applies to several other places in the driver.

I don't see any other potential crash caused by the binding order of the
components.

> 
>> +	de->fg = fg;
>> +
>> +	return 0;
>> +}
>> +
> 

-- 
Regards,
Liu Ying