Re: [PATCH v2 1/2] drm: Add NVIDIA Tegra20 support

[Mark Zhang <markz@nvidia.com>]

On 11/13/2012 03:48 PM, Thierry Reding wrote:
> * PGP Signed by an unknown key
> 
> On Tue, Nov 13, 2012 at 03:15:47PM +0800, Mark Zhang wrote:
>> On 11/13/2012 05:55 AM, Thierry Reding wrote:
>>> This commit adds a KMS driver for the Tegra20 SoC. This includes basic
>>> support for host1x and the two display controllers found on the Tegra20
>>> SoC. Each display controller can drive a separate RGB/LVDS output.
>>>
>>> Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
>>> ---
>>> Changes in v2:
>>> - drop Linux-specific drm subdirectory for DT bindings documentation
>>> - remove display helper leftovers that belong in a later patch
>>> - reuse debugfs infrastructure provided by the DRM core
>>> - move vblank syncpoint defines to dc.h
>>> - use drm_compat_ioctl()
>>>
>> [...]
>>> diff --git a/drivers/gpu/drm/tegra/Kconfig b/drivers/gpu/drm/tegra/Kconfig
>>> new file mode 100644
>>> index 0000000..be1daf7
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/tegra/Kconfig
>>> @@ -0,0 +1,23 @@
>>> +config DRM_TEGRA
>>> +       tristate "NVIDIA Tegra DRM"
>>> +       depends on DRM && OF && ARCH_TEGRA
>>> +       select DRM_KMS_HELPER
>>> +       select DRM_GEM_CMA_HELPER
>>> +       select DRM_KMS_CMA_HELPER
>>
>> Just for curious, according to my testing, why the "CONFIG_CMA" is not
>> enabled while DRM_GEM_CMA_HELPER & DRM_KMS_CMA_HELPER are enabled here?
> 
> The reason is that CMA doesn't actually provide any API for drivers to
> use and in fact unless you use very large buffers you could indeed run
> this code on top of a non-CMA kernel and it will likely even work.
> 

Okay. But I think it's better to turn on CMA defaultly. During my
testing, it's hard to allocate more 2MB without CMA...

