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
Re: [PATCH v3 3/5] drm: xlnx: zynqmp_dpsub: Clear status register ASAP
Hi Anatoliy, Thank you for the patch. On Tue, Jan 23, 2024 at 06:54:00PM -0800, Anatoliy Klymenko wrote: > Clear status register as soon as we read it. > > Addressing comments from > https://lore.kernel.org/dri-devel/beb551c7-bb7e-4cd0-b166-e9aad90c4...@ideasonboard.com/ > > Signed-off-by: Anatoliy Klymenko > --- > drivers/gpu/drm/xlnx/zynqmp_dp.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c > b/drivers/gpu/drm/xlnx/zynqmp_dp.c > index d60b7431603f..5a3335e1fffa 100644 > --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c > +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c > @@ -1624,6 +1624,8 @@ static irqreturn_t zynqmp_dp_irq_handler(int irq, void > *data) > u32 status, mask; > > status = zynqmp_dp_read(dp, ZYNQMP_DP_INT_STATUS); > + /* clear status register as soon as we read it */ I don't think a comment is strictly required, but I don't mind it. Reviewed-by: Laurent Pinchart > + zynqmp_dp_write(dp, ZYNQMP_DP_INT_STATUS, status); > mask = zynqmp_dp_read(dp, ZYNQMP_DP_INT_MASK); > if (!(status & ~mask)) > return IRQ_NONE; > @@ -1634,8 +1636,6 @@ static irqreturn_t zynqmp_dp_irq_handler(int irq, void > *data) > if (status & ZYNQMP_DP_INT_CHBUF_OVERFLW_MASK) > dev_dbg_ratelimited(dp->dev, "overflow interrupt\n"); > > - zynqmp_dp_write(dp, ZYNQMP_DP_INT_STATUS, status); > - > if (status & ZYNQMP_DP_INT_VBLANK_START) > zynqmp_dpsub_drm_handle_vblank(dp->dpsub); > -- Regards, Laurent Pinchart
Re: [PATCH v3 0/5] Fixing live video input in ZynqMP DPSUB
Hello, This series looks good. Tomi, could you get it merged through drm-misc ? 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: RE: Re: [PATCH 0/4] Fixing live video input in ZynqMP DPSUB
On Thu, Feb 01, 2024 at 06:01:01PM +0100, Maxime Ripard wrote: > On Fri, Jan 26, 2024 at 11:18:30PM +, Klymenko, Anatoliy wrote: > > On Friday, January 26, 2024 4:26 AM, Maxime Ripard wrote: > > > On Wed, Jan 17, 2024 at 04:23:43PM +0200, Laurent Pinchart wrote: > > > > On Mon, Jan 15, 2024 at 09:28:39AM +0100, Maxime Ripard wrote: > > > > > On Fri, Jan 12, 2024 at 03:42:18PM -0800, Anatoliy Klymenko wrote: > > > > > > Patches 1/4,2/4,3/4 are minor fixes. > > > > > > > > > > > > DPSUB requires input live video format to be configured. > > > > > > Patch 4/4: 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 > > > > > > and comes from the device tree. This is a proposed solution, as > > > > > > there are no api > > > > > > to query CRTC output bus format. > > > > > > > > > > > > Is this a good approach to go with? > > > > > > > > > > I guess you would need to expand a bit on what "live video input" is? > > > > > Is > > > > > it some kind of mechanism to bypass memory and take your pixels > > > > > straight > > > > > from a FIFO from another device, or something else? > > > > > > > > Yes and no. > > > > > > > > The DPSUB integrates DMA engines, a blending engine (two planes), and a > > > > DP encoder. The dpsub driver supports all of this, and creates a DRM > > > > device. The DP encoder hardware always takes its input data from the > > > > output of the blending engine. > > > > > > > > The blending engine can optionally take input data from a bus connected > > > > to the FPGA fabric, instead of taking it from the DPSUB internal DMA > > > > engines. When operating in that mode, the dpsub driver exposes the DP > > > > encoder as a bridge, and internally programs the blending engine to > > > > disable blending. Typically, the FPGA fabric will then contain a CRTC of > > > > some sort, with a driver that will acquire the DP encoder bridge as > > > > usually done. > > > > > > > > In this mode of operation, it is typical for the IP cores in FPGA fabric > > > > to be synthesized with a fixed format (as that saves resources), while > > > > the DPSUB supports multiple input formats. > > > > > > Where is that CRTC driver? It's not clear to me why the format would > > > need to be in the device tree at all. Format negociation between the > > > CRTC and whatever comes next is already done in a number of drivers so > > > it would be useful to have that kind of API outside of the bridge > > > support. > > > > One example of such CRTC driver: > > https://github.com/Xilinx/linux-xlnx/blob/master/drivers/gpu/drm/xlnx/xlnx_mixer.c > > It's not > > upstreamed yet. Bus format negotiations here are handled by utilizing > > Xilinx-specific bridge > > framework. Ideally, it would be nice to rework this to comply with the > > upstream DRM bridge > > framework. > > > > > > Bridge drivers in the upstream kernel work the other way around, with > > > > the bridge hardware supporting a limited set of formats, and the CRTC > > > > then being programmed with whatever the bridges chain needs. Here, the > > > > negotiation needs to go the other way around, as the CRTC is the > > > > limiting factor, not the bridge. > > > > > > Sounds like there's something to rework in the API then? > > > > > Adding an optional CRTC callback imposing CRTC specific bus format > > restrictions, which may be > > called from here > > https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/drm_bridge.c#L935 > > would solve the problem. > > CRTCs and bridges are orthogonal. If anything, I'd expect that callback > to be set at the CRTC, encoder and connector levels and filled by the > drm_bridge code if relevant. I'm thinking about a new CRTC operation that would be called by the bridge chain format negotiation helper drm_atomic_bridge_chain_select_bus_fmts() (or one of the functions it calls), to filter the list of formats supported by the chain based on what the CRTC supports, or possibly to pick a format in that list. This needs to be prototyped -- Regards, Laurent Pinchart
Re: [PATCH 3/5] drm/bridge: simple-bridge: Allow acquiring the next bridge with fwnode API
Hello Sui,
On Tue, Jan 23, 2024 at 08:18:22PM +0800, Sui Jingfeng wrote:
> On 2024/1/23 09:18, Laurent Pinchart wrote:
> > On Tue, Jan 23, 2024 at 12:32:18AM +0800, Sui Jingfeng wrote:
> >> Which make it possible to use this driver on non-DT based systems,
> >> meanwhile, made no functional changes for DT based systems.
> >>
> >> Signed-off-by: Sui Jingfeng
> >> ---
> >> drivers/gpu/drm/bridge/simple-bridge.c | 51 ++
> >> 1 file changed, 44 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/bridge/simple-bridge.c
> >> b/drivers/gpu/drm/bridge/simple-bridge.c
> >> index 595f672745b9..cfea5a67cc5b 100644
> >> --- a/drivers/gpu/drm/bridge/simple-bridge.c
> >> +++ b/drivers/gpu/drm/bridge/simple-bridge.c
> >> @@ -184,6 +184,39 @@ static const void *simple_bridge_get_match_data(const
> >> struct device *dev)
> >>return NULL;
> >> }
> >>
> >> +static int simple_bridge_get_next_bridge_by_fwnode(struct device *dev,
> >> + struct drm_bridge
> >> **next_bridge)
> >> +{
> >> + struct drm_bridge *bridge;
> >> + struct fwnode_handle *ep;
> >> + struct fwnode_handle *remote;
> >> +
> >> + ep = fwnode_graph_get_endpoint_by_id(dev->fwnode, 1, 0, 0);
> >> + if (!ep) {
> >> + dev_err(dev, "The endpoint is unconnected\n");
> >> + return -EINVAL;
> >> + }
> >> +
> >> + remote = fwnode_graph_get_remote_port_parent(ep);
> >> + fwnode_handle_put(ep);
> >> + if (!remote) {
> >> + dev_err(dev, "No valid remote node\n");
> >> + return -ENODEV;
> >> + }
> >> +
> >> + bridge = drm_bridge_find_by_fwnode(remote);
> >> + fwnode_handle_put(remote);
> >> +
> >> + if (!bridge) {
> >> + dev_warn(dev, "Next bridge not found, deferring probe\n");
> >> + return -EPROBE_DEFER;
> >> + }
> >> +
> >> + *next_bridge = bridge;
> >> +
> >> + return 0;
> >> +}
> >> +
> >
> > H yes, this convinces me further that we should switch to fwnode,
> > not implement fwnode and OF side-by-side.
>
> OK, I'm agree with you.
>
> But this means that I have to make the drm_bridge_find_by_fwnode() function
> works
> on both DT systems and non-DT systems. This is also means that we will no
> longer
> need to call of_drm_find_bridge() function anymore. This will eventually lead
> to
> completely remove of_drm_find_bridge()?
It would be replaced by fwnode_drm_find_bridge(). Although, if we need
to rename the function, I think it would be best to make have a drm_
prefix, maybe drm_bridge_find-by_fwnode() or something similar.
> As far as I can see, if I follow you suggestion, drm/bridge subsystem will
> encountering a *big* refactor. My 'side-by-side' approach allows co-exist.
> It is not really meant to purge OF. I feel it is a little bit of aggressive.
>
> hello Maxime, are you watching this? what do you think?
>
> >> static int simple_bridge_probe(struct platform_device *pdev)
> >> {
> >>struct simple_bridge *sbridge;
> >> @@ -199,14 +232,17 @@ static int simple_bridge_probe(struct
> >> platform_device *pdev)
> >>else
> >>sbridge->info = simple_bridge_get_match_data(>dev);
> >>
> >> - /* Get the next bridge in the pipeline. */
> >> - remote = of_graph_get_remote_node(pdev->dev.of_node, 1, -1);
> >> - if (!remote)
> >> - return -EINVAL;
> >> -
> >> - sbridge->next_bridge = of_drm_find_bridge(remote);
> >> - of_node_put(remote);
> >> + if (pdev->dev.of_node) {
> >> + /* Get the next bridge in the pipeline. */
> >> + remote = of_graph_get_remote_node(pdev->dev.of_node, 1, -1);
> >> + if (!remote)
> >> + return -EINVAL;
> >>
> >> + sbridge->next_bridge = of_drm_find_bridge(remote);
> >> + of_node_put(remote);
> >> + } else {
> >> + simple_bridge_get_next_bridge_by_fwnode(>dev,
> >> >next_bridge);
> >> + }
> >>if (!sbridge->next_bridge) {
> >>dev_dbg(>dev, "Next bridge not found, deferring probe\n");
> >>return -EPROBE_DEFER;
> >> @@ -231,6 +267,7 @@ static int simple_bridge_probe(struct platform_device
> >> *pdev)
> >>/* Register the bridge. */
> >>sbridge->bridge.funcs = _bridge_bridge_funcs;
> >>sbridge->bridge.of_node = pdev->dev.of_node;
> >> + sbridge->bridge.fwnode = pdev->dev.fwnode;
> >>sbridge->bridge.timings = sbridge->info->timings;
> >>
> >>drm_bridge_add(>bridge);
--
Regards,
Laurent Pinchart
Re: [PATCH 2/5] drm/bridge: simple-bridge: Extend match support for non-DT based systems
On Tue, Jan 23, 2024 at 04:20:04PM +0800, Sui Jingfeng wrote: > On 2024/1/23 09:21, Laurent Pinchart wrote: > > On Tue, Jan 23, 2024 at 12:32:17AM +0800, Sui Jingfeng wrote: > >> Which is intended to be used on non-DT environment, where the simple-bridge > >> platform device is created by either the display controller driver side or > >> platform firmware subsystem. > > > > Could you give an example of a platform where you intend to use this ? > > For example: > > 1) USB based display adapter, such as FL2000DX[1] which use > the it66121 HDMI transmitter to convert the RGB888 to HDMI. > > 2) Simple 2D PCIe display controller, such as SM750(EMPV-1201) > which using sii9022 HDMI transmitter to convert the RGB888 > to HDMI. > > 3) Some FPGA PCIe Board (sil9136) > > 4) Be able to run unit test of drm bridges on X86. Thank you, those are useful examples. It would be nice to capture at least some of them (first instance the first two) to the commit message. > [1] https://github.com/FrescoLogic/FL2000 -- Regards, Laurent Pinchart
Re: [PATCH 1/5] drm/bridge: Add drm_bridge_find_by_fwnode() helper
Hi Sui, On Tue, Jan 23, 2024 at 04:01:28PM +0800, Sui Jingfeng wrote: > On 2024/1/23 09:17, Laurent Pinchart wrote: > > On Tue, Jan 23, 2024 at 12:32:16AM +0800, Sui Jingfeng wrote: > >> Because ACPI based systems only has the fwnode associated, the of_node > >> member of struct device is NULL. To order to move things forward, we add > >> drm_bridge_find_by_fwnode() to extend the support. > >> > >> Signed-off-by: Sui Jingfeng > > > > Could we switch completely to fwnode, instead of maintaining the fwnode > > and OF options side-by-side ? > > The side-by-side approach allow us to migrate smoothly, But it increases the maintenance burden for the duration of the migration. I fear the migration would span years, with nobody really taking active care of it, and the OF and non-OF API will have a risk to diverge. > the main consideration is that the OF approach has been > works very well, it is flexible and very successful in > the embedded world. fwnode is a superset of OF, so I don't expect issues switching from OF to fwnode. For the non-OF, non-fwnode users, that's possibly a different question. > It seems that the fwnode API could NOT replace the OF > options completely. For example, the'of_device_id' and 'of_match_table' > related things are always there, there Yes, and that's not a problem. OF drivers still use of_device_id and of_match_table, even if they use the fwnode API. No issue there. > are large well-established helpers and subroutines and > already formed as a standard. Some part of it may suffer > from backward compatibility problems. fwnode has been designed to offer the same API as OF for drivers. If something is missing, it can be raised with the maintainers. > So I want to leave some space to other programmers. > Maybe there are other programmers who feel that using > OF alone is enough for a specific problem(domain). -- Regards, Laurent Pinchart
Re: [PATCH 2/5] drm/bridge: simple-bridge: Extend match support for non-DT based systems
On Tue, Jan 23, 2024 at 12:32:17AM +0800, Sui Jingfeng wrote:
> Which is intended to be used on non-DT environment, where the simple-bridge
> platform device is created by either the display controller driver side or
> platform firmware subsystem.
Could you give an example of a platform where you intend to use this ?
> To avoid duplication and to keep consistent,
> we choose to reuse the OF match tables. Because the potentional user may
> not has a of_node attached, nor a ACPI match id. If this is the case,
> a software node string property can be provide to fill the niche.
Shouldn't non-DT, non-ACPI platforms use swnodes ?
> Signed-off-by: Sui Jingfeng
> ---
> drivers/gpu/drm/bridge/simple-bridge.c | 24 +++-
> 1 file changed, 23 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/bridge/simple-bridge.c
> b/drivers/gpu/drm/bridge/simple-bridge.c
> index cbe8e778d7c7..595f672745b9 100644
> --- a/drivers/gpu/drm/bridge/simple-bridge.c
> +++ b/drivers/gpu/drm/bridge/simple-bridge.c
> @@ -166,6 +166,24 @@ static const struct drm_bridge_funcs
> simple_bridge_bridge_funcs = {
> .disable= simple_bridge_disable,
> };
>
> +static const void *simple_bridge_get_match_data(const struct device *dev)
> +{
> + const struct of_device_id *matches = dev->driver->of_match_table;
> +
> + /* Try to get the match data by software node */
> + while (matches) {
> + if (!matches->compatible[0])
> + break;
> +
> + if (device_is_compatible(dev, matches->compatible))
> + return matches->data;
> +
> + matches++;
> + }
> +
> + return NULL;
> +}
> +
> static int simple_bridge_probe(struct platform_device *pdev)
> {
> struct simple_bridge *sbridge;
> @@ -176,7 +194,10 @@ static int simple_bridge_probe(struct platform_device
> *pdev)
> return -ENOMEM;
> platform_set_drvdata(pdev, sbridge);
>
> - sbridge->info = of_device_get_match_data(>dev);
> + if (pdev->dev.of_node)
> + sbridge->info = of_device_get_match_data(>dev);
> + else
> + sbridge->info = simple_bridge_get_match_data(>dev);
>
> /* Get the next bridge in the pipeline. */
> remote = of_graph_get_remote_node(pdev->dev.of_node, 1, -1);
> @@ -309,3 +330,4 @@ module_platform_driver(simple_bridge_driver);
> MODULE_AUTHOR("Maxime Ripard ");
> MODULE_DESCRIPTION("Simple DRM bridge driver");
> MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:simple-bridge");
This is an unrelated change.
--
Regards,
Laurent Pinchart
Re: [PATCH 5/5] drm-bridge: display-connector: Switch to use fwnode API
On Tue, Jan 23, 2024 at 12:32:20AM +0800, Sui Jingfeng wrote:
> From: Sui Jingfeng
>
> Because API has wider coverage, it can be used on non-DT systems as well.
>
> Signed-off-by: Sui Jingfeng
> ---
> drivers/gpu/drm/bridge/display-connector.c | 22 --
> 1 file changed, 12 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/display-connector.c
> b/drivers/gpu/drm/bridge/display-connector.c
> index eb7e194e7735..2c3e54a458e8 100644
> --- a/drivers/gpu/drm/bridge/display-connector.c
> +++ b/drivers/gpu/drm/bridge/display-connector.c
> @@ -243,8 +243,8 @@ static int display_connector_probe(struct platform_device
> *pdev)
> case DRM_MODE_CONNECTOR_DVII: {
> bool analog, digital;
>
> - analog = of_property_read_bool(pdev->dev.of_node, "analog");
> - digital = of_property_read_bool(pdev->dev.of_node, "digital");
> + analog = fwnode_property_present(pdev->dev.fwnode, "analog");
> + digital = fwnode_property_present(pdev->dev.fwnode, "digital");
> if (analog && !digital) {
> conn->bridge.type = DRM_MODE_CONNECTOR_DVIA;
> } else if (!analog && digital) {
> @@ -261,8 +261,8 @@ static int display_connector_probe(struct platform_device
> *pdev)
> case DRM_MODE_CONNECTOR_HDMIA: {
> const char *hdmi_type;
>
> - ret = of_property_read_string(pdev->dev.of_node, "type",
> - _type);
> + ret = fwnode_property_read_string(pdev->dev.fwnode, "type",
> + _type);
> if (ret < 0) {
> dev_err(>dev, "HDMI connector with no type\n");
> return -EINVAL;
> @@ -292,7 +292,7 @@ static int display_connector_probe(struct platform_device
> *pdev)
> conn->bridge.interlace_allowed = true;
>
> /* Get the optional connector label. */
> - of_property_read_string(pdev->dev.of_node, "label", );
> + fwnode_property_read_string(pdev->dev.fwnode, "label", );
>
> /*
>* Get the HPD GPIO for DVI, HDMI and DP connectors. If the GPIO can
> provide
> @@ -330,12 +330,13 @@ static int display_connector_probe(struct
> platform_device *pdev)
> if (type == DRM_MODE_CONNECTOR_DVII ||
> type == DRM_MODE_CONNECTOR_HDMIA ||
> type == DRM_MODE_CONNECTOR_VGA) {
> - struct device_node *phandle;
> + struct fwnode_handle *fwnode;
>
> - phandle = of_parse_phandle(pdev->dev.of_node, "ddc-i2c-bus", 0);
> - if (phandle) {
> - conn->bridge.ddc = of_get_i2c_adapter_by_node(phandle);
> - of_node_put(phandle);
> + fwnode = fwnode_find_reference(pdev->dev.fwnode, "ddc-i2c-bus",
> 0);
> + if (!IS_ERR_OR_NULL(fwnode)) {
> + dev_info(>dev, "has I2C bus property\n");
This looks like a debugging leftover.
> + conn->bridge.ddc = i2c_get_adapter_by_fwnode(fwnode);
> + fwnode_handle_put(fwnode);
> if (!conn->bridge.ddc)
> return -EPROBE_DEFER;
> } else {
> @@ -380,6 +381,7 @@ static int display_connector_probe(struct platform_device
> *pdev)
>
> conn->bridge.funcs = _connector_bridge_funcs;
> conn->bridge.of_node = pdev->dev.of_node;
> + conn->bridge.fwnode = pdev->dev.fwnode;
This goes in the right direction. Let's address the other drivers and
drop the OF-based calls in the same series :-)
>
> if (conn->bridge.ddc)
> conn->bridge.ops |= DRM_BRIDGE_OP_EDID
--
Regards,
Laurent Pinchart
Re: [PATCH 3/5] drm/bridge: simple-bridge: Allow acquiring the next bridge with fwnode API
On Tue, Jan 23, 2024 at 12:32:18AM +0800, Sui Jingfeng wrote:
> Which make it possible to use this driver on non-DT based systems,
> meanwhile, made no functional changes for DT based systems.
>
> Signed-off-by: Sui Jingfeng
> ---
> drivers/gpu/drm/bridge/simple-bridge.c | 51 ++
> 1 file changed, 44 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/simple-bridge.c
> b/drivers/gpu/drm/bridge/simple-bridge.c
> index 595f672745b9..cfea5a67cc5b 100644
> --- a/drivers/gpu/drm/bridge/simple-bridge.c
> +++ b/drivers/gpu/drm/bridge/simple-bridge.c
> @@ -184,6 +184,39 @@ static const void *simple_bridge_get_match_data(const
> struct device *dev)
> return NULL;
> }
>
> +static int simple_bridge_get_next_bridge_by_fwnode(struct device *dev,
> +struct drm_bridge
> **next_bridge)
> +{
> + struct drm_bridge *bridge;
> + struct fwnode_handle *ep;
> + struct fwnode_handle *remote;
> +
> + ep = fwnode_graph_get_endpoint_by_id(dev->fwnode, 1, 0, 0);
> + if (!ep) {
> + dev_err(dev, "The endpoint is unconnected\n");
> + return -EINVAL;
> + }
> +
> + remote = fwnode_graph_get_remote_port_parent(ep);
> + fwnode_handle_put(ep);
> + if (!remote) {
> + dev_err(dev, "No valid remote node\n");
> + return -ENODEV;
> + }
> +
> + bridge = drm_bridge_find_by_fwnode(remote);
> + fwnode_handle_put(remote);
> +
> + if (!bridge) {
> + dev_warn(dev, "Next bridge not found, deferring probe\n");
> + return -EPROBE_DEFER;
> + }
> +
> + *next_bridge = bridge;
> +
> + return 0;
> +}
> +
H yes, this convinces me further that we should switch to fwnode,
not implement fwnode and OF side-by-side.
> static int simple_bridge_probe(struct platform_device *pdev)
> {
> struct simple_bridge *sbridge;
> @@ -199,14 +232,17 @@ static int simple_bridge_probe(struct platform_device
> *pdev)
> else
> sbridge->info = simple_bridge_get_match_data(>dev);
>
> - /* Get the next bridge in the pipeline. */
> - remote = of_graph_get_remote_node(pdev->dev.of_node, 1, -1);
> - if (!remote)
> - return -EINVAL;
> -
> - sbridge->next_bridge = of_drm_find_bridge(remote);
> - of_node_put(remote);
> + if (pdev->dev.of_node) {
> + /* Get the next bridge in the pipeline. */
> + remote = of_graph_get_remote_node(pdev->dev.of_node, 1, -1);
> + if (!remote)
> + return -EINVAL;
>
> + sbridge->next_bridge = of_drm_find_bridge(remote);
> + of_node_put(remote);
> + } else {
> + simple_bridge_get_next_bridge_by_fwnode(>dev,
> >next_bridge);
> + }
> if (!sbridge->next_bridge) {
> dev_dbg(>dev, "Next bridge not found, deferring probe\n");
> return -EPROBE_DEFER;
> @@ -231,6 +267,7 @@ static int simple_bridge_probe(struct platform_device
> *pdev)
> /* Register the bridge. */
> sbridge->bridge.funcs = _bridge_bridge_funcs;
> sbridge->bridge.of_node = pdev->dev.of_node;
> + sbridge->bridge.fwnode = pdev->dev.fwnode;
> sbridge->bridge.timings = sbridge->info->timings;
>
> drm_bridge_add(>bridge);
> --
> 2.25.1
>
--
Regards,
Laurent Pinchart
Re: [PATCH 1/5] drm/bridge: Add drm_bridge_find_by_fwnode() helper
Hi Sui,
Thank you for the patch.
On Tue, Jan 23, 2024 at 12:32:16AM +0800, Sui Jingfeng wrote:
> Because ACPI based systems only has the fwnode associated, the of_node
> member of struct device is NULL. To order to move things forward, we add
> drm_bridge_find_by_fwnode() to extend the support.
>
> Signed-off-by: Sui Jingfeng
Could we switch completely to fwnode, instead of maintaining the fwnode
and OF options side-by-side ?
> ---
> drivers/gpu/drm/drm_bridge.c | 33 +
> include/drm/drm_bridge.h | 4
> 2 files changed, 37 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index cee3188adf3d..ffd969adc2fb 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -1347,6 +1347,39 @@ struct drm_bridge *of_drm_find_bridge(struct
> device_node *np)
> EXPORT_SYMBOL(of_drm_find_bridge);
> #endif
>
> +/**
> + * drm_bridge_find_by_fwnode - Find the bridge corresponding to the
> associated fwnode
> + *
> + * @fwnode: fwnode for which to find the matching drm_bridge
> + *
> + * This function looks up a drm_bridge based on its associated fwnode.
> + *
> + * RETURNS:
> + * A reference to the drm_bridge control structure if found, NULL on failure.
> + */
> +struct drm_bridge *drm_bridge_find_by_fwnode(struct fwnode_handle *fwnode)
> +{
> + struct drm_bridge *ret = NULL;
> + struct drm_bridge *bridge;
> +
> + if (!fwnode)
> + return NULL;
> +
> + mutex_lock(_lock);
> +
> + list_for_each_entry(bridge, _list, list) {
> + if (bridge->fwnode == fwnode) {
> + ret = bridge;
> + break;
> + }
> + }
> +
> + mutex_unlock(_lock);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL(drm_bridge_find_by_fwnode);
> +
> MODULE_AUTHOR("Ajay Kumar ");
> MODULE_DESCRIPTION("DRM bridge infrastructure");
> MODULE_LICENSE("GPL and additional rights");
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index e39da5807ba7..fe3d5f4bf37f 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -720,6 +720,8 @@ struct drm_bridge {
> struct list_head chain_node;
> /** @of_node: device node pointer to the bridge */
> struct device_node *of_node;
> + /** @fwnode: associated fwnode supplied by platform firmware */
> + struct fwnode_handle *fwnode;
> /** @list: to keep track of all added bridges */
> struct list_head list;
> /**
> @@ -796,6 +798,8 @@ static inline struct drm_bridge
> *of_drm_find_bridge(struct device_node *np)
> }
> #endif
>
> +struct drm_bridge *drm_bridge_find_by_fwnode(struct fwnode_handle *fwnode);
> +
> /**
> * drm_bridge_get_next_bridge() - Get the next bridge in the chain
> * @bridge: bridge object
--
Regards,
Laurent Pinchart
Re: [PATCH 3/4] drm: xlnx: zynqmp_dpsub: Don't generate vblank in live mode
Hi Anatoliy, On Fri, Jan 19, 2024 at 05:53:30AM +, Klymenko, Anatoliy wrote: > On Wed, 17 Jan 2024 16:20:10 +0200, Tomi Valkeinen wrote: > > On 13/01/2024 01:42, Anatoliy Klymenko wrote: > > > Filter out status register against interrupts' mask. > > > Some events are being reported via DP status register, even if > > > corresponding interrupts have been disabled. Avoid processing of such > > > events in interrupt handler context. > > > > The subject talks about vblank and live mode, the the description doesn't. > > Can > > you elaborate in the desc a bit about when this is an issue and why it > > wasn't > > before? > > Yes, I should make patch comment more consistent and elaborate. I will fix it > in the next version. Thank you. > > > > > > Signed-off-by: Anatoliy Klymenko > > > --- > > > drivers/gpu/drm/xlnx/zynqmp_dp.c | 11 +-- > > > 1 file changed, 9 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c > > > b/drivers/gpu/drm/xlnx/zynqmp_dp.c > > > index d60b7431603f..571c5dbc97e5 100644 > > > --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c > > > +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c > > > @@ -1624,8 +1624,16 @@ static irqreturn_t zynqmp_dp_irq_handler(int irq, > > > void *data) > > > u32 status, mask; > > > > > > status = zynqmp_dp_read(dp, ZYNQMP_DP_INT_STATUS); > > > + 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 */ > > > @@ -1634,7 +1642,6 @@ static irqreturn_t zynqmp_dp_irq_handler(int irq, > > > void *data) > > > if (status & ZYNQMP_DP_INT_CHBUF_OVERFLW_MASK) > > > dev_dbg_ratelimited(dp->dev, "overflow interrupt\n"); > > > > > > - zynqmp_dp_write(dp, ZYNQMP_DP_INT_STATUS, status); > > > > > > if (status & ZYNQMP_DP_INT_VBLANK_START) > > > zynqmp_dpsub_drm_handle_vblank(dp->dpsub); > > > > Moving the zynqmp_dp_write() is not related to this fix, is it? I think it > > should be in > > a separate patch. > > The sole purpose of zynqmp_dp_write() is to clear status register. The > sooner we do it the better (after reading status in the local variable > of course). No disagreement about that. Tomi's point is that it's a good change, but it should be done in a separate patch, by itself, not bundled with other changes. The usual rule in the kernel is "one change, one commit". > We can also reuse status variable for in-place filtering > against the interrupt mask, which makes this change dependent on > zynqmp_dp_write() reordering. I will add a comment explaining this. Is > it ok? -- Regards, Laurent Pinchart
Re: [PATCH 1/4] drm: xlnx: zynqmp_dpsub: Make drm bridge discoverable
On Wed, Jan 17, 2024 at 04:06:31PM +0200, Tomi Valkeinen wrote: > On 13/01/2024 01:42, Anatoliy Klymenko wrote: > > Assign device of node to bridge prior registering it. This will > > make said bridge discoverable by separate crtc driver. > > I think a few words on why this is needed (and why it wasn't needed > before) would be nice. > > Other than that: > > Reviewed-by: Tomi Valkeinen Agreed. With a better commit message, Reviewed-by: Laurent Pinchart > > Signed-off-by: Anatoliy Klymenko > > --- > > drivers/gpu/drm/xlnx/zynqmp_dp.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c > > b/drivers/gpu/drm/xlnx/zynqmp_dp.c > > index a0606fab0e22..d60b7431603f 100644 > > --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c > > +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c > > @@ -1721,6 +1721,7 @@ int zynqmp_dp_probe(struct zynqmp_dpsub *dpsub) > > bridge->ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID > > | DRM_BRIDGE_OP_HPD; > > bridge->type = DRM_MODE_CONNECTOR_DisplayPort; > > + bridge->of_node = dp->dev->of_node; > > dpsub->bridge = bridge; > > > > /* -- Regards, Laurent Pinchart
Re: [PATCH 0/4] Fixing live video input in ZynqMP DPSUB
On Mon, Jan 15, 2024 at 09:28:39AM +0100, Maxime Ripard wrote: > On Fri, Jan 12, 2024 at 03:42:18PM -0800, Anatoliy Klymenko wrote: > > Patches 1/4,2/4,3/4 are minor fixes. > > > > DPSUB requires input live video format to be configured. > > Patch 4/4: 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 > > and comes from the device tree. This is a proposed solution, as there are > > no api > > to query CRTC output bus format. > > > > Is this a good approach to go with? > > I guess you would need to expand a bit on what "live video input" is? Is > it some kind of mechanism to bypass memory and take your pixels straight > from a FIFO from another device, or something else? Yes and no. The DPSUB integrates DMA engines, a blending engine (two planes), and a DP encoder. The dpsub driver supports all of this, and creates a DRM device. The DP encoder hardware always takes its input data from the output of the blending engine. The blending engine can optionally take input data from a bus connected to the FPGA fabric, instead of taking it from the DPSUB internal DMA engines. When operating in that mode, the dpsub driver exposes the DP encoder as a bridge, and internally programs the blending engine to disable blending. Typically, the FPGA fabric will then contain a CRTC of some sort, with a driver that will acquire the DP encoder bridge as usually done. In this mode of operation, it is typical for the IP cores in FPGA fabric to be synthesized with a fixed format (as that saves resources), while the DPSUB supports multiple input formats. Bridge drivers in the upstream kernel work the other way around, with the bridge hardware supporting a limited set of formats, and the CRTC then being programmed with whatever the bridges chain needs. Here, the negotiation needs to go the other way around, as the CRTC is the limiting factor, not the bridge. Is this explanation clear ? -- Regards, Laurent Pinchart
Re: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support
Hi Biju,
On Thu, Jan 04, 2024 at 02:17:39PM +, Biju Das wrote:
> On Friday, December 15, 2023 2:56 PM, Biju Das wrote:
> > On Friday, December 15, 2023 2:18 PM, Maxime Ripard wrote:
> > > On Fri, Dec 15, 2023 at 01:52:28PM +, Biju Das wrote:
> > > > > > > > > > +static int rzg2l_du_crtc_enable_vblank(struct drm_crtc
> > > > > > > > > > *crtc) {
> > > > > > > > > > + struct rzg2l_du_crtc *rcrtc = to_rzg2l_crtc(crtc);
> > > > > > > > > > +
> > > > > > > > > > + rcrtc->vblank_enable = true;
> > > > > > > > > > +
> > > > > > > > > > + return 0;
> > > > > > > > > > +}
> > > > > > > > > > +
> > > > > > > > > > +static void rzg2l_du_crtc_disable_vblank(struct drm_crtc
> > > > > > > > > > *crtc)
> > > > > > > > > > +{
> > > > > > > > > > + struct rzg2l_du_crtc *rcrtc = to_rzg2l_crtc(crtc);
> > > > > > > > > > +
> > > > > > > > > > + rcrtc->vblank_enable = false; }
> > > > > > > > >
> > > > > > > > > You should enable / disable your interrupts here
> > > > > > > >
> > > > > > > > We don't have dedicated vblank IRQ for enabling/disabling
> > > > > > > > vblank.
> > > > > > > >
> > > > > > > > vblank is handled by vspd.
> > > > > > > >
> > > > > > > > vspd is directly rendering images to display,
> > > > > > > > rcar_du_crtc_finish_page_flip() and drm_crtc_handle_vblank()
> > > > > > > > called in vspd's pageflip context.
> > > > > > > >
> > > > > > > > See rzg2l_du_vsp_complete()in rzg2l_du_vsp.c
> > > > > > >
> > > > > > > Sorry, I couldn't really get how the interrupt flow / vblank
> > > > > > > reporting is going to work. Could you explain it a bit more?
> > > > > >
> > > > > > We just need to handle vertical blanking in the VSP frame end
> > > > > > handler.
> > > > > > See the code below.
> > > > > >
> > > > > > static void rzg2l_du_vsp_complete(void *private, unsigned int
> > > > > > status,
> > > > > > u32 crc) {
> > > > > > struct rzg2l_du_crtc *crtc = private;
> > > > > >
> > > > > > if (crtc->vblank_enable)
> > > > > > drm_crtc_handle_vblank(>crtc);
> > > > > >
> > > > > > if (status & VSP1_DU_STATUS_COMPLETE)
> > > > > > rzg2l_du_crtc_finish_page_flip(crtc);
> > > > > >
> > > > > > drm_crtc_add_crc_entry(>crtc, false, 0, ); }
> > > > >
> > > > > Then we're back to the same question :)
> > > > >
> > > > > Why can't you mask the frame end interrupt?
> > > >
> > > > We are masking interrupts.
> > > >
> > > > [ 70.639139] ###rzg2l_du_crtc_disable_vblank###
> > > > [ 70.650243] #rzg2l_du_vsp_disable
> > > > [ 70.652003] ## vsp1_wpf_stop###
> > > >
> > > > Unmask is,
> > > >
> > > > [ 176.354520] ###rzg2l_du_crtc_enable_vblank###
> > > > [ 176.354922] #rzg2l_du_vsp_atomic_flush
> > > > [ 176.355198] ## wpf_configure_stream###
> > >
> > > Sorry, my question was why aren't you unmasking and masking them in
> > > the enable/disable_vblank hooks of the CRTC.
> >
> > I have n't tried that. Will try and provide feedback.
> >
> > Currently the IRQ source belongs to VSPD in media subsystem.
> > So I need to export an API though vsp1_drm and test it.
>
> + linux-media
>
> Laurent, are you ok with the below RZ/G2L specific patch[1] for
> enabling/disabling frame end interrupt in VSP driver?
> Note:
> I need to add a quirk for handling this only for RZ/G2L family as
> other SoCs have Vblank specific interrupt available in DU.
The DU driver on Gen3 handles vblank exactly as in your patch. What's
the problem with that ?
> [1]
>
> diff --git a/drivers/media/platform/renesas/vsp1/vsp1_drm.c
> b/drivers/media/platform/renesas/vsp1/vsp1_drm.c
> index 9b087bd8df7d..39347c16bb27 100644
> --- a/drivers/media/platform/renesas/vsp1/vsp1_drm.c
> +++ b/drivers/media/platform/renesas/vsp1/vsp1_drm.c
> @@ -936,6 +936,14 @@ void vsp1_du_unmap_sg(struct device *dev, struct
> sg_table *sgt)
> }
> EXPORT_SYMBOL_GPL(vsp1_du_unmap_sg);
>
> +void vsp1_du_mask_frame_end_interrupt(struct device *dev, bool mask)
> +{
> + struct vsp1_device *vsp1 = dev_get_drvdata(dev);
> +
> + vsp1_write(vsp1, VI6_WPF_IRQ_ENB(0), mask ? 0 : VI6_WPF_IRQ_ENB_DFEE);
That will break everything. As soon as you turn of vblank reporting, the
VSP will stop processing frames and the display will freeze.
> +}
> +EXPORT_SYMBOL_GPL(vsp1_du_mask_frame_end_interrupt);
> +
> /*
> -
> * Initialization
> */
> diff --git a/include/media/vsp1.h b/include/media/vsp1.h
> index 48f4a5023d81..ccac48a6bdd2 100644
> --- a/include/media/vsp1.h
> +++ b/include/media/vsp1.h
> @@ -117,4 +117,6 @@ void vsp1_du_atomic_flush(struct device *dev, unsigned
> int pipe_index,
> int vsp1_du_map_sg(struct device *dev, struct sg_table *sgt);
> void vsp1_du_unmap_sg(struct device *dev, struct sg_table *sgt);
>
> +void vsp1_du_mask_frame_end_interrupt(struct device *dev, bool mask);
> +
> #endif /* __MEDIA_VSP1_H__ */
--
Regards,
Laurent Pinchart
Re: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support
Hi Maxime, On Wed, Jan 10, 2024 at 05:14:41PM +0100, Maxime Ripard wrote: > On Thu, Jan 04, 2024 at 02:34:33PM +, Biju Das wrote: > > On Friday, December 15, 2023 2:24 PM, Maxime Ripard wrote: > > > On Fri, Dec 15, 2023 at 01:25:48PM +, Biju Das wrote: > > > > On Friday, December 15, 2023 10:24 AM, Maxime Ripard wrote: > > > > > On Thu, Dec 14, 2023 at 03:24:17PM +, Biju Das wrote: > > > > > > Hi Maxime Ripard, > > > > > > > > > > > > Thanks for the feedback. > > > > > > > > > > Thanks, that's super helpful. The architecture is thus similar to > > > > > vc4 > > > > > > > > > > Some general questions related to bugs we had at some point with vc4: > > > > > > > > > > * Where is the display list stored? In RAM or in a dedicated SRAM? > > > > > > > > [1] It is in DDR (RAM). > > > > > > > > > > > > > > * Are the pointer to the current display list latched? > > > > > > > > > > * Is the display list itself latched? If it's not, what happens when > > > > > the display list is changed while the frame is being generated? > > > > > > > > There is some protocol defined for SW side and HW side for updating > > > > display list See [1] 33.4.8.1 Operation flow of VSPD and DU. > > > > > > > > All the display list operations are manged here[2] > > > > > > > > [1] > > > > https://www.renesas.com/us/en/document/mah/rzg2l-group-rzg2lc-group-users-manual-hardware-0 > > > > > > > > [2] > > > > https://elixir.bootlin.com/linux/v6.7-rc5/source/drivers/media/platform/renesas/vsp1/vsp1_dl.c#L863 > > > > > > I'm sorry, but I'm not going to read a 3500+ to try to figure it out. > > > Could you answer the questions above? > > > > The answer for your question is, > > > > If a previous display list has been queued to the hardware but not > > processed yet, the VSP can start processing it at any time. In that > > case we can't replace the queued list by the new one, as we could > > race with the hardware. We thus mark the update as pending, it will > > be queued up to the hardware by the frame end interrupt handler. > > Ok, so you need to make sure that the list entries are allocated and > tied to the state. That way, you'll know for sure it'll get destroyed > only once the state isn't used anymore, so after the vblank. Because the VSP started as a memory-to-memory processing IP without being tied to the display, it got supported by a V4L2 (and MC) driver. Later on, the R-Car Gen2 added a direct connection between the output of *some* VSP instances and one input of the DU (display engine), using the VSP as an extra "plane" from a KMS point of view. Using the VSP was optional, as the DU had "normal" planes. R-Car Gen3 dropped support for direct memory access in the DU, making usage of the VSP mandatory. As using the VSP is mandatory for display operation on R-Car Gen3, we ruled out forcing userspace to deal with a KMS *and* a V4L2 device manually to get anything out on the screen. We also ruled out redeveloping VSP support inside the DU driver, as that would have duplicated (complex) code. Instead, the DU driver communicates with the VSP driver within the kernel, completely transparently for userspace. This in-kernel API is defined in include/media/vsp1.h, and consists of the following operations that have been modelled on the KMS atomic API: - One-time setup at driver initialization time: int vsp1_du_init(struct device *dev); - Configuration at CRTC enable/disable time: int vsp1_du_setup_lif(struct device *dev, unsigned int pipe_index, const struct vsp1_du_lif_config *cfg); This includes configuration of the output width/height. (LIF stands for "LCD InterFace" if I recall correctly, it's the block in the VSP that connects to the DU.) - Atomic updates void vsp1_du_atomic_begin(struct device *dev, unsigned int pipe_index); int vsp1_du_atomic_update(struct device *dev, unsigned int pipe_index, unsigned int rpf, const struct vsp1_du_atomic_config *cfg); void vsp1_du_atomic_flush(struct device *dev, unsigned int pipe_index, const struct vsp1_du_atomic_pipe_config *cfg); These operations configure planes (the VSP has a blending engine with 2 to 5 inputs depending on the exact SoC) and writeback (the VSP being historically just a memory-to-memory engine, it supports writing the output to memory in addition to forwarding it to the DU). The display lists are fully handled inside the VSP driver, the DU doesn't need to manage them. You can ignore the implementation details of the VSP itself. -- Regards, Laurent Pinchart
Re: [PATCH v3 2/2] drm/bridge: imx: add driver for HDMI TX Parallel Video Interface
On Fri, Dec 15, 2023 at 05:09:41PM -0300, Fabio Estevam wrote: > On Fri, Dec 15, 2023 at 4:01 PM Adam Ford wrote: > > > Thanks for the list. I was able to successfully build the stable 6.6 > > branch with those patches applied and I have the HDMI working. > > Unfortunately, I get build errors on the linux-next, so it's going to > > take me a little time to sort through all of this. > > If you need help to figure this problem out, please let me know. > > I haven't tried it against linux-next. > > > I am thinking that it would be better to consolidate all these > > together into one series instead of piecemealing it. However, there > > are some items that can be submitted right away without significantly > > reworking them against linux-next. Do people have a preference? > > I think it makes sense to re-submit the "easy ones" right away. Agreed. The more we can merge quickly, the easier it will become to rebase and upstream the rest. -- Regards, Laurent Pinchart
Re: [PATCH v2] drm: rcar-du: Fix memory leak in rcar_du_vsps_init()
Hi Biju,
Thank you for the patch.
On Thu, Nov 16, 2023 at 12:24:24PM +, Biju Das wrote:
> The rcar_du_vsps_init() doesn't free the np allocated by
> of_parse_phandle_with_fixed_args() for the non-error case.
>
> Fix memory leak for the non-error case.
>
> While at it, replace the label 'error'->'done' as it applies to non-error
> case as well and update the error check condition for rcar_du_vsp_init()
> to avoid breakage in future, if it returns positive value.
>
> Fixes: 3e81374e2014 ("drm: rcar-du: Support multiple sources from the same
> VSP")
> Signed-off-by: Biju Das
Reviewed-by: Laurent Pinchart
> ---
> v1->v2:
> * Replaced the label 'error'->'done' as it applies to non-error case as
>well.
> * Update the error check condition for rcar_du_vsp_init() to avoid
>breakage in future, if it returns positive value.
> ---
> drivers/gpu/drm/renesas/rcar-du/rcar_du_kms.c | 10 --
> 1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/renesas/rcar-du/rcar_du_kms.c
> b/drivers/gpu/drm/renesas/rcar-du/rcar_du_kms.c
> index 70d8ad065bfa..4c8fe83dd610 100644
> --- a/drivers/gpu/drm/renesas/rcar-du/rcar_du_kms.c
> +++ b/drivers/gpu/drm/renesas/rcar-du/rcar_du_kms.c
> @@ -705,7 +705,7 @@ static int rcar_du_vsps_init(struct rcar_du_device *rcdu)
> ret = of_parse_phandle_with_fixed_args(np, vsps_prop_name,
> cells, i, );
> if (ret < 0)
> - goto error;
> + goto done;
>
> /*
>* Add the VSP to the list or update the corresponding existing
> @@ -743,13 +743,11 @@ static int rcar_du_vsps_init(struct rcar_du_device
> *rcdu)
> vsp->dev = rcdu;
>
> ret = rcar_du_vsp_init(vsp, vsps[i].np, vsps[i].crtcs_mask);
> - if (ret < 0)
> - goto error;
> + if (ret)
> + goto done;
> }
>
> - return 0;
> -
> -error:
> +done:
> for (i = 0; i < ARRAY_SIZE(vsps); ++i)
> of_node_put(vsps[i].np);
>
--
Regards,
Laurent Pinchart
Re: [PATCH v3 2/2] drm/bridge: imx: add driver for HDMI TX Parallel Video Interface
On Fri, Dec 15, 2023 at 10:31:27AM -0300, Fabio Estevam wrote: > On Sun, Dec 10, 2023 at 2:35 PM Adam Ford wrote: > > > Lucas, > > > > It's been a few months since there has been any action. If you want, > > I can help apply the suggestions that Laurent has and re-submit with > > both of our names if you want. It would be nice to get this > > integrated. > > It would be nice if you could re-submit the series. Yes, that would be nice. It shouldn't cause any issue, the patches will retain Lucas' authorship. -- Regards, Laurent Pinchart
Re: [PATCH v15 3/5] drm: renesas: Add RZ/G2L DU Support
> > + struct rzg2l_du_device *rcdu = vsp->dev;
> > + struct platform_device *pdev;
> > + unsigned int num_crtcs = hweight32(crtcs);
> > + unsigned int num_planes = 2;
> > + unsigned int i;
> > + int ret;
> > +
> > + /* Find the VSP device and initialize it. */
> > + pdev = of_find_device_by_node(np);
> > + if (!pdev)
> > + return -ENXIO;
> > +
> > + vsp->vsp = >dev;
> > +
> > + ret = drmm_add_action_or_reset(>ddev, rzg2l_du_vsp_cleanup, vsp);
> > + if (ret < 0)
> > + return ret;
> > +
> > + ret = vsp1_du_init(vsp->vsp);
> > + if (ret < 0)
> > + return ret;
> > +
> > + vsp->planes = kcalloc(num_planes, sizeof(*vsp->planes), GFP_KERNEL);
> > + if (!vsp->planes)
> > + return -ENOMEM;
>
> drmm_kcalloc or drmm_kmalloc_array
>
> > +
> > + for (i = 0; i < num_planes; ++i) {
> > + enum drm_plane_type type = i < num_crtcs
> > +? DRM_PLANE_TYPE_PRIMARY
> > +: DRM_PLANE_TYPE_OVERLAY;
> > + struct rzg2l_du_vsp_plane *plane = >planes[i];
> > +
> > + plane->vsp = vsp;
> > + plane->index = i;
> > + ret = drm_universal_plane_init(>ddev, >plane,
> > + crtcs, _du_vsp_plane_funcs,
> > + rzg2l_du_vsp_formats,
> > + ARRAY_SIZE(rzg2l_du_vsp_formats),
> > + NULL, type, NULL);
> > + if (ret < 0)
> > + return ret;
>
> you need to use drmm variant here too.
--
Regards,
Laurent Pinchart
Re: [PATCH v3 101/108] drm/bridge: ti-sn65dsi86: Make use of devm_pwmchip_alloc() function
On Thu, Nov 23, 2023 at 11:10:18AM +0100, Uwe Kleine-König wrote:
> Hello Laurent,
>
> On Thu, Nov 23, 2023 at 11:46:52AM +0200, Laurent Pinchart wrote:
> > (CC'ing Bartosz)
>
> I'm already in discussion with Bart :-)
>
> > On Tue, Nov 21, 2023 at 02:50:43PM +0100, Uwe Kleine-König wrote:
> > > This prepares the pwm driver of the ti-sn65dsi86 to further changes of
> > > the pwm core outlined in the commit introducing devm_pwmchip_alloc().
> > > There is no intended semantical change and the driver should behave as
> > > before.
> > >
> > > Signed-off-by: Uwe Kleine-König
> > > ---
> > > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 25 -
> > > 1 file changed, 16 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > > b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > > index c45c07840f64..cd40530ffd71 100644
> > > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > > @@ -197,7 +197,7 @@ struct ti_sn65dsi86 {
> > > DECLARE_BITMAP(gchip_output, SN_NUM_GPIOS);
> > > #endif
> > > #if defined(CONFIG_PWM)
> > > - struct pwm_chip pchip;
> > > + struct pwm_chip *pchip;
> >
> > Dynamic allocation with devm_*() isn't the right solution for lifetime
> > issues related to cdev. See my talk at LPC 2022
> > (https://www.youtube.com/watch?v=kW8LHWlJPTU, slides at
> > https://lpc.events/event/16/contributions/1227/attachments/1103/2115/20220914-lpc-devm_kzalloc.pdf),
> > and Bartosz's talk at LPC 2023
> > (https://lpc.events/event/17/contributions/1627/attachments/1258/2725/Linux%20Plumbers%20Conference%202023.pdf).
>
> Once the series is completely applied, the pwm_chip isn't allocated
> using devm_kzalloc any more. You're only looking at an intermediate
> state where I push the broken lifetime tracking from all drivers into a
> single function in the core that is then fixed after all drivers are
> converted to it.
Indeed, I missed that devm_pwm_alloc() got changed later in the series
to not call devm_kzalloc(). The naming is quite unfortunate, a
devm_*_alloc() function really gives a strong hint that the
corresponding cleanup at device remove time will be a free(), not a
put() :-S That's pretty confusing for the readers.
> If you find issues with the complete series applied, please tell me.
One thing I still dislike is forcing drivers to dynamically allocate the
pwm_chip series.
--
Regards,
Laurent Pinchart
Re: [PATCH] drm: renesas: shmobile: Call drm_helper_force_disable_all() at shutdown time
On Tue, Dec 05, 2023 at 02:31:24PM +0100, Geert Uytterhoeven wrote: > On Tue, Dec 5, 2023 at 1:16 PM Laurent Pinchart wrote: > > On Tue, Dec 05, 2023 at 12:30:02PM +0100, Geert Uytterhoeven wrote: > > > From: Douglas Anderson > > > > > > Based on grepping through the source code, this driver appears to be > > > missing a call to drm_atomic_helper_shutdown() at system shutdown time. > > > This is important because drm_helper_force_disable_all() will cause > > > panels to get disabled cleanly which may be important for their power > > > sequencing. Future changes will remove any custom powering off in > > > individual panel drivers so the DRM drivers need to start getting this > > > right. > > > > > > The fact that we should call drm_atomic_helper_shutdown() in the case of > > > OS shutdown comes straight out of the kernel doc "driver instance > > > overview" in drm_drv.c. > > > > > > Suggested-by: Maxime Ripard > > > Signed-off-by: Douglas Anderson > > > Link: > > > https://lore.kernel.org/r/20230901164111.RFT.15.Iaf638a1d4c8b3c307a6192efabb4cbb06b195f15@changeid > > > [geert: s/drm_helper_force_disable_all/drm_atomic_helper_shutdown/] > > > [geert: shmob_drm_remove() already calls drm_atomic_helper_shutdown] > > > Signed-off-by: Geert Uytterhoeven > > > > Reviewed-by: Laurent Pinchart > > Thanks! > > > > Panel-simple does print two new warnings: > > > > > > +panel-simple panel: Skipping disable of already disabled panel > > > +panel-simple panel: Skipping unprepare of already unprepared panel > > > > Have you investigated where this comes from ? > > Meh, I knew I forgot something ;-) > > The panel is unprepared and disabled a first time from shmob_drm's > .shutdown() callback: > > shmob_drm_shutdown > drm_atomic_helper_shutdown > drm_atomic_helper_disable_all > drm_atomic_commit > drm_atomic_helper_commit > commit_tail > drm_atomic_helper_commit_tail > drm_atomic_helper_commit_modeset_disables > disable_outputs > drm_atomic_bridge_chain_disable > drm_panel_disable > drm_atomic_bridge_chain_post_disable > drm_panel_unprepare > > And a second time from simple_panel's .shutdown() callback(): > > panel_simple_platform_shutdown > panel_simple_shutdown > drm_panel_disable > drm_panel_unprepare That looks like what Doug mentioned should be removed in the commit message of this patch (a confirmation would be nice). It should be fine for now. -- Regards, Laurent Pinchart
Re: [PATCH] drm: renesas: shmobile: Call drm_helper_force_disable_all() at shutdown time
Hi Geert and Doug,
Thank you for the patch.
On Tue, Dec 05, 2023 at 12:30:02PM +0100, Geert Uytterhoeven wrote:
> From: Douglas Anderson
>
> Based on grepping through the source code, this driver appears to be
> missing a call to drm_atomic_helper_shutdown() at system shutdown time.
> This is important because drm_helper_force_disable_all() will cause
> panels to get disabled cleanly which may be important for their power
> sequencing. Future changes will remove any custom powering off in
> individual panel drivers so the DRM drivers need to start getting this
> right.
>
> The fact that we should call drm_atomic_helper_shutdown() in the case of
> OS shutdown comes straight out of the kernel doc "driver instance
> overview" in drm_drv.c.
>
> Suggested-by: Maxime Ripard
> Signed-off-by: Douglas Anderson
> Link:
> https://lore.kernel.org/r/20230901164111.RFT.15.Iaf638a1d4c8b3c307a6192efabb4cbb06b195f15@changeid
> [geert: s/drm_helper_force_disable_all/drm_atomic_helper_shutdown/]
> [geert: shmob_drm_remove() already calls drm_atomic_helper_shutdown]
> Signed-off-by: Geert Uytterhoeven
Reviewed-by: Laurent Pinchart
> ---
> Tested on Atmark Techno Armadillo-800-EVA, where the PWM instance
> driving the backlight is now stopped on shutdown.
> Panel-simple does print two new warnings:
>
> +panel-simple panel: Skipping disable of already disabled panel
> +panel-simple panel: Skipping unprepare of already unprepared panel
Have you investigated where this comes from ?
> reboot: System halted
> ---
> drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c | 8
> 1 file changed, 8 insertions(+)
>
> diff --git a/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c
> b/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c
> index bd16d4780c6436c3..a15162be26f259a4 100644
> --- a/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c
> +++ b/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c
> @@ -171,6 +171,13 @@ static void shmob_drm_remove(struct platform_device
> *pdev)
> drm_kms_helper_poll_fini(ddev);
> }
>
> +static void shmob_drm_shutdown(struct platform_device *pdev)
> +{
> + struct shmob_drm_device *sdev = platform_get_drvdata(pdev);
> +
> + drm_atomic_helper_shutdown(>ddev);
> +}
> +
> static int shmob_drm_probe(struct platform_device *pdev)
> {
> struct shmob_drm_platform_data *pdata = pdev->dev.platform_data;
> @@ -274,6 +281,7 @@ static const struct of_device_id shmob_drm_of_table[]
> __maybe_unused = {
> static struct platform_driver shmob_drm_platform_driver = {
> .probe = shmob_drm_probe,
> .remove_new = shmob_drm_remove,
> + .shutdown = shmob_drm_shutdown,
> .driver = {
> .name = "shmob-drm",
> .of_match_table = of_match_ptr(shmob_drm_of_table),
--
Regards,
Laurent Pinchart
Re: [PATCH] drm: improve the documentation of connector hpd ops
Hi Abhinav,
Thank you for the patch (and thank to Dmitry for pinging me on IRC, this
patch got burried in my inbox).
On Wed, Sep 20, 2023 at 01:13:58PM -0700, Abhinav Kumar wrote:
> While making the changes in [1], it was noted that the documentation
> of the enable_hpd() and disable_hpd() does not make it clear that
> these ops should not try to do hpd state maintenance and should only
> attempt to enable/disable hpd related hardware for the connector.
s/attempt to //
>
> The state management of these calls to make sure these calls are
> balanced is handled by the DRM core and we should keep it that way
> to minimize the overhead in the drivers which implement these ops.
>
> [1]: https://patchwork.freedesktop.org/patch/558387/
>
You could add a
Suggested-by: Laurent Pinchart
> Signed-off-by: Abhinav Kumar
Reviewed-by: Laurent Pinchart
> ---
> include/drm/drm_modeset_helper_vtables.h | 10 ++
> 1 file changed, 10 insertions(+)
>
> diff --git a/include/drm/drm_modeset_helper_vtables.h
> b/include/drm/drm_modeset_helper_vtables.h
> index e3c3ac615909..a33cf7488737 100644
> --- a/include/drm/drm_modeset_helper_vtables.h
> +++ b/include/drm/drm_modeset_helper_vtables.h
> @@ -1154,6 +1154,11 @@ struct drm_connector_helper_funcs {
>* This operation is optional.
>*
>* This callback is used by the drm_kms_helper_poll_enable() helpers.
> + *
> + * This operation does not need to perform any hpd state tracking as
> + * the DRM core handles that maintenance and ensures the calls to enable
> + * and disable hpd are balanced.
> + *
>*/
> void (*enable_hpd)(struct drm_connector *connector);
>
> @@ -1165,6 +1170,11 @@ struct drm_connector_helper_funcs {
>* This operation is optional.
>*
>* This callback is used by the drm_kms_helper_poll_disable() helpers.
> + *
> + * This operation does not need to perform any hpd state tracking as
> + * the DRM core handles that maintenance and ensures the calls to enable
> + * and disable hpd are balanced.
> + *
>*/
> void (*disable_hpd)(struct drm_connector *connector);
> };
--
Regards,
Laurent Pinchart
Re: [PATCH 1/3] drm/bridge: ti-sn65dsi86: Simplify using pm_runtime_resume_and_get()
Hi Uwe,
On Wed, Nov 29, 2023 at 10:51:37AM +0100, Uwe Kleine-König wrote:
> On Wed, Nov 29, 2023 at 02:39:55AM +0200, Laurent Pinchart wrote:
> > On Thu, Nov 23, 2023 at 06:54:27PM +0100, Uwe Kleine-König wrote:
> > > pm_runtime_resume_and_get() already drops the runtime PM usage counter
> > > in the error case. So a call to pm_runtime_put_sync() can be dropped.
> > >
> > > Signed-off-by: Uwe Kleine-König
> >
> > I wonder if checkpatch should warn about usage of pm_runtime_get_sync().
>
> It should not warn in general. There are cases where
> pm_runtime_get_sync() is the right function to use. See for example
Sure, the function most likely has some valid use cases (otherwise it
should just be removed), but I think those are are a minority. I was
just thinking out loud anyway.
> commit aec488051633 ("crypto: stm32 - Properly handle pm_runtime_get
> failing").
I don't know much about that device, but wouldn't the best option be to
avoid resuming the device at remove time ? In any case, that's getting
out of topic for the sn65dsi86 :-)
--
Regards,
Laurent Pinchart
Re: [PATCH 0/3] drm/bridge: ti-sn65dsi86: Some updates
Hello,
On Fri, Nov 24, 2023 at 09:56:55AM +0100, Neil Armstrong wrote:
> On 23/11/2023 18:54, Uwe Kleine-König wrote:
> > Hello,
> >
> > this is a series I created while starring at the ti-sn65dsi86 driver in
> > the context of my pwm-lifetime series.
> >
> > The first patch should be fine. The last one has a few rough edges, but
> > maybe you like the direction this is going to? The 2nd patch probably
> > only makes sense if you also take the third.
> >
> > Best regards
> > Uwe
> >
> > Uwe Kleine-König (3):
> >drm/bridge: ti-sn65dsi86: Simplify using pm_runtime_resume_and_get()
> >drm/bridge: ti-sn65dsi86: Change parameters of
> > ti_sn65dsi86_{read,write}_u16
> >drm/bridge: ti-sn65dsi86: Loosen coupling of PWM to ti-sn65dsi86 core
> >
> > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 146 +++---
> > 1 file changed, 83 insertions(+), 63 deletions(-)
> >
> > base-commit: 4e87148f80d198ba5febcbcc969c6b9471099a09
>
> It looks fine to me, even without the goal to move the driver to drivers/pwm
> I think it's same to move the pwm ddata out of the main pdata ans associate
> it to the pwm aux device lifetime.
>
> I don't anything wrong, and so far it's of for me, let's see if there's
> comments
> for other people before applying!
I like 1/3 very much, but as mentioned in a reply to 3/3, I'm not
convinced by that one at all. Not only does it make the driver more
complex for, I believe, very little gain (if any), usage of
devm_kzalloc() in ti_sn_pwm_probe() is most likely wrong. Lifetime of
driver-specific structures need to be handled through reference
counting.
--
Regards,
Laurent Pinchart
Re: [PATCH 2/3] drm/bridge: ti-sn65dsi86: Change parameters of ti_sn65dsi86_{read,write}_u16
On Tue, Nov 28, 2023 at 04:34:30PM -0800, Doug Anderson wrote:
> On Thu, Nov 23, 2023 at 9:54 AM Uwe Kleine-König wrote:
> >
> > This aligns the function's parameters to regmap_{read,write} and
> > simplifies the next change that takes pwm driver data out of struct
> > ti_sn65dsi86.
> >
> > Signed-off-by: Uwe Kleine-König
> > ---
> > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 20 ++--
> > 1 file changed, 10 insertions(+), 10 deletions(-)
>
> I'm on the fence for this one. It almost feels like if this takes a
> "regmap" as the first field then it should be part of the regmap API.
> Adding a concept like this to the regmap API might be interesting if
> there were more than one user, but that's a pretty big yak to shave.
See include/media/v4l2-cci.h and drivers/media/v4l2-core/v4l2-cci.c. We
could discuss moving it to regmap.
> I'd tend to agree with your statement in the cover letter that this
> patch really makes more sense if we were to take patch #3, and (as per
> my response there) I'm not convinced.
Likewise :-) 1/3 is good, but without 3/3, I'm not conviced by 2/3.
> That being said, similar to patch #3 if everything else thinks this is
> great then I won't stand in the way.
--
Regards,
Laurent Pinchart
Re: [PATCH 1/3] drm/bridge: ti-sn65dsi86: Simplify using pm_runtime_resume_and_get()
Hi Uwe,
Thank you for the patch.
On Thu, Nov 23, 2023 at 06:54:27PM +0100, Uwe Kleine-König wrote:
> pm_runtime_resume_and_get() already drops the runtime PM usage counter
> in the error case. So a call to pm_runtime_put_sync() can be dropped.
>
> Signed-off-by: Uwe Kleine-König
I wonder if checkpatch should warn about usage of pm_runtime_get_sync().
Reviewed-by: Laurent Pinchart
> ---
> drivers/gpu/drm/bridge/ti-sn65dsi86.c | 6 ++
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index c45c07840f64..5b8e1dfc458d 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -1413,11 +1413,9 @@ static int ti_sn_pwm_apply(struct pwm_chip *chip,
> struct pwm_device *pwm,
> int ret;
>
> if (!pdata->pwm_enabled) {
> - ret = pm_runtime_get_sync(pdata->dev);
> - if (ret < 0) {
> - pm_runtime_put_sync(pdata->dev);
> + ret = pm_runtime_resume_and_get(pdata->dev);
> + if (ret < 0)
> return ret;
> - }
> }
>
> if (state->enabled) {
--
Regards,
Laurent Pinchart
Re: [PATCH 3/3] drm/bridge: ti-sn65dsi86: Loosen coupling of PWM to ti-sn65dsi86 core
On Tue, Nov 28, 2023 at 04:32:10PM -0800, Doug Anderson wrote: > On Thu, Nov 23, 2023 at 9:54 AM Uwe Kleine-König wrote: > > > > Introduce a dedicated private data structure for the pwm aux driver > > provided by the sn65dsi86 driver. This way data needed for PWM operation > > is (to a certain degree) nicely separated and doesn't occupy memory in > > the ti_sn65dsi86 core's private data if the PWM isn't used. > > I suspect we still end up at a loss memory-wise. All of the extra code > + the overhead of another kmalloc seems like it would take up more > space than the tiny bit of data in the structure. > > > > The eventual goal is to decouple the PWM driver completely from the > > ti-sn65dsi86 core and maybe even move it to a dedicated driver below > > drivers/pwm. There are a few obstacles to that quest though: > > > > - The busy pin check (implemented in ti_sn_pwm_pin_request() and > >ti_sn_pwm_pin_release()) would need to be available unconditionally. > > > > - The refclk should probably abstracted by a struct clk such that the > >pwm_refclk_freq member that currently still lives in ti-sn65dsi86 > >core driver data can be dropped. > > Right that the above could be done with more abstraction layers. I > guess the question I have is: how much do we gain with that? > > Personally I'm not really sold on the idea. If others think this is a > great change then I won't stand in the way, but IMO without a > compelling reason this is just extra abstraction / complexity without > any gain... I'm not convinced either, especially on moving to a separate driver, but even when it comes to dynamically allocating a new structure. Splitting the PWM fields to a new ti_sn65dsi86_pwm would be fine (and I think would increase readibility), but we can then simply embed it in ti_sn65dsi86. > > +/* > > + * struct ti_sn65dsi86_pwm_ddata - Platform data for ti-sn65dsi86 pwm > > driver. > > Why "ddata" exactly? It seems like this is just the pwm "data" ? > > > > + * @chip: pwm_chip if the PWM is exposed. > > + * @pwm_enabled: Used to track if the PWM signal is currently enabled. > > + * @regmap: Regmap for accessing i2c. > > + * @pdata: platform data of the parent device > > "pdata" isn't a member of the struct, but "pwm_refclk_freq" is. -- Regards, Laurent Pinchart
