Message-ID: <d6c0df7a-15f3-4c21-928b-feed15ee02bc@nxp.com>
Date: Tue, 24 Dec 2024 16:26:24 +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 09/19] drm/imx: Add i.MX8qxp Display Controller display
 engine

On 12/24/2024, Dmitry Baryshkov wrote:
> On Tue, 24 Dec 2024 at 08:21, Liu Ying <victor.liu@....com> 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@...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.
>>>
>>> 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

Powered by blists - more mailing lists