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@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.
>
> 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
