Re: Safety of opening up /dev/dma_heap/* to physically present users (udev uaccess tag) ?
On Mon, May 13, 2024 at 10:29:22AM +0200, Maxime Ripard wrote: > On Wed, May 08, 2024 at 10:36:08AM +0200, Daniel Vetter wrote: > > On Tue, May 07, 2024 at 04:07:39PM -0400, Nicolas Dufresne wrote: > > > Hi, > > > > > > Le mardi 07 mai 2024 à 21:36 +0300, Laurent Pinchart a écrit : > > > > Shorter term, we have a problem to solve, and the best option we have > > > > found so far is to rely on dma-buf heaps as a backend for the frame > > > > buffer allocatro helper in libcamera for the use case described above. > > > > This won't work in 100% of the cases, clearly. It's a stop-gap measure > > > > until we can do better. > > > > > > Considering the security concerned raised on this thread with dmabuf heap > > > allocation not be restricted by quotas, you'd get what you want quickly > > > with > > > memfd + udmabuf instead (which is accounted already). > > > > > > It was raised that distro don't enable udmabuf, but as stated there by > > > Hans, in > > > any cases distro needs to take action to make the softISP works. This > > > alternative is easy and does not interfere in anyway with your future > > > plan or > > > the libcamera API. You could even have both dmabuf heap (for Raspbian) > > > and the > > > safer memfd+udmabuf for the distro with security concerns. > > > > > > And for the long term plan, we can certainly get closer by fixing that > > > issue > > > with accounting. This issue also applied to v4l2 io-ops, so it would be > > > nice to > > > find common set of helpers to fix these exporters. > > > > Yeah if this is just for softisp, then memfd + udmabuf is also what I was > > about to suggest. Not just as a stopgap, but as the real official thing. > > > > udmabuf does kinda allow you to pin memory, but we can easily fix that by > > adding the right accounting and then either let mlock rlimits or cgroups > > kernel memory limits enforce good behavior. > > I think the main drawback with memfd is that it'll be broken for devices > without an IOMMU, and while you said that it's uncommon for GPUs, it's > definitely not for codecs and display engines. If the application wants to share buffers between the camera and a display engine or codec, it should arguably not use the libcamera FrameBufferAllocator, but allocate the buffers from the display or the encoder. memfd wouldn't be used in that case. We need to eat our own dogfood though. If we want to push the responsibility for buffer allocation in the buffer sharing case to the application, we need to modify the cam application to do so when using the KMS backend. -- Regards, Laurent Pinchart
Re: [PATCH] drm/bridge: cdns-mhdp8546: Remove a redundant check on existence of bridge->encoder
Hi Sui,
Thank you for the patch.
On Sat, May 11, 2024 at 10:30:27PM +0800, Sui Jingfeng wrote:
> In the cdns_mhdp_connector_init() function, the check on the existence
> of bridge->encoder is not necessary, as it has already been done in the
> drm_bridge_attach() function. And the check on the drm bridge core
> happens before check in the implementation. Hence, it is guaranteed that
> the .encoder member of the struct drm_bridge is not NULL when
> adv7511_bridge_attach() function gets called.
>
> Remove the redundant checking codes "if (!bridge->encoder) { ... }".
>
> Signed-off-by: Sui Jingfeng
Reviewed-by: Laurent Pinchart
> ---
> drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c | 5 -
> 1 file changed, 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> index e226acc5c15e..16b58a7dcc54 100644
> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> @@ -1697,11 +1697,6 @@ static int cdns_mhdp_connector_init(struct
> cdns_mhdp_device *mhdp)
> struct drm_bridge *bridge = >bridge;
> int ret;
>
> - if (!bridge->encoder) {
> - dev_err(mhdp->dev, "Parent encoder object not found");
> - return -ENODEV;
> - }
> -
> conn->polled = DRM_CONNECTOR_POLL_HPD;
>
> ret = drm_connector_init(bridge->dev, conn, _mhdp_conn_funcs,
--
Regards,
Laurent Pinchart
Re: [PATCH] drm/bridge: megachips-stdpxxxx-ge-b850v3-fw: Remove a redundant check on existence of bridge->encoder
Hi Sui,
Thank you for the patch.
On Sat, May 11, 2024 at 10:38:20PM +0800, Sui Jingfeng wrote:
> In the ge_b850v3_lvds_create_connector function, the check on the existence
> of bridge->encoder is not necessary, as it has already been done in the
> drm_bridge_attach() function. And the check on the drm bridge core
> happens before check in the implementation. Hence, it is guaranteed that
> the .encoder member of the struct drm_bridge is not NULL when
> ge_b850v3_lvds_attach() function gets called.
>
> Remove the redundant checking codes "if (!bridge->encoder) { ... }".
>
> Signed-off-by: Sui Jingfeng
Reviewed-by: Laurent Pinchart
> ---
> drivers/gpu/drm/bridge/megachips-stdp-ge-b850v3-fw.c | 5 -
> 1 file changed, 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/megachips-stdp-ge-b850v3-fw.c
> b/drivers/gpu/drm/bridge/megachips-stdp-ge-b850v3-fw.c
> index 4480523244e4..37f1acf5c0f8 100644
> --- a/drivers/gpu/drm/bridge/megachips-stdp-ge-b850v3-fw.c
> +++ b/drivers/gpu/drm/bridge/megachips-stdp-ge-b850v3-fw.c
> @@ -165,11 +165,6 @@ static int ge_b850v3_lvds_create_connector(struct
> drm_bridge *bridge)
> struct drm_connector *connector = _b850v3_lvds_ptr->connector;
> int ret;
>
> - if (!bridge->encoder) {
> - DRM_ERROR("Parent encoder object not found");
> - return -ENODEV;
> - }
> -
> connector->polled = DRM_CONNECTOR_POLL_HPD;
>
> drm_connector_helper_add(connector,
--
Regards,
Laurent Pinchart
Re: [PATCH] drm/bridge: synopsys: dw-mipi-dsi: Remove a redundant check on existence of bridge->encoder
Hi Sui,
Thank you for the patch.
On Sat, May 11, 2024 at 10:47:13PM +0800, Sui Jingfeng wrote:
> In the dw_mipi_dsi_bridge_attach() function, the check on the existence
> of bridge->encoder is not necessary, as it has already been done in the
> drm_bridge_attach() function. And the check on the drm bridge core
> happens before check in the implementation. Hence, it is guaranteed that
> the .encoder member of the struct drm_bridge is not NULL when
> dw_mipi_dsi_bridge_attach() function gets called.
>
> Remove the redundant checking codes "if (!bridge->encoder) { ... }".
>
> Signed-off-by: Sui Jingfeng
Reviewed-by: Laurent Pinchart
> ---
> drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 5 -
> 1 file changed, 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> index 824fb3c65742..c4e9d96933dc 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> @@ -1071,11 +1071,6 @@ static int dw_mipi_dsi_bridge_attach(struct drm_bridge
> *bridge,
> {
> struct dw_mipi_dsi *dsi = bridge_to_dsi(bridge);
>
> - if (!bridge->encoder) {
> - DRM_ERROR("Parent encoder object not found\n");
> - return -ENODEV;
> - }
> -
> /* Set the encoder type as caller does not know it */
> bridge->encoder->encoder_type = DRM_MODE_ENCODER_DSI;
>
--
Regards,
Laurent Pinchart
Re: [PATCH] drm/bridge: lt9611uxc: Remove a redundant check on existence of bridge->encoder
Hi Sui,
Thank you for the patch.
On Sat, May 11, 2024 at 10:55:49PM +0800, Sui Jingfeng wrote:
> In the lt9611uxc_connector_init() function, the check on the existence
> of bridge->encoder is not necessary, as it has already been done in the
> drm_bridge_attach() function. And the check on the drm bridge core
> happens before check in the implementation. Hence, it is guaranteed that
> the .encoder member of the struct drm_bridge is not NULL when
> lt9611uxc_connector_init() function gets called.
>
> Remove the redundant checking codes "if (!bridge->encoder) { ... }".
>
> Signed-off-by: Sui Jingfeng
Reviewed-by: Laurent Pinchart
> ---
> drivers/gpu/drm/bridge/lontium-lt9611uxc.c | 5 -
> 1 file changed, 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/lontium-lt9611uxc.c
> b/drivers/gpu/drm/bridge/lontium-lt9611uxc.c
> index f4f593ad8f79..f1fccfe6c534 100644
> --- a/drivers/gpu/drm/bridge/lontium-lt9611uxc.c
> +++ b/drivers/gpu/drm/bridge/lontium-lt9611uxc.c
> @@ -339,11 +339,6 @@ static int lt9611uxc_connector_init(struct drm_bridge
> *bridge, struct lt9611uxc
> {
> int ret;
>
> - if (!bridge->encoder) {
> - DRM_ERROR("Parent encoder object not found");
> - return -ENODEV;
> - }
> -
> lt9611uxc->connector.polled = DRM_CONNECTOR_POLL_HPD;
>
> drm_connector_helper_add(>connector,
--
Regards,
Laurent Pinchart
Re: [PATCH] drm/bridge: adv7511: Remove a redundant check on existence of bridge->encoder
Hi Sui,
Thank you for the patch.
On Sat, May 11, 2024 at 10:23:08PM +0800, Sui Jingfeng wrote:
> In the adv7511_connector_init() function, the check on the existence
> of bridge->encoder is not necessary, as it has already been done in the
> drm_bridge_attach() function. And the check on the drm bridge core
> happens before check in the implementation. Hence, it is guaranteed that
> the .encoder member of the struct drm_bridge is not NULL when
> adv7511_bridge_attach() function is called.
>
> Remove the redundant checking codes "if (!bridge->encoder) { ... }".
>
> Signed-off-by: Sui Jingfeng
Reviewed-by: Laurent Pinchart
> ---
> drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 5 -
> 1 file changed, 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> index b5518ff97165..1a0e614e0fd3 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> @@ -871,11 +871,6 @@ static int adv7511_connector_init(struct adv7511 *adv)
> struct drm_bridge *bridge = >bridge;
> int ret;
>
> - if (!bridge->encoder) {
> - DRM_ERROR("Parent encoder object not found");
> - return -ENODEV;
> - }
> -
> if (adv->i2c_main->irq)
> adv->connector.polled = DRM_CONNECTOR_POLL_HPD;
> else
--
Regards,
Laurent Pinchart
Re: [PATCH] drm/bridge: imx: Remove redundant checks on existence of bridge->encoder
Hi Sui,
Thank you for the patch.
On Sat, May 11, 2024 at 11:08:16PM +0800, Sui Jingfeng wrote:
> The check on the existence of bridge->encoder on the implementation layer
> of drm bridge driver is not necessary, as it has already been done in the
> drm_bridge_attach() function. It is guaranteed that the .encoder member
> of the drm_bridge instance is not NULL when various imx_xxx_bridge_attach()
> function gets called.
>
> Remove the redundant checking codes "if (!bridge->encoder) { ... }".
>
> Signed-off-by: Sui Jingfeng
Reviewed-by: Laurent Pinchart
> ---
> drivers/gpu/drm/bridge/imx/imx-ldb-helper.c | 5 -
> drivers/gpu/drm/bridge/imx/imx8qxp-pixel-combiner.c | 5 -
> drivers/gpu/drm/bridge/imx/imx8qxp-pixel-link.c | 5 -
> drivers/gpu/drm/bridge/imx/imx8qxp-pxl2dpi.c| 5 -
> 4 files changed, 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/imx/imx-ldb-helper.c
> b/drivers/gpu/drm/bridge/imx/imx-ldb-helper.c
> index 6967325cd8ee..9b5bebbe357d 100644
> --- a/drivers/gpu/drm/bridge/imx/imx-ldb-helper.c
> +++ b/drivers/gpu/drm/bridge/imx/imx-ldb-helper.c
> @@ -116,11 +116,6 @@ int ldb_bridge_attach_helper(struct drm_bridge *bridge,
> return -EINVAL;
> }
>
> - if (!bridge->encoder) {
> - DRM_DEV_ERROR(ldb->dev, "missing encoder\n");
> - return -ENODEV;
> - }
> -
> return drm_bridge_attach(bridge->encoder,
> ldb_ch->next_bridge, bridge,
> DRM_BRIDGE_ATTACH_NO_CONNECTOR);
> diff --git a/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-combiner.c
> b/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-combiner.c
> index d0868a6ac6c9..e6dbbdc87ce2 100644
> --- a/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-combiner.c
> +++ b/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-combiner.c
> @@ -119,11 +119,6 @@ static int imx8qxp_pc_bridge_attach(struct drm_bridge
> *bridge,
> return -EINVAL;
> }
>
> - if (!bridge->encoder) {
> - DRM_DEV_ERROR(pc->dev, "missing encoder\n");
> - return -ENODEV;
> - }
> -
> return drm_bridge_attach(bridge->encoder,
>ch->next_bridge, bridge,
>DRM_BRIDGE_ATTACH_NO_CONNECTOR);
> diff --git a/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-link.c
> b/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-link.c
> index ed8b7a4e0e11..1d11cc1df43c 100644
> --- a/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-link.c
> +++ b/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-link.c
> @@ -138,11 +138,6 @@ static int imx8qxp_pixel_link_bridge_attach(struct
> drm_bridge *bridge,
> return -EINVAL;
> }
>
> - if (!bridge->encoder) {
> - DRM_DEV_ERROR(pl->dev, "missing encoder\n");
> - return -ENODEV;
> - }
> -
> return drm_bridge_attach(bridge->encoder,
>pl->next_bridge, bridge,
>DRM_BRIDGE_ATTACH_NO_CONNECTOR);
> diff --git a/drivers/gpu/drm/bridge/imx/imx8qxp-pxl2dpi.c
> b/drivers/gpu/drm/bridge/imx/imx8qxp-pxl2dpi.c
> index 4a886cb808ca..fb7cf4369bb8 100644
> --- a/drivers/gpu/drm/bridge/imx/imx8qxp-pxl2dpi.c
> +++ b/drivers/gpu/drm/bridge/imx/imx8qxp-pxl2dpi.c
> @@ -58,11 +58,6 @@ static int imx8qxp_pxl2dpi_bridge_attach(struct drm_bridge
> *bridge,
> return -EINVAL;
> }
>
> - if (!bridge->encoder) {
> - DRM_DEV_ERROR(p2d->dev, "missing encoder\n");
> - return -ENODEV;
> - }
> -
> return drm_bridge_attach(bridge->encoder,
>p2d->next_bridge, bridge,
>DRM_BRIDGE_ATTACH_NO_CONNECTOR);
--
Regards,
Laurent Pinchart
Re: [PATCH] drm/bridge: analogix: Remove redundant checks on existence of bridge->encoder
Hi Sui,
Thank you for the patch.
On Sat, May 11, 2024 at 11:22:22PM +0800, Sui Jingfeng wrote:
> The check on the existence of bridge->encoder on the implementation layer
> of drm bridge driver is not necessary, as it has already been done in the
> drm_bridge_attach() function. It is guaranteed that the .encoder member
> of the drm_bridge instance is not NULL when various an_bridge_attach()
> function gets called. And .atomic_enable() of struct drm_bridge_funcs
> shouldn't be able to called before the various is acctached.
>
> Remove the redundant checking codes "if (!bridge->encoder) { ... }".
>
> Signed-off-by: Sui Jingfeng
Reviewed-by: Laurent Pinchart
> ---
> drivers/gpu/drm/bridge/analogix/analogix-anx6345.c | 5 -
> drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c | 5 -
> drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 5 -
> drivers/gpu/drm/bridge/analogix/anx7625.c | 10 --
> 4 files changed, 25 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix-anx6345.c
> b/drivers/gpu/drm/bridge/analogix/analogix-anx6345.c
> index c9e35731e6a1..cfe43d2ca3be 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix-anx6345.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix-anx6345.c
> @@ -528,11 +528,6 @@ static int anx6345_bridge_attach(struct drm_bridge
> *bridge,
> return -EINVAL;
> }
>
> - if (!bridge->encoder) {
> - DRM_ERROR("Parent encoder object not found");
> - return -ENODEV;
> - }
> -
> /* Register aux channel */
> anx6345->aux.name = "DP-AUX";
> anx6345->aux.dev = >client->dev;
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c
> b/drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c
> index 5748a8581af4..58875dde496f 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c
> @@ -897,11 +897,6 @@ static int anx78xx_bridge_attach(struct drm_bridge
> *bridge,
> return -EINVAL;
> }
>
> - if (!bridge->encoder) {
> - DRM_ERROR("Parent encoder object not found");
> - return -ENODEV;
> - }
> -
> /* Register aux channel */
> anx78xx->aux.name = "DP-AUX";
> anx78xx->aux.dev = >client->dev;
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> index df9370e0ff23..7b841232321f 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> @@ -1228,11 +1228,6 @@ static int analogix_dp_bridge_attach(struct drm_bridge
> *bridge,
> return -EINVAL;
> }
>
> - if (!bridge->encoder) {
> - DRM_ERROR("Parent encoder object not found");
> - return -ENODEV;
> - }
> -
> if (!dp->plat_data->skip_connector) {
> connector = >connector;
> connector->polled = DRM_CONNECTOR_POLL_HPD;
> diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c
> b/drivers/gpu/drm/bridge/analogix/anx7625.c
> index 59e9ad349969..3d09efa4199c 100644
> --- a/drivers/gpu/drm/bridge/analogix/anx7625.c
> +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
> @@ -2193,11 +2193,6 @@ static int anx7625_bridge_attach(struct drm_bridge
> *bridge,
> if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR))
> return -EINVAL;
>
> - if (!bridge->encoder) {
> - DRM_DEV_ERROR(dev, "Parent encoder object not found");
> - return -ENODEV;
> - }
> -
> ctx->aux.drm_dev = bridge->dev;
> err = drm_dp_aux_register(>aux);
> if (err) {
> @@ -2435,11 +2430,6 @@ static void anx7625_bridge_atomic_enable(struct
> drm_bridge *bridge,
>
> dev_dbg(dev, "drm atomic enable\n");
>
> - if (!bridge->encoder) {
> - dev_err(dev, "Parent encoder object not found");
> - return;
> - }
> -
> connector = drm_atomic_get_new_connector_for_encoder(state->base.state,
>bridge->encoder);
> if (!connector)
--
Regards,
Laurent Pinchart
Re: [PATCH] drm/bridge: it6505: Remove a redundant check on existence of bridge->encoder
Hi Sui,
Thank you for the patch.
On Sat, May 11, 2024 at 10:10:56PM +0800, Sui Jingfeng wrote:
> In it6505_bridge_attach(), the check on the existence of bridge->encoder
> has already been done in the implementation of drm_bridge_attach(). And
> it is done before the bridge->funcs->attach function hook is called. Hence,
> it is guaranteed that the .encoder member of the struct drm_bridge is not
> NULL when the panel_bridge_attach() is called.
>
> There is no need to check the existence of bridge->encoder another time,
> remove the redundant checking codes "if (!bridge->encoder) { ... }".
>
> Signed-off-by: Sui Jingfeng
Reviewed-by: Laurent Pinchart
> ---
> drivers/gpu/drm/bridge/ite-it6505.c | 5 -
> 1 file changed, 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ite-it6505.c
> b/drivers/gpu/drm/bridge/ite-it6505.c
> index 27334173e911..494030a75dba 100644
> --- a/drivers/gpu/drm/bridge/ite-it6505.c
> +++ b/drivers/gpu/drm/bridge/ite-it6505.c
> @@ -2881,11 +2881,6 @@ static int it6505_bridge_attach(struct drm_bridge
> *bridge,
> return -EINVAL;
> }
>
> - if (!bridge->encoder) {
> - dev_err(dev, "Parent encoder object not found");
> - return -ENODEV;
> - }
> -
> /* Register aux channel */
> it6505->aux.drm_dev = bridge->dev;
>
--
Regards,
Laurent Pinchart
Re: [PATCH] drm/bridge: panel: Remove a redundant check on existence of bridge->encoder
Hi Sui,
Thank you for the patch.
On Sat, May 11, 2024 at 10:03:16PM +0800, Sui Jingfeng wrote:
> In panel_bridge_attach(), the check on the existence of bridge->encoder
> has already been done in the implementation of drm_bridge_attach(). And
> it is done before the bridge->funcs->attach hook is called. Hence, it is
> guaranteed that the .encoder member of the struct drm_bridge is not NULL
> when the panel_bridge_attach() is called.
>
> There is no need to check the existence of bridge->encoder another time
> at the implementation layer, therefore remove the redundant checking codes
> "if (!bridge->encoder) { ... }".
>
> Signed-off-by: Sui Jingfeng
Reviewed-by: Laurent Pinchart
> ---
> drivers/gpu/drm/bridge/panel.c | 5 -
> 1 file changed, 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c
> index 7f41525f7a6e..762402dca6dd 100644
> --- a/drivers/gpu/drm/bridge/panel.c
> +++ b/drivers/gpu/drm/bridge/panel.c
> @@ -65,11 +65,6 @@ static int panel_bridge_attach(struct drm_bridge *bridge,
> if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)
> return 0;
>
> - if (!bridge->encoder) {
> - DRM_ERROR("Missing encoder\n");
> - return -ENODEV;
> - }
> -
> drm_connector_helper_add(connector,
>_bridge_connector_helper_funcs);
>
--
Regards,
Laurent Pinchart
Re: [PATCH] drm/bridge: nxp-ptn3460: Remove a small useless code snippet
Hi Sui,
Thank you for the patch.
On Sat, May 11, 2024 at 09:42:50PM +0800, Sui Jingfeng wrote:
> In ptn3460_bridge_attach(), the check on the existence of bridge->encoder
> has already been done in the implementation of drm_bridge_attach(). The
> driver won't go further if bridge->encoder is NULL and the driver will quit
> even if drm_bridge_attach() fails for some reasons. Thereforei, there is
s/Thereforei/Therefore/
> no need to check another time at the later, remove the redundant checking
> codes "if (!bridge->encoder) { ... }".
>
> Signed-off-by: Sui Jingfeng
Reviewed-by: Laurent Pinchart
> ---
> drivers/gpu/drm/bridge/nxp-ptn3460.c | 5 -
> 1 file changed, 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/nxp-ptn3460.c
> b/drivers/gpu/drm/bridge/nxp-ptn3460.c
> index ed93fd4c3265..e77aab965fcf 100644
> --- a/drivers/gpu/drm/bridge/nxp-ptn3460.c
> +++ b/drivers/gpu/drm/bridge/nxp-ptn3460.c
> @@ -229,11 +229,6 @@ static int ptn3460_bridge_attach(struct drm_bridge
> *bridge,
> if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)
> return 0;
>
> - if (!bridge->encoder) {
> - DRM_ERROR("Parent encoder object not found");
> - return -ENODEV;
> - }
> -
> ptn_bridge->connector.polled = DRM_CONNECTOR_POLL_HPD;
> ret = drm_connector_init(bridge->dev, _bridge->connector,
> _connector_funcs, DRM_MODE_CONNECTOR_LVDS);
--
Regards,
Laurent Pinchart
Re: [PATCH] drm/bridge: tfp410: Remove a small useless code snippet
Hi Sui,
Thank you for the patch.
On Sat, May 11, 2024 at 09:24:23PM +0800, Sui Jingfeng wrote:
> In the tfp410_attach(), the check on the existence of bridge->encoder has
> already been done in the implementation of drm_bridge_attach() function.
> The driver won't go further if bridge->encoder is NULL and the driver will
> quit even if drm_bridge_attach() fails for some reasons.
>
> Therefore there is no need to check another time at the later, remove the
> redundant checking codes "if (!bridge->encoder) { ... }".
>
> Signed-off-by: Sui Jingfeng
Reviewed-by: Laurent Pinchart
> ---
> drivers/gpu/drm/bridge/ti-tfp410.c | 5 -
> 1 file changed, 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ti-tfp410.c
> b/drivers/gpu/drm/bridge/ti-tfp410.c
> index c7bef5c23927..b1b1e4d5a24a 100644
> --- a/drivers/gpu/drm/bridge/ti-tfp410.c
> +++ b/drivers/gpu/drm/bridge/ti-tfp410.c
> @@ -133,11 +133,6 @@ static int tfp410_attach(struct drm_bridge *bridge,
> if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)
> return 0;
>
> - if (!bridge->encoder) {
> - dev_err(dvi->dev, "Missing encoder\n");
> - return -ENODEV;
> - }
> -
> if (dvi->next_bridge->ops & DRM_BRIDGE_OP_DETECT)
> dvi->connector.polled = DRM_CONNECTOR_POLL_HPD;
> else
--
Regards,
Laurent Pinchart
Re: [PATCH] drm/bridge: Remove a small useless code snippet
Hi Sui,
Thank you for the patch.
On Sat, May 11, 2024 at 08:42:38PM +0800, Sui Jingfeng wrote:
> Because the check on the non-existence (encoder == NULL) has already been
> done in the implementation of drm_bridge_attach() function, and
> drm_bridge_attach() is called earlier. The driver won't get to check point
> even if drm_bridge_attach() fails for some reasons, as it will clear the
> bridge->encoder to NULL and return a negective error code.
s/negective/negative/
>
> Therefore, there is no need to check another again. Remove the redundant
> codes at the later.
>
> Signed-off-by: Sui Jingfeng
Reviewed-by: Laurent Pinchart
If you end up sending a second version of this patch, please include all
similar patches you have sent at the same time in a patch series,
instead of sending them separately.
> ---
> drivers/gpu/drm/bridge/simple-bridge.c | 5 -
> 1 file changed, 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/simple-bridge.c
> b/drivers/gpu/drm/bridge/simple-bridge.c
> index 28376d0ecd09..3caa918ac2e0 100644
> --- a/drivers/gpu/drm/bridge/simple-bridge.c
> +++ b/drivers/gpu/drm/bridge/simple-bridge.c
> @@ -116,11 +116,6 @@ static int simple_bridge_attach(struct drm_bridge
> *bridge,
> if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)
> return 0;
>
> - if (!bridge->encoder) {
> - DRM_ERROR("Missing encoder\n");
> - return -ENODEV;
> - }
> -
> drm_connector_helper_add(>connector,
> _bridge_con_helper_funcs);
> ret = drm_connector_init_with_ddc(bridge->dev, >connector,
--
Regards,
Laurent Pinchart
Re: [PATCH 2/4] dt-bindings: display: ti: Add schema for AM625 OLDI Transmitter
ti,oldi-io-ctrl = <_oldi_io_ctrl>;
> +ports {
> +#address-cells = <1>;
> +#size-cells = <0>;
> +port@0 {
> +reg = <0>;
> +oldi0_in: endpoint {
> +remote-endpoint = <_out0>;
> +};
> +};
> +};
> +};
> +oldi1: oldi@1 {
> +reg = <1>;
> +ti,secondary-oldi;
> +ports {
> +#address-cells = <1>;
> +#size-cells = <0>;
> +port@0 {
> +reg = <0>;
> +oldi1_in: endpoint {
> +remote-endpoint = <_out1>;
> +};
> +};
> +};
> +};
> +};
> +
> +...
> diff --git a/MAINTAINERS b/MAINTAINERS
> index c675fc296b19..4426c4d41a7f 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7480,6 +7480,7 @@ M: Tomi Valkeinen
> L: dri-devel@lists.freedesktop.org
> S: Maintained
> T: git https://gitlab.freedesktop.org/drm/misc/kernel.git
> +F: Documentation/devicetree/bindings/display/ti/ti,am625-oldi.yaml
> F: Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
> F: Documentation/devicetree/bindings/display/ti/ti,j721e-dss.yaml
> F: Documentation/devicetree/bindings/display/ti/ti,k2g-dss.yaml
--
Regards,
Laurent Pinchart
Re: [PATCH 1/4] dt-bindings: display: ti,am65x-dss: Minor Cleanup
Hi Aradhya,
Thank you for the patch.
On Sun, May 12, 2024 at 01:00:52AM +0530, Aradhya Bhatia wrote:
> Reduce tab size from 8 spaces to 4 spaces to make the bindings
> consistent, and easy to expand.
>
> Signed-off-by: Aradhya Bhatia
Reviewed-by: Laurent Pinchart
> ---
> .../bindings/display/ti/ti,am65x-dss.yaml | 54 +--
> 1 file changed, 27 insertions(+), 27 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
> b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
> index 55e3e490d0e6..399d68986326 100644
> --- a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
> +++ b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
> @@ -142,32 +142,32 @@ examples:
> #include
>
> dss: dss@4a0 {
> -compatible = "ti,am65x-dss";
> -reg = <0x04a0 0x1000>, /* common */
> -<0x04a02000 0x1000>, /* vidl1 */
> -<0x04a06000 0x1000>, /* vid */
> -<0x04a07000 0x1000>, /* ovr1 */
> -<0x04a08000 0x1000>, /* ovr2 */
> -<0x04a0a000 0x1000>, /* vp1 */
> -<0x04a0b000 0x1000>, /* vp2 */
> -<0x04a01000 0x1000>; /* common1 */
> -reg-names = "common", "vidl1", "vid",
> -"ovr1", "ovr2", "vp1", "vp2", "common1";
> -ti,am65x-oldi-io-ctrl = <_oldi_io_ctrl>;
> -power-domains = <_pds 67 TI_SCI_PD_EXCLUSIVE>;
> -clocks =<_clks 67 1>,
> -<_clks 216 1>,
> -<_clks 67 2>;
> -clock-names = "fck", "vp1", "vp2";
> -interrupts = ;
> -ports {
> -#address-cells = <1>;
> -#size-cells = <0>;
> -port@0 {
> -reg = <0>;
> -oldi_out0: endpoint {
> -remote-endpoint = <_in0>;
> -};
> -};
> +compatible = "ti,am65x-dss";
> +reg = <0x04a0 0x1000>, /* common */
> + <0x04a02000 0x1000>, /* vidl1 */
> + <0x04a06000 0x1000>, /* vid */
> + <0x04a07000 0x1000>, /* ovr1 */
> + <0x04a08000 0x1000>, /* ovr2 */
> + <0x04a0a000 0x1000>, /* vp1 */
> + <0x04a0b000 0x1000>, /* vp2 */
> + <0x04a01000 0x1000>; /* common1 */
> +reg-names = "common", "vidl1", "vid",
> +"ovr1", "ovr2", "vp1", "vp2", "common1";
> +ti,am65x-oldi-io-ctrl = <_oldi_io_ctrl>;
> +power-domains = <_pds 67 TI_SCI_PD_EXCLUSIVE>;
> +clocks =<_clks 67 1>,
> +<_clks 216 1>,
> +<_clks 67 2>;
> +clock-names = "fck", "vp1", "vp2";
> +interrupts = ;
> +ports {
> +#address-cells = <1>;
> +#size-cells = <0>;
> +port@0 {
> +reg = <0>;
> +oldi_out0: endpoint {
> +remote-endpoint = <_in0>;
> +};
> };
> +};
> };
--
Regards,
Laurent Pinchart
Re: [PATCH v7 8/8] gpu: ipu-v3: Use generic macro for rounding to nearest multiple
On Fri, May 10, 2024 at 06:03:52PM +0300, Andy Shevchenko wrote: > On Fri, May 10, 2024 at 12:10:10AM +0530, Devarsh Thakkar wrote: > > Use generic macro round_closest_up for rounding to nearest multiple instead > > round_closest_up() > > We refer to the functions as func(). > > > of using local function. > > ... > > > @@ -565,7 +563,7 @@ static void find_best_seam(struct ipu_image_convert_ctx > > *ctx, > > * The closest input sample position that we could actually > > * start the input tile at, 19.13 fixed point. > > */ > > - in_pos_aligned = round_closest(in_pos, 8192U * in_align); > > + in_pos_aligned = round_closest_up(in_pos, 8192U * in_align); > > /* Convert 19.13 fixed point to integer */ > > in_pos_rounded = in_pos_aligned / 8192U; > > Oh, these seems to be better to use either ALIGN*(), or PFN_*() / PAGE_*() > families of macros. What the semantic of 8192 is? The comment mentions 19.13 fixed point, so I assume that's the fractional part of the integer. It doesn't seem related to pages. -- Regards, Laurent Pinchart
Re: Safety of opening up /dev/dma_heap/* to physically present users (udev uaccess tag) ?
On Wed, May 08, 2024 at 10:39:58AM +0200, Daniel Vetter wrote: > On Tue, May 07, 2024 at 10:59:42PM +0300, Dmitry Baryshkov wrote: > > On Tue, 7 May 2024 at 21:40, Laurent Pinchart wrote: > > > On Tue, May 07, 2024 at 06:19:18PM +0300, Dmitry Baryshkov wrote: > > > > On Tue, 7 May 2024 at 18:15, Bryan O'Donoghue wrote: > > > > > On 07/05/2024 16:09, Dmitry Baryshkov wrote: > > > > > > Ah, I see. Then why do you require the DMA-ble buffer at all? If > > > > > > you are > > > > > > providing data to VPU or DRM, then you should be able to get the > > > > > > buffer > > > > > > from the data-consuming device. > > > > > > > > > > Because we don't necessarily know what the consuming device is, if > > > > > any. > > > > > > > > > > Could be VPU, could be Zoom/Hangouts via pipewire, could for argument > > > > > sake be GPU or DSP. > > > > > > > > > > Also if we introduce a dependency on another device to allocate the > > > > > output buffers - say always taking the output buffer from the GPU, > > > > > then > > > > > we've added another dependency which is more difficult to guarantee > > > > > across different arches. > > > > > > > > Yes. And it should be expected. It's a consumer who knows the > > > > restrictions on the buffer. As I wrote, Zoom/Hangouts should not > > > > require a DMA buffer at all. > > > > > > Why not ? If you want to capture to a buffer that you then compose on > > > the screen without copying data, dma-buf is the way to go. That's the > > > Linux solution for buffer sharing. > > > > Yes. But it should be allocated by the DRM driver. As Sima wrote, > > there is no guarantee that the buffer allocated from dma-heaps is > > accessible to the GPU. > > > > > > > > > Applications should be able to allocate > > > > the buffer out of the generic memory. > > > > > > If applications really want to copy data and degrade performance, they > > > are free to shoot themselves in the foot of course. Applications (or > > > compositors) need to support copying as a fallback in the worst case, > > > but all components should at least aim for the zero-copy case. > > > > I'd say that they should aim for the optimal case. It might include > > both zero-copying access from another DMA master or simple software > > processing of some kind. > > > > > > GPUs might also have different > > > > requirements. Consider GPUs with VRAM. It might be beneficial to > > > > allocate a buffer out of VRAM rather than generic DMA mem. > > > > > > Absolutely. For that we need a centralized device memory allocator in > > > userspace. An effort was started by James Jones in 2016, see [1]. It has > > > unfortunately stalled. If I didn't have a camera framework to develop, I > > > would try to tackle that issue :-) > > > > I'll review the talk. However the fact that the effort has stalled > > most likely means that 'one fits them all' approach didn't really fly > > well. We have too many usecases. > > I think there's two reasons: > > - It's a really hard problem with many aspects. Where you need to allocate > the buffer is just one of the myriad of issues a common allocator needs > to solve. The other large problem is picking up an optimal pixel format. I wonder if that could be decoupled from the allocation. That could help moving forward. > - Every linux-based os has their own solution for these, and the one that > suffers most has an entirely different one from everyone else: Android > uses binder services to allow apps to make these allocations, keep track > of them and make sure there's no abuse. And if there is, it can just > nuke the app. -- Regards, Laurent Pinchart
Re: Safety of opening up /dev/dma_heap/* to physically present users (udev uaccess tag) ?
On Thu, May 09, 2024 at 12:51:08AM +0300, Laurent Pinchart wrote: > On Wed, May 08, 2024 at 10:36:08AM +0200, Daniel Vetter wrote: > > On Tue, May 07, 2024 at 04:07:39PM -0400, Nicolas Dufresne wrote: > > > Le mardi 07 mai 2024 à 21:36 +0300, Laurent Pinchart a écrit : > > > > Shorter term, we have a problem to solve, and the best option we have > > > > found so far is to rely on dma-buf heaps as a backend for the frame > > > > buffer allocatro helper in libcamera for the use case described above. > > > > This won't work in 100% of the cases, clearly. It's a stop-gap measure > > > > until we can do better. > > > > > > Considering the security concerned raised on this thread with dmabuf heap > > > allocation not be restricted by quotas, you'd get what you want quickly > > > with > > > memfd + udmabuf instead (which is accounted already). > > > > > > It was raised that distro don't enable udmabuf, but as stated there by > > > Hans, in > > > any cases distro needs to take action to make the softISP works. This > > > alternative is easy and does not interfere in anyway with your future > > > plan or > > > the libcamera API. You could even have both dmabuf heap (for Raspbian) > > > and the > > > safer memfd+udmabuf for the distro with security concerns. > > > > > > And for the long term plan, we can certainly get closer by fixing that > > > issue > > > with accounting. This issue also applied to v4l2 io-ops, so it would be > > > nice to > > > find common set of helpers to fix these exporters. > > > > Yeah if this is just for softisp, then memfd + udmabuf is also what I was > > about to suggest. Not just as a stopgap, but as the real official thing. > > Long term I still want a centralized memory allocator, at which point > libcamera should stop allocating buffers at all. And to be clear, udmabuf could be fine for the time being. At least as long as we don't find any shortcoming while testing it :-) > > udmabuf does kinda allow you to pin memory, but we can easily fix that by > > adding the right accounting and then either let mlock rlimits or cgroups > > kernel memory limits enforce good behavior. -- Regards, Laurent Pinchart
Re: Safety of opening up /dev/dma_heap/* to physically present users (udev uaccess tag) ?
On Wed, May 08, 2024 at 10:36:08AM +0200, Daniel Vetter wrote: > On Tue, May 07, 2024 at 04:07:39PM -0400, Nicolas Dufresne wrote: > > Le mardi 07 mai 2024 à 21:36 +0300, Laurent Pinchart a écrit : > > > Shorter term, we have a problem to solve, and the best option we have > > > found so far is to rely on dma-buf heaps as a backend for the frame > > > buffer allocatro helper in libcamera for the use case described above. > > > This won't work in 100% of the cases, clearly. It's a stop-gap measure > > > until we can do better. > > > > Considering the security concerned raised on this thread with dmabuf heap > > allocation not be restricted by quotas, you'd get what you want quickly with > > memfd + udmabuf instead (which is accounted already). > > > > It was raised that distro don't enable udmabuf, but as stated there by > > Hans, in > > any cases distro needs to take action to make the softISP works. This > > alternative is easy and does not interfere in anyway with your future plan > > or > > the libcamera API. You could even have both dmabuf heap (for Raspbian) and > > the > > safer memfd+udmabuf for the distro with security concerns. > > > > And for the long term plan, we can certainly get closer by fixing that issue > > with accounting. This issue also applied to v4l2 io-ops, so it would be > > nice to > > find common set of helpers to fix these exporters. > > Yeah if this is just for softisp, then memfd + udmabuf is also what I was > about to suggest. Not just as a stopgap, but as the real official thing. Long term I still want a centralized memory allocator, at which point libcamera should stop allocating buffers at all. > udmabuf does kinda allow you to pin memory, but we can easily fix that by > adding the right accounting and then either let mlock rlimits or cgroups > kernel memory limits enforce good behavior. -- Regards, Laurent Pinchart
Re: [PATCH] drm/drm-bridge.c: Drop conditionals around of_node pointers
On Wed, May 08, 2024 at 02:00:00AM +0800, Sui Jingfeng wrote:
> Having conditional around the of_node pointer of the drm_bridge structure
> is not necessary, since drm_bridge structure always has the of_node as its
> member.
>
> Let's drop the conditional to get a better looks, please also note that
> this is following the already accepted commitments. see commit d8dfccde2709
> ("drm/bridge: Drop conditionals around of_node pointers") for reference.
>
> Signed-off-by: Sui Jingfeng
It looks like this was forgotten in commit d8dfccde2709 ("drm/bridge:
Drop conditionals around of_node pointers").
Reviewed-by: Laurent Pinchart
> ---
> drivers/gpu/drm/drm_bridge.c | 5 -
> 1 file changed, 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index 30d66bee0ec6..a6dbe1751e88 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -352,13 +352,8 @@ int drm_bridge_attach(struct drm_encoder *encoder,
> struct drm_bridge *bridge,
> bridge->encoder = NULL;
> list_del(>chain_node);
>
> -#ifdef CONFIG_OF
> DRM_ERROR("failed to attach bridge %pOF to encoder %s: %d\n",
> bridge->of_node, encoder->name, ret);
> -#else
> - DRM_ERROR("failed to attach bridge to encoder %s: %d\n",
> - encoder->name, ret);
> -#endif
>
> return ret;
> }
--
Regards,
Laurent Pinchart
Re: Safety of opening up /dev/dma_heap/* to physically present users (udev uaccess tag) ?
On Tue, May 07, 2024 at 10:59:42PM +0300, Dmitry Baryshkov wrote: > On Tue, 7 May 2024 at 21:40, Laurent Pinchart wrote: > > On Tue, May 07, 2024 at 06:19:18PM +0300, Dmitry Baryshkov wrote: > > > On Tue, 7 May 2024 at 18:15, Bryan O'Donoghue wrote: > > > > On 07/05/2024 16:09, Dmitry Baryshkov wrote: > > > > > Ah, I see. Then why do you require the DMA-ble buffer at all? If you > > > > > are > > > > > providing data to VPU or DRM, then you should be able to get the > > > > > buffer > > > > > from the data-consuming device. > > > > > > > > Because we don't necessarily know what the consuming device is, if any. > > > > > > > > Could be VPU, could be Zoom/Hangouts via pipewire, could for argument > > > > sake be GPU or DSP. > > > > > > > > Also if we introduce a dependency on another device to allocate the > > > > output buffers - say always taking the output buffer from the GPU, then > > > > we've added another dependency which is more difficult to guarantee > > > > across different arches. > > > > > > Yes. And it should be expected. It's a consumer who knows the > > > restrictions on the buffer. As I wrote, Zoom/Hangouts should not > > > require a DMA buffer at all. > > > > Why not ? If you want to capture to a buffer that you then compose on > > the screen without copying data, dma-buf is the way to go. That's the > > Linux solution for buffer sharing. > > Yes. But it should be allocated by the DRM driver. As Sima wrote, > there is no guarantee that the buffer allocated from dma-heaps is > accessible to the GPU. No disagreement there. From a libcamera point of view, we want to import buffers allocated externally. It's for use cases where applications can't provide dma buf instances easily that we need to allocate them through the libcamera buffer allocator helper. That helper has to a) provide dma buf fds, and b) make a best effort to allocate buffers that will have a decent chance of being usable by other devices. We're open to exploring other solutions than dma heaps, although I wonder what the dma heaps are for if nobody enables them :-) > > > Applications should be able to allocate > > > the buffer out of the generic memory. > > > > If applications really want to copy data and degrade performance, they > > are free to shoot themselves in the foot of course. Applications (or > > compositors) need to support copying as a fallback in the worst case, > > but all components should at least aim for the zero-copy case. > > I'd say that they should aim for the optimal case. It might include > both zero-copying access from another DMA master or simple software > processing of some kind. > > > > GPUs might also have different > > > requirements. Consider GPUs with VRAM. It might be beneficial to > > > allocate a buffer out of VRAM rather than generic DMA mem. > > > > Absolutely. For that we need a centralized device memory allocator in > > userspace. An effort was started by James Jones in 2016, see [1]. It has > > unfortunately stalled. If I didn't have a camera framework to develop, I > > would try to tackle that issue :-) > > I'll review the talk. However the fact that the effort has stalled > most likely means that 'one fits them all' approach didn't really fly > well. We have too many usecases. > > > [1] > > https://www.x.org/wiki/Events/XDC2016/Program/Unix_Device_Memory_Allocation.pdf -- Regards, Laurent Pinchart
Re: Safety of opening up /dev/dma_heap/* to physically present users (udev uaccess tag) ?
On Mon, May 06, 2024 at 03:38:24PM +0200, Daniel Vetter wrote: > On Mon, May 06, 2024 at 02:05:12PM +0200, Maxime Ripard wrote: > > On Mon, May 06, 2024 at 01:49:17PM GMT, Hans de Goede wrote: > > > Hi dma-buf maintainers, et.al., > > > > > > Various people have been working on making complex/MIPI cameras work OOTB > > > with mainline Linux kernels and an opensource userspace stack. > > > > > > The generic solution adds a software ISP (for Debayering and 3A) to > > > libcamera. Libcamera's API guarantees that buffers handed to applications > > > using it are dma-bufs so that these can be passed to e.g. a video encoder. > > > > > > In order to meet this API guarantee the libcamera software ISP allocates > > > dma-bufs from userspace through one of the /dev/dma_heap/* heaps. For > > > the Fedora COPR repo for the PoC of this: > > > https://hansdegoede.dreamwidth.org/28153.html > > > > For the record, we're also considering using them for ARM KMS devices, > > so it would be better if the solution wasn't only considering v4l2 > > devices. > > > > > I have added a simple udev rule to give physically present users access > > > to the dma_heap-s: > > > > > > KERNEL=="system", SUBSYSTEM=="dma_heap", TAG+="uaccess" > > > > > > (and on Rasperry Pi devices any users in the video group get access) > > > > > > This was just a quick fix for the PoC. Now that we are ready to move out > > > of the PoC phase and start actually integrating this into distributions > > > the question becomes if this is an acceptable solution; or if we need some > > > other way to deal with this ? > > > > > > Specifically the question is if this will have any negative security > > > implications? I can certainly see this being used to do some sort of > > > denial of service attack on the system (1). This is especially true for > > > the cma heap which generally speaking is a limited resource. > > > > There's plenty of other ways to exhaust CMA, like allocating too much > > KMS or v4l2 buffers. I'm not sure we should consider dma-heaps > > differently than those if it's part of our threat model. > > So generally for an arm soc where your display needs cma, your render node > doesn't. And user applications only have access to the later, while only > the compositor gets a kms fd through logind. At least in drm aside from > vc4 there's really no render driver that just gives you access to cma and > allows you to exhaust that, you need to be a compositor with drm master > access to the display. > > Which means we're mostly protected against bad applications, and that's > not a threat the "user physically sits in front of the machine accounts > for", and which giving cma access to everyone would open up. And with > flathub/snaps/... this is very much an issue. > > So you need more, either: > > - cgroups limits on dma-buf and dma-buf heaps. This has been bikeshedded > for years and is just not really moving. > > - An allocator service which checks whether you're allowed to allocate > these special buffers. Android does that through binder. I would split the latter into a centralized frame buffer allocator library for userspace (James Jones' Unix device memory allocator comes to mind), providing dma-buf instances using whatever backend is available and suitable (DMA heaps would likely be one of them), and a safe way to make this allocator available to applications. Exposing it through some system services could be useful, but with proper tracking and accounting of memory allocated through DMA heaps (and DRM, and V4L2) we could possibly do with just a library. What I really want is to move memory allocation for devices to a separate component, and make everything else a consumer of those buffers. > Probably also some way to nuke applications that refuse to release buffers > when they're no longer the right application. cgroups is a lot more > convenient for that. > > > > But devices tagged for uaccess are only opened up to users who are > > > physcially present behind the machine and those can just hit > > > the powerbutton, so I don't believe that any *on purpose* DOS is part of > > > the thread model. > > > > How would that work for headless devices? -- Regards, Laurent Pinchart
Re: Safety of opening up /dev/dma_heap/* to physically present users (udev uaccess tag) ?
On Tue, May 07, 2024 at 06:19:18PM +0300, Dmitry Baryshkov wrote: > On Tue, 7 May 2024 at 18:15, Bryan O'Donoghue wrote: > > On 07/05/2024 16:09, Dmitry Baryshkov wrote: > > > Ah, I see. Then why do you require the DMA-ble buffer at all? If you are > > > providing data to VPU or DRM, then you should be able to get the buffer > > > from the data-consuming device. > > > > Because we don't necessarily know what the consuming device is, if any. > > > > Could be VPU, could be Zoom/Hangouts via pipewire, could for argument > > sake be GPU or DSP. > > > > Also if we introduce a dependency on another device to allocate the > > output buffers - say always taking the output buffer from the GPU, then > > we've added another dependency which is more difficult to guarantee > > across different arches. > > Yes. And it should be expected. It's a consumer who knows the > restrictions on the buffer. As I wrote, Zoom/Hangouts should not > require a DMA buffer at all. Why not ? If you want to capture to a buffer that you then compose on the screen without copying data, dma-buf is the way to go. That's the Linux solution for buffer sharing. > Applications should be able to allocate > the buffer out of the generic memory. If applications really want to copy data and degrade performance, they are free to shoot themselves in the foot of course. Applications (or compositors) need to support copying as a fallback in the worst case, but all components should at least aim for the zero-copy case. > GPUs might also have different > requirements. Consider GPUs with VRAM. It might be beneficial to > allocate a buffer out of VRAM rather than generic DMA mem. Absolutely. For that we need a centralized device memory allocator in userspace. An effort was started by James Jones in 2016, see [1]. It has unfortunately stalled. If I didn't have a camera framework to develop, I would try to tackle that issue :-) [1] https://www.x.org/wiki/Events/XDC2016/Program/Unix_Device_Memory_Allocation.pdf -- Regards, Laurent Pinchart
Re: Safety of opening up /dev/dma_heap/* to physically present users (udev uaccess tag) ?
On Tue, May 07, 2024 at 07:36:59PM +0200, Daniel Vetter wrote: > On Tue, May 07, 2024 at 04:15:05PM +0100, Bryan O'Donoghue wrote: > > On 07/05/2024 16:09, Dmitry Baryshkov wrote: > > > Ah, I see. Then why do you require the DMA-ble buffer at all? If you are > > > providing data to VPU or DRM, then you should be able to get the buffer > > > from the data-consuming device. > > > > Because we don't necessarily know what the consuming device is, if any. > > Well ... that's an entirely different issue. And it's unsolved. > > Currently the approach is to allocate where the constraints are usually > most severe (like display, if you need that, or the camera module for > input) and then just pray the stack works out without too much copying. > All userspace (whether the generic glue or the userspace driver depends a > bit upon the exact api) does need to have a copy fallback for these > sharing cases, ideally with the copying accelerated by hw. > > If you try to solve this by just preemptive allocating everything as cma > buffers, then you'll make the situation substantially worse (because now > you're wasting tons of cma memory where you might not even need it). > And without really solving the problem, since for some gpus that memory > might be unusable (because you cannot scan that out on any discrete gpu, > and sometimes not even on an integrated one). I think we have a general agreement that the proposed solution is a stop-gap measure for an unsolved issue. Note that libcamera is already designed that way. The API is designed to import buffers, using dma-buf file handles. If an application has a way to allocate dma-buf instances through another means (from the display or from a video encoder for instance), it should do so, and use those buffers with libcamera. For applications that don't have an easy way to get hold of dma-buf instances, we have a buffer allocator helper as a side component. That allocator uses the underlying camera capture device, and allocates buffers from the V4L2 video device. It's only on platforms where we have no hardware camera processing (or, rather, platforms where the hardware vendors doesn't give us access to the camera hardware, such as recent Intel SoCs, or Qualcomm SoCs used in ARM laptops) that we need to allocate memory elsewhere. In the long run, I want a centralized memory allocator accessible by userspace applications (something similar in purpose to gralloc on Android), and I want to get rid of buffer allocation in libcamera (and even in V4L2, in the even longer term). That's the long run. Shorter term, we have a problem to solve, and the best option we have found so far is to rely on dma-buf heaps as a backend for the frame buffer allocatro helper in libcamera for the use case described above. This won't work in 100% of the cases, clearly. It's a stop-gap measure until we can do better. > > Could be VPU, could be Zoom/Hangouts via pipewire, could for argument sake > > be GPU or DSP. > > > > Also if we introduce a dependency on another device to allocate the output > > buffers - say always taking the output buffer from the GPU, then we've added > > another dependency which is more difficult to guarantee across different > > arches. -- Regards, Laurent Pinchart
Re: [BUG] drm: zynqmp_dp: Lockup in zynqmp_dp_bridge_detect when device is unbound
On Mon, May 06, 2024 at 10:57:17AM -0400, Sean Anderson wrote: > On 5/6/24 03:35, Laurent Pinchart wrote: > > On Mon, May 06, 2024 at 09:29:36AM +0200, Maxime Ripard wrote: > >> Hi Laurent, Sean, > >> > >> On Sat, May 04, 2024 at 03:21:18PM GMT, Laurent Pinchart wrote: > >> > On Fri, May 03, 2024 at 05:54:32PM -0400, Sean Anderson wrote: > >> > > I have discovered a bug in the displayport driver on drm-misc-next. To > >> > > trigger it, run > >> > > > >> > > echo fd4a.display > /sys/bus/platform/drivers/zynqmp-dpsub/unbind > >> > > > >> > > The system will become unresponsive and (after a bit) splat with a hard > >> > > LOCKUP. One core will be unresponsive at the first zynqmp_dp_read in > >> > > zynqmp_dp_bridge_detect. > >> > > > >> > > I believe the issue is due the registers being unmapped and the block > >> > > put into reset in zynqmp_dp_remove instead of zynqmp_dpsub_release. > >> > > >> > That is on purpose. Drivers are not allowed to access the device at all > >> > after .remove() returns. > >> > >> It's not "on purpose" no. Drivers indeed are not allowed to access the > >> device after remove, but the kernel shouldn't crash. This is exactly > >> why we have drm_dev_enter / drm_dev_exit. > > > > I didn't mean the crash was on purpose :-) It's the registers being > > unmapped that is, as nothing should touch those registers after > > .remove() returns. > > OK, so then we need to have some kind of flag in the driver or in the drm > subsystem so we know not to access those registers. To avoid race conditions, the .remove() function should mark the device as removed, wait for all ongoing access from userspace to be complete, and then proceed to unmapping registers and doing other cleanups. Userspace may still have open file descriptors to the device at that point. Any new userspace access should be disallowed (by checking the removed flag), with the only userspace-initiated operations that still need to run being the release-related operations (unmapping memory, closing file descriptors, ...). -- Regards, Laurent Pinchart
Re: [BUG] drm: zynqmp_dp: Lockup in zynqmp_dp_bridge_detect when device is unbound
On Mon, May 06, 2024 at 09:29:36AM +0200, Maxime Ripard wrote: > Hi Laurent, Sean, > > On Sat, May 04, 2024 at 03:21:18PM GMT, Laurent Pinchart wrote: > > On Fri, May 03, 2024 at 05:54:32PM -0400, Sean Anderson wrote: > > > I have discovered a bug in the displayport driver on drm-misc-next. To > > > trigger it, run > > > > > > echo fd4a.display > /sys/bus/platform/drivers/zynqmp-dpsub/unbind > > > > > > The system will become unresponsive and (after a bit) splat with a hard > > > LOCKUP. One core will be unresponsive at the first zynqmp_dp_read in > > > zynqmp_dp_bridge_detect. > > > > > > I believe the issue is due the registers being unmapped and the block > > > put into reset in zynqmp_dp_remove instead of zynqmp_dpsub_release. > > > > That is on purpose. Drivers are not allowed to access the device at all > > after .remove() returns. > > It's not "on purpose" no. Drivers indeed are not allowed to access the > device after remove, but the kernel shouldn't crash. This is exactly > why we have drm_dev_enter / drm_dev_exit. I didn't mean the crash was on purpose :-) It's the registers being unmapped that is, as nothing should touch those registers after .remove() returns. -- Regards, Laurent Pinchart
Re: [BUG] drm: zynqmp_dp: Lockup in zynqmp_dp_bridge_detect when device is unbound
Hi Sean,
On Fri, May 03, 2024 at 05:54:32PM -0400, Sean Anderson wrote:
> Hi,
>
> I have discovered a bug in the displayport driver on drm-misc-next. To
> trigger it, run
>
> echo fd4a.display > /sys/bus/platform/drivers/zynqmp-dpsub/unbind
>
> The system will become unresponsive and (after a bit) splat with a hard
> LOCKUP. One core will be unresponsive at the first zynqmp_dp_read in
> zynqmp_dp_bridge_detect.
>
> I believe the issue is due the registers being unmapped and the block
> put into reset in zynqmp_dp_remove instead of zynqmp_dpsub_release.
That is on purpose. Drivers are not allowed to access the device at all
after .remove() returns.
> This
> could be resolved by deferring things until zynqmp_dpsub_release
> (requiring us to skip devm_*), or by adding a flag to struct zynqmp_dp
> and checking it before each callback. A subsystem-level implementation
> might be better for the latter.
>
> For a better traceback, try applying the below patch and running the
> following commands before triggering the lockup:
>
> echo 4 > /sys/module/drm/parameters/debug
> echo 8 > /proc/sys/kernel/printk
>
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c
> b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> index 9df068a413f3..17b477b14ab5 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> @@ -296,6 +296,7 @@ struct zynqmp_dp_config {
> * @train_set: set of training data
> */
> struct zynqmp_dp {
> + unsigned long long magic;
> struct device *dev;
> struct zynqmp_dpsub *dpsub;
> void __iomem *iomem;
> @@ -1533,6 +1534,8 @@ static enum drm_connector_status
> zynqmp_dp_bridge_detect(struct drm_bridge *brid
> u32 state, i;
> int ret;
>
> + WARN_ON(dp->magic != 0x0123456789abcdefULL);
> +
> /*
> * This is from heuristic. It takes some delay (ex, 100 ~ 500 msec) to
> * get the HPD signal with some monitors.
> @@ -1723,6 +1726,7 @@ int zynqmp_dp_probe(struct zynqmp_dpsub *dpsub)
> if (!dp)
> return -ENOMEM;
>
> + dp->magic = 0x0123456789abcdefULL;
> dp->dev = >dev;
> dp->dpsub = dpsub;
> dp->status = connector_status_disconnected;
> @@ -1839,4 +1843,5 @@ void zynqmp_dp_remove(struct zynqmp_dpsub *dpsub)
>
> zynqmp_dp_phy_exit(dp);
> zynqmp_dp_reset(dp, true);
> + dp->magic = 0xdeadbeefdeadbeefULL;
> }
--
Regards,
Laurent Pinchart
Re: [PATCH] drm/fourcc: Add RGB161616 and BGR161616 formats
Hi Jacopo,
On Thu, May 02, 2024 at 11:02:27AM +0200, Jacopo Mondi wrote:
> Hello
>which tree should this patch be collected from now that it has
> been reviewed ?
I think this can go through drm-misc. I'm not sure what the rule is for
patches that touch core code like these, can then be pushed by anyone
with commit access, or do they need to be collected by a drm-misc
maintainer ?
> On Mon, Feb 26, 2024 at 02:25:43PM GMT, Jacopo Mondi wrote:
> > Add FourCC definitions for the 48-bit RGB/BGR formats to the
> > DRM/KMS uapi.
> >
> > The format will be used by the Raspberry Pi PiSP Back End,
> > supported by a V4L2 driver in kernel space and by libcamera in
> > userspace, which uses the DRM FourCC identifiers.
> >
> > Signed-off-by: Jacopo Mondi
> > ---
> > drivers/gpu/drm/drm_fourcc.c | 8
> > include/uapi/drm/drm_fourcc.h | 4
> > 2 files changed, 12 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
> > index 193cf8ed7912..908f20b96fd5 100644
> > --- a/drivers/gpu/drm/drm_fourcc.c
> > +++ b/drivers/gpu/drm/drm_fourcc.c
> > @@ -210,6 +210,14 @@ const struct drm_format_info *__drm_format_info(u32
> > format)
> > { .format = DRM_FORMAT_ABGR2101010, .depth = 30,
> > .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1, .has_alpha =
> > true },
> > { .format = DRM_FORMAT_RGBA1010102, .depth = 30,
> > .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1, .has_alpha =
> > true },
> > { .format = DRM_FORMAT_BGRA1010102, .depth = 30,
> > .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1, .has_alpha =
> > true },
> > + { .format = DRM_FORMAT_RGB161616, .depth = 0,
> > + .num_planes = 1, .char_per_block = { 6, 0, 0 },
> > + .block_w = { 1, 0, 0 }, .block_h = { 1, 0, 0 },
> > + .hsub = 1, .vsub = 1, .has_alpha = false },
> > + { .format = DRM_FORMAT_BGR161616, .depth = 0,
> > + .num_planes = 1, .char_per_block = { 6, 0, 0 },
> > + .block_w = { 1, 0, 0 }, .block_h = { 1, 0, 0 },
> > + .hsub = 1, .vsub = 1, .has_alpha = false },
> > { .format = DRM_FORMAT_ARGB,.depth = 32,
> > .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1, .has_alpha =
> > true },
> > { .format = DRM_FORMAT_ABGR,.depth = 32,
> > .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1, .has_alpha =
> > true },
> > { .format = DRM_FORMAT_RGBA,.depth = 32,
> > .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1, .has_alpha =
> > true },
> > diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> > index 84d502e42961..00db00083175 100644
> > --- a/include/uapi/drm/drm_fourcc.h
> > +++ b/include/uapi/drm/drm_fourcc.h
> > @@ -210,6 +210,10 @@ extern "C" {
> > #define DRM_FORMAT_RGBA1010102 fourcc_code('R', 'A', '3', '0') /*
> > [31:0] R:G:B:A 10:10:10:2 little endian */
> > #define DRM_FORMAT_BGRA1010102 fourcc_code('B', 'A', '3', '0') /*
> > [31:0] B:G:R:A 10:10:10:2 little endian */
> >
> > +/* 48 bpp RGB */
> > +#define DRM_FORMAT_RGB161616 fourcc_code('R', 'G', '4', '8') /* [47:0]
> > R:G:B 16:16:16 little endian */
> > +#define DRM_FORMAT_BGR161616 fourcc_code('B', 'G', '4', '8') /* [47:0]
> > B:G:R 16:16:16 little endian */
> > +
> > /* 64 bpp RGB */
> > #define DRM_FORMAT_XRGB16161616fourcc_code('X', 'R', '4', '8') /*
> > [63:0] x:R:G:B 16:16:16:16 little endian */
> > #define DRM_FORMAT_XBGR16161616fourcc_code('X', 'B', '4', '8') /*
> > [63:0] x:B:G:R 16:16:16:16 little endian */
--
Regards,
Laurent Pinchart
Re: [PATCH] drm: zynqmp_dpsub: Always register bridge
t; I have the drm_bridge_add() call in zynqmp_dp_probe(), as that's where > the bridge is set up, so it felt like a logical place. You add it later, > just before the bridge is used the first time. > > I like mine a bit more as it has all the bridge code in the same place, > but I also wonder if there might be some risks in adding the bridge > early (before zynqmp_disp_probe()), although I can't see any issue right > away... Seems we have the same concerns :-) I've reviewed your patch and wrote pretty much the same. I would be more comfortable with this version, even if I like gathering all bridge code in the same location. > In any case, as this works for me too: > > Reviewed-by: Tomi Valkeinen Reviewed-by: Laurent Pinchart -- Regards, Laurent Pinchart
Re: [PATCH] drm: xlnx: zynqmp_dpsub: Fix missing drm_bridge_add() call
Hi Tomi,
Thank you for the patch.
On Tue, Mar 12, 2024 at 10:51:15AM +0200, Tomi Valkeinen wrote:
> The driver creates a bridge, but never calls drm_bridge_add() when
> non-live input is used. This leaves the bridge's hpd_mutex
> uninitialized, leading to:
>
> WARNING: CPU: 0 PID: 9 at kernel/locking/mutex.c:582 __mutex_lock+0x708/0x840
>
> Add the bridge add & remove calls so that the bridge gets managed
> correctly.
>
> Signed-off-by: Tomi Valkeinen
> Fixes: 561671612394 ("drm: xlnx: zynqmp_dpsub: Add support for live video
> input")
> ---
> drivers/gpu/drm/xlnx/zynqmp_dp.c| 4
> drivers/gpu/drm/xlnx/zynqmp_dpsub.c | 4
> 2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c
> b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> index a0606fab0e22..9f750740dfb8 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> @@ -1761,6 +1761,8 @@ int zynqmp_dp_probe(struct zynqmp_dpsub *dpsub)
>
> dpsub->dp = dp;
>
> + drm_bridge_add(dpsub->bridge);
> +
This means that the bridge will be exposed to users before
zynqmp_disp_probe() is called, opening the door to a potential
use-before-init. The risk is mostly theoretical at this point I believe,
but it's still not a direction I'd like to that. Could you call
drm_bridge_add() in zynqmp_dpsub_probe(), between zynqmp_disp_probe()
and zynqmp_dpsub_drm_init() ?
> dev_dbg(dp->dev, "ZynqMP DisplayPort Tx probed with %u lanes\n",
> dp->num_lanes);
>
> @@ -1789,4 +1791,6 @@ void zynqmp_dp_remove(struct zynqmp_dpsub *dpsub)
>
> zynqmp_dp_phy_exit(dp);
> zynqmp_dp_reset(dp, true);
> +
> + drm_bridge_remove(dpsub->bridge);
> }
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
> b/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
> index 88eb33acd5f0..3933c4f1a44f 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
> @@ -260,8 +260,6 @@ static int zynqmp_dpsub_probe(struct platform_device
> *pdev)
> ret = zynqmp_dpsub_drm_init(dpsub);
> if (ret)
> goto err_disp;
> - } else {
> - drm_bridge_add(dpsub->bridge);
> }
>
> dev_info(>dev, "ZynqMP DisplayPort Subsystem driver probed");
> @@ -288,8 +286,6 @@ static void zynqmp_dpsub_remove(struct platform_device
> *pdev)
>
> if (dpsub->drm)
> zynqmp_dpsub_drm_cleanup(dpsub);
> - else
> - drm_bridge_remove(dpsub->bridge);
>
> zynqmp_disp_remove(dpsub);
> zynqmp_dp_remove(dpsub);
>
> ---
> base-commit: e8f897f4afef0031fe618a8e94127a0934896aba
> change-id: 20240312-xilinx-dp-lock-fix-cf68f43a7bab
--
Regards,
Laurent Pinchart
Re: [PATCH V2] drm/bridge: imx: Fix unmet depenency for PHY_FSL_SAMSUNG_HDMI_PHY
Hi Adam,
Thank you for the patch.
On Mon, Apr 22, 2024 at 05:33:52AM -0500, Adam Ford wrote:
> When enabling i.MX8MP DWC HDMI driver, it automatically selects
> PHY_FSL_SAMSUNG_HDMI_PHY, since it wont' work without the phy.
> This may cause some Kconfig warnings during various build tests.
> Fix this by implying the phy instead of selecting the phy.
>
> To prevent this from happening with the DRM_IMX8MP_HDMI_PVI, also
> imply it instead of selecting it.
>
> Fixes: 1f36d634670d ("drm/bridge: imx: add bridge wrapper driver for i.MX8MP
> DWC HDMI")
> Reported-by: kernel test robot
> Closes:
> https://lore.kernel.org/oe-kbuild-all/202404190103.llm8ltup-...@intel.com/
> Signed-off-by: Adam Ford
This looks sensible to me.
Reviewed-by: Laurent Pinchart
> ---
> V2: Change DRM_IMX8MP_HDMI_PVI at the same time since it was affected
> from the same commit.
>
> diff --git a/drivers/gpu/drm/bridge/imx/Kconfig
> b/drivers/gpu/drm/bridge/imx/Kconfig
> index 7687ed652df5..13142a6b8590 100644
> --- a/drivers/gpu/drm/bridge/imx/Kconfig
> +++ b/drivers/gpu/drm/bridge/imx/Kconfig
> @@ -8,8 +8,8 @@ config DRM_IMX8MP_DW_HDMI_BRIDGE
> depends on COMMON_CLK
> depends on DRM_DW_HDMI
> depends on OF
> - select DRM_IMX8MP_HDMI_PVI
> - select PHY_FSL_SAMSUNG_HDMI_PHY
> + imply DRM_IMX8MP_HDMI_PVI
> + imply PHY_FSL_SAMSUNG_HDMI_PHY
> help
> Choose this to enable support for the internal HDMI encoder found
> on the i.MX8MP SoC.
--
Regards,
Laurent Pinchart
Re: [PATCH] drm/bridge: imx: Fix unmet depenency for PHY_FSL_SAMSUNG_HDMI_PHY
Hi Adam,
Thank you for the patch.
On Sat, Apr 20, 2024 at 06:28:31AM -0500, Adam Ford wrote:
> When enabling i.MX8MP DWC HDMI driver, it automatically selects
> PHY_FSL_SAMSUNG_HDMI_PHY, since it wont' work without the phy.
> This may cause some Kconfig warnings during various build tests.
> Fix this by implying the phy instead of selecting the phy.
>
> Fixes: 1f36d634670d ("drm/bridge: imx: add bridge wrapper driver for i.MX8MP
> DWC HDMI")
> Reported-by: kernel test robot
> Closes:
> https://lore.kernel.org/oe-kbuild-all/202404190103.llm8ltup-...@intel.com/
> Signed-off-by: Adam Ford
>
> diff --git a/drivers/gpu/drm/bridge/imx/Kconfig
> b/drivers/gpu/drm/bridge/imx/Kconfig
> index 7687ed652df5..8f125c75050d 100644
> --- a/drivers/gpu/drm/bridge/imx/Kconfig
> +++ b/drivers/gpu/drm/bridge/imx/Kconfig
> @@ -9,7 +9,7 @@ config DRM_IMX8MP_DW_HDMI_BRIDGE
> depends on DRM_DW_HDMI
> depends on OF
> select DRM_IMX8MP_HDMI_PVI
This also looks wrong, even if in practice it will likely work because
DRM_IMX8MP_HDMI_PVI has no extra dependency compared to
DRM_IMX8MP_DW_HDMI_BRIDGE. Still, it would be good to fix it.
> - select PHY_FSL_SAMSUNG_HDMI_PHY
> + imply PHY_FSL_SAMSUNG_HDMI_PHY
> help
> Choose this to enable support for the internal HDMI encoder found
> on the i.MX8MP SoC.
--
Regards,
Laurent Pinchart
Re: [PATCH] i2c: mux: Remove class argument from i2c_mux_add_adapter()
Hi Heiner,
Thank you for the patch.
On Thu, Apr 18, 2024 at 10:55:39PM +0200, Heiner Kallweit wrote:
> 99a741aa7a2d ("i2c: mux: gpio: remove support for class-based device
> instantiation") removed the last call to i2c_mux_add_adapter() with a
> non-null class argument. Therefore the class argument can be removed.
>
> Note: Class-based device instantiation is a legacy mechanism which
> shouldn't be used in new code, so we can rule out that this argument
> may be needed again in the future.
>
> Signed-off-by: Heiner Kallweit
Reviewed-by: Laurent Pinchart
> ---
> drivers/gpu/drm/bridge/sii902x.c | 2 +-
> drivers/i2c/i2c-mux.c | 24 +-
> drivers/i2c/muxes/i2c-arb-gpio-challenge.c | 2 +-
> drivers/i2c/muxes/i2c-mux-gpio.c | 2 +-
> drivers/i2c/muxes/i2c-mux-gpmux.c | 2 +-
> drivers/i2c/muxes/i2c-mux-ltc4306.c| 2 +-
> drivers/i2c/muxes/i2c-mux-mlxcpld.c| 2 +-
> drivers/i2c/muxes/i2c-mux-pca9541.c| 2 +-
> drivers/i2c/muxes/i2c-mux-pca954x.c| 2 +-
> drivers/i2c/muxes/i2c-mux-pinctrl.c| 2 +-
> drivers/i2c/muxes/i2c-mux-reg.c| 2 +-
> drivers/iio/gyro/mpu3050-i2c.c | 2 +-
> drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c | 2 +-
> drivers/media/dvb-frontends/af9013.c | 2 +-
> drivers/media/dvb-frontends/lgdt3306a.c| 2 +-
> drivers/media/dvb-frontends/m88ds3103.c| 2 +-
> drivers/media/dvb-frontends/rtl2830.c | 2 +-
> drivers/media/dvb-frontends/rtl2832.c | 2 +-
> drivers/media/dvb-frontends/si2168.c | 2 +-
> drivers/media/i2c/max9286.c| 2 +-
> drivers/media/usb/cx231xx/cx231xx-i2c.c| 5 +
> drivers/of/unittest.c | 2 +-
> include/linux/i2c-mux.h| 3 +--
> 23 files changed, 23 insertions(+), 49 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/sii902x.c
> b/drivers/gpu/drm/bridge/sii902x.c
> index 8f84e9824..2fbeda902 100644
> --- a/drivers/gpu/drm/bridge/sii902x.c
> +++ b/drivers/gpu/drm/bridge/sii902x.c
> @@ -1092,7 +1092,7 @@ static int sii902x_init(struct sii902x *sii902x)
> }
>
> sii902x->i2cmux->priv = sii902x;
> - ret = i2c_mux_add_adapter(sii902x->i2cmux, 0, 0, 0);
> + ret = i2c_mux_add_adapter(sii902x->i2cmux, 0, 0);
> if (ret)
> goto err_unreg_audio;
>
> diff --git a/drivers/i2c/i2c-mux.c b/drivers/i2c/i2c-mux.c
> index 57ff09f18..fda72e8be 100644
> --- a/drivers/i2c/i2c-mux.c
> +++ b/drivers/i2c/i2c-mux.c
> @@ -127,19 +127,6 @@ static u32 i2c_mux_functionality(struct i2c_adapter
> *adap)
> return parent->algo->functionality(parent);
> }
>
> -/* Return all parent classes, merged */
> -static unsigned int i2c_mux_parent_classes(struct i2c_adapter *parent)
> -{
> - unsigned int class = 0;
> -
> - do {
> - class |= parent->class;
> - parent = i2c_parent_is_i2c_adapter(parent);
> - } while (parent);
> -
> - return class;
> -}
> -
> static void i2c_mux_lock_bus(struct i2c_adapter *adapter, unsigned int flags)
> {
> struct i2c_mux_priv *priv = adapter->algo_data;
> @@ -281,8 +268,7 @@ static const struct i2c_lock_operations
> i2c_parent_lock_ops = {
> };
>
> int i2c_mux_add_adapter(struct i2c_mux_core *muxc,
> - u32 force_nr, u32 chan_id,
> - unsigned int class)
> + u32 force_nr, u32 chan_id)
> {
> struct i2c_adapter *parent = muxc->parent;
> struct i2c_mux_priv *priv;
> @@ -340,14 +326,6 @@ int i2c_mux_add_adapter(struct i2c_mux_core *muxc,
> else
> priv->adap.lock_ops = _parent_lock_ops;
>
> - /* Sanity check on class */
> - if (i2c_mux_parent_classes(parent) & class & ~I2C_CLASS_DEPRECATED)
> - dev_err(>dev,
> - "Segment %d behind mux can't share classes with
> ancestors\n",
> - chan_id);
> - else
> - priv->adap.class = class;
> -
> /*
>* Try to populate the mux adapter's of_node, expands to
>* nothing if !CONFIG_OF.
> diff --git a/drivers/i2c/muxes/i2c-arb-gpio-challenge.c
> b/drivers/i2c/muxes/i2c-arb-gpio-challenge.c
> index 24168e9f7..7aa6e795d 100644
> --- a/drivers/i2c/muxes/i2c-arb-gpio-challenge.c
> +++ b/drivers/i2c/muxes/i2c-arb-gpio-challenge.c
> @@ -167,7 +167,7 @@ static int i2c_arbitrator_probe(struct platform_device
> *pdev)
> }
>
> /* Actually add the mux adapter */
> - ret = i2c_mux_add_adapter(muxc, 0, 0, 0);
&g
Re: [PATCH v2 28/43] drm/renesas/rcar-du: Use fbdev-dma
On Fri, Apr 12, 2024 at 09:57:27PM +0300, Laurent Pinchart wrote: > Hi Thomas, > > Thank you for the patch. > > On Wed, Apr 10, 2024 at 03:02:24PM +0200, Thomas Zimmermann wrote: > > Implement fbdev emulation with fbdev-dma. Fbdev-dma now supports > > damage handling, which is required by rcar-du. Avoids the overhead of > > fbdev-generic's additional shadow buffering. No functional changes. > > > > Signed-off-by: Thomas Zimmermann > > Cc: Laurent Pinchart > > Cc: Kieran Bingham > > Reviewed-by: Laurent Pinchart I meant Reviewed-by: Laurent Pinchart > On a side note, I noticed that drm_fbdev_generic_client_funcs and > drm_fbdev_dma_client_funcs point to functions that are identical. Would > there be a way to avoid the code duplication ? > > > --- > > drivers/gpu/drm/renesas/rcar-du/rcar_du_drv.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/renesas/rcar-du/rcar_du_drv.c > > b/drivers/gpu/drm/renesas/rcar-du/rcar_du_drv.c > > index dee530e4c8b27..fb719d9aff10d 100644 > > --- a/drivers/gpu/drm/renesas/rcar-du/rcar_du_drv.c > > +++ b/drivers/gpu/drm/renesas/rcar-du/rcar_du_drv.c > > @@ -20,7 +20,7 @@ > > > > #include > > #include > > -#include > > +#include > > #include > > #include > > #include > > @@ -716,7 +716,7 @@ static int rcar_du_probe(struct platform_device *pdev) > > > > drm_info(>ddev, "Device %s probed\n", dev_name(>dev)); > > > > - drm_fbdev_generic_setup(>ddev, 32); > > + drm_fbdev_dma_setup(>ddev, 32); > > > > return 0; > > -- Regards, Laurent Pinchart
Re: [PATCH v2 30/43] drm/renesas/shmobile: Use fbdev-dma
Hi Thomas, Thank you for the patch. On Wed, Apr 10, 2024 at 03:02:26PM +0200, Thomas Zimmermann wrote: > Implement fbdev emulation with fbdev-dma. Fbdev-dma now supports > damage handling, which is required by shmobile. Avoids the overhead of > fbdev-generic's additional shadow buffering. No functional changes. > > Signed-off-by: Thomas Zimmermann > Cc: Laurent Pinchart > Cc: Geert Uytterhoeven Reviewed-by: Laurent Pinchart > --- > drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c > b/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c > index e83c3e52251de..890cc2f6408d6 100644 > --- a/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c > +++ b/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c > @@ -19,7 +19,7 @@ > > #include > #include > -#include > +#include > #include > #include > #include > @@ -250,7 +250,7 @@ static int shmob_drm_probe(struct platform_device *pdev) > if (ret < 0) > goto err_modeset_cleanup; > > - drm_fbdev_generic_setup(ddev, 16); > + drm_fbdev_dma_setup(ddev, 16); > > return 0; > -- Regards, Laurent Pinchart
Re: [PATCH v2 28/43] drm/renesas/rcar-du: Use fbdev-dma
Hi Thomas, Thank you for the patch. On Wed, Apr 10, 2024 at 03:02:24PM +0200, Thomas Zimmermann wrote: > Implement fbdev emulation with fbdev-dma. Fbdev-dma now supports > damage handling, which is required by rcar-du. Avoids the overhead of > fbdev-generic's additional shadow buffering. No functional changes. > > Signed-off-by: Thomas Zimmermann > Cc: Laurent Pinchart > Cc: Kieran Bingham Reviewed-by: Laurent Pinchart On a side note, I noticed that drm_fbdev_generic_client_funcs and drm_fbdev_dma_client_funcs point to functions that are identical. Would there be a way to avoid the code duplication ? > --- > drivers/gpu/drm/renesas/rcar-du/rcar_du_drv.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/renesas/rcar-du/rcar_du_drv.c > b/drivers/gpu/drm/renesas/rcar-du/rcar_du_drv.c > index dee530e4c8b27..fb719d9aff10d 100644 > --- a/drivers/gpu/drm/renesas/rcar-du/rcar_du_drv.c > +++ b/drivers/gpu/drm/renesas/rcar-du/rcar_du_drv.c > @@ -20,7 +20,7 @@ > > #include > #include > -#include > +#include > #include > #include > #include > @@ -716,7 +716,7 @@ static int rcar_du_probe(struct platform_device *pdev) > > drm_info(>ddev, "Device %s probed\n", dev_name(>dev)); > > - drm_fbdev_generic_setup(>ddev, 32); > + drm_fbdev_dma_setup(>ddev, 32); > > return 0; > -- Regards, Laurent Pinchart
Re: [PATCH 20/21] drm/rcar-du: Allow build with COMPILE_TEST=y
Hi Ville, Thank you for the patch. On Mon, Apr 08, 2024 at 08:04:25PM +0300, Ville Syrjala wrote: > From: Ville Syrjälä > > Allow rcar-du to be built with COMPILE_TEST=y for greater > coverage. Builds fine on x86/x86_64 at least. > > Cc: Laurent Pinchart > Cc: Kieran Bingham > Cc: linux-renesas-...@vger.kernel.org > Signed-off-by: Ville Syrjälä > --- > drivers/gpu/drm/renesas/rcar-du/Kconfig | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/renesas/rcar-du/Kconfig > b/drivers/gpu/drm/renesas/rcar-du/Kconfig > index 2dc739db2ba3..df8b08b1e537 100644 > --- a/drivers/gpu/drm/renesas/rcar-du/Kconfig > +++ b/drivers/gpu/drm/renesas/rcar-du/Kconfig > @@ -2,7 +2,7 @@ > config DRM_RCAR_DU > tristate "DRM Support for R-Car Display Unit" > depends on DRM && OF > - depends on ARM || ARM64 > + depends on ARM || ARM64 || COMPILE_TEST I'll trust 0-day to tell us if this causes any issue on exotic (from the R-Car DU driver's point of view) platforms. Reviewed-by: Laurent Pinchart I expect you to push this patch, please let me know if you don't intend to. > depends on ARCH_RENESAS || COMPILE_TEST > select DRM_KMS_HELPER > select DRM_GEM_DMA_HELPER -- Regards, Laurent Pinchart
Re: 2024 X.Org Foundation Membership deadline for voting in the election
Hi Pekka, On Tue, Apr 02, 2024 at 10:46:08AM +0300, Pekka Paalanen wrote: > On Tue, 26 Mar 2024 11:42:48 -0400 Christopher Michael wrote: > > > The 2024 X.Org Foundation membership renewal period has been extended > > one additional week and elections will start the following week on 01 > > April 2024. > > > > Please note that only current members can vote in the upcoming election, > > and that the deadline for new memberships or renewals to vote in the > > upcoming election is 01 April 2024 at 23:59 UTC. > > > > If you are interested in joining the X.Org Foundation or in renewing > > your membership, please visit the membership system site at: > > https://members.x.org/ > > > > Christopher Michael, on behalf of the X.Org elections committee > > Hi everyone, > > given that the year's first email reminding everyone to renew their > memberships was sent on Feb 7 when the renewal was NOT open yet, I > wonder how many people thought they had already renewed and are now > thinking they don't need to do anything? > > I fell for that: On Feb 7, I went to members.x.org to check my status, > it said I was registered for "2023-2024" and there was no button to > renew, so I closed the page confident that I was a member for 2024. > After all, it said 2024. This was a mistake I realised only after being > personally poked to renew. I know for sure of one other person falling > for the same. Make that two. Thanks for the notice. > Now, the members page for this year says "Application for the period: > 02/2024-02/2025". Thanks to the people adding the month to reduce > confusion. -- Regards, Laurent Pinchart
Re: [PATCH] MAINTAINERS: Add myself as maintainer for Xilinx DRM drivers
Hi Tomi, Thank you for the patch. On Wed, Mar 27, 2024 at 03:03:33PM +0200, Tomi Valkeinen wrote: > Add myself as a co-maintainer for Xilinx DRM drivers to help Laurent. > > Signed-off-by: Tomi Valkeinen Much appreciated :-) Reviewed-by: Laurent Pinchart > --- > MAINTAINERS | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/MAINTAINERS b/MAINTAINERS > index 1aabf1c15bb3..79ef5a6bf21b 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -7306,6 +7306,7 @@ F: drivers/gpu/drm/xen/ > > DRM DRIVERS FOR XILINX > M: Laurent Pinchart > +M: Tomi Valkeinen > L: dri-devel@lists.freedesktop.org > S: Maintained > T: git git://anongit.freedesktop.org/drm/drm-misc > > --- > base-commit: e8f897f4afef0031fe618a8e94127a0934896aba > change-id: 20240327-xilinx-maintainer-f6020f6cba4d -- Regards, Laurent Pinchart
Re: [PATCH 1/9] fbdev: shmobile: fix snprintf truncation
Hi Arnd,
Thank you for the patch.
On Tue, Mar 26, 2024 at 11:38:00PM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann
>
> The name of the overlay does not fit into the fixed-length field:
>
> drivers/video/fbdev/sh_mobile_lcdcfb.c:1577:2: error: 'snprintf' will always
> be truncated; specified size is 16, but format string expands to at least 25
>
> Make it short enough by changing the string.
>
> Fixes: c5deac3c9b22 ("fbdev: sh_mobile_lcdc: Implement overlays support")
> Signed-off-by: Arnd Bergmann
Reviewed-by: Laurent Pinchart
> ---
> drivers/video/fbdev/sh_mobile_lcdcfb.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/video/fbdev/sh_mobile_lcdcfb.c
> b/drivers/video/fbdev/sh_mobile_lcdcfb.c
> index eb2297b37504..d35d2cf8 100644
> --- a/drivers/video/fbdev/sh_mobile_lcdcfb.c
> +++ b/drivers/video/fbdev/sh_mobile_lcdcfb.c
> @@ -1575,7 +1575,7 @@ sh_mobile_lcdc_overlay_fb_init(struct
> sh_mobile_lcdc_overlay *ovl)
>*/
> info->fix = sh_mobile_lcdc_overlay_fix;
> snprintf(info->fix.id, sizeof(info->fix.id),
> - "SH Mobile LCDC Overlay %u", ovl->index);
> + "SHMobile ovl %u", ovl->index);
> info->fix.smem_start = ovl->dma_handle;
> info->fix.smem_len = ovl->fb_size;
> info->fix.line_length = ovl->pitch;
--
Regards,
Laurent Pinchart
Re: [PATCH V8 00/12] soc: imx8mp: Add support for HDMI
Hi Tommaso, On Mon, Mar 25, 2024 at 10:48:56PM +0100, Tommaso Merciai wrote: > Hi Adam, Lucas, > Thanks for this series. > > This series make HDMI work on evk. > All is working properly on my side. > > Tested on: Linux imx8mp-lpddr4-evk 6.9.0-rc1. > Hope this help. > > Tested-by: Tommaso Merciai The DRM side has been merged already. The only missing patches are for the PHY, and the latest version can be found in https://lore.kernel.org/linux-phy/20240227220444.77566-1-aford...@gmail.com/. You can test that series and send a Tested-by tag. I'm crossing my fingers and hoping it will be merged in v6.10. > On Sat, Feb 03, 2024 at 10:52:40AM -0600, Adam Ford wrote: > > The i.MX8M Plus has an HDMI controller, but it depends on two > > other systems, the Parallel Video Interface (PVI) and the > > HDMI PHY from Samsung. The LCDIF controller generates the display > > and routes it to the PVI which converts passes the parallel video > > to the HDMI bridge. The HDMI system has a corresponding power > > domain controller whose driver was partially written, but the > > device tree for it was never applied, so some changes to the > > power domain should be harmless because they've not really been > > used yet. > > > > This series is adapted from multiple series from Lucas Stach with > > edits and suggestions from feedback from various series, but it > > since it's difficult to use and test them independently, > > I merged them into on unified series. The version history is a > > bit ambiguous since different components were submitted at different > > times and had different amount of retries. In an effort to merge them > > I used the highest version attempt. > > > > Adam Ford (3): > > dt-bindings: soc: imx: add missing clock and power-domains to > > imx8mp-hdmi-blk-ctrl > > pmdomain: imx8mp-blk-ctrl: imx8mp_blk: Add fdcc clock to hdmimix > > domain > > arm64: defconfig: Enable DRM_IMX8MP_DW_HDMI_BRIDGE as module > > > > Lucas Stach (9): > > dt-bindings: phy: add binding for the i.MX8MP HDMI PHY > > phy: freescale: add Samsung HDMI PHY > > arm64: dts: imx8mp: add HDMI power-domains > > arm64: dts: imx8mp: add HDMI irqsteer > > dt-bindings: display: imx: add binding for i.MX8MP HDMI PVI > > drm/bridge: imx: add driver for HDMI TX Parallel Video Interface > > dt-bindings: display: imx: add binding for i.MX8MP HDMI TX > > drm/bridge: imx: add bridge wrapper driver for i.MX8MP DWC HDMI > > arm64: dts: imx8mp: add HDMI display pipeline > > > > .../display/bridge/fsl,imx8mp-hdmi-tx.yaml| 102 ++ > > .../display/imx/fsl,imx8mp-hdmi-pvi.yaml | 84 ++ > > .../bindings/phy/fsl,imx8mp-hdmi-phy.yaml | 62 + > > .../soc/imx/fsl,imx8mp-hdmi-blk-ctrl.yaml | 22 +- > > arch/arm64/boot/dts/freescale/imx8mp.dtsi | 145 +++ > > arch/arm64/configs/defconfig |1 + > > drivers/gpu/drm/bridge/imx/Kconfig| 18 + > > drivers/gpu/drm/bridge/imx/Makefile |2 + > > drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pvi.c | 207 > > drivers/gpu/drm/bridge/imx/imx8mp-hdmi-tx.c | 154 +++ > > drivers/phy/freescale/Kconfig |6 + > > drivers/phy/freescale/Makefile|1 + > > drivers/phy/freescale/phy-fsl-samsung-hdmi.c | 1075 + > > drivers/pmdomain/imx/imx8mp-blk-ctrl.c| 10 +- > > 14 files changed, 1876 insertions(+), 13 deletions(-) > > create mode 100644 > > Documentation/devicetree/bindings/display/bridge/fsl,imx8mp-hdmi-tx.yaml > > create mode 100644 > > Documentation/devicetree/bindings/display/imx/fsl,imx8mp-hdmi-pvi.yaml > > create mode 100644 > > Documentation/devicetree/bindings/phy/fsl,imx8mp-hdmi-phy.yaml > > create mode 100644 drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pvi.c > > create mode 100644 drivers/gpu/drm/bridge/imx/imx8mp-hdmi-tx.c > > create mode 100644 drivers/phy/freescale/phy-fsl-samsung-hdmi.c -- Regards, Laurent Pinchart
Re: [PATCH v2 5/8] drm: xlnx: zynqmp_dpsub: Set input live format
DRM_FORMAT_RGB888,
> + }, {
> + .bus_fmt = MEDIA_BUS_FMT_UYVY8_1X16,
> + .drm_fmt = DRM_FORMAT_YUV422,
> + }, {
> + .bus_fmt = MEDIA_BUS_FMT_VUY8_1X24,
> + .drm_fmt = DRM_FORMAT_YUV444,
> + }, {
> + .bus_fmt = MEDIA_BUS_FMT_UYVY10_1X20,
> + .drm_fmt = DRM_FORMAT_P210,
> + },
H... Looking at this, I think you should have both bus_fmt and
drm_fmt in zynqmp_disp_format. It seems it would simplify the code flow
and make things more readable. If you would like me to give it a try,
please let me know.
> + };
> + unsigned int i;
> +
> + for (i = 0; i < ARRAY_SIZE(format_map); ++i)
> + if (format_map[i].bus_fmt == bus_format)
> + return format_map[i].drm_fmt;
> +
> + return DRM_FORMAT_INVALID;
> +}
> +
> /**
> * zynqmp_disp_layer_set_format - Set the layer format
> * @layer: The layer
> - * @info: The format info
> + * @drm_or_bus_format: DRM or media bus format
> *
> * Set the format for @layer to @info. The layer must be disabled.
> */
> void zynqmp_disp_layer_set_format(struct zynqmp_disp_layer *layer,
> - const struct drm_format_info *info)
> + u32 drm_or_bus_format)
> {
> unsigned int i;
>
> - layer->disp_fmt = zynqmp_disp_layer_find_format(layer, info->format);
> - layer->drm_fmt = info;
> -
> + layer->disp_fmt = zynqmp_disp_layer_find_format(layer,
> drm_or_bus_format);
> zynqmp_disp_avbuf_set_format(layer->disp, layer, layer->disp_fmt);
>
> + if (layer->mode == ZYNQMP_DPSUB_LAYER_LIVE)
> + drm_or_bus_format =
> zynqmp_disp_reference_drm_format(drm_or_bus_format);
> +
> + layer->drm_fmt = drm_format_info(drm_or_bus_format);
> + if (!layer->drm_fmt)
> + return;
> +
> if (layer->mode == ZYNQMP_DPSUB_LAYER_LIVE)
> return;
>
> @@ -1008,7 +1067,7 @@ void zynqmp_disp_layer_set_format(struct
> zynqmp_disp_layer *layer,
>* Set pconfig for each DMA channel to indicate they're part of a
>* video group.
>*/
> - for (i = 0; i < info->num_planes; i++) {
> + for (i = 0; i < layer->drm_fmt->num_planes; i++) {
> struct zynqmp_disp_layer_dma *dma = >dmas[i];
> struct xilinx_dpdma_peripheral_config pconfig = {
> .video_group = true,
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.h
> b/drivers/gpu/drm/xlnx/zynqmp_disp.h
> index 88c285a12e23..9f9a5f50ffbc 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_disp.h
> +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.h
> @@ -55,7 +55,7 @@ u32 *zynqmp_disp_layer_formats(struct zynqmp_disp_layer
> *layer,
> void zynqmp_disp_layer_enable(struct zynqmp_disp_layer *layer);
> void zynqmp_disp_layer_disable(struct zynqmp_disp_layer *layer);
> void zynqmp_disp_layer_set_format(struct zynqmp_disp_layer *layer,
> - const struct drm_format_info *info);
> + u32 drm_or_bus_format);
> int zynqmp_disp_layer_update(struct zynqmp_disp_layer *layer,
>struct drm_plane_state *state);
>
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c
> b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> index a0d169ac48c0..fc6b1d783c28 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> @@ -1281,7 +1281,8 @@ static void zynqmp_dp_disp_enable(struct zynqmp_dp *dp,
> {
> enum zynqmp_dpsub_layer_id layer_id;
> struct zynqmp_disp_layer *layer;
> - const struct drm_format_info *info;
> + struct drm_bridge_state *bridge_state;
> + u32 bus_fmt;
>
> if (dp->dpsub->connected_ports & BIT(ZYNQMP_DPSUB_PORT_LIVE_VIDEO))
> layer_id = ZYNQMP_DPSUB_LAYER_VID;
> @@ -1291,10 +1292,14 @@ static void zynqmp_dp_disp_enable(struct zynqmp_dp
> *dp,
> return;
>
> layer = dp->dpsub->layers[layer_id];
> + bridge_state =
> drm_atomic_get_new_bridge_state(old_bridge_state->base.state,
> +
> old_bridge_state->bridge);
> + if (WARN_ON(!bridge_state))
> + return;
> +
> + bus_fmt = bridge_state->input_bus_cfg.format;
> + zynqmp_disp_layer_set_format(layer, bus_fmt);
>
> - /* TODO: Make the format configurable. */
> - info = drm_format_info(DRM_FORMAT_YUV422);
> - zynqmp_disp_layer_set_format(layer, info);
> zynqmp_disp_layer_enable(layer);
>
> if (layer_id == ZYNQMP_DPSUB_LAYER_GFX)
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_kms.c
> b/drivers/gpu/drm/xlnx/zynqmp_kms.c
> index bf9fba01df0e..d96b3f3f2e3a 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_kms.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_kms.c
> @@ -111,7 +111,7 @@ static void zynqmp_dpsub_plane_atomic_update(struct
> drm_plane *plane,
> if (old_state->fb)
> zynqmp_disp_layer_disable(layer);
>
> - zynqmp_disp_layer_set_format(layer, new_state->fb->format);
> + zynqmp_disp_layer_set_format(layer,
> new_state->fb->format->format);
> }
>
> zynqmp_disp_layer_update(layer, new_state);
>
--
Regards,
Laurent Pinchart
Re: [PATCH v2 4/8] drm: xlnx: zynqmp_dpsub: Minimize usage of global flag
Hi Anatoliy,
Thank you for the patch.
On Tue, Mar 12, 2024 at 05:55:01PM -0700, Anatoliy Klymenko wrote:
> Avoid usage of global zynqmp_dpsub.dma_enabled flag in DPSUB layer
> context. This flag signals whether the driver runs in DRM CRTC or DRM
> bridge mode, assuming that all display layers share the same live or
> non-live mode of operation. Using per-layer mode instead of global flag
> will siplify future support of the hybrid scenario.
s/siplify/simplify/
> Signed-off-by: Anatoliy Klymenko
> ---
> drivers/gpu/drm/xlnx/zynqmp_disp.c | 11 ---
> 1 file changed, 4 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c
> b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> index af851190f447..dd48fa60fa9a 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> @@ -975,7 +975,7 @@ void zynqmp_disp_layer_disable(struct zynqmp_disp_layer
> *layer)
> {
> unsigned int i;
>
> - if (layer->disp->dpsub->dma_enabled) {
> + if (layer->mode == ZYNQMP_DPSUB_LAYER_NONLIVE) {
> for (i = 0; i < layer->drm_fmt->num_planes; i++)
> dmaengine_terminate_sync(layer->dmas[i].chan);
> }
> @@ -1001,7 +1001,7 @@ void zynqmp_disp_layer_set_format(struct
> zynqmp_disp_layer *layer,
>
> zynqmp_disp_avbuf_set_format(layer->disp, layer, layer->disp_fmt);
>
> - if (!layer->disp->dpsub->dma_enabled)
> + if (layer->mode == ZYNQMP_DPSUB_LAYER_LIVE)
> return;
>
> /*
> @@ -1039,7 +1039,7 @@ int zynqmp_disp_layer_update(struct zynqmp_disp_layer
> *layer,
> const struct drm_format_info *info = layer->drm_fmt;
> unsigned int i;
>
> - if (!layer->disp->dpsub->dma_enabled)
> + if (layer->mode == ZYNQMP_DPSUB_LAYER_LIVE)
> return 0;
The above changes look nice.
>
> for (i = 0; i < info->num_planes; i++) {
> @@ -1089,7 +1089,7 @@ static void zynqmp_disp_layer_release_dma(struct
> zynqmp_disp *disp,
> {
> unsigned int i;
>
> - if (!layer->info || !disp->dpsub->dma_enabled)
> + if (!layer->info)
This, however, doesn't seem right, as this function is called
unconditionally from the remove path. The change below seems weird too.
If I'm missing something, it should at least be explained in the commit
message.
> return;
>
> for (i = 0; i < layer->info->num_channels; i++) {
> @@ -1132,9 +1132,6 @@ static int zynqmp_disp_layer_request_dma(struct
> zynqmp_disp *disp,
> unsigned int i;
> int ret;
>
> - if (!disp->dpsub->dma_enabled)
> - return 0;
> -
> for (i = 0; i < layer->info->num_channels; i++) {
> struct zynqmp_disp_layer_dma *dma = >dmas[i];
> char dma_channel_name[16];
>
--
Regards,
Laurent Pinchart
Re: [PATCH v2 3/8] drm: xlnx: zynqmp_dpsub: Anounce supported input formats
mats(struct zynqmp_disp_layer *layer,
> -unsigned int *num_formats)
> +u32 *zynqmp_disp_layer_formats(struct zynqmp_disp_layer *layer,
> +unsigned int *num_formats)
> {
> unsigned int i;
> u32 *formats;
> @@ -1131,6 +1173,11 @@ static int zynqmp_disp_create_layers(struct
> zynqmp_disp *disp)
> .num_channels = 1,
> },
> };
> + static const struct zynqmp_disp_layer_info live_layer_info = {
> + .formats = avbuf_live_fmts,
> + .num_formats = ARRAY_SIZE(avbuf_live_fmts),
> + .num_channels = 0,
> + };
>
> unsigned int i;
> int ret;
> @@ -1140,12 +1187,16 @@ static int zynqmp_disp_create_layers(struct
> zynqmp_disp *disp)
>
> layer->id = i;
> layer->disp = disp;
> - layer->info = _info[i];
> /* For now assume dpsub works in either live or non-live mode
> for both layers.
While are it, could you please turn this into
/*
* For now assume dpsub works in either live or non-live mode
for both layers.
with a blank line just above it ?
>* Hybrid mode is not supported yet.
>*/
> - layer->mode = disp->dpsub->dma_enabled ?
> ZYNQMP_DPSUB_LAYER_NONLIVE
> -:
> ZYNQMP_DPSUB_LAYER_LIVE;
> + if (disp->dpsub->dma_enabled) {
> + layer->mode = ZYNQMP_DPSUB_LAYER_NONLIVE;
> + layer->info = _info[i];
> + } else {
> + layer->mode = ZYNQMP_DPSUB_LAYER_LIVE;
> + layer->info = _layer_info;
> + }
>
> ret = zynqmp_disp_layer_request_dma(disp, layer);
> if (ret)
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.h
> b/drivers/gpu/drm/xlnx/zynqmp_disp.h
> index 9b8b202224d9..88c285a12e23 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_disp.h
> +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.h
> @@ -50,8 +50,8 @@ int zynqmp_disp_setup_clock(struct zynqmp_disp *disp,
> void zynqmp_disp_blend_set_global_alpha(struct zynqmp_disp *disp,
> bool enable, u32 alpha);
>
> -u32 *zynqmp_disp_layer_drm_formats(struct zynqmp_disp_layer *layer,
> -unsigned int *num_formats);
> +u32 *zynqmp_disp_layer_formats(struct zynqmp_disp_layer *layer,
> +unsigned int *num_formats);
> void zynqmp_disp_layer_enable(struct zynqmp_disp_layer *layer);
> void zynqmp_disp_layer_disable(struct zynqmp_disp_layer *layer);
> void zynqmp_disp_layer_set_format(struct zynqmp_disp_layer *layer,
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c
> b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> index 04b6bcac3b07..a0d169ac48c0 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> @@ -1568,6 +1568,31 @@ static const struct drm_edid
> *zynqmp_dp_bridge_edid_read(struct drm_bridge *brid
> return drm_edid_read_ddc(connector, >aux.ddc);
> }
>
> +static u32 *
> +zynqmp_dp_bridge_get_input_bus_fmts(struct drm_bridge *bridge,
> + struct drm_bridge_state *bridge_state,
> + struct drm_crtc_state *crtc_state,
> + struct drm_connector_state *conn_state,
> + u32 output_fmt,
> + unsigned int *num_input_fmts)
> +{
> + struct zynqmp_dp *dp = bridge_to_dp(bridge);
> + struct zynqmp_disp_layer *layer;
> + enum zynqmp_dpsub_layer_id layer_id;
> +
> + if (dp->dpsub->connected_ports & BIT(ZYNQMP_DPSUB_PORT_LIVE_VIDEO))
> + layer_id = ZYNQMP_DPSUB_LAYER_VID;
> + else if (dp->dpsub->connected_ports & BIT(ZYNQMP_DPSUB_PORT_LIVE_GFX))
> + layer_id = ZYNQMP_DPSUB_LAYER_GFX;
> + else {
> + *num_input_fmts = 0;
> + return NULL;
> + }
You need curly braces around all branches if one of them has multiple
statements.
Given that the above pattern is repeated twice already, a helper
function that returns the layer pointer would be useful. Then you could
simply write
layer = ...(dp);
if (!layer) {
*num_input_fmts = 0;
return NULL;
}
With these small issues addressed,
Reviewed-by: Laurent Pinchart
> + layer = dp->dpsub->layers[layer_id];
> +
> + return zynqmp_disp_layer_formats(layer, num_input_fmts);
> +}
> +
> static const struct drm_bridge_funcs zynqmp_dp_bridge_fun
Re: [PATCH v2 2/8] drm: xlnx: zynqmp_dpsub: Update live format defines
Hi Anatoliy, Thank you for the patch. On Tue, Mar 12, 2024 at 05:54:59PM -0700, Anatoliy Klymenko wrote: > Update live format defines to match DPSUB AV_BUF_LIVE_VID_CONFIG register > layout. > > Signed-off-by: Anatoliy Klymenko Reviewed-by: Laurent Pinchart > --- > drivers/gpu/drm/xlnx/zynqmp_disp_regs.h | 8 > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h > b/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h > index f92a006d5070..fa3935384834 100644 > --- a/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h > +++ b/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h > @@ -165,10 +165,10 @@ > #define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_100x2 > #define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_120x3 > #define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_MASK GENMASK(2, 0) > -#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_RGB 0x0 > -#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV4440x1 > -#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV4220x2 > -#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YONLY 0x3 > +#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_RGB (0x0 << 4) > +#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV444(0x1 << 4) > +#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV422(0x2 << 4) > +#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YONLY (0x3 << 4) > #define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_MASK GENMASK(5, 4) > #define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_CB_FIRST BIT(8) > #define ZYNQMP_DISP_AV_BUF_PALETTE_MEMORY0x400 > -- Regards, Laurent Pinchart
Re: [PATCH] drm: bridge: thc63lvd1024: Print error message when DT parsing fails
Hi Sui,
On Tue, Mar 19, 2024 at 12:42:41AM +0800, Sui Jingfeng wrote:
> On 2024/3/19 00:06, Laurent Pinchart wrote:
> > Commit 00084f0c01bf ("drm: bridge: thc63lvd1024: Switch to use
> > of_graph_get_remote_node()") simplified the thc63lvd1024 driver by
> > replacing hand-rolled code with a helper function.
>
> [...]
>
> > While doing so, it
> > created an error code path at probe time without any error message,
>
> If this is a reason or a concern, then every drm bridges drivers will suffer
> from
> such a concern. Right?
Yes, bridge drivers (or any driver, really) should avoid failing probe
silently.
> > potentially causing probe issues that get annoying to debug.
>
> Sorry, let's keep it fair enough, it creates nothing annoyed.
>
> If there is a probe issues, then, it is caused by ill-behavioral DT.
> *NOT* my patch. And should be found during review stage.
Even before the review stage, in the DT development stage. My point is
that creating a silent failure path in probe will make it more difficult
for DT developers to debug issues.
> If the of_graph_get_remote_node() function is not good enough,
> I suggest to improve the of_graph_get_remote_node() function,
> then all callers of it will benefits.
>
> Well, the strong word here just terrifying new programmers to call
> core function helpers. Please use more *soft* description in the
> commit message.
Could you please propose a wording that you would consider more soft ?
> > Fix it by
> > adding an error message.
> >
> > Fixes: 00084f0c01bf ("drm: bridge: thc63lvd1024: Switch to use
> > of_graph_get_remote_node()")
>
> Please drop the fixes tag at here, append the tag to a real bug-fix patch
> will make more sense imo.
> I suggest to improve the of_graph_get_remote_node() function, then all
> callers of it will benefits.
> NOT a single implement like this.
Improving core helpers is certainly a good idea, and if we do so, we can
simplify drivers. What I'm concerned is that commit 00084f0c01bf creates
a silent probe failure path, which didn't exist before it. This is why
this patch references it in the Fixes: tag, making sure that this patch
will get backported to any stable kernel that includes commit
00084f0c01bf. As far as I understand, this is business as usual. There's
nothing personal here, and no judgement on the quality of your code.
> > Signed-off-by: Laurent Pinchart
> > ---
> > drivers/gpu/drm/bridge/thc63lvd1024.c | 5 -
> > 1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/thc63lvd1024.c
> > b/drivers/gpu/drm/bridge/thc63lvd1024.c
> > index 5f99f9724081..674efc489e3a 100644
> > --- a/drivers/gpu/drm/bridge/thc63lvd1024.c
> > +++ b/drivers/gpu/drm/bridge/thc63lvd1024.c
> > @@ -125,8 +125,11 @@ static int thc63_parse_dt(struct thc63_dev *thc63)
> >
> > remote = of_graph_get_remote_node(thc63->dev->of_node,
> > THC63_RGB_OUT0, -1);
> > - if (!remote)
> > + if (!remote) {
> > + dev_err(thc63->dev, "No remote endpoint for port@%u\n",
> > + THC63_RGB_OUT0);
> > return -ENODEV;
> > + }
> >
> > thc63->next = of_drm_find_bridge(remote);
> > of_node_put(remote);
> >
> > base-commit: 00084f0c01bf3a2591d007010b196e048281c455
--
Regards,
Laurent Pinchart
Re: [PATCH 3/6] drm: zynqmp_dp: Add locking
Hi Sean,
On Mon, Mar 18, 2024 at 01:29:12PM -0400, Sean Anderson wrote:
> On 3/18/24 13:16, Laurent Pinchart wrote:
> > On Fri, Mar 15, 2024 at 07:09:13PM -0400, Sean Anderson wrote:
> >> Add some locking, since none is provided by the drm subsystem. This will
> >
> > That's not quite right, the DRM core doesn't call bridge operations
> > concurrently.
>
> I figured something like this was going on.
>
> > We may need locking to protect against race conditions
> > between bridge operations and interrupts though.
>
> And of course this will only get worse once we let userspace get involved.
>
> >> prevent the IRQ/workers/bridge API calls from stepping on each other's
> >> toes.
> >>
> >> Signed-off-by: Sean Anderson
> >> ---
> >>
> >> drivers/gpu/drm/xlnx/zynqmp_dp.c | 59 +++-
> >> 1 file changed, 42 insertions(+), 17 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c
> >> b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> >> index 8635b5673386..d2dee58e7bf2 100644
> >> --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
> >> +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> >> @@ -279,6 +279,7 @@ struct zynqmp_dp_config {
> >> * @dpsub: Display subsystem
> >> * @iomem: device I/O memory for register access
> >> * @reset: reset controller
> >> + * @lock: Mutex protecting this struct and register access (but not AUX)
> >
> > This patch does two things at once, it defers link training from the IRQ
> > handler to a work queue, and covers everything with a big lock. The
> > scope is too large.
>
> OK, I can split this.
>
> > Please restrict the lock scope and document the
> > individual fields that need to be protected, and explain the locking
> > design in the commit message (or comments in the code).
>
> As said, this lock protects
>
> - Non-atomic registers configuring the link. That is, everything but the IRQ
> registers (since these are accessed in an atomic fashion), and the DP AUX
> registers (since these don't affect the link).
> - Link configuration. This is effectively everything in zynqmp_dp which isn't
> read-only after probe time. So from next_bridge onward.
>
> It's designed to protect configuration changes so we don't have to do anything
> tricky. Configuration should never be in the hot path, so I'm not worried
> about
> performance.
If userspace can control all this directly through debugfs, can you
guarantee that locks will be enough ? The driver doesn't expect direct
userspace access. I have a feeling this is really quite hacky.
> >> * @irq: irq
> >> * @bridge: DRM bridge for the DP encoder
> >> * @next_bridge: The downstream bridge
> >> @@ -299,6 +300,7 @@ struct zynqmp_dp {
> >>struct zynqmp_dpsub *dpsub;
> >>void __iomem *iomem;
> >>struct reset_control *reset;
> >> + struct mutex lock;
> >>int irq;
> >>
> >>struct drm_bridge bridge;
> >> @@ -308,7 +310,7 @@ struct zynqmp_dp {
> >>struct drm_dp_aux aux;
> >>struct phy *phy[ZYNQMP_DP_MAX_LANES];
> >>u8 num_lanes;
> >> - struct delayed_work hpd_work;
> >> + struct delayed_work hpd_work, hpd_irq_work;
> >
> > One variable per line please.
>
> OK
>
> >>enum drm_connector_status status;
> >>bool enabled;
> >>
> >> @@ -1371,8 +1373,10 @@ zynqmp_dp_bridge_mode_valid(struct drm_bridge
> >> *bridge,
> >>}
> >>
> >>/* Check with link rate and lane count */
> >> + mutex_lock(>lock);
> >>rate = zynqmp_dp_max_rate(dp->link_config.max_rate,
> >> dp->link_config.max_lanes, dp->config.bpp);
> >> + mutex_unlock(>lock);
> >>if (mode->clock > rate) {
> >>dev_dbg(dp->dev, "filtered mode %s for high pixel rate\n",
> >>mode->name);
> >> @@ -1399,6 +1403,7 @@ static void zynqmp_dp_bridge_atomic_enable(struct
> >> drm_bridge *bridge,
> >>
> >>pm_runtime_get_sync(dp->dev);
> >>
> >> + mutex_lock(>lock);
> >>zynqmp_dp_disp_enable(dp, old_bridge_state);
> >>
> >>/*
> >> @@ -1459,6 +1464,7 @@ static void zynqmp_dp_bridge_atomic_enable(struct
> >> drm_bridge *bridge,
> >>zynqmp_dp_write(dp, ZYNQMP_DP_SOFTWARE_RESET,
> >>ZYNQMP_DP_SOFTWARE_RESET_ALL);
> >>
Re: [PATCH 6/6] drm: zynqmp_dp: Add debugfs interface for compliance testing
fops_zynqmp_dp_preemphasis,
> > zynqmp_dp_preemphasis_get,
> > +zynqmp_dp_preemphasis_set, "%llu\n");
> > +
> > +static int zynqmp_dp_lanes_get(void *data, u64 *val)
> > +{
> > + struct zynqmp_dp *dp = data;
> > +
> > + mutex_lock(>lock);
> > + *val = dp->test.link_cnt;
> > + mutex_unlock(>lock);
> > + return 0;
> > +}
> > +
> > +static int zynqmp_dp_lanes_set(void *data, u64 val)
> > +{
> > + struct zynqmp_dp *dp = data;
> > + int ret = 0;
> > +
> > + if (val > ZYNQMP_DP_MAX_LANES)
> > + return -EINVAL;
> > +
> > + mutex_lock(>lock);
> > + if (val > dp->num_lanes) {
> > + ret = -EINVAL;
> > + } else {
> > + dp->test.link_cnt = val;
> > + if (dp->test.active)
> > + ret = zynqmp_dp_test_setup(dp);
> > + }
> > + mutex_unlock(>lock);
> > +
> > + return ret;
> > +}
> > +
> > +DEFINE_DEBUGFS_ATTRIBUTE(fops_zynqmp_dp_lanes, zynqmp_dp_lanes_get,
> > +zynqmp_dp_lanes_set, "%llu\n");
> > +
> > +static int zynqmp_dp_rate_get(void *data, u64 *val)
> > +{
> > + struct zynqmp_dp *dp = data;
> > +
> > + mutex_lock(>lock);
> > + *val = drm_dp_bw_code_to_link_rate(dp->test.bw_code) * 1;
> > + mutex_unlock(>lock);
> > + return 0;
> > +}
> > +
> > +static int zynqmp_dp_rate_set(void *data, u64 val)
> > +{
> > + struct zynqmp_dp *dp = data;
> > + u8 bw_code = drm_dp_link_rate_to_bw_code(val / 1);
> > + int link_rate = drm_dp_bw_code_to_link_rate(bw_code);
> > + int ret = 0;
> > +
> > + if (val / 1 != link_rate)
> > + return -EINVAL;
> > +
> > + if (bw_code != DP_LINK_BW_1_62 && bw_code != DP_LINK_BW_2_7 &&
> > + bw_code != DP_LINK_BW_5_4)
> > + return -EINVAL;
> > +
> > + mutex_lock(>lock);
> > + dp->test.bw_code = bw_code;
> > + if (dp->test.active)
> > + ret = zynqmp_dp_test_setup(dp);
> > + mutex_unlock(>lock);
> > +
> > + return ret;
> > +}
> > +
> > +DEFINE_DEBUGFS_ATTRIBUTE(fops_zynqmp_dp_rate, zynqmp_dp_rate_get,
> > +zynqmp_dp_rate_set, "%llu\n");
> > +
> > +static void zynqmp_dp_bridge_debugfs_init(struct drm_bridge *bridge,
> > + struct dentry *root)
> > +{
> > + struct zynqmp_dp *dp = bridge_to_dp(bridge);
> > + struct dentry *test;
> > + int i;
> > +
> > + dp->test.bw_code = DP_LINK_BW_5_4;
> > + dp->test.link_cnt = dp->num_lanes;
> > +
> > + test = debugfs_create_dir("test", root);
> > +#define CREATE_FILE(name) \
> > + debugfs_create_file(#name, 0600, test, dp, _zynqmp_dp_##name)
> > + CREATE_FILE(pattern);
> > + CREATE_FILE(enhanced);
> > + CREATE_FILE(downspread);
> > + CREATE_FILE(active);
> > + CREATE_FILE(custom);
> > + CREATE_FILE(rate);
> > + CREATE_FILE(lanes);
> > +
> > + for (i = 0; i < dp->num_lanes; i++) {
> > + static const char fmt[] = "lane%d_preemphasis";
> > + char name[sizeof(fmt)];
> > +
> > + dp->debugfs_train_set[i].dp = dp;
> > + dp->debugfs_train_set[i].lane = i;
> > +
> > + sprintf(name, fmt, i);
> > + debugfs_create_file(name, 0600, test,
> > + >debugfs_train_set[i],
> > + _zynqmp_dp_preemphasis);
> > +
> > + sprintf(name, "lane%d_swing", i);
> > + debugfs_create_file(name, 0600, test,
> > + >debugfs_train_set[i],
> > + _zynqmp_dp_swing);
> > + }
> > +}
> > +
> > static const struct drm_bridge_funcs zynqmp_dp_bridge_funcs = {
> > .attach = zynqmp_dp_bridge_attach,
> > .detach = zynqmp_dp_bridge_detach,
> > @@ -1611,6 +2189,7 @@ static const struct drm_bridge_funcs
> > zynqmp_dp_bridge_funcs = {
> > .atomic_check = zynqmp_dp_bridge_atomic_check,
> > .detect = zynqmp_dp_bridge_detect,
> > .get_edid = zynqmp_dp_bridge_get_edid,
> > + .debugfs_init = zynqmp_dp_bridge_debugfs_init,
> > };
> >
> > /*
> > -
> > @@ -1645,6 +2224,9 @@ static void zynqmp_dp_hpd_work_func(struct
> > work_struct *work)
> > hpd_work.work);
> > enum drm_connector_status status;
> >
> > + if (dp->test.active)
> > + return;
> > +
> > status = zynqmp_dp_bridge_detect(>bridge);
> > drm_bridge_hpd_notify(>bridge, status);
> > }
> > @@ -1666,7 +2248,14 @@ static void zynqmp_dp_hpd_irq_work_func(struct
> > work_struct *work)
> > if (status[4] & DP_LINK_STATUS_UPDATED ||
> > !drm_dp_clock_recovery_ok([2],
> > dp->mode.lane_cnt) ||
> > !drm_dp_channel_eq_ok([2], dp->mode.lane_cnt)) {
> > - zynqmp_dp_train_loop(dp);
> > + if (dp->test.active) {
> > + dev_dbg(dp->dev, "Ignoring HPD IRQ in test
> > mode\n");
> > + } else {
> > + dev_dbg(dp->dev,
> > + "Retraining due to HPD IRQ (status
> > is [%*ph])\n",
> > + (int)sizeof(status), status);
> > + zynqmp_dp_train_loop(dp);
> > + }
> > }
> > }
> > mutex_unlock(>lock);
--
Regards,
Laurent Pinchart
Re: [PATCH 6/6] drm: zynqmp_dp: Add debugfs interface for compliance testing
On Mon, Mar 18, 2024 at 11:06:40AM -0400, Sean Anderson wrote:
> On 3/16/24 06:14, kernel test robot wrote:
> > Hi Sean,
> >
> > kernel test robot noticed the following build warnings:
> >
> > [auto build test WARNING on v6.8]
> > [cannot apply to drm-misc/drm-misc-next linus/master next-20240315]
> > [If your patch is applied to the wrong git tree, kindly drop us a note.
> > And when submitting patch, we suggest to use '--base' as documented in
> > https://git-scm.com/docs/git-format-patch#_base_tree_information]
> >
> > url:
> > https://github.com/intel-lab-lkp/linux/commits/Sean-Anderson/drm-zynqmp_dp-Downgrade-log-level-for-aux-retries-message/20240316-071208
> > base: v6.8
> > patch link:
> > https://lore.kernel.org/r/20240315230916.1759060-7-sean.anderson%40linux.dev
> > patch subject: [PATCH 6/6] drm: zynqmp_dp: Add debugfs interface for
> > compliance testing
> > config: microblaze-allmodconfig
> > (https://download.01.org/0day-ci/archive/20240316/202403161704.achjdsjg-...@intel.com/config)
> > compiler: microblaze-linux-gcc (GCC) 13.2.0
> > reproduce (this is a W=1 build):
> > (https://download.01.org/0day-ci/archive/20240316/202403161704.achjdsjg-...@intel.com/reproduce)
> >
> > If you fix the issue in a separate patch/commit (i.e. not just a new
> > version of
> > the same patch/commit), kindly add following tags
> > | Reported-by: kernel test robot
> > | Closes:
> > https://lore.kernel.org/oe-kbuild-all/202403161704.achjdsjg-...@intel.com/
> >
> > All warnings (new ones prefixed by >>):
> >
> >drivers/gpu/drm/xlnx/zynqmp_dp.c: In function
> > 'zynqmp_dp_bridge_debugfs_init':
> >>> drivers/gpu/drm/xlnx/zynqmp_dp.c:2168:31: warning: 'sprintf' may write a
> >>> terminating nul past the end of the destination [-Wformat-overflow=]
> > 2168 | sprintf(name, fmt, i);
> > | ^~~
> >drivers/gpu/drm/xlnx/zynqmp_dp.c:2168:17: note: 'sprintf' output between
> > 18 and 20 bytes into a destination of size 19
> > 2168 | sprintf(name, fmt, i);
> > | ^
>
> Not a bug, as i will be at most 4, which uses 1 digit.
The compiler can't know that. Please fix this, there's a zero warning
policy.
> > vim +/sprintf +2168 drivers/gpu/drm/xlnx/zynqmp_dp.c
> >
> > 2136
> > 2137 DEFINE_DEBUGFS_ATTRIBUTE(fops_zynqmp_dp_rate,
> > zynqmp_dp_rate_get,
> > 2138 zynqmp_dp_rate_set, "%llu\n");
> > 2139
> > 2140 static void zynqmp_dp_bridge_debugfs_init(struct drm_bridge
> > *bridge,
> > 2141struct dentry *root)
> > 2142 {
> > 2143 struct zynqmp_dp *dp = bridge_to_dp(bridge);
> > 2144 struct dentry *test;
> > 2145 int i;
> > 2146
> > 2147 dp->test.bw_code = DP_LINK_BW_5_4;
> > 2148 dp->test.link_cnt = dp->num_lanes;
> > 2149
> > 2150 test = debugfs_create_dir("test", root);
> > 2151 #define CREATE_FILE(name) \
> > 2152 debugfs_create_file(#name, 0600, test, dp,
> > _zynqmp_dp_##name)
> > 2153 CREATE_FILE(pattern);
> > 2154 CREATE_FILE(enhanced);
> > 2155 CREATE_FILE(downspread);
> > 2156 CREATE_FILE(active);
> > 2157 CREATE_FILE(custom);
> > 2158 CREATE_FILE(rate);
> > 2159 CREATE_FILE(lanes);
> > 2160
> > 2161 for (i = 0; i < dp->num_lanes; i++) {
> > 2162 static const char fmt[] = "lane%d_preemphasis";
> > 2163 char name[sizeof(fmt)];
> > 2164
> > 2165 dp->debugfs_train_set[i].dp = dp;
> > 2166 dp->debugfs_train_set[i].lane = i;
> > 2167
> >> 2168 sprintf(name, fmt, i);
> > 2169 debugfs_create_file(name, 0600, test,
> > 2170 >debugfs_train_set[i],
> > 2171
> > _zynqmp_dp_preemphasis);
> > 2172
> > 2173 sprintf(name, "lane%d_swing", i);
> > 2174 debugfs_create_file(name, 0600, test,
> > 2175 >debugfs_train_set[i],
> > 2176 _zynqmp_dp_swing);
> > 2177 }
> > 2178 }
> > 2179
--
Regards,
Laurent Pinchart
Re: [PATCH 5/6] drm: zynqmp_dp: Optionally ignore DPCD errors
> + return ret;
> }
>
> ret = drm_dp_dpcd_writeb(>aux, DP_LINK_BW_SET, bw_code);
> if (ret < 0) {
> - dev_err(dp->dev, "failed to set DP bandwidth\n");
> - return ret;
> + dev_warn(dp->dev, "failed to set DP bandwidth\n");
> + if (!ignore_dpcd)
> + return ret;
> }
>
> zynqmp_dp_write(dp, ZYNQMP_DP_LINK_BW_SET, bw_code);
> @@ -860,7 +869,7 @@ static int zynqmp_dp_train(struct zynqmp_dp *dp)
>
> ret = zynqmp_dp_setup(dp, dp->mode.bw_code, dp->mode.lane_cnt,
> drm_dp_enhanced_frame_cap(dp->dpcd),
> - dp->dpcd[3] & 0x1);
> + dp->dpcd[3] & 0x1, false);
> if (ret)
> return ret;
>
> @@ -877,7 +886,7 @@ static int zynqmp_dp_train(struct zynqmp_dp *dp)
> ret = drm_dp_dpcd_writeb(>aux, DP_TRAINING_PATTERN_SET,
>DP_TRAINING_PATTERN_DISABLE);
> if (ret < 0) {
> - dev_err(dp->dev, "failed to disable training pattern\n");
> + dev_warn(dp->dev, "failed to disable training pattern\n");
> return ret;
> }
> zynqmp_dp_write(dp, ZYNQMP_DP_TRAINING_PATTERN_SET,
--
Regards,
Laurent Pinchart
Re: [PATCH 4/6] drm: zynqmp_dp: Split off several helper functions
if (ret < 0)
> + return zynqmp_dp_phy_ready(dp);
> +}
> +
> +
> +/**
> + * zynqmp_dp_train - Train the link
> + * @dp: DisplayPort IP core structure
> + *
> + * Return: 0 if all trains are done successfully, or corresponding error
> code.
> + */
> +static int zynqmp_dp_train(struct zynqmp_dp *dp)
> +{
> + int ret;
> +
> + ret = zynqmp_dp_setup(dp, dp->mode.bw_code, dp->mode.lane_cnt,
> + drm_dp_enhanced_frame_cap(dp->dpcd),
> + dp->dpcd[3] & 0x1);
This patch looks OK. Assuming you make correct use of the new functions
in the next patches,
Reviewed-by: Laurent Pinchart
On a side note, I think the above could be written
ret = zynqmp_dp_setup(dp, dp->mode.bw_code, dp->mode.lane_cnt,
drm_dp_enhanced_frame_cap(dp->dpcd),
dp->dpcd[DP_MAX_DOWNSPREAD] &
DP_MAX_DOWNSPREAD_0_5);
> + if (ret)
> return ret;
>
> zynqmp_dp_write(dp, ZYNQMP_DP_SCRAMBLING_DISABLE, 1);
--
Regards,
Laurent Pinchart
Re: [PATCH 3/6] drm: zynqmp_dp: Add locking
cted;
> }
>
> @@ -1611,6 +1623,29 @@ static void zynqmp_dp_hpd_work_func(struct work_struct
> *work)
> drm_bridge_hpd_notify(>bridge, status);
> }
>
> +static void zynqmp_dp_hpd_irq_work_func(struct work_struct *work)
> +{
> + struct zynqmp_dp *dp = container_of(work, struct zynqmp_dp,
> + hpd_irq_work.work);
> + u8 status[DP_LINK_STATUS_SIZE + 2];
> + int err;
> +
> + mutex_lock(>lock);
> + err = drm_dp_dpcd_read(>aux, DP_SINK_COUNT, status,
> +DP_LINK_STATUS_SIZE + 2);
> + if (err < 0) {
> + dev_dbg_ratelimited(dp->dev,
> + "could not read sink status: %d\n", err);
> + } else {
> + if (status[4] & DP_LINK_STATUS_UPDATED ||
> + !drm_dp_clock_recovery_ok([2], dp->mode.lane_cnt) ||
> + !drm_dp_channel_eq_ok([2], dp->mode.lane_cnt)) {
> + zynqmp_dp_train_loop(dp);
> + }
> + }
> + mutex_unlock(>lock);
> +}
> +
> static irqreturn_t zynqmp_dp_irq_handler(int irq, void *data)
> {
> struct zynqmp_dp *dp = (struct zynqmp_dp *)data;
> @@ -1635,23 +1670,9 @@ static irqreturn_t zynqmp_dp_irq_handler(int irq, void
> *data)
> if (status & ZYNQMP_DP_INT_HPD_EVENT)
> schedule_delayed_work(>hpd_work, 0);
>
> - if (status & ZYNQMP_DP_INT_HPD_IRQ) {
> - int ret;
> - u8 status[DP_LINK_STATUS_SIZE + 2];
> + if (status & ZYNQMP_DP_INT_HPD_IRQ)
> + schedule_delayed_work(>hpd_irq_work, 0);
>
> - ret = drm_dp_dpcd_read(>aux, DP_SINK_COUNT, status,
> -DP_LINK_STATUS_SIZE + 2);
> - if (ret < 0)
> - goto handled;
> -
> - if (status[4] & DP_LINK_STATUS_UPDATED ||
> - !drm_dp_clock_recovery_ok([2], dp->mode.lane_cnt) ||
> - !drm_dp_channel_eq_ok([2], dp->mode.lane_cnt)) {
> - zynqmp_dp_train_loop(dp);
> - }
> - }
> -
> -handled:
> return IRQ_HANDLED;
> }
>
> @@ -1674,8 +1695,10 @@ int zynqmp_dp_probe(struct zynqmp_dpsub *dpsub)
> dp->dev = >dev;
> dp->dpsub = dpsub;
> dp->status = connector_status_disconnected;
> + mutex_init(>lock);
>
> INIT_DELAYED_WORK(>hpd_work, zynqmp_dp_hpd_work_func);
> + INIT_DELAYED_WORK(>hpd_irq_work, zynqmp_dp_hpd_irq_work_func);
>
> /* Acquire all resources (IOMEM, IRQ and PHYs). */
> res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dp");
> @@ -1775,6 +1798,7 @@ void zynqmp_dp_remove(struct zynqmp_dpsub *dpsub)
> zynqmp_dp_write(dp, ZYNQMP_DP_INT_DS, ZYNQMP_DP_INT_ALL);
> disable_irq(dp->irq);
>
> + cancel_delayed_work_sync(>hpd_irq_work);
> cancel_delayed_work_sync(>hpd_work);
>
> zynqmp_dp_write(dp, ZYNQMP_DP_TRANSMITTER_ENABLE, 0);
> @@ -1782,4 +1806,5 @@ void zynqmp_dp_remove(struct zynqmp_dpsub *dpsub)
>
> zynqmp_dp_phy_exit(dp);
> zynqmp_dp_reset(dp, true);
> + mutex_destroy(>lock);
> }
--
Regards,
Laurent Pinchart
Re: [PATCH 2/6] drm: zynqmp_dp: Adjust training values per-lane
Hi Sean,
Thank you for the patch.
On Fri, Mar 15, 2024 at 07:09:12PM -0400, Sean Anderson wrote:
> The feedback we get from the DPRX is per-lane. Make changes using this
> information, instead of picking the maximum values from all lanes. This
> results in more-consistent training on marginal links.
>
> Signed-off-by: Sean Anderson
> ---
>
> drivers/gpu/drm/xlnx/zynqmp_dp.c | 23 ---
> 1 file changed, 8 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c
> b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> index 98a32e6a0459..8635b5673386 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> @@ -605,28 +605,21 @@ static void zynqmp_dp_adjust_train(struct zynqmp_dp *dp,
> u8 link_status[DP_LINK_STATUS_SIZE])
> {
> u8 *train_set = dp->train_set;
> - u8 voltage = 0, preemphasis = 0;
> u8 i;
>
> for (i = 0; i < dp->mode.lane_cnt; i++) {
> - u8 v = drm_dp_get_adjust_request_voltage(link_status, i);
> - u8 p = drm_dp_get_adjust_request_pre_emphasis(link_status, i);
> + u8 voltage = drm_dp_get_adjust_request_voltage(link_status, i);
> + u8 preemphasis =
> + drm_dp_get_adjust_request_pre_emphasis(link_status, i);
>
> - if (v > voltage)
> - voltage = v;
> + if (voltage >= DP_TRAIN_VOLTAGE_SWING_LEVEL_3)
> + voltage |= DP_TRAIN_MAX_SWING_REACHED;
>
> - if (p > preemphasis)
> - preemphasis = p;
> - }
> + if (preemphasis >= DP_TRAIN_PRE_EMPH_LEVEL_2)
> + preemphasis |= DP_TRAIN_MAX_PRE_EMPHASIS_REACHED;
>
> - if (voltage >= DP_TRAIN_VOLTAGE_SWING_LEVEL_3)
> - voltage |= DP_TRAIN_MAX_SWING_REACHED;
> -
> - if (preemphasis >= DP_TRAIN_PRE_EMPH_LEVEL_2)
> - preemphasis |= DP_TRAIN_MAX_PRE_EMPHASIS_REACHED;
> -
> - for (i = 0; i < dp->mode.lane_cnt; i++)
> train_set[i] = voltage | preemphasis;
> + }
I don't have enough DP knowledge to review this :-(
> }
>
> /**
--
Regards,
Laurent Pinchart
Re: [PATCH 1/6] drm: zynqmp_dp: Downgrade log level for aux retries message
Hi Sean,
Thank you for the patch.
On Fri, Mar 15, 2024 at 07:09:11PM -0400, Sean Anderson wrote:
> Enable this message for verbose debugging only as it is otherwise
> printed after every AUX message, quickly filling the log buffer.
>
> Signed-off-by: Sean Anderson
> ---
>
> drivers/gpu/drm/xlnx/zynqmp_dp.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c
> b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> index a0606fab0e22..98a32e6a0459 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> @@ -1006,7 +1006,7 @@ zynqmp_dp_aux_transfer(struct drm_dp_aux *aux, struct
> drm_dp_aux_msg *msg)
> msg->buffer, msg->size,
> >reply);
> if (!ret) {
> - dev_dbg(dp->dev, "aux %d retries\n", i);
> + dev_vdbg(dp->dev, "aux %d retries\n", i);
I didn't know about dev_vdbg(). I suppose this makes sense,
Reviewed-by: Laurent Pinchart
> return msg->size;
> }
>
--
Regards,
Laurent Pinchart
[PATCH] drm: bridge: thc63lvd1024: Print error message when DT parsing fails
Commit 00084f0c01bf ("drm: bridge: thc63lvd1024: Switch to use
of_graph_get_remote_node()") simplified the thc63lvd1024 driver by
replacing hand-rolled code with a helper function. While doing so, it
created an error code path at probe time without any error message,
potentially causing probe issues that get annoying to debug. Fix it by
adding an error message.
Fixes: 00084f0c01bf ("drm: bridge: thc63lvd1024: Switch to use
of_graph_get_remote_node()")
Signed-off-by: Laurent Pinchart
---
drivers/gpu/drm/bridge/thc63lvd1024.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/bridge/thc63lvd1024.c
b/drivers/gpu/drm/bridge/thc63lvd1024.c
index 5f99f9724081..674efc489e3a 100644
--- a/drivers/gpu/drm/bridge/thc63lvd1024.c
+++ b/drivers/gpu/drm/bridge/thc63lvd1024.c
@@ -125,8 +125,11 @@ static int thc63_parse_dt(struct thc63_dev *thc63)
remote = of_graph_get_remote_node(thc63->dev->of_node,
THC63_RGB_OUT0, -1);
- if (!remote)
+ if (!remote) {
+ dev_err(thc63->dev, "No remote endpoint for port@%u\n",
+ THC63_RGB_OUT0);
return -ENODEV;
+ }
thc63->next = of_drm_find_bridge(remote);
of_node_put(remote);
base-commit: 00084f0c01bf3a2591d007010b196e048281c455
--
Regards,
Laurent Pinchart
Re: [PATCH] drm: bridge: thc63lvd1024: Switch to use of_graph_get_remote_node()
On Mon, Mar 18, 2024 at 11:53:11PM +0800, Sui Jingfeng wrote:
> On 2024/3/18 23:33, Laurent Pinchart wrote:
> > On Sun, Mar 17, 2024 at 01:28:00AM +0800, Sui Jingfeng wrote:
> >> To reduce boilerplate, use of_graph_get_remote_node() helper instead of
> >> the hand-rolling code.
> >>
> >> Signed-off-by: Sui Jingfeng
> >> ---
> >> drivers/gpu/drm/bridge/thc63lvd1024.c | 24 +++-
> >> 1 file changed, 3 insertions(+), 21 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/bridge/thc63lvd1024.c
> >> b/drivers/gpu/drm/bridge/thc63lvd1024.c
> >> index d4c1a601bbb5..5f99f9724081 100644
> >> --- a/drivers/gpu/drm/bridge/thc63lvd1024.c
> >> +++ b/drivers/gpu/drm/bridge/thc63lvd1024.c
> >> @@ -123,28 +123,10 @@ static int thc63_parse_dt(struct thc63_dev *thc63)
> >>struct device_node *endpoint;
> >>struct device_node *remote;
> >>
> >> - endpoint = of_graph_get_endpoint_by_regs(thc63->dev->of_node,
> >> - THC63_RGB_OUT0, -1);
> >> - if (!endpoint) {
> >> - dev_err(thc63->dev, "Missing endpoint in port@%u\n",
> >> - THC63_RGB_OUT0);
> >> - return -ENODEV;
> >> - }
> >> -
> >> - remote = of_graph_get_remote_port_parent(endpoint);
> >> - of_node_put(endpoint);
> >> - if (!remote) {
> >> - dev_err(thc63->dev, "Endpoint in port@%u unconnected\n",
> >> - THC63_RGB_OUT0);
> >> + remote = of_graph_get_remote_node(thc63->dev->of_node,
> >> +THC63_RGB_OUT0, -1);
> >> + if (!remote)
> > The old logic is equivalent to of_graph_get_remote_node(), but now the
> > driver will fail probing without an error message. That's not very nice
> > as it leads to difficult to debug problems. I would keep one dev_err()
> > here:
> >
> > dev_err(thc63->dev, "No remote endpoint for port@%u\n",
> > THC63_RGB_OUT0);
> >
> > As your patch has been merged already, would you like to post a
> > follow-up patch to fix this ?
>
> Yes, this is indeed a difference. I will post a follow-up patch to fix this.
I'm actually build-testing a patch I already wrote :-) I'll post it in a
moment.
> >>return -ENODEV;
> >> - }
> >> -
> >> - if (!of_device_is_available(remote)) {
> >> - dev_err(thc63->dev, "port@%u remote endpoint is disabled\n",
> >> - THC63_RGB_OUT0);
> >> - of_node_put(remote);
> >> - return -ENODEV;
> >> - }
> >>
> >>thc63->next = of_drm_find_bridge(remote);
> >>of_node_put(remote);
--
Regards,
Laurent Pinchart
Re: [PATCH] drm: bridge: thc63lvd1024: Switch to use of_graph_get_remote_node()
Hi Sui,
Thank you for the patch.
On Sun, Mar 17, 2024 at 01:28:00AM +0800, Sui Jingfeng wrote:
> To reduce boilerplate, use of_graph_get_remote_node() helper instead of
> the hand-rolling code.
>
> Signed-off-by: Sui Jingfeng
> ---
> drivers/gpu/drm/bridge/thc63lvd1024.c | 24 +++-
> 1 file changed, 3 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/thc63lvd1024.c
> b/drivers/gpu/drm/bridge/thc63lvd1024.c
> index d4c1a601bbb5..5f99f9724081 100644
> --- a/drivers/gpu/drm/bridge/thc63lvd1024.c
> +++ b/drivers/gpu/drm/bridge/thc63lvd1024.c
> @@ -123,28 +123,10 @@ static int thc63_parse_dt(struct thc63_dev *thc63)
> struct device_node *endpoint;
> struct device_node *remote;
>
> - endpoint = of_graph_get_endpoint_by_regs(thc63->dev->of_node,
> - THC63_RGB_OUT0, -1);
> - if (!endpoint) {
> - dev_err(thc63->dev, "Missing endpoint in port@%u\n",
> - THC63_RGB_OUT0);
> - return -ENODEV;
> - }
> -
> - remote = of_graph_get_remote_port_parent(endpoint);
> - of_node_put(endpoint);
> - if (!remote) {
> - dev_err(thc63->dev, "Endpoint in port@%u unconnected\n",
> - THC63_RGB_OUT0);
> + remote = of_graph_get_remote_node(thc63->dev->of_node,
> + THC63_RGB_OUT0, -1);
> + if (!remote)
The old logic is equivalent to of_graph_get_remote_node(), but now the
driver will fail probing without an error message. That's not very nice
as it leads to difficult to debug problems. I would keep one dev_err()
here:
dev_err(thc63->dev, "No remote endpoint for port@%u\n",
THC63_RGB_OUT0);
As your patch has been merged already, would you like to post a
follow-up patch to fix this ?
> return -ENODEV;
> - }
> -
> - if (!of_device_is_available(remote)) {
> - dev_err(thc63->dev, "port@%u remote endpoint is disabled\n",
> - THC63_RGB_OUT0);
> - of_node_put(remote);
> - return -ENODEV;
> - }
>
> thc63->next = of_drm_find_bridge(remote);
> of_node_put(remote);
--
Regards,
Laurent Pinchart
Re: [PATCH] drm/panel: ilitek-ili9881c: Fix warning with GPIO controllers that sleep
Hi Neil,
On Mon, Mar 18, 2024 at 10:03:21AM +0100, Neil Armstrong wrote:
> On 17/03/2024 16:48, Laurent Pinchart wrote:
> > The ilitek-ili9881c controls the reset GPIO using the non-sleeping
> > gpiod_set_value() function. This complains loudly when the GPIO
> > controller needs to sleep. As the caller can sleep, use
> > gpiod_set_value_cansleep() to fix the issue.
>
> Seems something buggy happened to the patchset, this patch doesn't appear
> in the cover letter and insn't numbered...
I've sent it separately, as it's functionally independent from the small
series for the same driver.
> > Signed-off-by: Laurent Pinchart
> > ---
> > drivers/gpu/drm/panel/panel-ilitek-ili9881c.c | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/panel/panel-ilitek-ili9881c.c
> > b/drivers/gpu/drm/panel/panel-ilitek-ili9881c.c
> > index 80b386f91320..084c37fa7348 100644
> > --- a/drivers/gpu/drm/panel/panel-ilitek-ili9881c.c
> > +++ b/drivers/gpu/drm/panel/panel-ilitek-ili9881c.c
> > @@ -1276,10 +1276,10 @@ static int ili9881c_prepare(struct drm_panel *panel)
> > msleep(5);
> >
> > /* And reset it */
> > - gpiod_set_value(ctx->reset, 1);
> > + gpiod_set_value_cansleep(ctx->reset, 1);
> > msleep(20);
> >
> > - gpiod_set_value(ctx->reset, 0);
> > + gpiod_set_value_cansleep(ctx->reset, 0);
> > msleep(20);
> >
> > for (i = 0; i < ctx->desc->init_length; i++) {
> > @@ -1334,7 +1334,7 @@ static int ili9881c_unprepare(struct drm_panel *panel)
> >
> > mipi_dsi_dcs_enter_sleep_mode(ctx->dsi);
> > regulator_disable(ctx->power);
> > - gpiod_set_value(ctx->reset, 1);
> > + gpiod_set_value_cansleep(ctx->reset, 1);
> >
> > return 0;
> > }
>
> Anyway:
> Reviewed-by: Neil Armstrong
--
Regards,
Laurent Pinchart
[PATCH 1/2] dt-bindings: ili9881c: Add Startek KD050HDFIA020-C020A support
Document the compatible value for Startek KD050HDFIA020-C020A panels. Signed-off-by: Laurent Pinchart --- .../devicetree/bindings/display/panel/ilitek,ili9881c.yaml | 1 + 1 file changed, 1 insertion(+) diff --git a/Documentation/devicetree/bindings/display/panel/ilitek,ili9881c.yaml b/Documentation/devicetree/bindings/display/panel/ilitek,ili9881c.yaml index b1e624be3e33..a015dce72f60 100644 --- a/Documentation/devicetree/bindings/display/panel/ilitek,ili9881c.yaml +++ b/Documentation/devicetree/bindings/display/panel/ilitek,ili9881c.yaml @@ -19,6 +19,7 @@ properties: - ampire,am8001280g - bananapi,lhr050h41 - feixin,k101-im2byl02 + - startek,kd050hdfia020 - tdo,tl050hdv35 - wanchanglong,w552946aba - const: ilitek,ili9881c -- Regards, Laurent Pinchart
[PATCH 2/2] drm/panel: ilitek-ili9881c: Add Startek KD050HDFIA020-C020A support
Add support for the Startek KD050HDFIA020-C020A panel.
The init table comes from the Compulab BSP ([1]).
[1]
https://github.com/compulab-yokneam/meta-bsp-imx8mp/blob/ucm-imx8m-plus-r1.1/recipes-kernel/linux/compulab/imx8mp/-compulab-imx8m-plus-Add-boards-support.patch
Signed-off-by: Laurent Pinchart
---
drivers/gpu/drm/panel/panel-ilitek-ili9881c.c | 222 ++
1 file changed, 222 insertions(+)
diff --git a/drivers/gpu/drm/panel/panel-ilitek-ili9881c.c
b/drivers/gpu/drm/panel/panel-ilitek-ili9881c.c
index 2ffe5f68a890..80b386f91320 100644
--- a/drivers/gpu/drm/panel/panel-ilitek-ili9881c.c
+++ b/drivers/gpu/drm/panel/panel-ilitek-ili9881c.c
@@ -455,6 +455,202 @@ static const struct ili9881c_instr k101_im2byl02_init[] =
{
ILI9881C_COMMAND_INSTR(0xD3, 0x3F), /* VN0 */
};
+static const struct ili9881c_instr kd050hdfia020_init[] = {
+ ILI9881C_SWITCH_PAGE_INSTR(3),
+ ILI9881C_COMMAND_INSTR(0x01, 0x00),
+ ILI9881C_COMMAND_INSTR(0x02, 0x00),
+ ILI9881C_COMMAND_INSTR(0x03, 0x72),
+ ILI9881C_COMMAND_INSTR(0x04, 0x00),
+ ILI9881C_COMMAND_INSTR(0x05, 0x00),
+ ILI9881C_COMMAND_INSTR(0x06, 0x09),
+ ILI9881C_COMMAND_INSTR(0x07, 0x00),
+ ILI9881C_COMMAND_INSTR(0x08, 0x00),
+ ILI9881C_COMMAND_INSTR(0x09, 0x01),
+ ILI9881C_COMMAND_INSTR(0x0a, 0x00),
+ ILI9881C_COMMAND_INSTR(0x0b, 0x00),
+ ILI9881C_COMMAND_INSTR(0x0c, 0x01),
+ ILI9881C_COMMAND_INSTR(0x0d, 0x00),
+ ILI9881C_COMMAND_INSTR(0x0e, 0x00),
+ ILI9881C_COMMAND_INSTR(0x0f, 0x00),
+ ILI9881C_COMMAND_INSTR(0x10, 0x00),
+ ILI9881C_COMMAND_INSTR(0x11, 0x00),
+ ILI9881C_COMMAND_INSTR(0x12, 0x00),
+ ILI9881C_COMMAND_INSTR(0x13, 0x00),
+ ILI9881C_COMMAND_INSTR(0x14, 0x00),
+ ILI9881C_COMMAND_INSTR(0x15, 0x00),
+ ILI9881C_COMMAND_INSTR(0x16, 0x00),
+ ILI9881C_COMMAND_INSTR(0x17, 0x00),
+ ILI9881C_COMMAND_INSTR(0x18, 0x00),
+ ILI9881C_COMMAND_INSTR(0x19, 0x00),
+ ILI9881C_COMMAND_INSTR(0x1a, 0x00),
+ ILI9881C_COMMAND_INSTR(0x1b, 0x00),
+ ILI9881C_COMMAND_INSTR(0x1c, 0x00),
+ ILI9881C_COMMAND_INSTR(0x1d, 0x00),
+ ILI9881C_COMMAND_INSTR(0x1e, 0x40),
+ ILI9881C_COMMAND_INSTR(0x1f, 0x80),
+ ILI9881C_COMMAND_INSTR(0x20, 0x05),
+ ILI9881C_COMMAND_INSTR(0x20, 0x05),
+ ILI9881C_COMMAND_INSTR(0x21, 0x02),
+ ILI9881C_COMMAND_INSTR(0x22, 0x00),
+ ILI9881C_COMMAND_INSTR(0x23, 0x00),
+ ILI9881C_COMMAND_INSTR(0x24, 0x00),
+ ILI9881C_COMMAND_INSTR(0x25, 0x00),
+ ILI9881C_COMMAND_INSTR(0x26, 0x00),
+ ILI9881C_COMMAND_INSTR(0x27, 0x00),
+ ILI9881C_COMMAND_INSTR(0x28, 0x33),
+ ILI9881C_COMMAND_INSTR(0x29, 0x02),
+ ILI9881C_COMMAND_INSTR(0x2a, 0x00),
+ ILI9881C_COMMAND_INSTR(0x2b, 0x00),
+ ILI9881C_COMMAND_INSTR(0x2c, 0x00),
+ ILI9881C_COMMAND_INSTR(0x2d, 0x00),
+ ILI9881C_COMMAND_INSTR(0x2e, 0x00),
+ ILI9881C_COMMAND_INSTR(0x2f, 0x00),
+ ILI9881C_COMMAND_INSTR(0x30, 0x00),
+ ILI9881C_COMMAND_INSTR(0x31, 0x00),
+ ILI9881C_COMMAND_INSTR(0x32, 0x00),
+ ILI9881C_COMMAND_INSTR(0x32, 0x00),
+ ILI9881C_COMMAND_INSTR(0x33, 0x00),
+ ILI9881C_COMMAND_INSTR(0x34, 0x04),
+ ILI9881C_COMMAND_INSTR(0x35, 0x00),
+ ILI9881C_COMMAND_INSTR(0x36, 0x00),
+ ILI9881C_COMMAND_INSTR(0x37, 0x00),
+ ILI9881C_COMMAND_INSTR(0x38, 0x3C),
+ ILI9881C_COMMAND_INSTR(0x39, 0x00),
+ ILI9881C_COMMAND_INSTR(0x3a, 0x40),
+ ILI9881C_COMMAND_INSTR(0x3b, 0x40),
+ ILI9881C_COMMAND_INSTR(0x3c, 0x00),
+ ILI9881C_COMMAND_INSTR(0x3d, 0x00),
+ ILI9881C_COMMAND_INSTR(0x3e, 0x00),
+ ILI9881C_COMMAND_INSTR(0x3f, 0x00),
+ ILI9881C_COMMAND_INSTR(0x40, 0x00),
+ ILI9881C_COMMAND_INSTR(0x41, 0x00),
+ ILI9881C_COMMAND_INSTR(0x42, 0x00),
+ ILI9881C_COMMAND_INSTR(0x43, 0x00),
+ ILI9881C_COMMAND_INSTR(0x44, 0x00),
+ ILI9881C_COMMAND_INSTR(0x50, 0x01),
+ ILI9881C_COMMAND_INSTR(0x51, 0x23),
+ ILI9881C_COMMAND_INSTR(0x52, 0x45),
+ ILI9881C_COMMAND_INSTR(0x53, 0x67),
+ ILI9881C_COMMAND_INSTR(0x54, 0x89),
+ ILI9881C_COMMAND_INSTR(0x55, 0xab),
+ ILI9881C_COMMAND_INSTR(0x56, 0x01),
+ ILI9881C_COMMAND_INSTR(0x57, 0x23),
+ ILI9881C_COMMAND_INSTR(0x58, 0x45),
+ ILI9881C_COMMAND_INSTR(0x59, 0x67),
+ ILI9881C_COMMAND_INSTR(0x5a, 0x89),
+ ILI9881C_COMMAND_INSTR(0x5b, 0xab),
+ ILI9881C_COMMAND_INSTR(0x5c, 0xcd),
+ ILI9881C_COMMAND_INSTR(0x5d, 0xef),
+ ILI9881C_COMMAND_INSTR(0x5e, 0x11),
+ ILI9881C_COMMAND_INSTR(0x5f, 0x01),
+ ILI9881C_COMMAND_INSTR(0x60, 0x00),
+ ILI9881C_COMMAND_INSTR(0x61, 0x15),
+ ILI9881C_COMMAND_INSTR(0x62, 0x14),
+ ILI9881C_COMMAND_INSTR(0x63, 0x0E),
+ ILI9881C_COMMAND_INSTR(0x64, 0x0F),
+ ILI9881C_COMMAND_INSTR(0x65, 0x0C),
+ ILI9881C_COMMAND_INSTR
[PATCH 0/2] drm/panel: Add Startek KD050HDFIA020-C020A support
Hello, This small series adds support for the Startek KD050HDFIA020-C020A panel to the ilitek-ili9881c driver. There's not much special here, patch 1/2 addresses the DT binding and patch 2/2 the driver. Please see individual patches for details. The series has been tested witha Compulab SB-UCM-iMX8MPLUS carrier board. Laurent Pinchart (2): dt-bindings: ili9881c: Add Startek KD050HDFIA020-C020A support drm/panel: ilitek-ili9881c: Add Startek KD050HDFIA020-C020A support .../display/panel/ilitek,ili9881c.yaml| 1 + drivers/gpu/drm/panel/panel-ilitek-ili9881c.c | 222 ++ 2 files changed, 223 insertions(+) -- Regards, Laurent Pinchart
[PATCH] drm/panel: ilitek-ili9881c: Fix warning with GPIO controllers that sleep
The ilitek-ili9881c controls the reset GPIO using the non-sleeping
gpiod_set_value() function. This complains loudly when the GPIO
controller needs to sleep. As the caller can sleep, use
gpiod_set_value_cansleep() to fix the issue.
Signed-off-by: Laurent Pinchart
---
drivers/gpu/drm/panel/panel-ilitek-ili9881c.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/panel/panel-ilitek-ili9881c.c
b/drivers/gpu/drm/panel/panel-ilitek-ili9881c.c
index 80b386f91320..084c37fa7348 100644
--- a/drivers/gpu/drm/panel/panel-ilitek-ili9881c.c
+++ b/drivers/gpu/drm/panel/panel-ilitek-ili9881c.c
@@ -1276,10 +1276,10 @@ static int ili9881c_prepare(struct drm_panel *panel)
msleep(5);
/* And reset it */
- gpiod_set_value(ctx->reset, 1);
+ gpiod_set_value_cansleep(ctx->reset, 1);
msleep(20);
- gpiod_set_value(ctx->reset, 0);
+ gpiod_set_value_cansleep(ctx->reset, 0);
msleep(20);
for (i = 0; i < ctx->desc->init_length; i++) {
@@ -1334,7 +1334,7 @@ static int ili9881c_unprepare(struct drm_panel *panel)
mipi_dsi_dcs_enter_sleep_mode(ctx->dsi);
regulator_disable(ctx->power);
- gpiod_set_value(ctx->reset, 1);
+ gpiod_set_value_cansleep(ctx->reset, 1);
return 0;
}
--
Regards,
Laurent Pinchart
Re: [PATCH v3 2/2] drm/bridge: adv7511: get edid in hpd_work to update CEC phys address
struct drm_connector *connector)
> {
> - struct edid *edid;
> -
> - /* Reading the EDID only works if the device is powered */
> - if (!adv7511->powered) {
> - unsigned int edid_i2c_addr =
> - (adv7511->i2c_edid->addr << 1);
> -
> - __adv7511_power_on(adv7511);
> -
> - /* Reset the EDID_I2C_ADDR register as it might be cleared */
> - regmap_write(adv7511->regmap, ADV7511_REG_EDID_I2C_ADDR,
> - edid_i2c_addr);
> - }
> -
> - edid = drm_do_get_edid(connector, adv7511_get_edid_block, adv7511);
> -
> - if (!adv7511->powered)
> - __adv7511_power_off(adv7511);
> -
> - adv7511_set_config_csc(adv7511, connector, adv7511->rgb,
> -drm_detect_hdmi_monitor(edid));
> -
> - cec_s_phys_addr_from_edid(adv7511->cec_adap, edid);
> -
> - return edid;
> + return __adv7511_get_edid(adv7511, connector);
> }
>
> static int adv7511_get_modes(struct adv7511 *adv7511,
>
--
Regards,
Laurent Pinchart
Re: [PATCH v3 1/2] drm/bridge: adv7511: rearrange hotplug work code
Hi Alvin,
Thank you for the patch.
On Mon, Feb 19, 2024 at 09:12:58PM +0100, Alvin Šipraga wrote:
> From: Alvin Šipraga
>
> In preparation for calling EDID helpers from the hotplug work, move the
> hotplug work below the EDID helper section. No functional change.
>
> Reviewed-by: Robert Foss
> Signed-off-by: Alvin Šipraga
Reviewed-by: Laurent Pinchart
> ---
> drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 120
> ++-
> 1 file changed, 62 insertions(+), 58 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> index 8be235144f6d..5ffc5904bd59 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> @@ -406,64 +406,6 @@ static void adv7511_power_off(struct adv7511 *adv7511)
> * Interrupt and hotplug detection
> */
>
> -static bool adv7511_hpd(struct adv7511 *adv7511)
> -{
> - unsigned int irq0;
> - int ret;
> -
> - ret = regmap_read(adv7511->regmap, ADV7511_REG_INT(0), );
> - if (ret < 0)
> - return false;
> -
> - if (irq0 & ADV7511_INT0_HPD) {
> - regmap_write(adv7511->regmap, ADV7511_REG_INT(0),
> - ADV7511_INT0_HPD);
> - return true;
> - }
> -
> - return false;
> -}
> -
> -static void adv7511_hpd_work(struct work_struct *work)
> -{
> - struct adv7511 *adv7511 = container_of(work, struct adv7511, hpd_work);
> - enum drm_connector_status status;
> - unsigned int val;
> - int ret;
> -
> - ret = regmap_read(adv7511->regmap, ADV7511_REG_STATUS, );
> - if (ret < 0)
> - status = connector_status_disconnected;
> - else if (val & ADV7511_STATUS_HPD)
> - status = connector_status_connected;
> - else
> - status = connector_status_disconnected;
> -
> - /*
> - * The bridge resets its registers on unplug. So when we get a plug
> - * event and we're already supposed to be powered, cycle the bridge to
> - * restore its state.
> - */
> - if (status == connector_status_connected &&
> - adv7511->connector.status == connector_status_disconnected &&
> - adv7511->powered) {
> - regcache_mark_dirty(adv7511->regmap);
> - adv7511_power_on(adv7511);
> - }
> -
> - if (adv7511->connector.status != status) {
> - adv7511->connector.status = status;
> -
> - if (adv7511->connector.dev) {
> - if (status == connector_status_disconnected)
> - cec_phys_addr_invalidate(adv7511->cec_adap);
> - drm_kms_helper_hotplug_event(adv7511->connector.dev);
> - } else {
> - drm_bridge_hpd_notify(>bridge, status);
> - }
> - }
> -}
> -
> static int adv7511_irq_process(struct adv7511 *adv7511, bool process_hpd)
> {
> unsigned int irq0, irq1;
> @@ -600,6 +542,68 @@ static int adv7511_get_edid_block(void *data, u8 *buf,
> unsigned int block,
> return 0;
> }
>
> +/*
> -
> + * Hotplug handling
> + */
> +
> +static bool adv7511_hpd(struct adv7511 *adv7511)
> +{
> + unsigned int irq0;
> + int ret;
> +
> + ret = regmap_read(adv7511->regmap, ADV7511_REG_INT(0), );
> + if (ret < 0)
> + return false;
> +
> + if (irq0 & ADV7511_INT0_HPD) {
> + regmap_write(adv7511->regmap, ADV7511_REG_INT(0),
> + ADV7511_INT0_HPD);
> + return true;
> + }
> +
> + return false;
> +}
> +
> +static void adv7511_hpd_work(struct work_struct *work)
> +{
> + struct adv7511 *adv7511 = container_of(work, struct adv7511, hpd_work);
> + enum drm_connector_status status;
> + unsigned int val;
> + int ret;
> +
> + ret = regmap_read(adv7511->regmap, ADV7511_REG_STATUS, );
> + if (ret < 0)
> + status = connector_status_disconnected;
> + else if (val & ADV7511_STATUS_HPD)
> + status = connector_status_connected;
> + else
> + status = connector_status_disconnected;
> +
> + /*
> + * The bridge resets its registers on unplug. So when we get a plug
> + * event and we're already supposed to be powered, cycle the bridge to
> + * restore its state.
> + */
> + if (status == connector_status_connected &&
> +
Re: [PATCH V2 1/2] drm/bridge: adv7511: Allow IRQ to share GPIO pins
Hello Adam, Thank you for the patch. On Mon, Mar 04, 2024 at 06:48:57PM -0600, Adam Ford wrote: > The IRQ registration currently assumes that the GPIO is dedicated > to it, but that may not necessarily be the case. If the board has > another device sharing the GPIO, it won't be registered and the > hot-plug detect fails to function. > > Currently, the handler reads two registers and blindly > assumes one of them caused the interrupt and returns IRQ_HANDLED > unless there is an error. In order to properly do this, the IRQ > handler needs to check if it needs to handle the IRQ and return > IRQ_NONE if there is nothing to handle. With the check added > and the return code properly indicating whether or not it there > was an IRQ, the IRQF_SHARED can be set to share a GPIO IRQ. > > Signed-off-by: Adam Ford Reviewed-by: Laurent Pinchart > --- > V2: Add check to see if there is IRQ data to handle > > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > index b5518ff97165..f3b4616a8fb6 100644 > --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > @@ -477,6 +477,11 @@ static int adv7511_irq_process(struct adv7511 *adv7511, > bool process_hpd) > if (ret < 0) > return ret; > > + /* If there is no IRQ to handle, exit indicating no IRQ data */ > + if (!(irq0 & (ADV7511_INT0_HPD | ADV7511_INT0_EDID_READY)) && > + !(irq1 & ADV7511_INT1_DDC_ERROR)) > + return -ENODATA; > + > regmap_write(adv7511->regmap, ADV7511_REG_INT(0), irq0); > regmap_write(adv7511->regmap, ADV7511_REG_INT(1), irq1); > > @@ -1318,7 +1323,8 @@ static int adv7511_probe(struct i2c_client *i2c) > > ret = devm_request_threaded_irq(dev, i2c->irq, NULL, > adv7511_irq_handler, > - IRQF_ONESHOT, dev_name(dev), > + IRQF_ONESHOT | IRQF_SHARED, > + dev_name(dev), > adv7511); > if (ret) > goto err_unregister_audio; -- Regards, Laurent Pinchart
Re: [PATCH 1/2] drm/bridge: adv7511: Allow IRQ to share GPIO pins
On Sun, Mar 03, 2024 at 09:44:03AM -0600, Adam Ford wrote:
> On Wed, Feb 28, 2024 at 10:31 AM Laurent Pinchart wrote:
> > On Wed, Feb 28, 2024 at 05:37:35AM -0600, Adam Ford wrote:
> > > The IRQ registration currently assumes that the GPIO is
> > > dedicated to it, but that may not necessarily be the case.
> > > If the board has another device sharing the IRQ, it won't be
> > > registered and the hot-plug detect fails. This is easily
> > > fixed by add the IRQF_SHARED flag.
> > >
> > > Signed-off-by: Adam Ford
> > >
> > > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> > > b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> > > index b5518ff97165..21f08b2ae265 100644
> > > --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> > > +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> > > @@ -1318,7 +1318,8 @@ static int adv7511_probe(struct i2c_client *i2c)
> > >
> > > ret = devm_request_threaded_irq(dev, i2c->irq, NULL,
> > > adv7511_irq_handler,
> > > - IRQF_ONESHOT, dev_name(dev),
> > > + IRQF_ONESHOT | IRQF_SHARED,
> > > + dev_name(dev),
> >
> > This looks fine, but the IRQ handler doesn't.
>
> Thanks for the review.
>
> > static int adv7511_irq_process(struct adv7511 *adv7511, bool process_hpd)
> > {
> > unsigned int irq0, irq1;
> > int ret;
> >
> > ret = regmap_read(adv7511->regmap, ADV7511_REG_INT(0), );
> > if (ret < 0)
> > return ret;
> >
> > ret = regmap_read(adv7511->regmap, ADV7511_REG_INT(1), );
> > if (ret < 0)
> > return ret;
>
> If I did a check here and returned if there was no IRQ to handle,
> would that be sufficient?
>
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> @@ -477,6 +477,11 @@ static int adv7511_irq_process(struct adv7511
> *adv7511, bool process_hpd)
> if (ret < 0)
> return ret;
>
> + /* If there is no IRQ to handle, exit indicating no IRQ handled */
> + if (!(irq0 & (ADV7511_INT0_HPD | ADV7511_INT0_EDID_READY)) &&
> + !(irq1 & ADV7511_INT1_DDC_ERROR))
If these are the only interrupt sources that the driver enables, this is
fine.
> + return -1;
Maybe a defined error code instead ?
> +
> regmap_write(adv7511->regmap, ADV7511_REG_INT(0), irq0);
> regmap_write(adv7511->regmap, ADV7511_REG_INT(1), irq1);
>
> With this, I would expect adv7511_irq_handler to return IRQ_NONE. If
> you're OK with that approach, I can do that. If you want me to merge
> adv7511_irq_handler, and adv7511_irq_process, I can do that too to
> make the return codes a little more intuitive.
>
> >
> > regmap_write(adv7511->regmap, ADV7511_REG_INT(0), irq0);
> > regmap_write(adv7511->regmap, ADV7511_REG_INT(1), irq1);
> >
> > if (process_hpd && irq0 & ADV7511_INT0_HPD &&
> > adv7511->bridge.encoder)
> > schedule_work(>hpd_work);
> >
> > if (irq0 & ADV7511_INT0_EDID_READY || irq1 &
> > ADV7511_INT1_DDC_ERROR) {
> > adv7511->edid_read = true;
> >
> > if (adv7511->i2c_main->irq)
> > wake_up_all(>wq);
> > }
> >
> > #ifdef CONFIG_DRM_I2C_ADV7511_CEC
> > adv7511_cec_irq_process(adv7511, irq1);
> > #endif
> >
> > return 0;
> > }
> >
> > static irqreturn_t adv7511_irq_handler(int irq, void *devid)
> > {
> > struct adv7511 *adv7511 = devid;
> > int ret;
> >
> > ret = adv7511_irq_process(adv7511, true);
> > return ret < 0 ? IRQ_NONE : IRQ_HANDLED;
> > }
> >
> > The function will return IRQ_HANDLED as long as the registers can be
> > read, even if they don't report any interrupt.
> >
> > > adv7511);
> > > if (ret)
> > > goto err_unregister_audio;
--
Regards,
Laurent Pinchart
Re: [PATCH v2 0/9] drm: Switch from dev_err to dev_err_probe for missing DSI host error path
On Fri, Mar 01, 2024 at 11:19:27AM -0500, Nícolas F. R. A. Prado wrote:
> On Fri, Mar 01, 2024 at 08:34:31AM +0200, Laurent Pinchart wrote:
> > Hi Nícolas,
> >
> > On Thu, Feb 29, 2024 at 07:12:06PM -0500, Nícolas F. R. A. Prado wrote:
> > > This series changes every occurence of the following pattern:
> > >
> > > dsi_host = of_find_mipi_dsi_host_by_node(dsi);
> > > if (!dsi_host) {
> > > dev_err(dev, "failed to find dsi host\n");
> > > return -EPROBE_DEFER;
> > > }
> > >
> > > into
> > >
> > > dsi_host = of_find_mipi_dsi_host_by_node(dsi);
> > > if (!dsi_host)
> > > return dev_err_probe(dev, -EPROBE_DEFER, "failed to find dsi
> > > host\n");
> > >
> > > This registers the defer probe reason (so it can later be printed by the
> > > driver core or checked on demand through the devices_deferred file in
> > > debugfs) and prevents errors to be spammed in the kernel log every time
> > > the driver retries to probe, unnecessarily alerting userspace about
> > > something that is a normal part of the boot process.
> >
> > The idea is good, but I have a small issue with patches 1/9 to 7/9. They
> > all patch a function that is called by the probe function. Calling
> > dev_err_probe() in such functions is error-prone. I had to manually
> > check when reviewing the patches that those functions were indeed called
> > at probe time, and not through other code paths, and I also had to check
> > that no callers were using dev_err_probe() in the error handling path,
> > as that would have overridden the error message.
> >
> > Would there be a way to move the dev_err_probe() to the top-level ? I
> > understand it's not always possible or convenient, but if it was doable
> > in at least some of the drivers, I think it would be better. I'll let
> > you be the judge.
>
> Hey Laurent, thanks for the review.
>
> I get where you're coming from, as I checked those things myself while writing
> the patch. That said, I don't think moving dev_err_probe() to the top-level
> is a
> good move for a few reasons:
> * Keeping the log message as close to the source of the error makes it more
> specific, and consequently, more useful.
> * The original code already returned -EPROBE_DEFER, implying the function is
> expected to be called only from the probe function.
>
> With those points in mind, the only way I see to guarantee
> dev_err_probe(...,-EPROBE_DEFER...) would only be called by probe, and that
> the
> reason wouldn't be overriden, would be to move the entire code path of that
> function that calls into dev_err_probe() up into the probe function. But if we
> adopt this pattern consistently across the drivers in the tree, I think it
> would
> drastically worsen readability and cancel out the benefits.
>
> IMO the way forward with the API we have, is to make use of warnings and
> static
> checkers to catch cases where dev_err_probe() is overriding a defer probe
> reason, and where it's called outside of the probe function scope.
>
> So I'm inclined to leave the patches as they are, but am happy to discuss this
> further or other ideas.
Thanks for checking and having taken the time to explain your rationale.
For the whole series,
Reviewed-by: Laurent Pinchart
--
Regards,
Laurent Pinchart
Re: [PATCH v2 9/9] drm/panel: truly-nt35597: Don't log an error when DSI host can't be found
On Fri, Mar 01, 2024 at 09:44:36AM +0100, AngeloGioacchino Del Regno wrote:
> Il 01/03/24 07:30, Laurent Pinchart ha scritto:
> > On Thu, Feb 29, 2024 at 07:12:15PM -0500, Nícolas F. R. A. Prado wrote:
> >> Given that failing to find a DSI host causes the driver to defer probe,
> >> make use of dev_err_probe() to log the reason. This makes the defer
> >> probe reason available and avoids alerting userspace about something
> >> that is not necessarily an error.
> >>
> >> Suggested-by: AngeloGioacchino Del Regno
> >>
> >> Signed-off-by: Nícolas F. R. A. Prado
> >> ---
> >> drivers/gpu/drm/panel/panel-truly-nt35597.c | 6 ++
> >> 1 file changed, 2 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/panel/panel-truly-nt35597.c
> >> b/drivers/gpu/drm/panel/panel-truly-nt35597.c
> >> index b73448cf349d..d447db912a61 100644
> >> --- a/drivers/gpu/drm/panel/panel-truly-nt35597.c
> >> +++ b/drivers/gpu/drm/panel/panel-truly-nt35597.c
> >> @@ -550,10 +550,8 @@ static int truly_nt35597_probe(struct mipi_dsi_device
> >> *dsi)
> >>
> >>dsi1_host = of_find_mipi_dsi_host_by_node(dsi1);
> >>of_node_put(dsi1);
> >> - if (!dsi1_host) {
> >> - dev_err(dev, "failed to find dsi host\n");
> >> - return -EPROBE_DEFER;
> >> - }
> >> + if (!dsi1_host)
> >> + return dev_err_probe(dev, -EPROBE_DEFER, "failed to find dsi
> >> host\n");
> >
> > return dev_err_probe(dev, -EPROBE_DEFER,
> > "failed to find dsi host\n");
> >
> > With this addressed,
>
> I disagree. That's 87 columns, and the 80-col rule is long gone.
It's still a maintainer's preference. I soft-enforce it in drivers I
maintain. In this case I'll let Neil decide, as he's listed as the
maintainer for drivers/gpu/drm/panel/.
> Reviewed-by: AngeloGioacchino Del Regno
>
>
> > Reviewed-by: Laurent Pinchart
> >
> >>
> >>/* register the second DSI device */
> >>dsi1_device = mipi_dsi_device_register_full(dsi1_host, );
--
Regards,
Laurent Pinchart
Re: [PATCH v2 0/9] drm: Switch from dev_err to dev_err_probe for missing DSI host error path
Hi Nícolas,
On Thu, Feb 29, 2024 at 07:12:06PM -0500, Nícolas F. R. A. Prado wrote:
> This series changes every occurence of the following pattern:
>
> dsi_host = of_find_mipi_dsi_host_by_node(dsi);
> if (!dsi_host) {
> dev_err(dev, "failed to find dsi host\n");
> return -EPROBE_DEFER;
> }
>
> into
>
> dsi_host = of_find_mipi_dsi_host_by_node(dsi);
> if (!dsi_host)
> return dev_err_probe(dev, -EPROBE_DEFER, "failed to find dsi
> host\n");
>
> This registers the defer probe reason (so it can later be printed by the
> driver core or checked on demand through the devices_deferred file in
> debugfs) and prevents errors to be spammed in the kernel log every time
> the driver retries to probe, unnecessarily alerting userspace about
> something that is a normal part of the boot process.
The idea is good, but I have a small issue with patches 1/9 to 7/9. They
all patch a function that is called by the probe function. Calling
dev_err_probe() in such functions is error-prone. I had to manually
check when reviewing the patches that those functions were indeed called
at probe time, and not through other code paths, and I also had to check
that no callers were using dev_err_probe() in the error handling path,
as that would have overridden the error message.
Would there be a way to move the dev_err_probe() to the top-level ? I
understand it's not always possible or convenient, but if it was doable
in at least some of the drivers, I think it would be better. I'll let
you be the judge.
> I have omitted a Fixes: tag in the last patch, for the truly-nt35597
> panel, because it predates the dev_err_probe() helper.
>
> Changes in v2:
> - Added patches 2 onwards to fix all occurences of this pattern instead
> of just for the anx7625 driver
> - Link to v1:
> https://lore.kernel.org/r/20240226-anx7625-defer-log-no-dsi-host-v1-1-242b1af31...@collabora.com
>
> ---
> Nícolas F. R. A. Prado (9):
> drm/bridge: anx7625: Don't log an error when DSI host can't be found
> drm/bridge: icn6211: Don't log an error when DSI host can't be found
> drm/bridge: lt8912b: Don't log an error when DSI host can't be found
> drm/bridge: lt9611: Don't log an error when DSI host can't be found
> drm/bridge: lt9611uxc: Don't log an error when DSI host can't be found
> drm/bridge: tc358775: Don't log an error when DSI host can't be found
> drm/bridge: dpc3433: Don't log an error when DSI host can't be found
> drm/panel: novatek-nt35950: Don't log an error when DSI host can't be
> found
> drm/panel: truly-nt35597: Don't log an error when DSI host can't be
> found
>
> drivers/gpu/drm/bridge/analogix/anx7625.c | 6 ++
> drivers/gpu/drm/bridge/chipone-icn6211.c | 6 ++
> drivers/gpu/drm/bridge/lontium-lt8912b.c | 6 ++
> drivers/gpu/drm/bridge/lontium-lt9611.c | 6 ++
> drivers/gpu/drm/bridge/lontium-lt9611uxc.c| 6 ++
> drivers/gpu/drm/bridge/tc358775.c | 6 ++
> drivers/gpu/drm/bridge/ti-dlpc3433.c | 17 +
> drivers/gpu/drm/panel/panel-novatek-nt35950.c | 6 ++
> drivers/gpu/drm/panel/panel-truly-nt35597.c | 6 ++
> 9 files changed, 25 insertions(+), 40 deletions(-)
> ---
> base-commit: 2ae0a045e6814c8c1d676d6153c605a65746aa29
> change-id: 20240226-anx7625-defer-log-no-dsi-host-c3f9ccbcb287
--
Regards,
Laurent Pinchart
Re: [PATCH v2 9/9] drm/panel: truly-nt35597: Don't log an error when DSI host can't be found
Hi Nícolas,
Thank you for the patch.
On Thu, Feb 29, 2024 at 07:12:15PM -0500, Nícolas F. R. A. Prado wrote:
> Given that failing to find a DSI host causes the driver to defer probe,
> make use of dev_err_probe() to log the reason. This makes the defer
> probe reason available and avoids alerting userspace about something
> that is not necessarily an error.
>
> Suggested-by: AngeloGioacchino Del Regno
>
> Signed-off-by: Nícolas F. R. A. Prado
> ---
> drivers/gpu/drm/panel/panel-truly-nt35597.c | 6 ++
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/panel/panel-truly-nt35597.c
> b/drivers/gpu/drm/panel/panel-truly-nt35597.c
> index b73448cf349d..d447db912a61 100644
> --- a/drivers/gpu/drm/panel/panel-truly-nt35597.c
> +++ b/drivers/gpu/drm/panel/panel-truly-nt35597.c
> @@ -550,10 +550,8 @@ static int truly_nt35597_probe(struct mipi_dsi_device
> *dsi)
>
> dsi1_host = of_find_mipi_dsi_host_by_node(dsi1);
> of_node_put(dsi1);
> - if (!dsi1_host) {
> - dev_err(dev, "failed to find dsi host\n");
> - return -EPROBE_DEFER;
> - }
> + if (!dsi1_host)
> + return dev_err_probe(dev, -EPROBE_DEFER, "failed to find dsi
> host\n");
return dev_err_probe(dev, -EPROBE_DEFER,
"failed to find dsi host\n");
With this addressed,
Reviewed-by: Laurent Pinchart
>
> /* register the second DSI device */
> dsi1_device = mipi_dsi_device_register_full(dsi1_host, );
>
--
Regards,
Laurent Pinchart
Re: [PATCH v2 8/9] drm/panel: novatek-nt35950: Don't log an error when DSI host can't be found
Hi Nicolas,
Thank you for the patch.
On Thu, Feb 29, 2024 at 07:12:14PM -0500, Nícolas F. R. A. Prado wrote:
> Given that failing to find a DSI host causes the driver to defer probe,
> make use of dev_err_probe() to log the reason. This makes the defer
> probe reason available and avoids alerting userspace about something
> that is not necessarily an error.
>
> Fixes: 623a3531e9cf ("drm/panel: Add driver for Novatek NT35950 DSI DriverIC
> panels")
> Suggested-by: AngeloGioacchino Del Regno
>
> Signed-off-by: Nícolas F. R. A. Prado
> ---
> drivers/gpu/drm/panel/panel-novatek-nt35950.c | 6 ++
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/panel/panel-novatek-nt35950.c
> b/drivers/gpu/drm/panel/panel-novatek-nt35950.c
> index 648ce9201426..028fdac293f7 100644
> --- a/drivers/gpu/drm/panel/panel-novatek-nt35950.c
> +++ b/drivers/gpu/drm/panel/panel-novatek-nt35950.c
> @@ -556,10 +556,8 @@ static int nt35950_probe(struct mipi_dsi_device *dsi)
> }
> dsi_r_host = of_find_mipi_dsi_host_by_node(dsi_r);
> of_node_put(dsi_r);
> - if (!dsi_r_host) {
> - dev_err(dev, "Cannot get secondary DSI host\n");
> - return -EPROBE_DEFER;
> - }
> + if (!dsi_r_host)
> + return dev_err_probe(dev, -EPROBE_DEFER, "Cannot get
> secondary DSI host\n");
return dev_err_probe(dev, -EPROBE_DEFER,
"Cannot get secondary DSI host\n");
With this,
Reviewed-by: Laurent Pinchart
>
> nt->dsi[1] = mipi_dsi_device_register_full(dsi_r_host, info);
> if (!nt->dsi[1]) {
>
--
Regards,
Laurent Pinchart
Re: [PATCH 0/3] kci-gitlab: Introducing GitLab-CI Pipeline for Kernel Testing
On Thu, Feb 29, 2024 at 02:20:41PM +0200, Laurent Pinchart wrote: > On Thu, Feb 29, 2024 at 12:53:38PM +0100, Guillaume Tucker wrote: > > On 29/02/2024 12:41, Mark Brown wrote: > > > On Thu, Feb 29, 2024 at 01:19:19PM +0200, Laurent Pinchart wrote: > > >> On Thu, Feb 29, 2024 at 01:10:16PM +0200, Nikolai Kondrashov wrote: > > > > > >>> Of course. You're also welcome to join the #kernelci channel on > > >>> libera.chat. > > > > > >> Isn't that a bit pointless if it's no the main IM channel ? > > > > > > It *was* the original channel and still gets some usage (mostly started > > > by me admittedly since I've never joined slack for a bunch of reasons > > > that make it hassle), IIRC the Slack was started because there were some > > > interns who had trouble figuring out IRC and intermittent connectivity > > > but people seem to have migrated. > > > > In fact it was initially created for the members of the Linux > > Foundation project only, which is why registration is moderated > > for emails that don't have a domain linked to a member (BTW not > > any Google account will just work e.g. @gmail.com is moderated, > > only @google.com for Google employees isn't). > > > > And yes IRC is the "least common denominator" chat platform. > > Maybe having a bridge between the main Slack channel and IRC > > would help. > > If the gitlab CI pipeline proposal wants to be considered for inclusion > in the kernel, I think it needs to switch to a free software solution > for its *main* communication channels. And to clarify, I didn't meant the kernel CI project, but only the gitlab CI pipeline for the Linux kernel project. I don't know how tightly integrated the two projects are though. -- Regards, Laurent Pinchart
Re: [PATCH 0/3] kci-gitlab: Introducing GitLab-CI Pipeline for Kernel Testing
On Thu, Feb 29, 2024 at 12:53:38PM +0100, Guillaume Tucker wrote: > On 29/02/2024 12:41, Mark Brown wrote: > > On Thu, Feb 29, 2024 at 01:19:19PM +0200, Laurent Pinchart wrote: > >> On Thu, Feb 29, 2024 at 01:10:16PM +0200, Nikolai Kondrashov wrote: > > > >>> Of course. You're also welcome to join the #kernelci channel on > >>> libera.chat. > > > >> Isn't that a bit pointless if it's no the main IM channel ? > > > > It *was* the original channel and still gets some usage (mostly started > > by me admittedly since I've never joined slack for a bunch of reasons > > that make it hassle), IIRC the Slack was started because there were some > > interns who had trouble figuring out IRC and intermittent connectivity > > but people seem to have migrated. > > In fact it was initially created for the members of the Linux > Foundation project only, which is why registration is moderated > for emails that don't have a domain linked to a member (BTW not > any Google account will just work e.g. @gmail.com is moderated, > only @google.com for Google employees isn't). > > And yes IRC is the "least common denominator" chat platform. > Maybe having a bridge between the main Slack channel and IRC > would help. If the gitlab CI pipeline proposal wants to be considered for inclusion in the kernel, I think it needs to switch to a free software solution for its *main* communication channels. -- Regards, Laurent Pinchart
Re: [PATCH 0/3] kci-gitlab: Introducing GitLab-CI Pipeline for Kernel Testing
On Thu, Feb 29, 2024 at 01:10:16PM +0200, Nikolai Kondrashov wrote: > On 2/29/24 11:34 AM, Laurent Pinchart wrote: > > On Thu, Feb 29, 2024 at 11:26:51AM +0200, Nikolai Kondrashov wrote: > >> On 2/29/24 01:07, Laurent Pinchart wrote: > >>> On Wed, Feb 28, 2024 at 07:55:24PM -0300, Helen Koike wrote: > >>>> **Join Our Slack Channel:** > >>>> We have a Slack channel, #gitlab-ci, on the KernelCI Slack instance > >>>> https://kernelci.slack.com/ . > >>>> Feel free to join and contribute to the conversation. The KernelCI team > >>>> has > >>>> weekly calls where we also discuss the GitLab-CI pipeline. > >>> > >>> Could we communicate using free software please ? Furthermore, it's not > >>> possible to create an account on that slack instance unless you have an > >>> e-mail address affiliated with a small number of companies > >>> (https://kernelci.slack.com/signup#/domain-signup). That's a big no-go > >>> for me. > >> > >> Yes, it's not ideal that we use closed-source software for communication, > >> but > >> FWIW I'd be happy to invite you there. Perhaps if you try logging in e.g. > >> with > >> a Google account, I'd be able to see and approve your attempt too. > > > > I don't use Google accounts to authenticate to third-party services, > > sorry. And in any case, that won't make slack free software. > > Of course. You're also welcome to join the #kernelci channel on libera.chat. Isn't that a bit pointless if it's no the main IM channel ? > It's much quieter there, though. -- Regards, Laurent Pinchart
Re: [RFC] drm/fourcc: Add RPI modifiers
On Thu, Feb 29, 2024 at 11:39:24AM +0100, Daniel Vetter wrote: > On Wed, Feb 28, 2024 at 01:13:45PM +0200, Laurent Pinchart wrote: > > On Wed, Feb 28, 2024 at 11:41:57AM +0100, Jacopo Mondi wrote: > > > On Tue, Feb 27, 2024 at 03:08:27PM +0200, Laurent Pinchart wrote: > > > > On Mon, Feb 26, 2024 at 04:46:24PM +0100, Daniel Vetter wrote: > > > > > On Mon, 26 Feb 2024 at 16:39, Jacopo Mondi wrote: > > > > > > > > > > > > Add modifiers for the Raspberry Pi PiSP compressed formats. > > > > > > > > > > > > The compressed formats are documented at: > > > > > > Documentation/userspace-api/media/v4l/pixfmt-pisp-comp-rggb.rst > > > > > > > > > > > > and in the PiSP datasheet: > > > > > > https://datasheets.raspberrypi.com/camera/raspberry-pi-image-signal-processor-specification.pdf > > > > > > > > > > > > Signed-off-by: Jacopo Mondi > > > > > > --- > > > > > > > > > > > > Background: > > > > > > --- > > > > > > > > > > > > The Raspberry Pi PiSP camera subsystem is on its way to upstream > > > > > > through the > > > > > > Video4Linux2 subsystem: > > > > > > https://patchwork.linuxtv.org/project/linux-media/list/?series=12310 > > > > > > > > > > > > The PiSP camera system is composed by a "Front End" and a "Back > > > > > > End". > > > > > > The FrontEnd part is a MIPI CSI-2 receiver that store frames to > > > > > > memory and > > > > > > produce statistics, and the BackEnd is a memory-to-memory ISP that > > > > > > converts > > > > > > images in a format usable by application. > > > > > > > > > > > > The "FrontEnd" is capable of encoding RAW Bayer images as received > > > > > > by the > > > > > > image sensor in a 'compressed' format defined by Raspberry Pi and > > > > > > fully > > > > > > documented in the PiSP manual: > > > > > > https://datasheets.raspberrypi.com/camera/raspberry-pi-image-signal-processor-specification.pdf > > > > > > > > > > > > The compression scheme is documented in the in-review patch series > > > > > > for the BE > > > > > > support at: > > > > > > https://patchwork.linuxtv.org/project/linux-media/patch/20240223163012.300763-7-jacopo.mo...@ideasonboard.com/ > > > > > > > > > > > > The "BackEnd" is capable of consuming images in the compressed > > > > > > format and > > > > > > optionally user application might want to inspect those images for > > > > > > debugging > > > > > > purposes. > > > > > > > > > > > > Why a DRM modifier > > > > > > -- > > > > > > > > > > > > The PiSP support is entirely implemented in libcamera, with the > > > > > > support of an > > > > > > hw-specific library called 'libpisp'. > > > > > > > > > > > > libcamera uses the fourcc codes defined by DRM to define its > > > > > > formats: > > > > > > https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/formats.yaml > > > > > > > > > > > > And to define a new libcamera format for the Raspberry Pi > > > > > > compressed ones we > > > > > > need to associate the above proposed modifiers with a RAW Bayer > > > > > > format > > > > > > identifier. > > > > > > > > > > > > In example: > > > > > > > > > > > > - RGGB16_PISP_COMP1: > > > > > > fourcc: DRM_FORMAT_SRGGB16 > > > > > > > > An "interesting" issue here is that these formats currently live in > > > > libcamera only, we haven't merged them in DRM "yet". This may be a > > > > prerequisite ? > > > > > > > > > > Ah right! I didn't notice! > > > > > > I think there are two issues at play here, one to be clarified by the > > > DRM maintainers, the other more technically involved with the > > > definition
Re: [PATCH 1/3] kci-gitlab: Introducing GitLab-CI Pipeline for Kernel Testing
On Thu, Feb 29, 2024 at 10:56:58AM +0100, Maxime Ripard wrote: > Hi! > > On Thu, Feb 29, 2024 at 11:23:22AM +0200, Nikolai Kondrashov wrote: > > Hi everyone, > > > > On 2/29/24 11:02, Maxime Ripard wrote: > > > On Wed, Feb 28, 2024 at 07:55:25PM -0300, Helen Koike wrote: > > > > Which rating would you select? > > > > > > 4.5 :) > > > > > > One thing I'm wondering here is how we're going to cope with the > > > different requirements each user / framework has. > > > > > > Like, Linus probably want to have a different set of CI before merging a > > > PR than (say) linux-next does, or stable, or before doing an actual > > > release. > > > > > > Similarly, DRM probably has a different set of requirements than > > > drm-misc, drm-amd or nouveau. > > > > > > I don't see how the current architecture could accomodate for that. I > > > know that Gitlab allows to store issues template in a separate repo, > > > maybe we could ask them to provide a feature where the actions would be > > > separate from the main repo? That way, any gitlab project could provide > > > its own set of tests, without conflicting with each others (and we could > > > still share them if we wanted to) > > > > > > I know some of use had good relationship with Gitlab, so maybe it would > > > be worth asking? > > > > GitLab already supports getting the CI YAML from other repos. You can change > > that in the repo settings. > > I'm interested but couldn't find it in the doc, do you have a link to > the right section? e.g. https://gitlab.freedesktop.org/camera/libcamera/-/settings/ci_cd Expand "General pipelines", the setting is "CI/CD configuration file". You can specify the path to a file in the local repository, or in a separate repository. See https://gitlab.freedesktop.org/help/ci/pipelines/settings#specify-a-custom-cicd-configuration-file for the syntax. > > However, I think a better approach would be *not* to add the .gitlab-ci.yaml > > file in the root of the source tree, but instead change the very same repo > > setting to point to a particular entry YAML, *inside* the repo (somewhere > > under "ci" directory) instead. > > > > This way all the different subtrees can have completely different setup, but > > some could still use Helen's work and employ the "scenarios" she > > implemented. > > I'm worried that this kind of setup will just create duplicated YAML > that will be developped in complete silos and will become difficult to > maintain. But that's definitely an opinion :) -- Regards, Laurent Pinchart
Re: [PATCH 0/3] kci-gitlab: Introducing GitLab-CI Pipeline for Kernel Testing
On Thu, Feb 29, 2024 at 11:26:51AM +0200, Nikolai Kondrashov wrote: > On 2/29/24 01:07, Laurent Pinchart wrote: > > On Wed, Feb 28, 2024 at 07:55:24PM -0300, Helen Koike wrote: > >> **Join Our Slack Channel:** > >> We have a Slack channel, #gitlab-ci, on the KernelCI Slack instance > >> https://kernelci.slack.com/ . > >> Feel free to join and contribute to the conversation. The KernelCI team has > >> weekly calls where we also discuss the GitLab-CI pipeline. > > > > Could we communicate using free software please ? Furthermore, it's not > > possible to create an account on that slack instance unless you have an > > e-mail address affiliated with a small number of companies > > (https://kernelci.slack.com/signup#/domain-signup). That's a big no-go > > for me. > > Yes, it's not ideal that we use closed-source software for communication, but > FWIW I'd be happy to invite you there. Perhaps if you try logging in e.g. > with > a Google account, I'd be able to see and approve your attempt too. I don't use Google accounts to authenticate to third-party services, sorry. And in any case, that won't make slack free software. > Otherwise, our video meetings are generally open to everyone and run in Jitsi. -- Regards, Laurent Pinchart
Re: [PATCH] drm: xlnx: dp: Reset DisplayPort IP
Tomi, could you push this through drm-misc ?
On Wed, Feb 28, 2024 at 06:22:25PM +0200, Laurent Pinchart wrote:
> Hello Rohit,
>
> Thank you for the patch.
>
> On Fri, Feb 16, 2024 at 04:40:43AM -0800, Rohit Visavalia wrote:
> > Assert DisplayPort reset signal before deasserting,
> > it is to clear out any registers programmed before booting kernel.
> >
> > Signed-off-by: Rohit Visavalia
> > ---
> > drivers/gpu/drm/xlnx/zynqmp_dp.c | 4
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c
> > b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> > index 1846c4971fd8..5a40aa1d4283 100644
> > --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
> > +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> > @@ -1714,6 +1714,10 @@ int zynqmp_dp_probe(struct zynqmp_dpsub *dpsub)
> > goto err_free;
> > }
> >
> > + ret = zynqmp_dp_reset(dp, true);
> > + if (ret < 0)
> > + return ret;
> > +
>
> This looks fine to me, so
>
> Reviewed-by: Laurent Pinchart
>
> However, looking at zynqmp_dp_reset(), we have
>
> static int zynqmp_dp_reset(struct zynqmp_dp *dp, bool assert)
> {
> unsigned long timeout;
>
> if (assert)
> reset_control_assert(dp->reset);
> else
> reset_control_deassert(dp->reset);
>
> /* Wait for the (de)assert to complete. */
> timeout = jiffies + msecs_to_jiffies(RST_TIMEOUT_MS);
> while (!time_after_eq(jiffies, timeout)) {
> bool status = !!reset_control_status(dp->reset);
>
> if (assert == status)
> return 0;
>
> cpu_relax();
> }
>
> dev_err(dp->dev, "reset %s timeout\n", assert ? "assert" : "deassert");
> return -ETIMEDOUT;
> }
>
> That doesn't seem quite right. Aren't reset_control_assert() and
> reset_control_deassert() supposed to be synchronous ? If so there should
> be no need to wait, and if there's a need to wait, it could be a bug in
> the reset controller driver. This should be fixed, and then the code in
> probe could be replaced with a call to reset_control_reset().
>
> > ret = zynqmp_dp_reset(dp, false);
> > if (ret < 0)
> > goto err_free;
--
Regards,
Laurent Pinchart
Re: [PATCH 0/3] kci-gitlab: Introducing GitLab-CI Pipeline for Kernel Testing
- Submit results to KCIDB > > **Lightweight Implementation for All Developers:** > We aim to design these tests to be lightweight, ensuring developers with > limited > computing resources can still run essential tests. Resource-intensive tests > can > be set to trigger manually, rather than automatically, to accommodate diverse > development environments. > > > Chat Discussions > > > For those interested in further discussions: > > **Join Our Slack Channel:** > We have a Slack channel, #gitlab-ci, on the KernelCI Slack instance > https://kernelci.slack.com/ . > Feel free to join and contribute to the conversation. The KernelCI team has > weekly calls where we also discuss the GitLab-CI pipeline. Could we communicate using free software please ? Furthermore, it's not possible to create an account on that slack instance unless you have an e-mail address affiliated with a small number of companies (https://kernelci.slack.com/signup#/domain-signup). That's a big no-go for me. > **Acknowledgments:** > A special thanks to Nikolai Kondrashov, Tales da Aparecida - both from Red > Hat - > and KernelCI community for their valuable feedback and support in this > proposal. > > > I eagerly await your thoughts and suggestions on this initiative. > > Also, if you want to see this initiave move faster, we are happy to discuss > funding options. > > Best regards, > Helen Koike > > Helen Koike (3): > kci-gitlab: Introducing GitLab-CI Pipeline for Kernel Testing > kci-gitlab: Add documentation > kci-gitlab: docs: Add images > > .gitlab-ci.yml| 2 + > Documentation/ci/gitlab-ci/gitlab-ci.rst | 404 ++ > .../ci/gitlab-ci/images/job-matrix.png| Bin 0 -> 159752 bytes > .../gitlab-ci/images/new-project-runner.png | Bin 0 -> 607737 bytes > .../ci/gitlab-ci/images/pipelines-on-push.png | Bin 0 -> 532143 bytes > .../ci/gitlab-ci/images/the-pipeline.png | Bin 0 -> 91675 bytes > .../ci/gitlab-ci/images/variables.png | Bin 0 -> 277518 bytes > Documentation/index.rst | 7 + > MAINTAINERS | 9 + > ci/gitlab-ci/bootstrap-gitlab-runner.sh | 55 +++ > ci/gitlab-ci/ci-scripts/build-docs.sh | 35 ++ > ci/gitlab-ci/ci-scripts/build-kernel.sh | 35 ++ > ci/gitlab-ci/ci-scripts/ici-functions.sh | 104 + > ci/gitlab-ci/ci-scripts/install-smatch.sh | 13 + > .../ci-scripts/parse_commit_message.sh| 27 ++ > ci/gitlab-ci/ci-scripts/run-checkpatch.sh | 19 + > ci/gitlab-ci/ci-scripts/run-smatch.sh | 45 ++ > ci/gitlab-ci/docker-compose.yaml | 18 + > ci/gitlab-ci/linux.code-workspace | 11 + > ci/gitlab-ci/yml/build.yml| 43 ++ > ci/gitlab-ci/yml/cache.yml| 26 ++ > ci/gitlab-ci/yml/container.yml| 36 ++ > ci/gitlab-ci/yml/gitlab-ci.yml| 71 +++ > ci/gitlab-ci/yml/kernel-combinations.yml | 18 + > ci/gitlab-ci/yml/scenarios.yml| 12 + > ci/gitlab-ci/yml/scenarios/file-systems.yml | 21 + > ci/gitlab-ci/yml/scenarios/media.yml | 21 + > ci/gitlab-ci/yml/scenarios/network.yml| 21 + > ci/gitlab-ci/yml/static-checks.yml| 21 + > 29 files changed, 1074 insertions(+) > create mode 100644 .gitlab-ci.yml > create mode 100644 Documentation/ci/gitlab-ci/gitlab-ci.rst > create mode 100644 Documentation/ci/gitlab-ci/images/job-matrix.png > create mode 100644 Documentation/ci/gitlab-ci/images/new-project-runner.png > create mode 100644 Documentation/ci/gitlab-ci/images/pipelines-on-push.png > create mode 100644 Documentation/ci/gitlab-ci/images/the-pipeline.png > create mode 100644 Documentation/ci/gitlab-ci/images/variables.png > create mode 100755 ci/gitlab-ci/bootstrap-gitlab-runner.sh > create mode 100755 ci/gitlab-ci/ci-scripts/build-docs.sh > create mode 100755 ci/gitlab-ci/ci-scripts/build-kernel.sh > create mode 100644 ci/gitlab-ci/ci-scripts/ici-functions.sh > create mode 100755 ci/gitlab-ci/ci-scripts/install-smatch.sh > create mode 100755 ci/gitlab-ci/ci-scripts/parse_commit_message.sh > create mode 100755 ci/gitlab-ci/ci-scripts/run-checkpatch.sh > create mode 100755 ci/gitlab-ci/ci-scripts/run-smatch.sh > create mode 100644 ci/gitlab-ci/docker-compose.yaml > create mode 100644 ci/gitlab-ci/linux.code-workspace > create mode 100644 ci/gitlab-ci/yml/build.yml > create mode 100644 ci/gitlab-ci/yml/cache.yml > create mode 100644 ci/gitlab-ci/yml/container.yml > create mode 100644 ci/gitlab-ci/yml/gitlab-ci.yml > create mode 100644 ci/gitlab-ci/yml/kernel-combinations.yml > create mode 100644 ci/gitlab-ci/yml/scenarios.yml > create mode 100644 ci/gitlab-ci/yml/scenarios/file-systems.yml > create mode 100644 ci/gitlab-ci/yml/scenarios/media.yml > create mode 100644 ci/gitlab-ci/yml/scenarios/network.yml > create mode 100644 ci/gitlab-ci/yml/static-checks.yml -- Regards, Laurent Pinchart
Re: [PATCH 2/2] arm64: dts: imx8mp-beacon-kit: Enable HDMI bridge HPD
Hi Adam,
Thank you for the patch.
On Wed, Feb 28, 2024 at 05:37:36AM -0600, Adam Ford wrote:
> The DSI to HDMI bridge supports hot-plut-detect, but the
> driver didn't previously support a shared IRQ GPIO. With
> the driver updated, the interrupt can be added to the bridge.
You can reflow the commit message to 72 columns.
> Signed-off-by: Adam Ford
Reviewed-by: Laurent Pinchart
> diff --git a/arch/arm64/boot/dts/freescale/imx8mp-beacon-kit.dts
> b/arch/arm64/boot/dts/freescale/imx8mp-beacon-kit.dts
> index 8de4dd268908..d854c94ec997 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mp-beacon-kit.dts
> +++ b/arch/arm64/boot/dts/freescale/imx8mp-beacon-kit.dts
> @@ -405,6 +405,8 @@ adv_bridge: hdmi@3d {
> compatible = "adi,adv7535";
> reg = <0x3d>, <0x3c>, <0x3e>, <0x3f>;
> reg-names = "main", "cec", "edid", "packet";
> + interrupt-parent = <>;
> + interrupts = <27 IRQ_TYPE_EDGE_FALLING>;
> adi,dsi-lanes = <4>;
> #sound-dai-cells = <0>;
>
--
Regards,
Laurent Pinchart
Re: [PATCH 1/2] drm/bridge: adv7511: Allow IRQ to share GPIO pins
Hi Adam,
Thank you for the patch.
On Wed, Feb 28, 2024 at 05:37:35AM -0600, Adam Ford wrote:
> The IRQ registration currently assumes that the GPIO is
> dedicated to it, but that may not necessarily be the case.
> If the board has another device sharing the IRQ, it won't be
> registered and the hot-plug detect fails. This is easily
> fixed by add the IRQF_SHARED flag.
>
> Signed-off-by: Adam Ford
>
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> index b5518ff97165..21f08b2ae265 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> @@ -1318,7 +1318,8 @@ static int adv7511_probe(struct i2c_client *i2c)
>
> ret = devm_request_threaded_irq(dev, i2c->irq, NULL,
> adv7511_irq_handler,
> - IRQF_ONESHOT, dev_name(dev),
> + IRQF_ONESHOT | IRQF_SHARED,
> + dev_name(dev),
This looks fine, but the IRQ handler doesn't.
static int adv7511_irq_process(struct adv7511 *adv7511, bool process_hpd)
{
unsigned int irq0, irq1;
int ret;
ret = regmap_read(adv7511->regmap, ADV7511_REG_INT(0), );
if (ret < 0)
return ret;
ret = regmap_read(adv7511->regmap, ADV7511_REG_INT(1), );
if (ret < 0)
return ret;
regmap_write(adv7511->regmap, ADV7511_REG_INT(0), irq0);
regmap_write(adv7511->regmap, ADV7511_REG_INT(1), irq1);
if (process_hpd && irq0 & ADV7511_INT0_HPD && adv7511->bridge.encoder)
schedule_work(>hpd_work);
if (irq0 & ADV7511_INT0_EDID_READY || irq1 & ADV7511_INT1_DDC_ERROR) {
adv7511->edid_read = true;
if (adv7511->i2c_main->irq)
wake_up_all(>wq);
}
#ifdef CONFIG_DRM_I2C_ADV7511_CEC
adv7511_cec_irq_process(adv7511, irq1);
#endif
return 0;
}
static irqreturn_t adv7511_irq_handler(int irq, void *devid)
{
struct adv7511 *adv7511 = devid;
int ret;
ret = adv7511_irq_process(adv7511, true);
return ret < 0 ? IRQ_NONE : IRQ_HANDLED;
}
The function will return IRQ_HANDLED as long as the registers can be
read, even if they don't report any interrupt.
> adv7511);
> if (ret)
> goto err_unregister_audio;
--
Regards,
Laurent Pinchart
Re: [PATCH] drm: xlnx: dp: Reset DisplayPort IP
Hello Rohit,
Thank you for the patch.
On Fri, Feb 16, 2024 at 04:40:43AM -0800, Rohit Visavalia wrote:
> Assert DisplayPort reset signal before deasserting,
> it is to clear out any registers programmed before booting kernel.
>
> Signed-off-by: Rohit Visavalia
> ---
> drivers/gpu/drm/xlnx/zynqmp_dp.c | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c
> b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> index 1846c4971fd8..5a40aa1d4283 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> @@ -1714,6 +1714,10 @@ int zynqmp_dp_probe(struct zynqmp_dpsub *dpsub)
> goto err_free;
> }
>
> + ret = zynqmp_dp_reset(dp, true);
> + if (ret < 0)
> + return ret;
> +
This looks fine to me, so
Reviewed-by: Laurent Pinchart
However, looking at zynqmp_dp_reset(), we have
static int zynqmp_dp_reset(struct zynqmp_dp *dp, bool assert)
{
unsigned long timeout;
if (assert)
reset_control_assert(dp->reset);
else
reset_control_deassert(dp->reset);
/* Wait for the (de)assert to complete. */
timeout = jiffies + msecs_to_jiffies(RST_TIMEOUT_MS);
while (!time_after_eq(jiffies, timeout)) {
bool status = !!reset_control_status(dp->reset);
if (assert == status)
return 0;
cpu_relax();
}
dev_err(dp->dev, "reset %s timeout\n", assert ? "assert" : "deassert");
return -ETIMEDOUT;
}
That doesn't seem quite right. Aren't reset_control_assert() and
reset_control_deassert() supposed to be synchronous ? If so there should
be no need to wait, and if there's a need to wait, it could be a bug in
the reset controller driver. This should be fixed, and then the code in
probe could be replaced with a call to reset_control_reset().
> ret = zynqmp_dp_reset(dp, false);
> if (ret < 0)
> goto err_free;
--
Regards,
Laurent Pinchart
Re: [PATCH 4/4] drm/atomic-helper: Add select_output_bus_format callback
On Wed, Feb 28, 2024 at 04:29:33PM +0100, Maxime Ripard wrote:
> On Mon, Feb 26, 2024 at 08:44:45PM -0800, Anatoliy Klymenko wrote:
> > Add select_output_bus_format to CRTC atomic helpers callbacks. This
> > callback Will allow CRTC to participate in media bus format negotiation
> > over connected DRM bridge chain and impose CRTC-specific restrictions.
> > A good example is CRTC implemented as FPGA soft IP. This kind of CRTC will
> > most certainly support a single output media bus format, as supporting
> > multiple runtime options consumes extra FPGA resources. A variety of
> > options for FPGA are usually achieved by synthesizing IP with different
> > parameters.
> >
> > Incorporate select_output_bus_format callback into the format negotiation
> > stage to fix the input bus format of the first DRM bridge in the chain.
> >
> > Signed-off-by: Anatoliy Klymenko
> > ---
> > drivers/gpu/drm/drm_bridge.c | 19 +--
> > include/drm/drm_modeset_helper_vtables.h | 31
> > +++
> > 2 files changed, 48 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> > index 521a71c61b16..453ae3d174b4 100644
> > --- a/drivers/gpu/drm/drm_bridge.c
> > +++ b/drivers/gpu/drm/drm_bridge.c
> > @@ -32,6 +32,7 @@
> > #include
> > #include
> > #include
> > +#include
> > #include
> > #include
> >
> > @@ -879,7 +880,8 @@ static int select_bus_fmt_recursive(struct drm_bridge
> > *first_bridge,
> > unsigned int i, num_in_bus_fmts = 0;
> > struct drm_bridge_state *cur_state;
> > struct drm_bridge *prev_bridge;
> > - u32 *in_bus_fmts;
> > + struct drm_crtc *crtc = crtc_state->crtc;
> > + u32 *in_bus_fmts, in_fmt;
> > int ret;
> >
> > prev_bridge = drm_bridge_get_prev_bridge(cur_bridge);
> > @@ -933,7 +935,20 @@ static int select_bus_fmt_recursive(struct drm_bridge
> > *first_bridge,
> > return -ENOMEM;
> >
> > if (first_bridge == cur_bridge) {
> > - cur_state->input_bus_cfg.format = in_bus_fmts[0];
> > + in_fmt = in_bus_fmts[0];
> > + if (crtc->helper_private &&
> > + crtc->helper_private->select_output_bus_format) {
> > + in_fmt = crtc->helper_private->select_output_bus_format(
> > + crtc,
> > + crtc->state,
> > + in_bus_fmts,
> > + num_in_bus_fmts);
> > + if (!in_fmt) {
> > + kfree(in_bus_fmts);
> > + return -ENOTSUPP;
> > + }
> > + }
> > + cur_state->input_bus_cfg.format = in_fmt;
>
> I don't think we should start poking at the CRTC internals, but we
> should rather provide a helper here.
It would probably look cleaner, yes.
> > cur_state->output_bus_cfg.format = out_bus_fmt;
> > kfree(in_bus_fmts);
> > return 0;
> > diff --git a/include/drm/drm_modeset_helper_vtables.h
> > b/include/drm/drm_modeset_helper_vtables.h
> > index 881b03e4dc28..7c21ae1fe3ad 100644
> > --- a/include/drm/drm_modeset_helper_vtables.h
> > +++ b/include/drm/drm_modeset_helper_vtables.h
> > @@ -489,6 +489,37 @@ struct drm_crtc_helper_funcs {
> > bool in_vblank_irq, int *vpos, int *hpos,
> > ktime_t *stime, ktime_t *etime,
> > const struct drm_display_mode *mode);
> > +
> > + /**
> > +* @select_output_bus_format
> > +*
> > +* Called by the first connected DRM bridge to negotiate input media
> > +* bus format. CRTC is expected to pick preferable media formats from
> > +* the list supported by the DRM bridge chain.
>
> There's nothing restricting it to bridges here. Please rephrase this to
> remove the bridge mention. The user is typically going to be the
> encoder, and bridges are just an automagic implementation of an encoder.
>
> And generally speaking, I'd really like to have an implementation
> available before merging this.
There's a downstream implementation in the Xilinx kernel, which would
indeed be nice to upstream. This shouldn't block patches 1/4 to 3/4
though, those can be merged once review comments are taken into account.
--
Regards,
Laurent Pinchart
Re: [PATCH 3/4] drm: xlnx: zynqmp_dpsub: Set input live format
a3935384834 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h
> +++ b/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h
> @@ -165,10 +165,10 @@
> #define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_100x2
> #define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_120x3
> #define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_MASK GENMASK(2, 0)
> -#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_RGB 0x0
> -#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV4440x1
> -#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV4220x2
> -#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YONLY 0x3
> +#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_RGB (0x0 << 4)
> +#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV444(0x1 << 4)
> +#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV422(0x2 << 4)
> +#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YONLY (0x3 << 4)
This change isn't even mentioned in the commit message. It should be
split to a separate patch.
> #define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_MASK GENMASK(5, 4)
> #define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_CB_FIRST BIT(8)
> #define ZYNQMP_DISP_AV_BUF_PALETTE_MEMORY0x400
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c
> b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> index 9cb7ac9f3097..0d5dffd20ad1 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> @@ -1281,7 +1281,8 @@ static void zynqmp_dp_disp_enable(struct zynqmp_dp *dp,
> {
> enum zynqmp_dpsub_layer_id layer_id;
> struct zynqmp_disp_layer *layer;
> - const struct drm_format_info *info;
> + struct drm_bridge_state *bridge_state;
> + u32 bus_fmt;
>
> if (dp->dpsub->connected_ports & BIT(ZYNQMP_DPSUB_PORT_LIVE_VIDEO))
> layer_id = ZYNQMP_DPSUB_LAYER_VID;
> @@ -1291,10 +1292,14 @@ static void zynqmp_dp_disp_enable(struct zynqmp_dp
> *dp,
> return;
>
> layer = dp->dpsub->layers[layer_id];
> + bridge_state =
> drm_atomic_get_new_bridge_state(old_bridge_state->base.state,
> +
> old_bridge_state->bridge);
> + if (bridge_state) {
> + bus_fmt = bridge_state->input_bus_cfg.format;
> + zynqmp_disp_layer_set_live_format(layer, bus_fmt);
> + } else
> + return;
if (!bridge_state)
return;
bus_fmt = bridge_state->input_bus_cfg.format;
zynqmp_disp_layer_set_live_format(layer, bus_fmt);
But more importantly, why would this fail ? If it does something is
seriously wrong and the display won't be working. I'd expect at least a
warning, but you should instead ensure it never fails.
>
> - /* TODO: Make the format configurable. */
> - info = drm_format_info(DRM_FORMAT_YUV422);
> - zynqmp_disp_layer_set_format(layer, info);
> zynqmp_disp_layer_enable(layer);
>
> if (layer_id == ZYNQMP_DPSUB_LAYER_GFX)
>
--
Regards,
Laurent Pinchart
Re: [PATCH 2/4] drm: xlnx: zynqmp_dpsub: Anounce supported input formats
rt that
accesses the formats array in zynqmp_disp.c.
> void zynqmp_disp_layer_enable(struct zynqmp_disp_layer *layer);
> void zynqmp_disp_layer_disable(struct zynqmp_disp_layer *layer);
> void zynqmp_disp_layer_set_format(struct zynqmp_disp_layer *layer,
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c
> b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> index 04b6bcac3b07..9cb7ac9f3097 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> @@ -1580,6 +1580,7 @@ static const struct drm_bridge_funcs
> zynqmp_dp_bridge_funcs = {
> .atomic_check = zynqmp_dp_bridge_atomic_check,
> .detect = zynqmp_dp_bridge_detect,
> .edid_read = zynqmp_dp_bridge_edid_read,
> + .atomic_get_input_bus_fmts = zynqmp_disp_get_input_bus_fmts,
> };
>
> /*
> -
--
Regards,
Laurent Pinchart
Re: [PATCH 1/4] drm: xlnx: zynqmp_dpsub: Set layer mode during creation
Hi Anatoliy,
Thank you for the patch.
On Mon, Feb 26, 2024 at 08:44:42PM -0800, Anatoliy Klymenko wrote:
> Set layer mode of operation (live or dma-based) during layer creation.
>
> Each DPSUB layer mode of operation is defined by corresponding DT node port
> connection, so it is possible to assign it during layer object creation.
> Previously it was set in layer enable functions, although it is too late
> as setting layer format depends on layer mode, and should be done before
> given layer enabled.
>
> Signed-off-by: Anatoliy Klymenko
> ---
> drivers/gpu/drm/xlnx/zynqmp_disp.c | 20
> drivers/gpu/drm/xlnx/zynqmp_disp.h | 13 +
> drivers/gpu/drm/xlnx/zynqmp_dp.c | 2 +-
> drivers/gpu/drm/xlnx/zynqmp_kms.c | 2 +-
> 4 files changed, 19 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c
> b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> index 8a39b3accce5..e6d26ef60e89 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> @@ -64,6 +64,16 @@
>
> #define ZYNQMP_DISP_MAX_NUM_SUB_PLANES 3
>
> +/**
> + * enum zynqmp_dpsub_layer_mode - Layer mode
> + * @ZYNQMP_DPSUB_LAYER_NONLIVE: non-live (memory) mode
> + * @ZYNQMP_DPSUB_LAYER_LIVE: live (stream) mode
> + */
> +enum zynqmp_dpsub_layer_mode {
> + ZYNQMP_DPSUB_LAYER_NONLIVE,
> + ZYNQMP_DPSUB_LAYER_LIVE,
> +};
> +
> /**
> * struct zynqmp_disp_format - Display subsystem format information
> * @drm_fmt: DRM format (4CC)
> @@ -902,15 +912,12 @@ u32 *zynqmp_disp_layer_drm_formats(struct
> zynqmp_disp_layer *layer,
> /**
> * zynqmp_disp_layer_enable - Enable a layer
> * @layer: The layer
> - * @mode: Operating mode of layer
> *
> * Enable the @layer in the audio/video buffer manager and the blender. DMA
> * channels are started separately by zynqmp_disp_layer_update().
> */
> -void zynqmp_disp_layer_enable(struct zynqmp_disp_layer *layer,
> - enum zynqmp_dpsub_layer_mode mode)
> +void zynqmp_disp_layer_enable(struct zynqmp_disp_layer *layer)
> {
> - layer->mode = mode;
> zynqmp_disp_avbuf_enable_video(layer->disp, layer);
> zynqmp_disp_blend_layer_enable(layer->disp, layer);
> }
> @@ -1134,6 +1141,11 @@ static int zynqmp_disp_create_layers(struct
> zynqmp_disp *disp)
> layer->id = i;
> layer->disp = disp;
> layer->info = _info[i];
> + /* For now assume dpsub works in either live or non-live mode
> for both layers.
> + * Hybrid mode is not supported yet.
> + */
/*
* For now assume dpsub works in either live or non-live mode
* for both layers. Hybrid mode is not supported yet.
*/
Sounds like a reasonable restriction for now.
Reviewed-by: Laurent Pinchart
> + layer->mode = disp->dpsub->dma_enabled ?
> ZYNQMP_DPSUB_LAYER_NONLIVE
> +:
> ZYNQMP_DPSUB_LAYER_LIVE;
>
> ret = zynqmp_disp_layer_request_dma(disp, layer);
> if (ret)
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.h
> b/drivers/gpu/drm/xlnx/zynqmp_disp.h
> index 123cffac08be..9b8b202224d9 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_disp.h
> +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.h
> @@ -42,16 +42,6 @@ enum zynqmp_dpsub_layer_id {
> ZYNQMP_DPSUB_LAYER_GFX,
> };
>
> -/**
> - * enum zynqmp_dpsub_layer_mode - Layer mode
> - * @ZYNQMP_DPSUB_LAYER_NONLIVE: non-live (memory) mode
> - * @ZYNQMP_DPSUB_LAYER_LIVE: live (stream) mode
> - */
> -enum zynqmp_dpsub_layer_mode {
> - ZYNQMP_DPSUB_LAYER_NONLIVE,
> - ZYNQMP_DPSUB_LAYER_LIVE,
> -};
> -
> void zynqmp_disp_enable(struct zynqmp_disp *disp);
> void zynqmp_disp_disable(struct zynqmp_disp *disp);
> int zynqmp_disp_setup_clock(struct zynqmp_disp *disp,
> @@ -62,8 +52,7 @@ void zynqmp_disp_blend_set_global_alpha(struct zynqmp_disp
> *disp,
>
> u32 *zynqmp_disp_layer_drm_formats(struct zynqmp_disp_layer *layer,
> unsigned int *num_formats);
> -void zynqmp_disp_layer_enable(struct zynqmp_disp_layer *layer,
> - enum zynqmp_dpsub_layer_mode mode);
> +void zynqmp_disp_layer_enable(struct zynqmp_disp_layer *layer);
> void zynqmp_disp_layer_disable(struct zynqmp_disp_layer *layer);
> void zynqmp_disp_layer_set_format(struct zynqmp_disp_layer *layer,
> const struct drm_format_info *info);
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c
> b/drivers/gpu/drm/xlnx/zynqmp_dp.c
>
Re: [RFC] drm/fourcc: Add RPI modifiers
On Wed, Feb 28, 2024 at 11:41:57AM +0100, Jacopo Mondi wrote: > On Tue, Feb 27, 2024 at 03:08:27PM +0200, Laurent Pinchart wrote: > > On Mon, Feb 26, 2024 at 04:46:24PM +0100, Daniel Vetter wrote: > > > On Mon, 26 Feb 2024 at 16:39, Jacopo Mondi wrote: > > > > > > > > Add modifiers for the Raspberry Pi PiSP compressed formats. > > > > > > > > The compressed formats are documented at: > > > > Documentation/userspace-api/media/v4l/pixfmt-pisp-comp-rggb.rst > > > > > > > > and in the PiSP datasheet: > > > > https://datasheets.raspberrypi.com/camera/raspberry-pi-image-signal-processor-specification.pdf > > > > > > > > Signed-off-by: Jacopo Mondi > > > > --- > > > > > > > > Background: > > > > --- > > > > > > > > The Raspberry Pi PiSP camera subsystem is on its way to upstream > > > > through the > > > > Video4Linux2 subsystem: > > > > https://patchwork.linuxtv.org/project/linux-media/list/?series=12310 > > > > > > > > The PiSP camera system is composed by a "Front End" and a "Back End". > > > > The FrontEnd part is a MIPI CSI-2 receiver that store frames to memory > > > > and > > > > produce statistics, and the BackEnd is a memory-to-memory ISP that > > > > converts > > > > images in a format usable by application. > > > > > > > > The "FrontEnd" is capable of encoding RAW Bayer images as received by > > > > the > > > > image sensor in a 'compressed' format defined by Raspberry Pi and fully > > > > documented in the PiSP manual: > > > > https://datasheets.raspberrypi.com/camera/raspberry-pi-image-signal-processor-specification.pdf > > > > > > > > The compression scheme is documented in the in-review patch series for > > > > the BE > > > > support at: > > > > https://patchwork.linuxtv.org/project/linux-media/patch/20240223163012.300763-7-jacopo.mo...@ideasonboard.com/ > > > > > > > > The "BackEnd" is capable of consuming images in the compressed format > > > > and > > > > optionally user application might want to inspect those images for > > > > debugging > > > > purposes. > > > > > > > > Why a DRM modifier > > > > -- > > > > > > > > The PiSP support is entirely implemented in libcamera, with the support > > > > of an > > > > hw-specific library called 'libpisp'. > > > > > > > > libcamera uses the fourcc codes defined by DRM to define its formats: > > > > https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/formats.yaml > > > > > > > > And to define a new libcamera format for the Raspberry Pi compressed > > > > ones we > > > > need to associate the above proposed modifiers with a RAW Bayer format > > > > identifier. > > > > > > > > In example: > > > > > > > > - RGGB16_PISP_COMP1: > > > > fourcc: DRM_FORMAT_SRGGB16 > > > > An "interesting" issue here is that these formats currently live in > > libcamera only, we haven't merged them in DRM "yet". This may be a > > prerequisite ? > > > > Ah right! I didn't notice! > > I think there are two issues at play here, one to be clarified by the > DRM maintainers, the other more technically involved with the > definition of the Bayer formats themselves. > > - Does DRM want RAW Bayer formats to be listed here, as these are not > typically 'graphic' formats. What's the DRM maintainers opinion here ? To give some context, the "historical mistake" I keep referring to regarding V4L2 is the decision to combine the bit depth of raw formats with the colour filter array (a.k.a. Bayer) pattern into a fourcc. I think we should have defined raw pixel formats that only encode a bit depth, and conveyed the CFA pattern out-of-band. This is especially the case for media bus codes (formats on the buses between hardware devices). The reasoning here is that most devices only care about the bit depth and not the CFA pattern. Adding a new CFA pattern (for instance for RGB+Ir) for a camera sensor would currently require adding a new media bus code (easy), using it in the sensor driver (obvious), and patching *every* CSI-2 receiver driver to pass it through. Userspace would similarly need to grow support for the new format, even for userspa
Re: [RFC] drm/fourcc: Add RPI modifiers
get that patch properly acked (by kernel and
> > libcamera folks) to record the community consensus.
>
> I see your point, but I wonder if libcamera actually need to be part
> of this list; we want in-kernel upstream driver and open-source
For the kernel side we'll use V4L2, so the DRM modifier won't have an
in-kernel user.
On the userspace side, yes, there will be an open-source user with
libcamera.
> userspace components. We allow binary 3A modules to be loaded by the
> library, but I'm not sure they will ever need to modify the DRM 4cc list.
>
> Anyway, with other libcamera people ack, I can certainly do so!
>
> > For the rpi modifiers themselves: They need to be properly documented,
> > least to exclude a screw-up like with the rpi modifiers we already
> > have, which unfortunately encode the buffer height (instead of just
> > the rounding algorithim to align the height to the right tile size) in
> > the modifiers, which breaks assumptions everywhere. For details see
> > https://gitlab.freedesktop.org/wlroots/wlroots/-/merge_requests/4529#note_2262057
>
> I see. The formats are fully documented in the above linked datasheet,
> and I've provided (with the help of RPi engineers, as I don't
> understand the compression algorithm part :) a shorter description as
> part of the V4L2 patch series that upstreams the BE driver
>
> https://patchwork.linuxtv.org/project/linux-media/patch/20240223163012.300763-7-jacopo.mo...@ideasonboard.com/
>
> The only indication about the buffer's layout I see is
>
> Each scanline is padded to a multiple of 8 pixels wide, and each block of 8
> horizontally-contiguous pixels is coded using 8 bytes.
>
> While the rest of the text describes the compression algorithm which
> (afaiu) doesn't affect the buffer geometry.
>
> Would you be ok with me replicating the above description (or maybe
> just reference the V4L2 documentation ?)
>
> > > ---
> > > include/uapi/drm/drm_fourcc.h | 5 +
> > > 1 file changed, 5 insertions(+)
> > >
> > > diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> > > index 00db00083175..09b182a959ad 100644
> > > --- a/include/uapi/drm/drm_fourcc.h
> > > +++ b/include/uapi/drm/drm_fourcc.h
> > > @@ -425,6 +425,7 @@ extern "C" {
> > > #define DRM_FORMAT_MOD_VENDOR_ARM 0x08
> > > #define DRM_FORMAT_MOD_VENDOR_ALLWINNER 0x09
> > > #define DRM_FORMAT_MOD_VENDOR_AMLOGIC 0x0a
> > > +#define DRM_FORMAT_MOD_VENDOR_RPI 0x0b
> > >
> > > /* add more to the end as needed */
> > >
> > > @@ -1568,6 +1569,10 @@ drm_fourcc_canonicalize_nvidia_format_mod(__u64
> > > modifier)
> > > #define AMD_FMT_MOD_CLEAR(field) \
> > > (~((__u64)AMD_FMT_MOD_##field##_MASK <<
> > > AMD_FMT_MOD_##field##_SHIFT))
> > >
> > > +/* RPI (Raspberry Pi) modifiers */
> > > +#define PISP_FORMAT_MOD_COMPRESS_MODE1 fourcc_mod_code(RPI, 1)
> > > +#define PISP_FORMAT_MOD_COMPRESS_MODE2 fourcc_mod_code(RPI, 2)
> > > +
> > > #if defined(__cplusplus)
> > > }
> > > #endif
--
Regards,
Laurent Pinchart
Re: [RFC] drm/fourcc: Add RPI modifiers
t;
> For the rpi modifiers themselves: They need to be properly documented,
> least to exclude a screw-up like with the rpi modifiers we already
> have, which unfortunately encode the buffer height (instead of just
> the rounding algorithim to align the height to the right tile size) in
> the modifiers, which breaks assumptions everywhere. For details see
> https://gitlab.freedesktop.org/wlroots/wlroots/-/merge_requests/4529#note_2262057
>
> > ---
> > include/uapi/drm/drm_fourcc.h | 5 +
> > 1 file changed, 5 insertions(+)
> >
> > diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> > index 00db00083175..09b182a959ad 100644
> > --- a/include/uapi/drm/drm_fourcc.h
> > +++ b/include/uapi/drm/drm_fourcc.h
> > @@ -425,6 +425,7 @@ extern "C" {
> > #define DRM_FORMAT_MOD_VENDOR_ARM 0x08
> > #define DRM_FORMAT_MOD_VENDOR_ALLWINNER 0x09
> > #define DRM_FORMAT_MOD_VENDOR_AMLOGIC 0x0a
> > +#define DRM_FORMAT_MOD_VENDOR_RPI 0x0b
> >
> > /* add more to the end as needed */
> >
> > @@ -1568,6 +1569,10 @@ drm_fourcc_canonicalize_nvidia_format_mod(__u64
> > modifier)
> > #define AMD_FMT_MOD_CLEAR(field) \
> > (~((__u64)AMD_FMT_MOD_##field##_MASK <<
> > AMD_FMT_MOD_##field##_SHIFT))
> >
> > +/* RPI (Raspberry Pi) modifiers */
> > +#define PISP_FORMAT_MOD_COMPRESS_MODE1 fourcc_mod_code(RPI, 1)
> > +#define PISP_FORMAT_MOD_COMPRESS_MODE2 fourcc_mod_code(RPI, 2)
> > +
> > #if defined(__cplusplus)
> > }
> > #endif
--
Regards,
Laurent Pinchart
Re: [PATCH V8 09/12] dt-bindings: display: imx: add binding for i.MX8MP HDMI TX
On Fri, Feb 16, 2024 at 10:05:26AM +0100, Alexander Stein wrote:
> Hi all,
>
> Am Samstag, 3. Februar 2024, 17:52:49 CET schrieb Adam Ford:
> > From: Lucas Stach
> >
> > The HDMI TX controller on the i.MX8MP SoC is a Synopsys designware IP
> > core with a little bit of SoC integration around it.
> >
> > Signed-off-by: Lucas Stach
> > Signed-off-by: Adam Ford
> >
> > ---
> > V3: Change name and location to better idenfity as a bridge and
> > HDMI 2.0a transmitter
> >
> > Fix typos and feedback from Rob and added ports.
> > ---
> > .../display/bridge/fsl,imx8mp-hdmi-tx.yaml| 102 ++
> > 1 file changed, 102 insertions(+)
> > create mode 100644
> > Documentation/devicetree/bindings/display/bridge/fsl,imx8mp-hdmi-tx.yaml
> >
> > diff --git
> > a/Documentation/devicetree/bindings/display/bridge/fsl,imx8mp-hdmi-tx.yaml
> > b/Documentation/devicetree/bindings/display/bridge/fsl,imx8mp-hdmi-tx.yaml
> > new file mode 100644
> > index ..3791c9f4ebab
> > --- /dev/null
> > +++
> > b/Documentation/devicetree/bindings/display/bridge/fsl,imx8mp-hdmi-tx.yaml
> > @@ -0,0 +1,102 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/display/bridge/fsl,imx8mp-hdmi-tx.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Freescale i.MX8MP DWC HDMI TX Encoder
> > +
> > +maintainers:
> > + - Lucas Stach
> > +
> > +description:
> > + The i.MX8MP HDMI transmitter is a Synopsys DesignWare
> > + HDMI 2.0a TX controller IP.
> > +
> > +allOf:
> > + - $ref: /schemas/display/bridge/synopsys,dw-hdmi.yaml#
> > +
> > +properties:
> > + compatible:
> > +enum:
> > + - fsl,imx8mp-hdmi-tx
> > +
> > + reg-io-width:
> > +const: 1
> > +
> > + clocks:
> > +maxItems: 4
> > +
> > + clock-names:
> > +items:
> > + - const: iahb
> > + - const: isfr
> > + - const: cec
> > + - const: pix
> > +
> > + power-domains:
> > +maxItems: 1
> > +
> > + ports:
> > +$ref: /schemas/graph.yaml#/properties/ports
> > +
> > +properties:
> > + port@0:
> > +$ref: /schemas/graph.yaml#/properties/port
> > +description: Parallel RGB input port
> > +
> > + port@1:
> > +$ref: /schemas/graph.yaml#/properties/port
> > +description: HDMI output port
> > +
> > +required:
> > + - port@0
> > + - port@1
>
> Is this really correct that port@1 is required? AFAICS this host already
> supports HPD and DDC by itself, so there is no need for a dedicated HDMI
> connector.
The chip has an HDMI output, so there's an output port.
> With the current state of the drivers this output port is completely ignored
> anyway. Yet it works for a lot of people.
DT bindings describe the hardware. From a DT point of view, tt's fine
for drivers to ignore the port (that may or may not be true from a DRM
point of view, but that's a separate discussion).
> > +
> > +required:
> > + - compatible
> > + - reg
> > + - clocks
> > + - clock-names
> > + - interrupts
> > + - power-domains
> > + - ports
> > +
> > +unevaluatedProperties: false
> > +
> > +examples:
> > + - |
> > +#include
> > +#include
> > +#include
> > +
> > +hdmi@32fd8000 {
> > +compatible = "fsl,imx8mp-hdmi-tx";
> > +reg = <0x32fd8000 0x7eff>;
> > +interrupts = <0 IRQ_TYPE_LEVEL_HIGH>;
> > +clocks = < IMX8MP_CLK_HDMI_APB>,
> > + < IMX8MP_CLK_HDMI_REF_266M>,
> > + < IMX8MP_CLK_32K>,
> > + <_tx_phy>;
> > +clock-names = "iahb", "isfr", "cec", "pix";
> > +power-domains = <_blk_ctrl IMX8MP_HDMIBLK_PD_HDMI_TX>;
> > +reg-io-width = <1>;
> > +ports {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > + port@0 {
> > + reg = <0>;
> > +
> > + hdmi_tx_from_pvi: endpoint {
> > + remote-endpoint = <_to_hdmi_tx>;
> > + };
> > + };
> > +
> > + port@1 {
> > +reg = <1>;
> > + hdmi_tx_out: endpoint {
> > +remote-endpoint = <_con>;
> > + };
> > + };
> > +};
> > +};
--
Regards,
Laurent Pinchart
Re: [PATCH 3/4] media: platform: replace of_graph_get_next_endpoint()
On Tue, Feb 06, 2024 at 11:45:58PM +, Kuninori Morimoto wrote: > > Hi Laurent > > Thank you for your review > > > > diff --git a/drivers/media/platform/samsung/exynos4-is/mipi-csis.c > > > b/drivers/media/platform/samsung/exynos4-is/mipi-csis.c > > > index 686ca8753ba2..3f8bea2e3934 100644 > > > --- a/drivers/media/platform/samsung/exynos4-is/mipi-csis.c > > > +++ b/drivers/media/platform/samsung/exynos4-is/mipi-csis.c > > > @@ -728,7 +728,7 @@ static int s5pcsis_parse_dt(struct platform_device > > > *pdev, > > >>max_num_lanes)) > > > return -EINVAL; > > > > > > - node = of_graph_get_next_endpoint(node, NULL); > > > + node = of_graph_get_endpoint_by_regs(node, 0, -1); > > > > This is not correct, see > > Documentation/devicetree/bindings/media/samsung,exynos4210-csis.yaml. > > Hmm... Then, It can be like this ? > > + node = of_graph_get_endpoint_by_regs(node, -1, -1); I suppose that would work, even if we should really try not to pass -1 for the port. Rob, any opinion ? -- Regards, Laurent Pinchart
Re: [PATCH 2/4] media: i2c: replace of_graph_get_next_endpoint()
On Wed, Feb 07, 2024 at 02:13:05PM +0100, Krzysztof Hałasa wrote:
> Laurent,
>
> Laurent Pinchart writes:
>
> >> +++ b/drivers/media/i2c/adv7604.c
> >> @@ -3205,7 +3205,7 @@ static int adv76xx_parse_dt(struct adv76xx_state
> >> *state)
> >> np = state->i2c_clients[ADV76XX_PAGE_IO]->dev.of_node;
> >>
> >> /* Parse the endpoint. */
> >> - endpoint = of_graph_get_next_endpoint(np, NULL);
> >> + endpoint = of_graph_get_endpoint_by_regs(np, 0, -1);
> >
> > I think this should be port 1 for the adv7611 and port2 for the adv7612.
> > The adv7610 may need to use port 1 too, but the bindings likely need to
> > be updated.
>
> To be honest I have no idea about ADV7611 and 7612.
> The 7610 I have on Tinyrex "mobo" seems to be single port.
One issue is that the commit that added the adv7610 compatible string to
the DT bindings (commit 901a52c43359, "media: Add ADV7610 support for
adv7604 driver - DT docs.") didn't extend the conditional rules further
down in the file that defines what ports are needed. That's why I don't
know what was intended. I believe the adv7610 should be treated like the
adv7611, given that the driver has
{ .compatible = "adi,adv7610", .data = _chip_info[ADV7611] },
{ .compatible = "adi,adv7611", .data = _chip_info[ADV7611] },
Could you send a patch to address that in the DT bindings ?
> ADV7611 seems to be mostly a 7610 in a different package (LQFP 64
> instead of some BGA 76). The driver simply treats ADV7610 as a 7611.
>
> ADV7612 is apparently dual port (only one port can be used at a time)
> though:
>
> [ADV7612] = {
> .type = ADV7612,
> .has_afe = false,
> .max_port = ADV76XX_PAD_HDMI_PORT_A,/* B not supported */
> .num_dv_ports = 1, /* normally 2 */
>
>
> All related in-tree DTS entries (as of v6.8.0-rc1) seem to be ADV7612.
>
> To me it seems all known devices use the first port only.
--
Regards,
Laurent Pinchart
Re: [PATCH 2/4] media: i2c: replace of_graph_get_next_endpoint()
On Wed, Feb 07, 2024 at 02:14:33PM +0100, Krzysztof Hałasa wrote: > Hans, > > Hans Verkuil writes: > > > Ideally someone would have to actually test this, perhaps with one of those > > Renesas boards. While I do have one, it got bricked after I attempted a > > u-boot update :-( > > May be reversible, though. Maybe Morimoto-san could help ? :-) What board did you use Hans ? -- Regards, Laurent Pinchart
Re: [PATCH 4/4] video: fbdev: replace of_graph_get_next_endpoint()
Hi Morimoto-san,
Thank you for the patch.
On Tue, Feb 06, 2024 at 02:55:45AM +, Kuninori Morimoto wrote:
> From DT point of view, in general, drivers should be asking for a
> specific port number because their function is fixed in the binding.
>
> of_graph_get_next_endpoint() doesn't match to this concept.
>
> Simply replace
>
> - of_graph_get_next_endpoint(xxx, NULL);
> + of_graph_get_endpoint_by_regs(xxx, 0, -1);
>
> Link: https://lore.kernel.org/r/20240202174941.ga310089-r...@kernel.org
> Signed-off-by: Kuninori Morimoto
> ---
> drivers/video/fbdev/amba-clcd.c | 2 +-
> drivers/video/fbdev/omap2/omapfb/dss/dsi.c| 3 ++-
> drivers/video/fbdev/omap2/omapfb/dss/dss-of.c | 20 +--
> drivers/video/fbdev/omap2/omapfb/dss/hdmi4.c | 3 ++-
> drivers/video/fbdev/omap2/omapfb/dss/hdmi5.c | 3 ++-
> drivers/video/fbdev/omap2/omapfb/dss/venc.c | 3 ++-
> drivers/video/fbdev/pxafb.c | 2 +-
> include/video/omapfb_dss.h| 3 ---
> 8 files changed, 11 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/video/fbdev/amba-clcd.c b/drivers/video/fbdev/amba-clcd.c
> index 0399db369e70..2371b204cfd2 100644
> --- a/drivers/video/fbdev/amba-clcd.c
> +++ b/drivers/video/fbdev/amba-clcd.c
This driver has been deleted in v6.8-rc1.
> @@ -691,7 +691,7 @@ static int clcdfb_of_init_display(struct clcd_fb *fb)
> /*
>* Fetch the panel endpoint.
>*/
> - endpoint = of_graph_get_next_endpoint(fb->dev->dev.of_node, NULL);
> + endpoint = of_graph_get_endpoint_by_regs(fb->dev->dev.of_node, 0, -1);
> if (!endpoint)
> return -ENODEV;
>
> diff --git a/drivers/video/fbdev/omap2/omapfb/dss/dsi.c
> b/drivers/video/fbdev/omap2/omapfb/dss/dsi.c
> index b7eb17a16ec4..1f13bcf73da5 100644
> --- a/drivers/video/fbdev/omap2/omapfb/dss/dsi.c
> +++ b/drivers/video/fbdev/omap2/omapfb/dss/dsi.c
> @@ -28,6 +28,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
>
> @@ -5079,7 +5080,7 @@ static int dsi_probe_of(struct platform_device *pdev)
> struct device_node *ep;
> struct omap_dsi_pin_config pin_cfg;
>
> - ep = omapdss_of_get_first_endpoint(node);
> + ep = of_graph_get_endpoint_by_regs(node, 0, -1);
> if (!ep)
> return 0;
>
> diff --git a/drivers/video/fbdev/omap2/omapfb/dss/dss-of.c
> b/drivers/video/fbdev/omap2/omapfb/dss/dss-of.c
> index 0282d4eef139..14965a3fd05b 100644
> --- a/drivers/video/fbdev/omap2/omapfb/dss/dss-of.c
> +++ b/drivers/video/fbdev/omap2/omapfb/dss/dss-of.c
> @@ -130,24 +130,6 @@ static struct device_node
> *omapdss_of_get_remote_port(const struct device_node *
> return np;
> }
>
> -struct device_node *
> -omapdss_of_get_first_endpoint(const struct device_node *parent)
> -{
> - struct device_node *port, *ep;
> -
> - port = omapdss_of_get_next_port(parent, NULL);
> -
> - if (!port)
> - return NULL;
> -
> - ep = omapdss_of_get_next_endpoint(port, NULL);
> -
> - of_node_put(port);
> -
> - return ep;
> -}
> -EXPORT_SYMBOL_GPL(omapdss_of_get_first_endpoint);
> -
I *think* replacing omapdss_of_get_first_endpoint() with
of_graph_get_endpoint_by_regs(0, -1) is functionally equivalent in all
cases, but a confirmation from Tomi would be nice. I wonder if it
wouldn't be time to drop the fbdev driver though...
Reviewed-by: Laurent Pinchart
> struct omap_dss_device *
> omapdss_of_find_source_for_first_ep(struct device_node *node)
> {
> @@ -155,7 +137,7 @@ omapdss_of_find_source_for_first_ep(struct device_node
> *node)
> struct device_node *src_port;
> struct omap_dss_device *src;
>
> - ep = omapdss_of_get_first_endpoint(node);
> + ep = of_graph_get_endpoint_by_regs(node, 0, -1);
> if (!ep)
> return ERR_PTR(-EINVAL);
>
> diff --git a/drivers/video/fbdev/omap2/omapfb/dss/hdmi4.c
> b/drivers/video/fbdev/omap2/omapfb/dss/hdmi4.c
> index f05b4e35a842..8f407ec134dc 100644
> --- a/drivers/video/fbdev/omap2/omapfb/dss/hdmi4.c
> +++ b/drivers/video/fbdev/omap2/omapfb/dss/hdmi4.c
> @@ -20,6 +20,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -529,7 +530,7 @@ static int hdmi_probe_of(struct platform_device *pdev)
> struct device_node *ep;
> int r;
>
> - ep = omapdss_of_get_first_endpoint(node);
> + ep = of_graph_get_endpoint_by_regs(node, 0, -1);
> if (!ep)
> return 0;
>
> diff --git a/drivers/video/fbdev/omap2/omapfb/dss/hdmi5.c
> b/drivers/video/
Re: [PATCH 3/4] media: platform: replace of_graph_get_next_endpoint()
> @@ -1915,7 +1915,7 @@ static int dcmi_probe(struct platform_device *pdev)
>"Could not get reset control\n");
>
> /* Get bus characteristics from devicetree */
> - np = of_graph_get_next_endpoint(np, NULL);
> + np = of_graph_get_endpoint_by_regs(np, 0, -1);
> if (!np) {
> dev_err(>dev, "Could not find the endpoint\n");
> return -ENODEV;
> diff --git a/drivers/media/platform/ti/davinci/vpif.c
> b/drivers/media/platform/ti/davinci/vpif.c
> index 63cdfed37bc9..f4e1fa76bf37 100644
> --- a/drivers/media/platform/ti/davinci/vpif.c
> +++ b/drivers/media/platform/ti/davinci/vpif.c
> @@ -465,8 +465,7 @@ static int vpif_probe(struct platform_device *pdev)
>* so their devices need to be registered manually here
>* for their legacy platform_drivers to work.
>*/
> - endpoint = of_graph_get_next_endpoint(pdev->dev.of_node,
> - endpoint);
> + endpoint = of_graph_get_endpoint_by_regs(pdev->dev.of_node, 0, -1);
> if (!endpoint)
> return 0;
> of_node_put(endpoint);
--
Regards,
Laurent Pinchart
Re: [PATCH 2/4] media: i2c: replace of_graph_get_next_endpoint()
; - endpoint = of_graph_get_next_endpoint(dev->of_node, NULL);
> + endpoint = of_graph_get_endpoint_by_regs(dev->of_node, 0, -1);
> if (!endpoint) {
> dev_err(dev, "endpoint node not found\n");
> return -EINVAL;
> diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
> index dcfe3129c63a..beab2813c672 100644
> --- a/drivers/media/i2c/ov5647.c
> +++ b/drivers/media/i2c/ov5647.c
> @@ -1363,7 +1363,7 @@ static int ov5647_parse_dt(struct ov5647 *sensor,
> struct device_node *np)
> struct device_node *ep;
> int ret;
>
> - ep = of_graph_get_next_endpoint(np, NULL);
> + ep = of_graph_get_endpoint_by_regs(np, 0, -1);
> if (!ep)
> return -EINVAL;
>
> diff --git a/drivers/media/i2c/s5c73m3/s5c73m3-core.c
> b/drivers/media/i2c/s5c73m3/s5c73m3-core.c
> index ed5b10731a14..aaec5f64f31a 100644
> --- a/drivers/media/i2c/s5c73m3/s5c73m3-core.c
> +++ b/drivers/media/i2c/s5c73m3/s5c73m3-core.c
> @@ -1555,7 +1555,7 @@ static int s5c73m3_get_dt_data(struct s5c73m3 *state)
>"failed to request gpio S5C73M3_RST\n");
> gpiod_set_consumer_name(state->reset, "S5C73M3_RST");
>
> - node_ep = of_graph_get_next_endpoint(node, NULL);
> + node_ep = of_graph_get_endpoint_by_regs(node, 0, -1);
> if (!node_ep) {
> dev_warn(dev, "no endpoint defined for node: %pOF\n", node);
> return 0;
> diff --git a/drivers/media/i2c/s5k5baf.c b/drivers/media/i2c/s5k5baf.c
> index 67da2045f543..af7e49e6cc5b 100644
> --- a/drivers/media/i2c/s5k5baf.c
> +++ b/drivers/media/i2c/s5k5baf.c
> @@ -1836,7 +1836,7 @@ static int s5k5baf_parse_device_node(struct s5k5baf
> *state, struct device *dev)
>state->mclk_frequency);
> }
>
> - node_ep = of_graph_get_next_endpoint(node, NULL);
> + node_ep = of_graph_get_endpoint_by_regs(node, 0, -1);
> if (!node_ep) {
> dev_err(dev, "no endpoint defined at node %pOF\n", node);
> return -EINVAL;
> diff --git a/drivers/media/i2c/tc358743.c b/drivers/media/i2c/tc358743.c
> index 2785935da497..872e802cdfbe 100644
> --- a/drivers/media/i2c/tc358743.c
> +++ b/drivers/media/i2c/tc358743.c
> @@ -1895,7 +1895,7 @@ static int tc358743_probe_of(struct tc358743_state
> *state)
> return dev_err_probe(dev, PTR_ERR(refclk),
>"failed to get refclk\n");
>
> - ep = of_graph_get_next_endpoint(dev->of_node, NULL);
> + ep = of_graph_get_endpoint_by_regs(dev->of_node, 0, -1);
> if (!ep) {
> dev_err(dev, "missing endpoint node\n");
> return -EINVAL;
> diff --git a/drivers/media/i2c/tda1997x.c b/drivers/media/i2c/tda1997x.c
> index 325e99125941..662efd5e69b9 100644
> --- a/drivers/media/i2c/tda1997x.c
> +++ b/drivers/media/i2c/tda1997x.c
> @@ -2307,7 +2307,7 @@ static int tda1997x_parse_dt(struct tda1997x_state
> *state)
> pdata->vidout_sel_de = DE_FREF_SEL_DE_VHREF;
>
> np = state->client->dev.of_node;
> - ep = of_graph_get_next_endpoint(np, NULL);
> + ep = of_graph_get_endpoint_by_regs(np, 0, -1);
> if (!ep)
> return -EINVAL;
>
> diff --git a/drivers/media/i2c/tvp514x.c b/drivers/media/i2c/tvp514x.c
> index c37f605cb75f..b1630a6c396b 100644
> --- a/drivers/media/i2c/tvp514x.c
> +++ b/drivers/media/i2c/tvp514x.c
> @@ -988,7 +988,7 @@ tvp514x_get_pdata(struct i2c_client *client)
> if (!IS_ENABLED(CONFIG_OF) || !client->dev.of_node)
> return client->dev.platform_data;
>
> - endpoint = of_graph_get_next_endpoint(client->dev.of_node, NULL);
> + endpoint = of_graph_get_endpoint_by_regs(client->dev.of_node, 0, -1);
> if (!endpoint)
> return NULL;
>
> diff --git a/drivers/media/i2c/tvp7002.c b/drivers/media/i2c/tvp7002.c
> index a2d7bc799849..8e58ce400df2 100644
> --- a/drivers/media/i2c/tvp7002.c
> +++ b/drivers/media/i2c/tvp7002.c
> @@ -893,7 +893,7 @@ tvp7002_get_pdata(struct i2c_client *client)
> if (!IS_ENABLED(CONFIG_OF) || !client->dev.of_node)
> return client->dev.platform_data;
>
> - endpoint = of_graph_get_next_endpoint(client->dev.of_node, NULL);
> + endpoint = of_graph_get_endpoint_by_regs(client->dev.of_node, 0, -1);
> if (!endpoint)
> return NULL;
>
--
Regards,
Laurent Pinchart
Re: [PATCH 1/4] gpu: drm: replace of_graph_get_next_endpoint()
Hello Morimoto-san,
Thank you for the patch.
On Tue, Feb 06, 2024 at 02:55:01AM +, Kuninori Morimoto wrote:
> From DT point of view, in general, drivers should be asking for a
> specific port number because their function is fixed in the binding.
>
> of_graph_get_next_endpoint() doesn't match to this concept.
>
> Simply replace
>
> - of_graph_get_next_endpoint(xxx, NULL);
> + of_graph_get_endpoint_by_regs(xxx, 0, -1);
>
> Link: https://lore.kernel.org/r/20240202174941.ga310089-r...@kernel.org
> Signed-off-by: Kuninori Morimoto
> ---
> drivers/gpu/drm/drm_of.c | 2 +-
> drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c | 2 +-
> drivers/gpu/drm/tiny/arcpgu.c | 2 +-
> 3 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
> index 177b600895d3..c2eae9296012 100644
> --- a/drivers/gpu/drm/drm_of.c
> +++ b/drivers/gpu/drm/drm_of.c
> @@ -516,7 +516,7 @@ struct mipi_dsi_host *drm_of_get_dsi_bus(struct device
> *dev)
> /*
>* Get first endpoint child from device.
>*/
> - endpoint = of_graph_get_next_endpoint(dev->of_node, NULL);
> + endpoint = of_graph_get_endpoint_by_regs(dev->of_node, 0, -1);
This assumes that the DSI device's port@0 will also be the input. That's
fine for current users of this function, but we should at least document
it.
diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
index 177b600895d3..012c4d04cf51 100644
--- a/drivers/gpu/drm/drm_of.c
+++ b/drivers/gpu/drm/drm_of.c
@@ -504,6 +504,8 @@ EXPORT_SYMBOL_GPL(drm_of_get_data_lanes_count_ep);
* Gets parent DSI bus for a DSI device controlled through a bus other
* than MIPI-DCS (SPI, I2C, etc.) using the Device Tree.
*
+ * This function assumes that the device's port@0 is the DSI input.
+ *
* Returns pointer to mipi_dsi_host if successful, -EINVAL if the
* request is unsupported, -EPROBE_DEFER if the DSI host is found but
* not available, or -ENODEV otherwise.
With this,
Reviewed-by: Laurent Pinchart
> if (!endpoint)
> return ERR_PTR(-ENODEV);
>
> diff --git a/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c
> b/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c
> index 4618c892cdd6..e10e469aa7a6 100644
> --- a/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c
> +++ b/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c
> @@ -400,7 +400,7 @@ static int rpi_touchscreen_probe(struct i2c_client *i2c)
> rpi_touchscreen_i2c_write(ts, REG_POWERON, 0);
>
> /* Look up the DSI host. It needs to probe before we do. */
> - endpoint = of_graph_get_next_endpoint(dev->of_node, NULL);
> + endpoint = of_graph_get_endpoint_by_regs(dev->of_node, 0, -1);
> if (!endpoint)
> return -ENODEV;
>
> diff --git a/drivers/gpu/drm/tiny/arcpgu.c b/drivers/gpu/drm/tiny/arcpgu.c
> index e5b10e41554a..04d0053b9315 100644
> --- a/drivers/gpu/drm/tiny/arcpgu.c
> +++ b/drivers/gpu/drm/tiny/arcpgu.c
> @@ -288,7 +288,7 @@ static int arcpgu_load(struct arcpgu_drm_private *arcpgu)
>* There is only one output port inside each device. It is linked with
>* encoder endpoint.
>*/
> - endpoint_node = of_graph_get_next_endpoint(pdev->dev.of_node, NULL);
> + endpoint_node = of_graph_get_endpoint_by_regs(pdev->dev.of_node, 0, -1);
> if (endpoint_node) {
> encoder_node = of_graph_get_remote_port_parent(endpoint_node);
> of_node_put(endpoint_node);
--
Regards,
Laurent Pinchart
Re: [PATCH v3 0/5] Fixing live video input in ZynqMP DPSUB
On Mon, Feb 05, 2024 at 09:29:09AM +0200, Laurent Pinchart wrote: > Hello, > > This series looks good. Tomi, could you get it merged through drm-misc ? I got things mixed up, sorry. Patches 1/5 to 4/5 are fine, but 5/5 needs a different approach. I've reviewed the first four patches, which I think are fine and can be applied already. > On Tue, Jan 23, 2024 at 06:53:57PM -0800, Anatoliy Klymenko wrote: > > Add few missing pieces to support ZynqMP DPSUB live video in mode. > > > > ZynqMP DPSUB supports 2 modes of operations in regard to video data > > input. > > > > In the first mode, DPSUB uses DMA engine to pull video data from memory > > buffers. To support this the driver implements CRTC and DRM bridge > > representing DP encoder. > > > > In the second mode, DPSUB acquires video data pushed from FPGA and > > passes it downstream to DP output. This mode of operation is modeled in > > the driver as a DRM bridge that should be attached to some external > > CRTC. > > > > Patches 1/5,2/5,3/5,4/5 are minor fixes. > > > > DPSUB requires input live video format to be configured. > > Patch 5/5: The DP Subsystem requires the input live video format to be > > configured. In this patch, we are assuming that the CRTC's bus format is > > fixed (typical for FPGA CRTC) and comes from the device tree. This is a > > proposed solution, as there is no API to query CRTC output bus format > > or negotiate it in any other way. > > > > Changes in v2: > > - Address reviewers' comments: > > - More elaborate and consistent comments / commit messages > > - Fix includes' order > > - Replace of_property_read_u32_index() with of_property_read_u32() > > > > Changes in v3: > > - Split patch #3 into 3) moving status register clear immediately after > > read; 4) masking status against interrupts' mask > > > > Link to v1: > > https://lore.kernel.org/all/20240112234222.913138-1-anatoliy.klyme...@amd.com/ > > Link to v2: > > https://lore.kernel.org/all/20240119055437.2549149-1-anatoliy.klyme...@amd.com/ > > > > Anatoliy Klymenko (5): > > drm: xlnx: zynqmp_dpsub: Make drm bridge discoverable > > drm: xlnx: zynqmp_dpsub: Fix timing for live mode > > drm: xlnx: zynqmp_dpsub: Clear status register ASAP > > drm: xlnx: zynqmp_dpsub: Filter interrupts against mask > > drm: xlnx: zynqmp_dpsub: Set live video in format > > > > drivers/gpu/drm/xlnx/zynqmp_disp.c | 111 +--- > > drivers/gpu/drm/xlnx/zynqmp_disp.h | 3 +- > > drivers/gpu/drm/xlnx/zynqmp_disp_regs.h | 8 +- > > drivers/gpu/drm/xlnx/zynqmp_dp.c| 16 +++- > > drivers/gpu/drm/xlnx/zynqmp_kms.c | 2 +- > > 5 files changed, 119 insertions(+), 21 deletions(-) -- Regards, Laurent Pinchart
Re: [PATCH v3 2/5] drm: xlnx: zynqmp_dpsub: Fix timing for live mode
Hi Anatoliy, Thank you for the patch. On Tue, Jan 23, 2024 at 06:53:59PM -0800, Anatoliy Klymenko wrote: > Expect external video timing in live video input mode, program > DPSUB acordingly. Are there no designs where the DPSUB operates in non-live mode, but uses a video clock from the PL, for instance to use a different clock frequency ? I don't think that use case is very common, so I'm fine with this patch in order to properly support the more common use case of live input, and leave the PL clock without live input use case for later. > > Reviewed-by: Tomi Valkeinen > No need for a blank line here. > Signed-off-by: Anatoliy Klymenko Reviewed-by: Laurent Pinchart > --- > drivers/gpu/drm/xlnx/zynqmp_disp.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c > b/drivers/gpu/drm/xlnx/zynqmp_disp.c > index 407bc07cec69..8a39b3accce5 100644 > --- a/drivers/gpu/drm/xlnx/zynqmp_disp.c > +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c > @@ -1166,7 +1166,7 @@ void zynqmp_disp_enable(struct zynqmp_disp *disp) > /* Choose clock source based on the DT clock handle. */ > zynqmp_disp_avbuf_set_clocks_sources(disp, disp->dpsub->vid_clk_from_ps, >disp->dpsub->aud_clk_from_ps, > - true); > + disp->dpsub->vid_clk_from_ps); > zynqmp_disp_avbuf_enable_channels(disp); > zynqmp_disp_avbuf_enable_audio(disp); > -- Regards, Laurent Pinchart
Re: [PATCH v3 4/5] drm: xlnx: zynqmp_dpsub: Filter interrupts against mask
Hi Anatoliy, Thank you for the patch. On Tue, Jan 23, 2024 at 06:54:01PM -0800, Anatoliy Klymenko wrote: > Filter out status register against the interrupts' mask. > > Some events are being reported via DP status register, even if > corresponding interrupts have been disabled. One instance of such event > leads to generation of VBLANK when the driver is in DRM bridge mode, > which in turn results in NULL pointer dereferencing. We should avoid > processing such events in an interrupt handler context. > > This problem is less noticeable when the driver operates in DMA mode, as > in this case we have DRM CRTC object instantiated and DRM framework > simply discards unwanted VBLANKs in drm_handle_vblank(). > > Signed-off-by: Anatoliy Klymenko Reviewed-by: Laurent Pinchart > --- > drivers/gpu/drm/xlnx/zynqmp_dp.c | 9 - > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c > b/drivers/gpu/drm/xlnx/zynqmp_dp.c > index 5a3335e1fffa..9f48e5bbcdec 100644 > --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c > +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c > @@ -1627,7 +1627,14 @@ static irqreturn_t zynqmp_dp_irq_handler(int irq, void > *data) > /* clear status register as soon as we read it */ > zynqmp_dp_write(dp, ZYNQMP_DP_INT_STATUS, status); > mask = zynqmp_dp_read(dp, ZYNQMP_DP_INT_MASK); > - if (!(status & ~mask)) > + > + /* > + * Status register may report some events, which corresponding > interrupts > + * have been disabled. Filter out those events against interrupts' mask. > + */ > + status &= ~mask; > + > + if (!status) > return IRQ_NONE; > > /* dbg for diagnostic, but not much that the driver can do */ -- Regards, Laurent Pinchart
