On 12/24/2024, Dmitry Baryshkov wrote:
> On Tue, 24 Dec 2024 at 08:21, Liu Ying <victor.liu@xxxxxxx> wrote:
>>
>> 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.
>
> Patch 12 uses de->id for two purposes: to assign dc_drm->de[ID] and to
> include ID into error messages. It might be better to use the DE
> device in dev_err instead of using generic DRM device and de->id.
Will use the DE device in dev_err and drop the id field from struct dc_de.
>
>>
>>>
>>>> +};
>>>> +
>>>
>>> [...]
>>>
>>>> +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);
>
> Yes, this depends on the particular behaviour of both
> component_bind_all() (which is not documented this way, so somebody
> might decide to optimise things somehow) and of your component
> management. While you have control on the latter, you don't have
> control on the former code.
>
> Please, don't depend on the undocumented behaviour!
Alright, then I'll drop the access to the DE/PE pointers from those
grandchild device drivers' bind() callbacks and cache pointers in drm_dc
for the grandchild devices. Thanks.
>
>>
>> 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
>
>
>
--
Regards,
Liu Ying
