Re: [PATCH] drm/tidss: Add MIT license along with GPL-2.0
/tidss_drv.c > b/drivers/gpu/drm/tidss/tidss_drv.c > index d15f836dca95..b060e420ddec 100644 > --- a/drivers/gpu/drm/tidss/tidss_drv.c > +++ b/drivers/gpu/drm/tidss/tidss_drv.c > @@ -1,4 +1,4 @@ > -// SPDX-License-Identifier: GPL-2.0 > +// SPDX-License-Identifier: GPL-2.0 OR MIT > /* > * Copyright (C) 2018 Texas Instruments Incorporated - https://www.ti.com/ > * Author: Tomi Valkeinen > diff --git a/drivers/gpu/drm/tidss/tidss_drv.h > b/drivers/gpu/drm/tidss/tidss_drv.h > index d7f27b0b0315..d4209234f59c 100644 > --- a/drivers/gpu/drm/tidss/tidss_drv.h > +++ b/drivers/gpu/drm/tidss/tidss_drv.h > @@ -1,4 +1,4 @@ > -/* SPDX-License-Identifier: GPL-2.0 */ > +/* SPDX-License-Identifier: GPL-2.0 OR MIT */ > /* > * Copyright (C) 2018 Texas Instruments Incorporated - https://www.ti.com/ > * Author: Tomi Valkeinen > diff --git a/drivers/gpu/drm/tidss/tidss_encoder.c > b/drivers/gpu/drm/tidss/tidss_encoder.c > index 17a86bed8054..9749fbc0e056 100644 > --- a/drivers/gpu/drm/tidss/tidss_encoder.c > +++ b/drivers/gpu/drm/tidss/tidss_encoder.c > @@ -1,4 +1,4 @@ > -// SPDX-License-Identifier: GPL-2.0 > +// SPDX-License-Identifier: GPL-2.0 OR MIT > /* > * Copyright (C) 2018 Texas Instruments Incorporated - https://www.ti.com/ > * Author: Tomi Valkeinen > diff --git a/drivers/gpu/drm/tidss/tidss_encoder.h > b/drivers/gpu/drm/tidss/tidss_encoder.h > index 3e561d6b1e83..85db3835a335 100644 > --- a/drivers/gpu/drm/tidss/tidss_encoder.h > +++ b/drivers/gpu/drm/tidss/tidss_encoder.h > @@ -1,4 +1,4 @@ > -/* SPDX-License-Identifier: GPL-2.0 */ > +/* SPDX-License-Identifier: GPL-2.0 OR MIT */ > /* > * Copyright (C) 2018 Texas Instruments Incorporated - https://www.ti.com/ > * Author: Tomi Valkeinen > diff --git a/drivers/gpu/drm/tidss/tidss_irq.c > b/drivers/gpu/drm/tidss/tidss_irq.c > index 604334ef526a..51939744695a 100644 > --- a/drivers/gpu/drm/tidss/tidss_irq.c > +++ b/drivers/gpu/drm/tidss/tidss_irq.c > @@ -1,4 +1,4 @@ > -// SPDX-License-Identifier: GPL-2.0 > +// SPDX-License-Identifier: GPL-2.0 OR MIT > /* > * Copyright (C) 2018 Texas Instruments Incorporated - https://www.ti.com/ > * Author: Tomi Valkeinen > diff --git a/drivers/gpu/drm/tidss/tidss_irq.h > b/drivers/gpu/drm/tidss/tidss_irq.h > index b512614d5863..cbfd684ecd26 100644 > --- a/drivers/gpu/drm/tidss/tidss_irq.h > +++ b/drivers/gpu/drm/tidss/tidss_irq.h > @@ -1,4 +1,4 @@ > -/* SPDX-License-Identifier: GPL-2.0 */ > +/* SPDX-License-Identifier: GPL-2.0 OR MIT */ > /* > * Copyright (C) 2018 Texas Instruments Incorporated - https://www.ti.com/ > * Author: Tomi Valkeinen > diff --git a/drivers/gpu/drm/tidss/tidss_kms.c > b/drivers/gpu/drm/tidss/tidss_kms.c > index f371518f8697..05afd57b9128 100644 > --- a/drivers/gpu/drm/tidss/tidss_kms.c > +++ b/drivers/gpu/drm/tidss/tidss_kms.c > @@ -1,4 +1,4 @@ > -// SPDX-License-Identifier: GPL-2.0 > +// SPDX-License-Identifier: GPL-2.0 OR MIT > /* > * Copyright (C) 2018 Texas Instruments Incorporated - https://www.ti.com/ > * Author: Tomi Valkeinen > diff --git a/drivers/gpu/drm/tidss/tidss_kms.h > b/drivers/gpu/drm/tidss/tidss_kms.h > index 632d79f5983f..69b6bca14550 100644 > --- a/drivers/gpu/drm/tidss/tidss_kms.h > +++ b/drivers/gpu/drm/tidss/tidss_kms.h > @@ -1,4 +1,4 @@ > -/* SPDX-License-Identifier: GPL-2.0 */ > +/* SPDX-License-Identifier: GPL-2.0 OR MIT */ > /* > * Copyright (C) 2018 Texas Instruments Incorporated - https://www.ti.com/ > * Author: Tomi Valkeinen > diff --git a/drivers/gpu/drm/tidss/tidss_plane.c > b/drivers/gpu/drm/tidss/tidss_plane.c > index a5d86822c9e3..37ffaea15c73 100644 > --- a/drivers/gpu/drm/tidss/tidss_plane.c > +++ b/drivers/gpu/drm/tidss/tidss_plane.c > @@ -1,4 +1,4 @@ > -// SPDX-License-Identifier: GPL-2.0 > +// SPDX-License-Identifier: GPL-2.0 OR MIT > /* > * Copyright (C) 2018 Texas Instruments Incorporated - https://www.ti.com/ > * Author: Tomi Valkeinen > diff --git a/drivers/gpu/drm/tidss/tidss_plane.h > b/drivers/gpu/drm/tidss/tidss_plane.h > index e933e158b617..3e00bc853813 100644 > --- a/drivers/gpu/drm/tidss/tidss_plane.h > +++ b/drivers/gpu/drm/tidss/tidss_plane.h > @@ -1,4 +1,4 @@ > -/* SPDX-License-Identifier: GPL-2.0 */ > +/* SPDX-License-Identifier: GPL-2.0 OR MIT */ > /* > * Copyright (C) 2018 Texas Instruments Incorporated - https://www.ti.com/ > * Author: Tomi Valkeinen > diff --git a/drivers/gpu/drm/tidss/tidss_scale_coefs.c > b/drivers/gpu/drm/tidss/tidss_scale_coefs.c > index c2b84fea89a5..686ea63e0f45 100644 > --- a/drivers/gpu/drm/tidss/tidss_scale_coefs.c > +++ b/drivers/gpu/drm/tidss/tidss_scale_coefs.c > @@ -1,4 +1,4 @@ > -// SPDX-License-Identifier: GPL-2.0 > +// SPDX-License-Identifier: GPL-2.0 OR MIT > /* > * Copyright (C) 2018 Texas Instruments Incorporated - https://www.ti.com/ > * Author: Jyri Sarha > diff --git a/drivers/gpu/drm/tidss/tidss_scale_coefs.h > b/drivers/gpu/drm/tidss/tidss_scale_coefs.h > index 9c560d0fdac0..4689109fe560 100644 > --- a/drivers/gpu/drm/tidss/tidss_scale_coefs.h > +++ b/drivers/gpu/drm/tidss/tidss_scale_coefs.h > @@ -1,4 +1,4 @@ > -/* SPDX-License-Identifier: GPL-2.0 */ > +/* SPDX-License-Identifier: GPL-2.0 OR MIT */ > /* > * Copyright (C) 2018 Texas Instruments Incorporated - https://www.ti.com/ > * Author: Jyri Sarha -- Regards, Laurent Pinchart
Re: [PATCH] drm/tidss: Add MIT license along with GPL-2.0
On Thu, Sep 12, 2024 at 09:21:19PM +0300, Tomi Valkeinen wrote: > On 12/09/2024 21:08, Maxime Ripard wrote: > > On Thu, Sep 12, 2024 at 06:04:11PM GMT, Maxime Ripard wrote: > >> On Thu, 12 Sep 2024 22:41:42 +0530, Devarsh Thakkar wrote: > >>> Modify license to include dual licensing as GPL-2.0-only OR MIT license > >>> for > >>> tidss display driver. This allows other operating system ecosystems such > >>> as > >>> Zephyr and also the commercial firmwares to refer and derive code from > >>> this > >>> display driver in a more permissive manner. > >>> > >>> > >>> [ ... ] > >> > >> Acked-by: Maxime Ripard > > > > Also, we need the ack of all contributors to that driver, so my ack > > isn't enough to merge that patch. > > IANAL, maybe a silly question: if someone from company XYZ has sent a > patch for tidss, don't we then need an ack from someone in XYZ who's > high enough in XYZ to allow changing the license for their code? This patch needs to be acked by all copyright holders indeed. For contributions whose copyright has been assigned to other entities (such as work performed by employees for their employers), those other entities have to ack the license change. What constitutes a substantial enough contribution to be copyrightable is a question I won't attempt to answer. -- Regards, Laurent Pinchart
Re: [PATCH 2/8] drm: renesas: rcar-du: annotate rcar_cmm_read() with __maybe_unused
On Tue, Sep 10, 2024 at 04:36:01PM +0300, Jani Nikula wrote:
> On Tue, 10 Sep 2024, Geert Uytterhoeven wrote:
> > Hi Jani,
> >
> > On Tue, Sep 10, 2024 at 12:06 PM Jani Nikula wrote:
> >> Building with clang and and W=1 leads to warning about unused
> >> rcar_cmm_read(). Fix by annotating it with __maybe_unused.
> >>
> >> See also commit 6863f5643dd7 ("kbuild: allow Clang to find unused static
> >> inline functions for W=1 build").
> >>
> >> Signed-off-by: Jani Nikula
> >
> > Thanks for your patch!
> >
> >> --- a/drivers/gpu/drm/renesas/rcar-du/rcar_cmm.c
> >> +++ b/drivers/gpu/drm/renesas/rcar-du/rcar_cmm.c
> >> @@ -32,7 +32,7 @@ struct rcar_cmm {
> >> } lut;
> >> };
> >>
> >> -static inline int rcar_cmm_read(struct rcar_cmm *rcmm, u32 reg)
> >> +static inline __maybe_unused int rcar_cmm_read(struct rcar_cmm *rcmm, u32
> >> reg)
> >> {
> >> return ioread32(rcmm->base + reg);
> >> }
> >
> > This function was never used. Why not remove it instead?
>
> Can do if that's what the maintainers desire. It's just that sometimes
> it's better to have the implementation reviewed and ready waiting for
> the users than requiring the first user to add the implementation. I
> opted for __maybe_unused across the series.
Jiapeng Chong has sent a patch to drop the function, and I've reviewed
it. See
https://lore.kernel.org/r/20240619075436.86407-1-jiapeng.ch...@linux.alibaba.com
I've sent a pull request for v6.12 but it hasn't been processed in time
:-( See
https://lore.kernel.org/r/20240822234445.ga23...@pendragon.ideasonboard.com
--
Regards,
Laurent Pinchart
Re: [GIT PULL FOR v6.12] Miscellaneous small fixes for Renesas DRM drivers
Did this one fall through the cracks ? :-( On Fri, Aug 23, 2024 at 02:44:45AM +0300, Laurent Pinchart wrote: > Hello Dave, Sima, > > The following changes since commit 11df68c265460d4dff5d19a1313f0fff69470f98: > > Merge tag 'drm-misc-next-2024-08-16' of > https://gitlab.freedesktop.org/drm/misc/kernel into drm-next (2024-08-22 > 09:42:23 +0200) > > are available in the Git repository at: > > git://git.kernel.org/pub/scm/linux/kernel/git/pinchartl/linux.git > tags/drm-next-20240823 > > for you to fetch changes up to caef454889d150d7267992f0d95fbc25dfb621a4: > > drm: renesas: cmm: Remove unused function rcar_cmm_write (2024-08-23 > 02:30:26 +0300) > > > Miscellaneous small fixes for Renesas DRM drivers > > > Biju Das (2): > drm: renesas: rcar-du: rzg2l_mipi_dsi: Update the comment in > rzg2l_mipi_dsi_start_video() > drm: rcar-du: Fix memory leak in rcar_du_vsps_init() > > Jiapeng Chong (1): > drm: renesas: cmm: Remove unused function rcar_cmm_write > > drivers/gpu/drm/renesas/rcar-du/rcar_cmm.c | 5 - > drivers/gpu/drm/renesas/rcar-du/rcar_du_kms.c| 10 -- > drivers/gpu/drm/renesas/rcar-du/rzg2l_mipi_dsi.c | 2 +- > 3 files changed, 5 insertions(+), 12 deletions(-) -- Regards, Laurent Pinchart
Re: [PATCH] docs: devicetree: Fix typo in lvds.yaml
Hi Yu-Chun, Thank you for the patch. On Sun, Sep 01, 2024 at 09:30:46PM +0800, Yu-Chun Lin wrote: > Corrected the spelling in the description of LVDS Display Common > Properties. > > Signed-off-by: Yu-Chun Lin Reviewed-by: Laurent Pinchart > --- > Documentation/devicetree/bindings/display/lvds.yaml | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/display/lvds.yaml > b/Documentation/devicetree/bindings/display/lvds.yaml > index 224db4932011..b74efbea3be2 100644 > --- a/Documentation/devicetree/bindings/display/lvds.yaml > +++ b/Documentation/devicetree/bindings/display/lvds.yaml > @@ -16,7 +16,7 @@ maintainers: > description: >This binding extends the data mapping defined in lvds-data-mapping.yaml. >It supports reversing the bit order on the formats defined there in order > - to accomodate for even more specialized data formats, since a variety of > + to accommodate for even more specialized data formats, since a variety of >data formats and layouts is used to drive LVDS displays. > > properties: -- Regards, Laurent Pinchart
Re: [PATCH v2] drm: rcar-du: Fix memory leak in rcar_du_vsps_init()
On Wed, Aug 28, 2024 at 07:21:32AM +, Biju Das wrote:
> On Tuesday, August 27, 2024 11:23 PM, Laurent Pinchart wrote:
> > Subject: Re: [PATCH v2] drm: rcar-du: Fix memory leak in rcar_du_vsps_init()
> >
> > Hi Biju,
> >
> > On Tue, Aug 27, 2024 at 04:43:12PM +, Biju Das wrote:
> > > Hi Laurent and Tomi,
> > >
> > > Gentle ping. Are you happy with this patch?
> >
> > I've sent a pull request last Friday, see
> > https://lore.kernel.org/r/20240822234445.ga23...@pendragon.ideasonboard.com
>
> I believe the commit[1] won't apply any more as the source location is moved.
> Shall I resend the patch or You will take care, please let me know.
>
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/pinchartl/linux.git/commit/?h=drm-next-20240823&id=866dfbc953d46cf7682c6a434a4dd6249677fd68
I think git will be able to handle that nicely.
> > > > -Original Message-
> > > > From: Laurent Pinchart
> > > > Sent: Monday, December 18, 2023 10:39 AM
> > > > Subject: 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, &args);
> > > > > 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 -next] drm: zynqmp_dp: Use devm_platform_ioremap_resource_byname()
Hi Jinjie,
Thank you for the patch.
On Wed, Aug 28, 2024 at 04:49:29PM +0800, Jinjie Ruan wrote:
> platform_get_resource_byname() and devm_ioremap_resource() can be
> replaced by devm_platform_ioremap_resource_byname(), which can
> simplify the code logic a bit, No functional change here.
>
> Signed-off-by: Jinjie Ruan
Reviewed-by: Laurent Pinchart
Tomi, feel free to push this to drm-misc.
> ---
> drivers/gpu/drm/xlnx/zynqmp_dp.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c
> b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> index 129beac4c073..4eb19ac95bdb 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> @@ -1715,7 +1715,6 @@ int zynqmp_dp_probe(struct zynqmp_dpsub *dpsub)
> struct platform_device *pdev = to_platform_device(dpsub->dev);
> struct drm_bridge *bridge;
> struct zynqmp_dp *dp;
> - struct resource *res;
> int ret;
>
> dp = kzalloc(sizeof(*dp), GFP_KERNEL);
> @@ -1729,8 +1728,7 @@ int zynqmp_dp_probe(struct zynqmp_dpsub *dpsub)
> INIT_WORK(&dp->hpd_work, zynqmp_dp_hpd_work_func);
>
> /* Acquire all resources (IOMEM, IRQ and PHYs). */
> - res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dp");
> - dp->iomem = devm_ioremap_resource(dp->dev, res);
> + dp->iomem = devm_platform_ioremap_resource_byname(pdev, "dp");
> if (IS_ERR(dp->iomem)) {
> ret = PTR_ERR(dp->iomem);
> goto err_free;
--
Regards,
Laurent Pinchart
Re: [PATCH 1/2] dt-bindings: display: renesas,du: narrow interrupts and resets per variants
On Wed, Aug 28, 2024 at 02:52:25PM +0200, Krzysztof Kozlowski wrote: > On 28/08/2024 14:45, Laurent Pinchart wrote: > > On Sun, Aug 18, 2024 at 08:48:54PM +0200, Krzysztof Kozlowski wrote: > >> On 18/08/2024 19:51, Laurent Pinchart wrote: > >>> On Sun, Aug 18, 2024 at 07:44:22PM +0200, Krzysztof Kozlowski wrote: > >>>> On 18/08/2024 19:41, Laurent Pinchart wrote: > >>>>> On Sun, Aug 18, 2024 at 07:30:02PM +0200, Krzysztof Kozlowski wrote: > >>>>>> Each variable-length property like interrupts or resets must have fixed > >>>>>> constraints on number of items for given variant in binding. The > >>>>>> clauses in "if:then:" block should define both limits: upper and lower. > >>>>> > >>>>> I thought that, when only one of minItems or maxItems was specified, the > >>>>> other automatically defaulted to the same value. I'm pretty sure I > >>>>> recall Rob asking me to drop one of the two in some bindings. Has the > >>>>> rule changes ? Is it documented somewhere ? > >>>> > >>>> New dtschema changed it and, even if previous behavior is restored, the > >>>> size in if:then: always had to be constrained. You could have skipped > >>>> one side of limit if it was equal to outer/top-level limit, e.g: > >>>> > >>>> properties: > >>>> clocks: > >>>> minItems: 1 > >>>> maxItems: 2 > >>>> > >>>> > >>>> if:then:properties: > >>>> clocks: > >>>> minItems: 2 > >>> > >>> Where can I find a description of the behaviour of the new dtschema > >>> (hopefully with some documentation) ? > >> > >> No clue, but I feel there is some core concept missing. Your earlier > >> statement: > >> "I thought that, when only one of minItems or maxItems was specified, the" > >> > >> was never logically correct for the "if:then", except for the case I > >> mentioned above. That's why all schema used as examples had it explicit: > >> > >> My talk from 2022, page 30: > >> https://static.sched.com/hosted_files/osseu2022/bd/How%20to%20Get%20Your%20DT%20Schema%20Bindings%20Accepted%20in%20Less%20than%2010%20Iterations%20-%20Krzysztof%20Kozlowski%2C%20Linaro.pdf?_gl=1*kmzqmt*_gcl_au*MTU2MzQ1MjY0Mi4xNzIxNzE0NDc1 > >> all constraints defined,. > >> > >> My talk from 2023, page 34: > >> https://static.sched.com/hosted_files/eoss2023/a8/How%20to%20Get%20Your%20DT%20Schema%20Bindings%20Accepted%20in%20Less%20than%2010%20Iterations%20-%20Krzysztof%20Kozlowski%2C%20Linaro%20-%20ELCE%202023.pdf?_gl=1*1jgx6d3*_gcl_au*MTU2MzQ1MjY0Mi4xNzIxNzE0NDc1 > >> > >> Recently, I started using other example as "useful reference": > >> https://elixir.bootlin.com/linux/v6.8/source/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml#L132 > >> > >> That's nothing. All three above reference examples I keep giving are > >> already there and repeated in emails all the time. > >> > >> So aren't you confusing the entire "skip one limit" for top-level > >> properties? This patch is not about it all and dtschema did not change. > > > > There must have been a misunderstanding indeed, I interpreted "New > > dtschema changed it" as meaning there were now new rules. Is that > > incorrect ? > > For the binding with a property defined only in top-level properties: no > changes, no new rules. > > For the binding with top-level and if:then:else: dtschema since few > months changed interpretation. OK, that's what I didn't understand correctly. > > If you don't mind clarifying, what is the current recommendation to > > indicate that a property has a fixed number of items ? Which of the > > following three options is preferred ? > > Answer below assumes we have clocks defined in top-level properties and > there is no if:then:else customizing it. > > > properties: > > clocks: > > minItems: 2 > > That's wrong, because items are unconstrained. > > > properties: > > clocks: > > maxItems: 2 > > This one is preferred. > > > properties: > > clocks: > > minItems: 2 > > maxItems: 2 > > This one is correct, but less preferred. Thank you, that is clear now. -- Regards, Laurent Pinchart
Re: [PATCH 1/2] dt-bindings: display: renesas,du: narrow interrupts and resets per variants
Hi Krzysztof, On Sun, Aug 18, 2024 at 08:48:54PM +0200, Krzysztof Kozlowski wrote: > On 18/08/2024 19:51, Laurent Pinchart wrote: > > On Sun, Aug 18, 2024 at 07:44:22PM +0200, Krzysztof Kozlowski wrote: > >> On 18/08/2024 19:41, Laurent Pinchart wrote: > >>> On Sun, Aug 18, 2024 at 07:30:02PM +0200, Krzysztof Kozlowski wrote: > >>>> Each variable-length property like interrupts or resets must have fixed > >>>> constraints on number of items for given variant in binding. The > >>>> clauses in "if:then:" block should define both limits: upper and lower. > >>> > >>> I thought that, when only one of minItems or maxItems was specified, the > >>> other automatically defaulted to the same value. I'm pretty sure I > >>> recall Rob asking me to drop one of the two in some bindings. Has the > >>> rule changes ? Is it documented somewhere ? > >> > >> New dtschema changed it and, even if previous behavior is restored, the > >> size in if:then: always had to be constrained. You could have skipped > >> one side of limit if it was equal to outer/top-level limit, e.g: > >> > >> properties: > >> clocks: > >> minItems: 1 > >> maxItems: 2 > >> > >> > >> if:then:properties: > >> clocks: > >> minItems: 2 > > > > Where can I find a description of the behaviour of the new dtschema > > (hopefully with some documentation) ? > > No clue, but I feel there is some core concept missing. Your earlier > statement: > "I thought that, when only one of minItems or maxItems was specified, the" > > was never logically correct for the "if:then", except for the case I > mentioned above. That's why all schema used as examples had it explicit: > > My talk from 2022, page 30: > https://static.sched.com/hosted_files/osseu2022/bd/How%20to%20Get%20Your%20DT%20Schema%20Bindings%20Accepted%20in%20Less%20than%2010%20Iterations%20-%20Krzysztof%20Kozlowski%2C%20Linaro.pdf?_gl=1*kmzqmt*_gcl_au*MTU2MzQ1MjY0Mi4xNzIxNzE0NDc1 > all constraints defined,. > > My talk from 2023, page 34: > https://static.sched.com/hosted_files/eoss2023/a8/How%20to%20Get%20Your%20DT%20Schema%20Bindings%20Accepted%20in%20Less%20than%2010%20Iterations%20-%20Krzysztof%20Kozlowski%2C%20Linaro%20-%20ELCE%202023.pdf?_gl=1*1jgx6d3*_gcl_au*MTU2MzQ1MjY0Mi4xNzIxNzE0NDc1 > > Recently, I started using other example as "useful reference": > https://elixir.bootlin.com/linux/v6.8/source/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml#L132 > > That's nothing. All three above reference examples I keep giving are > already there and repeated in emails all the time. > > So aren't you confusing the entire "skip one limit" for top-level > properties? This patch is not about it all and dtschema did not change. There must have been a misunderstanding indeed, I interpreted "New dtschema changed it" as meaning there were now new rules. Is that incorrect ? If you don't mind clarifying, what is the current recommendation to indicate that a property has a fixed number of items ? Which of the following three options is preferred ? properties: clocks: minItems: 2 properties: clocks: maxItems: 2 properties: clocks: minItems: 2 maxItems: 2 -- Regards, Laurent Pinchart
Re: [PATCH v2] drm: rcar-du: Fix memory leak in rcar_du_vsps_init()
Hi Biju,
On Tue, Aug 27, 2024 at 04:43:12PM +, Biju Das wrote:
> Hi Laurent and Tomi,
>
> Gentle ping. Are you happy with this patch?
I've sent a pull request last Friday, see
https://lore.kernel.org/r/20240822234445.ga23...@pendragon.ideasonboard.com
> > -Original Message-----
> > From: Laurent Pinchart
> > Sent: Monday, December 18, 2023 10:39 AM
> > Subject: 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, &args);
> > > 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] drm: renesas: rz-du: Add Kconfig dependency between RZG2L_DU and RZG2L_MIPI_DSI
Hi Biju, Thank you for the patch. On Tue, Aug 27, 2024 at 05:37:24PM +0100, Biju Das wrote: > Add Kconfig dependency between RZG2L_DU and RZG2L_MIPI_DSI, so that > DSI module has functional dependency on DU. It is similar way that > the R-Car MIPI DSI encoder is handled. > > While at it drop ARCH_RENESAS dependency as DRM_RZG2L_DU depend on > ARCH_RZG2L. > > Suggested-by: Tomi Valkeinen > Signed-off-by: Biju Das Reviewed-by: Laurent Pinchart > --- > drivers/gpu/drm/renesas/rz-du/Kconfig | 15 ++- > 1 file changed, 10 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/renesas/rz-du/Kconfig > b/drivers/gpu/drm/renesas/rz-du/Kconfig > index 8ec14271ebba..a9488f873436 100644 > --- a/drivers/gpu/drm/renesas/rz-du/Kconfig > +++ b/drivers/gpu/drm/renesas/rz-du/Kconfig > @@ -11,10 +11,15 @@ config DRM_RZG2L_DU > Choose this option if you have an RZ/G2L alike chipset. > If M is selected the module will be called rzg2l-du-drm. > > -config DRM_RZG2L_MIPI_DSI > - tristate "RZ/G2L MIPI DSI Encoder Support" > - depends on DRM && DRM_BRIDGE && OF > - depends on ARCH_RENESAS || COMPILE_TEST > - select DRM_MIPI_DSI > +config DRM_RZG2L_USE_MIPI_DSI > + bool "RZ/G2L MIPI DSI Encoder Support" > + depends on DRM_BRIDGE && OF > + depends on DRM_RZG2L_DU || COMPILE_TEST > + default DRM_RZG2L_DU > help > Enable support for the RZ/G2L Display Unit embedded MIPI DSI encoders. > + > +config DRM_RZG2L_MIPI_DSI > + def_tristate DRM_RZG2L_DU > + depends on DRM_RZG2L_USE_MIPI_DSI > + select DRM_MIPI_DSI -- Regards, Laurent Pinchart
Re: [PATCH] drm: renesas: Move RZ/G2L MIPI DSI driver to rz-du
On Fri, Aug 23, 2024 at 02:33:49PM +0100, Lad, Prabhakar wrote: > On Wed, Jun 26, 2024 at 6:51 AM Laurent Pinchart wrote: > > On Tue, Jun 25, 2024 at 01:32:44PM +0100, Prabhakar wrote: > > > From: Lad Prabhakar > > > > > > All the RZ/G2L DU specific components are located under the rz-du folder, > > > so it makes sense to move the RZ/G2L MIPI DSI driver there instead of > > > keeping it in the rcar-du folder. This change improves the organization > > > and modularity of the driver configuration by grouping related settings > > > together. > > > > I was thinking the same the other day. Thanks for beating me at sending > > a patch :-) > > > > Reviewed-by: Laurent Pinchart > > > > Do you or Biju has committer rights to drm-misc to push this patch ? > > We dont, can you please queue this patch via your tree? I don't have other pending patches for DRM at the moment. Tomi, could you push this to drm-misc ? -- Regards, Laurent Pinchart
Re: [PATCH v4 0/4] Add support for RZ/G2UL Display Unit
Hi Biju,
On Fri, Aug 23, 2024 at 01:52:14PM +, Biju Das wrote:
> On Friday, August 23, 2024 2:15 PM, Laurent Pinchart wrote:
> > On Thu, Aug 22, 2024 at 05:23:13PM +0100, Biju Das wrote:
> > > This patch series aims to add support for RZ/G2UL DU.
> > >
> > > The LCD controller is composed of Frame Compression Processor (FCPVD),
> > > Video Signal Processor (VSPD), and Display Unit (DU).
> > >
> > > The output of LCDC is connected display parallel interface (DPI) and
> > > supports a maximum resolution of WXGA along with 2 RPFs to support the
> > > blending of two picture layers and raster operations (ROPs)
> > >
> > > It is similar to LCDC IP on RZ/G2L SoCs, but does not have DSI interface.
> > >
> > > v3->v4:
> > > * Restored the ports property for RZ/G2UL and described port@0 for the
> > >DPI interface in bindings patch.
> > > * Restored tags from Geert and Conor as the change is trivial
> > >(Replaced port 1->0 from v2).
> > > * Used "&" instead of "==" in rzg2l_du_start_stop() for scalability.
> > > * Restored port variable in struct rzg2l_du_output_routing
> > > * Updated rzg2l_du_encoders_init() to handle port based on hardware
> > > indices.
> > > * Restored ports property in du node and used port@0 for connected
> > >DPI interface.
> > > v2->v3:
> > > * Split patch series based on subsystem from DU patch series [1].
> > > * Replaced ports->port property for RZ/G2UL as it supports only DPI
> > >and retained ports property for RZ/{G2L,V2L} as it supports both DSI
> > >and DPI output interface.
> > > * Added missing blank line before example.
> > > * Dropped tags from Conor and Geert as there are new changes in bindings
> > > * Avoided the line break in rzg2l_du_start_stop() for rstate.
> > > * Replaced port->du_output in struct rzg2l_du_output_routing and
> > >dropped using the port number to indicate the output type in
> > >rzg2l_du_encoders_init().
> > > * Updated rzg2l_du_r9a07g043u_info and rzg2l_du_r9a07g044_info.
> > >
> > > [1]
> > > https://lore.kernel.org/all/20240709135152.185042-1-biju.das...@bp.renesas.com/
> > > v1->v2:
> > > * Updated cover letter header "DU IP->Display Unit".
> > > * Updated commit description related to non ABI breakage for patch#3.
> > > * Added Ack from Conor for binding patches.
> > >
> > > Biju Das (4):
> > > dt-bindings: display: renesas,rzg2l-du: Document RZ/G2UL DU bindings
> > > drm: renesas: rz-du: Add RZ/G2UL DU Support
> >
> > The first two patches look good to me. Do you have access to drm-misc, will
> > you push them yourself, or
> > do you expect a maintainer to pick them up ?
>
> I don’t have access to drm-misc. I expect a maintainer to pick it up(Maybe
> via rcar-du tree or
> drm-misc tree), or else if it is ok, what is the procedure to get access for
> drm-misc tree??
You can find instructions to request drm-misc commit access at
https://drm.pages.freedesktop.org/maintainer-tools/committer/commit-access.html
Tomi, to avoid delays, could you push the first two patches to drm-misc
?
> > > arm64: dts: renesas: r9a07g043u: Add DU node
> > > arm64: dts: renesas: r9a07g043u11-smarc: Enable DU
> > >
> > > .../bindings/display/renesas,rzg2l-du.yaml| 32 -
> > > arch/arm64/boot/dts/renesas/r9a07g043u.dtsi | 25
> > > .../boot/dts/renesas/r9a07g043u11-smarc.dts | 111 ++
> > > drivers/gpu/drm/renesas/rz-du/rzg2l_du_crtc.c | 8 +-
> > > drivers/gpu/drm/renesas/rz-du/rzg2l_du_drv.c | 11 ++
> > > drivers/gpu/drm/renesas/rz-du/rzg2l_du_kms.c | 3 +-
> > > 6 files changed, 185 insertions(+), 5 deletions(-)
--
Regards,
Laurent Pinchart
Re: [PATCH v4 0/4] Add support for RZ/G2UL Display Unit
Hi Biju,
On Thu, Aug 22, 2024 at 05:23:13PM +0100, Biju Das wrote:
> This patch series aims to add support for RZ/G2UL DU.
>
> The LCD controller is composed of Frame Compression Processor (FCPVD),
> Video Signal Processor (VSPD), and Display Unit (DU).
>
> The output of LCDC is connected display parallel interface (DPI) and
> supports a maximum resolution of WXGA along with 2 RPFs to support the
> blending of two picture layers and raster operations (ROPs)
>
> It is similar to LCDC IP on RZ/G2L SoCs, but does not have DSI interface.
>
> v3->v4:
> * Restored the ports property for RZ/G2UL and described port@0 for the
>DPI interface in bindings patch.
> * Restored tags from Geert and Conor as the change is trivial
>(Replaced port 1->0 from v2).
> * Used "&" instead of "==" in rzg2l_du_start_stop() for scalability.
> * Restored port variable in struct rzg2l_du_output_routing
> * Updated rzg2l_du_encoders_init() to handle port based on hardware indices.
> * Restored ports property in du node and used port@0 for connected
>DPI interface.
> v2->v3:
> * Split patch series based on subsystem from DU patch series [1].
> * Replaced ports->port property for RZ/G2UL as it supports only DPI
>and retained ports property for RZ/{G2L,V2L} as it supports both DSI
>and DPI output interface.
> * Added missing blank line before example.
> * Dropped tags from Conor and Geert as there are new changes in bindings
> * Avoided the line break in rzg2l_du_start_stop() for rstate.
> * Replaced port->du_output in struct rzg2l_du_output_routing and
>dropped using the port number to indicate the output type in
>rzg2l_du_encoders_init().
> * Updated rzg2l_du_r9a07g043u_info and rzg2l_du_r9a07g044_info.
>
> [1]
> https://lore.kernel.org/all/20240709135152.185042-1-biju.das...@bp.renesas.com/
> v1->v2:
> * Updated cover letter header "DU IP->Display Unit".
> * Updated commit description related to non ABI breakage for patch#3.
> * Added Ack from Conor for binding patches.
>
> Biju Das (4):
> dt-bindings: display: renesas,rzg2l-du: Document RZ/G2UL DU bindings
> drm: renesas: rz-du: Add RZ/G2UL DU Support
The first two patches look good to me. Do you have access to drm-misc,
will you push them yourself, or do you expect a maintainer to pick them
up ?
> arm64: dts: renesas: r9a07g043u: Add DU node
> arm64: dts: renesas: r9a07g043u11-smarc: Enable DU
>
> .../bindings/display/renesas,rzg2l-du.yaml| 32 -
> arch/arm64/boot/dts/renesas/r9a07g043u.dtsi | 25
> .../boot/dts/renesas/r9a07g043u11-smarc.dts | 111 ++
> drivers/gpu/drm/renesas/rz-du/rzg2l_du_crtc.c | 8 +-
> drivers/gpu/drm/renesas/rz-du/rzg2l_du_drv.c | 11 ++
> drivers/gpu/drm/renesas/rz-du/rzg2l_du_kms.c | 3 +-
> 6 files changed, 185 insertions(+), 5 deletions(-)
--
Regards,
Laurent Pinchart
Re: [PATCH v4 2/4] drm: renesas: rz-du: Add RZ/G2UL DU Support
Hi Biju,
Thank you for the patch.
On Thu, Aug 22, 2024 at 05:23:15PM +0100, Biju Das wrote:
> The LCD controller is composed of Frame Compression Processor (FCPVD),
> Video Signal Processor (VSPD), and Display Unit (DU).
>
> It has DPI interface and supports a maximum resolution of WXGA along
> with 2 RPFs to support the blending of two picture layers and raster
> operations (ROPs).
>
> The DU module is connected to VSPD. Add RZ/G2UL DU support.
>
> Signed-off-by: Biju Das
Reviewed-by: Laurent Pinchart
> ---
> v3->v4:
> * Used "&" instead of "==" in rzg2l_du_start_stop() for scalability.
> * Restored port variable in struct rzg2l_du_output_routing
> * Updated rzg2l_du_encoders_init() to handle port based on hardware indices.
> v2->v3:
> * Avoided the line break in rzg2l_du_start_stop() for rstate.
> * Replaced port->du_output in struct rzg2l_du_output_routing and
>dropped using the port number to indicate the output type in
>rzg2l_du_encoders_init().
> * Updated rzg2l_du_r9a07g043u_info and rzg2l_du_r9a07g044_info
> v1->v2:
> * No change.
> ---
> drivers/gpu/drm/renesas/rz-du/rzg2l_du_crtc.c | 8 +++-
> drivers/gpu/drm/renesas/rz-du/rzg2l_du_drv.c | 11 +++
> drivers/gpu/drm/renesas/rz-du/rzg2l_du_kms.c | 3 ++-
> 3 files changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/renesas/rz-du/rzg2l_du_crtc.c
> b/drivers/gpu/drm/renesas/rz-du/rzg2l_du_crtc.c
> index 6e7aac6219be..c4c1474d487e 100644
> --- a/drivers/gpu/drm/renesas/rz-du/rzg2l_du_crtc.c
> +++ b/drivers/gpu/drm/renesas/rz-du/rzg2l_du_crtc.c
> @@ -28,6 +28,7 @@
> #include "rzg2l_du_vsp.h"
>
> #define DU_MCR0 0x00
> +#define DU_MCR0_DPI_OE BIT(0)
> #define DU_MCR0_DI_ENBIT(8)
>
> #define DU_DITR0 0x10
> @@ -216,9 +217,14 @@ static void rzg2l_du_crtc_put(struct rzg2l_du_crtc
> *rcrtc)
>
> static void rzg2l_du_start_stop(struct rzg2l_du_crtc *rcrtc, bool start)
> {
> + struct rzg2l_du_crtc_state *rstate =
> to_rzg2l_crtc_state(rcrtc->crtc.state);
> struct rzg2l_du_device *rcdu = rcrtc->dev;
> + u32 val = DU_MCR0_DI_EN;
>
> - writel(start ? DU_MCR0_DI_EN : 0, rcdu->mmio + DU_MCR0);
> + if (rstate->outputs & BIT(RZG2L_DU_OUTPUT_DPAD0))
> + val |= DU_MCR0_DPI_OE;
> +
> + writel(start ? val : 0, rcdu->mmio + DU_MCR0);
> }
>
> static void rzg2l_du_crtc_start(struct rzg2l_du_crtc *rcrtc)
> diff --git a/drivers/gpu/drm/renesas/rz-du/rzg2l_du_drv.c
> b/drivers/gpu/drm/renesas/rz-du/rzg2l_du_drv.c
> index e5eca8691a33..bc7c381f92ac 100644
> --- a/drivers/gpu/drm/renesas/rz-du/rzg2l_du_drv.c
> +++ b/drivers/gpu/drm/renesas/rz-du/rzg2l_du_drv.c
> @@ -25,6 +25,16 @@
> * Device Information
> */
>
> +static const struct rzg2l_du_device_info rzg2l_du_r9a07g043u_info = {
> + .channels_mask = BIT(0),
> + .routes = {
> + [RZG2L_DU_OUTPUT_DPAD0] = {
> + .possible_outputs = BIT(0),
> + .port = 0,
> + },
> + },
> +};
> +
> static const struct rzg2l_du_device_info rzg2l_du_r9a07g044_info = {
> .channels_mask = BIT(0),
> .routes = {
> @@ -40,6 +50,7 @@ static const struct rzg2l_du_device_info
> rzg2l_du_r9a07g044_info = {
> };
>
> static const struct of_device_id rzg2l_du_of_table[] = {
> + { .compatible = "renesas,r9a07g043u-du", .data =
> &rzg2l_du_r9a07g043u_info },
> { .compatible = "renesas,r9a07g044-du", .data =
> &rzg2l_du_r9a07g044_info },
> { /* sentinel */ }
> };
> diff --git a/drivers/gpu/drm/renesas/rz-du/rzg2l_du_kms.c
> b/drivers/gpu/drm/renesas/rz-du/rzg2l_du_kms.c
> index 07b312b6f81e..b99217b4e05d 100644
> --- a/drivers/gpu/drm/renesas/rz-du/rzg2l_du_kms.c
> +++ b/drivers/gpu/drm/renesas/rz-du/rzg2l_du_kms.c
> @@ -183,7 +183,8 @@ static int rzg2l_du_encoders_init(struct rzg2l_du_device
> *rcdu)
>
> /* Find the output route corresponding to the port number. */
> for (i = 0; i < RZG2L_DU_OUTPUT_MAX; ++i) {
> - if (rcdu->info->routes[i].port == ep.port) {
> + if (rcdu->info->routes[i].possible_outputs &&
> + rcdu->info->routes[i].port == ep.port) {
> output = i;
> break;
> }
--
Regards,
Laurent Pinchart
Re: [PATCH v4 1/4] dt-bindings: display: renesas,rzg2l-du: Document RZ/G2UL DU bindings
Hi Biju,
Thank you for the patch.
On Thu, Aug 22, 2024 at 05:23:14PM +0100, Biju Das wrote:
> Document DU found in RZ/G2UL SoC. The DU block is identical to RZ/G2L
> SoC, but has only DPI interface.
>
> While at it, add missing required property port@1 for RZ/G2L and RZ/V2L
> SoCs. Currently there is no user for the DPI interface and hence there
> won't be any ABI breakage for adding port@1 as required property for
> RZ/G2L and RZ/V2L SoCs.
>
> Acked-by: Conor Dooley
> Reviewed-by: Geert Uytterhoeven
> Signed-off-by: Biju Das
Reviewed-by: Laurent Pinchart
> ---
> v3->v4:
> * Restored the ports property for RZ/G2UL and described port@0 for the
>DPI interface.
> * Restored tags from Geert and Conor as the change is trivial
>(Replaced port 1->0 from v2).
> v2->v3:
> * Replaced ports->port property for RZ/G2UL as it supports only DPI.
>and retained ports property for RZ/{G2L,V2L} as it supports both DSI
>and DPI output interface.
> * Added missing blank line before example.
> * Dropped tags from Conor and Geert as there are new changes.
> v1->v2:
> * Updated commit description related to non ABI breakage.
> * Added Ack from Conor.
> ---
> .../bindings/display/renesas,rzg2l-du.yaml| 32 +--
> 1 file changed, 29 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/display/renesas,rzg2l-du.yaml
> b/Documentation/devicetree/bindings/display/renesas,rzg2l-du.yaml
> index 08e5b9478051..95e3d5e74b87 100644
> --- a/Documentation/devicetree/bindings/display/renesas,rzg2l-du.yaml
> +++ b/Documentation/devicetree/bindings/display/renesas,rzg2l-du.yaml
> @@ -18,6 +18,7 @@ properties:
>compatible:
> oneOf:
>- enum:
> + - renesas,r9a07g043u-du # RZ/G2UL
>- renesas,r9a07g044-du # RZ/G2{L,LC}
>- items:
>- enum:
> @@ -60,9 +61,6 @@ properties:
> $ref: /schemas/graph.yaml#/properties/port
> unevaluatedProperties: false
>
> -required:
> - - port@0
> -
> unevaluatedProperties: false
>
>renesas,vsps:
> @@ -88,6 +86,34 @@ required:
>
> additionalProperties: false
>
> +allOf:
> + - if:
> + properties:
> +compatible:
> + contains:
> +const: renesas,r9a07g043u-du
> +then:
> + properties:
> +ports:
> + properties:
> +port@0:
> + description: DPI
> +
> + required:
> +- port@0
> +else:
> + properties:
> +ports:
> + properties:
> + port@0:
> + description: DSI
> +port@1:
> + description: DPI
> +
> + required:
> +- port@0
> +- port@1
> +
> examples:
># RZ/G2L DU
>- |
--
Regards,
Laurent Pinchart
[GIT PULL FOR v6.12] Miscellaneous small fixes for Renesas DRM drivers
Hello Dave, Sima, The following changes since commit 11df68c265460d4dff5d19a1313f0fff69470f98: Merge tag 'drm-misc-next-2024-08-16' of https://gitlab.freedesktop.org/drm/misc/kernel into drm-next (2024-08-22 09:42:23 +0200) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/pinchartl/linux.git tags/drm-next-20240823 for you to fetch changes up to caef454889d150d7267992f0d95fbc25dfb621a4: drm: renesas: cmm: Remove unused function rcar_cmm_write (2024-08-23 02:30:26 +0300) Miscellaneous small fixes for Renesas DRM drivers Biju Das (2): drm: renesas: rcar-du: rzg2l_mipi_dsi: Update the comment in rzg2l_mipi_dsi_start_video() drm: rcar-du: Fix memory leak in rcar_du_vsps_init() Jiapeng Chong (1): drm: renesas: cmm: Remove unused function rcar_cmm_write drivers/gpu/drm/renesas/rcar-du/rcar_cmm.c | 5 - drivers/gpu/drm/renesas/rcar-du/rcar_du_kms.c| 10 -- drivers/gpu/drm/renesas/rcar-du/rzg2l_mipi_dsi.c | 2 +- 3 files changed, 5 insertions(+), 12 deletions(-) -- Regards, Laurent Pinchart
[PATCH] drm/msm: Remove prototypes for non-existing functions
The msm_atomic_state_clear() and msm_atomic_state_free() functions are declared but never defined. Remove their prototypes. Signed-off-by: Laurent Pinchart Reviewed-by: Kieran Bingham --- drivers/gpu/drm/msm/msm_drv.h | 2 -- 1 file changed, 2 deletions(-) diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h index be016d7b4ef1..48799c678a6b 100644 --- a/drivers/gpu/drm/msm/msm_drv.h +++ b/drivers/gpu/drm/msm/msm_drv.h @@ -254,8 +254,6 @@ void msm_atomic_destroy_pending_timer(struct msm_pending_timer *timer); void msm_atomic_commit_tail(struct drm_atomic_state *state); int msm_atomic_check(struct drm_device *dev, struct drm_atomic_state *state); struct drm_atomic_state *msm_atomic_state_alloc(struct drm_device *dev); -void msm_atomic_state_clear(struct drm_atomic_state *state); -void msm_atomic_state_free(struct drm_atomic_state *state); int msm_crtc_enable_vblank(struct drm_crtc *crtc); void msm_crtc_disable_vblank(struct drm_crtc *crtc); base-commit: f60ef67ff21ede6f3d27d439a136481446dbd8aa -- Regards, Laurent Pinchart
Re: [PATCH v3 1/4] dt-bindings: display: renesas,rzg2l-du: Document RZ/G2UL DU bindings
On Tue, Aug 20, 2024 at 06:42:48AM +, Biju Das wrote:
> Hi Rob and all,
>
> > -Original Message-
> > From: Biju Das
> > Sent: Monday, August 19, 2024 1:37 PM
> > Subject: RE: [PATCH v3 1/4] dt-bindings: display: renesas,rzg2l-du:
> > Document RZ/G2UL DU bindings
> >
> > Hi Laurent and Rob,
> >
> > Thanks for the feedback
> >
> > > -Original Message-
> > > From: Laurent Pinchart
> > > Sent: Tuesday, August 13, 2024 8:39 PM
> > > Subject: Re: [PATCH v3 1/4] dt-bindings: display: renesas,rzg2l-du:
> > > Document RZ/G2UL DU bindings
> > >
> > > Hi Rob,
> > >
> > > On Tue, Aug 13, 2024 at 10:32:20AM -0600, Rob Herring wrote:
> > > > On Mon, Aug 05, 2024 at 04:52:35PM +0100, Biju Das wrote:
> > > > > Document DU found in RZ/G2UL SoC. The DU block is identical to
> > > > > RZ/G2L SoC, but has only DPI interface.
> > > > >
> > > > > While at it, add missing required property port@1 for RZ/G2L and
> > > > > RZ/V2L SoCs. Currently there is no user for the DPI interface and
> > > > > hence there won't be any ABI breakage for adding port@1 as
> > > > > required property for RZ/G2L and RZ/V2L SoCs.
> > > > >
> > > > > Signed-off-by: Biju Das
> > > > > ---
> > > > > v2->v3:
> > > > > * Replaced ports->port property for RZ/G2UL as it supports only DPI.
> > > > >and retained ports property for RZ/{G2L,V2L} as it supports both
> > > > > DSI
> > > > >and DPI output interface.
> > > >
> > > > Why? Having port and ports is just a needless complication.
> > >
> > > I agree that making the ports node mandatory, even when the device has
> > > a single port, will simplify the bindings. In hindsight we should never
> > > have made ports optional,
> > but that can't be changed.
> > >
> > > > > * Added missing blank line before example.
> > > > > * Dropped tags from Conor and Geert as there are new changes.
> > > > > v1->v2:
> > > > > * Updated commit description related to non ABI breakage.
> > > > > * Added Ack from Conor.
> > > > > ---
> > > > > .../bindings/display/renesas,rzg2l-du.yaml| 35
> > > > > +--
> > > > > 1 file changed, 32 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git
> > > > > a/Documentation/devicetree/bindings/display/renesas,rzg2l-du.yaml
> > > > > b/Documentation/devicetree/bindings/display/renesas,rzg2l-du.yaml
> > > > > index 08e5b9478051..ca01bf26c4c0 100644
> > > > > ---
> > > > > a/Documentation/devicetree/bindings/display/renesas,rzg2l-du.yaml
> > > > > +++ b/Documentation/devicetree/bindings/display/renesas,rzg2l-du.y
> > > > > +++ am
> > > > > +++ l
> > > > > @@ -18,6 +18,7 @@ properties:
> > > > >compatible:
> > > > > oneOf:
> > > > >- enum:
> > > > > + - renesas,r9a07g043u-du # RZ/G2UL
> > > > >- renesas,r9a07g044-du # RZ/G2{L,LC}
> > > > >- items:
> > > > >- enum:
> > > > > @@ -60,8 +61,9 @@ properties:
> > > > > $ref: /schemas/graph.yaml#/properties/port
> > > > > unevaluatedProperties: false
> > > > >
> > > > > -required:
> > > > > - - port@0
> > > > > + port:
> > > > > +$ref: /schemas/graph.yaml#/properties/port
> > > > > +description: Connection to the DU output video port.
> > > > >
> > > > > unevaluatedProperties: false
> > > > >
> > > > > @@ -83,11 +85,38 @@ required:
> > > > >- clock-names
> > > > >- resets
> > > > >- power-domains
> > > > > - - ports
> > > > >- renesas,vsps
> > > > >
> > > > > additionalProperties: false
> > > > >
> > > > > +allOf:
> > > > > + - if:
> > > > > + properties:
> > > > > +compatible:
> > > > > + contains:
> > > > > +const: renesas,r9a07g043u-du
> > > > >
Re: [PATCH 04/86] drm: Add client-agnostic setup helper
Hi Thomas,
On Tue, Aug 20, 2024 at 09:22:36AM +0200, Thomas Zimmermann wrote:
> Am 18.08.24 um 22:07 schrieb Laurent Pinchart:
> > On Fri, Aug 16, 2024 at 02:22:30PM +0200, Thomas Zimmermann wrote:
> >> DRM may support multiple in-kernel clients that run as soon as a DRM
> >> driver has been registered. To select the client(s) in a single place,
> >> introduce drm_client_setup().
> >>
> >> Drivers that call the new helper automatically instantiate the kernel's
> >> configured default clients. Only fbdev emulation is currently supported.
> >> Later versions can add support for DRM-based logging, a boot logo or even
> >> a console.
> > I really like the direction this is taking :-)
> >
> >> Some drivers handle the color mode for clients internally. Provide the
> >> helper drm_client_setup_with_color_mode() for them.
> >>
> >> Signed-off-by: Thomas Zimmermann
> >> ---
> >> drivers/gpu/drm/Makefile | 1 +
> >> drivers/gpu/drm/drm_client_setup.c | 55 ++
> >> include/drm/drm_client_setup.h | 12 +++
> >> 3 files changed, 68 insertions(+)
> >> create mode 100644 drivers/gpu/drm/drm_client_setup.c
> >> create mode 100644 include/drm/drm_client_setup.h
> >>
> >> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> >> index 0beb55d028a8..e7fc77d1d573 100644
> >> --- a/drivers/gpu/drm/Makefile
> >> +++ b/drivers/gpu/drm/Makefile
> >> @@ -129,6 +129,7 @@ drm_kms_helper-y := \
> >>drm_atomic_helper.o \
> >>drm_atomic_state_helper.o \
> >>drm_bridge_connector.o \
> >> + drm_client_setup.o \
> >>drm_crtc_helper.o \
> >>drm_damage_helper.o \
> >>drm_encoder_slave.o \
> >> diff --git a/drivers/gpu/drm/drm_client_setup.c
> >> b/drivers/gpu/drm/drm_client_setup.c
> >> new file mode 100644
> >> index ..2e3315f5c3e2
> >> --- /dev/null
> >> +++ b/drivers/gpu/drm/drm_client_setup.c
> >> @@ -0,0 +1,55 @@
> >> +// SPDX-License-Identifier: MIT
> >> +
> >> +#include
> >> +#include
> >> +#include
> >> +#include
> >> +#include
> >> +
> >> +/**
> >> + * drm_client_setup() - Setup in-kernel DRM clients
> >> + * @dev: DRM device
> >> + * @format: Preferred color format for the device. Use NULL, unless
> >>
> > s/color format/pixel format/
>
> Ah, ok.
>
> >> + * there is clearly a driver-preferred format.
> >> + *
> >> + * This function sets up the in-kernel DRM clients. Restore, hotplug
> >> + * events and teardown are all taken care of.
> >> + *
> >> + * Drivers should call drm_client_setup() after registering the new
> >> + * DRM device with drm_dev_register(). This function is safe to call
> >> + * even when there are no connectors present. Setup will be retried
> >> + * on the next hotplug event.
> >> + *
> >> + * The clients are destroyed by drm_dev_unregister().
> >> + */
> >> +void drm_client_setup(struct drm_device *dev, const struct
> >> drm_format_info *format)
> >> +{
> >> + int ret;
> >> +
> >> + if (!format)
> >> + format = drm_format_info(DRM_FORMAT_XRGB);
> >> +
> >> + ret = drm_fbdev_client_setup(dev, format);
> >> + if (ret)
> >> + drm_warn(dev, "Failed to set up DRM client; error %d\n", ret);
> >> +}
> >> +EXPORT_SYMBOL(drm_client_setup);
> >> +
> >> +/**
> >> + * drm_client_setup_with_color_mode() - Setup in-kernel DRM clients for
> >> color mode
> >> + * @dev: DRM device
> >> + * @color_mode: Preferred color mode for the device
> >> + *
> >> + * This function sets up the in-kernel DRM clients. It is equivalent
> >> + * to drm_client_setup(), but expects a color mode as second argument.
> >> + *
> >> + * Do not use this function in new drivers. Prefer drm_client_setup()
> >> with a
> >> + * format of NULL.
> >> + */
> >> +void drm_client_setup_with_color_mode(struct drm_device *dev, unsigned
> >> int color_mode)
> >> +{
> >> + uint32_t fmt = drm_driver_color_mode_format(dev, color_mode);
> >> +
> >> + drm_client_setup(dev, drm_format_info(fmt));
> >> +}
> >> +EXPORT_SYMBOL(drm_client_setup_with_color_mode);
> >> diff --git a/i
Re: [PATCH 48/86] drm/xlnx: Run DRM default client setup
Hi Thomas,
Thank you for the patch.
On Fri, Aug 16, 2024 at 02:23:14PM +0200, Thomas Zimmermann wrote:
> Call drm_client_setup() to run the kernel's default client setup
> for DRM. Set fbdev_probe in struct drm_driver, so that the client
> setup can start the common fbdev client.
>
> Signed-off-by: Thomas Zimmermann
> Cc: Laurent Pinchart
> Cc: Tomi Valkeinen
> Cc: Michal Simek
> ---
> drivers/gpu/drm/xlnx/zynqmp_kms.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_kms.c
> b/drivers/gpu/drm/xlnx/zynqmp_kms.c
> index bd1368df7870..f26b119322d5 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_kms.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_kms.c
> @@ -14,6 +14,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -402,6 +403,7 @@ static const struct drm_driver zynqmp_dpsub_drm_driver = {
> DRIVER_ATOMIC,
>
> DRM_GEM_DMA_DRIVER_OPS_WITH_DUMB_CREATE(zynqmp_dpsub_dumb_create),
> + DRM_FBDEV_DMA_DRIVER_OPS,
>
> .fops = &zynqmp_dpsub_drm_fops,
>
> @@ -523,7 +525,7 @@ int zynqmp_dpsub_drm_init(struct zynqmp_dpsub *dpsub)
> goto err_poll_fini;
>
> /* Initialize fbdev generic emulation. */
> - drm_fbdev_dma_setup(drm, 24);
> + drm_client_setup(drm, drm_format_info(DRM_FORMAT_RGB888));
I know this would be a hassle to change, but do the majority of the
callers of drm_client_setup() have a drm_format_info * already, or do
they need to call drm_format_info() on a 4CC ? In the latter case, would
it be better to pass the 4CC to drm_client_setup() ?
As far as this patch goes,
Reviewed-by: Laurent Pinchart
>
> return 0;
>
--
Regards,
Laurent Pinchart
Re: [PATCH 36/86] drm/renesas/shmobile: Run DRM default client setup
Hi Thomas,
Thank you for the patch.
On Fri, Aug 16, 2024 at 02:23:02PM +0200, Thomas Zimmermann wrote:
> Call drm_client_setup() to run the kernel's default client setup
> for DRM. Set fbdev_probe in struct drm_driver, so that the client
> setup can start the common fbdev client.
>
> Signed-off-by: Thomas Zimmermann
> Cc: Laurent Pinchart
> Cc: Geert Uytterhoeven
Reviewed-by: Laurent Pinchart
> ---
> drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c | 5 -
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c
> b/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c
> index ff2883c7fd46..e82624836c97 100644
> --- a/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c
> +++ b/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c
> @@ -18,8 +18,10 @@
> #include
>
> #include
> +#include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -101,6 +103,7 @@ DEFINE_DRM_GEM_DMA_FOPS(shmob_drm_fops);
> static const struct drm_driver shmob_drm_driver = {
> .driver_features= DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC,
> DRM_GEM_DMA_DRIVER_OPS,
> + DRM_FBDEV_DMA_DRIVER_OPS,
> .fops = &shmob_drm_fops,
> .name = "shmob-drm",
> .desc = "Renesas SH Mobile DRM",
> @@ -257,7 +260,7 @@ static int shmob_drm_probe(struct platform_device *pdev)
> if (ret < 0)
> goto err_modeset_cleanup;
>
> - drm_fbdev_dma_setup(ddev, 16);
> + drm_client_setup(ddev, drm_format_info(DRM_FORMAT_RGB565));
>
> return 0;
>
--
Regards,
Laurent Pinchart
Re: [PATCH 34/86] drm/renesas/rcar-du: Run DRM default client setup
Hi Thomas,
Thank you for the patch.
On Fri, Aug 16, 2024 at 02:23:00PM +0200, Thomas Zimmermann wrote:
> Call drm_client_setup() to run the kernel's default client setup
> for DRM. Set fbdev_probe in struct drm_driver, so that the client
> setup can start the common fbdev client.
>
> The rcar-du driver specifies a preferred color mode of 32. As this
> is the default if no format has been given, leave it out entirely.
>
> Signed-off-by: Thomas Zimmermann
> Cc: Laurent Pinchart
> Cc: Kieran Bingham
Reviewed-by: Laurent Pinchart
> ---
> drivers/gpu/drm/renesas/rcar-du/rcar_du_drv.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> 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 fb719d9aff10..4e0bafc86f50 100644
> --- a/drivers/gpu/drm/renesas/rcar-du/rcar_du_drv.c
> +++ b/drivers/gpu/drm/renesas/rcar-du/rcar_du_drv.c
> @@ -19,6 +19,7 @@
> #include
>
> #include
> +#include
> #include
> #include
> #include
> @@ -606,6 +607,7 @@ static const struct drm_driver rcar_du_driver = {
> .driver_features= DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC,
> .dumb_create= rcar_du_dumb_create,
> .gem_prime_import_sg_table = rcar_du_gem_prime_import_sg_table,
> + DRM_FBDEV_DMA_DRIVER_OPS,
> .fops = &rcar_du_fops,
> .name = "rcar-du",
> .desc = "Renesas R-Car Display Unit",
> @@ -716,7 +718,7 @@ static int rcar_du_probe(struct platform_device *pdev)
>
> drm_info(&rcdu->ddev, "Device %s probed\n", dev_name(&pdev->dev));
>
> - drm_fbdev_dma_setup(&rcdu->ddev, 32);
> + drm_client_setup(&rcdu->ddev, NULL);
>
> return 0;
>
--
Regards,
Laurent Pinchart
Re: [PATCH 04/86] drm: Add client-agnostic setup helper
_Generic(format_or_color_mode, \ const struct drm_format_info *: drm_client_setup_with_format(dev, format_or_color_mode), \ unsigned int: drm_client_setup_with_color_mode(dev, format_or_color_mode)\ ) Up to you. Reviewed-by: Laurent Pinchart > + > +#endif -- Regards, Laurent Pinchart
Re: [PATCH 01/86] drm/fbdev-helper: Move color-mode lookup into 4CC format helper
Hi Thomas,
Thank you for the patch.
On Fri, Aug 16, 2024 at 02:22:27PM +0200, Thomas Zimmermann wrote:
> The color mode specified on the kernel command line gives the user's
> preferred color depth and number of bits per pixel. Move the
> color-mode-to-format conversion form fbdev helpers into a 4CC helper,
s/form/from/
> so that is can be shared among DRM clients.
>
> Signed-off-by: Thomas Zimmermann
Reviewed-by: Laurent Pinchart
> ---
> drivers/gpu/drm/drm_fb_helper.c | 70 +++--
> drivers/gpu/drm/drm_fourcc.c| 30 +-
> include/drm/drm_fourcc.h| 1 +
> 3 files changed, 45 insertions(+), 56 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 29c53f9f449c..af1fe79c701d 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -1441,67 +1441,27 @@ int drm_fb_helper_pan_display(struct
> fb_var_screeninfo *var,
> EXPORT_SYMBOL(drm_fb_helper_pan_display);
>
> static uint32_t drm_fb_helper_find_format(struct drm_fb_helper *fb_helper,
> const uint32_t *formats,
> - size_t format_count, uint32_t bpp,
> uint32_t depth)
> + size_t format_count, unsigned int
> color_mode)
> {
> struct drm_device *dev = fb_helper->dev;
> uint32_t format;
> size_t i;
>
> - /*
> - * Do not consider YUV or other complicated formats
> - * for framebuffers. This means only legacy formats
> - * are supported (fmt->depth is a legacy field), but
> - * the framebuffer emulation can only deal with such
> - * formats, specifically RGB/BGA formats.
> - */
> - format = drm_mode_legacy_fb_format(bpp, depth);
> - if (!format)
> - goto err;
> + format = drm_driver_color_mode_format(dev, color_mode);
> + if (!format) {
> + drm_info(dev, "unsupported color mode of %d\n", color_mode);
> + return DRM_FORMAT_INVALID;
> + }
>
> for (i = 0; i < format_count; ++i) {
> if (formats[i] == format)
> return format;
> }
> -
> -err:
> - /* We found nothing. */
> - drm_warn(dev, "bpp/depth value of %u/%u not supported\n", bpp, depth);
> + drm_warn(dev, "format %p4cc not supported\n", &format);
>
> return DRM_FORMAT_INVALID;
> }
>
> -static uint32_t drm_fb_helper_find_color_mode_format(struct drm_fb_helper
> *fb_helper,
> - const uint32_t *formats,
> size_t format_count,
> - unsigned int color_mode)
> -{
> - struct drm_device *dev = fb_helper->dev;
> - uint32_t bpp, depth;
> -
> - switch (color_mode) {
> - case 1:
> - case 2:
> - case 4:
> - case 8:
> - case 16:
> - case 24:
> - bpp = depth = color_mode;
> - break;
> - case 15:
> - bpp = 16;
> - depth = 15;
> - break;
> - case 32:
> - bpp = 32;
> - depth = 24;
> - break;
> - default:
> - drm_info(dev, "unsupported color mode of %d\n", color_mode);
> - return DRM_FORMAT_INVALID;
> - }
> -
> - return drm_fb_helper_find_format(fb_helper, formats, format_count, bpp,
> depth);
> -}
> -
> static int __drm_fb_helper_find_sizes(struct drm_fb_helper *fb_helper,
> struct drm_fb_helper_surface_size *sizes)
> {
> @@ -1531,10 +1491,10 @@ static int __drm_fb_helper_find_sizes(struct
> drm_fb_helper *fb_helper,
> if (!cmdline_mode->bpp_specified)
> continue;
>
> - surface_format =
> drm_fb_helper_find_color_mode_format(fb_helper,
> -
> plane->format_types,
> -
> plane->format_count,
> -
> cmdline_mode->bpp);
> + surface_format = drm_fb_helper_find_format(fb_helper,
> +
> plane->format_types,
> +
> plane->format_count,
> +
> cmdline_mode->bpp);
>
Re: [PATCH 1/2] dt-bindings: display: renesas,du: narrow interrupts and resets per variants
On Sun, Aug 18, 2024 at 07:44:22PM +0200, Krzysztof Kozlowski wrote: > On 18/08/2024 19:41, Laurent Pinchart wrote: > > Hi Krzysztof, > > > > Thank you for the patch. > > > > On Sun, Aug 18, 2024 at 07:30:02PM +0200, Krzysztof Kozlowski wrote: > >> Each variable-length property like interrupts or resets must have fixed > >> constraints on number of items for given variant in binding. The > >> clauses in "if:then:" block should define both limits: upper and lower. > > > > I thought that, when only one of minItems or maxItems was specified, the > > other automatically defaulted to the same value. I'm pretty sure I > > recall Rob asking me to drop one of the two in some bindings. Has the > > rule changes ? Is it documented somewhere ? > > New dtschema changed it and, even if previous behavior is restored, the > size in if:then: always had to be constrained. You could have skipped > one side of limit if it was equal to outer/top-level limit, e.g: > > properties: > clocks: > minItems: 1 > maxItems: 2 > > > if:then:properties: > clocks: > minItems: 2 Where can I find a description of the behaviour of the new dtschema (hopefully with some documentation) ? -- Regards, Laurent Pinchart
Re: [PATCH 2/2] dt-bindings: display: renesas,du: add top-level constraints
Hi Krzysztof, Thank you for the patch. On Sun, Aug 18, 2024 at 07:30:03PM +0200, Krzysztof Kozlowski wrote: > Properties with variable number of items per each device are expected to > have widest constraints in top-level "properties:" block and further > customized (narrowed) in "if:then:". Add missing top-level constraints > for clocks, clock-names, interrupts, resets, reset-names, renesas,cmms > and renesas,vsps. > > Signed-off-by: Krzysztof Kozlowski Reviewed-by: Laurent Pinchart > --- > .../bindings/display/renesas,du.yaml | 26 --- > 1 file changed, 22 insertions(+), 4 deletions(-) > > diff --git a/Documentation/devicetree/bindings/display/renesas,du.yaml > b/Documentation/devicetree/bindings/display/renesas,du.yaml > index 147842b6465a..9a2d1c08cb1f 100644 > --- a/Documentation/devicetree/bindings/display/renesas,du.yaml > +++ b/Documentation/devicetree/bindings/display/renesas,du.yaml > @@ -46,12 +46,26 @@ properties: > maxItems: 1 > ># See compatible-specific constraints below. > - clocks: true > - clock-names: true > + clocks: > +minItems: 1 > +maxItems: 8 > + > + clock-names: > +minItems: 1 > +maxItems: 8 > + >interrupts: > +minItems: 1 > +maxItems: 4 > description: Interrupt specifiers, one per DU channel > - resets: true > - reset-names: true > + > + resets: > +minItems: 1 > +maxItems: 2 > + > + reset-names: > +minItems: 1 > +maxItems: 2 > >power-domains: > maxItems: 1 > @@ -77,6 +91,8 @@ properties: > >renesas,cmms: > $ref: /schemas/types.yaml#/definitions/phandle-array > +minItems: 2 > +maxItems: 4 > items: >maxItems: 1 > description: > @@ -85,6 +101,8 @@ properties: > >renesas,vsps: > $ref: /schemas/types.yaml#/definitions/phandle-array > +minItems: 1 > +maxItems: 4 > items: >items: > - description: phandle to VSP instance that serves the DU channel -- Regards, Laurent Pinchart
Re: [PATCH 1/2] dt-bindings: display: renesas,du: narrow interrupts and resets per variants
Hi Krzysztof, Thank you for the patch. On Sun, Aug 18, 2024 at 07:30:02PM +0200, Krzysztof Kozlowski wrote: > Each variable-length property like interrupts or resets must have fixed > constraints on number of items for given variant in binding. The > clauses in "if:then:" block should define both limits: upper and lower. I thought that, when only one of minItems or maxItems was specified, the other automatically defaulted to the same value. I'm pretty sure I recall Rob asking me to drop one of the two in some bindings. Has the rule changes ? Is it documented somewhere ? > Signed-off-by: Krzysztof Kozlowski > --- > .../bindings/display/renesas,du.yaml | 22 +++ > 1 file changed, 22 insertions(+) > > diff --git a/Documentation/devicetree/bindings/display/renesas,du.yaml > b/Documentation/devicetree/bindings/display/renesas,du.yaml > index c5b9e6812bce..147842b6465a 100644 > --- a/Documentation/devicetree/bindings/display/renesas,du.yaml > +++ b/Documentation/devicetree/bindings/display/renesas,du.yaml > @@ -173,6 +173,7 @@ allOf: > - pattern: '^dclkin\.[01]$' > > interrupts: > + minItems: 2 >maxItems: 2 > > resets: > @@ -228,6 +229,7 @@ allOf: > - pattern: '^dclkin\.[01]$' > > interrupts: > + minItems: 2 >maxItems: 2 > > resets: > @@ -281,6 +283,7 @@ allOf: > - pattern: '^dclkin\.[01]$' > > interrupts: > + minItems: 2 >maxItems: 2 > > resets: > @@ -335,6 +338,7 @@ allOf: > - pattern: '^dclkin\.[01]$' > > interrupts: > + minItems: 2 >maxItems: 2 > > resets: > @@ -396,6 +400,7 @@ allOf: > - pattern: '^dclkin\.[012]$' > > interrupts: > + minItems: 3 >maxItems: 3 > > resets: > @@ -460,9 +465,11 @@ allOf: > - pattern: '^dclkin\.[0123]$' > > interrupts: > + minItems: 4 >maxItems: 4 > > resets: > + minItems: 2 >maxItems: 2 > > reset-names: > @@ -489,9 +496,11 @@ allOf: > > renesas,cmms: >minItems: 4 > + maxItems: 4 > > renesas,vsps: >minItems: 4 > + maxItems: 4 > >required: > - clock-names > @@ -531,9 +540,11 @@ allOf: > - pattern: '^dclkin\.[012]$' > > interrupts: > + minItems: 3 >maxItems: 3 > > resets: > + minItems: 2 >maxItems: 2 > > reset-names: > @@ -558,9 +569,11 @@ allOf: > > renesas,cmms: >minItems: 3 > + maxItems: 3 > > renesas,vsps: >minItems: 3 > + maxItems: 3 > >required: > - clock-names > @@ -600,9 +613,11 @@ allOf: > - pattern: '^dclkin\.[013]$' > > interrupts: > + minItems: 3 >maxItems: 3 > > resets: > + minItems: 2 >maxItems: 2 > > reset-names: > @@ -627,9 +642,11 @@ allOf: > > renesas,cmms: >minItems: 3 > + maxItems: 3 > > renesas,vsps: >minItems: 3 > + maxItems: 3 > >required: > - clock-names > @@ -684,6 +701,7 @@ allOf: > > renesas,vsps: >minItems: 1 > + maxItems: 1 > >required: > - clock-names > @@ -719,6 +737,7 @@ allOf: > - pattern: '^dclkin\.[01]$' > > interrupts: > + minItems: 2 >maxItems: 2 > > resets: > @@ -746,9 +765,11 @@ allOf: > > renesas,cmms: >minItems: 2 > + maxItems: 2 > > renesas,vsps: >minItems: 2 > + maxItems: 2 > >required: > - clock-names > @@ -799,6 +820,7 @@ allOf: > > renesas,vsps: >minItems: 2 > + maxItems: 2 > >required: > - clock-names -- Regards, Laurent Pinchart
Re: [PATCH v3 2/4] drm: renesas: rz-du: Add RZ/G2UL DU Support
tputs
> - * @port: device tree port number corresponding to this output route
> + * @du_output: DU output
> *
> * The DU has 2 possible outputs (DPAD0, DSI0). Output routing data
> * specify the valid SoC outputs, which CRTC can drive the output, and the
> type
> @@ -37,7 +37,7 @@ enum rzg2l_du_output {
> */
> struct rzg2l_du_output_routing {
> unsigned int possible_outputs;
> - unsigned int port;
> + unsigned int du_output;
> };
>
> /*
> @@ -53,6 +53,7 @@ struct rzg2l_du_device_info {
> #define RZG2L_DU_MAX_CRTCS 1
> #define RZG2L_DU_MAX_VSPS1
> #define RZG2L_DU_MAX_DSI 1
> +#define RZG2L_DU_OUTPUT_INVALID -1
>
> struct rzg2l_du_device {
> struct device *dev;
> diff --git a/drivers/gpu/drm/renesas/rz-du/rzg2l_du_kms.c
> b/drivers/gpu/drm/renesas/rz-du/rzg2l_du_kms.c
> index 07b312b6f81e..361350f2999e 100644
> --- a/drivers/gpu/drm/renesas/rz-du/rzg2l_du_kms.c
> +++ b/drivers/gpu/drm/renesas/rz-du/rzg2l_du_kms.c
> @@ -183,8 +183,8 @@ static int rzg2l_du_encoders_init(struct rzg2l_du_device
> *rcdu)
>
> /* Find the output route corresponding to the port number. */
> for (i = 0; i < RZG2L_DU_OUTPUT_MAX; ++i) {
> - if (rcdu->info->routes[i].port == ep.port) {
> - output = i;
> + if (i == rcdu->info->routes[i].du_output) {
If I understand the code correctly, this will always be true except for
the routes marked with RZG2L_DU_OUTPUT_INVALID, so you will match the
first valid route, regardless of the value of ep.port. I don't think
that's correct.
I would keep the port field in the rzg2l_du_output_routing, drop the
newly added du_output field, and use the following logic:
for (i = 0; i < RZG2L_DU_OUTPUT_MAX; ++i) {
if (rcdu->info->routes[i].possible_outputs &&
rcdu->info->routes[i].port == ep.port) {
output = i;
break;
}
}
Testing possible_outputs skips the routes that don't exist for the
device, and the ep.port comparison picks the route corresponding to the
port.
> + output = rcdu->info->routes[i].du_output;
> break;
> }
> }
--
Regards,
Laurent Pinchart
Re: [PATCH v3 1/4] dt-bindings: display: renesas,rzg2l-du: Document RZ/G2UL DU bindings
Hi Rob,
On Tue, Aug 13, 2024 at 10:32:20AM -0600, Rob Herring wrote:
> On Mon, Aug 05, 2024 at 04:52:35PM +0100, Biju Das wrote:
> > Document DU found in RZ/G2UL SoC. The DU block is identical to RZ/G2L
> > SoC, but has only DPI interface.
> >
> > While at it, add missing required property port@1 for RZ/G2L and RZ/V2L
> > SoCs. Currently there is no user for the DPI interface and hence there
> > won't be any ABI breakage for adding port@1 as required property for
> > RZ/G2L and RZ/V2L SoCs.
> >
> > Signed-off-by: Biju Das
> > ---
> > v2->v3:
> > * Replaced ports->port property for RZ/G2UL as it supports only DPI.
> >and retained ports property for RZ/{G2L,V2L} as it supports both DSI
> >and DPI output interface.
>
> Why? Having port and ports is just a needless complication.
I agree that making the ports node mandatory, even when the device has a
single port, will simplify the bindings. In hindsight we should never
have made ports optional, but that can't be changed.
> > * Added missing blank line before example.
> > * Dropped tags from Conor and Geert as there are new changes.
> > v1->v2:
> > * Updated commit description related to non ABI breakage.
> > * Added Ack from Conor.
> > ---
> > .../bindings/display/renesas,rzg2l-du.yaml| 35 +--
> > 1 file changed, 32 insertions(+), 3 deletions(-)
> >
> > diff --git
> > a/Documentation/devicetree/bindings/display/renesas,rzg2l-du.yaml
> > b/Documentation/devicetree/bindings/display/renesas,rzg2l-du.yaml
> > index 08e5b9478051..ca01bf26c4c0 100644
> > --- a/Documentation/devicetree/bindings/display/renesas,rzg2l-du.yaml
> > +++ b/Documentation/devicetree/bindings/display/renesas,rzg2l-du.yaml
> > @@ -18,6 +18,7 @@ properties:
> >compatible:
> > oneOf:
> >- enum:
> > + - renesas,r9a07g043u-du # RZ/G2UL
> >- renesas,r9a07g044-du # RZ/G2{L,LC}
> >- items:
> >- enum:
> > @@ -60,8 +61,9 @@ properties:
> > $ref: /schemas/graph.yaml#/properties/port
> > unevaluatedProperties: false
> >
> > -required:
> > - - port@0
> > + port:
> > +$ref: /schemas/graph.yaml#/properties/port
> > +description: Connection to the DU output video port.
> >
> > unevaluatedProperties: false
> >
> > @@ -83,11 +85,38 @@ required:
> >- clock-names
> >- resets
> >- power-domains
> > - - ports
> >- renesas,vsps
> >
> > additionalProperties: false
> >
> > +allOf:
> > + - if:
> > + properties:
> > +compatible:
> > + contains:
> > +const: renesas,r9a07g043u-du
> > +then:
> > + properties:
> > +port:
> > + description: DPI
>
> This is equivalent to 'port@0'. IMO, this case should have a 'port@1'
> node so that DPI interface is *always* the same port.
That's what Biju did in the previous version, and I recommended to
number the ports based on hardware indices, not types. Mapping port
numbers to the hardware documentation makes it more consistent for DT
writers, makes the logic simpler to understand (in my opinion, based on
my experience with the R-Car DU) on the driver side, but most
importantly, type-based numbering wouldn't scale as SoCs could have
multiple ports of the same type (we've seen that happening with R-Car).
> > +
> > + required:
> > +- port
> > +else:
> > + properties:
> > +ports:
> > + properties:
> > +port@0:
> > + description: DSI
> > +port@1:
> > + description: DPI
> > +
> > + required:
> > +- port@0
> > +- port@1
> > + required:
> > +- ports
> > +
> > examples:
> ># RZ/G2L DU
> >- |
--
Regards,
Laurent Pinchart
Re: [PATCH v2 1/9] of: property: add of_graph_get_next_port()
ph_get_port_by_id(struct device_node *node, u32
> id);
> struct device_node *of_graph_get_next_endpoint(const struct device_node
> *parent,
> struct device_node *previous);
> +struct device_node *of_graph_get_next_ports(struct device_node *parent,
> + struct device_node *ports);
> +struct device_node *of_graph_get_next_port(struct device_node *parent,
> +struct device_node *port);
> struct device_node *of_graph_get_endpoint_by_regs(
> const struct device_node *parent, int port_reg, int reg);
> struct device_node *of_graph_get_remote_endpoint(
> @@ -73,6 +100,11 @@ static inline unsigned int
> of_graph_get_endpoint_count(const struct device_node
> return 0;
> }
>
> +static inline unsigned int of_graph_get_port_count(struct device_node *np)
> +{
> + return 0;
> +}
> +
> static inline struct device_node *of_graph_get_port_by_id(
> struct device_node *node, u32 id)
> {
> @@ -86,6 +118,20 @@ static inline struct device_node
> *of_graph_get_next_endpoint(
> return NULL;
> }
>
> +static inline struct device_node *of_graph_get_next_ports(
> + struct device_node *parent,
> + struct device_node *previous)
> +{
> + return NULL;
> +}
> +
> +static inline struct device_node *of_graph_get_next_port(
> + struct device_node *parent,
> + struct device_node *previous)
> +{
> + return NULL;
> +}
> +
> static inline struct device_node *of_graph_get_endpoint_by_regs(
> const struct device_node *parent, int port_reg, int reg)
> {
--
Regards,
Laurent Pinchart
Re: [PATCH] gpu: drm: use for_each_endpoint_of_node()
Tomi, could you please push this through drm-misc ?
On Tue, Jul 30, 2024 at 12:34:40AM +, Kuninori Morimoto wrote:
> We already have for_each_endpoint_of_node(), don't use
> of_graph_get_next_endpoint() directly. Replace it.
>
> Signed-off-by: Kuninori Morimoto
> Acked-by: Dmitry Baryshkov
> Reviewed-by: Laurent Pinchart
> ---
> drivers/gpu/drm/omapdrm/dss/base.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/omapdrm/dss/base.c
> b/drivers/gpu/drm/omapdrm/dss/base.c
> index 050ca7eafac58..5f8002f6bb7a5 100644
> --- a/drivers/gpu/drm/omapdrm/dss/base.c
> +++ b/drivers/gpu/drm/omapdrm/dss/base.c
> @@ -242,8 +242,7 @@ static void omapdss_walk_device(struct device *dev,
> struct device_node *node,
>
> of_node_put(n);
>
> - n = NULL;
> - while ((n = of_graph_get_next_endpoint(node, n)) != NULL) {
> + for_each_endpoint_of_node(node, n) {
> struct device_node *pn = of_graph_get_remote_port_parent(n);
>
> if (!pn)
--
Regards,
Laurent Pinchart
Re: [PATCH] drm: bridge: adv7511: Accept audio sample widths of 32 bits via I2S
On Mon, Jul 29, 2024 at 10:15:55AM +0200, Ricard Wanderlof wrote: > > Hi, > > I submitted the patch below a while ago (two months) but as far as I can > make out it has not been included. There was an initial concern from > Dmitry Baryshkov which was subsequently addressed but no other objections. > > On Tue, 28 May 2024, Ricard Wanderlof wrote: > > > > > Even though data is truncated to 24 bits, the I2S interface does > > accept 32 bit data (the slot widths according to the data sheet > > can be 16 or 32 bits) so let the hw_params callback reflect this, > > even if the lowest 8 bits are not used when 32 bits are specified. > > > > This is normally how 24 bit audio data is handled (i.e. as 32 bit > > data, with the LSB:s unused) and this is also reflected in other > > bridge drivers which handle audio, for instance sii902x.c and > > synopsis/dw-hdmi-i2s-audio.c . > > > > Signed-off-by: Ricard Wanderlof > > --- > > drivers/gpu/drm/bridge/adv7511/adv7511_audio.c | 11 +++ > > 1 file changed, 7 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_audio.c > > b/drivers/gpu/drm/bridge/adv7511/adv7511_audio.c > > index 61f4a38e7d2b..4563f5d8136f 100644 > > --- a/drivers/gpu/drm/bridge/adv7511/adv7511_audio.c > > +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_audio.c > > @@ -101,11 +101,14 @@ static int adv7511_hdmi_hw_params(struct device *dev, > > void *data, > > case 20: > > len = ADV7511_I2S_SAMPLE_LEN_20; > > break; > > - case 32: > > - if (fmt->bit_fmt != SNDRV_PCM_FORMAT_IEC958_SUBFRAME_LE) > > - return -EINVAL; > > - fallthrough; > > case 24: > > + case 32: > > + /* > > +* 32 bits are handled like 24 bits, except that the lowest > > +* 8 bits are discarded. In fact, the accepted I2S slot widths > > +* are 16 and 32 bits, so the chip is fully compatible with > > +* 32 bit data. > > +*/ > > len = ADV7511_I2S_SAMPLE_LEN_24; > > break; > > default: > > I recently discovered that the maintainer for the ADV7511 driver (in the > I2C) framework is not included by the get_maintainers script, so perhaps > this is the reason? > > Otherwise, please enlighten me on what I need to do to get this patch > accepted! I have no experience with HDMI audio, so I didn't comment on your patch. Hans, is this within your area of expertise ? -- Regards, Laurent Pinchart
Re: [PATCH v2 3/9] dt-bindings: display: renesas,rzg2l-du: Document RZ/G2UL DU bindings
Hi Biju,
On Mon, Jul 29, 2024 at 09:05:59AM +, Biju Das wrote:
> On Saturday, July 27, 2024 1:50 AM, Laurent Pinchart wrote:
> > On Tue, Jul 09, 2024 at 02:51:41PM +0100, Biju Das wrote:
> > > Document DU found in RZ/G2UL SoC. The DU block is identical to RZ/G2L
> > > SoC, but has only DPI interface.
> > >
> > > While at it, add missing required property port@1 for RZ/G2L and
> > > RZ/V2L SoCs. Currently there is no user for the DPI interface and
> > > hence there won't be any ABI breakage for adding port@1 as required
> > > property for RZ/G2L and RZ/V2L SoCs.
> > >
> > > Signed-off-by: Biju Das
> > > Acked-by: Conor Dooley
> > > ---
> > > v1->v2:
> > > * Updated commit description related to non ABI breakage.
> > > * Added Ack from Conor.
> > > ---
> > > .../bindings/display/renesas,rzg2l-du.yaml| 32 +--
> > > 1 file changed, 29 insertions(+), 3 deletions(-)
> > >
> > > diff --git
> > > a/Documentation/devicetree/bindings/display/renesas,rzg2l-du.yaml
> > > b/Documentation/devicetree/bindings/display/renesas,rzg2l-du.yaml
> > > index 08e5b9478051..c0fec282fa45 100644
> > > --- a/Documentation/devicetree/bindings/display/renesas,rzg2l-du.yaml
> > > +++ b/Documentation/devicetree/bindings/display/renesas,rzg2l-du.yaml
> > > @@ -18,6 +18,7 @@ properties:
> > >compatible:
> > > oneOf:
> > >- enum:
> > > + - renesas,r9a07g043u-du # RZ/G2UL
> > >- renesas,r9a07g044-du # RZ/G2{L,LC}
> > >- items:
> > >- enum:
> > > @@ -60,9 +61,6 @@ properties:
> > > $ref: /schemas/graph.yaml#/properties/port
> > > unevaluatedProperties: false
> > >
> > > -required:
> > > - - port@0
> > > -
> > > unevaluatedProperties: false
> > >
> > >renesas,vsps:
> > > @@ -88,6 +86,34 @@ required:
> > >
> > > additionalProperties: false
> > >
> > > +allOf:
> > > + - if:
> > > + properties:
> > > +compatible:
> > > + contains:
> > > +const: renesas,r9a07g043u-du
> > > +then:
> > > + properties:
> > > +ports:
> > > + properties:
> > > +port@0: false
> > > +port@1:
> > > + description: DPI
> > > +
> > > + required:
> > > +- port@1
> >
> > Why do you use port@1 for the DPI output here, and not port@0 ?
>
> Currently the output is based on port number and port = 1 corresponds to DPI.
> See [1].
>
> For consistency, I documented bindings for RZ/G2L family DU's similar to
> RZ/G2{H,M,N,E} DU [2].
>
> So please let me know, are you ok with this?
I won't insist strongly, but I don't think that using the port number to
indicate the output type is the best idea. In the R-Car DU driver at
least, that wouldn't have scaled. We have multiple outputs of the same
type on some SoCs. Furthemore, the same DU hardware channel number (i.e.
the offset of the registers specific to that channel in the DU register
space) is not the same across SoCs for the same output type. I recommend
numbering the ports based on the hardware number of the output (the
exact meaning of this is specific to your device, I haven't checked what
it means for RZ/G2L), not on the output type.
> [1]
> https://elixir.bootlin.com/linux/v6.10.2/source/drivers/gpu/drm/renesas/rz-du/rzg2l_du_kms.c#L187
>
> [2]
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/display/renesas,du.yaml?h=next-20240729#n546
>
> > > +else:
> > > + properties:
> > > +ports:
> > > + properties:
> > > +port@0:
> > > + description: DSI
> > > +port@1:
> > > + description: DPI
> > > +
> > > + required:
> > > +- port@0
> > > +- port@1
> >
> > You're missing a blank line here.
>
> OK, will fix this'
--
Regards,
Laurent Pinchart
Re: [bug report] drm/omap: gem: Replace struct_mutex usage with omap_obj private lock
Hi Dan,
(CC'ing Tomi)
Thank for the report. It indeed seems that something is wrong.
Tomi, could you handle this and send a fix ?
On Tue, Jul 30, 2024 at 05:01:35PM -0500, Dan Carpenter wrote:
> Hello Laurent Pinchart,
>
> Commit 3cbd0c587b12 ("drm/omap: gem: Replace struct_mutex usage with
> omap_obj private lock") from May 26, 2018 (linux-next), leads to the
> following Smatch static checker warning:
>
> drivers/gpu/drm/omapdrm/omap_gem.c:1435 omap_gem_new_dmabuf()
> warn: 'omap_obj' was already freed. (line 1434)
>
> drivers/gpu/drm/omapdrm/omap_gem.c
> 1386 struct drm_gem_object *omap_gem_new_dmabuf(struct drm_device *dev,
> size_t size,
> 1387struct sg_table *sgt)
> 1388 {
> 1389 struct omap_drm_private *priv = dev->dev_private;
> 1390 struct omap_gem_object *omap_obj;
> 1391 struct drm_gem_object *obj;
> 1392 union omap_gem_size gsize;
> 1393
> 1394 /* Without a DMM only physically contiguous buffers can be
> supported. */
> 1395 if (!omap_gem_sgt_is_contiguous(sgt, size) && !priv->has_dmm)
> 1396 return ERR_PTR(-EINVAL);
> 1397
> 1398 gsize.bytes = PAGE_ALIGN(size);
> 1399 obj = omap_gem_new(dev, gsize, OMAP_BO_MEM_DMABUF |
> OMAP_BO_WC);
> 1400 if (!obj)
> 1401 return ERR_PTR(-ENOMEM);
> 1402
> 1403 omap_obj = to_omap_bo(obj);
> ^^
> This is omap_obj
>
> 1404
> 1405 mutex_lock(&omap_obj->lock);
> 1406
> 1407 omap_obj->sgt = sgt;
> 1408
> 1409 if (omap_gem_sgt_is_contiguous(sgt, size)) {
> 1410 omap_obj->dma_addr = sg_dma_address(sgt->sgl);
> 1411 } else {
> 1412 /* Create pages list from sgt */
> 1413 struct page **pages;
> 1414 unsigned int npages;
> 1415 unsigned int ret;
> 1416
> 1417 npages = DIV_ROUND_UP(size, PAGE_SIZE);
> 1418 pages = kcalloc(npages, sizeof(*pages), GFP_KERNEL);
> 1419 if (!pages) {
> 1420 omap_gem_free_object(obj);
> ^^^
> It gets free inside this function
>
> 1421 obj = ERR_PTR(-ENOMEM);
> 1422 goto done;
> 1423 }
> 1424
> 1425 omap_obj->pages = pages;
> 1426 ret = drm_prime_sg_to_page_array(sgt, pages, npages);
> 1427 if (ret) {
> 1428 omap_gem_free_object(obj);
> ^^^
> Same
>
> 1429 obj = ERR_PTR(-ENOMEM);
> 1430 goto done;
>
> So I think we can just return directly instead of unlocking.
>
> 1431 }
> 1432 }
> 1433
> 1434 done:
> --> 1435 mutex_unlock(&omap_obj->lock);
> 1436 return obj;
> 1437 }
--
Regards,
Laurent Pinchart
Re: [PATCH v2 8/9] arm64: dts: renesas: r9a07g043u: Add DU node
Hi Biju,
Thank you for the patch.
On Tue, Jul 09, 2024 at 02:51:46PM +0100, Biju Das wrote:
> Add DU node to RZ/G2UL SoC DTSI.
>
> Signed-off-by: Biju Das
> ---
> v1->v2:
> * No change.
> ---
> arch/arm64/boot/dts/renesas/r9a07g043u.dtsi | 25 +
> 1 file changed, 25 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/renesas/r9a07g043u.dtsi
> b/arch/arm64/boot/dts/renesas/r9a07g043u.dtsi
> index d88bf23b0782..0a4f24d83791 100644
> --- a/arch/arm64/boot/dts/renesas/r9a07g043u.dtsi
> +++ b/arch/arm64/boot/dts/renesas/r9a07g043u.dtsi
> @@ -153,6 +153,31 @@ fcpvd: fcp@1088 {
> resets = <&cpg R9A07G043_LCDC_RESET_N>;
> };
>
> + du: display@1089 {
> + compatible = "renesas,r9a07g043u-du";
> + reg = <0 0x1089 0 0x1>;
> + interrupts = ;
> + clocks = <&cpg CPG_MOD R9A07G043_LCDC_CLK_A>,
> + <&cpg CPG_MOD R9A07G043_LCDC_CLK_P>,
> + <&cpg CPG_MOD R9A07G043_LCDC_CLK_D>;
> + clock-names = "aclk", "pclk", "vclk";
> + power-domains = <&cpg>;
> + resets = <&cpg R9A07G043_LCDC_RESET_N>;
> + renesas,vsps = <&vspd 0>;
> + status = "disabled";
> +
> + ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + port@1 {
> + reg = <1>;
This may need to change depending on the outcome of the DT bindings
discussion. Other than that,
Reviewed-by: Laurent Pinchart
> + du_out_rgb: endpoint {
> + };
> + };
> + };
> + };
> +
> irqc: interrupt-controller@110a {
> compatible = "renesas,r9a07g043u-irqc",
>"renesas,rzg2l-irqc";
--
Regards,
Laurent Pinchart
Re: [PATCH v2 7/9] arm64: dts: renesas: r9a07g043u: Add fcpvd node
Hi Biju,
Thank you for the patch.
On Tue, Jul 09, 2024 at 02:51:45PM +0100, Biju Das wrote:
> Add fcpvd node to RZ/G2UL SoC DTSI.
>
> Signed-off-by: Biju Das
Reviewed-by: Laurent Pinchart
> ---
> v1->v2:
> * No change.
> ---
> arch/arm64/boot/dts/renesas/r9a07g043u.dtsi | 11 +++
> 1 file changed, 11 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/renesas/r9a07g043u.dtsi
> b/arch/arm64/boot/dts/renesas/r9a07g043u.dtsi
> index 15e84a5428ef..d88bf23b0782 100644
> --- a/arch/arm64/boot/dts/renesas/r9a07g043u.dtsi
> +++ b/arch/arm64/boot/dts/renesas/r9a07g043u.dtsi
> @@ -142,6 +142,17 @@ vspd: vsp@1087 {
> renesas,fcp = <&fcpvd>;
> };
>
> + fcpvd: fcp@1088 {
> + compatible = "renesas,r9a07g043u-fcpvd", "renesas,fcpv";
> + reg = <0 0x1088 0 0x1>;
> + clocks = <&cpg CPG_MOD R9A07G043_LCDC_CLK_A>,
> + <&cpg CPG_MOD R9A07G043_LCDC_CLK_P>,
> + <&cpg CPG_MOD R9A07G043_LCDC_CLK_D>;
> + clock-names = "aclk", "pclk", "vclk";
> + power-domains = <&cpg>;
> + resets = <&cpg R9A07G043_LCDC_RESET_N>;
> + };
> +
> irqc: interrupt-controller@110a {
> compatible = "renesas,r9a07g043u-irqc",
>"renesas,rzg2l-irqc";
--
Regards,
Laurent Pinchart
Re: [PATCH v2 6/9] arm64: dts: renesas: r9a07g043u: Add vspd node
Hi Biju,
Thank you for the patch.
On Tue, Jul 09, 2024 at 02:51:44PM +0100, Biju Das wrote:
> Add vspd node to RZ/G2UL SoC DTSI.
>
> Signed-off-by: Biju Das
> ---
> v1->v2:
> * No change.
> ---
> arch/arm64/boot/dts/renesas/r9a07g043u.dtsi | 13 +
> 1 file changed, 13 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/renesas/r9a07g043u.dtsi
> b/arch/arm64/boot/dts/renesas/r9a07g043u.dtsi
> index 18ef297db933..15e84a5428ef 100644
> --- a/arch/arm64/boot/dts/renesas/r9a07g043u.dtsi
> +++ b/arch/arm64/boot/dts/renesas/r9a07g043u.dtsi
> @@ -129,6 +129,19 @@ csi2cru: endpoint@0 {
> };
> };
>
> + vspd: vsp@1087 {
> + compatible = "renesas,r9a07g043u-vsp2",
> "renesas,r9a07g044-vsp2";
> + reg = <0 0x1087 0 0x1>;
> + interrupts = ;
> + clocks = <&cpg CPG_MOD R9A07G043_LCDC_CLK_A>,
> + <&cpg CPG_MOD R9A07G043_LCDC_CLK_P>,
> + <&cpg CPG_MOD R9A07G043_LCDC_CLK_D>;
> + clock-names = "aclk", "pclk", "vclk";
> + power-domains = <&cpg>;
> + resets = <&cpg R9A07G043_LCDC_RESET_N>;
> + renesas,fcp = <&fcpvd>;
This patch looks fine, but I would move it after 7/9, as here you
reference a node that doesn't exist yet.
Reviewed-by: Laurent Pinchart
> + };
> +
> irqc: interrupt-controller@110a {
> compatible = "renesas,r9a07g043u-irqc",
>"renesas,rzg2l-irqc";
--
Regards,
Laurent Pinchart
Re: [PATCH v2 5/9] drm: renesas: rz-du: Add RZ/G2UL DU Support
Hi Biju,
Thank you for the patch.
On Tue, Jul 09, 2024 at 02:51:43PM +0100, Biju Das wrote:
> The LCD controller is composed of Frame Compression Processor (FCPVD),
> Video Signal Processor (VSPD), and Display Unit (DU).
>
> It has DPI interface and supports a maximum resolution of WXGA along
> with 2 RPFs to support the blending of two picture layers and raster
> operations (ROPs).
>
> The DU module is connected to VSPD. Add RZ/G2UL DU support.
>
> Signed-off-by: Biju Das
> ---
> v1->v2:
> * No change.
> ---
> drivers/gpu/drm/renesas/rz-du/rzg2l_du_crtc.c | 9 -
> drivers/gpu/drm/renesas/rz-du/rzg2l_du_drv.c | 11 +++
> 2 files changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/renesas/rz-du/rzg2l_du_crtc.c
> b/drivers/gpu/drm/renesas/rz-du/rzg2l_du_crtc.c
> index 6e7aac6219be..b1812f947252 100644
> --- a/drivers/gpu/drm/renesas/rz-du/rzg2l_du_crtc.c
> +++ b/drivers/gpu/drm/renesas/rz-du/rzg2l_du_crtc.c
> @@ -28,6 +28,7 @@
> #include "rzg2l_du_vsp.h"
>
> #define DU_MCR0 0x00
> +#define DU_MCR0_DPI_OE BIT(0)
> #define DU_MCR0_DI_ENBIT(8)
>
> #define DU_DITR0 0x10
> @@ -216,9 +217,15 @@ static void rzg2l_du_crtc_put(struct rzg2l_du_crtc
> *rcrtc)
>
> static void rzg2l_du_start_stop(struct rzg2l_du_crtc *rcrtc, bool start)
> {
> + struct rzg2l_du_crtc_state *rstate =
> + to_rzg2l_crtc_state(rcrtc->crtc.state);
I think you can avoid the line break here.
> struct rzg2l_du_device *rcdu = rcrtc->dev;
> + u32 val = DU_MCR0_DI_EN;
>
> - writel(start ? DU_MCR0_DI_EN : 0, rcdu->mmio + DU_MCR0);
> + if (rstate->outputs == BIT(RZG2L_DU_OUTPUT_DPAD0))
> + val |= DU_MCR0_DPI_OE;
> +
> + writel(start ? val : 0, rcdu->mmio + DU_MCR0);
> }
>
> static void rzg2l_du_crtc_start(struct rzg2l_du_crtc *rcrtc)
> diff --git a/drivers/gpu/drm/renesas/rz-du/rzg2l_du_drv.c
> b/drivers/gpu/drm/renesas/rz-du/rzg2l_du_drv.c
> index e5eca8691a33..34534441b7ec 100644
> --- a/drivers/gpu/drm/renesas/rz-du/rzg2l_du_drv.c
> +++ b/drivers/gpu/drm/renesas/rz-du/rzg2l_du_drv.c
> @@ -25,6 +25,16 @@
> * Device Information
> */
>
> +static const struct rzg2l_du_device_info rzg2l_du_r9a07g043u_info = {
> + .channels_mask = BIT(0),
> + .routes = {
> + [RZG2L_DU_OUTPUT_DPAD0] = {
> + .possible_outputs = BIT(0),
> + .port = 1,
This may need to be port 0 depending on the outcome of the discussion on
the DT bindings.
> + },
> + },
> +};
> +
> static const struct rzg2l_du_device_info rzg2l_du_r9a07g044_info = {
> .channels_mask = BIT(0),
> .routes = {
> @@ -40,6 +50,7 @@ static const struct rzg2l_du_device_info
> rzg2l_du_r9a07g044_info = {
> };
>
> static const struct of_device_id rzg2l_du_of_table[] = {
> + { .compatible = "renesas,r9a07g043u-du", .data =
> &rzg2l_du_r9a07g043u_info },
> { .compatible = "renesas,r9a07g044-du", .data =
> &rzg2l_du_r9a07g044_info },
> { /* sentinel */ }
> };
--
Regards,
Laurent Pinchart
Re: [PATCH v2 3/9] dt-bindings: display: renesas,rzg2l-du: Document RZ/G2UL DU bindings
Hi Biju,
Thank you for the patch.
On Tue, Jul 09, 2024 at 02:51:41PM +0100, Biju Das wrote:
> Document DU found in RZ/G2UL SoC. The DU block is identical to RZ/G2L
> SoC, but has only DPI interface.
>
> While at it, add missing required property port@1 for RZ/G2L and RZ/V2L
> SoCs. Currently there is no user for the DPI interface and hence there
> won't be any ABI breakage for adding port@1 as required property for
> RZ/G2L and RZ/V2L SoCs.
>
> Signed-off-by: Biju Das
> Acked-by: Conor Dooley
> ---
> v1->v2:
> * Updated commit description related to non ABI breakage.
> * Added Ack from Conor.
> ---
> .../bindings/display/renesas,rzg2l-du.yaml| 32 +--
> 1 file changed, 29 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/display/renesas,rzg2l-du.yaml
> b/Documentation/devicetree/bindings/display/renesas,rzg2l-du.yaml
> index 08e5b9478051..c0fec282fa45 100644
> --- a/Documentation/devicetree/bindings/display/renesas,rzg2l-du.yaml
> +++ b/Documentation/devicetree/bindings/display/renesas,rzg2l-du.yaml
> @@ -18,6 +18,7 @@ properties:
>compatible:
> oneOf:
>- enum:
> + - renesas,r9a07g043u-du # RZ/G2UL
>- renesas,r9a07g044-du # RZ/G2{L,LC}
>- items:
>- enum:
> @@ -60,9 +61,6 @@ properties:
> $ref: /schemas/graph.yaml#/properties/port
> unevaluatedProperties: false
>
> -required:
> - - port@0
> -
> unevaluatedProperties: false
>
>renesas,vsps:
> @@ -88,6 +86,34 @@ required:
>
> additionalProperties: false
>
> +allOf:
> + - if:
> + properties:
> +compatible:
> + contains:
> +const: renesas,r9a07g043u-du
> +then:
> + properties:
> +ports:
> + properties:
> +port@0: false
> +port@1:
> + description: DPI
> +
> + required:
> +- port@1
Why do you use port@1 for the DPI output here, and not port@0 ?
> +else:
> + properties:
> +ports:
> + properties:
> +port@0:
> + description: DSI
> + port@1:
> + description: DPI
> +
> + required:
> +- port@0
> +- port@1
You're missing a blank line here.
> examples:
># RZ/G2L DU
>- |
--
Regards,
Laurent Pinchart
CFP: Complex Cameras – Unlocking the Future of Open-Source Camera Software
Hello, Ricardo and I are co-organizing a microconference during Linux Plumbers 2024 in Vienna to discuss problems related to support of complex cameras in Linux systems, and find how to solve them. The camera hardware landscape has undergone a dramatic transformation, moving from simple frame producers to highly configurable and programmable systems. Unfortunately, open-source camera software has struggled to keep pace, creating a bottleneck that hinders innovation and limits the potential of modern camera technology. The Complex Camera microconference will bring together key stakeholders to address the urgent challenges and opportunities in open-source camera software development. Call for Proposals: We invite proposals for topics in the following and related areas: - What kind of kernel API is required for Complex Cameras? - What level of hardware documentation do we require from vendors? - In which kernel subsystems should Complex Cameras reside? - How shall the camera stack interact with other subsystems like NPUs/GPUs? - What does the perfect camera software stack look like? - How can we support dual camera stacks (open and proprietary) on top of a single upstream kernel driver? Can we support non-open features? - How can we allocate/share memory efficiently between the different subsystems? Who Should Attend: - Kernel developers - ISP vendors - OEMs - Camera software developers - Linux distribution maintainers Microconference Format: The microconference will consist of short discussion topics, introduced and moderated by the participants. Each topic lead is expected to prepare a short presentation that will be shared with all the attendees in advance so we can use the Micro Conference for questions and face to face discussions. After the conference we will divide in smaller working groups. Submission Deadline: 15th July 2024 We look forward to your contributions in making complex cameras a reality in Linux! [1] https://lpc.events/event/18/contributions/1679/ -- Regards, Laurent Pinchart
Re: [PATCH] drm: renesas: Move RZ/G2L MIPI DSI driver to rz-du
Hi Prabhakar,
Thank you for the patch.
On Tue, Jun 25, 2024 at 01:32:44PM +0100, Prabhakar wrote:
> From: Lad Prabhakar
>
> All the RZ/G2L DU specific components are located under the rz-du folder,
> so it makes sense to move the RZ/G2L MIPI DSI driver there instead of
> keeping it in the rcar-du folder. This change improves the organization
> and modularity of the driver configuration by grouping related settings
> together.
I was thinking the same the other day. Thanks for beating me at sending
a patch :-)
Reviewed-by: Laurent Pinchart
Do you or Biju has committer rights to drm-misc to push this patch ?
> Signed-off-by: Lad Prabhakar
> ---
> drivers/gpu/drm/renesas/rcar-du/Kconfig | 8
> drivers/gpu/drm/renesas/rcar-du/Makefile | 2 --
> drivers/gpu/drm/renesas/rz-du/Kconfig | 8
> drivers/gpu/drm/renesas/rz-du/Makefile| 2 ++
> .../gpu/drm/renesas/{rcar-du => rz-du}/rzg2l_mipi_dsi.c | 0
> .../drm/renesas/{rcar-du => rz-du}/rzg2l_mipi_dsi_regs.h | 0
> 6 files changed, 10 insertions(+), 10 deletions(-)
> rename drivers/gpu/drm/renesas/{rcar-du => rz-du}/rzg2l_mipi_dsi.c (100%)
> rename drivers/gpu/drm/renesas/{rcar-du => rz-du}/rzg2l_mipi_dsi_regs.h
> (100%)
>
> diff --git a/drivers/gpu/drm/renesas/rcar-du/Kconfig
> b/drivers/gpu/drm/renesas/rcar-du/Kconfig
> index 53c356aed5d5..39af73cf2092 100644
> --- a/drivers/gpu/drm/renesas/rcar-du/Kconfig
> +++ b/drivers/gpu/drm/renesas/rcar-du/Kconfig
> @@ -60,14 +60,6 @@ config DRM_RCAR_MIPI_DSI
> select DRM_MIPI_DSI
> select RESET_CONTROLLER
>
> -config DRM_RZG2L_MIPI_DSI
> - tristate "RZ/G2L MIPI DSI Encoder Support"
> - depends on DRM && DRM_BRIDGE && OF
> - depends on ARCH_RENESAS || COMPILE_TEST
> - select DRM_MIPI_DSI
> - help
> - Enable support for the RZ/G2L Display Unit embedded MIPI DSI encoders.
> -
> config DRM_RCAR_VSP
> bool "R-Car DU VSP Compositor Support" if ARM
> default y if ARM64
> diff --git a/drivers/gpu/drm/renesas/rcar-du/Makefile
> b/drivers/gpu/drm/renesas/rcar-du/Makefile
> index b8f2c82651d9..6f132325c8b7 100644
> --- a/drivers/gpu/drm/renesas/rcar-du/Makefile
> +++ b/drivers/gpu/drm/renesas/rcar-du/Makefile
> @@ -14,5 +14,3 @@ obj-$(CONFIG_DRM_RCAR_DU) += rcar-du-drm.o
> obj-$(CONFIG_DRM_RCAR_DW_HDMI) += rcar_dw_hdmi.o
> obj-$(CONFIG_DRM_RCAR_LVDS) += rcar_lvds.o
> obj-$(CONFIG_DRM_RCAR_MIPI_DSI) += rcar_mipi_dsi.o
> -
> -obj-$(CONFIG_DRM_RZG2L_MIPI_DSI) += rzg2l_mipi_dsi.o
> diff --git a/drivers/gpu/drm/renesas/rz-du/Kconfig
> b/drivers/gpu/drm/renesas/rz-du/Kconfig
> index 5f0db2c5fee6..8ec14271ebba 100644
> --- a/drivers/gpu/drm/renesas/rz-du/Kconfig
> +++ b/drivers/gpu/drm/renesas/rz-du/Kconfig
> @@ -10,3 +10,11 @@ config DRM_RZG2L_DU
> help
> Choose this option if you have an RZ/G2L alike chipset.
> If M is selected the module will be called rzg2l-du-drm.
> +
> +config DRM_RZG2L_MIPI_DSI
> + tristate "RZ/G2L MIPI DSI Encoder Support"
> + depends on DRM && DRM_BRIDGE && OF
> + depends on ARCH_RENESAS || COMPILE_TEST
> + select DRM_MIPI_DSI
> + help
> + Enable support for the RZ/G2L Display Unit embedded MIPI DSI encoders.
> diff --git a/drivers/gpu/drm/renesas/rz-du/Makefile
> b/drivers/gpu/drm/renesas/rz-du/Makefile
> index 663b82a2577f..2987900ea6b6 100644
> --- a/drivers/gpu/drm/renesas/rz-du/Makefile
> +++ b/drivers/gpu/drm/renesas/rz-du/Makefile
> @@ -6,3 +6,5 @@ rzg2l-du-drm-y := rzg2l_du_crtc.o \
>
> rzg2l-du-drm-$(CONFIG_VIDEO_RENESAS_VSP1)+= rzg2l_du_vsp.o
> obj-$(CONFIG_DRM_RZG2L_DU) += rzg2l-du-drm.o
> +
> +obj-$(CONFIG_DRM_RZG2L_MIPI_DSI) += rzg2l_mipi_dsi.o
> diff --git a/drivers/gpu/drm/renesas/rcar-du/rzg2l_mipi_dsi.c
> b/drivers/gpu/drm/renesas/rz-du/rzg2l_mipi_dsi.c
> similarity index 100%
> rename from drivers/gpu/drm/renesas/rcar-du/rzg2l_mipi_dsi.c
> rename to drivers/gpu/drm/renesas/rz-du/rzg2l_mipi_dsi.c
> diff --git a/drivers/gpu/drm/renesas/rcar-du/rzg2l_mipi_dsi_regs.h
> b/drivers/gpu/drm/renesas/rz-du/rzg2l_mipi_dsi_regs.h
> similarity index 100%
> rename from drivers/gpu/drm/renesas/rcar-du/rzg2l_mipi_dsi_regs.h
> rename to drivers/gpu/drm/renesas/rz-du/rzg2l_mipi_dsi_regs.h
--
Regards,
Laurent Pinchart
Re: [PATCH 2/4] drm: ti-sn65dsi86: Check bridge connection failure
On Thu, Jun 20, 2024 at 09:43:05AM +0300, Tomi Valkeinen wrote:
> On 19/06/2024 22:32, Laurent Pinchart wrote:
> > Hi Jacopo,
> >
> > Thank you for the patch.
> >
> > On Wed, Jun 19, 2024 at 12:22:16PM +0200, Jacopo Mondi wrote:
> >> From: Phong Hoang
> >>
> >> Add a check to the register access function when attaching a bridge
> >> device.
>
> I think the desc is missing the "why". I'm guessing it's the first
> register access to the IC, and thus verifies that it is accessible.
Isn't it a good idea in general to always check if I2C reads succeeded ?
> >>
> >> Signed-off-by: Phong Hoang
> >> Signed-off-by: Jacopo Mondi
> >
> > Reviewed-by: Laurent Pinchart
> >
> >> ---
> >> drivers/gpu/drm/bridge/ti-sn65dsi86.c | 6 +-
> >> 1 file changed, 5 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> >> b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> >> index 84698a0b27a8..b7df53577987 100644
> >> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> >> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> >> @@ -696,6 +696,7 @@ static struct ti_sn65dsi86
> >> *bridge_to_ti_sn65dsi86(struct drm_bridge *bridge)
> >>
> >> static int ti_sn_attach_host(struct auxiliary_device *adev, struct
> >> ti_sn65dsi86 *pdata)
> >> {
> >> + int ret;
> >>int val;
> >>struct mipi_dsi_host *host;
> >>struct mipi_dsi_device *dsi;
> >> @@ -720,8 +721,11 @@ static int ti_sn_attach_host(struct auxiliary_device
> >> *adev, struct ti_sn65dsi86
> >>
> >>/* check if continuous dsi clock is required or not */
> >>pm_runtime_get_sync(dev);
> >> - regmap_read(pdata->regmap, SN_DPPLL_SRC_REG, &val);
> >> + ret = regmap_read(pdata->regmap, SN_DPPLL_SRC_REG, &val);
> >>pm_runtime_put_autosuspend(dev);
> >> + if (ret)
> >> + return ret;
> >> +
> >>if (!(val & DPPLL_CLK_SRC_DSICLK))
> >>dsi->mode_flags |= MIPI_DSI_CLOCK_NON_CONTINUOUS;
> >>
--
Regards,
Laurent Pinchart
Re: [PATCH 4/4] drm: rcar-du: Add support for R8A779H0
Hi Jacopo,
Thank you for the patch.
On Wed, Jun 19, 2024 at 12:22:18PM +0200, Jacopo Mondi wrote:
> Add support for R-Car R8A779H0 V4M which has similar characteristics
> as the already supported R-Car V4H R8A779G0, but with a single output
> channel.
>
> Signed-off-by: Jacopo Mondi
>
> ---
> BSP patch
> https://github.com/renesas-rcar/linux-bsp/commit/f2fc3314dab2052240653c1a31ba3d7c7190038e
> ---
> ---
> .../bindings/display/renesas,du.yaml | 1 +
> drivers/gpu/drm/renesas/rcar-du/rcar_du_drv.c | 18 ++
> .../gpu/drm/renesas/rcar-du/rcar_du_group.c| 17 -
> 3 files changed, 31 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/display/renesas,du.yaml
> b/Documentation/devicetree/bindings/display/renesas,du.yaml
> index c5b9e6812bce..d369953f16f7 100644
> --- a/Documentation/devicetree/bindings/display/renesas,du.yaml
> +++ b/Documentation/devicetree/bindings/display/renesas,du.yaml
> @@ -41,6 +41,7 @@ properties:
>- renesas,du-r8a77995 # for R-Car D3 compatible DU
>- renesas,du-r8a779a0 # for R-Car V3U compatible DU
>- renesas,du-r8a779g0 # for R-Car V4H compatible DU
> + - renesas,du-r8a779h0 # for R-Car V4M compatible DU
>
>reg:
> maxItems: 1
This should be split to a separate patch.
You need to add a conditional validation rule below to address the
clocks, interrupts, ports, ...
> 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 dee530e4c8b2..a1d174b0b00b 100644
> --- a/drivers/gpu/drm/renesas/rcar-du/rcar_du_drv.c
> +++ b/drivers/gpu/drm/renesas/rcar-du/rcar_du_drv.c
> @@ -545,6 +545,23 @@ static const struct rcar_du_device_info
> rcar_du_r8a779g0_info = {
> .dsi_clk_mask = BIT(1) | BIT(0),
> };
>
> +static const struct rcar_du_device_info rcar_du_r8a779h0_info = {
> + .gen = 4,
> + .features = RCAR_DU_FEATURE_CRTC_IRQ
> + | RCAR_DU_FEATURE_VSP1_SOURCE
> + | RCAR_DU_FEATURE_NO_BLENDING,
> + .channels_mask = BIT(0),
> + .routes = {
> + /* R8A779H0 has a single MIPI DSI output. */
> + [RCAR_DU_OUTPUT_DSI0] = {
> + .possible_crtcs = BIT(0),
> + .port = 0,
> + },
> + },
> + .num_rpf = 5,
> + .dsi_clk_mask = BIT(0),
> +};
This looks good.
> +
> static const struct of_device_id rcar_du_of_table[] = {
> { .compatible = "renesas,du-r8a7742", .data = &rcar_du_r8a7790_info },
> { .compatible = "renesas,du-r8a7743", .data = &rzg1_du_r8a7743_info },
> @@ -571,6 +588,7 @@ static const struct of_device_id rcar_du_of_table[] = {
> { .compatible = "renesas,du-r8a77995", .data = &rcar_du_r8a7799x_info },
> { .compatible = "renesas,du-r8a779a0", .data = &rcar_du_r8a779a0_info },
> { .compatible = "renesas,du-r8a779g0", .data = &rcar_du_r8a779g0_info },
> + { .compatible = "renesas,du-r8a779h0", .data = &rcar_du_r8a779h0_info },
> { }
> };
>
> diff --git a/drivers/gpu/drm/renesas/rcar-du/rcar_du_group.c
> b/drivers/gpu/drm/renesas/rcar-du/rcar_du_group.c
> index 2ccd2581f544..361e1d01b817 100644
> --- a/drivers/gpu/drm/renesas/rcar-du/rcar_du_group.c
> +++ b/drivers/gpu/drm/renesas/rcar-du/rcar_du_group.c
> @@ -111,6 +111,8 @@ static void rcar_du_group_setup_didsr(struct
> rcar_du_group *rgrp)
> /*
>* On Gen3 dot clocks are setup through per-group registers,
>* only available when the group has two channels.
> + *
> + * R-Car V4M (R8A779H0) has only one channel, index is == 0.
Is it relevant here ?
>*/
> rcrtc = &rcdu->crtcs[rgrp->index * 2];
> num_crtcs = rgrp->num_crtcs;
> @@ -185,11 +187,16 @@ static void rcar_du_group_setup(struct rcar_du_group
> *rgrp)
> dorcr |= DORCR_PG1T | DORCR_DK1S | DORCR_PG1D_DS1;
> rcar_du_group_write(rgrp, DORCR, dorcr);
>
> - /* Apply planes to CRTCs association. */
> - mutex_lock(&rgrp->lock);
> - rcar_du_group_write(rgrp, DPTSR, (rgrp->dptsr_planes << 16) |
> - rgrp->dptsr_planes);
> - mutex_unlock(&rgrp->lock);
> + /*
> + * Apply planes to CRTCs association, skip for V4M which has a single
> + * channel.
" and doesn't implement the DPTSR register."
I'm pretty sure writing it is still harmless, but...
> + */
> + if (rcdu->info->gen < 4 || rgrp->num_crtcs > 1) {
> + mutex_lock(&rgrp->lock);
> + rcar_du_group_write(rgrp, DPTSR, (rgrp->dptsr_planes << 16) |
> + rgrp->dptsr_planes);
> + mutex_unlock(&rgrp->lock);
> + }
> }
>
> /*
--
Regards,
Laurent Pinchart
Re: [PATCH 2/4] drm: ti-sn65dsi86: Check bridge connection failure
Hi Jacopo,
Thank you for the patch.
On Wed, Jun 19, 2024 at 12:22:16PM +0200, Jacopo Mondi wrote:
> From: Phong Hoang
>
> Add a check to the register access function when attaching a bridge
> device.
>
> Signed-off-by: Phong Hoang
> Signed-off-by: Jacopo Mondi
Reviewed-by: Laurent Pinchart
> ---
> drivers/gpu/drm/bridge/ti-sn65dsi86.c | 6 +-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index 84698a0b27a8..b7df53577987 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -696,6 +696,7 @@ static struct ti_sn65dsi86 *bridge_to_ti_sn65dsi86(struct
> drm_bridge *bridge)
>
> static int ti_sn_attach_host(struct auxiliary_device *adev, struct
> ti_sn65dsi86 *pdata)
> {
> + int ret;
> int val;
> struct mipi_dsi_host *host;
> struct mipi_dsi_device *dsi;
> @@ -720,8 +721,11 @@ static int ti_sn_attach_host(struct auxiliary_device
> *adev, struct ti_sn65dsi86
>
> /* check if continuous dsi clock is required or not */
> pm_runtime_get_sync(dev);
> - regmap_read(pdata->regmap, SN_DPPLL_SRC_REG, &val);
> + ret = regmap_read(pdata->regmap, SN_DPPLL_SRC_REG, &val);
> pm_runtime_put_autosuspend(dev);
> + if (ret)
> + return ret;
> +
> if (!(val & DPPLL_CLK_SRC_DSICLK))
> dsi->mode_flags |= MIPI_DSI_CLOCK_NON_CONTINUOUS;
>
--
Regards,
Laurent Pinchart
Re: [PATCH 1/4] drm: rcar-mipi-dsi: Fix CLOCKSET1_LOCK definition
Hi Jacopo, Thank you for the patch. CC'ing Tomi. On Wed, Jun 19, 2024 at 12:22:15PM +0200, Jacopo Mondi wrote: > From: Takeshi Kihara > > Version 0.51 of the Renesas R-Car Gen4 TRM reports bit 16 of the > CLOCKSET1 register of the DSI transmitter module to be a reserved > field. > > Fix this by correcting the CLOCKSET1_LOCK definition to match the TRM > and remove the CLOCKSET1_LOCK_PHY definition, as the register is simply > called "lock" in the datasheet. > > Signed-off-by: Takeshi Kihara > Signed-off-by: Jacopo Mondi > --- > drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h > b/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h > index f8114d11f2d1..1bf9c4717d5a 100644 > --- a/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h > +++ b/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h > @@ -141,8 +141,7 @@ > #define PHYSETUP_RSTZ(1 << 0) > > #define CLOCKSET10x101c > -#define CLOCKSET1_LOCK_PHY (1 << 17) > -#define CLOCKSET1_LOCK (1 << 16) > +#define CLOCKSET1_LOCK (1 << 17) This matches the documentation, but we should get it tested on V4H to make sure it doesn't cause a regression. Tomi, would you be able to test the patch ? > #define CLOCKSET1_CLKSEL (1 << 8) > #define CLOCKSET1_CLKINSEL_EXTAL (0 << 2) > #define CLOCKSET1_CLKINSEL_DIG (1 << 2) -- Regards, Laurent Pinchart
Re: [PATCH] drm: Remove unused function rcar_cmm_write
On Wed, Jun 19, 2024 at 12:16:47PM +0300, Sergey Shtylyov wrote: > On 6/19/24 10:54 AM, Jiapeng Chong wrote: > > > The function are defined in the rcar_cmm.c file, but not called > >s/are/is/. > > > elsewhere, so delete the unused function. > >Anywhere, maybe? I'll fix those. Reviewed-by: Laurent Pinchart > > drivers/gpu/drm/renesas/rcar-du/rcar_cmm.c:35:19: warning: unused function > > 'rcar_cmm_read'. > > > > Reported-by: Abaci Robot > > Closes: https://bugzilla.openanolis.cn/show_bug.cgi?id=9364 > > Signed-off-by: Jiapeng Chong > > [...] -- Regards, Laurent Pinchart
Re: [PATCH] drm: zynqmp_dpsub: Fix an error handling path in zynqmp_dpsub_probe()
On Mon, Jun 17, 2024 at 10:43:44AM +0300, Tomi Valkeinen wrote:
> On 16/06/2024 21:43, Laurent Pinchart wrote:
> > On Thu, Jun 13, 2024 at 11:05:01AM -0400, Sean Anderson wrote:
> >> On 5/20/24 11:05, Sean Anderson wrote:
> >>> On 5/20/24 05:40, Christophe JAILLET wrote:
> >>>> If zynqmp_dpsub_drm_init() fails, we must undo the previous
> >>>> drm_bridge_add() call.
> >>>>
> >>>> Fixes: be3f3042391d ("drm: zynqmp_dpsub: Always register bridge")
> >>>> Signed-off-by: Christophe JAILLET
> >>>> ---
> >>>> Compile tested only
> >>>> ---
> >>>> drivers/gpu/drm/xlnx/zynqmp_dpsub.c | 1 +
> >>>> 1 file changed, 1 insertion(+)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
> >>>> b/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
> >>>> index face8d6b2a6f..f5781939de9c 100644
> >>>> --- a/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
> >>>> +++ b/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
> >>>> @@ -269,6 +269,7 @@ static int zynqmp_dpsub_probe(struct platform_device
> >>>> *pdev)
> >>>> return 0;
> >>>>
> >>>> err_disp:
> >>>> +drm_bridge_remove(dpsub->bridge);
> >>>> zynqmp_disp_remove(dpsub);
> >>>> err_dp:
> >>>> zynqmp_dp_remove(dpsub);
> >>>
> >>> Reviewed-by: Sean Anderson
> >>
> >> Will this be applied soon? The patch it fixes has made its way into
> >> the stable tree already.
> >
> > If someone can merge it in drm-misc that would be the fastest way.
> > Otherwise I'll send a pull request at some point, but I'm overworked at
> > the moment.
>
> Thanks, I have pushed this to drm-misc-next.
Thank you Tomi, much appreciated.
--
Regards,
Laurent Pinchart
Re: [PATCH] drm: zynqmp_dpsub: Fix an error handling path in zynqmp_dpsub_probe()
On Thu, Jun 13, 2024 at 11:05:01AM -0400, Sean Anderson wrote:
> On 5/20/24 11:05, Sean Anderson wrote:
> > On 5/20/24 05:40, Christophe JAILLET wrote:
> >> If zynqmp_dpsub_drm_init() fails, we must undo the previous
> >> drm_bridge_add() call.
> >>
> >> Fixes: be3f3042391d ("drm: zynqmp_dpsub: Always register bridge")
> >> Signed-off-by: Christophe JAILLET
> >> ---
> >> Compile tested only
> >> ---
> >> drivers/gpu/drm/xlnx/zynqmp_dpsub.c | 1 +
> >> 1 file changed, 1 insertion(+)
> >>
> >> diff --git a/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
> >> b/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
> >> index face8d6b2a6f..f5781939de9c 100644
> >> --- a/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
> >> +++ b/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
> >> @@ -269,6 +269,7 @@ static int zynqmp_dpsub_probe(struct platform_device
> >> *pdev)
> >>return 0;
> >>
> >> err_disp:
> >> + drm_bridge_remove(dpsub->bridge);
> >>zynqmp_disp_remove(dpsub);
> >> err_dp:
> >>zynqmp_dp_remove(dpsub);
> >
> > Reviewed-by: Sean Anderson
>
> Will this be applied soon? The patch it fixes has made its way into
> the stable tree already.
If someone can merge it in drm-misc that would be the fastest way.
Otherwise I'll send a pull request at some point, but I'm overworked at
the moment.
--
Regards,
Laurent Pinchart
Re: [PATCH v6,04/24] v4l: add documentation for restricted memory flag
On Wed, Jun 12, 2024 at 03:43:58PM -0400, Nicolas Dufresne wrote: > Le mercredi 12 juin 2024 à 13:37 +0900, Tomasz Figa a écrit : > > > Why is this flag needed ? Given that the usage model requires the V4L2 > > > device to be a dma buf importer, why would userspace set the > > > V4L2_BUF_CAP_SUPPORTS_RESTRICTED_MEM flag and pass a non-restricted > > > buffer to the device ? > > > > Given that the flag is specified at REQBUF / CREATE_BUFS time, it's > > actually useful to tell the driver the queue is operating in restricted > > (aka secure) mode. > > > > I suppose we could handle that at the time of a first QBUF, but that > > would make the driver initialization and validation quite a bit of pain. > > So I'd say that the design being proposed here makes things simpler and > > more clear, even if it doesn't add any extra functionality. > > There is few more reasons I notice in previous series (haven't read the > latest): > > - The driver needs to communicate through the OPTEE rather then SCP and some > communication are needed just to figure-out things like supported > profile/level > resolutions etc. > - The driver needs to allocate auxiliary buffers in secure heap too, > allocation > at runtime are not the best Will the same driver support both modes on the same system ? > Note that the discussion around this flag already took place in the very first > iteration of the serie, it was originally using a CID and that was a proposed > replacement from Hans. -- Regards, Laurent Pinchart
Re: [PATCH] drm/bridge: add missing MODULE_DESCRIPTION() macros
Hi Jeff,
Thank you for the patch.
On Sun, Jun 09, 2024 at 10:06:17AM -0700, Jeff Johnson wrote:
> make allmodconfig && make W=1 C=1 reports:
> WARNING: modpost: missing MODULE_DESCRIPTION() in
> drivers/gpu/drm/bridge/lontium-lt9611.o
> WARNING: modpost: missing MODULE_DESCRIPTION() in
> drivers/gpu/drm/bridge/lontium-lt9611uxc.o
> WARNING: modpost: missing MODULE_DESCRIPTION() in
> drivers/gpu/drm/bridge/sil-sii8620.o
> WARNING: modpost: missing MODULE_DESCRIPTION() in
> drivers/gpu/drm/bridge/sii9234.o
>
> Add the missing invocations of the MODULE_DESCRIPTION() macro.
>
> Signed-off-by: Jeff Johnson
Reviewed-by: Laurent Pinchart
> ---
> drivers/gpu/drm/bridge/lontium-lt9611.c| 1 +
> drivers/gpu/drm/bridge/lontium-lt9611uxc.c | 1 +
> drivers/gpu/drm/bridge/sii9234.c | 1 +
> drivers/gpu/drm/bridge/sil-sii8620.c | 1 +
> 4 files changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/bridge/lontium-lt9611.c
> b/drivers/gpu/drm/bridge/lontium-lt9611.c
> index b99fe87ec738..73983f9b50cb 100644
> --- a/drivers/gpu/drm/bridge/lontium-lt9611.c
> +++ b/drivers/gpu/drm/bridge/lontium-lt9611.c
> @@ -1195,4 +1195,5 @@ static struct i2c_driver lt9611_driver = {
> };
> module_i2c_driver(lt9611_driver);
>
> +MODULE_DESCRIPTION("Lontium LT9611 DSI/HDMI bridge driver");
> MODULE_LICENSE("GPL v2");
> diff --git a/drivers/gpu/drm/bridge/lontium-lt9611uxc.c
> b/drivers/gpu/drm/bridge/lontium-lt9611uxc.c
> index ab702471f3ab..724a08f526db 100644
> --- a/drivers/gpu/drm/bridge/lontium-lt9611uxc.c
> +++ b/drivers/gpu/drm/bridge/lontium-lt9611uxc.c
> @@ -1021,6 +1021,7 @@ static struct i2c_driver lt9611uxc_driver = {
> module_i2c_driver(lt9611uxc_driver);
>
> MODULE_AUTHOR("Dmitry Baryshkov ");
> +MODULE_DESCRIPTION("Lontium LT9611UXC DSI/HDMI bridge driver");
> MODULE_LICENSE("GPL v2");
>
> MODULE_FIRMWARE(FW_FILE);
> diff --git a/drivers/gpu/drm/bridge/sii9234.c
> b/drivers/gpu/drm/bridge/sii9234.c
> index d8373d918324..0c74cdc07032 100644
> --- a/drivers/gpu/drm/bridge/sii9234.c
> +++ b/drivers/gpu/drm/bridge/sii9234.c
> @@ -961,4 +961,5 @@ static struct i2c_driver sii9234_driver = {
> };
>
> module_i2c_driver(sii9234_driver);
> +MODULE_DESCRIPTION("Silicon Image SII9234 HDMI/MHL bridge driver");
> MODULE_LICENSE("GPL");
> diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c
> b/drivers/gpu/drm/bridge/sil-sii8620.c
> index 599164e3877d..6bb755e9f0a5 100644
> --- a/drivers/gpu/drm/bridge/sil-sii8620.c
> +++ b/drivers/gpu/drm/bridge/sil-sii8620.c
> @@ -2384,4 +2384,5 @@ static struct i2c_driver sii8620_driver = {
> };
>
> module_i2c_driver(sii8620_driver);
> +MODULE_DESCRIPTION("Silicon Image SiI8620 HDMI/MHL bridge driver");
> MODULE_LICENSE("GPL v2");
>
> ---
> base-commit: 19ca0d8a433ff37018f9429f7e7739e9f3d3d2b4
> change-id: 20240609-md-drivers-gpu-drm-bridge-6ab32656df86
>
--
Regards,
Laurent Pinchart
Re: [PATCH v2 resend 2/8] hwtracing: use for_each_endpoint_of_node()
On Wed, May 29, 2024 at 06:19:33PM +0300, Dan Carpenter wrote:
> On Wed, May 29, 2024 at 05:52:53PM +0300, Laurent Pinchart wrote:
> > On Wed, May 29, 2024 at 05:34:41PM +0300, Dan Carpenter wrote:
> > > On Wed, May 29, 2024 at 03:40:47AM +0300, Laurent Pinchart wrote:
> > > > > @@ -286,7 +286,7 @@ static int of_get_coresight_platform_data(struct
> > > > > device *dev,
> > > > > }
> > > > >
> > > > > /* Iterate through each output port to discover topology */
> > > > > - while ((ep = of_graph_get_next_endpoint(parent, ep))) {
> > > > > + for_each_endpoint_of_node(parent, ep) {
> > > > > /*
> > > > >* Legacy binding mixes input/output ports under the
> > > > >* same parent. So, skip the input ports if we are
> > > > > dealing
> > > >
> > > > I think there's a bug below. The loop contains
> > > >
> > > > ret = of_coresight_parse_endpoint(dev, ep, pdata);
> > > > if (ret)
> > > > return ret;
> > > >
> > > > which leaks the reference to ep. This is not introduced by this patch,
> > >
> > > Someone should create for_each_endpoint_of_node_scoped().
> > >
> > > #define for_each_endpoint_of_node_scoped(parent, child) \
> > > for (struct device_node *child __free(device_node) = \
> > > of_graph_get_next_endpoint(parent, NULL); child != NULL; \
> > > child = of_graph_get_next_endpoint(parent, child))
> >
> > I was thinking about that too :-) I wondered if we should then bother
> > taking and releasing references, given that references to the children
> > can't be leaked out of the loop. My reasoning was that the parent
> > device_node is guaranteed to be valid throughout the loop, so borrowing
> > references to children instead of creating new ones within the loop
> > should be fine. This assumes that endpoints and ports can't vanish while
> > the parent is there. Thinking further about it, it may not be a safe
> > assumption for the future. As we anyway use functions internally that
> > create new references, we can as well handle them correctly.
> >
> > Using this new macro, the loop body would need to call of_node_get() if
> > it wants to get a reference out of the loop.
>
> The child pointer is declared local to just the loop so you'd need
> create a different function scoped variable. If it's not local to the
> loop then we'd end up taking a reference on each iteration and never
> releasing anything except on error paths.
>
> > That's the right thing to
> > do, and I think it would be less error-prone than having to drop
> > references when exiting from the loop as we do today. It would still be
> > nice if we could have an API that allows catching this missing
> > of_node_get() automatically, but I don't see a simple way to do so at
> > the moment.
>
> That's an interesting point.
>
> If we did "function_scope_var = ep;" here then we'd need to take a
> second reference as you say.
Yes, that's what I meant above, sorry if that wasn't clear.
> With other cleanup stuff like kfree() it's
> very hard to miss it if we forget to call "no_free_ptr(&ep)" because
> it's on the success path. It leads to an immediate crash in testing.
> But here it's just ref counting so possibly we might miss that sort of
> bug.
All this calls for std::shared_ptr :-D
Jokes aside, I think for_each_endpoint_of_node_scoped() would still be
safer, as the number of cases where we would have to pass a reference to
the outer scope should be quite smaller than the number of cases where
we break from for_each_endpoint_of_node() loops today.
On the other hand, the consequence of a bug with
for_each_endpoint_of_node_scoped() would be a dangling reference,
instead of a reference leak with for_each_endpoint_of_node(), so it may
be more dangerous the same way that UAF is more dangerous than memory
leaks.
--
Regards,
Laurent Pinchart
Re: [PATCH v2 resend 2/8] hwtracing: use for_each_endpoint_of_node()
On Wed, May 29, 2024 at 05:34:41PM +0300, Dan Carpenter wrote:
> On Wed, May 29, 2024 at 03:40:47AM +0300, Laurent Pinchart wrote:
> > > @@ -286,7 +286,7 @@ static int of_get_coresight_platform_data(struct
> > > device *dev,
> > > }
> > >
> > > /* Iterate through each output port to discover topology */
> > > - while ((ep = of_graph_get_next_endpoint(parent, ep))) {
> > > + for_each_endpoint_of_node(parent, ep) {
> > > /*
> > >* Legacy binding mixes input/output ports under the
> > >* same parent. So, skip the input ports if we are dealing
> >
> > I think there's a bug below. The loop contains
> >
> > ret = of_coresight_parse_endpoint(dev, ep, pdata);
> > if (ret)
> > return ret;
> >
> > which leaks the reference to ep. This is not introduced by this patch,
>
> Someone should create for_each_endpoint_of_node_scoped().
>
> #define for_each_endpoint_of_node_scoped(parent, child) \
> for (struct device_node *child __free(device_node) = \
> of_graph_get_next_endpoint(parent, NULL); child != NULL; \
> child = of_graph_get_next_endpoint(parent, child))
I was thinking about that too :-) I wondered if we should then bother
taking and releasing references, given that references to the children
can't be leaked out of the loop. My reasoning was that the parent
device_node is guaranteed to be valid throughout the loop, so borrowing
references to children instead of creating new ones within the loop
should be fine. This assumes that endpoints and ports can't vanish while
the parent is there. Thinking further about it, it may not be a safe
assumption for the future. As we anyway use functions internally that
create new references, we can as well handle them correctly.
Using this new macro, the loop body would need to call of_node_get() if
it wants to get a reference out of the loop. That's the right thing to
do, and I think it would be less error-prone than having to drop
references when exiting from the loop as we do today. It would still be
nice if we could have an API that allows catching this missing
of_node_get() automatically, but I don't see a simple way to do so at
the moment.
--
Regards,
Laurent Pinchart
Re: [PATCH] drm: renesas: shmobile: Add drm_panic support
On Wed, May 29, 2024 at 04:28:44PM +0300, Dmitry Baryshkov wrote:
> On Wed, May 29, 2024 at 12:55:06PM +0300, Laurent Pinchart wrote:
> > On Wed, May 29, 2024 at 12:25:56PM +0300, Dmitry Baryshkov wrote:
> > > On Wed, May 29, 2024 at 12:10:18PM +0300, Laurent Pinchart wrote:
> > > > Hi Dmitry,
> > > >
> > > > On Wed, May 29, 2024 at 11:27:02AM +0300, Dmitry Baryshkov wrote:
> > > > > On Wed, May 29, 2024 at 04:03:20AM +0300, Laurent Pinchart wrote:
> > > > > > On Mon, May 27, 2024 at 03:34:48PM +0200, Geert Uytterhoeven wrote:
> > > > > > > Add support for the drm_panic module, which displays a message on
> > > > > > > the screen when a kernel panic occurs.
> > > > > > >
> > > > > > > Signed-off-by: Geert Uytterhoeven
> > > > > > > ---
> > > > > > > Tested on Armadillo-800-EVA.
> > > > > > > ---
> > > > > > > drivers/gpu/drm/renesas/shmobile/shmob_drm_plane.c | 14
> > > > > > > +-
> > > > > > > 1 file changed, 13 insertions(+), 1 deletion(-)
> > > > > > >
> > > > > > > diff --git a/drivers/gpu/drm/renesas/shmobile/shmob_drm_plane.c
> > > > > > > b/drivers/gpu/drm/renesas/shmobile/shmob_drm_plane.c
> > > > > > > index 07ad17d24294d5e6..9d166ab2af8bd231 100644
> > > > > > > --- a/drivers/gpu/drm/renesas/shmobile/shmob_drm_plane.c
> > > > > > > +++ b/drivers/gpu/drm/renesas/shmobile/shmob_drm_plane.c
> > > > > > > @@ -273,6 +273,13 @@ static const struct drm_plane_helper_funcs
> > > > > > > shmob_drm_plane_helper_funcs = {
> > > > > > > .atomic_disable = shmob_drm_plane_atomic_disable,
> > > > > > > };
> > > > > > >
> > > > > > > +static const struct drm_plane_helper_funcs
> > > > > > > shmob_drm_primary_plane_helper_funcs = {
> > > > > > > + .atomic_check = shmob_drm_plane_atomic_check,
> > > > > > > + .atomic_update = shmob_drm_plane_atomic_update,
> > > > > > > + .atomic_disable = shmob_drm_plane_atomic_disable,
> > > > > > > + .get_scanout_buffer = drm_fb_dma_get_scanout_buffer,
> > > > > > > +};
> > > > > > > +
> > > > > > > static const struct drm_plane_funcs shmob_drm_plane_funcs = {
> > > > > > > .update_plane = drm_atomic_helper_update_plane,
> > > > > > > .disable_plane = drm_atomic_helper_disable_plane,
> > > > > > > @@ -310,7 +317,12 @@ struct drm_plane
> > > > > > > *shmob_drm_plane_create(struct shmob_drm_device *sdev,
> > > > > > >
> > > > > > > splane->index = index;
> > > > > > >
> > > > > > > - drm_plane_helper_add(&splane->base,
> > > > > > > &shmob_drm_plane_helper_funcs);
> > > > > > > + if (type == DRM_PLANE_TYPE_PRIMARY)
> > > > > > > + drm_plane_helper_add(&splane->base,
> > > > > > > +
> > > > > > > &shmob_drm_primary_plane_helper_funcs);
> > > > > > > + else
> > > > > > > + drm_plane_helper_add(&splane->base,
> > > > > > > + &shmob_drm_plane_helper_funcs);
> > > > > >
> > > > > > It's not very nice to have to provide different operations for the
> > > > > > primary and overlay planes. The documentation of
> > > > > > drm_fb_dma_get_scanout_buffer() states
> > > > > >
> > > > > > * @plane: DRM primary plane
> > > > > >
> > > > > > If the intent is that only primary planes will be used to display
> > > > > > the
> > > > > > panic message, shouldn't drm_panic_register() skip overlay planes ?
> > > > > > It
> > > > > > would simplify drivers.
> > > > >
> > > > > What about the drivers where all the planes are actually universal?
> > > > > In such a case the planes registered as primary can easily get
> > > > > replaced
> > > > > by 'overlay' planes.
> > > >
> > > >
Re: [PATCH] drm: renesas: shmobile: Add drm_panic support
On Wed, May 29, 2024 at 12:25:56PM +0300, Dmitry Baryshkov wrote:
> On Wed, May 29, 2024 at 12:10:18PM +0300, Laurent Pinchart wrote:
> > Hi Dmitry,
> >
> > On Wed, May 29, 2024 at 11:27:02AM +0300, Dmitry Baryshkov wrote:
> > > On Wed, May 29, 2024 at 04:03:20AM +0300, Laurent Pinchart wrote:
> > > > On Mon, May 27, 2024 at 03:34:48PM +0200, Geert Uytterhoeven wrote:
> > > > > Add support for the drm_panic module, which displays a message on
> > > > > the screen when a kernel panic occurs.
> > > > >
> > > > > Signed-off-by: Geert Uytterhoeven
> > > > > ---
> > > > > Tested on Armadillo-800-EVA.
> > > > > ---
> > > > > drivers/gpu/drm/renesas/shmobile/shmob_drm_plane.c | 14
> > > > > +-
> > > > > 1 file changed, 13 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/renesas/shmobile/shmob_drm_plane.c
> > > > > b/drivers/gpu/drm/renesas/shmobile/shmob_drm_plane.c
> > > > > index 07ad17d24294d5e6..9d166ab2af8bd231 100644
> > > > > --- a/drivers/gpu/drm/renesas/shmobile/shmob_drm_plane.c
> > > > > +++ b/drivers/gpu/drm/renesas/shmobile/shmob_drm_plane.c
> > > > > @@ -273,6 +273,13 @@ static const struct drm_plane_helper_funcs
> > > > > shmob_drm_plane_helper_funcs = {
> > > > > .atomic_disable = shmob_drm_plane_atomic_disable,
> > > > > };
> > > > >
> > > > > +static const struct drm_plane_helper_funcs
> > > > > shmob_drm_primary_plane_helper_funcs = {
> > > > > + .atomic_check = shmob_drm_plane_atomic_check,
> > > > > + .atomic_update = shmob_drm_plane_atomic_update,
> > > > > + .atomic_disable = shmob_drm_plane_atomic_disable,
> > > > > + .get_scanout_buffer = drm_fb_dma_get_scanout_buffer,
> > > > > +};
> > > > > +
> > > > > static const struct drm_plane_funcs shmob_drm_plane_funcs = {
> > > > > .update_plane = drm_atomic_helper_update_plane,
> > > > > .disable_plane = drm_atomic_helper_disable_plane,
> > > > > @@ -310,7 +317,12 @@ struct drm_plane *shmob_drm_plane_create(struct
> > > > > shmob_drm_device *sdev,
> > > > >
> > > > > splane->index = index;
> > > > >
> > > > > - drm_plane_helper_add(&splane->base,
> > > > > &shmob_drm_plane_helper_funcs);
> > > > > + if (type == DRM_PLANE_TYPE_PRIMARY)
> > > > > + drm_plane_helper_add(&splane->base,
> > > > > +
> > > > > &shmob_drm_primary_plane_helper_funcs);
> > > > > + else
> > > > > + drm_plane_helper_add(&splane->base,
> > > > > + &shmob_drm_plane_helper_funcs);
> > > >
> > > > It's not very nice to have to provide different operations for the
> > > > primary and overlay planes. The documentation of
> > > > drm_fb_dma_get_scanout_buffer() states
> > > >
> > > > * @plane: DRM primary plane
> > > >
> > > > If the intent is that only primary planes will be used to display the
> > > > panic message, shouldn't drm_panic_register() skip overlay planes ? It
> > > > would simplify drivers.
> > >
> > > What about the drivers where all the planes are actually universal?
> > > In such a case the planes registered as primary can easily get replaced
> > > by 'overlay' planes.
> >
> > Good point.
> >
> > Another option, if we wanted to avoid duplicating the drm_plane_funcs,
> > would be to add a field to drm_plane to indicate whether the plane is
> > suitable for drm_panic.
>
> ... or maybe let the driver decide. For the fully-universal-plane
> devices we probably want to select the planes which cover the largest
> part of the CRTC.
Are there devices where certain planes can only cover a subset of the
CRTC (apart from planes meant for cursors) ?
I think that what would matter the most in the end is selecting the
plane that is on top of the stack, and that doesn't seem to be addressed
by the drm_panic infrastructure. This is getting out of scope for this
patch though :-)
> > I don't think this patch should be blocked just for this reason, but I'm
> > a bit bothered by duplicating the ops structure to indicate drm_panic
> > support.
> >
> > > > >
> > > > > return &splane->base;
> > > > > }
--
Regards,
Laurent Pinchart
Re: [PATCH] drm: renesas: shmobile: Add drm_panic support
Hi Dmitry,
On Wed, May 29, 2024 at 11:27:02AM +0300, Dmitry Baryshkov wrote:
> On Wed, May 29, 2024 at 04:03:20AM +0300, Laurent Pinchart wrote:
> > On Mon, May 27, 2024 at 03:34:48PM +0200, Geert Uytterhoeven wrote:
> > > Add support for the drm_panic module, which displays a message on
> > > the screen when a kernel panic occurs.
> > >
> > > Signed-off-by: Geert Uytterhoeven
> > > ---
> > > Tested on Armadillo-800-EVA.
> > > ---
> > > drivers/gpu/drm/renesas/shmobile/shmob_drm_plane.c | 14 +-
> > > 1 file changed, 13 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/renesas/shmobile/shmob_drm_plane.c
> > > b/drivers/gpu/drm/renesas/shmobile/shmob_drm_plane.c
> > > index 07ad17d24294d5e6..9d166ab2af8bd231 100644
> > > --- a/drivers/gpu/drm/renesas/shmobile/shmob_drm_plane.c
> > > +++ b/drivers/gpu/drm/renesas/shmobile/shmob_drm_plane.c
> > > @@ -273,6 +273,13 @@ static const struct drm_plane_helper_funcs
> > > shmob_drm_plane_helper_funcs = {
> > > .atomic_disable = shmob_drm_plane_atomic_disable,
> > > };
> > >
> > > +static const struct drm_plane_helper_funcs
> > > shmob_drm_primary_plane_helper_funcs = {
> > > + .atomic_check = shmob_drm_plane_atomic_check,
> > > + .atomic_update = shmob_drm_plane_atomic_update,
> > > + .atomic_disable = shmob_drm_plane_atomic_disable,
> > > + .get_scanout_buffer = drm_fb_dma_get_scanout_buffer,
> > > +};
> > > +
> > > static const struct drm_plane_funcs shmob_drm_plane_funcs = {
> > > .update_plane = drm_atomic_helper_update_plane,
> > > .disable_plane = drm_atomic_helper_disable_plane,
> > > @@ -310,7 +317,12 @@ struct drm_plane *shmob_drm_plane_create(struct
> > > shmob_drm_device *sdev,
> > >
> > > splane->index = index;
> > >
> > > - drm_plane_helper_add(&splane->base, &shmob_drm_plane_helper_funcs);
> > > + if (type == DRM_PLANE_TYPE_PRIMARY)
> > > + drm_plane_helper_add(&splane->base,
> > > + &shmob_drm_primary_plane_helper_funcs);
> > > + else
> > > + drm_plane_helper_add(&splane->base,
> > > + &shmob_drm_plane_helper_funcs);
> >
> > It's not very nice to have to provide different operations for the
> > primary and overlay planes. The documentation of
> > drm_fb_dma_get_scanout_buffer() states
> >
> > * @plane: DRM primary plane
> >
> > If the intent is that only primary planes will be used to display the
> > panic message, shouldn't drm_panic_register() skip overlay planes ? It
> > would simplify drivers.
>
> What about the drivers where all the planes are actually universal?
> In such a case the planes registered as primary can easily get replaced
> by 'overlay' planes.
Good point.
Another option, if we wanted to avoid duplicating the drm_plane_funcs,
would be to add a field to drm_plane to indicate whether the plane is
suitable for drm_panic.
I don't think this patch should be blocked just for this reason, but I'm
a bit bothered by duplicating the ops structure to indicate drm_panic
support.
> > >
> > > return &splane->base;
> > > }
--
Regards,
Laurent Pinchart
Re: [PATCH] drm: renesas: rcar-du: Add drm_panic support for non-vsp
Hi Geert,
Thank you for the patch.
On Mon, May 27, 2024 at 03:35:49PM +0200, Geert Uytterhoeven wrote:
> Add support for the drm_panic module for DU variants not using the
> VSP-compositor, to display a message on the screen when a kernel panic
> occurs.
>
> Signed-off-by: Geert Uytterhoeven
> ---
> Tested on Koelsch (R-Car M2-W).
>
> Support for DU variants using the VSP-compositor is more convoluted,
> and left to the DU experts.
That's not high on my priority list, so if anyone wants to play, be my
guest :-)
> ---
> drivers/gpu/drm/renesas/rcar-du/rcar_du_plane.c | 14 --
> 1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/renesas/rcar-du/rcar_du_plane.c
> b/drivers/gpu/drm/renesas/rcar-du/rcar_du_plane.c
> index e445fac8e0b46c21..c546ab0805d656f6 100644
> --- a/drivers/gpu/drm/renesas/rcar-du/rcar_du_plane.c
> +++ b/drivers/gpu/drm/renesas/rcar-du/rcar_du_plane.c
> @@ -680,6 +680,12 @@ static const struct drm_plane_helper_funcs
> rcar_du_plane_helper_funcs = {
> .atomic_update = rcar_du_plane_atomic_update,
> };
>
> +static const struct drm_plane_helper_funcs
> rcar_du_primary_plane_helper_funcs = {
> + .atomic_check = rcar_du_plane_atomic_check,
> + .atomic_update = rcar_du_plane_atomic_update,
> + .get_scanout_buffer = drm_fb_dma_get_scanout_buffer,
> +};
> +
> static struct drm_plane_state *
> rcar_du_plane_atomic_duplicate_state(struct drm_plane *plane)
> {
> @@ -812,8 +818,12 @@ int rcar_du_planes_init(struct rcar_du_group *rgrp)
> if (ret < 0)
> return ret;
>
> - drm_plane_helper_add(&plane->plane,
> - &rcar_du_plane_helper_funcs);
> + if (type == DRM_PLANE_TYPE_PRIMARY)
> + drm_plane_helper_add(&plane->plane,
> +
> &rcar_du_primary_plane_helper_funcs);
> + else
> + drm_plane_helper_add(&plane->plane,
> + &rcar_du_plane_helper_funcs);
Same comment as for the shmobile-drm patch. Let's discuss it there.
>
> drm_plane_create_alpha_property(&plane->plane);
>
--
Regards,
Laurent Pinchart
Re: [PATCH] drm: renesas: shmobile: Add drm_panic support
Hi Geert,
Thank you for the patch.
On Mon, May 27, 2024 at 03:34:48PM +0200, Geert Uytterhoeven wrote:
> Add support for the drm_panic module, which displays a message on
> the screen when a kernel panic occurs.
>
> Signed-off-by: Geert Uytterhoeven
> ---
> Tested on Armadillo-800-EVA.
> ---
> drivers/gpu/drm/renesas/shmobile/shmob_drm_plane.c | 14 +-
> 1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/renesas/shmobile/shmob_drm_plane.c
> b/drivers/gpu/drm/renesas/shmobile/shmob_drm_plane.c
> index 07ad17d24294d5e6..9d166ab2af8bd231 100644
> --- a/drivers/gpu/drm/renesas/shmobile/shmob_drm_plane.c
> +++ b/drivers/gpu/drm/renesas/shmobile/shmob_drm_plane.c
> @@ -273,6 +273,13 @@ static const struct drm_plane_helper_funcs
> shmob_drm_plane_helper_funcs = {
> .atomic_disable = shmob_drm_plane_atomic_disable,
> };
>
> +static const struct drm_plane_helper_funcs
> shmob_drm_primary_plane_helper_funcs = {
> + .atomic_check = shmob_drm_plane_atomic_check,
> + .atomic_update = shmob_drm_plane_atomic_update,
> + .atomic_disable = shmob_drm_plane_atomic_disable,
> + .get_scanout_buffer = drm_fb_dma_get_scanout_buffer,
> +};
> +
> static const struct drm_plane_funcs shmob_drm_plane_funcs = {
> .update_plane = drm_atomic_helper_update_plane,
> .disable_plane = drm_atomic_helper_disable_plane,
> @@ -310,7 +317,12 @@ struct drm_plane *shmob_drm_plane_create(struct
> shmob_drm_device *sdev,
>
> splane->index = index;
>
> - drm_plane_helper_add(&splane->base, &shmob_drm_plane_helper_funcs);
> + if (type == DRM_PLANE_TYPE_PRIMARY)
> + drm_plane_helper_add(&splane->base,
> + &shmob_drm_primary_plane_helper_funcs);
> + else
> + drm_plane_helper_add(&splane->base,
> + &shmob_drm_plane_helper_funcs);
It's not very nice to have to provide different operations for the
primary and overlay planes. The documentation of
drm_fb_dma_get_scanout_buffer() states
* @plane: DRM primary plane
If the intent is that only primary planes will be used to display the
panic message, shouldn't drm_panic_register() skip overlay planes ? It
would simplify drivers.
>
> return &splane->base;
> }
--
Regards,
Laurent Pinchart
Re: [PATCH v2 resend 8/8] fbdev: omapfb: use of_graph_get_remote_port()
Hi Morimoto-san,
Thank you for the patch.
On Tue, May 28, 2024 at 11:55:59PM +, Kuninori Morimoto wrote:
> We already have of_graph_get_remote_port(), Let's use it.
>
> Signed-off-by: Kuninori Morimoto
Reviewed-by: Laurent Pinchart
> ---
> drivers/video/fbdev/omap2/omapfb/dss/dss-of.c | 15 +--
> 1 file changed, 1 insertion(+), 14 deletions(-)
>
> diff --git a/drivers/video/fbdev/omap2/omapfb/dss/dss-of.c
> b/drivers/video/fbdev/omap2/omapfb/dss/dss-of.c
> index 14965a3fd05b7..4040e247e026e 100644
> --- a/drivers/video/fbdev/omap2/omapfb/dss/dss-of.c
> +++ b/drivers/video/fbdev/omap2/omapfb/dss/dss-of.c
> @@ -117,19 +117,6 @@ u32 dss_of_port_get_port_number(struct device_node *port)
> return reg;
> }
>
> -static struct device_node *omapdss_of_get_remote_port(const struct
> device_node *node)
> -{
> - struct device_node *np;
> -
> - np = of_graph_get_remote_endpoint(node);
> - if (!np)
> - return NULL;
> -
> - np = of_get_next_parent(np);
> -
> - return np;
> -}
> -
> struct omap_dss_device *
> omapdss_of_find_source_for_first_ep(struct device_node *node)
> {
> @@ -141,7 +128,7 @@ omapdss_of_find_source_for_first_ep(struct device_node
> *node)
> if (!ep)
> return ERR_PTR(-EINVAL);
>
> - src_port = omapdss_of_get_remote_port(ep);
> + src_port = of_graph_get_remote_port(ep);
> if (!src_port) {
> of_node_put(ep);
> return ERR_PTR(-EINVAL);
--
Regards,
Laurent Pinchart
Re: [PATCH v2 resend 7/8] video: fbdev: use for_each_endpoint_of_node()
Hi Morimoto-san,
Thank you for the patch.
On Tue, May 28, 2024 at 11:55:55PM +, Kuninori Morimoto wrote:
> We already have for_each_endpoint_of_node(), don't use
> of_graph_get_next_endpoint() directly. Replace it.
>
> Signed-off-by: Kuninori Morimoto
Reviewed-by: Laurent Pinchart
> ---
> drivers/video/fbdev/omap2/omapfb/dss/omapdss-boot-init.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/video/fbdev/omap2/omapfb/dss/omapdss-boot-init.c
> b/drivers/video/fbdev/omap2/omapfb/dss/omapdss-boot-init.c
> index 09f719af0d0c9..d80720c843235 100644
> --- a/drivers/video/fbdev/omap2/omapfb/dss/omapdss-boot-init.c
> +++ b/drivers/video/fbdev/omap2/omapfb/dss/omapdss-boot-init.c
> @@ -149,8 +149,7 @@ static void __init omapdss_walk_device(struct device_node
> *node, bool root)
>
> of_node_put(n);
>
> - n = NULL;
> - while ((n = of_graph_get_next_endpoint(node, n)) != NULL) {
> + for_each_endpoint_of_node(node, n) {
> struct device_node *pn;
>
> pn = of_graph_get_remote_port_parent(n);
--
Regards,
Laurent Pinchart
Re: [PATCH v2 resend 6/8] staging: media: atmel: use for_each_endpoint_of_node()
Hi Morimoto-san,
Thank you for the patch.
On Tue, May 28, 2024 at 11:55:51PM +, Kuninori Morimoto wrote:
> We already have for_each_endpoint_of_node(), don't use
> of_graph_get_next_endpoint() directly. Replace it.
>
> Signed-off-by: Kuninori Morimoto
> ---
> drivers/staging/media/deprecated/atmel/atmel-sama5d2-isc.c | 6 +-
> drivers/staging/media/deprecated/atmel/atmel-sama7g5-isc.c | 6 +-
> 2 files changed, 2 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/staging/media/deprecated/atmel/atmel-sama5d2-isc.c
> b/drivers/staging/media/deprecated/atmel/atmel-sama5d2-isc.c
> index 31b2b48085c59..cbfbec0c6cb57 100644
> --- a/drivers/staging/media/deprecated/atmel/atmel-sama5d2-isc.c
> +++ b/drivers/staging/media/deprecated/atmel/atmel-sama5d2-isc.c
> @@ -340,13 +340,9 @@ static int isc_parse_dt(struct device *dev, struct
> isc_device *isc)
>
> INIT_LIST_HEAD(&isc->subdev_entities);
>
> - while (1) {
> + for_each_endpoint_of_node(np, epn) {
I think epn doesn't have to be initialized to NULL anymore. Same below.
Reviewed-by: Laurent Pinchart
> struct v4l2_fwnode_endpoint v4l2_epn = { .bus_type = 0 };
>
> - epn = of_graph_get_next_endpoint(np, epn);
> - if (!epn)
> - return 0;
> -
> ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(epn),
>&v4l2_epn);
> if (ret) {
> diff --git a/drivers/staging/media/deprecated/atmel/atmel-sama7g5-isc.c
> b/drivers/staging/media/deprecated/atmel/atmel-sama7g5-isc.c
> index 020034f631f57..7c477b1d3c484 100644
> --- a/drivers/staging/media/deprecated/atmel/atmel-sama7g5-isc.c
> +++ b/drivers/staging/media/deprecated/atmel/atmel-sama7g5-isc.c
> @@ -326,13 +326,9 @@ static int xisc_parse_dt(struct device *dev, struct
> isc_device *isc)
>
> mipi_mode = of_property_read_bool(np, "microchip,mipi-mode");
>
> - while (1) {
> + for_each_endpoint_of_node(np, epn) {
> struct v4l2_fwnode_endpoint v4l2_epn = { .bus_type = 0 };
>
> - epn = of_graph_get_next_endpoint(np, epn);
> - if (!epn)
> - return 0;
> -
> ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(epn),
>&v4l2_epn);
> if (ret) {
--
Regards,
Laurent Pinchart
Re: [PATCH v2 resend 5/8] media: platform: xilinx: use for_each_endpoint_of_node()
Hi Morimoto-san,
Thank you for the patch.
On Tue, May 28, 2024 at 11:55:46PM +, Kuninori Morimoto wrote:
> We already have for_each_endpoint_of_node(), don't use
> of_graph_get_next_endpoint() directly. Replace it.
>
> Signed-off-by: Kuninori Morimoto
> ---
> drivers/media/platform/xilinx/xilinx-vipp.c | 7 +--
> 1 file changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/drivers/media/platform/xilinx/xilinx-vipp.c
> b/drivers/media/platform/xilinx/xilinx-vipp.c
> index 996684a730383..38818b82a575e 100644
> --- a/drivers/media/platform/xilinx/xilinx-vipp.c
> +++ b/drivers/media/platform/xilinx/xilinx-vipp.c
> @@ -205,12 +205,7 @@ static int xvip_graph_build_dma(struct
> xvip_composite_device *xdev)
I think ep doesn't have to be initialized to NULL anymore.
Reviewed-by: Laurent Pinchart
>
> dev_dbg(xdev->dev, "creating links for DMA engines\n");
>
> - while (1) {
> - /* Get the next endpoint and parse its link. */
> - ep = of_graph_get_next_endpoint(node, ep);
> - if (ep == NULL)
> - break;
> -
> + for_each_endpoint_of_node(node, ep) {
> dev_dbg(xdev->dev, "processing endpoint %pOF\n", ep);
>
> ret = v4l2_fwnode_parse_link(of_fwnode_handle(ep), &link);
--
Regards,
Laurent Pinchart
Re: [PATCH v2 resend 4/8] media: platform: ti: use for_each_endpoint_of_node()
Hello Morimoto-san,
Thank you for the patch.
On Tue, May 28, 2024 at 11:55:42PM +, Kuninori Morimoto wrote:
> We already have for_each_endpoint_of_node(), don't use
> of_graph_get_next_endpoint() directly. Replace it.
>
> Signed-off-by: Kuninori Morimoto
Reviewed-by: Laurent Pinchart
> ---
> drivers/media/platform/ti/am437x/am437x-vpfe.c | 12 +---
> drivers/media/platform/ti/davinci/vpif_capture.c | 12 ++--
> 2 files changed, 11 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/media/platform/ti/am437x/am437x-vpfe.c
> b/drivers/media/platform/ti/am437x/am437x-vpfe.c
> index 77e12457d1495..009ff68a2b43c 100644
> --- a/drivers/media/platform/ti/am437x/am437x-vpfe.c
> +++ b/drivers/media/platform/ti/am437x/am437x-vpfe.c
> @@ -2287,7 +2287,7 @@ static const struct v4l2_async_notifier_operations
> vpfe_async_ops = {
> static struct vpfe_config *
> vpfe_get_pdata(struct vpfe_device *vpfe)
> {
> - struct device_node *endpoint = NULL;
> + struct device_node *endpoint;
> struct device *dev = vpfe->pdev;
> struct vpfe_subdev_info *sdinfo;
> struct vpfe_config *pdata;
> @@ -2306,14 +2306,11 @@ vpfe_get_pdata(struct vpfe_device *vpfe)
> if (!pdata)
> return NULL;
>
> - for (i = 0; ; i++) {
> + i = 0;
> + for_each_endpoint_of_node(dev->of_node, endpoint) {
> struct v4l2_fwnode_endpoint bus_cfg = { .bus_type = 0 };
> struct device_node *rem;
>
> - endpoint = of_graph_get_next_endpoint(dev->of_node, endpoint);
> - if (!endpoint)
> - break;
> -
> sdinfo = &pdata->sub_devs[i];
> sdinfo->grp_id = 0;
>
> @@ -2371,9 +2368,10 @@ vpfe_get_pdata(struct vpfe_device *vpfe)
> of_node_put(rem);
> if (IS_ERR(pdata->asd[i]))
> goto cleanup;
> +
> + i++;
> }
>
> - of_node_put(endpoint);
> return pdata;
>
> cleanup:
> diff --git a/drivers/media/platform/ti/davinci/vpif_capture.c
> b/drivers/media/platform/ti/davinci/vpif_capture.c
> index c28794b6677b7..078ae11cd0787 100644
> --- a/drivers/media/platform/ti/davinci/vpif_capture.c
> +++ b/drivers/media/platform/ti/davinci/vpif_capture.c
> @@ -1517,16 +1517,12 @@ vpif_capture_get_pdata(struct platform_device *pdev,
> if (!pdata->subdev_info)
> return NULL;
>
> - for (i = 0; i < VPIF_CAPTURE_NUM_CHANNELS; i++) {
> + i = 0;
> + for_each_endpoint_of_node(pdev->dev.of_node, endpoint) {
> struct v4l2_fwnode_endpoint bus_cfg = { .bus_type = 0 };
> unsigned int flags;
> int err;
>
> - endpoint = of_graph_get_next_endpoint(pdev->dev.of_node,
> - endpoint);
> - if (!endpoint)
> - break;
> -
> rem = of_graph_get_remote_port_parent(endpoint);
> if (!rem) {
> dev_dbg(&pdev->dev, "Remote device at %pOF not found\n",
> @@ -1577,6 +1573,10 @@ vpif_capture_get_pdata(struct platform_device *pdev,
> goto err_cleanup;
>
> of_node_put(rem);
> +
> + i++;
> + if (i >= VPIF_CAPTURE_NUM_CHANNELS)
> + break;
> }
>
> done:
--
Regards,
Laurent Pinchart
Re: [PATCH v2 resend 3/8] media: platform: microchip: use for_each_endpoint_of_node()
Hello Morimoto-san,
Thank you for the patch.
On Tue, May 28, 2024 at 11:55:37PM +, Kuninori Morimoto wrote:
> We already have for_each_endpoint_of_node(), don't use
> of_graph_get_next_endpoint() directly. Replace it.
>
> Signed-off-by: Kuninori Morimoto
> ---
> .../microchip/microchip-sama5d2-isc.c | 19 +++
> .../microchip/microchip-sama7g5-isc.c | 19 +++
> 2 files changed, 14 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/media/platform/microchip/microchip-sama5d2-isc.c
> b/drivers/media/platform/microchip/microchip-sama5d2-isc.c
> index 5ac149cf3647f..d9298771f5097 100644
> --- a/drivers/media/platform/microchip/microchip-sama5d2-isc.c
> +++ b/drivers/media/platform/microchip/microchip-sama5d2-isc.c
> @@ -356,30 +356,26 @@ static int isc_parse_dt(struct device *dev, struct
> isc_device *isc)
> struct device_node *epn = NULL;
There's no need anymore to initialize epn to NULL. Same in
microchip-sama7g5-isc.c. With this addressed,
Reviewed-by: Laurent Pinchart
> struct isc_subdev_entity *subdev_entity;
> unsigned int flags;
> - int ret;
>
> INIT_LIST_HEAD(&isc->subdev_entities);
>
> - while (1) {
> + for_each_endpoint_of_node(np, epn) {
> struct v4l2_fwnode_endpoint v4l2_epn = { .bus_type = 0 };
> -
> - epn = of_graph_get_next_endpoint(np, epn);
> - if (!epn)
> - return 0;
> + int ret;
>
> ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(epn),
>&v4l2_epn);
> if (ret) {
> - ret = -EINVAL;
> + of_node_put(epn);
> dev_err(dev, "Could not parse the endpoint\n");
> - break;
> + return -EINVAL;
> }
>
> subdev_entity = devm_kzalloc(dev, sizeof(*subdev_entity),
>GFP_KERNEL);
> if (!subdev_entity) {
> - ret = -ENOMEM;
> - break;
> + of_node_put(epn);
> + return -ENOMEM;
> }
> subdev_entity->epn = epn;
>
> @@ -400,9 +396,8 @@ static int isc_parse_dt(struct device *dev, struct
> isc_device *isc)
>
> list_add_tail(&subdev_entity->list, &isc->subdev_entities);
> }
> - of_node_put(epn);
>
> - return ret;
> + return 0;
> }
>
> static int microchip_isc_probe(struct platform_device *pdev)
> diff --git a/drivers/media/platform/microchip/microchip-sama7g5-isc.c
> b/drivers/media/platform/microchip/microchip-sama7g5-isc.c
> index 73445f33d26ba..36204fee10aa2 100644
> --- a/drivers/media/platform/microchip/microchip-sama7g5-isc.c
> +++ b/drivers/media/platform/microchip/microchip-sama7g5-isc.c
> @@ -339,33 +339,29 @@ static int xisc_parse_dt(struct device *dev, struct
> isc_device *isc)
> struct device_node *epn = NULL;
> struct isc_subdev_entity *subdev_entity;
> unsigned int flags;
> - int ret;
> bool mipi_mode;
>
> INIT_LIST_HEAD(&isc->subdev_entities);
>
> mipi_mode = of_property_read_bool(np, "microchip,mipi-mode");
>
> - while (1) {
> + for_each_endpoint_of_node(np, epn) {
> struct v4l2_fwnode_endpoint v4l2_epn = { .bus_type = 0 };
> -
> - epn = of_graph_get_next_endpoint(np, epn);
> - if (!epn)
> - return 0;
> + int ret;
>
> ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(epn),
>&v4l2_epn);
> if (ret) {
> - ret = -EINVAL;
> + of_node_put(epn);
> dev_err(dev, "Could not parse the endpoint\n");
> - break;
> + return -EINVAL;
> }
>
> subdev_entity = devm_kzalloc(dev, sizeof(*subdev_entity),
>GFP_KERNEL);
> if (!subdev_entity) {
> - ret = -ENOMEM;
> - break;
> + of_node_put(epn);
> + return -ENOMEM;
> }
> subdev_entity->epn = epn;
>
> @@ -389,9 +385,8 @@ static int xisc_parse_dt(struct device *dev, struct
> isc_device *isc)
>
> list_add_tail(&subdev_entity->list, &isc->subdev_entities);
> }
> - of_node_put(epn);
>
> - return ret;
> + return 0;
> }
>
> static int microchip_xisc_probe(struct platform_device *pdev)
--
Regards,
Laurent Pinchart
Re: [PATCH v2 resend 2/8] hwtracing: use for_each_endpoint_of_node()
Hi Morimoto-san,
Thank you for the patch.
On Tue, May 28, 2024 at 11:55:32PM +, Kuninori Morimoto wrote:
> We already have for_each_endpoint_of_node(), don't use
> of_graph_get_next_endpoint() directly. Replace it.
>
> Signed-off-by: Kuninori Morimoto
> Reviewed-by: Suzuki K Poulose
> ---
> drivers/hwtracing/coresight/coresight-platform.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-platform.c
> b/drivers/hwtracing/coresight/coresight-platform.c
> index 9d550f5697fa8..e9683e613d520 100644
> --- a/drivers/hwtracing/coresight/coresight-platform.c
> +++ b/drivers/hwtracing/coresight/coresight-platform.c
> @@ -275,7 +275,7 @@ static int of_get_coresight_platform_data(struct device
> *dev,
>*/
> if (!parent) {
> /*
> - * Avoid warnings in of_graph_get_next_endpoint()
> + * Avoid warnings in for_each_endpoint_of_node()
>* if the device doesn't have any graph connections
>*/
> if (!of_graph_is_present(node))
> @@ -286,7 +286,7 @@ static int of_get_coresight_platform_data(struct device
> *dev,
> }
>
> /* Iterate through each output port to discover topology */
> - while ((ep = of_graph_get_next_endpoint(parent, ep))) {
> + for_each_endpoint_of_node(parent, ep) {
> /*
>* Legacy binding mixes input/output ports under the
>* same parent. So, skip the input ports if we are dealing
I think there's a bug below. The loop contains
ret = of_coresight_parse_endpoint(dev, ep, pdata);
if (ret)
return ret;
which leaks the reference to ep. This is not introduced by this patch,
so
Reviewed-by: Laurent Pinchart
--
Regards,
Laurent Pinchart
Re: [PATCH v2 resend 1/8] gpu: drm: use for_each_endpoint_of_node()
Hello Morimoto-san,
Thank you for the patch.
On Tue, May 28, 2024 at 11:55:26PM +, Kuninori Morimoto wrote:
> We already have for_each_endpoint_of_node(), don't use
> of_graph_get_next_endpoint() directly. Replace it.
>
> Signed-off-by: Kuninori Morimoto
Reviewed-by: Laurent Pinchart
> ---
> drivers/gpu/drm/omapdrm/dss/base.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/omapdrm/dss/base.c
> b/drivers/gpu/drm/omapdrm/dss/base.c
> index 050ca7eafac58..5f8002f6bb7a5 100644
> --- a/drivers/gpu/drm/omapdrm/dss/base.c
> +++ b/drivers/gpu/drm/omapdrm/dss/base.c
> @@ -242,8 +242,7 @@ static void omapdss_walk_device(struct device *dev,
> struct device_node *node,
>
> of_node_put(n);
>
> - n = NULL;
> - while ((n = of_graph_get_next_endpoint(node, n)) != NULL) {
> + for_each_endpoint_of_node(node, n) {
> struct device_node *pn = of_graph_get_remote_port_parent(n);
>
> if (!pn)
--
Regards,
Laurent Pinchart
Re: [PATCH] drm: xlnx: zynqmp_dpsub: Enable plane in atomic update
On Wed, May 22, 2024 at 05:52:56PM +, Klymenko, Anatoliy wrote:
> On Wednesday, May 22, 2024 8:32 AM, Laurent Pinchart wrote:
> > On Mon, May 20, 2024 at 08:22:31PM -0700, Anatoliy Klymenko wrote:
> > > Unconditionally enable the DPSUB layer in the corresponding atomic plane
> > > update callback. Setting the new display mode may require disabling and
> > > re-enabling the CRTC. This effectively resets DPSUB to the default state
> > > with all layers disabled.
> >
> > Ah, I went through the code and I see that. Oops.
> >
> > > The original implementation of the plane atomic
> > > update enables the corresponding DPSUB layer only if the framebuffer
> > > format has changed. This would leave the layer disabled after switching to
> > > a different display mode with the same framebuffer format.
> >
> > Do we need a Fixes: tag or has this issue been there since the beginning
> > ?
>
> Yes, this was introduced in the initial driver commit.
>
> > > Signed-off-by: Anatoliy Klymenko
> > > ---
> > > drivers/gpu/drm/xlnx/zynqmp_kms.c | 5 ++---
> > > 1 file changed, 2 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/xlnx/zynqmp_kms.c
> > > b/drivers/gpu/drm/xlnx/zynqmp_kms.c
> > > index 43bf416b33d5..c4f038e34814 100644
> > > --- a/drivers/gpu/drm/xlnx/zynqmp_kms.c
> > > +++ b/drivers/gpu/drm/xlnx/zynqmp_kms.c
> > > @@ -120,9 +120,8 @@ static void
> > zynqmp_dpsub_plane_atomic_update(struct drm_plane *plane,
> > > zynqmp_disp_blend_set_global_alpha(dpsub->disp, true,
> > > plane->state->alpha >>
> > > 8);
> > >
> > > - /* Enable or re-enable the plane if the format has changed. */
> > > - if (format_changed)
> > > - zynqmp_disp_layer_enable(layer);
> > > + /* Enable or re-enable the plane. */
> > > + zynqmp_disp_layer_enable(layer);
> >
> > This should be safe for now, as the function will just write the plane
> > registers with identical values. The waste of CPU cycles may not be a
> > big issue, even if it would be best to avoid it.
>
> The CPU time wasted on doubling down layer enablement is neglectable
> compared to DP link training time.
Good point.
> > What bothers me more is that we may be working around a larger
> > problem.
> > Resetting the CRTC when disabling it affects the hardware state of the
> > whole device, and thus the state of all software DRM objects. The
> > hardware and software states lose sync. Maybe this patch is all that is
> > needed for now, but other similar issues could pop up in the future.
>
> I had similar thoughts about proper HW state tracking, but that would be
> rather large rework.
>
> > Would it be possible, at atomic check time, to detect the objects whose
> > hardware state will need to be synced, and marked that in their state ?
> > Then in zynqmp_dpsub_plane_atomic_update() you could re-enable the
> > layer
> > based on that. You may need to subclass the drm_plane_state if there's
> > no field in that structure that is suitable to store the information.
> > The format_changed local variable would move there.
>
> Thank you for the idea! I'll check it out.
If it ends up being overkill I think this patch is probably OK. A
comment to explain the reasoning in the code would be nice though.
> > > }
> > >
> > > static const struct drm_plane_helper_funcs
> > zynqmp_dpsub_plane_helper_funcs = {
> > >
> > > ---
> > > base-commit: 673087d8b023faf34b84e8faf63bbeea3da87bab
> > > change-id: 20240520-dp-layer-enable-7b561af29ca8
--
Regards,
Laurent Pinchart
Re: [PATCH] drm: zynqmp_dpsub: Fix an error handling path in zynqmp_dpsub_probe()
Hi Christophe,
Thank you for the patch.
On Mon, May 20, 2024 at 11:40:37AM +0200, Christophe JAILLET wrote:
> If zynqmp_dpsub_drm_init() fails, we must undo the previous
> drm_bridge_add() call.
>
> Fixes: be3f3042391d ("drm: zynqmp_dpsub: Always register bridge")
> Signed-off-by: Christophe JAILLET
Reviewed-by: Laurent Pinchart
> ---
> Compile tested only
> ---
> drivers/gpu/drm/xlnx/zynqmp_dpsub.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
> b/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
> index face8d6b2a6f..f5781939de9c 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
> @@ -269,6 +269,7 @@ static int zynqmp_dpsub_probe(struct platform_device
> *pdev)
> return 0;
>
> err_disp:
> + drm_bridge_remove(dpsub->bridge);
> zynqmp_disp_remove(dpsub);
> err_dp:
> zynqmp_dp_remove(dpsub);
--
Regards,
Laurent Pinchart
Re: [PATCH] drm: xlnx: zynqmp_disp: Fix WARN_ON build warning
On Wed, May 22, 2024 at 06:45:29PM +0300, Laurent Pinchart wrote:
> On Wed, May 22, 2024 at 05:44:02PM +0300, Laurent Pinchart wrote:
> > Hi Palmer,
> >
> > (CC'ing Anatoliy)
> >
> > Thank you for the patch.
> >
> > On Tue, May 21, 2024 at 07:28:15AM -0700, Palmer Dabbelt wrote:
> > > From: Palmer Dabbelt
> > >
> > > Without this I get warnings along the lines of
> > >
> > > drivers/gpu/drm/xlnx/zynqmp_disp.c:949:14: error: logical not is only
> > > applied to the left hand side of this comparison
> > > [-Werror,-Wlogical-not-parentheses]
> > > 949 | if (WARN_ON(!layer->mode ==
> > > ZYNQMP_DPSUB_LAYER_NONLIVE)) {
> > > | ^~~
> > > arch/s390/include/asm/bug.h:54:25: note: expanded from macro 'WARN_ON'
> > >54 | int __ret_warn_on = !!(x); \
> > > |^
> > > drivers/gpu/drm/xlnx/zynqmp_disp.c:949:14: note: add parentheses
> > > after the '!' to evaluate the comparison first
> > > drivers/gpu/drm/xlnx/zynqmp_disp.c:949:14: note: add parentheses
> > > around left hand side expression to silence this warning
> > >
> > > which get promoted to errors in my test builds. Adding the suggested
> > > parens elides those warnings.
> >
> > I think this should have
> >
> > Fixes: b0f0469ab662 ("drm: xlnx: zynqmp_dpsub: Anounce supported input
> > formats")
> >
> > > Reported-by: kernel test robot
> > > Closes:
> > > https://lore.kernel.org/oe-kbuild-all/202405080553.tfh9ems8-...@intel.com/
> > > Signed-off-by: Palmer Dabbelt
> > > ---
> > > I couldn't find a patch for this in Linus' tree or on the lists, sorry
> > > if someone's already fixed it. No rush on my end, I'll just stash this
> > > in a local branch for the tester.
> > > ---
> > > 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 13157da0089e..d37b4a9c99ea 100644
> > > --- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
> > > +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> > > @@ -981,7 +981,7 @@ u32 *zynqmp_disp_layer_drm_formats(struct
> > > zynqmp_disp_layer *layer,
> > > unsigned int i;
> > > u32 *formats;
> > >
> > > - if (WARN_ON(!layer->mode == ZYNQMP_DPSUB_LAYER_NONLIVE)) {
> > > + if (WARN_ON((!layer->mode) == ZYNQMP_DPSUB_LAYER_NONLIVE)) {
> >
> > That doesn't seem right. layer->mode isn't a boolean, it's an enum. The
> > right fix seems to be
> >
> > if (WARN_ON(layer->mode != ZYNQMP_DPSUB_LAYER_NONLIVE)) {
> >
> > Anatoliy, could you check this ? Palmer, do you plan to submit a new
> > version of the patch, or should I send the right fix separately ?
>
> I see a fix is already present in the drm-misc-fixes branch. Please
> ignore my previous e-mail.
I meant drm-misc-next-fixes.
> > > *num_formats = 0;
> > > return NULL;
> > > }
--
Regards,
Laurent Pinchart
Re: [PATCH] drm: xlnx: zynqmp_disp: Fix WARN_ON build warning
On Wed, May 22, 2024 at 05:44:02PM +0300, Laurent Pinchart wrote:
> Hi Palmer,
>
> (CC'ing Anatoliy)
>
> Thank you for the patch.
>
> On Tue, May 21, 2024 at 07:28:15AM -0700, Palmer Dabbelt wrote:
> > From: Palmer Dabbelt
> >
> > Without this I get warnings along the lines of
> >
> > drivers/gpu/drm/xlnx/zynqmp_disp.c:949:14: error: logical not is only
> > applied to the left hand side of this comparison
> > [-Werror,-Wlogical-not-parentheses]
> > 949 | if (WARN_ON(!layer->mode ==
> > ZYNQMP_DPSUB_LAYER_NONLIVE)) {
> > | ^~~
> > arch/s390/include/asm/bug.h:54:25: note: expanded from macro 'WARN_ON'
> >54 | int __ret_warn_on = !!(x); \
> > |^
> > drivers/gpu/drm/xlnx/zynqmp_disp.c:949:14: note: add parentheses after
> > the '!' to evaluate the comparison first
> > drivers/gpu/drm/xlnx/zynqmp_disp.c:949:14: note: add parentheses around
> > left hand side expression to silence this warning
> >
> > which get promoted to errors in my test builds. Adding the suggested
> > parens elides those warnings.
>
> I think this should have
>
> Fixes: b0f0469ab662 ("drm: xlnx: zynqmp_dpsub: Anounce supported input
> formats")
>
> > Reported-by: kernel test robot
> > Closes:
> > https://lore.kernel.org/oe-kbuild-all/202405080553.tfh9ems8-...@intel.com/
> > Signed-off-by: Palmer Dabbelt
> > ---
> > I couldn't find a patch for this in Linus' tree or on the lists, sorry
> > if someone's already fixed it. No rush on my end, I'll just stash this
> > in a local branch for the tester.
> > ---
> > 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 13157da0089e..d37b4a9c99ea 100644
> > --- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
> > +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> > @@ -981,7 +981,7 @@ u32 *zynqmp_disp_layer_drm_formats(struct
> > zynqmp_disp_layer *layer,
> > unsigned int i;
> > u32 *formats;
> >
> > - if (WARN_ON(!layer->mode == ZYNQMP_DPSUB_LAYER_NONLIVE)) {
> > + if (WARN_ON((!layer->mode) == ZYNQMP_DPSUB_LAYER_NONLIVE)) {
>
> That doesn't seem right. layer->mode isn't a boolean, it's an enum. The
> right fix seems to be
>
> if (WARN_ON(layer->mode != ZYNQMP_DPSUB_LAYER_NONLIVE)) {
>
> Anatoliy, could you check this ? Palmer, do you plan to submit a new
> version of the patch, or should I send the right fix separately ?
I see a fix is already present in the drm-misc-fixes branch. Please
ignore my previous e-mail.
> > *num_formats = 0;
> > return NULL;
> > }
--
Regards,
Laurent Pinchart
Re: [PATCH] drm: xlnx: zynqmp_dpsub: Enable plane in atomic update
Hi Anatoliy,
Thank you for the patch.
On Mon, May 20, 2024 at 08:22:31PM -0700, Anatoliy Klymenko wrote:
> Unconditionally enable the DPSUB layer in the corresponding atomic plane
> update callback. Setting the new display mode may require disabling and
> re-enabling the CRTC. This effectively resets DPSUB to the default state
> with all layers disabled.
Ah, I went through the code and I see that. Oops.
> The original implementation of the plane atomic
> update enables the corresponding DPSUB layer only if the framebuffer
> format has changed. This would leave the layer disabled after switching to
> a different display mode with the same framebuffer format.
Do we need a Fixes: tag or has this issue been there since the beginning
?
> Signed-off-by: Anatoliy Klymenko
> ---
> drivers/gpu/drm/xlnx/zynqmp_kms.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_kms.c
> b/drivers/gpu/drm/xlnx/zynqmp_kms.c
> index 43bf416b33d5..c4f038e34814 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_kms.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_kms.c
> @@ -120,9 +120,8 @@ static void zynqmp_dpsub_plane_atomic_update(struct
> drm_plane *plane,
> zynqmp_disp_blend_set_global_alpha(dpsub->disp, true,
> plane->state->alpha >> 8);
>
> - /* Enable or re-enable the plane if the format has changed. */
> - if (format_changed)
> - zynqmp_disp_layer_enable(layer);
> + /* Enable or re-enable the plane. */
> + zynqmp_disp_layer_enable(layer);
This should be safe for now, as the function will just write the plane
registers with identical values. The waste of CPU cycles may not be a
big issue, even if it would be best to avoid it.
What bothers me more is that we may be working around a larger problem.
Resetting the CRTC when disabling it affects the hardware state of the
whole device, and thus the state of all software DRM objects. The
hardware and software states lose sync. Maybe this patch is all that is
needed for now, but other similar issues could pop up in the future.
Would it be possible, at atomic check time, to detect the objects whose
hardware state will need to be synced, and marked that in their state ?
Then in zynqmp_dpsub_plane_atomic_update() you could re-enable the layer
based on that. You may need to subclass the drm_plane_state if there's
no field in that structure that is suitable to store the information.
The format_changed local variable would move there.
> }
>
> static const struct drm_plane_helper_funcs zynqmp_dpsub_plane_helper_funcs =
> {
>
> ---
> base-commit: 673087d8b023faf34b84e8faf63bbeea3da87bab
> change-id: 20240520-dp-layer-enable-7b561af29ca8
--
Regards,
Laurent Pinchart
Re: [PATCH] drm: xlnx: zynqmp_disp: Fix WARN_ON build warning
Hi Palmer,
(CC'ing Anatoliy)
Thank you for the patch.
On Tue, May 21, 2024 at 07:28:15AM -0700, Palmer Dabbelt wrote:
> From: Palmer Dabbelt
>
> Without this I get warnings along the lines of
>
> drivers/gpu/drm/xlnx/zynqmp_disp.c:949:14: error: logical not is only
> applied to the left hand side of this comparison
> [-Werror,-Wlogical-not-parentheses]
> 949 | if (WARN_ON(!layer->mode == ZYNQMP_DPSUB_LAYER_NONLIVE)) {
> | ^~~
> arch/s390/include/asm/bug.h:54:25: note: expanded from macro 'WARN_ON'
>54 | int __ret_warn_on = !!(x); \
> |^
> drivers/gpu/drm/xlnx/zynqmp_disp.c:949:14: note: add parentheses after
> the '!' to evaluate the comparison first
> drivers/gpu/drm/xlnx/zynqmp_disp.c:949:14: note: add parentheses around
> left hand side expression to silence this warning
>
> which get promoted to errors in my test builds. Adding the suggested
> parens elides those warnings.
I think this should have
Fixes: b0f0469ab662 ("drm: xlnx: zynqmp_dpsub: Anounce supported input formats")
> Reported-by: kernel test robot
> Closes:
> https://lore.kernel.org/oe-kbuild-all/202405080553.tfh9ems8-...@intel.com/
> Signed-off-by: Palmer Dabbelt
> ---
> I couldn't find a patch for this in Linus' tree or on the lists, sorry
> if someone's already fixed it. No rush on my end, I'll just stash this
> in a local branch for the tester.
> ---
> 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 13157da0089e..d37b4a9c99ea 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> @@ -981,7 +981,7 @@ u32 *zynqmp_disp_layer_drm_formats(struct
> zynqmp_disp_layer *layer,
> unsigned int i;
> u32 *formats;
>
> - if (WARN_ON(!layer->mode == ZYNQMP_DPSUB_LAYER_NONLIVE)) {
> + if (WARN_ON((!layer->mode) == ZYNQMP_DPSUB_LAYER_NONLIVE)) {
That doesn't seem right. layer->mode isn't a boolean, it's an enum. The
right fix seems to be
if (WARN_ON(layer->mode != ZYNQMP_DPSUB_LAYER_NONLIVE)) {
Anatoliy, could you check this ? Palmer, do you plan to submit a new
version of the patch, or should I send the right fix separately ?
> *num_formats = 0;
> return NULL;
> }
--
Regards,
Laurent Pinchart
Re: [PATCH v6,04/24] v4l: add documentation for restricted memory flag
Hi Jefrey, Thank you for the patch. On Thu, May 16, 2024 at 08:20:42PM +0800, Yunfei Dong wrote: > From: Jeffrey Kardatzke > > Adds documentation for V4L2_MEMORY_FLAG_RESTRICTED. > > Signed-off-by: Jeffrey Kardatzke > Signed-off-by: Yunfei Dong > --- > Documentation/userspace-api/media/v4l/buffer.rst | 10 +- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/Documentation/userspace-api/media/v4l/buffer.rst > b/Documentation/userspace-api/media/v4l/buffer.rst > index 52bbee81c080..807e43bfed2b 100644 > --- a/Documentation/userspace-api/media/v4l/buffer.rst > +++ b/Documentation/userspace-api/media/v4l/buffer.rst > @@ -696,7 +696,7 @@ enum v4l2_memory > > .. _memory-flags: > > -Memory Consistency Flags > +Memory Flags > > > .. raw:: latex > @@ -728,6 +728,14 @@ Memory Consistency Flags > only if the buffer is used for :ref:`memory mapping ` I/O and the > queue reports the :ref:`V4L2_BUF_CAP_SUPPORTS_MMAP_CACHE_HINTS > ` capability. > +* .. _`V4L2-MEMORY-FLAG-RESTRICTED`: > + > + - ``V4L2_MEMORY_FLAG_RESTRICTED`` > + - 0x0002 > + - The queued buffers are expected to be in restricted memory. If not, > an > + error will be returned. This flag can only be used with > ``V4L2_MEMORY_DMABUF``. > + Typically restricted buffers are allocated using a restricted dma-heap. > This flag > + can only be specified if the ``V4L2_BUF_CAP_SUPPORTS_RESTRICTED_MEM`` > is set. Why is this flag needed ? Given that the usage model requires the V4L2 device to be a dma buf importer, why would userspace set the V4L2_BUF_CAP_SUPPORTS_RESTRICTED_MEM flag and pass a non-restricted buffer to the device ? The V4L2_BUF_CAP_SUPPORTS_RESTRICTED_MEM flag also needs to be documented in the relevant section, I don't think that's done in this series. > > .. raw:: latex > -- Regards, Laurent Pinchart
Re: [PATCH v4 02/10] drm/bridge: add common api for inno hdmi
Hi Keith,
On Wed, May 22, 2024 at 05:58:00AM +, Keith Zhao wrote:
> Hi Alex, Laurent:
>
> I want to make as few changes as possible on the current basis, and add
> bridge_fun,
>
> On 2024年5月21日 23:42, Laurent Pinchart wrote:
> > On Tue, May 21, 2024 at 05:36:43PM +0200, Alex Bee wrote:
> > > Hi Keith,
> > >
> > > thanks a lot for working on this. See some general remarks below
> > >
> > > Am 21.05.24 um 12:58 schrieb keith:
> > > > Add INNO common api so that it can be used by vendor drivers which
> > > > implement vendor specific extensions to Innosilicon HDMI.
> > > >
> > > > Signed-off-by: keith
> > > > ---
> > > > MAINTAINERS | 2 +
> > > > drivers/gpu/drm/bridge/Kconfig| 2 +
> > > > drivers/gpu/drm/bridge/Makefile | 1 +
> > > > drivers/gpu/drm/bridge/innosilicon/Kconfig| 6 +
> > > > drivers/gpu/drm/bridge/innosilicon/Makefile | 2 +
> > > > .../gpu/drm/bridge/innosilicon/inno-hdmi.c| 587 ++
> > > > .../gpu/drm/bridge/innosilicon/inno-hdmi.h| 97 +++
> > > > include/drm/bridge/inno_hdmi.h| 69 ++
> > > > 8 files changed, 766 insertions(+)
> > > > create mode 100644 drivers/gpu/drm/bridge/innosilicon/Kconfig
> > > > create mode 100644 drivers/gpu/drm/bridge/innosilicon/Makefile
> > > > create mode 100644 drivers/gpu/drm/bridge/innosilicon/inno-hdmi.c
> > > > create mode 100644 drivers/gpu/drm/bridge/innosilicon/inno-hdmi.h
> > > > create mode 100644 include/drm/bridge/inno_hdmi.h
> > > >
> > >
> > >
> > > > + drm_encoder_helper_add(encoder, pdata->helper_private);
> > > > +
> > > > + hdmi->connector.polled = DRM_CONNECTOR_POLL_HPD;
> > > > +
> > > > + drm_connector_helper_add(&hdmi->connector,
> > > > +&inno_hdmi_connector_helper_funcs);
> > > > +
> > > > + drmm_connector_init(drm, &hdmi->connector,
> > > > + &inno_hdmi_connector_funcs,
> > > > + DRM_MODE_CONNECTOR_HDMIA,
> > > > + hdmi->ddc);
> > > > +
> > >
> > > I really don't want to anticipate bridge maintainer's feedback, but
> > > new bridge drivers must not contain connector creation. That must
> > > happen somewhere else.
> >
> > You're absolutely right :-) Connector creation should be handled by the
> > drm_bridge_connector helper. The HDMI bridge driver should focus on the
> > HDMI bridge itself.
>
> static int inno_bridge_attach(struct drm_bridge *bridge,
>enum drm_bridge_attach_flags flags)
> {
> struct inno_hdmi *hdmi = bridge_to_inno(bridge);
>
> if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
> DRM_ERROR("Fix bridge driver to make connector optional!");
> return -EINVAL;
> }
This kind of code was added to existing bridge drivers when we
transitioned to the new model where bridges don't create the connectors,
because we couldn't fix all the existing bridges in one go. New bridges
must do the opposite. See imx8qxp-pixel-link.c for instance, it has this
code instead in its attach function:
if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)) {
DRM_DEV_ERROR(pl->dev,
"do not support creating a drm_connector\n");
return -EINVAL;
}
(I would personally drop the DRM_DEV_ERROR message)
> hdmi->connector.polled = DRM_CONNECTOR_POLL_HPD;
>
> drm_connector_helper_add(&hdmi->connector,
>&inno_hdmi_connector_helper_funcs);
>
> drmm_connector_init(drm, &hdmi->connector,
> &inno_hdmi_connector_funcs,
> DRM_MODE_CONNECTOR_HDMIA,
> hdmi->ddc);
>
> drm_connector_attach_encoder(&hdmi->connector, encoder);
> }
>
> static const struct drm_bridge_funcs inno_bridge_attach = {
> .attach = inno_bridge_attach,
> };
>
> Connector creation is handled by the drm_bridge_funcs ->attach.
> Is it ok?
No, the connector should be created by the display controller driver by
calling drm_brige_connector_init(). It should not be created by the
bridge driver. The bridge driver should provide the bridge functions
(drm_bridge_funcs), but not create any connector.
> > > Also I'm neither seeing any drm_brige struct nor drm_bridge_funcs,
> > > which are both essential for a bridge driver. I don't think moving a
> > > part of a driver to .../drm/bridge/ makes it a bridge driver.
> > >
> > > > + drm_connector_attach_encoder(&hdmi->connector, encoder);
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > >
> > >
--
Regards,
Laurent Pinchart
Re: [PATCH v4 02/10] drm/bridge: add common api for inno hdmi
On Tue, May 21, 2024 at 05:36:43PM +0200, Alex Bee wrote: > Hi Keith, > > thanks a lot for working on this. See some general remarks below > > Am 21.05.24 um 12:58 schrieb keith: > > Add INNO common api so that it can be used by vendor > > drivers which implement vendor specific extensions to Innosilicon HDMI. > > > > Signed-off-by: keith > > --- > > MAINTAINERS | 2 + > > drivers/gpu/drm/bridge/Kconfig| 2 + > > drivers/gpu/drm/bridge/Makefile | 1 + > > drivers/gpu/drm/bridge/innosilicon/Kconfig| 6 + > > drivers/gpu/drm/bridge/innosilicon/Makefile | 2 + > > .../gpu/drm/bridge/innosilicon/inno-hdmi.c| 587 ++ > > .../gpu/drm/bridge/innosilicon/inno-hdmi.h| 97 +++ > > include/drm/bridge/inno_hdmi.h| 69 ++ > > 8 files changed, 766 insertions(+) > > create mode 100644 drivers/gpu/drm/bridge/innosilicon/Kconfig > > create mode 100644 drivers/gpu/drm/bridge/innosilicon/Makefile > > create mode 100644 drivers/gpu/drm/bridge/innosilicon/inno-hdmi.c > > create mode 100644 drivers/gpu/drm/bridge/innosilicon/inno-hdmi.h > > create mode 100644 include/drm/bridge/inno_hdmi.h > > > > > > + drm_encoder_helper_add(encoder, pdata->helper_private); > > + > > + hdmi->connector.polled = DRM_CONNECTOR_POLL_HPD; > > + > > + drm_connector_helper_add(&hdmi->connector, > > +&inno_hdmi_connector_helper_funcs); > > + > > + drmm_connector_init(drm, &hdmi->connector, > > + &inno_hdmi_connector_funcs, > > + DRM_MODE_CONNECTOR_HDMIA, > > + hdmi->ddc); > > + > > I really don't want to anticipate bridge maintainer's feedback, but new > bridge drivers must not contain connector creation. That must happen > somewhere else. You're absolutely right :-) Connector creation should be handled by the drm_bridge_connector helper. The HDMI bridge driver should focus on the HDMI bridge itself. > Also I'm neither seeing any drm_brige struct nor drm_bridge_funcs, which > are both essential for a bridge driver. I don't think moving a part of a > driver to .../drm/bridge/ makes it a bridge driver. > > > + drm_connector_attach_encoder(&hdmi->connector, encoder); > > + > > + return 0; > > +} > > + > > -- Regards, Laurent Pinchart
Re: [PATCH] dt-bindings: display: synopsys, dw-hdmi: Mark ddc-i2c-bus as deprecated
Hi Marek, Thank you for the patch. On Tue, May 21, 2024 at 12:40:47PM +0200, Marek Vasut wrote: > The ddc-i2c-bus property should be placed in connector node, > mark the HDMI TX side property as deprecated. > > Signed-off-by: Marek Vasut > --- > Cc: Andrzej Hajda > Cc: Conor Dooley > Cc: Daniel Vetter > Cc: David Airlie > Cc: Fabio Estevam > Cc: Jernej Skrabec > Cc: Jonas Karlman > Cc: Krzysztof Kozlowski > Cc: Laurent Pinchart > Cc: Liu Ying > Cc: Maarten Lankhorst > Cc: Mark Yao > Cc: Maxime Ripard > Cc: Neil Armstrong > Cc: Pengutronix Kernel Team > Cc: Philipp Zabel > Cc: Rob Herring > Cc: Robert Foss > Cc: Sascha Hauer > Cc: Shawn Guo > Cc: Thomas Zimmermann > Cc: devicet...@vger.kernel.org > Cc: dri-devel@lists.freedesktop.org > Cc: i...@lists.linux.dev > Cc: ker...@dh-electronics.com > Cc: linux-arm-ker...@lists.infradead.org > --- > .../devicetree/bindings/display/bridge/synopsys,dw-hdmi.yaml | 1 + > 1 file changed, 1 insertion(+) > > diff --git > a/Documentation/devicetree/bindings/display/bridge/synopsys,dw-hdmi.yaml > b/Documentation/devicetree/bindings/display/bridge/synopsys,dw-hdmi.yaml > index 828709a8ded26..d09a0bee54247 100644 > --- a/Documentation/devicetree/bindings/display/bridge/synopsys,dw-hdmi.yaml > +++ b/Documentation/devicetree/bindings/display/bridge/synopsys,dw-hdmi.yaml > @@ -47,6 +47,7 @@ properties: > >ddc-i2c-bus: > $ref: /schemas/types.yaml#/definitions/phandle > +deprecated: true > description: >The HDMI DDC bus can be connected to either a system I2C master or the >functionally-reduced I2C master contained in the DWC HDMI. When > connected How about adding an additional sentence here to explain what should be used instead ? to a system I2C master this property contains a phandle to that I2C master controller. + + This property is deprecated, the system I2C master controller should + be referenced through the ddc-i2c-bus property of the HDMI connector + node. Reviewed-by: Laurent Pinchart -- Regards, Laurent Pinchart
Re: DRM Accel BoF at Linux Plumbers
Hi Tomeu, On Sat, May 18, 2024 at 10:46:01AM +0200, Tomeu Vizoso wrote: > Hi, > > I would like to use the chance at the next Plumbers to discuss the > present challenges related to ML accelerators in mainline. > > I'm myself more oriented towards edge-oriented deployments, and don't > know enough about how these accelerators are being used in the cloud > (and maybe desktop?) to tell if there is enough overlap to warrant a > common BoF. > > In any case, these are the topics I would like to discuss, some > probably more relevant to the edge than to the cloud or desktop: > > * What is stopping vendors from mainlining their drivers? > > * How could we make it easier for them? > > * Userspace API: how close are we from a common API that we can ask > userspace drivers to implement? What can be done to further this goal? > > * Automated testing: DRM CI can be used, but would be good to have a > common test suite to run there. This is probably dependent on a common > userspace API. > > * Other shared userspace infrastructure (compiler, execution, > synchronization, virtualization, ...) > > * Firmware-mediated IP: what should we do about it, if anything? > > * Any standing issues in DRM infra (GEM, gpu scheduler, DMABuf, etc) > that are hurting accel drivers? > > What do people think, should we have a drivers/accel-wide BoF at > Plumbers? If so, what other topics should we have in the agenda? I'm interested in attending, even if so far I have limited involvement in that area. Looking forward we're considering usage of ML accelerators in libcamera for various purposes, so an open ecosystem will be crucial for us. -- Regards, Laurent Pinchart
Re: drm/bridge: adv7511: Attach next bridge without creating connector
Hi Sui,
On Thu, May 16, 2024 at 12:13:03AM +0800, Sui Jingfeng wrote:
> On 5/14/24 23:12, Laurent Pinchart wrote:
> > On Tue, May 14, 2024 at 12:26:15AM +0800, Sui Jingfeng wrote:
> >> On 5/13/24 16:02, Liu Ying wrote:
> >>> The connector is created by either this ADV7511 bridge driver or
> >>> any DRM device driver/previous bridge driver, so this ADV7511
> >>> bridge driver should not let the next bridge driver create connector.
> >>>
> >>> If the next bridge is a HDMI connector, the next bridge driver
> >>> would fail to attach bridge from display_connector_attach() without
> >>> the DRM_BRIDGE_ATTACH_NO_CONNECTOR flag.
> >
> > In theory we could have another HDMI-to-something bridge connected to
> > the ADV7511 output, and that bridge could create a connector. However,
> > before commit 14b3cdbd0e5b the adv7511 driver didn't try to attach to
> > the next bridge, so it's clear that no platform support in mainline had
> > such a setup. It should be safe to set DRM_BRIDGE_ATTACH_NO_CONNECTOR
> > unconditionally here.
>
> But what if there is a drm bridge prior to adv7511 but after the KMS
> engine? Even though we are still safe if it doesn't create connector
> by obeying modern rule.
In the "old world", that bridge wouln't have created a connector,
because it would detect that there's a next bridge. With modern KMS
drivers, DRM_BRIDGE_ATTACH_NO_CONNECTOR is passed by the
drm_bridge_connector helper to the very first bridge when attaching to
it, so no bridge will create a connector. Modern bridge drivers should
not support the !DRM_BRIDGE_ATTACH_NO_CONNECTOR case at all, they should
not offer the option of creating a connector.
> > Reviewed-by: Laurent Pinchart
> >
> >>>
> >>> Add that flag to drm_bridge_attach() function call in
> >>> adv7511_bridge_attach() to fix the issue.
> >>>
> >>> This fixes the issue where the HDMI connector bridge fails to attach
> >>> to the previous ADV7535 bridge on i.MX8MP EVK platform:
> >>>
> >>> [2.216442] [drm:drm_bridge_attach] *ERROR* failed to attach bridge
> >>> /hdmi-connector to encoder None-37: -22
> >>> [2.220675] mmc1: SDHCI controller on 30b5.mmc [30b5.mmc]
> >>> using ADMA
> >>> [2.226262] [drm:drm_bridge_attach] *ERROR* failed to attach bridge
> >>> /soc@0/bus@3080/i2c@30a3/hdmi@3d to encoder None-37: -22
> >>> [2.245204] [drm:drm_bridge_attach] *ERROR* failed to attach bridge
> >>> /soc@0/bus@32c0/dsi@32e6 to encoder None-37: -22
> >>> [2.256445] imx-lcdif 32e8.display-controller: error -EINVAL:
> >>> Failed to attach bridge for endpoint0
> >>> [2.265850] imx-lcdif 32e8.display-controller: error -EINVAL:
> >>> Cannot connect bridge
> >>> [2.274009] imx-lcdif 32e8.display-controller: probe with driver
> >>> imx-lcdif failed with error -22
> >>>
> >>> Fixes: 14b3cdbd0e5b ("drm/bridge: adv7511: make it honour next bridge in
> >>> DT")
> >>> Signed-off-by: Liu Ying
> >>> ---
> >>>drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 3 ++-
> >>>1 file changed, 2 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> >>> b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> >>> index dd21b81bd28f..66ccb61e2a66 100644
> >>> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> >>> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> >>> @@ -953,7 +953,8 @@ static int adv7511_bridge_attach(struct drm_bridge
> >>> *bridge,
> >>> int ret = 0;
> >>>
> >>> if (adv->next_bridge) {
> >>> - ret = drm_bridge_attach(bridge->encoder, adv->next_bridge,
> >>> bridge, flags);
> >>> + ret = drm_bridge_attach(bridge->encoder, adv->next_bridge,
> >>> bridge,
> >>> + flags | DRM_BRIDGE_ATTACH_NO_CONNECTOR);
> >>
> >> As a side note, I think, maybe you could do better in the future.
> >>
> >> If we know that the KMS display driver side has the HDMI connector
> >> already created for us, we should pass DRM_BRIDGE_ATTACH_NO_CONNECTOR
> >> from the root KMS driver side. Which is to forbidden all potential
> >> drm bridge drivers to create a connector in the middle.
> >
> >
Re: Safety of opening up /dev/dma_heap/* to physically present users (udev uaccess tag) ?
Hi Nicolas, On Wed, May 15, 2024 at 01:43:58PM -0400, nicolas.dufre...@collabora.corp-partner.google.com wrote: > Le mardi 14 mai 2024 à 23:42 +0300, Laurent Pinchart a écrit : > > > You'll hit the same limitation as we hit in GStreamer, which is that KMS > > > driver > > > only offer allocation for render buffers and most of them are missing > > > allocators > > > for YUV buffers, even though they can import in these formats. (kms > > > allocators, > > > except dumb, which has other issues, are format aware). > > > > My experience on Arm platforms is that the KMS drivers offer allocation > > for scanout buffers, not render buffers, and mostly using the dumb > > allocator API. If the KMS device can scan out YUV natively, YUV buffer > > allocation should be supported. Am I missing something here ? > > There is two APIs, Dumb is the legacy allocation API, only used by display Is it legacy only ? I understand the dumb buffers API to be officially supported, to allocate scanout buffers suitable for software rendering. > drivers indeed, and the API does not include a pixel format or a modifier. The > allocation of YUV buffer has been made through a small hack, > > bpp = number of bits per component (of luma plane if multiple planes) > width = width > height = height * X > > Where X will vary, "3 / 2" is used for 420 subsampling, "2" for 422 and "3" > for > 444. It is far from idea, requires deep knowledge of each formats in the > application I'm not sure I see that as an issue, but our experiences and uses cases may vary :-) > and cannot allocate each planes seperatly. For semi-planar or planar formats, unless I'm mistaken, you can either allocate a single buffer and use it with appropriate offsets when constructing your framebuffer (with DRM_IOCTL_MODE_ADDFB2), or allocate one buffer per plane. > The second is to use the driver specific allocation API. This is then > abstracted > by GBM. This allows allocating render buffers with notably modifiers and/or > use > cases. But no support for YUV formats or multi-planar formats. GBM is the way to go for render buffers indeed. It has been designed with only graphics buffer management use cases in mind, so it's unfortunately not an option as a generic allocator, at least in its current form. -- Regards, Laurent Pinchart
Re: Safety of opening up /dev/dma_heap/* to physically present users (udev uaccess tag) ?
On Thu, May 16, 2024 at 07:00:31AM +, Simon Ser wrote: > On Tuesday, May 14th, 2024 at 22:42, Laurent Pinchart wrote: > > > My experience on Arm platforms is that the KMS drivers offer allocation > > for scanout buffers, not render buffers, and mostly using the dumb > > allocator API. If the KMS device can scan out YUV natively, YUV buffer > > allocation should be supported. Am I missing something here ? > > Note that dumb buffers are only intended for simple software-rendering > use-cases. Anything more complicated (e.g. involving GPU rendering) > should use another mechanism. Sure. Even if dumb buffers may work for GPU rendering in some cases, there's no guarantee they will, so they shouldn't be used. My comment was related to scanout buffers, as I was puzzled by Nicolas mentioning how "KMS drivers only offer allocation for render buffers". On Arm platforms the render buffers are allocated on the GPU's DRM device as far as I understand, while the KMS drivers allocate scanout buffers using the dumb buffers API. -- Regards, Laurent Pinchart
Re: [PATCH v2] dt-bindings: display: synopsys,dw-hdmi: Document ddc-i2c-bus in core
Hi Marek, Thank you for the patch. On Wed, May 15, 2024 at 08:27:44AM +0200, Marek Vasut wrote: > The DW HDMI driver core is responsible for parsing the 'ddc-i2c-bus' property, > move the property description into the DW HDMI common DT schema too, so this > property can be used on all devices integrating the DW HDMI core. De-duplicating documentation is good :-) I see no reason why this property should be disallowed on any of the platforms that integrate a DW HDMI (unless that platform has no other I2C controller, but I think we can ignore that in the bindings). There could be platforms where the DW HDMI DDC pins are not exposed, making the ddc-i2c-bus property mandatory, but that's something for platform-specific bindings to handle by simply adding a required: - ddc-i2c-bus That's a separate issue. This patch looks good to me. Reviewed-by: Laurent Pinchart > Signed-off-by: Marek Vasut > --- > Cc: Andrzej Hajda > Cc: Conor Dooley > Cc: Daniel Vetter > Cc: David Airlie > Cc: Fabio Estevam > Cc: Jernej Skrabec > Cc: Jonas Karlman > Cc: Krzysztof Kozlowski > Cc: Laurent Pinchart > Cc: Liu Ying > Cc: Maarten Lankhorst > Cc: Maxime Ripard > Cc: Neil Armstrong > Cc: Pengutronix Kernel Team > Cc: Philipp Zabel > Cc: Rob Herring > Cc: Robert Foss > Cc: Sascha Hauer > Cc: Shawn Guo > Cc: Thomas Zimmermann > Cc: devicet...@vger.kernel.org > Cc: dri-devel@lists.freedesktop.org > Cc: i...@lists.linux.dev > Cc: ker...@dh-electronics.com > Cc: linux-arm-ker...@lists.infradead.org > --- > V2: Update rockchip,dw-hdmi.yaml as well > --- > .../bindings/display/bridge/synopsys,dw-hdmi.yaml | 8 > .../devicetree/bindings/display/imx/fsl,imx6-hdmi.yaml| 8 > .../bindings/display/rockchip/rockchip,dw-hdmi.yaml | 8 > 3 files changed, 8 insertions(+), 16 deletions(-) > > diff --git > a/Documentation/devicetree/bindings/display/bridge/synopsys,dw-hdmi.yaml > b/Documentation/devicetree/bindings/display/bridge/synopsys,dw-hdmi.yaml > index 4b7e54a8f037f..828709a8ded26 100644 > --- a/Documentation/devicetree/bindings/display/bridge/synopsys,dw-hdmi.yaml > +++ b/Documentation/devicetree/bindings/display/bridge/synopsys,dw-hdmi.yaml > @@ -45,6 +45,14 @@ properties: >- const: isfr > additionalItems: true > > + ddc-i2c-bus: > +$ref: /schemas/types.yaml#/definitions/phandle > +description: > + The HDMI DDC bus can be connected to either a system I2C master or the > + functionally-reduced I2C master contained in the DWC HDMI. When > connected > + to a system I2C master this property contains a phandle to that I2C > + master controller. > + >interrupts: > maxItems: 1 > > diff --git a/Documentation/devicetree/bindings/display/imx/fsl,imx6-hdmi.yaml > b/Documentation/devicetree/bindings/display/imx/fsl,imx6-hdmi.yaml > index 7979cf07f1199..180c4b510fb12 100644 > --- a/Documentation/devicetree/bindings/display/imx/fsl,imx6-hdmi.yaml > +++ b/Documentation/devicetree/bindings/display/imx/fsl,imx6-hdmi.yaml > @@ -31,14 +31,6 @@ properties: >clock-names: > maxItems: 2 > > - ddc-i2c-bus: > -$ref: /schemas/types.yaml#/definitions/phandle > -description: > - The HDMI DDC bus can be connected to either a system I2C master or the > - functionally-reduced I2C master contained in the DWC HDMI. When > connected > - to a system I2C master this property contains a phandle to that I2C > - master controller. > - >gpr: > $ref: /schemas/types.yaml#/definitions/phandle > description: > diff --git > a/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml > b/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml > index 2aac62219ff64..9d096856a79a6 100644 > --- a/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml > +++ b/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml > @@ -70,14 +70,6 @@ properties: >- vpll >- ref > > - ddc-i2c-bus: > -$ref: /schemas/types.yaml#/definitions/phandle > -description: > - The HDMI DDC bus can be connected to either a system I2C master or the > - functionally-reduced I2C master contained in the DWC HDMI. When > connected > - to a system I2C master this property contains a phandle to that I2C > - master controller. > - >phys: > maxItems: 1 > description: The HDMI PHY -- Regards, Laurent Pinchart
Re: Safety of opening up /dev/dma_heap/* to physically present users (udev uaccess tag) ?
On Mon, May 13, 2024 at 11:06:24AM -0400, Nicolas Dufresne wrote: > Le lundi 13 mai 2024 à 15:51 +0200, Maxime Ripard a écrit : > > On Mon, May 13, 2024 at 09:42:00AM -0400, Nicolas Dufresne wrote: > > > Le lundi 13 mai 2024 à 10:29 +0200, Maxime Ripard a écrit : > > > > On Wed, May 08, 2024 at 10:36:08AM +0200, Daniel Vetter wrote: > > > > > On Tue, May 07, 2024 at 04:07:39PM -0400, Nicolas Dufresne wrote: > > > > > > Le mardi 07 mai 2024 à 21:36 +0300, Laurent Pinchart a écrit : > > > > > > > Shorter term, we have a problem to solve, and the best option we > > > > > > > have > > > > > > > found so far is to rely on dma-buf heaps as a backend for the > > > > > > > frame > > > > > > > buffer allocatro helper in libcamera for the use case described > > > > > > > above. > > > > > > > This won't work in 100% of the cases, clearly. It's a stop-gap > > > > > > > measure > > > > > > > until we can do better. > > > > > > > > > > > > Considering the security concerned raised on this thread with > > > > > > dmabuf heap > > > > > > allocation not be restricted by quotas, you'd get what you want > > > > > > quickly with > > > > > > memfd + udmabuf instead (which is accounted already). > > > > > > > > > > > > It was raised that distro don't enable udmabuf, but as stated there > > > > > > by Hans, in > > > > > > any cases distro needs to take action to make the softISP works. > > > > > > This > > > > > > alternative is easy and does not interfere in anyway with your > > > > > > future plan or > > > > > > the libcamera API. You could even have both dmabuf heap (for > > > > > > Raspbian) and the > > > > > > safer memfd+udmabuf for the distro with security concerns. > > > > > > > > > > > > And for the long term plan, we can certainly get closer by fixing > > > > > > that issue > > > > > > with accounting. This issue also applied to v4l2 io-ops, so it > > > > > > would be nice to > > > > > > find common set of helpers to fix these exporters. > > > > > > > > > > Yeah if this is just for softisp, then memfd + udmabuf is also what I > > > > > was > > > > > about to suggest. Not just as a stopgap, but as the real official > > > > > thing. > > > > > > > > > > udmabuf does kinda allow you to pin memory, but we can easily fix > > > > > that by > > > > > adding the right accounting and then either let mlock rlimits or > > > > > cgroups > > > > > kernel memory limits enforce good behavior. > > > > > > > > I think the main drawback with memfd is that it'll be broken for devices > > > > without an IOMMU, and while you said that it's uncommon for GPUs, it's > > > > definitely not for codecs and display engines. > > > > > > In the context of libcamera, the allocation and the alignment done to the > > > video > > > frame is done completely blindly. In that context, there is a lot more > > > then just > > > the allocation type that can go wrong and will lead to a memory copy. The > > > upside > > > of memfd, is that the read cache will help speeding up the copies if they > > > are > > > needed. > > > > dma-heaps provide cacheable buffers too... > > Yes, and why we have cache hints in V4L2 now. There is no clue that softISP > code > can read to make the right call. The required cache management in undefined > until all the importer are known. I also don't think heaps currently care to > adapt the dmabuf sync behaviour based on the different importers, or the > addition of a new importer. On top of which, there is insufficient information > on the device to really deduce what is needed. > > > > Another important point is that this is only used if the application > > > haven't > > > provided frames. If your embedded application is non-generic, and you have > > > permissions to access the right heap, the application can solve your > > > specific > > > issue. But in the generic Linux space, Linux kernel API are just > > > insuffici
Re: Safety of opening up /dev/dma_heap/* to physically present users (udev uaccess tag) ?
On Mon, May 13, 2024 at 11:10:00AM -0400, Nicolas Dufresne wrote: > Le lundi 13 mai 2024 à 11:34 +0300, Laurent Pinchart a écrit : > > On Mon, May 13, 2024 at 10:29:22AM +0200, Maxime Ripard wrote: > > > On Wed, May 08, 2024 at 10:36:08AM +0200, Daniel Vetter wrote: > > > > On Tue, May 07, 2024 at 04:07:39PM -0400, Nicolas Dufresne wrote: > > > > > Hi, > > > > > > > > > > Le mardi 07 mai 2024 à 21:36 +0300, Laurent Pinchart a écrit : > > > > > > Shorter term, we have a problem to solve, and the best option we > > > > > > have > > > > > > found so far is to rely on dma-buf heaps as a backend for the frame > > > > > > buffer allocatro helper in libcamera for the use case described > > > > > > above. > > > > > > This won't work in 100% of the cases, clearly. It's a stop-gap > > > > > > measure > > > > > > until we can do better. > > > > > > > > > > Considering the security concerned raised on this thread with dmabuf > > > > > heap > > > > > allocation not be restricted by quotas, you'd get what you want > > > > > quickly with > > > > > memfd + udmabuf instead (which is accounted already). > > > > > > > > > > It was raised that distro don't enable udmabuf, but as stated there > > > > > by Hans, in > > > > > any cases distro needs to take action to make the softISP works. This > > > > > alternative is easy and does not interfere in anyway with your future > > > > > plan or > > > > > the libcamera API. You could even have both dmabuf heap (for > > > > > Raspbian) and the > > > > > safer memfd+udmabuf for the distro with security concerns. > > > > > > > > > > And for the long term plan, we can certainly get closer by fixing > > > > > that issue > > > > > with accounting. This issue also applied to v4l2 io-ops, so it would > > > > > be nice to > > > > > find common set of helpers to fix these exporters. > > > > > > > > Yeah if this is just for softisp, then memfd + udmabuf is also what I > > > > was > > > > about to suggest. Not just as a stopgap, but as the real official thing. > > > > > > > > udmabuf does kinda allow you to pin memory, but we can easily fix that > > > > by > > > > adding the right accounting and then either let mlock rlimits or cgroups > > > > kernel memory limits enforce good behavior. > > > > > > I think the main drawback with memfd is that it'll be broken for devices > > > without an IOMMU, and while you said that it's uncommon for GPUs, it's > > > definitely not for codecs and display engines. > > > > If the application wants to share buffers between the camera and a > > display engine or codec, it should arguably not use the libcamera > > FrameBufferAllocator, but allocate the buffers from the display or the > > encoder. memfd wouldn't be used in that case. > > > > We need to eat our own dogfood though. If we want to push the > > responsibility for buffer allocation in the buffer sharing case to the > > application, we need to modify the cam application to do so when using > > the KMS backend. > > Agreed, and the new dmabuf feedback on wayland can also be used on top of > this. > > You'll hit the same limitation as we hit in GStreamer, which is that KMS > driver > only offer allocation for render buffers and most of them are missing > allocators > for YUV buffers, even though they can import in these formats. (kms > allocators, > except dumb, which has other issues, are format aware). My experience on Arm platforms is that the KMS drivers offer allocation for scanout buffers, not render buffers, and mostly using the dumb allocator API. If the KMS device can scan out YUV natively, YUV buffer allocation should be supported. Am I missing something here ? -- Regards, Laurent Pinchart
Re: drm/bridge: adv7511: Attach next bridge without creating connector
Hello,
On Tue, May 14, 2024 at 12:26:15AM +0800, Sui Jingfeng wrote:
> On 5/13/24 16:02, Liu Ying wrote:
> > The connector is created by either this ADV7511 bridge driver or
> > any DRM device driver/previous bridge driver, so this ADV7511
> > bridge driver should not let the next bridge driver create connector.
> >
> > If the next bridge is a HDMI connector, the next bridge driver
> > would fail to attach bridge from display_connector_attach() without
> > the DRM_BRIDGE_ATTACH_NO_CONNECTOR flag.
In theory we could have another HDMI-to-something bridge connected to
the ADV7511 output, and that bridge could create a connector. However,
before commit 14b3cdbd0e5b the adv7511 driver didn't try to attach to
the next bridge, so it's clear that no platform support in mainline had
such a setup. It should be safe to set DRM_BRIDGE_ATTACH_NO_CONNECTOR
unconditionally here.
Reviewed-by: Laurent Pinchart
> >
> > Add that flag to drm_bridge_attach() function call in
> > adv7511_bridge_attach() to fix the issue.
> >
> > This fixes the issue where the HDMI connector bridge fails to attach
> > to the previous ADV7535 bridge on i.MX8MP EVK platform:
> >
> > [2.216442] [drm:drm_bridge_attach] *ERROR* failed to attach bridge
> > /hdmi-connector to encoder None-37: -22
> > [2.220675] mmc1: SDHCI controller on 30b5.mmc [30b5.mmc] using
> > ADMA
> > [2.226262] [drm:drm_bridge_attach] *ERROR* failed to attach bridge
> > /soc@0/bus@3080/i2c@30a3/hdmi@3d to encoder None-37: -22
> > [2.245204] [drm:drm_bridge_attach] *ERROR* failed to attach bridge
> > /soc@0/bus@32c0/dsi@32e6 to encoder None-37: -22
> > [2.256445] imx-lcdif 32e8.display-controller: error -EINVAL: Failed
> > to attach bridge for endpoint0
> > [2.265850] imx-lcdif 32e8.display-controller: error -EINVAL: Cannot
> > connect bridge
> > [2.274009] imx-lcdif 32e8.display-controller: probe with driver
> > imx-lcdif failed with error -22
> >
> > Fixes: 14b3cdbd0e5b ("drm/bridge: adv7511: make it honour next bridge in
> > DT")
> > Signed-off-by: Liu Ying
> > ---
> > drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> > b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> > index dd21b81bd28f..66ccb61e2a66 100644
> > --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> > +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> > @@ -953,7 +953,8 @@ static int adv7511_bridge_attach(struct drm_bridge
> > *bridge,
> > int ret = 0;
> >
> > if (adv->next_bridge) {
> > - ret = drm_bridge_attach(bridge->encoder, adv->next_bridge,
> > bridge, flags);
> > + ret = drm_bridge_attach(bridge->encoder, adv->next_bridge,
> > bridge,
> > + flags | DRM_BRIDGE_ATTACH_NO_CONNECTOR);
>
> As a side note, I think, maybe you could do better in the future.
>
> If we know that the KMS display driver side has the HDMI connector
> already created for us, we should pass DRM_BRIDGE_ATTACH_NO_CONNECTOR
> from the root KMS driver side. Which is to forbidden all potential
> drm bridge drivers to create a connector in the middle.
That's the recommended way for new drivers. Using the
drm_bridge_connector helper handles all this for you.
> The KMS display driver side could parse the DT to know if there is
> a hdmi connector, or merely just hdmi connector device node, or
> something else.
No, that would violate the basic principle of not peeking into the DT of
devices you know nothing about. The display engine driver can't walk the
pipeline in DT and expect to understand all the DT nodes on the path,
and what their properties mean.
What KMS drivers should do is to use the drm_bridge_connector helper.
Calling drm_bridge_connector_init() will create a connector for a chain
of bridges. The KMS driver should then attach to the first bridge with
DRM_BRIDGE_ATTACH_NO_CONNECTOR, unconditionally.
> However, other maintainer and/or reviewer's opinion are of cause
> more valuable. I send a A-b because I thought the bug is urgency
> and it's probably more important to solve this bug first. And
> maybe you can Cc: if you like.
>
> > if (ret)
> > return ret;
> > }
--
Regards,
Laurent Pinchart
Re: Safety of opening up /dev/dma_heap/* to physically present users (udev uaccess tag) ?
On Mon, May 13, 2024 at 10:29:22AM +0200, Maxime Ripard wrote: > On Wed, May 08, 2024 at 10:36:08AM +0200, Daniel Vetter wrote: > > On Tue, May 07, 2024 at 04:07:39PM -0400, Nicolas Dufresne wrote: > > > Hi, > > > > > > Le mardi 07 mai 2024 à 21:36 +0300, Laurent Pinchart a écrit : > > > > Shorter term, we have a problem to solve, and the best option we have > > > > found so far is to rely on dma-buf heaps as a backend for the frame > > > > buffer allocatro helper in libcamera for the use case described above. > > > > This won't work in 100% of the cases, clearly. It's a stop-gap measure > > > > until we can do better. > > > > > > Considering the security concerned raised on this thread with dmabuf heap > > > allocation not be restricted by quotas, you'd get what you want quickly > > > with > > > memfd + udmabuf instead (which is accounted already). > > > > > > It was raised that distro don't enable udmabuf, but as stated there by > > > Hans, in > > > any cases distro needs to take action to make the softISP works. This > > > alternative is easy and does not interfere in anyway with your future > > > plan or > > > the libcamera API. You could even have both dmabuf heap (for Raspbian) > > > and the > > > safer memfd+udmabuf for the distro with security concerns. > > > > > > And for the long term plan, we can certainly get closer by fixing that > > > issue > > > with accounting. This issue also applied to v4l2 io-ops, so it would be > > > nice to > > > find common set of helpers to fix these exporters. > > > > Yeah if this is just for softisp, then memfd + udmabuf is also what I was > > about to suggest. Not just as a stopgap, but as the real official thing. > > > > udmabuf does kinda allow you to pin memory, but we can easily fix that by > > adding the right accounting and then either let mlock rlimits or cgroups > > kernel memory limits enforce good behavior. > > I think the main drawback with memfd is that it'll be broken for devices > without an IOMMU, and while you said that it's uncommon for GPUs, it's > definitely not for codecs and display engines. If the application wants to share buffers between the camera and a display engine or codec, it should arguably not use the libcamera FrameBufferAllocator, but allocate the buffers from the display or the encoder. memfd wouldn't be used in that case. We need to eat our own dogfood though. If we want to push the responsibility for buffer allocation in the buffer sharing case to the application, we need to modify the cam application to do so when using the KMS backend. -- Regards, Laurent Pinchart
Re: [PATCH] drm/bridge: cdns-mhdp8546: Remove a redundant check on existence of bridge->encoder
Hi Sui,
Thank you for the patch.
On Sat, May 11, 2024 at 10:30:27PM +0800, Sui Jingfeng wrote:
> In the cdns_mhdp_connector_init() function, the check on the existence
> of bridge->encoder is not necessary, as it has already been done in the
> drm_bridge_attach() function. And the check on the drm bridge core
> happens before check in the implementation. Hence, it is guaranteed that
> the .encoder member of the struct drm_bridge is not NULL when
> adv7511_bridge_attach() function gets called.
>
> Remove the redundant checking codes "if (!bridge->encoder) { ... }".
>
> Signed-off-by: Sui Jingfeng
Reviewed-by: Laurent Pinchart
> ---
> drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c | 5 -
> 1 file changed, 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> index e226acc5c15e..16b58a7dcc54 100644
> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> @@ -1697,11 +1697,6 @@ static int cdns_mhdp_connector_init(struct
> cdns_mhdp_device *mhdp)
> struct drm_bridge *bridge = &mhdp->bridge;
> int ret;
>
> - if (!bridge->encoder) {
> - dev_err(mhdp->dev, "Parent encoder object not found");
> - return -ENODEV;
> - }
> -
> conn->polled = DRM_CONNECTOR_POLL_HPD;
>
> ret = drm_connector_init(bridge->dev, conn, &cdns_mhdp_conn_funcs,
--
Regards,
Laurent Pinchart
Re: [PATCH] drm/bridge: megachips-stdpxxxx-ge-b850v3-fw: Remove a redundant check on existence of bridge->encoder
Hi Sui,
Thank you for the patch.
On Sat, May 11, 2024 at 10:38:20PM +0800, Sui Jingfeng wrote:
> In the ge_b850v3_lvds_create_connector function, the check on the existence
> of bridge->encoder is not necessary, as it has already been done in the
> drm_bridge_attach() function. And the check on the drm bridge core
> happens before check in the implementation. Hence, it is guaranteed that
> the .encoder member of the struct drm_bridge is not NULL when
> ge_b850v3_lvds_attach() function gets called.
>
> Remove the redundant checking codes "if (!bridge->encoder) { ... }".
>
> Signed-off-by: Sui Jingfeng
Reviewed-by: Laurent Pinchart
> ---
> drivers/gpu/drm/bridge/megachips-stdp-ge-b850v3-fw.c | 5 -
> 1 file changed, 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/megachips-stdp-ge-b850v3-fw.c
> b/drivers/gpu/drm/bridge/megachips-stdp-ge-b850v3-fw.c
> index 4480523244e4..37f1acf5c0f8 100644
> --- a/drivers/gpu/drm/bridge/megachips-stdp-ge-b850v3-fw.c
> +++ b/drivers/gpu/drm/bridge/megachips-stdp-ge-b850v3-fw.c
> @@ -165,11 +165,6 @@ static int ge_b850v3_lvds_create_connector(struct
> drm_bridge *bridge)
> struct drm_connector *connector = &ge_b850v3_lvds_ptr->connector;
> int ret;
>
> - if (!bridge->encoder) {
> - DRM_ERROR("Parent encoder object not found");
> - return -ENODEV;
> - }
> -
> connector->polled = DRM_CONNECTOR_POLL_HPD;
>
> drm_connector_helper_add(connector,
--
Regards,
Laurent Pinchart
Re: [PATCH] drm/bridge: synopsys: dw-mipi-dsi: Remove a redundant check on existence of bridge->encoder
Hi Sui,
Thank you for the patch.
On Sat, May 11, 2024 at 10:47:13PM +0800, Sui Jingfeng wrote:
> In the dw_mipi_dsi_bridge_attach() function, the check on the existence
> of bridge->encoder is not necessary, as it has already been done in the
> drm_bridge_attach() function. And the check on the drm bridge core
> happens before check in the implementation. Hence, it is guaranteed that
> the .encoder member of the struct drm_bridge is not NULL when
> dw_mipi_dsi_bridge_attach() function gets called.
>
> Remove the redundant checking codes "if (!bridge->encoder) { ... }".
>
> Signed-off-by: Sui Jingfeng
Reviewed-by: Laurent Pinchart
> ---
> drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 5 -
> 1 file changed, 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> index 824fb3c65742..c4e9d96933dc 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> @@ -1071,11 +1071,6 @@ static int dw_mipi_dsi_bridge_attach(struct drm_bridge
> *bridge,
> {
> struct dw_mipi_dsi *dsi = bridge_to_dsi(bridge);
>
> - if (!bridge->encoder) {
> - DRM_ERROR("Parent encoder object not found\n");
> - return -ENODEV;
> - }
> -
> /* Set the encoder type as caller does not know it */
> bridge->encoder->encoder_type = DRM_MODE_ENCODER_DSI;
>
--
Regards,
Laurent Pinchart
Re: [PATCH] drm/bridge: lt9611uxc: Remove a redundant check on existence of bridge->encoder
Hi Sui,
Thank you for the patch.
On Sat, May 11, 2024 at 10:55:49PM +0800, Sui Jingfeng wrote:
> In the lt9611uxc_connector_init() function, the check on the existence
> of bridge->encoder is not necessary, as it has already been done in the
> drm_bridge_attach() function. And the check on the drm bridge core
> happens before check in the implementation. Hence, it is guaranteed that
> the .encoder member of the struct drm_bridge is not NULL when
> lt9611uxc_connector_init() function gets called.
>
> Remove the redundant checking codes "if (!bridge->encoder) { ... }".
>
> Signed-off-by: Sui Jingfeng
Reviewed-by: Laurent Pinchart
> ---
> drivers/gpu/drm/bridge/lontium-lt9611uxc.c | 5 -
> 1 file changed, 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/lontium-lt9611uxc.c
> b/drivers/gpu/drm/bridge/lontium-lt9611uxc.c
> index f4f593ad8f79..f1fccfe6c534 100644
> --- a/drivers/gpu/drm/bridge/lontium-lt9611uxc.c
> +++ b/drivers/gpu/drm/bridge/lontium-lt9611uxc.c
> @@ -339,11 +339,6 @@ static int lt9611uxc_connector_init(struct drm_bridge
> *bridge, struct lt9611uxc
> {
> int ret;
>
> - if (!bridge->encoder) {
> - DRM_ERROR("Parent encoder object not found");
> - return -ENODEV;
> - }
> -
> lt9611uxc->connector.polled = DRM_CONNECTOR_POLL_HPD;
>
> drm_connector_helper_add(<9611uxc->connector,
--
Regards,
Laurent Pinchart
Re: [PATCH] drm/bridge: adv7511: Remove a redundant check on existence of bridge->encoder
Hi Sui,
Thank you for the patch.
On Sat, May 11, 2024 at 10:23:08PM +0800, Sui Jingfeng wrote:
> In the adv7511_connector_init() function, the check on the existence
> of bridge->encoder is not necessary, as it has already been done in the
> drm_bridge_attach() function. And the check on the drm bridge core
> happens before check in the implementation. Hence, it is guaranteed that
> the .encoder member of the struct drm_bridge is not NULL when
> adv7511_bridge_attach() function is called.
>
> Remove the redundant checking codes "if (!bridge->encoder) { ... }".
>
> Signed-off-by: Sui Jingfeng
Reviewed-by: Laurent Pinchart
> ---
> drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 5 -
> 1 file changed, 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> index b5518ff97165..1a0e614e0fd3 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> @@ -871,11 +871,6 @@ static int adv7511_connector_init(struct adv7511 *adv)
> struct drm_bridge *bridge = &adv->bridge;
> int ret;
>
> - if (!bridge->encoder) {
> - DRM_ERROR("Parent encoder object not found");
> - return -ENODEV;
> - }
> -
> if (adv->i2c_main->irq)
> adv->connector.polled = DRM_CONNECTOR_POLL_HPD;
> else
--
Regards,
Laurent Pinchart
Re: [PATCH] drm/bridge: imx: Remove redundant checks on existence of bridge->encoder
Hi Sui,
Thank you for the patch.
On Sat, May 11, 2024 at 11:08:16PM +0800, Sui Jingfeng wrote:
> The check on the existence of bridge->encoder on the implementation layer
> of drm bridge driver is not necessary, as it has already been done in the
> drm_bridge_attach() function. It is guaranteed that the .encoder member
> of the drm_bridge instance is not NULL when various imx_xxx_bridge_attach()
> function gets called.
>
> Remove the redundant checking codes "if (!bridge->encoder) { ... }".
>
> Signed-off-by: Sui Jingfeng
Reviewed-by: Laurent Pinchart
> ---
> drivers/gpu/drm/bridge/imx/imx-ldb-helper.c | 5 -
> drivers/gpu/drm/bridge/imx/imx8qxp-pixel-combiner.c | 5 -
> drivers/gpu/drm/bridge/imx/imx8qxp-pixel-link.c | 5 -
> drivers/gpu/drm/bridge/imx/imx8qxp-pxl2dpi.c| 5 -
> 4 files changed, 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/imx/imx-ldb-helper.c
> b/drivers/gpu/drm/bridge/imx/imx-ldb-helper.c
> index 6967325cd8ee..9b5bebbe357d 100644
> --- a/drivers/gpu/drm/bridge/imx/imx-ldb-helper.c
> +++ b/drivers/gpu/drm/bridge/imx/imx-ldb-helper.c
> @@ -116,11 +116,6 @@ int ldb_bridge_attach_helper(struct drm_bridge *bridge,
> return -EINVAL;
> }
>
> - if (!bridge->encoder) {
> - DRM_DEV_ERROR(ldb->dev, "missing encoder\n");
> - return -ENODEV;
> - }
> -
> return drm_bridge_attach(bridge->encoder,
> ldb_ch->next_bridge, bridge,
> DRM_BRIDGE_ATTACH_NO_CONNECTOR);
> diff --git a/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-combiner.c
> b/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-combiner.c
> index d0868a6ac6c9..e6dbbdc87ce2 100644
> --- a/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-combiner.c
> +++ b/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-combiner.c
> @@ -119,11 +119,6 @@ static int imx8qxp_pc_bridge_attach(struct drm_bridge
> *bridge,
> return -EINVAL;
> }
>
> - if (!bridge->encoder) {
> - DRM_DEV_ERROR(pc->dev, "missing encoder\n");
> - return -ENODEV;
> - }
> -
> return drm_bridge_attach(bridge->encoder,
>ch->next_bridge, bridge,
>DRM_BRIDGE_ATTACH_NO_CONNECTOR);
> diff --git a/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-link.c
> b/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-link.c
> index ed8b7a4e0e11..1d11cc1df43c 100644
> --- a/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-link.c
> +++ b/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-link.c
> @@ -138,11 +138,6 @@ static int imx8qxp_pixel_link_bridge_attach(struct
> drm_bridge *bridge,
> return -EINVAL;
> }
>
> - if (!bridge->encoder) {
> - DRM_DEV_ERROR(pl->dev, "missing encoder\n");
> - return -ENODEV;
> - }
> -
> return drm_bridge_attach(bridge->encoder,
>pl->next_bridge, bridge,
>DRM_BRIDGE_ATTACH_NO_CONNECTOR);
> diff --git a/drivers/gpu/drm/bridge/imx/imx8qxp-pxl2dpi.c
> b/drivers/gpu/drm/bridge/imx/imx8qxp-pxl2dpi.c
> index 4a886cb808ca..fb7cf4369bb8 100644
> --- a/drivers/gpu/drm/bridge/imx/imx8qxp-pxl2dpi.c
> +++ b/drivers/gpu/drm/bridge/imx/imx8qxp-pxl2dpi.c
> @@ -58,11 +58,6 @@ static int imx8qxp_pxl2dpi_bridge_attach(struct drm_bridge
> *bridge,
> return -EINVAL;
> }
>
> - if (!bridge->encoder) {
> - DRM_DEV_ERROR(p2d->dev, "missing encoder\n");
> - return -ENODEV;
> - }
> -
> return drm_bridge_attach(bridge->encoder,
>p2d->next_bridge, bridge,
>DRM_BRIDGE_ATTACH_NO_CONNECTOR);
--
Regards,
Laurent Pinchart
Re: [PATCH] drm/bridge: analogix: Remove redundant checks on existence of bridge->encoder
Hi Sui,
Thank you for the patch.
On Sat, May 11, 2024 at 11:22:22PM +0800, Sui Jingfeng wrote:
> The check on the existence of bridge->encoder on the implementation layer
> of drm bridge driver is not necessary, as it has already been done in the
> drm_bridge_attach() function. It is guaranteed that the .encoder member
> of the drm_bridge instance is not NULL when various an_bridge_attach()
> function gets called. And .atomic_enable() of struct drm_bridge_funcs
> shouldn't be able to called before the various is acctached.
>
> Remove the redundant checking codes "if (!bridge->encoder) { ... }".
>
> Signed-off-by: Sui Jingfeng
Reviewed-by: Laurent Pinchart
> ---
> drivers/gpu/drm/bridge/analogix/analogix-anx6345.c | 5 -
> drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c | 5 -
> drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 5 -
> drivers/gpu/drm/bridge/analogix/anx7625.c | 10 --
> 4 files changed, 25 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix-anx6345.c
> b/drivers/gpu/drm/bridge/analogix/analogix-anx6345.c
> index c9e35731e6a1..cfe43d2ca3be 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix-anx6345.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix-anx6345.c
> @@ -528,11 +528,6 @@ static int anx6345_bridge_attach(struct drm_bridge
> *bridge,
> return -EINVAL;
> }
>
> - if (!bridge->encoder) {
> - DRM_ERROR("Parent encoder object not found");
> - return -ENODEV;
> - }
> -
> /* Register aux channel */
> anx6345->aux.name = "DP-AUX";
> anx6345->aux.dev = &anx6345->client->dev;
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c
> b/drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c
> index 5748a8581af4..58875dde496f 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c
> @@ -897,11 +897,6 @@ static int anx78xx_bridge_attach(struct drm_bridge
> *bridge,
> return -EINVAL;
> }
>
> - if (!bridge->encoder) {
> - DRM_ERROR("Parent encoder object not found");
> - return -ENODEV;
> - }
> -
> /* Register aux channel */
> anx78xx->aux.name = "DP-AUX";
> anx78xx->aux.dev = &anx78xx->client->dev;
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> index df9370e0ff23..7b841232321f 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> @@ -1228,11 +1228,6 @@ static int analogix_dp_bridge_attach(struct drm_bridge
> *bridge,
> return -EINVAL;
> }
>
> - if (!bridge->encoder) {
> - DRM_ERROR("Parent encoder object not found");
> - return -ENODEV;
> - }
> -
> if (!dp->plat_data->skip_connector) {
> connector = &dp->connector;
> connector->polled = DRM_CONNECTOR_POLL_HPD;
> diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c
> b/drivers/gpu/drm/bridge/analogix/anx7625.c
> index 59e9ad349969..3d09efa4199c 100644
> --- a/drivers/gpu/drm/bridge/analogix/anx7625.c
> +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
> @@ -2193,11 +2193,6 @@ static int anx7625_bridge_attach(struct drm_bridge
> *bridge,
> if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR))
> return -EINVAL;
>
> - if (!bridge->encoder) {
> - DRM_DEV_ERROR(dev, "Parent encoder object not found");
> - return -ENODEV;
> - }
> -
> ctx->aux.drm_dev = bridge->dev;
> err = drm_dp_aux_register(&ctx->aux);
> if (err) {
> @@ -2435,11 +2430,6 @@ static void anx7625_bridge_atomic_enable(struct
> drm_bridge *bridge,
>
> dev_dbg(dev, "drm atomic enable\n");
>
> - if (!bridge->encoder) {
> - dev_err(dev, "Parent encoder object not found");
> - return;
> - }
> -
> connector = drm_atomic_get_new_connector_for_encoder(state->base.state,
>bridge->encoder);
> if (!connector)
--
Regards,
Laurent Pinchart
Re: [PATCH] drm/bridge: it6505: Remove a redundant check on existence of bridge->encoder
Hi Sui,
Thank you for the patch.
On Sat, May 11, 2024 at 10:10:56PM +0800, Sui Jingfeng wrote:
> In it6505_bridge_attach(), the check on the existence of bridge->encoder
> has already been done in the implementation of drm_bridge_attach(). And
> it is done before the bridge->funcs->attach function hook is called. Hence,
> it is guaranteed that the .encoder member of the struct drm_bridge is not
> NULL when the panel_bridge_attach() is called.
>
> There is no need to check the existence of bridge->encoder another time,
> remove the redundant checking codes "if (!bridge->encoder) { ... }".
>
> Signed-off-by: Sui Jingfeng
Reviewed-by: Laurent Pinchart
> ---
> drivers/gpu/drm/bridge/ite-it6505.c | 5 -
> 1 file changed, 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ite-it6505.c
> b/drivers/gpu/drm/bridge/ite-it6505.c
> index 27334173e911..494030a75dba 100644
> --- a/drivers/gpu/drm/bridge/ite-it6505.c
> +++ b/drivers/gpu/drm/bridge/ite-it6505.c
> @@ -2881,11 +2881,6 @@ static int it6505_bridge_attach(struct drm_bridge
> *bridge,
> return -EINVAL;
> }
>
> - if (!bridge->encoder) {
> - dev_err(dev, "Parent encoder object not found");
> - return -ENODEV;
> - }
> -
> /* Register aux channel */
> it6505->aux.drm_dev = bridge->dev;
>
--
Regards,
Laurent Pinchart
Re: [PATCH] drm/bridge: panel: Remove a redundant check on existence of bridge->encoder
Hi Sui,
Thank you for the patch.
On Sat, May 11, 2024 at 10:03:16PM +0800, Sui Jingfeng wrote:
> In panel_bridge_attach(), the check on the existence of bridge->encoder
> has already been done in the implementation of drm_bridge_attach(). And
> it is done before the bridge->funcs->attach hook is called. Hence, it is
> guaranteed that the .encoder member of the struct drm_bridge is not NULL
> when the panel_bridge_attach() is called.
>
> There is no need to check the existence of bridge->encoder another time
> at the implementation layer, therefore remove the redundant checking codes
> "if (!bridge->encoder) { ... }".
>
> Signed-off-by: Sui Jingfeng
Reviewed-by: Laurent Pinchart
> ---
> drivers/gpu/drm/bridge/panel.c | 5 -
> 1 file changed, 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c
> index 7f41525f7a6e..762402dca6dd 100644
> --- a/drivers/gpu/drm/bridge/panel.c
> +++ b/drivers/gpu/drm/bridge/panel.c
> @@ -65,11 +65,6 @@ static int panel_bridge_attach(struct drm_bridge *bridge,
> if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)
> return 0;
>
> - if (!bridge->encoder) {
> - DRM_ERROR("Missing encoder\n");
> - return -ENODEV;
> - }
> -
> drm_connector_helper_add(connector,
>&panel_bridge_connector_helper_funcs);
>
--
Regards,
Laurent Pinchart
Re: [PATCH] drm/bridge: nxp-ptn3460: Remove a small useless code snippet
Hi Sui,
Thank you for the patch.
On Sat, May 11, 2024 at 09:42:50PM +0800, Sui Jingfeng wrote:
> In ptn3460_bridge_attach(), the check on the existence of bridge->encoder
> has already been done in the implementation of drm_bridge_attach(). The
> driver won't go further if bridge->encoder is NULL and the driver will quit
> even if drm_bridge_attach() fails for some reasons. Thereforei, there is
s/Thereforei/Therefore/
> no need to check another time at the later, remove the redundant checking
> codes "if (!bridge->encoder) { ... }".
>
> Signed-off-by: Sui Jingfeng
Reviewed-by: Laurent Pinchart
> ---
> drivers/gpu/drm/bridge/nxp-ptn3460.c | 5 -
> 1 file changed, 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/nxp-ptn3460.c
> b/drivers/gpu/drm/bridge/nxp-ptn3460.c
> index ed93fd4c3265..e77aab965fcf 100644
> --- a/drivers/gpu/drm/bridge/nxp-ptn3460.c
> +++ b/drivers/gpu/drm/bridge/nxp-ptn3460.c
> @@ -229,11 +229,6 @@ static int ptn3460_bridge_attach(struct drm_bridge
> *bridge,
> if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)
> return 0;
>
> - if (!bridge->encoder) {
> - DRM_ERROR("Parent encoder object not found");
> - return -ENODEV;
> - }
> -
> ptn_bridge->connector.polled = DRM_CONNECTOR_POLL_HPD;
> ret = drm_connector_init(bridge->dev, &ptn_bridge->connector,
> &ptn3460_connector_funcs, DRM_MODE_CONNECTOR_LVDS);
--
Regards,
Laurent Pinchart
Re: [PATCH] drm/bridge: tfp410: Remove a small useless code snippet
Hi Sui,
Thank you for the patch.
On Sat, May 11, 2024 at 09:24:23PM +0800, Sui Jingfeng wrote:
> In the tfp410_attach(), the check on the existence of bridge->encoder has
> already been done in the implementation of drm_bridge_attach() function.
> The driver won't go further if bridge->encoder is NULL and the driver will
> quit even if drm_bridge_attach() fails for some reasons.
>
> Therefore there is no need to check another time at the later, remove the
> redundant checking codes "if (!bridge->encoder) { ... }".
>
> Signed-off-by: Sui Jingfeng
Reviewed-by: Laurent Pinchart
> ---
> drivers/gpu/drm/bridge/ti-tfp410.c | 5 -
> 1 file changed, 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ti-tfp410.c
> b/drivers/gpu/drm/bridge/ti-tfp410.c
> index c7bef5c23927..b1b1e4d5a24a 100644
> --- a/drivers/gpu/drm/bridge/ti-tfp410.c
> +++ b/drivers/gpu/drm/bridge/ti-tfp410.c
> @@ -133,11 +133,6 @@ static int tfp410_attach(struct drm_bridge *bridge,
> if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)
> return 0;
>
> - if (!bridge->encoder) {
> - dev_err(dvi->dev, "Missing encoder\n");
> - return -ENODEV;
> - }
> -
> if (dvi->next_bridge->ops & DRM_BRIDGE_OP_DETECT)
> dvi->connector.polled = DRM_CONNECTOR_POLL_HPD;
> else
--
Regards,
Laurent Pinchart
Re: [PATCH] drm/bridge: Remove a small useless code snippet
Hi Sui,
Thank you for the patch.
On Sat, May 11, 2024 at 08:42:38PM +0800, Sui Jingfeng wrote:
> Because the check on the non-existence (encoder == NULL) has already been
> done in the implementation of drm_bridge_attach() function, and
> drm_bridge_attach() is called earlier. The driver won't get to check point
> even if drm_bridge_attach() fails for some reasons, as it will clear the
> bridge->encoder to NULL and return a negective error code.
s/negective/negative/
>
> Therefore, there is no need to check another again. Remove the redundant
> codes at the later.
>
> Signed-off-by: Sui Jingfeng
Reviewed-by: Laurent Pinchart
If you end up sending a second version of this patch, please include all
similar patches you have sent at the same time in a patch series,
instead of sending them separately.
> ---
> drivers/gpu/drm/bridge/simple-bridge.c | 5 -
> 1 file changed, 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/simple-bridge.c
> b/drivers/gpu/drm/bridge/simple-bridge.c
> index 28376d0ecd09..3caa918ac2e0 100644
> --- a/drivers/gpu/drm/bridge/simple-bridge.c
> +++ b/drivers/gpu/drm/bridge/simple-bridge.c
> @@ -116,11 +116,6 @@ static int simple_bridge_attach(struct drm_bridge
> *bridge,
> if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)
> return 0;
>
> - if (!bridge->encoder) {
> - DRM_ERROR("Missing encoder\n");
> - return -ENODEV;
> - }
> -
> drm_connector_helper_add(&sbridge->connector,
>&simple_bridge_con_helper_funcs);
> ret = drm_connector_init_with_ddc(bridge->dev, &sbridge->connector,
--
Regards,
Laurent Pinchart
Re: [PATCH 2/4] dt-bindings: display: ti: Add schema for AM625 OLDI Transmitter
ti,companion-oldi = <&oldi1>;
> +ti,oldi-io-ctrl = <&dss_oldi_io_ctrl>;
> +ports {
> +#address-cells = <1>;
> +#size-cells = <0>;
> +port@0 {
> +reg = <0>;
> +oldi0_in: endpoint {
> +remote-endpoint = <&dpi0_out0>;
> +};
> +};
> +};
> +};
> +oldi1: oldi@1 {
> +reg = <1>;
> +ti,secondary-oldi;
> +ports {
> +#address-cells = <1>;
> +#size-cells = <0>;
> +port@0 {
> +reg = <0>;
> +oldi1_in: endpoint {
> + remote-endpoint = <&dpi0_out1>;
> +};
> +};
> +};
> +};
> +};
> +
> +...
> diff --git a/MAINTAINERS b/MAINTAINERS
> index c675fc296b19..4426c4d41a7f 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7480,6 +7480,7 @@ M: Tomi Valkeinen
> L: dri-devel@lists.freedesktop.org
> S: Maintained
> T: git https://gitlab.freedesktop.org/drm/misc/kernel.git
> +F: Documentation/devicetree/bindings/display/ti/ti,am625-oldi.yaml
> F: Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
> F: Documentation/devicetree/bindings/display/ti/ti,j721e-dss.yaml
> F: Documentation/devicetree/bindings/display/ti/ti,k2g-dss.yaml
--
Regards,
Laurent Pinchart
Re: [PATCH 1/4] dt-bindings: display: ti,am65x-dss: Minor Cleanup
Hi Aradhya,
Thank you for the patch.
On Sun, May 12, 2024 at 01:00:52AM +0530, Aradhya Bhatia wrote:
> Reduce tab size from 8 spaces to 4 spaces to make the bindings
> consistent, and easy to expand.
>
> Signed-off-by: Aradhya Bhatia
Reviewed-by: Laurent Pinchart
> ---
> .../bindings/display/ti/ti,am65x-dss.yaml | 54 +--
> 1 file changed, 27 insertions(+), 27 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
> b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
> index 55e3e490d0e6..399d68986326 100644
> --- a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
> +++ b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
> @@ -142,32 +142,32 @@ examples:
> #include
>
> dss: dss@4a0 {
> -compatible = "ti,am65x-dss";
> -reg = <0x04a0 0x1000>, /* common */
> -<0x04a02000 0x1000>, /* vidl1 */
> -<0x04a06000 0x1000>, /* vid */
> -<0x04a07000 0x1000>, /* ovr1 */
> -<0x04a08000 0x1000>, /* ovr2 */
> -<0x04a0a000 0x1000>, /* vp1 */
> -<0x04a0b000 0x1000>, /* vp2 */
> -<0x04a01000 0x1000>; /* common1 */
> -reg-names = "common", "vidl1", "vid",
> -"ovr1", "ovr2", "vp1", "vp2", "common1";
> -ti,am65x-oldi-io-ctrl = <&dss_oldi_io_ctrl>;
> -power-domains = <&k3_pds 67 TI_SCI_PD_EXCLUSIVE>;
> -clocks =<&k3_clks 67 1>,
> -<&k3_clks 216 1>,
> -<&k3_clks 67 2>;
> -clock-names = "fck", "vp1", "vp2";
> -interrupts = ;
> -ports {
> -#address-cells = <1>;
> -#size-cells = <0>;
> -port@0 {
> -reg = <0>;
> -oldi_out0: endpoint {
> -remote-endpoint = <&lcd_in0>;
> -};
> -};
> +compatible = "ti,am65x-dss";
> +reg = <0x04a0 0x1000>, /* common */
> + <0x04a02000 0x1000>, /* vidl1 */
> + <0x04a06000 0x1000>, /* vid */
> + <0x04a07000 0x1000>, /* ovr1 */
> + <0x04a08000 0x1000>, /* ovr2 */
> + <0x04a0a000 0x1000>, /* vp1 */
> + <0x04a0b000 0x1000>, /* vp2 */
> + <0x04a01000 0x1000>; /* common1 */
> +reg-names = "common", "vidl1", "vid",
> +"ovr1", "ovr2", "vp1", "vp2", "common1";
> +ti,am65x-oldi-io-ctrl = <&dss_oldi_io_ctrl>;
> +power-domains = <&k3_pds 67 TI_SCI_PD_EXCLUSIVE>;
> +clocks =<&k3_clks 67 1>,
> +<&k3_clks 216 1>,
> +<&k3_clks 67 2>;
> +clock-names = "fck", "vp1", "vp2";
> +interrupts = ;
> +ports {
> +#address-cells = <1>;
> +#size-cells = <0>;
> +port@0 {
> +reg = <0>;
> +oldi_out0: endpoint {
> +remote-endpoint = <&lcd_in0>;
> +};
> };
> +};
> };
--
Regards,
Laurent Pinchart
Re: [PATCH v7 8/8] gpu: ipu-v3: Use generic macro for rounding to nearest multiple
On Fri, May 10, 2024 at 06:03:52PM +0300, Andy Shevchenko wrote: > On Fri, May 10, 2024 at 12:10:10AM +0530, Devarsh Thakkar wrote: > > Use generic macro round_closest_up for rounding to nearest multiple instead > > round_closest_up() > > We refer to the functions as func(). > > > of using local function. > > ... > > > @@ -565,7 +563,7 @@ static void find_best_seam(struct ipu_image_convert_ctx > > *ctx, > > * The closest input sample position that we could actually > > * start the input tile at, 19.13 fixed point. > > */ > > - in_pos_aligned = round_closest(in_pos, 8192U * in_align); > > + in_pos_aligned = round_closest_up(in_pos, 8192U * in_align); > > /* Convert 19.13 fixed point to integer */ > > in_pos_rounded = in_pos_aligned / 8192U; > > Oh, these seems to be better to use either ALIGN*(), or PFN_*() / PAGE_*() > families of macros. What the semantic of 8192 is? The comment mentions 19.13 fixed point, so I assume that's the fractional part of the integer. It doesn't seem related to pages. -- Regards, Laurent Pinchart