>>> +static struct of_device_id tegra_dc_of_match[] = {
>>> +       { .compatible = "nvidia,tegra20-dc", },
>>> +       { .compatible = "nvidia,tegra30-dc", },
>>
>> If you don't want add Tegra 3 support in this patch set, remove
>> { .compatible = "nvidia,tegra30-dc", } here.
> 
> Good catch! I'll move that into the Tegra30 support patch.
> 
>>> +static int host1x_activate_drm_client(struct host1x *host1x,
>>> +                                     struct host1x_drm_client *drm,
>>> +                                     struct host1x_client *client)
>>> +{
>>> +       mutex_lock(&host1x->drm_clients_lock);
>>> +       list_del_init(&drm->list);
>>> +       list_add_tail(&drm->list, &host1x->drm_active);
>>
>> Why we need this "drm_active" list? We can combine this function and
>> function "host1x_remove_drm_client" and free the drm client just here.
>> It's useless after host1x clients registered themselves.
> 
> The list is used to properly remove all clients and resources when the
> module is unloaded. Granted, this code isn't executed if you don't build
> the driver as a loadable module, but it should still be a supported use-
> case.
> 

My opinion is, after registration is completed, host1x_drm_client is
useless, host1x_client is enough for follow-up operations.
I still don't get how this is related with building the driver into the
kernel or as a kernel module, so if something I misunderstood, please
let me know it. Thanks.

>>> +int host1x_unregister_client(struct host1x *host1x,
>>> +                            struct host1x_client *client)
>>> +{
>>> +       struct host1x_drm_client *drm, *tmp;
>>> +       int err;
>>> +
>>> +       list_for_each_entry_safe(drm, tmp, &host1x->drm_active, list) {
>>> +               if (drm->client == client) {
>>> +                       err = host1x_drm_exit(host1x);
>>
>> Although this code works but I think it looks confusing.
>> "host1x_drm_exit" calls every client's "drm_exit" callback then free all
>> the host1x clients, but this function is placed inside a loop.
>>
>> I think the better way is, free this host1x_client first, then remove it
>> from host1x's "clients" list, finally free the host1x(call
>> host1x_drm_exit) when the "clients" list get empty.
> 
> But that would be the same thing, only slightly more explicit. I find
> that the above reads quite well as: look through the list of active
> clients and if the client to be removed is in that list, teardown the
> DRM part.
> 

All right, this is just coding style problem and I think your words make
sense as well.

> I suppose I could add a comment to explain this and avoid confusion.
> 
>>> +int tegra_output_init(struct drm_device *drm, struct tegra_output *output)
>>> +{
>>> +       int connector, encoder, err;
>>> +       enum of_gpio_flags flags;
>>> +       struct device_node *ddc;
>>> +       size_t size;
>>> +
>>> +       if (!output->of_node)
>>> +               output->of_node = output->dev->of_node;
>>> +
>>> +       output->edid = of_get_property(output->of_node, "nvidia,edid", &size);
>>> +
>>> +       ddc = of_parse_phandle(output->of_node, "nvidia,ddc-i2c-bus", 0);
>>> +       if (ddc) {
>>> +               output->ddc = of_find_i2c_adapter_by_node(ddc);
>>
>> The i2c adapter may not be ready at this time. For Tegra 2, the I2C bus
>> for HDMI is not dedicated and we need the i2cmux driver loaded before
>> this i2c can be used. It proved that sometimes i2cmux driver loads after
>> drm driver.
>>
>> So we need to add some logics to support driver probe deferral here.
>> Anyway, I'm just want you know about this and we can improve this later.
> 
> Good point. Unfortunately tegra_output_init() isn't always used from
> within .probe(), so it isn't quite easy to handle deferred probe here.
> I'll have to take a look at how to solve this properly.
> 
>>> +int tegra_dc_rgb_init(struct drm_device *drm, struct tegra_dc *dc)
>>> +{
>>> +       struct device_node *np;
>>> +       struct tegra_rgb *rgb;
>>> +       int err;
>>> +
>>> +       np = of_get_child_by_name(dc->dev->of_node, "rgb");
>>> +       if (!np || !of_device_is_available(np))
>>> +               return -ENODEV;
>>> +
>>> +       rgb = devm_kzalloc(dc->dev, sizeof(*rgb), GFP_KERNEL);
>>> +       if (!rgb)
>>> +               return -ENOMEM;
>>> +
>>> +       rgb->clk = devm_clk_get(dc->dev, NULL);
>>> +       if (IS_ERR_OR_NULL(rgb->clk))
>>> +               return PTR_ERR(rgb->clk);
>>> +
>>> +       rgb->clk_parent = devm_clk_get(dc->dev, "parent");
>>> +       if (IS_ERR_OR_NULL(rgb->clk_parent))
>>> +               return PTR_ERR(rgb->clk_parent);
>>> +
>>> +       err = clk_set_parent(rgb->clk, rgb->clk_parent);
>>> +       if (err < 0) {
>>> +               dev_err(dc->dev, "failed to set parent clock: %d\n", err);
>>> +               return err;
>>> +       }
>>
>> Okay, seems this works with the "CLK_DUPLICATE" in tegra20_clocks_data.c.
>> I think the purpose of all these is to make sure the dc has a correct
>> parent clock. Hm... But I feel this may bring confusing to do dc clock
>> settings in a drm output component.
> 
> How do you think this would be confusing?
> 

I just feel that all dc related works should be handled in crtc while
not in output. Anyway, this is not a big deal and I think the current
implementation also makes sense.

> Thierry
> 
> * Unknown Key
> * 0x7F3EB3A1
>