Message-ID: <d76a40de-7e42-4870-86c7-f168666b3e59@xs4all.nl>
Date: Fri, 7 Jun 2024 14:04:23 +0200
From: Hans Verkuil <hverkuil-cisco@...all.nl>
To: Paweł Anikiel <panikiel@...gle.com>
Cc: airlied@...il.com, akpm@...ux-foundation.org, conor+dt@...nel.org,
 daniel@...ll.ch, dinguyen@...nel.org, krzysztof.kozlowski+dt@...aro.org,
 maarten.lankhorst@...ux.intel.com, mchehab@...nel.org, mripard@...nel.org,
 robh+dt@...nel.org, tzimmermann@...e.de, devicetree@...r.kernel.org,
 dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
 linux-media@...r.kernel.org, chromeos-krk-upstreaming@...gle.com
Subject: Re: [PATCH v3 07/10] media: intel: Add Displayport RX IP driver

On 04/06/2024 14:32, Paweł Anikiel wrote:
> On Mon, Jun 3, 2024 at 10:37 AM Hans Verkuil <hverkuil-cisco@...all.nl> wrote:
>>
>> On 07/05/2024 17:54, Paweł Anikiel wrote:
>>> Add v4l2 subdev driver for the Intel Displayport receiver FPGA IP.
>>> It is a part of the DisplayPort Intel FPGA IP Core, and supports
>>> DisplayPort 1.4, HBR3 video capture and Multi-Stream Transport.
>>>
>>> Signed-off-by: Paweł Anikiel <panikiel@...gle.com>
>>> ---
>>>  drivers/media/platform/intel/Kconfig      |   12 +
>>>  drivers/media/platform/intel/Makefile     |    1 +
>>>  drivers/media/platform/intel/intel-dprx.c | 2283 +++++++++++++++++++++
>>>  3 files changed, 2296 insertions(+)
>>>  create mode 100644 drivers/media/platform/intel/intel-dprx.c
>>>

<snip>

>>> +static int dprx_probe(struct platform_device *pdev)
>>> +{
>>> +     struct dprx *dprx;
>>> +     int irq;
>>> +     int res;
>>> +     int i;
>>> +
>>> +     dprx = devm_kzalloc(&pdev->dev, sizeof(*dprx), GFP_KERNEL);
>>> +     if (!dprx)
>>> +             return -ENOMEM;
>>> +     dprx->dev = &pdev->dev;
>>> +     platform_set_drvdata(pdev, dprx);
>>> +
>>> +     dprx->iobase = devm_platform_ioremap_resource(pdev, 0);
>>> +     if (IS_ERR(dprx->iobase))
>>> +             return PTR_ERR(dprx->iobase);
>>> +
>>> +     irq = platform_get_irq(pdev, 0);
>>> +     if (irq < 0)
>>> +             return irq;
>>> +
>>> +     res = devm_request_irq(dprx->dev, irq, dprx_isr, 0, "intel-dprx", dprx);
>>> +     if (res)
>>> +             return res;
>>> +
>>> +     res = dprx_parse_fwnode(dprx);
>>> +     if (res)
>>> +             return res;
>>> +
>>> +     dprx_init_caps(dprx);
>>> +
>>> +     dprx->subdev.owner = THIS_MODULE;
>>> +     dprx->subdev.dev = &pdev->dev;
>>> +     v4l2_subdev_init(&dprx->subdev, &dprx_subdev_ops);
>>> +     v4l2_set_subdevdata(&dprx->subdev, &pdev->dev);
>>> +     snprintf(dprx->subdev.name, sizeof(dprx->subdev.name), "%s %s",
>>> +              KBUILD_MODNAME, dev_name(&pdev->dev));
>>> +     dprx->subdev.flags = V4L2_SUBDEV_FL_HAS_DEVNODE;
>>> +
>>> +     dprx->subdev.entity.function = MEDIA_ENT_F_DV_DECODER;
>>> +     dprx->subdev.entity.ops = &dprx_entity_ops;
>>> +
>>> +     v4l2_ctrl_handler_init(&dprx->ctrl_handler, 1);
>>> +     v4l2_ctrl_new_std(&dprx->ctrl_handler, NULL,
>>> +                       V4L2_CID_DV_RX_POWER_PRESENT, 0, 1, 0, 0);
>>
>> You are creating this control, but it is never set to 1 when the driver detects
>> that a source is connected. I am wondering if POWER_PRESENT makes sense for a
>> DisplayPort connector. Is there a clean way for a sink driver to detect if a
>> source is connected? For HDMI it detects the 5V pin, but it is not clear if
>> there is an equivalent to that in the DP spec.
> 
> The DP spec says the source can be detected using the AUX lines:
> 
> "The Downstream devices must very weakly pull up AUX+ line and very
> weakly pull down AUX- line with 1MΩ (+/-5%) resistors between the
> Downstream device Connector and the AC-coupling capacitors. When AUX+
> line DC voltage is L level, it means a DisplayPort Upstream device is
> connected. When AUX- line DC voltage is H level, it means that a
> powered DisplayPort Upstream device is connected."
> 
> This exact IP has two input signals: rx_cable_detect, and
> rx_pwr_detect, which are meant to be connected to the AUX+/AUX- lines
> via 10k resistors (or rather that's what the reference design does).
> They're exposed to software via status registers, but there's no way
> to get interrupts from them, so it wouldn't be possible to set the
> control exactly when a source gets plugged in.
> 
>>
>> If there is no good way to detect if a source is connected, then it might be
>> better to drop POWER_PRESENT support.
>>
>> This control is supposed to signal that a source is connected as early as possible,
>> ideally before link training etc. starts.
>>
>> It helps the software detect that there is a source, and report an error if a source
>> is detected, but you never get a stable signal (e.g. link training fails).
> 
> This poses another problem, because the chameleon board doesn't have
> this detection circuitry, and instead sets the rx_cable_detect and
> rx_pwr_detect signals to always logical high. That would make the
> control read "always plugged in", which IIUC is not desired.

OK, so it is best to drop support for this control.

I recommend adding a comment in the source code explaining why it is not supported.
And in the cover letter you can mention this as well as an explanation of why
there is a v4l2-compliance warning.

Regards,

	Hans

Powered by blists - more mailing lists