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()" 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(>base,
> > > > > > > _drm_plane_helper_funcs);
> > > > > > > + if (type == DRM_PLANE_TYPE_PRIMARY)
> > > > > > > + drm_plane_helper_add(>base,
> > > > > > > +
> > > > > > > _drm_primary_plane_helper_funcs);
> > > > > > > + else
> > > > > > > + drm_plane_helper_add(>base,
> > > > > > > + _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
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(>base,
> > > > > _drm_plane_helper_funcs);
> > > > > + if (type == DRM_PLANE_TYPE_PRIMARY)
> > > > > + drm_plane_helper_add(>base,
> > > > > +
> > > > > _drm_primary_plane_helper_funcs);
> > > > > + else
> > > > > + drm_plane_helper_add(>base,
> > > > > + _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 >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(>base, _drm_plane_helper_funcs);
> > > + if (type == DRM_PLANE_TYPE_PRIMARY)
> > > + drm_plane_helper_add(>base,
> > > + _drm_primary_plane_helper_funcs);
> > > + else
> > > + drm_plane_helper_add(>base,
> > > + _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 >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,
> - _du_plane_helper_funcs);
> + if (type == DRM_PLANE_TYPE_PRIMARY)
> + drm_plane_helper_add(>plane,
> +
> _du_primary_plane_helper_funcs);
> + else
> + drm_plane_helper_add(>plane,
> + _du_plane_helper_funcs);
Same comment as for the shmobile-drm patch. Let's discuss it there.
>
> drm_plane_create_alpha_property(>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(>base, _drm_plane_helper_funcs);
> + if (type == DRM_PLANE_TYPE_PRIMARY)
> + drm_plane_helper_add(>base,
> + _drm_primary_plane_helper_funcs);
> + else
> + drm_plane_helper_add(>base,
> + _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 >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(>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),
>_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),
>_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), );
--
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 = >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(>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(>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),
>_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(_entity->list, >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(>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),
>_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(_entity->list, >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(>connector,
> > > > +_hdmi_connector_helper_funcs);
> > > > +
> > > > + drmm_connector_init(drm, >connector,
> > > > + _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(>connector,
>_hdmi_connector_helper_funcs);
>
> drmm_connector_init(drm, >connector,
> _hdmi_connector_funcs,
> DRM_MODE_CONNECTOR_HDMIA,
> hdmi->ddc);
>
> drm_connector_attach_encoder(>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(>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(>connector, > > +_hdmi_connector_helper_funcs); > > + > > + drmm_connector_init(drm, >connector, > > + _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(>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.
> >
> > That's the recommended w
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 > > > insufficient > > > for the "just work&qu
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 = >bridge;
> int ret;
>
> - if (!bridge->encoder) {
> - dev_err(mhdp->dev, "Parent encoder object not found");
> - return -ENODEV;
> - }
> -
> conn->polled = DRM_CONNECTOR_POLL_HPD;
>
> ret = drm_connector_init(bridge->dev, conn, _mhdp_conn_funcs,
--
Regards,
Laurent Pinchart
Re: [PATCH] drm/bridge: megachips-stdpxxxx-ge-b850v3-fw: Remove a redundant check on existence of bridge->encoder
Hi Sui,
Thank you for the patch.
On Sat, May 11, 2024 at 10:38:20PM +0800, Sui Jingfeng wrote:
> In the ge_b850v3_lvds_create_connector function, the check on the existence
> of bridge->encoder is not necessary, as it has already been done in the
> drm_bridge_attach() function. And the check on the drm bridge core
> happens before check in the implementation. Hence, it is guaranteed that
> the .encoder member of the struct drm_bridge is not NULL when
> ge_b850v3_lvds_attach() function gets called.
>
> Remove the redundant checking codes "if (!bridge->encoder) { ... }".
>
> Signed-off-by: Sui Jingfeng
Reviewed-by: Laurent Pinchart
> ---
> drivers/gpu/drm/bridge/megachips-stdp-ge-b850v3-fw.c | 5 -
> 1 file changed, 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/megachips-stdp-ge-b850v3-fw.c
> b/drivers/gpu/drm/bridge/megachips-stdp-ge-b850v3-fw.c
> index 4480523244e4..37f1acf5c0f8 100644
> --- a/drivers/gpu/drm/bridge/megachips-stdp-ge-b850v3-fw.c
> +++ b/drivers/gpu/drm/bridge/megachips-stdp-ge-b850v3-fw.c
> @@ -165,11 +165,6 @@ static int ge_b850v3_lvds_create_connector(struct
> drm_bridge *bridge)
> struct drm_connector *connector = _b850v3_lvds_ptr->connector;
> int ret;
>
> - if (!bridge->encoder) {
> - DRM_ERROR("Parent encoder object not found");
> - return -ENODEV;
> - }
> -
> connector->polled = DRM_CONNECTOR_POLL_HPD;
>
> drm_connector_helper_add(connector,
--
Regards,
Laurent Pinchart
Re: [PATCH] drm/bridge: synopsys: dw-mipi-dsi: Remove a redundant check on existence of bridge->encoder
Hi Sui,
Thank you for the patch.
On Sat, May 11, 2024 at 10:47:13PM +0800, Sui Jingfeng wrote:
> In the dw_mipi_dsi_bridge_attach() function, the check on the existence
> of bridge->encoder is not necessary, as it has already been done in the
> drm_bridge_attach() function. And the check on the drm bridge core
> happens before check in the implementation. Hence, it is guaranteed that
> the .encoder member of the struct drm_bridge is not NULL when
> dw_mipi_dsi_bridge_attach() function gets called.
>
> Remove the redundant checking codes "if (!bridge->encoder) { ... }".
>
> Signed-off-by: Sui Jingfeng
Reviewed-by: Laurent Pinchart
> ---
> drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 5 -
> 1 file changed, 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> index 824fb3c65742..c4e9d96933dc 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> @@ -1071,11 +1071,6 @@ static int dw_mipi_dsi_bridge_attach(struct drm_bridge
> *bridge,
> {
> struct dw_mipi_dsi *dsi = bridge_to_dsi(bridge);
>
> - if (!bridge->encoder) {
> - DRM_ERROR("Parent encoder object not found\n");
> - return -ENODEV;
> - }
> -
> /* Set the encoder type as caller does not know it */
> bridge->encoder->encoder_type = DRM_MODE_ENCODER_DSI;
>
--
Regards,
Laurent Pinchart
Re: [PATCH] drm/bridge: lt9611uxc: Remove a redundant check on existence of bridge->encoder
Hi Sui,
Thank you for the patch.
On Sat, May 11, 2024 at 10:55:49PM +0800, Sui Jingfeng wrote:
> In the lt9611uxc_connector_init() function, the check on the existence
> of bridge->encoder is not necessary, as it has already been done in the
> drm_bridge_attach() function. And the check on the drm bridge core
> happens before check in the implementation. Hence, it is guaranteed that
> the .encoder member of the struct drm_bridge is not NULL when
> lt9611uxc_connector_init() function gets called.
>
> Remove the redundant checking codes "if (!bridge->encoder) { ... }".
>
> Signed-off-by: Sui Jingfeng
Reviewed-by: Laurent Pinchart
> ---
> drivers/gpu/drm/bridge/lontium-lt9611uxc.c | 5 -
> 1 file changed, 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/lontium-lt9611uxc.c
> b/drivers/gpu/drm/bridge/lontium-lt9611uxc.c
> index f4f593ad8f79..f1fccfe6c534 100644
> --- a/drivers/gpu/drm/bridge/lontium-lt9611uxc.c
> +++ b/drivers/gpu/drm/bridge/lontium-lt9611uxc.c
> @@ -339,11 +339,6 @@ static int lt9611uxc_connector_init(struct drm_bridge
> *bridge, struct lt9611uxc
> {
> int ret;
>
> - if (!bridge->encoder) {
> - DRM_ERROR("Parent encoder object not found");
> - return -ENODEV;
> - }
> -
> lt9611uxc->connector.polled = DRM_CONNECTOR_POLL_HPD;
>
> drm_connector_helper_add(>connector,
--
Regards,
Laurent Pinchart
Re: [PATCH] drm/bridge: adv7511: Remove a redundant check on existence of bridge->encoder
Hi Sui,
Thank you for the patch.
On Sat, May 11, 2024 at 10:23:08PM +0800, Sui Jingfeng wrote:
> In the adv7511_connector_init() function, the check on the existence
> of bridge->encoder is not necessary, as it has already been done in the
> drm_bridge_attach() function. And the check on the drm bridge core
> happens before check in the implementation. Hence, it is guaranteed that
> the .encoder member of the struct drm_bridge is not NULL when
> adv7511_bridge_attach() function is called.
>
> Remove the redundant checking codes "if (!bridge->encoder) { ... }".
>
> Signed-off-by: Sui Jingfeng
Reviewed-by: Laurent Pinchart
> ---
> drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 5 -
> 1 file changed, 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> index b5518ff97165..1a0e614e0fd3 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> @@ -871,11 +871,6 @@ static int adv7511_connector_init(struct adv7511 *adv)
> struct drm_bridge *bridge = >bridge;
> int ret;
>
> - if (!bridge->encoder) {
> - DRM_ERROR("Parent encoder object not found");
> - return -ENODEV;
> - }
> -
> if (adv->i2c_main->irq)
> adv->connector.polled = DRM_CONNECTOR_POLL_HPD;
> else
--
Regards,
Laurent Pinchart
Re: [PATCH] drm/bridge: imx: Remove redundant checks on existence of bridge->encoder
Hi Sui,
Thank you for the patch.
On Sat, May 11, 2024 at 11:08:16PM +0800, Sui Jingfeng wrote:
> The check on the existence of bridge->encoder on the implementation layer
> of drm bridge driver is not necessary, as it has already been done in the
> drm_bridge_attach() function. It is guaranteed that the .encoder member
> of the drm_bridge instance is not NULL when various imx_xxx_bridge_attach()
> function gets called.
>
> Remove the redundant checking codes "if (!bridge->encoder) { ... }".
>
> Signed-off-by: Sui Jingfeng
Reviewed-by: Laurent Pinchart
> ---
> drivers/gpu/drm/bridge/imx/imx-ldb-helper.c | 5 -
> drivers/gpu/drm/bridge/imx/imx8qxp-pixel-combiner.c | 5 -
> drivers/gpu/drm/bridge/imx/imx8qxp-pixel-link.c | 5 -
> drivers/gpu/drm/bridge/imx/imx8qxp-pxl2dpi.c| 5 -
> 4 files changed, 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/imx/imx-ldb-helper.c
> b/drivers/gpu/drm/bridge/imx/imx-ldb-helper.c
> index 6967325cd8ee..9b5bebbe357d 100644
> --- a/drivers/gpu/drm/bridge/imx/imx-ldb-helper.c
> +++ b/drivers/gpu/drm/bridge/imx/imx-ldb-helper.c
> @@ -116,11 +116,6 @@ int ldb_bridge_attach_helper(struct drm_bridge *bridge,
> return -EINVAL;
> }
>
> - if (!bridge->encoder) {
> - DRM_DEV_ERROR(ldb->dev, "missing encoder\n");
> - return -ENODEV;
> - }
> -
> return drm_bridge_attach(bridge->encoder,
> ldb_ch->next_bridge, bridge,
> DRM_BRIDGE_ATTACH_NO_CONNECTOR);
> diff --git a/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-combiner.c
> b/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-combiner.c
> index d0868a6ac6c9..e6dbbdc87ce2 100644
> --- a/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-combiner.c
> +++ b/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-combiner.c
> @@ -119,11 +119,6 @@ static int imx8qxp_pc_bridge_attach(struct drm_bridge
> *bridge,
> return -EINVAL;
> }
>
> - if (!bridge->encoder) {
> - DRM_DEV_ERROR(pc->dev, "missing encoder\n");
> - return -ENODEV;
> - }
> -
> return drm_bridge_attach(bridge->encoder,
>ch->next_bridge, bridge,
>DRM_BRIDGE_ATTACH_NO_CONNECTOR);
> diff --git a/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-link.c
> b/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-link.c
> index ed8b7a4e0e11..1d11cc1df43c 100644
> --- a/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-link.c
> +++ b/drivers/gpu/drm/bridge/imx/imx8qxp-pixel-link.c
> @@ -138,11 +138,6 @@ static int imx8qxp_pixel_link_bridge_attach(struct
> drm_bridge *bridge,
> return -EINVAL;
> }
>
> - if (!bridge->encoder) {
> - DRM_DEV_ERROR(pl->dev, "missing encoder\n");
> - return -ENODEV;
> - }
> -
> return drm_bridge_attach(bridge->encoder,
>pl->next_bridge, bridge,
>DRM_BRIDGE_ATTACH_NO_CONNECTOR);
> diff --git a/drivers/gpu/drm/bridge/imx/imx8qxp-pxl2dpi.c
> b/drivers/gpu/drm/bridge/imx/imx8qxp-pxl2dpi.c
> index 4a886cb808ca..fb7cf4369bb8 100644
> --- a/drivers/gpu/drm/bridge/imx/imx8qxp-pxl2dpi.c
> +++ b/drivers/gpu/drm/bridge/imx/imx8qxp-pxl2dpi.c
> @@ -58,11 +58,6 @@ static int imx8qxp_pxl2dpi_bridge_attach(struct drm_bridge
> *bridge,
> return -EINVAL;
> }
>
> - if (!bridge->encoder) {
> - DRM_DEV_ERROR(p2d->dev, "missing encoder\n");
> - return -ENODEV;
> - }
> -
> return drm_bridge_attach(bridge->encoder,
>p2d->next_bridge, bridge,
>DRM_BRIDGE_ATTACH_NO_CONNECTOR);
--
Regards,
Laurent Pinchart
Re: [PATCH] drm/bridge: analogix: Remove redundant checks on existence of bridge->encoder
Hi Sui,
Thank you for the patch.
On Sat, May 11, 2024 at 11:22:22PM +0800, Sui Jingfeng wrote:
> The check on the existence of bridge->encoder on the implementation layer
> of drm bridge driver is not necessary, as it has already been done in the
> drm_bridge_attach() function. It is guaranteed that the .encoder member
> of the drm_bridge instance is not NULL when various an_bridge_attach()
> function gets called. And .atomic_enable() of struct drm_bridge_funcs
> shouldn't be able to called before the various is acctached.
>
> Remove the redundant checking codes "if (!bridge->encoder) { ... }".
>
> Signed-off-by: Sui Jingfeng
Reviewed-by: Laurent Pinchart
> ---
> drivers/gpu/drm/bridge/analogix/analogix-anx6345.c | 5 -
> drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c | 5 -
> drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 5 -
> drivers/gpu/drm/bridge/analogix/anx7625.c | 10 --
> 4 files changed, 25 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix-anx6345.c
> b/drivers/gpu/drm/bridge/analogix/analogix-anx6345.c
> index c9e35731e6a1..cfe43d2ca3be 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix-anx6345.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix-anx6345.c
> @@ -528,11 +528,6 @@ static int anx6345_bridge_attach(struct drm_bridge
> *bridge,
> return -EINVAL;
> }
>
> - if (!bridge->encoder) {
> - DRM_ERROR("Parent encoder object not found");
> - return -ENODEV;
> - }
> -
> /* Register aux channel */
> anx6345->aux.name = "DP-AUX";
> anx6345->aux.dev = >client->dev;
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c
> b/drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c
> index 5748a8581af4..58875dde496f 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c
> @@ -897,11 +897,6 @@ static int anx78xx_bridge_attach(struct drm_bridge
> *bridge,
> return -EINVAL;
> }
>
> - if (!bridge->encoder) {
> - DRM_ERROR("Parent encoder object not found");
> - return -ENODEV;
> - }
> -
> /* Register aux channel */
> anx78xx->aux.name = "DP-AUX";
> anx78xx->aux.dev = >client->dev;
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> index df9370e0ff23..7b841232321f 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> @@ -1228,11 +1228,6 @@ static int analogix_dp_bridge_attach(struct drm_bridge
> *bridge,
> return -EINVAL;
> }
>
> - if (!bridge->encoder) {
> - DRM_ERROR("Parent encoder object not found");
> - return -ENODEV;
> - }
> -
> if (!dp->plat_data->skip_connector) {
> connector = >connector;
> connector->polled = DRM_CONNECTOR_POLL_HPD;
> diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c
> b/drivers/gpu/drm/bridge/analogix/anx7625.c
> index 59e9ad349969..3d09efa4199c 100644
> --- a/drivers/gpu/drm/bridge/analogix/anx7625.c
> +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
> @@ -2193,11 +2193,6 @@ static int anx7625_bridge_attach(struct drm_bridge
> *bridge,
> if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR))
> return -EINVAL;
>
> - if (!bridge->encoder) {
> - DRM_DEV_ERROR(dev, "Parent encoder object not found");
> - return -ENODEV;
> - }
> -
> ctx->aux.drm_dev = bridge->dev;
> err = drm_dp_aux_register(>aux);
> if (err) {
> @@ -2435,11 +2430,6 @@ static void anx7625_bridge_atomic_enable(struct
> drm_bridge *bridge,
>
> dev_dbg(dev, "drm atomic enable\n");
>
> - if (!bridge->encoder) {
> - dev_err(dev, "Parent encoder object not found");
> - return;
> - }
> -
> connector = drm_atomic_get_new_connector_for_encoder(state->base.state,
>bridge->encoder);
> if (!connector)
--
Regards,
Laurent Pinchart
Re: [PATCH] drm/bridge: it6505: Remove a redundant check on existence of bridge->encoder
Hi Sui,
Thank you for the patch.
On Sat, May 11, 2024 at 10:10:56PM +0800, Sui Jingfeng wrote:
> In it6505_bridge_attach(), the check on the existence of bridge->encoder
> has already been done in the implementation of drm_bridge_attach(). And
> it is done before the bridge->funcs->attach function hook is called. Hence,
> it is guaranteed that the .encoder member of the struct drm_bridge is not
> NULL when the panel_bridge_attach() is called.
>
> There is no need to check the existence of bridge->encoder another time,
> remove the redundant checking codes "if (!bridge->encoder) { ... }".
>
> Signed-off-by: Sui Jingfeng
Reviewed-by: Laurent Pinchart
> ---
> drivers/gpu/drm/bridge/ite-it6505.c | 5 -
> 1 file changed, 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ite-it6505.c
> b/drivers/gpu/drm/bridge/ite-it6505.c
> index 27334173e911..494030a75dba 100644
> --- a/drivers/gpu/drm/bridge/ite-it6505.c
> +++ b/drivers/gpu/drm/bridge/ite-it6505.c
> @@ -2881,11 +2881,6 @@ static int it6505_bridge_attach(struct drm_bridge
> *bridge,
> return -EINVAL;
> }
>
> - if (!bridge->encoder) {
> - dev_err(dev, "Parent encoder object not found");
> - return -ENODEV;
> - }
> -
> /* Register aux channel */
> it6505->aux.drm_dev = bridge->dev;
>
--
Regards,
Laurent Pinchart
Re: [PATCH] drm/bridge: panel: Remove a redundant check on existence of bridge->encoder
Hi Sui,
Thank you for the patch.
On Sat, May 11, 2024 at 10:03:16PM +0800, Sui Jingfeng wrote:
> In panel_bridge_attach(), the check on the existence of bridge->encoder
> has already been done in the implementation of drm_bridge_attach(). And
> it is done before the bridge->funcs->attach hook is called. Hence, it is
> guaranteed that the .encoder member of the struct drm_bridge is not NULL
> when the panel_bridge_attach() is called.
>
> There is no need to check the existence of bridge->encoder another time
> at the implementation layer, therefore remove the redundant checking codes
> "if (!bridge->encoder) { ... }".
>
> Signed-off-by: Sui Jingfeng
Reviewed-by: Laurent Pinchart
> ---
> drivers/gpu/drm/bridge/panel.c | 5 -
> 1 file changed, 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c
> index 7f41525f7a6e..762402dca6dd 100644
> --- a/drivers/gpu/drm/bridge/panel.c
> +++ b/drivers/gpu/drm/bridge/panel.c
> @@ -65,11 +65,6 @@ static int panel_bridge_attach(struct drm_bridge *bridge,
> if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)
> return 0;
>
> - if (!bridge->encoder) {
> - DRM_ERROR("Missing encoder\n");
> - return -ENODEV;
> - }
> -
> drm_connector_helper_add(connector,
>_bridge_connector_helper_funcs);
>
--
Regards,
Laurent Pinchart
Re: [PATCH] drm/bridge: nxp-ptn3460: Remove a small useless code snippet
Hi Sui,
Thank you for the patch.
On Sat, May 11, 2024 at 09:42:50PM +0800, Sui Jingfeng wrote:
> In ptn3460_bridge_attach(), the check on the existence of bridge->encoder
> has already been done in the implementation of drm_bridge_attach(). The
> driver won't go further if bridge->encoder is NULL and the driver will quit
> even if drm_bridge_attach() fails for some reasons. Thereforei, there is
s/Thereforei/Therefore/
> no need to check another time at the later, remove the redundant checking
> codes "if (!bridge->encoder) { ... }".
>
> Signed-off-by: Sui Jingfeng
Reviewed-by: Laurent Pinchart
> ---
> drivers/gpu/drm/bridge/nxp-ptn3460.c | 5 -
> 1 file changed, 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/nxp-ptn3460.c
> b/drivers/gpu/drm/bridge/nxp-ptn3460.c
> index ed93fd4c3265..e77aab965fcf 100644
> --- a/drivers/gpu/drm/bridge/nxp-ptn3460.c
> +++ b/drivers/gpu/drm/bridge/nxp-ptn3460.c
> @@ -229,11 +229,6 @@ static int ptn3460_bridge_attach(struct drm_bridge
> *bridge,
> if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)
> return 0;
>
> - if (!bridge->encoder) {
> - DRM_ERROR("Parent encoder object not found");
> - return -ENODEV;
> - }
> -
> ptn_bridge->connector.polled = DRM_CONNECTOR_POLL_HPD;
> ret = drm_connector_init(bridge->dev, _bridge->connector,
> _connector_funcs, DRM_MODE_CONNECTOR_LVDS);
--
Regards,
Laurent Pinchart
Re: [PATCH] drm/bridge: tfp410: Remove a small useless code snippet
Hi Sui,
Thank you for the patch.
On Sat, May 11, 2024 at 09:24:23PM +0800, Sui Jingfeng wrote:
> In the tfp410_attach(), the check on the existence of bridge->encoder has
> already been done in the implementation of drm_bridge_attach() function.
> The driver won't go further if bridge->encoder is NULL and the driver will
> quit even if drm_bridge_attach() fails for some reasons.
>
> Therefore there is no need to check another time at the later, remove the
> redundant checking codes "if (!bridge->encoder) { ... }".
>
> Signed-off-by: Sui Jingfeng
Reviewed-by: Laurent Pinchart
> ---
> drivers/gpu/drm/bridge/ti-tfp410.c | 5 -
> 1 file changed, 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ti-tfp410.c
> b/drivers/gpu/drm/bridge/ti-tfp410.c
> index c7bef5c23927..b1b1e4d5a24a 100644
> --- a/drivers/gpu/drm/bridge/ti-tfp410.c
> +++ b/drivers/gpu/drm/bridge/ti-tfp410.c
> @@ -133,11 +133,6 @@ static int tfp410_attach(struct drm_bridge *bridge,
> if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)
> return 0;
>
> - if (!bridge->encoder) {
> - dev_err(dvi->dev, "Missing encoder\n");
> - return -ENODEV;
> - }
> -
> if (dvi->next_bridge->ops & DRM_BRIDGE_OP_DETECT)
> dvi->connector.polled = DRM_CONNECTOR_POLL_HPD;
> else
--
Regards,
Laurent Pinchart
Re: [PATCH] drm/bridge: Remove a small useless code snippet
Hi Sui,
Thank you for the patch.
On Sat, May 11, 2024 at 08:42:38PM +0800, Sui Jingfeng wrote:
> Because the check on the non-existence (encoder == NULL) has already been
> done in the implementation of drm_bridge_attach() function, and
> drm_bridge_attach() is called earlier. The driver won't get to check point
> even if drm_bridge_attach() fails for some reasons, as it will clear the
> bridge->encoder to NULL and return a negective error code.
s/negective/negative/
>
> Therefore, there is no need to check another again. Remove the redundant
> codes at the later.
>
> Signed-off-by: Sui Jingfeng
Reviewed-by: Laurent Pinchart
If you end up sending a second version of this patch, please include all
similar patches you have sent at the same time in a patch series,
instead of sending them separately.
> ---
> drivers/gpu/drm/bridge/simple-bridge.c | 5 -
> 1 file changed, 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/simple-bridge.c
> b/drivers/gpu/drm/bridge/simple-bridge.c
> index 28376d0ecd09..3caa918ac2e0 100644
> --- a/drivers/gpu/drm/bridge/simple-bridge.c
> +++ b/drivers/gpu/drm/bridge/simple-bridge.c
> @@ -116,11 +116,6 @@ static int simple_bridge_attach(struct drm_bridge
> *bridge,
> if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)
> return 0;
>
> - if (!bridge->encoder) {
> - DRM_ERROR("Missing encoder\n");
> - return -ENODEV;
> - }
> -
> drm_connector_helper_add(>connector,
> _bridge_con_helper_funcs);
> ret = drm_connector_init_with_ddc(bridge->dev, >connector,
--
Regards,
Laurent Pinchart
Re: [PATCH 2/4] dt-bindings: display: ti: Add schema for AM625 OLDI Transmitter
ti,oldi-io-ctrl = <_oldi_io_ctrl>;
> +ports {
> +#address-cells = <1>;
> +#size-cells = <0>;
> +port@0 {
> +reg = <0>;
> +oldi0_in: endpoint {
> +remote-endpoint = <_out0>;
> +};
> +};
> +};
> +};
> +oldi1: oldi@1 {
> +reg = <1>;
> +ti,secondary-oldi;
> +ports {
> +#address-cells = <1>;
> +#size-cells = <0>;
> +port@0 {
> +reg = <0>;
> +oldi1_in: endpoint {
> +remote-endpoint = <_out1>;
> +};
> +};
> +};
> +};
> +};
> +
> +...
> diff --git a/MAINTAINERS b/MAINTAINERS
> index c675fc296b19..4426c4d41a7f 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7480,6 +7480,7 @@ M: Tomi Valkeinen
> L: dri-devel@lists.freedesktop.org
> S: Maintained
> T: git https://gitlab.freedesktop.org/drm/misc/kernel.git
> +F: Documentation/devicetree/bindings/display/ti/ti,am625-oldi.yaml
> F: Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
> F: Documentation/devicetree/bindings/display/ti/ti,j721e-dss.yaml
> F: Documentation/devicetree/bindings/display/ti/ti,k2g-dss.yaml
--
Regards,
Laurent Pinchart
Re: [PATCH 1/4] dt-bindings: display: ti,am65x-dss: Minor Cleanup
Hi Aradhya,
Thank you for the patch.
On Sun, May 12, 2024 at 01:00:52AM +0530, Aradhya Bhatia wrote:
> Reduce tab size from 8 spaces to 4 spaces to make the bindings
> consistent, and easy to expand.
>
> Signed-off-by: Aradhya Bhatia
Reviewed-by: Laurent Pinchart
> ---
> .../bindings/display/ti/ti,am65x-dss.yaml | 54 +--
> 1 file changed, 27 insertions(+), 27 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
> b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
> index 55e3e490d0e6..399d68986326 100644
> --- a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
> +++ b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
> @@ -142,32 +142,32 @@ examples:
> #include
>
> dss: dss@4a0 {
> -compatible = "ti,am65x-dss";
> -reg = <0x04a0 0x1000>, /* common */
> -<0x04a02000 0x1000>, /* vidl1 */
> -<0x04a06000 0x1000>, /* vid */
> -<0x04a07000 0x1000>, /* ovr1 */
> -<0x04a08000 0x1000>, /* ovr2 */
> -<0x04a0a000 0x1000>, /* vp1 */
> -<0x04a0b000 0x1000>, /* vp2 */
> -<0x04a01000 0x1000>; /* common1 */
> -reg-names = "common", "vidl1", "vid",
> -"ovr1", "ovr2", "vp1", "vp2", "common1";
> -ti,am65x-oldi-io-ctrl = <_oldi_io_ctrl>;
> -power-domains = <_pds 67 TI_SCI_PD_EXCLUSIVE>;
> -clocks =<_clks 67 1>,
> -<_clks 216 1>,
> -<_clks 67 2>;
> -clock-names = "fck", "vp1", "vp2";
> -interrupts = ;
> -ports {
> -#address-cells = <1>;
> -#size-cells = <0>;
> -port@0 {
> -reg = <0>;
> -oldi_out0: endpoint {
> -remote-endpoint = <_in0>;
> -};
> -};
> +compatible = "ti,am65x-dss";
> +reg = <0x04a0 0x1000>, /* common */
> + <0x04a02000 0x1000>, /* vidl1 */
> + <0x04a06000 0x1000>, /* vid */
> + <0x04a07000 0x1000>, /* ovr1 */
> + <0x04a08000 0x1000>, /* ovr2 */
> + <0x04a0a000 0x1000>, /* vp1 */
> + <0x04a0b000 0x1000>, /* vp2 */
> + <0x04a01000 0x1000>; /* common1 */
> +reg-names = "common", "vidl1", "vid",
> +"ovr1", "ovr2", "vp1", "vp2", "common1";
> +ti,am65x-oldi-io-ctrl = <_oldi_io_ctrl>;
> +power-domains = <_pds 67 TI_SCI_PD_EXCLUSIVE>;
> +clocks =<_clks 67 1>,
> +<_clks 216 1>,
> +<_clks 67 2>;
> +clock-names = "fck", "vp1", "vp2";
> +interrupts = ;
> +ports {
> +#address-cells = <1>;
> +#size-cells = <0>;
> +port@0 {
> +reg = <0>;
> +oldi_out0: endpoint {
> +remote-endpoint = <_in0>;
> +};
> };
> +};
> };
--
Regards,
Laurent Pinchart
Re: [PATCH v7 8/8] gpu: ipu-v3: Use generic macro for rounding to nearest multiple
On Fri, May 10, 2024 at 06:03:52PM +0300, Andy Shevchenko wrote: > On Fri, May 10, 2024 at 12:10:10AM +0530, Devarsh Thakkar wrote: > > Use generic macro round_closest_up for rounding to nearest multiple instead > > round_closest_up() > > We refer to the functions as func(). > > > of using local function. > > ... > > > @@ -565,7 +563,7 @@ static void find_best_seam(struct ipu_image_convert_ctx > > *ctx, > > * The closest input sample position that we could actually > > * start the input tile at, 19.13 fixed point. > > */ > > - in_pos_aligned = round_closest(in_pos, 8192U * in_align); > > + in_pos_aligned = round_closest_up(in_pos, 8192U * in_align); > > /* Convert 19.13 fixed point to integer */ > > in_pos_rounded = in_pos_aligned / 8192U; > > Oh, these seems to be better to use either ALIGN*(), or PFN_*() / PAGE_*() > families of macros. What the semantic of 8192 is? The comment mentions 19.13 fixed point, so I assume that's the fractional part of the integer. It doesn't seem related to pages. -- Regards, Laurent Pinchart
Re: Safety of opening up /dev/dma_heap/* to physically present users (udev uaccess tag) ?
On Wed, May 08, 2024 at 10:39:58AM +0200, Daniel Vetter wrote: > On Tue, May 07, 2024 at 10:59:42PM +0300, Dmitry Baryshkov wrote: > > On Tue, 7 May 2024 at 21:40, Laurent Pinchart wrote: > > > On Tue, May 07, 2024 at 06:19:18PM +0300, Dmitry Baryshkov wrote: > > > > On Tue, 7 May 2024 at 18:15, Bryan O'Donoghue wrote: > > > > > On 07/05/2024 16:09, Dmitry Baryshkov wrote: > > > > > > Ah, I see. Then why do you require the DMA-ble buffer at all? If > > > > > > you are > > > > > > providing data to VPU or DRM, then you should be able to get the > > > > > > buffer > > > > > > from the data-consuming device. > > > > > > > > > > Because we don't necessarily know what the consuming device is, if > > > > > any. > > > > > > > > > > Could be VPU, could be Zoom/Hangouts via pipewire, could for argument > > > > > sake be GPU or DSP. > > > > > > > > > > Also if we introduce a dependency on another device to allocate the > > > > > output buffers - say always taking the output buffer from the GPU, > > > > > then > > > > > we've added another dependency which is more difficult to guarantee > > > > > across different arches. > > > > > > > > Yes. And it should be expected. It's a consumer who knows the > > > > restrictions on the buffer. As I wrote, Zoom/Hangouts should not > > > > require a DMA buffer at all. > > > > > > Why not ? If you want to capture to a buffer that you then compose on > > > the screen without copying data, dma-buf is the way to go. That's the > > > Linux solution for buffer sharing. > > > > Yes. But it should be allocated by the DRM driver. As Sima wrote, > > there is no guarantee that the buffer allocated from dma-heaps is > > accessible to the GPU. > > > > > > > > > Applications should be able to allocate > > > > the buffer out of the generic memory. > > > > > > If applications really want to copy data and degrade performance, they > > > are free to shoot themselves in the foot of course. Applications (or > > > compositors) need to support copying as a fallback in the worst case, > > > but all components should at least aim for the zero-copy case. > > > > I'd say that they should aim for the optimal case. It might include > > both zero-copying access from another DMA master or simple software > > processing of some kind. > > > > > > GPUs might also have different > > > > requirements. Consider GPUs with VRAM. It might be beneficial to > > > > allocate a buffer out of VRAM rather than generic DMA mem. > > > > > > Absolutely. For that we need a centralized device memory allocator in > > > userspace. An effort was started by James Jones in 2016, see [1]. It has > > > unfortunately stalled. If I didn't have a camera framework to develop, I > > > would try to tackle that issue :-) > > > > I'll review the talk. However the fact that the effort has stalled > > most likely means that 'one fits them all' approach didn't really fly > > well. We have too many usecases. > > I think there's two reasons: > > - It's a really hard problem with many aspects. Where you need to allocate > the buffer is just one of the myriad of issues a common allocator needs > to solve. The other large problem is picking up an optimal pixel format. I wonder if that could be decoupled from the allocation. That could help moving forward. > - Every linux-based os has their own solution for these, and the one that > suffers most has an entirely different one from everyone else: Android > uses binder services to allow apps to make these allocations, keep track > of them and make sure there's no abuse. And if there is, it can just > nuke the app. -- Regards, Laurent Pinchart
Re: Safety of opening up /dev/dma_heap/* to physically present users (udev uaccess tag) ?
On Thu, May 09, 2024 at 12:51:08AM +0300, Laurent Pinchart wrote: > On Wed, May 08, 2024 at 10:36:08AM +0200, Daniel Vetter wrote: > > On Tue, May 07, 2024 at 04:07:39PM -0400, Nicolas Dufresne wrote: > > > Le mardi 07 mai 2024 à 21:36 +0300, Laurent Pinchart a écrit : > > > > Shorter term, we have a problem to solve, and the best option we have > > > > found so far is to rely on dma-buf heaps as a backend for the frame > > > > buffer allocatro helper in libcamera for the use case described above. > > > > This won't work in 100% of the cases, clearly. It's a stop-gap measure > > > > until we can do better. > > > > > > Considering the security concerned raised on this thread with dmabuf heap > > > allocation not be restricted by quotas, you'd get what you want quickly > > > with > > > memfd + udmabuf instead (which is accounted already). > > > > > > It was raised that distro don't enable udmabuf, but as stated there by > > > Hans, in > > > any cases distro needs to take action to make the softISP works. This > > > alternative is easy and does not interfere in anyway with your future > > > plan or > > > the libcamera API. You could even have both dmabuf heap (for Raspbian) > > > and the > > > safer memfd+udmabuf for the distro with security concerns. > > > > > > And for the long term plan, we can certainly get closer by fixing that > > > issue > > > with accounting. This issue also applied to v4l2 io-ops, so it would be > > > nice to > > > find common set of helpers to fix these exporters. > > > > Yeah if this is just for softisp, then memfd + udmabuf is also what I was > > about to suggest. Not just as a stopgap, but as the real official thing. > > Long term I still want a centralized memory allocator, at which point > libcamera should stop allocating buffers at all. And to be clear, udmabuf could be fine for the time being. At least as long as we don't find any shortcoming while testing it :-) > > udmabuf does kinda allow you to pin memory, but we can easily fix that by > > adding the right accounting and then either let mlock rlimits or cgroups > > kernel memory limits enforce good behavior. -- Regards, Laurent Pinchart
Re: Safety of opening up /dev/dma_heap/* to physically present users (udev uaccess tag) ?
On Wed, May 08, 2024 at 10:36:08AM +0200, Daniel Vetter wrote: > On Tue, May 07, 2024 at 04:07:39PM -0400, Nicolas Dufresne wrote: > > Le mardi 07 mai 2024 à 21:36 +0300, Laurent Pinchart a écrit : > > > Shorter term, we have a problem to solve, and the best option we have > > > found so far is to rely on dma-buf heaps as a backend for the frame > > > buffer allocatro helper in libcamera for the use case described above. > > > This won't work in 100% of the cases, clearly. It's a stop-gap measure > > > until we can do better. > > > > Considering the security concerned raised on this thread with dmabuf heap > > allocation not be restricted by quotas, you'd get what you want quickly with > > memfd + udmabuf instead (which is accounted already). > > > > It was raised that distro don't enable udmabuf, but as stated there by > > Hans, in > > any cases distro needs to take action to make the softISP works. This > > alternative is easy and does not interfere in anyway with your future plan > > or > > the libcamera API. You could even have both dmabuf heap (for Raspbian) and > > the > > safer memfd+udmabuf for the distro with security concerns. > > > > And for the long term plan, we can certainly get closer by fixing that issue > > with accounting. This issue also applied to v4l2 io-ops, so it would be > > nice to > > find common set of helpers to fix these exporters. > > Yeah if this is just for softisp, then memfd + udmabuf is also what I was > about to suggest. Not just as a stopgap, but as the real official thing. Long term I still want a centralized memory allocator, at which point libcamera should stop allocating buffers at all. > udmabuf does kinda allow you to pin memory, but we can easily fix that by > adding the right accounting and then either let mlock rlimits or cgroups > kernel memory limits enforce good behavior. -- Regards, Laurent Pinchart
Re: [PATCH] drm/drm-bridge.c: Drop conditionals around of_node pointers
On Wed, May 08, 2024 at 02:00:00AM +0800, Sui Jingfeng wrote:
> Having conditional around the of_node pointer of the drm_bridge structure
> is not necessary, since drm_bridge structure always has the of_node as its
> member.
>
> Let's drop the conditional to get a better looks, please also note that
> this is following the already accepted commitments. see commit d8dfccde2709
> ("drm/bridge: Drop conditionals around of_node pointers") for reference.
>
> Signed-off-by: Sui Jingfeng
It looks like this was forgotten in commit d8dfccde2709 ("drm/bridge:
Drop conditionals around of_node pointers").
Reviewed-by: Laurent Pinchart
> ---
> drivers/gpu/drm/drm_bridge.c | 5 -
> 1 file changed, 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index 30d66bee0ec6..a6dbe1751e88 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -352,13 +352,8 @@ int drm_bridge_attach(struct drm_encoder *encoder,
> struct drm_bridge *bridge,
> bridge->encoder = NULL;
> list_del(>chain_node);
>
> -#ifdef CONFIG_OF
> DRM_ERROR("failed to attach bridge %pOF to encoder %s: %d\n",
> bridge->of_node, encoder->name, ret);
> -#else
> - DRM_ERROR("failed to attach bridge to encoder %s: %d\n",
> - encoder->name, ret);
> -#endif
>
> return ret;
> }
--
Regards,
Laurent Pinchart
Re: Safety of opening up /dev/dma_heap/* to physically present users (udev uaccess tag) ?
On Tue, May 07, 2024 at 10:59:42PM +0300, Dmitry Baryshkov wrote: > On Tue, 7 May 2024 at 21:40, Laurent Pinchart wrote: > > On Tue, May 07, 2024 at 06:19:18PM +0300, Dmitry Baryshkov wrote: > > > On Tue, 7 May 2024 at 18:15, Bryan O'Donoghue wrote: > > > > On 07/05/2024 16:09, Dmitry Baryshkov wrote: > > > > > Ah, I see. Then why do you require the DMA-ble buffer at all? If you > > > > > are > > > > > providing data to VPU or DRM, then you should be able to get the > > > > > buffer > > > > > from the data-consuming device. > > > > > > > > Because we don't necessarily know what the consuming device is, if any. > > > > > > > > Could be VPU, could be Zoom/Hangouts via pipewire, could for argument > > > > sake be GPU or DSP. > > > > > > > > Also if we introduce a dependency on another device to allocate the > > > > output buffers - say always taking the output buffer from the GPU, then > > > > we've added another dependency which is more difficult to guarantee > > > > across different arches. > > > > > > Yes. And it should be expected. It's a consumer who knows the > > > restrictions on the buffer. As I wrote, Zoom/Hangouts should not > > > require a DMA buffer at all. > > > > Why not ? If you want to capture to a buffer that you then compose on > > the screen without copying data, dma-buf is the way to go. That's the > > Linux solution for buffer sharing. > > Yes. But it should be allocated by the DRM driver. As Sima wrote, > there is no guarantee that the buffer allocated from dma-heaps is > accessible to the GPU. No disagreement there. From a libcamera point of view, we want to import buffers allocated externally. It's for use cases where applications can't provide dma buf instances easily that we need to allocate them through the libcamera buffer allocator helper. That helper has to a) provide dma buf fds, and b) make a best effort to allocate buffers that will have a decent chance of being usable by other devices. We're open to exploring other solutions than dma heaps, although I wonder what the dma heaps are for if nobody enables them :-) > > > Applications should be able to allocate > > > the buffer out of the generic memory. > > > > If applications really want to copy data and degrade performance, they > > are free to shoot themselves in the foot of course. Applications (or > > compositors) need to support copying as a fallback in the worst case, > > but all components should at least aim for the zero-copy case. > > I'd say that they should aim for the optimal case. It might include > both zero-copying access from another DMA master or simple software > processing of some kind. > > > > GPUs might also have different > > > requirements. Consider GPUs with VRAM. It might be beneficial to > > > allocate a buffer out of VRAM rather than generic DMA mem. > > > > Absolutely. For that we need a centralized device memory allocator in > > userspace. An effort was started by James Jones in 2016, see [1]. It has > > unfortunately stalled. If I didn't have a camera framework to develop, I > > would try to tackle that issue :-) > > I'll review the talk. However the fact that the effort has stalled > most likely means that 'one fits them all' approach didn't really fly > well. We have too many usecases. > > > [1] > > https://www.x.org/wiki/Events/XDC2016/Program/Unix_Device_Memory_Allocation.pdf -- Regards, Laurent Pinchart
Re: Safety of opening up /dev/dma_heap/* to physically present users (udev uaccess tag) ?
On Mon, May 06, 2024 at 03:38:24PM +0200, Daniel Vetter wrote: > On Mon, May 06, 2024 at 02:05:12PM +0200, Maxime Ripard wrote: > > On Mon, May 06, 2024 at 01:49:17PM GMT, Hans de Goede wrote: > > > Hi dma-buf maintainers, et.al., > > > > > > Various people have been working on making complex/MIPI cameras work OOTB > > > with mainline Linux kernels and an opensource userspace stack. > > > > > > The generic solution adds a software ISP (for Debayering and 3A) to > > > libcamera. Libcamera's API guarantees that buffers handed to applications > > > using it are dma-bufs so that these can be passed to e.g. a video encoder. > > > > > > In order to meet this API guarantee the libcamera software ISP allocates > > > dma-bufs from userspace through one of the /dev/dma_heap/* heaps. For > > > the Fedora COPR repo for the PoC of this: > > > https://hansdegoede.dreamwidth.org/28153.html > > > > For the record, we're also considering using them for ARM KMS devices, > > so it would be better if the solution wasn't only considering v4l2 > > devices. > > > > > I have added a simple udev rule to give physically present users access > > > to the dma_heap-s: > > > > > > KERNEL=="system", SUBSYSTEM=="dma_heap", TAG+="uaccess" > > > > > > (and on Rasperry Pi devices any users in the video group get access) > > > > > > This was just a quick fix for the PoC. Now that we are ready to move out > > > of the PoC phase and start actually integrating this into distributions > > > the question becomes if this is an acceptable solution; or if we need some > > > other way to deal with this ? > > > > > > Specifically the question is if this will have any negative security > > > implications? I can certainly see this being used to do some sort of > > > denial of service attack on the system (1). This is especially true for > > > the cma heap which generally speaking is a limited resource. > > > > There's plenty of other ways to exhaust CMA, like allocating too much > > KMS or v4l2 buffers. I'm not sure we should consider dma-heaps > > differently than those if it's part of our threat model. > > So generally for an arm soc where your display needs cma, your render node > doesn't. And user applications only have access to the later, while only > the compositor gets a kms fd through logind. At least in drm aside from > vc4 there's really no render driver that just gives you access to cma and > allows you to exhaust that, you need to be a compositor with drm master > access to the display. > > Which means we're mostly protected against bad applications, and that's > not a threat the "user physically sits in front of the machine accounts > for", and which giving cma access to everyone would open up. And with > flathub/snaps/... this is very much an issue. > > So you need more, either: > > - cgroups limits on dma-buf and dma-buf heaps. This has been bikeshedded > for years and is just not really moving. > > - An allocator service which checks whether you're allowed to allocate > these special buffers. Android does that through binder. I would split the latter into a centralized frame buffer allocator library for userspace (James Jones' Unix device memory allocator comes to mind), providing dma-buf instances using whatever backend is available and suitable (DMA heaps would likely be one of them), and a safe way to make this allocator available to applications. Exposing it through some system services could be useful, but with proper tracking and accounting of memory allocated through DMA heaps (and DRM, and V4L2) we could possibly do with just a library. What I really want is to move memory allocation for devices to a separate component, and make everything else a consumer of those buffers. > Probably also some way to nuke applications that refuse to release buffers > when they're no longer the right application. cgroups is a lot more > convenient for that. > > > > But devices tagged for uaccess are only opened up to users who are > > > physcially present behind the machine and those can just hit > > > the powerbutton, so I don't believe that any *on purpose* DOS is part of > > > the thread model. > > > > How would that work for headless devices? -- Regards, Laurent Pinchart
Re: Safety of opening up /dev/dma_heap/* to physically present users (udev uaccess tag) ?
On Tue, May 07, 2024 at 06:19:18PM +0300, Dmitry Baryshkov wrote: > On Tue, 7 May 2024 at 18:15, Bryan O'Donoghue wrote: > > On 07/05/2024 16:09, Dmitry Baryshkov wrote: > > > Ah, I see. Then why do you require the DMA-ble buffer at all? If you are > > > providing data to VPU or DRM, then you should be able to get the buffer > > > from the data-consuming device. > > > > Because we don't necessarily know what the consuming device is, if any. > > > > Could be VPU, could be Zoom/Hangouts via pipewire, could for argument > > sake be GPU or DSP. > > > > Also if we introduce a dependency on another device to allocate the > > output buffers - say always taking the output buffer from the GPU, then > > we've added another dependency which is more difficult to guarantee > > across different arches. > > Yes. And it should be expected. It's a consumer who knows the > restrictions on the buffer. As I wrote, Zoom/Hangouts should not > require a DMA buffer at all. Why not ? If you want to capture to a buffer that you then compose on the screen without copying data, dma-buf is the way to go. That's the Linux solution for buffer sharing. > Applications should be able to allocate > the buffer out of the generic memory. If applications really want to copy data and degrade performance, they are free to shoot themselves in the foot of course. Applications (or compositors) need to support copying as a fallback in the worst case, but all components should at least aim for the zero-copy case. > GPUs might also have different > requirements. Consider GPUs with VRAM. It might be beneficial to > allocate a buffer out of VRAM rather than generic DMA mem. Absolutely. For that we need a centralized device memory allocator in userspace. An effort was started by James Jones in 2016, see [1]. It has unfortunately stalled. If I didn't have a camera framework to develop, I would try to tackle that issue :-) [1] https://www.x.org/wiki/Events/XDC2016/Program/Unix_Device_Memory_Allocation.pdf -- Regards, Laurent Pinchart
Re: Safety of opening up /dev/dma_heap/* to physically present users (udev uaccess tag) ?
On Tue, May 07, 2024 at 07:36:59PM +0200, Daniel Vetter wrote: > On Tue, May 07, 2024 at 04:15:05PM +0100, Bryan O'Donoghue wrote: > > On 07/05/2024 16:09, Dmitry Baryshkov wrote: > > > Ah, I see. Then why do you require the DMA-ble buffer at all? If you are > > > providing data to VPU or DRM, then you should be able to get the buffer > > > from the data-consuming device. > > > > Because we don't necessarily know what the consuming device is, if any. > > Well ... that's an entirely different issue. And it's unsolved. > > Currently the approach is to allocate where the constraints are usually > most severe (like display, if you need that, or the camera module for > input) and then just pray the stack works out without too much copying. > All userspace (whether the generic glue or the userspace driver depends a > bit upon the exact api) does need to have a copy fallback for these > sharing cases, ideally with the copying accelerated by hw. > > If you try to solve this by just preemptive allocating everything as cma > buffers, then you'll make the situation substantially worse (because now > you're wasting tons of cma memory where you might not even need it). > And without really solving the problem, since for some gpus that memory > might be unusable (because you cannot scan that out on any discrete gpu, > and sometimes not even on an integrated one). I think we have a general agreement that the proposed solution is a stop-gap measure for an unsolved issue. Note that libcamera is already designed that way. The API is designed to import buffers, using dma-buf file handles. If an application has a way to allocate dma-buf instances through another means (from the display or from a video encoder for instance), it should do so, and use those buffers with libcamera. For applications that don't have an easy way to get hold of dma-buf instances, we have a buffer allocator helper as a side component. That allocator uses the underlying camera capture device, and allocates buffers from the V4L2 video device. It's only on platforms where we have no hardware camera processing (or, rather, platforms where the hardware vendors doesn't give us access to the camera hardware, such as recent Intel SoCs, or Qualcomm SoCs used in ARM laptops) that we need to allocate memory elsewhere. In the long run, I want a centralized memory allocator accessible by userspace applications (something similar in purpose to gralloc on Android), and I want to get rid of buffer allocation in libcamera (and even in V4L2, in the even longer term). That's the long run. Shorter term, we have a problem to solve, and the best option we have found so far is to rely on dma-buf heaps as a backend for the frame buffer allocatro helper in libcamera for the use case described above. This won't work in 100% of the cases, clearly. It's a stop-gap measure until we can do better. > > Could be VPU, could be Zoom/Hangouts via pipewire, could for argument sake > > be GPU or DSP. > > > > Also if we introduce a dependency on another device to allocate the output > > buffers - say always taking the output buffer from the GPU, then we've added > > another dependency which is more difficult to guarantee across different > > arches. -- Regards, Laurent Pinchart
Re: [BUG] drm: zynqmp_dp: Lockup in zynqmp_dp_bridge_detect when device is unbound
On Mon, May 06, 2024 at 10:57:17AM -0400, Sean Anderson wrote: > On 5/6/24 03:35, Laurent Pinchart wrote: > > On Mon, May 06, 2024 at 09:29:36AM +0200, Maxime Ripard wrote: > >> Hi Laurent, Sean, > >> > >> On Sat, May 04, 2024 at 03:21:18PM GMT, Laurent Pinchart wrote: > >> > On Fri, May 03, 2024 at 05:54:32PM -0400, Sean Anderson wrote: > >> > > I have discovered a bug in the displayport driver on drm-misc-next. To > >> > > trigger it, run > >> > > > >> > > echo fd4a.display > /sys/bus/platform/drivers/zynqmp-dpsub/unbind > >> > > > >> > > The system will become unresponsive and (after a bit) splat with a hard > >> > > LOCKUP. One core will be unresponsive at the first zynqmp_dp_read in > >> > > zynqmp_dp_bridge_detect. > >> > > > >> > > I believe the issue is due the registers being unmapped and the block > >> > > put into reset in zynqmp_dp_remove instead of zynqmp_dpsub_release. > >> > > >> > That is on purpose. Drivers are not allowed to access the device at all > >> > after .remove() returns. > >> > >> It's not "on purpose" no. Drivers indeed are not allowed to access the > >> device after remove, but the kernel shouldn't crash. This is exactly > >> why we have drm_dev_enter / drm_dev_exit. > > > > I didn't mean the crash was on purpose :-) It's the registers being > > unmapped that is, as nothing should touch those registers after > > .remove() returns. > > OK, so then we need to have some kind of flag in the driver or in the drm > subsystem so we know not to access those registers. To avoid race conditions, the .remove() function should mark the device as removed, wait for all ongoing access from userspace to be complete, and then proceed to unmapping registers and doing other cleanups. Userspace may still have open file descriptors to the device at that point. Any new userspace access should be disallowed (by checking the removed flag), with the only userspace-initiated operations that still need to run being the release-related operations (unmapping memory, closing file descriptors, ...). -- Regards, Laurent Pinchart
Re: [BUG] drm: zynqmp_dp: Lockup in zynqmp_dp_bridge_detect when device is unbound
On Mon, May 06, 2024 at 09:29:36AM +0200, Maxime Ripard wrote: > Hi Laurent, Sean, > > On Sat, May 04, 2024 at 03:21:18PM GMT, Laurent Pinchart wrote: > > On Fri, May 03, 2024 at 05:54:32PM -0400, Sean Anderson wrote: > > > I have discovered a bug in the displayport driver on drm-misc-next. To > > > trigger it, run > > > > > > echo fd4a.display > /sys/bus/platform/drivers/zynqmp-dpsub/unbind > > > > > > The system will become unresponsive and (after a bit) splat with a hard > > > LOCKUP. One core will be unresponsive at the first zynqmp_dp_read in > > > zynqmp_dp_bridge_detect. > > > > > > I believe the issue is due the registers being unmapped and the block > > > put into reset in zynqmp_dp_remove instead of zynqmp_dpsub_release. > > > > That is on purpose. Drivers are not allowed to access the device at all > > after .remove() returns. > > It's not "on purpose" no. Drivers indeed are not allowed to access the > device after remove, but the kernel shouldn't crash. This is exactly > why we have drm_dev_enter / drm_dev_exit. I didn't mean the crash was on purpose :-) It's the registers being unmapped that is, as nothing should touch those registers after .remove() returns. -- Regards, Laurent Pinchart
Re: [BUG] drm: zynqmp_dp: Lockup in zynqmp_dp_bridge_detect when device is unbound
Hi Sean,
On Fri, May 03, 2024 at 05:54:32PM -0400, Sean Anderson wrote:
> Hi,
>
> I have discovered a bug in the displayport driver on drm-misc-next. To
> trigger it, run
>
> echo fd4a.display > /sys/bus/platform/drivers/zynqmp-dpsub/unbind
>
> The system will become unresponsive and (after a bit) splat with a hard
> LOCKUP. One core will be unresponsive at the first zynqmp_dp_read in
> zynqmp_dp_bridge_detect.
>
> I believe the issue is due the registers being unmapped and the block
> put into reset in zynqmp_dp_remove instead of zynqmp_dpsub_release.
That is on purpose. Drivers are not allowed to access the device at all
after .remove() returns.
> This
> could be resolved by deferring things until zynqmp_dpsub_release
> (requiring us to skip devm_*), or by adding a flag to struct zynqmp_dp
> and checking it before each callback. A subsystem-level implementation
> might be better for the latter.
>
> For a better traceback, try applying the below patch and running the
> following commands before triggering the lockup:
>
> echo 4 > /sys/module/drm/parameters/debug
> echo 8 > /proc/sys/kernel/printk
>
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c
> b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> index 9df068a413f3..17b477b14ab5 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> @@ -296,6 +296,7 @@ struct zynqmp_dp_config {
> * @train_set: set of training data
> */
> struct zynqmp_dp {
> + unsigned long long magic;
> struct device *dev;
> struct zynqmp_dpsub *dpsub;
> void __iomem *iomem;
> @@ -1533,6 +1534,8 @@ static enum drm_connector_status
> zynqmp_dp_bridge_detect(struct drm_bridge *brid
> u32 state, i;
> int ret;
>
> + WARN_ON(dp->magic != 0x0123456789abcdefULL);
> +
> /*
> * This is from heuristic. It takes some delay (ex, 100 ~ 500 msec) to
> * get the HPD signal with some monitors.
> @@ -1723,6 +1726,7 @@ int zynqmp_dp_probe(struct zynqmp_dpsub *dpsub)
> if (!dp)
> return -ENOMEM;
>
> + dp->magic = 0x0123456789abcdefULL;
> dp->dev = >dev;
> dp->dpsub = dpsub;
> dp->status = connector_status_disconnected;
> @@ -1839,4 +1843,5 @@ void zynqmp_dp_remove(struct zynqmp_dpsub *dpsub)
>
> zynqmp_dp_phy_exit(dp);
> zynqmp_dp_reset(dp, true);
> + dp->magic = 0xdeadbeefdeadbeefULL;
> }
--
Regards,
Laurent Pinchart
Re: [PATCH] drm/fourcc: Add RGB161616 and BGR161616 formats
Hi Jacopo,
On Thu, May 02, 2024 at 11:02:27AM +0200, Jacopo Mondi wrote:
> Hello
>which tree should this patch be collected from now that it has
> been reviewed ?
I think this can go through drm-misc. I'm not sure what the rule is for
patches that touch core code like these, can then be pushed by anyone
with commit access, or do they need to be collected by a drm-misc
maintainer ?
> On Mon, Feb 26, 2024 at 02:25:43PM GMT, Jacopo Mondi wrote:
> > Add FourCC definitions for the 48-bit RGB/BGR formats to the
> > DRM/KMS uapi.
> >
> > The format will be used by the Raspberry Pi PiSP Back End,
> > supported by a V4L2 driver in kernel space and by libcamera in
> > userspace, which uses the DRM FourCC identifiers.
> >
> > Signed-off-by: Jacopo Mondi
> > ---
> > drivers/gpu/drm/drm_fourcc.c | 8
> > include/uapi/drm/drm_fourcc.h | 4
> > 2 files changed, 12 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
> > index 193cf8ed7912..908f20b96fd5 100644
> > --- a/drivers/gpu/drm/drm_fourcc.c
> > +++ b/drivers/gpu/drm/drm_fourcc.c
> > @@ -210,6 +210,14 @@ const struct drm_format_info *__drm_format_info(u32
> > format)
> > { .format = DRM_FORMAT_ABGR2101010, .depth = 30,
> > .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1, .has_alpha =
> > true },
> > { .format = DRM_FORMAT_RGBA1010102, .depth = 30,
> > .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1, .has_alpha =
> > true },
> > { .format = DRM_FORMAT_BGRA1010102, .depth = 30,
> > .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1, .has_alpha =
> > true },
> > + { .format = DRM_FORMAT_RGB161616, .depth = 0,
> > + .num_planes = 1, .char_per_block = { 6, 0, 0 },
> > + .block_w = { 1, 0, 0 }, .block_h = { 1, 0, 0 },
> > + .hsub = 1, .vsub = 1, .has_alpha = false },
> > + { .format = DRM_FORMAT_BGR161616, .depth = 0,
> > + .num_planes = 1, .char_per_block = { 6, 0, 0 },
> > + .block_w = { 1, 0, 0 }, .block_h = { 1, 0, 0 },
> > + .hsub = 1, .vsub = 1, .has_alpha = false },
> > { .format = DRM_FORMAT_ARGB,.depth = 32,
> > .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1, .has_alpha =
> > true },
> > { .format = DRM_FORMAT_ABGR,.depth = 32,
> > .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1, .has_alpha =
> > true },
> > { .format = DRM_FORMAT_RGBA,.depth = 32,
> > .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1, .has_alpha =
> > true },
> > diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> > index 84d502e42961..00db00083175 100644
> > --- a/include/uapi/drm/drm_fourcc.h
> > +++ b/include/uapi/drm/drm_fourcc.h
> > @@ -210,6 +210,10 @@ extern "C" {
> > #define DRM_FORMAT_RGBA1010102 fourcc_code('R', 'A', '3', '0') /*
> > [31:0] R:G:B:A 10:10:10:2 little endian */
> > #define DRM_FORMAT_BGRA1010102 fourcc_code('B', 'A', '3', '0') /*
> > [31:0] B:G:R:A 10:10:10:2 little endian */
> >
> > +/* 48 bpp RGB */
> > +#define DRM_FORMAT_RGB161616 fourcc_code('R', 'G', '4', '8') /* [47:0]
> > R:G:B 16:16:16 little endian */
> > +#define DRM_FORMAT_BGR161616 fourcc_code('B', 'G', '4', '8') /* [47:0]
> > B:G:R 16:16:16 little endian */
> > +
> > /* 64 bpp RGB */
> > #define DRM_FORMAT_XRGB16161616fourcc_code('X', 'R', '4', '8') /*
> > [63:0] x:R:G:B 16:16:16:16 little endian */
> > #define DRM_FORMAT_XBGR16161616fourcc_code('X', 'B', '4', '8') /*
> > [63:0] x:B:G:R 16:16:16:16 little endian */
--
Regards,
Laurent Pinchart
Re: [PATCH] drm: zynqmp_dpsub: Always register bridge
t; I have the drm_bridge_add() call in zynqmp_dp_probe(), as that's where > the bridge is set up, so it felt like a logical place. You add it later, > just before the bridge is used the first time. > > I like mine a bit more as it has all the bridge code in the same place, > but I also wonder if there might be some risks in adding the bridge > early (before zynqmp_disp_probe()), although I can't see any issue right > away... Seems we have the same concerns :-) I've reviewed your patch and wrote pretty much the same. I would be more comfortable with this version, even if I like gathering all bridge code in the same location. > In any case, as this works for me too: > > Reviewed-by: Tomi Valkeinen Reviewed-by: Laurent Pinchart -- Regards, Laurent Pinchart
Re: [PATCH] drm: xlnx: zynqmp_dpsub: Fix missing drm_bridge_add() call
Hi Tomi,
Thank you for the patch.
On Tue, Mar 12, 2024 at 10:51:15AM +0200, Tomi Valkeinen wrote:
> The driver creates a bridge, but never calls drm_bridge_add() when
> non-live input is used. This leaves the bridge's hpd_mutex
> uninitialized, leading to:
>
> WARNING: CPU: 0 PID: 9 at kernel/locking/mutex.c:582 __mutex_lock+0x708/0x840
>
> Add the bridge add & remove calls so that the bridge gets managed
> correctly.
>
> Signed-off-by: Tomi Valkeinen
> Fixes: 561671612394 ("drm: xlnx: zynqmp_dpsub: Add support for live video
> input")
> ---
> drivers/gpu/drm/xlnx/zynqmp_dp.c| 4
> drivers/gpu/drm/xlnx/zynqmp_dpsub.c | 4
> 2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c
> b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> index a0606fab0e22..9f750740dfb8 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> @@ -1761,6 +1761,8 @@ int zynqmp_dp_probe(struct zynqmp_dpsub *dpsub)
>
> dpsub->dp = dp;
>
> + drm_bridge_add(dpsub->bridge);
> +
This means that the bridge will be exposed to users before
zynqmp_disp_probe() is called, opening the door to a potential
use-before-init. The risk is mostly theoretical at this point I believe,
but it's still not a direction I'd like to that. Could you call
drm_bridge_add() in zynqmp_dpsub_probe(), between zynqmp_disp_probe()
and zynqmp_dpsub_drm_init() ?
> dev_dbg(dp->dev, "ZynqMP DisplayPort Tx probed with %u lanes\n",
> dp->num_lanes);
>
> @@ -1789,4 +1791,6 @@ void zynqmp_dp_remove(struct zynqmp_dpsub *dpsub)
>
> zynqmp_dp_phy_exit(dp);
> zynqmp_dp_reset(dp, true);
> +
> + drm_bridge_remove(dpsub->bridge);
> }
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
> b/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
> index 88eb33acd5f0..3933c4f1a44f 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
> @@ -260,8 +260,6 @@ static int zynqmp_dpsub_probe(struct platform_device
> *pdev)
> ret = zynqmp_dpsub_drm_init(dpsub);
> if (ret)
> goto err_disp;
> - } else {
> - drm_bridge_add(dpsub->bridge);
> }
>
> dev_info(>dev, "ZynqMP DisplayPort Subsystem driver probed");
> @@ -288,8 +286,6 @@ static void zynqmp_dpsub_remove(struct platform_device
> *pdev)
>
> if (dpsub->drm)
> zynqmp_dpsub_drm_cleanup(dpsub);
> - else
> - drm_bridge_remove(dpsub->bridge);
>
> zynqmp_disp_remove(dpsub);
> zynqmp_dp_remove(dpsub);
>
> ---
> base-commit: e8f897f4afef0031fe618a8e94127a0934896aba
> change-id: 20240312-xilinx-dp-lock-fix-cf68f43a7bab
--
Regards,
Laurent Pinchart
Re: [PATCH V2] drm/bridge: imx: Fix unmet depenency for PHY_FSL_SAMSUNG_HDMI_PHY
Hi Adam,
Thank you for the patch.
On Mon, Apr 22, 2024 at 05:33:52AM -0500, Adam Ford wrote:
> When enabling i.MX8MP DWC HDMI driver, it automatically selects
> PHY_FSL_SAMSUNG_HDMI_PHY, since it wont' work without the phy.
> This may cause some Kconfig warnings during various build tests.
> Fix this by implying the phy instead of selecting the phy.
>
> To prevent this from happening with the DRM_IMX8MP_HDMI_PVI, also
> imply it instead of selecting it.
>
> Fixes: 1f36d634670d ("drm/bridge: imx: add bridge wrapper driver for i.MX8MP
> DWC HDMI")
> Reported-by: kernel test robot
> Closes:
> https://lore.kernel.org/oe-kbuild-all/202404190103.llm8ltup-...@intel.com/
> Signed-off-by: Adam Ford
This looks sensible to me.
Reviewed-by: Laurent Pinchart
> ---
> V2: Change DRM_IMX8MP_HDMI_PVI at the same time since it was affected
> from the same commit.
>
> diff --git a/drivers/gpu/drm/bridge/imx/Kconfig
> b/drivers/gpu/drm/bridge/imx/Kconfig
> index 7687ed652df5..13142a6b8590 100644
> --- a/drivers/gpu/drm/bridge/imx/Kconfig
> +++ b/drivers/gpu/drm/bridge/imx/Kconfig
> @@ -8,8 +8,8 @@ config DRM_IMX8MP_DW_HDMI_BRIDGE
> depends on COMMON_CLK
> depends on DRM_DW_HDMI
> depends on OF
> - select DRM_IMX8MP_HDMI_PVI
> - select PHY_FSL_SAMSUNG_HDMI_PHY
> + imply DRM_IMX8MP_HDMI_PVI
> + imply PHY_FSL_SAMSUNG_HDMI_PHY
> help
> Choose this to enable support for the internal HDMI encoder found
> on the i.MX8MP SoC.
--
Regards,
Laurent Pinchart
Re: [PATCH] drm/bridge: imx: Fix unmet depenency for PHY_FSL_SAMSUNG_HDMI_PHY
Hi Adam,
Thank you for the patch.
On Sat, Apr 20, 2024 at 06:28:31AM -0500, Adam Ford wrote:
> When enabling i.MX8MP DWC HDMI driver, it automatically selects
> PHY_FSL_SAMSUNG_HDMI_PHY, since it wont' work without the phy.
> This may cause some Kconfig warnings during various build tests.
> Fix this by implying the phy instead of selecting the phy.
>
> Fixes: 1f36d634670d ("drm/bridge: imx: add bridge wrapper driver for i.MX8MP
> DWC HDMI")
> Reported-by: kernel test robot
> Closes:
> https://lore.kernel.org/oe-kbuild-all/202404190103.llm8ltup-...@intel.com/
> Signed-off-by: Adam Ford
>
> diff --git a/drivers/gpu/drm/bridge/imx/Kconfig
> b/drivers/gpu/drm/bridge/imx/Kconfig
> index 7687ed652df5..8f125c75050d 100644
> --- a/drivers/gpu/drm/bridge/imx/Kconfig
> +++ b/drivers/gpu/drm/bridge/imx/Kconfig
> @@ -9,7 +9,7 @@ config DRM_IMX8MP_DW_HDMI_BRIDGE
> depends on DRM_DW_HDMI
> depends on OF
> select DRM_IMX8MP_HDMI_PVI
This also looks wrong, even if in practice it will likely work because
DRM_IMX8MP_HDMI_PVI has no extra dependency compared to
DRM_IMX8MP_DW_HDMI_BRIDGE. Still, it would be good to fix it.
> - select PHY_FSL_SAMSUNG_HDMI_PHY
> + imply PHY_FSL_SAMSUNG_HDMI_PHY
> help
> Choose this to enable support for the internal HDMI encoder found
> on the i.MX8MP SoC.
--
Regards,
Laurent Pinchart
Re: [PATCH] i2c: mux: Remove class argument from i2c_mux_add_adapter()
Hi Heiner,
Thank you for the patch.
On Thu, Apr 18, 2024 at 10:55:39PM +0200, Heiner Kallweit wrote:
> 99a741aa7a2d ("i2c: mux: gpio: remove support for class-based device
> instantiation") removed the last call to i2c_mux_add_adapter() with a
> non-null class argument. Therefore the class argument can be removed.
>
> Note: Class-based device instantiation is a legacy mechanism which
> shouldn't be used in new code, so we can rule out that this argument
> may be needed again in the future.
>
> Signed-off-by: Heiner Kallweit
Reviewed-by: Laurent Pinchart
> ---
> drivers/gpu/drm/bridge/sii902x.c | 2 +-
> drivers/i2c/i2c-mux.c | 24 +-
> drivers/i2c/muxes/i2c-arb-gpio-challenge.c | 2 +-
> drivers/i2c/muxes/i2c-mux-gpio.c | 2 +-
> drivers/i2c/muxes/i2c-mux-gpmux.c | 2 +-
> drivers/i2c/muxes/i2c-mux-ltc4306.c| 2 +-
> drivers/i2c/muxes/i2c-mux-mlxcpld.c| 2 +-
> drivers/i2c/muxes/i2c-mux-pca9541.c| 2 +-
> drivers/i2c/muxes/i2c-mux-pca954x.c| 2 +-
> drivers/i2c/muxes/i2c-mux-pinctrl.c| 2 +-
> drivers/i2c/muxes/i2c-mux-reg.c| 2 +-
> drivers/iio/gyro/mpu3050-i2c.c | 2 +-
> drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c | 2 +-
> drivers/media/dvb-frontends/af9013.c | 2 +-
> drivers/media/dvb-frontends/lgdt3306a.c| 2 +-
> drivers/media/dvb-frontends/m88ds3103.c| 2 +-
> drivers/media/dvb-frontends/rtl2830.c | 2 +-
> drivers/media/dvb-frontends/rtl2832.c | 2 +-
> drivers/media/dvb-frontends/si2168.c | 2 +-
> drivers/media/i2c/max9286.c| 2 +-
> drivers/media/usb/cx231xx/cx231xx-i2c.c| 5 +
> drivers/of/unittest.c | 2 +-
> include/linux/i2c-mux.h| 3 +--
> 23 files changed, 23 insertions(+), 49 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/sii902x.c
> b/drivers/gpu/drm/bridge/sii902x.c
> index 8f84e9824..2fbeda902 100644
> --- a/drivers/gpu/drm/bridge/sii902x.c
> +++ b/drivers/gpu/drm/bridge/sii902x.c
> @@ -1092,7 +1092,7 @@ static int sii902x_init(struct sii902x *sii902x)
> }
>
> sii902x->i2cmux->priv = sii902x;
> - ret = i2c_mux_add_adapter(sii902x->i2cmux, 0, 0, 0);
> + ret = i2c_mux_add_adapter(sii902x->i2cmux, 0, 0);
> if (ret)
> goto err_unreg_audio;
>
> diff --git a/drivers/i2c/i2c-mux.c b/drivers/i2c/i2c-mux.c
> index 57ff09f18..fda72e8be 100644
> --- a/drivers/i2c/i2c-mux.c
> +++ b/drivers/i2c/i2c-mux.c
> @@ -127,19 +127,6 @@ static u32 i2c_mux_functionality(struct i2c_adapter
> *adap)
> return parent->algo->functionality(parent);
> }
>
> -/* Return all parent classes, merged */
> -static unsigned int i2c_mux_parent_classes(struct i2c_adapter *parent)
> -{
> - unsigned int class = 0;
> -
> - do {
> - class |= parent->class;
> - parent = i2c_parent_is_i2c_adapter(parent);
> - } while (parent);
> -
> - return class;
> -}
> -
> static void i2c_mux_lock_bus(struct i2c_adapter *adapter, unsigned int flags)
> {
> struct i2c_mux_priv *priv = adapter->algo_data;
> @@ -281,8 +268,7 @@ static const struct i2c_lock_operations
> i2c_parent_lock_ops = {
> };
>
> int i2c_mux_add_adapter(struct i2c_mux_core *muxc,
> - u32 force_nr, u32 chan_id,
> - unsigned int class)
> + u32 force_nr, u32 chan_id)
> {
> struct i2c_adapter *parent = muxc->parent;
> struct i2c_mux_priv *priv;
> @@ -340,14 +326,6 @@ int i2c_mux_add_adapter(struct i2c_mux_core *muxc,
> else
> priv->adap.lock_ops = _parent_lock_ops;
>
> - /* Sanity check on class */
> - if (i2c_mux_parent_classes(parent) & class & ~I2C_CLASS_DEPRECATED)
> - dev_err(>dev,
> - "Segment %d behind mux can't share classes with
> ancestors\n",
> - chan_id);
> - else
> - priv->adap.class = class;
> -
> /*
>* Try to populate the mux adapter's of_node, expands to
>* nothing if !CONFIG_OF.
> diff --git a/drivers/i2c/muxes/i2c-arb-gpio-challenge.c
> b/drivers/i2c/muxes/i2c-arb-gpio-challenge.c
> index 24168e9f7..7aa6e795d 100644
> --- a/drivers/i2c/muxes/i2c-arb-gpio-challenge.c
> +++ b/drivers/i2c/muxes/i2c-arb-gpio-challenge.c
> @@ -167,7 +167,7 @@ static int i2c_arbitrator_probe(struct platform_device
> *pdev)
> }
>
> /* Actually add the mux adapter */
> - ret = i2c_mux_add_adapter(muxc, 0, 0, 0);
&g
Re: [PATCH v2 28/43] drm/renesas/rcar-du: Use fbdev-dma
On Fri, Apr 12, 2024 at 09:57:27PM +0300, Laurent Pinchart wrote: > Hi Thomas, > > Thank you for the patch. > > On Wed, Apr 10, 2024 at 03:02:24PM +0200, Thomas Zimmermann wrote: > > Implement fbdev emulation with fbdev-dma. Fbdev-dma now supports > > damage handling, which is required by rcar-du. Avoids the overhead of > > fbdev-generic's additional shadow buffering. No functional changes. > > > > Signed-off-by: Thomas Zimmermann > > Cc: Laurent Pinchart > > Cc: Kieran Bingham > > Reviewed-by: Laurent Pinchart I meant Reviewed-by: Laurent Pinchart > On a side note, I noticed that drm_fbdev_generic_client_funcs and > drm_fbdev_dma_client_funcs point to functions that are identical. Would > there be a way to avoid the code duplication ? > > > --- > > drivers/gpu/drm/renesas/rcar-du/rcar_du_drv.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/renesas/rcar-du/rcar_du_drv.c > > b/drivers/gpu/drm/renesas/rcar-du/rcar_du_drv.c > > index dee530e4c8b27..fb719d9aff10d 100644 > > --- a/drivers/gpu/drm/renesas/rcar-du/rcar_du_drv.c > > +++ b/drivers/gpu/drm/renesas/rcar-du/rcar_du_drv.c > > @@ -20,7 +20,7 @@ > > > > #include > > #include > > -#include > > +#include > > #include > > #include > > #include > > @@ -716,7 +716,7 @@ static int rcar_du_probe(struct platform_device *pdev) > > > > drm_info(>ddev, "Device %s probed\n", dev_name(>dev)); > > > > - drm_fbdev_generic_setup(>ddev, 32); > > + drm_fbdev_dma_setup(>ddev, 32); > > > > return 0; > > -- Regards, Laurent Pinchart
Re: [PATCH v2 30/43] drm/renesas/shmobile: Use fbdev-dma
Hi Thomas, Thank you for the patch. On Wed, Apr 10, 2024 at 03:02:26PM +0200, Thomas Zimmermann wrote: > Implement fbdev emulation with fbdev-dma. Fbdev-dma now supports > damage handling, which is required by shmobile. Avoids the overhead of > fbdev-generic's additional shadow buffering. No functional changes. > > Signed-off-by: Thomas Zimmermann > Cc: Laurent Pinchart > Cc: Geert Uytterhoeven Reviewed-by: Laurent Pinchart > --- > drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c > b/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c > index e83c3e52251de..890cc2f6408d6 100644 > --- a/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c > +++ b/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c > @@ -19,7 +19,7 @@ > > #include > #include > -#include > +#include > #include > #include > #include > @@ -250,7 +250,7 @@ static int shmob_drm_probe(struct platform_device *pdev) > if (ret < 0) > goto err_modeset_cleanup; > > - drm_fbdev_generic_setup(ddev, 16); > + drm_fbdev_dma_setup(ddev, 16); > > return 0; > -- Regards, Laurent Pinchart
Re: [PATCH v2 28/43] drm/renesas/rcar-du: Use fbdev-dma
Hi Thomas, Thank you for the patch. On Wed, Apr 10, 2024 at 03:02:24PM +0200, Thomas Zimmermann wrote: > Implement fbdev emulation with fbdev-dma. Fbdev-dma now supports > damage handling, which is required by rcar-du. Avoids the overhead of > fbdev-generic's additional shadow buffering. No functional changes. > > Signed-off-by: Thomas Zimmermann > Cc: Laurent Pinchart > Cc: Kieran Bingham Reviewed-by: Laurent Pinchart On a side note, I noticed that drm_fbdev_generic_client_funcs and drm_fbdev_dma_client_funcs point to functions that are identical. Would there be a way to avoid the code duplication ? > --- > drivers/gpu/drm/renesas/rcar-du/rcar_du_drv.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/renesas/rcar-du/rcar_du_drv.c > b/drivers/gpu/drm/renesas/rcar-du/rcar_du_drv.c > index dee530e4c8b27..fb719d9aff10d 100644 > --- a/drivers/gpu/drm/renesas/rcar-du/rcar_du_drv.c > +++ b/drivers/gpu/drm/renesas/rcar-du/rcar_du_drv.c > @@ -20,7 +20,7 @@ > > #include > #include > -#include > +#include > #include > #include > #include > @@ -716,7 +716,7 @@ static int rcar_du_probe(struct platform_device *pdev) > > drm_info(>ddev, "Device %s probed\n", dev_name(>dev)); > > - drm_fbdev_generic_setup(>ddev, 32); > + drm_fbdev_dma_setup(>ddev, 32); > > return 0; > -- Regards, Laurent Pinchart
Re: [PATCH 20/21] drm/rcar-du: Allow build with COMPILE_TEST=y
Hi Ville, Thank you for the patch. On Mon, Apr 08, 2024 at 08:04:25PM +0300, Ville Syrjala wrote: > From: Ville Syrjälä > > Allow rcar-du to be built with COMPILE_TEST=y for greater > coverage. Builds fine on x86/x86_64 at least. > > Cc: Laurent Pinchart > Cc: Kieran Bingham > Cc: linux-renesas-...@vger.kernel.org > Signed-off-by: Ville Syrjälä > --- > drivers/gpu/drm/renesas/rcar-du/Kconfig | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/renesas/rcar-du/Kconfig > b/drivers/gpu/drm/renesas/rcar-du/Kconfig > index 2dc739db2ba3..df8b08b1e537 100644 > --- a/drivers/gpu/drm/renesas/rcar-du/Kconfig > +++ b/drivers/gpu/drm/renesas/rcar-du/Kconfig > @@ -2,7 +2,7 @@ > config DRM_RCAR_DU > tristate "DRM Support for R-Car Display Unit" > depends on DRM && OF > - depends on ARM || ARM64 > + depends on ARM || ARM64 || COMPILE_TEST I'll trust 0-day to tell us if this causes any issue on exotic (from the R-Car DU driver's point of view) platforms. Reviewed-by: Laurent Pinchart I expect you to push this patch, please let me know if you don't intend to. > depends on ARCH_RENESAS || COMPILE_TEST > select DRM_KMS_HELPER > select DRM_GEM_DMA_HELPER -- Regards, Laurent Pinchart
Re: 2024 X.Org Foundation Membership deadline for voting in the election
Hi Pekka, On Tue, Apr 02, 2024 at 10:46:08AM +0300, Pekka Paalanen wrote: > On Tue, 26 Mar 2024 11:42:48 -0400 Christopher Michael wrote: > > > The 2024 X.Org Foundation membership renewal period has been extended > > one additional week and elections will start the following week on 01 > > April 2024. > > > > Please note that only current members can vote in the upcoming election, > > and that the deadline for new memberships or renewals to vote in the > > upcoming election is 01 April 2024 at 23:59 UTC. > > > > If you are interested in joining the X.Org Foundation or in renewing > > your membership, please visit the membership system site at: > > https://members.x.org/ > > > > Christopher Michael, on behalf of the X.Org elections committee > > Hi everyone, > > given that the year's first email reminding everyone to renew their > memberships was sent on Feb 7 when the renewal was NOT open yet, I > wonder how many people thought they had already renewed and are now > thinking they don't need to do anything? > > I fell for that: On Feb 7, I went to members.x.org to check my status, > it said I was registered for "2023-2024" and there was no button to > renew, so I closed the page confident that I was a member for 2024. > After all, it said 2024. This was a mistake I realised only after being > personally poked to renew. I know for sure of one other person falling > for the same. Make that two. Thanks for the notice. > Now, the members page for this year says "Application for the period: > 02/2024-02/2025". Thanks to the people adding the month to reduce > confusion. -- Regards, Laurent Pinchart
Re: [PATCH] MAINTAINERS: Add myself as maintainer for Xilinx DRM drivers
Hi Tomi, Thank you for the patch. On Wed, Mar 27, 2024 at 03:03:33PM +0200, Tomi Valkeinen wrote: > Add myself as a co-maintainer for Xilinx DRM drivers to help Laurent. > > Signed-off-by: Tomi Valkeinen Much appreciated :-) Reviewed-by: Laurent Pinchart > --- > MAINTAINERS | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/MAINTAINERS b/MAINTAINERS > index 1aabf1c15bb3..79ef5a6bf21b 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -7306,6 +7306,7 @@ F: drivers/gpu/drm/xen/ > > DRM DRIVERS FOR XILINX > M: Laurent Pinchart > +M: Tomi Valkeinen > L: dri-devel@lists.freedesktop.org > S: Maintained > T: git git://anongit.freedesktop.org/drm/drm-misc > > --- > base-commit: e8f897f4afef0031fe618a8e94127a0934896aba > change-id: 20240327-xilinx-maintainer-f6020f6cba4d -- Regards, Laurent Pinchart
Re: [PATCH 1/9] fbdev: shmobile: fix snprintf truncation
Hi Arnd,
Thank you for the patch.
On Tue, Mar 26, 2024 at 11:38:00PM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann
>
> The name of the overlay does not fit into the fixed-length field:
>
> drivers/video/fbdev/sh_mobile_lcdcfb.c:1577:2: error: 'snprintf' will always
> be truncated; specified size is 16, but format string expands to at least 25
>
> Make it short enough by changing the string.
>
> Fixes: c5deac3c9b22 ("fbdev: sh_mobile_lcdc: Implement overlays support")
> Signed-off-by: Arnd Bergmann
Reviewed-by: Laurent Pinchart
> ---
> drivers/video/fbdev/sh_mobile_lcdcfb.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/video/fbdev/sh_mobile_lcdcfb.c
> b/drivers/video/fbdev/sh_mobile_lcdcfb.c
> index eb2297b37504..d35d2cf8 100644
> --- a/drivers/video/fbdev/sh_mobile_lcdcfb.c
> +++ b/drivers/video/fbdev/sh_mobile_lcdcfb.c
> @@ -1575,7 +1575,7 @@ sh_mobile_lcdc_overlay_fb_init(struct
> sh_mobile_lcdc_overlay *ovl)
>*/
> info->fix = sh_mobile_lcdc_overlay_fix;
> snprintf(info->fix.id, sizeof(info->fix.id),
> - "SH Mobile LCDC Overlay %u", ovl->index);
> + "SHMobile ovl %u", ovl->index);
> info->fix.smem_start = ovl->dma_handle;
> info->fix.smem_len = ovl->fb_size;
> info->fix.line_length = ovl->pitch;
--
Regards,
Laurent Pinchart
Re: [PATCH V8 00/12] soc: imx8mp: Add support for HDMI
Hi Tommaso, On Mon, Mar 25, 2024 at 10:48:56PM +0100, Tommaso Merciai wrote: > Hi Adam, Lucas, > Thanks for this series. > > This series make HDMI work on evk. > All is working properly on my side. > > Tested on: Linux imx8mp-lpddr4-evk 6.9.0-rc1. > Hope this help. > > Tested-by: Tommaso Merciai The DRM side has been merged already. The only missing patches are for the PHY, and the latest version can be found in https://lore.kernel.org/linux-phy/20240227220444.77566-1-aford...@gmail.com/. You can test that series and send a Tested-by tag. I'm crossing my fingers and hoping it will be merged in v6.10. > On Sat, Feb 03, 2024 at 10:52:40AM -0600, Adam Ford wrote: > > The i.MX8M Plus has an HDMI controller, but it depends on two > > other systems, the Parallel Video Interface (PVI) and the > > HDMI PHY from Samsung. The LCDIF controller generates the display > > and routes it to the PVI which converts passes the parallel video > > to the HDMI bridge. The HDMI system has a corresponding power > > domain controller whose driver was partially written, but the > > device tree for it was never applied, so some changes to the > > power domain should be harmless because they've not really been > > used yet. > > > > This series is adapted from multiple series from Lucas Stach with > > edits and suggestions from feedback from various series, but it > > since it's difficult to use and test them independently, > > I merged them into on unified series. The version history is a > > bit ambiguous since different components were submitted at different > > times and had different amount of retries. In an effort to merge them > > I used the highest version attempt. > > > > Adam Ford (3): > > dt-bindings: soc: imx: add missing clock and power-domains to > > imx8mp-hdmi-blk-ctrl > > pmdomain: imx8mp-blk-ctrl: imx8mp_blk: Add fdcc clock to hdmimix > > domain > > arm64: defconfig: Enable DRM_IMX8MP_DW_HDMI_BRIDGE as module > > > > Lucas Stach (9): > > dt-bindings: phy: add binding for the i.MX8MP HDMI PHY > > phy: freescale: add Samsung HDMI PHY > > arm64: dts: imx8mp: add HDMI power-domains > > arm64: dts: imx8mp: add HDMI irqsteer > > dt-bindings: display: imx: add binding for i.MX8MP HDMI PVI > > drm/bridge: imx: add driver for HDMI TX Parallel Video Interface > > dt-bindings: display: imx: add binding for i.MX8MP HDMI TX > > drm/bridge: imx: add bridge wrapper driver for i.MX8MP DWC HDMI > > arm64: dts: imx8mp: add HDMI display pipeline > > > > .../display/bridge/fsl,imx8mp-hdmi-tx.yaml| 102 ++ > > .../display/imx/fsl,imx8mp-hdmi-pvi.yaml | 84 ++ > > .../bindings/phy/fsl,imx8mp-hdmi-phy.yaml | 62 + > > .../soc/imx/fsl,imx8mp-hdmi-blk-ctrl.yaml | 22 +- > > arch/arm64/boot/dts/freescale/imx8mp.dtsi | 145 +++ > > arch/arm64/configs/defconfig |1 + > > drivers/gpu/drm/bridge/imx/Kconfig| 18 + > > drivers/gpu/drm/bridge/imx/Makefile |2 + > > drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pvi.c | 207 > > drivers/gpu/drm/bridge/imx/imx8mp-hdmi-tx.c | 154 +++ > > drivers/phy/freescale/Kconfig |6 + > > drivers/phy/freescale/Makefile|1 + > > drivers/phy/freescale/phy-fsl-samsung-hdmi.c | 1075 + > > drivers/pmdomain/imx/imx8mp-blk-ctrl.c| 10 +- > > 14 files changed, 1876 insertions(+), 13 deletions(-) > > create mode 100644 > > Documentation/devicetree/bindings/display/bridge/fsl,imx8mp-hdmi-tx.yaml > > create mode 100644 > > Documentation/devicetree/bindings/display/imx/fsl,imx8mp-hdmi-pvi.yaml > > create mode 100644 > > Documentation/devicetree/bindings/phy/fsl,imx8mp-hdmi-phy.yaml > > create mode 100644 drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pvi.c > > create mode 100644 drivers/gpu/drm/bridge/imx/imx8mp-hdmi-tx.c > > create mode 100644 drivers/phy/freescale/phy-fsl-samsung-hdmi.c -- Regards, Laurent Pinchart
Re: [PATCH v2 5/8] drm: xlnx: zynqmp_dpsub: Set input live format
DRM_FORMAT_RGB888,
> + }, {
> + .bus_fmt = MEDIA_BUS_FMT_UYVY8_1X16,
> + .drm_fmt = DRM_FORMAT_YUV422,
> + }, {
> + .bus_fmt = MEDIA_BUS_FMT_VUY8_1X24,
> + .drm_fmt = DRM_FORMAT_YUV444,
> + }, {
> + .bus_fmt = MEDIA_BUS_FMT_UYVY10_1X20,
> + .drm_fmt = DRM_FORMAT_P210,
> + },
H... Looking at this, I think you should have both bus_fmt and
drm_fmt in zynqmp_disp_format. It seems it would simplify the code flow
and make things more readable. If you would like me to give it a try,
please let me know.
> + };
> + unsigned int i;
> +
> + for (i = 0; i < ARRAY_SIZE(format_map); ++i)
> + if (format_map[i].bus_fmt == bus_format)
> + return format_map[i].drm_fmt;
> +
> + return DRM_FORMAT_INVALID;
> +}
> +
> /**
> * zynqmp_disp_layer_set_format - Set the layer format
> * @layer: The layer
> - * @info: The format info
> + * @drm_or_bus_format: DRM or media bus format
> *
> * Set the format for @layer to @info. The layer must be disabled.
> */
> void zynqmp_disp_layer_set_format(struct zynqmp_disp_layer *layer,
> - const struct drm_format_info *info)
> + u32 drm_or_bus_format)
> {
> unsigned int i;
>
> - layer->disp_fmt = zynqmp_disp_layer_find_format(layer, info->format);
> - layer->drm_fmt = info;
> -
> + layer->disp_fmt = zynqmp_disp_layer_find_format(layer,
> drm_or_bus_format);
> zynqmp_disp_avbuf_set_format(layer->disp, layer, layer->disp_fmt);
>
> + if (layer->mode == ZYNQMP_DPSUB_LAYER_LIVE)
> + drm_or_bus_format =
> zynqmp_disp_reference_drm_format(drm_or_bus_format);
> +
> + layer->drm_fmt = drm_format_info(drm_or_bus_format);
> + if (!layer->drm_fmt)
> + return;
> +
> if (layer->mode == ZYNQMP_DPSUB_LAYER_LIVE)
> return;
>
> @@ -1008,7 +1067,7 @@ void zynqmp_disp_layer_set_format(struct
> zynqmp_disp_layer *layer,
>* Set pconfig for each DMA channel to indicate they're part of a
>* video group.
>*/
> - for (i = 0; i < info->num_planes; i++) {
> + for (i = 0; i < layer->drm_fmt->num_planes; i++) {
> struct zynqmp_disp_layer_dma *dma = >dmas[i];
> struct xilinx_dpdma_peripheral_config pconfig = {
> .video_group = true,
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.h
> b/drivers/gpu/drm/xlnx/zynqmp_disp.h
> index 88c285a12e23..9f9a5f50ffbc 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_disp.h
> +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.h
> @@ -55,7 +55,7 @@ u32 *zynqmp_disp_layer_formats(struct zynqmp_disp_layer
> *layer,
> void zynqmp_disp_layer_enable(struct zynqmp_disp_layer *layer);
> void zynqmp_disp_layer_disable(struct zynqmp_disp_layer *layer);
> void zynqmp_disp_layer_set_format(struct zynqmp_disp_layer *layer,
> - const struct drm_format_info *info);
> + u32 drm_or_bus_format);
> int zynqmp_disp_layer_update(struct zynqmp_disp_layer *layer,
>struct drm_plane_state *state);
>
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c
> b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> index a0d169ac48c0..fc6b1d783c28 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> @@ -1281,7 +1281,8 @@ static void zynqmp_dp_disp_enable(struct zynqmp_dp *dp,
> {
> enum zynqmp_dpsub_layer_id layer_id;
> struct zynqmp_disp_layer *layer;
> - const struct drm_format_info *info;
> + struct drm_bridge_state *bridge_state;
> + u32 bus_fmt;
>
> if (dp->dpsub->connected_ports & BIT(ZYNQMP_DPSUB_PORT_LIVE_VIDEO))
> layer_id = ZYNQMP_DPSUB_LAYER_VID;
> @@ -1291,10 +1292,14 @@ static void zynqmp_dp_disp_enable(struct zynqmp_dp
> *dp,
> return;
>
> layer = dp->dpsub->layers[layer_id];
> + bridge_state =
> drm_atomic_get_new_bridge_state(old_bridge_state->base.state,
> +
> old_bridge_state->bridge);
> + if (WARN_ON(!bridge_state))
> + return;
> +
> + bus_fmt = bridge_state->input_bus_cfg.format;
> + zynqmp_disp_layer_set_format(layer, bus_fmt);
>
> - /* TODO: Make the format configurable. */
> - info = drm_format_info(DRM_FORMAT_YUV422);
> - zynqmp_disp_layer_set_format(layer, info);
> zynqmp_disp_layer_enable(layer);
>
> if (layer_id == ZYNQMP_DPSUB_LAYER_GFX)
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_kms.c
> b/drivers/gpu/drm/xlnx/zynqmp_kms.c
> index bf9fba01df0e..d96b3f3f2e3a 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_kms.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_kms.c
> @@ -111,7 +111,7 @@ static void zynqmp_dpsub_plane_atomic_update(struct
> drm_plane *plane,
> if (old_state->fb)
> zynqmp_disp_layer_disable(layer);
>
> - zynqmp_disp_layer_set_format(layer, new_state->fb->format);
> + zynqmp_disp_layer_set_format(layer,
> new_state->fb->format->format);
> }
>
> zynqmp_disp_layer_update(layer, new_state);
>
--
Regards,
Laurent Pinchart
Re: [PATCH v2 4/8] drm: xlnx: zynqmp_dpsub: Minimize usage of global flag
Hi Anatoliy,
Thank you for the patch.
On Tue, Mar 12, 2024 at 05:55:01PM -0700, Anatoliy Klymenko wrote:
> Avoid usage of global zynqmp_dpsub.dma_enabled flag in DPSUB layer
> context. This flag signals whether the driver runs in DRM CRTC or DRM
> bridge mode, assuming that all display layers share the same live or
> non-live mode of operation. Using per-layer mode instead of global flag
> will siplify future support of the hybrid scenario.
s/siplify/simplify/
> Signed-off-by: Anatoliy Klymenko
> ---
> drivers/gpu/drm/xlnx/zynqmp_disp.c | 11 ---
> 1 file changed, 4 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c
> b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> index af851190f447..dd48fa60fa9a 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> @@ -975,7 +975,7 @@ void zynqmp_disp_layer_disable(struct zynqmp_disp_layer
> *layer)
> {
> unsigned int i;
>
> - if (layer->disp->dpsub->dma_enabled) {
> + if (layer->mode == ZYNQMP_DPSUB_LAYER_NONLIVE) {
> for (i = 0; i < layer->drm_fmt->num_planes; i++)
> dmaengine_terminate_sync(layer->dmas[i].chan);
> }
> @@ -1001,7 +1001,7 @@ void zynqmp_disp_layer_set_format(struct
> zynqmp_disp_layer *layer,
>
> zynqmp_disp_avbuf_set_format(layer->disp, layer, layer->disp_fmt);
>
> - if (!layer->disp->dpsub->dma_enabled)
> + if (layer->mode == ZYNQMP_DPSUB_LAYER_LIVE)
> return;
>
> /*
> @@ -1039,7 +1039,7 @@ int zynqmp_disp_layer_update(struct zynqmp_disp_layer
> *layer,
> const struct drm_format_info *info = layer->drm_fmt;
> unsigned int i;
>
> - if (!layer->disp->dpsub->dma_enabled)
> + if (layer->mode == ZYNQMP_DPSUB_LAYER_LIVE)
> return 0;
The above changes look nice.
>
> for (i = 0; i < info->num_planes; i++) {
> @@ -1089,7 +1089,7 @@ static void zynqmp_disp_layer_release_dma(struct
> zynqmp_disp *disp,
> {
> unsigned int i;
>
> - if (!layer->info || !disp->dpsub->dma_enabled)
> + if (!layer->info)
This, however, doesn't seem right, as this function is called
unconditionally from the remove path. The change below seems weird too.
If I'm missing something, it should at least be explained in the commit
message.
> return;
>
> for (i = 0; i < layer->info->num_channels; i++) {
> @@ -1132,9 +1132,6 @@ static int zynqmp_disp_layer_request_dma(struct
> zynqmp_disp *disp,
> unsigned int i;
> int ret;
>
> - if (!disp->dpsub->dma_enabled)
> - return 0;
> -
> for (i = 0; i < layer->info->num_channels; i++) {
> struct zynqmp_disp_layer_dma *dma = >dmas[i];
> char dma_channel_name[16];
>
--
Regards,
Laurent Pinchart
Re: [PATCH v2 3/8] drm: xlnx: zynqmp_dpsub: Anounce supported input formats
mats(struct zynqmp_disp_layer *layer,
> -unsigned int *num_formats)
> +u32 *zynqmp_disp_layer_formats(struct zynqmp_disp_layer *layer,
> +unsigned int *num_formats)
> {
> unsigned int i;
> u32 *formats;
> @@ -1131,6 +1173,11 @@ static int zynqmp_disp_create_layers(struct
> zynqmp_disp *disp)
> .num_channels = 1,
> },
> };
> + static const struct zynqmp_disp_layer_info live_layer_info = {
> + .formats = avbuf_live_fmts,
> + .num_formats = ARRAY_SIZE(avbuf_live_fmts),
> + .num_channels = 0,
> + };
>
> unsigned int i;
> int ret;
> @@ -1140,12 +1187,16 @@ static int zynqmp_disp_create_layers(struct
> zynqmp_disp *disp)
>
> layer->id = i;
> layer->disp = disp;
> - layer->info = _info[i];
> /* For now assume dpsub works in either live or non-live mode
> for both layers.
While are it, could you please turn this into
/*
* For now assume dpsub works in either live or non-live mode
for both layers.
with a blank line just above it ?
>* Hybrid mode is not supported yet.
>*/
> - layer->mode = disp->dpsub->dma_enabled ?
> ZYNQMP_DPSUB_LAYER_NONLIVE
> -:
> ZYNQMP_DPSUB_LAYER_LIVE;
> + if (disp->dpsub->dma_enabled) {
> + layer->mode = ZYNQMP_DPSUB_LAYER_NONLIVE;
> + layer->info = _info[i];
> + } else {
> + layer->mode = ZYNQMP_DPSUB_LAYER_LIVE;
> + layer->info = _layer_info;
> + }
>
> ret = zynqmp_disp_layer_request_dma(disp, layer);
> if (ret)
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.h
> b/drivers/gpu/drm/xlnx/zynqmp_disp.h
> index 9b8b202224d9..88c285a12e23 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_disp.h
> +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.h
> @@ -50,8 +50,8 @@ int zynqmp_disp_setup_clock(struct zynqmp_disp *disp,
> void zynqmp_disp_blend_set_global_alpha(struct zynqmp_disp *disp,
> bool enable, u32 alpha);
>
> -u32 *zynqmp_disp_layer_drm_formats(struct zynqmp_disp_layer *layer,
> -unsigned int *num_formats);
> +u32 *zynqmp_disp_layer_formats(struct zynqmp_disp_layer *layer,
> +unsigned int *num_formats);
> void zynqmp_disp_layer_enable(struct zynqmp_disp_layer *layer);
> void zynqmp_disp_layer_disable(struct zynqmp_disp_layer *layer);
> void zynqmp_disp_layer_set_format(struct zynqmp_disp_layer *layer,
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c
> b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> index 04b6bcac3b07..a0d169ac48c0 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> @@ -1568,6 +1568,31 @@ static const struct drm_edid
> *zynqmp_dp_bridge_edid_read(struct drm_bridge *brid
> return drm_edid_read_ddc(connector, >aux.ddc);
> }
>
> +static u32 *
> +zynqmp_dp_bridge_get_input_bus_fmts(struct drm_bridge *bridge,
> + struct drm_bridge_state *bridge_state,
> + struct drm_crtc_state *crtc_state,
> + struct drm_connector_state *conn_state,
> + u32 output_fmt,
> + unsigned int *num_input_fmts)
> +{
> + struct zynqmp_dp *dp = bridge_to_dp(bridge);
> + struct zynqmp_disp_layer *layer;
> + enum zynqmp_dpsub_layer_id layer_id;
> +
> + if (dp->dpsub->connected_ports & BIT(ZYNQMP_DPSUB_PORT_LIVE_VIDEO))
> + layer_id = ZYNQMP_DPSUB_LAYER_VID;
> + else if (dp->dpsub->connected_ports & BIT(ZYNQMP_DPSUB_PORT_LIVE_GFX))
> + layer_id = ZYNQMP_DPSUB_LAYER_GFX;
> + else {
> + *num_input_fmts = 0;
> + return NULL;
> + }
You need curly braces around all branches if one of them has multiple
statements.
Given that the above pattern is repeated twice already, a helper
function that returns the layer pointer would be useful. Then you could
simply write
layer = ...(dp);
if (!layer) {
*num_input_fmts = 0;
return NULL;
}
With these small issues addressed,
Reviewed-by: Laurent Pinchart
> + layer = dp->dpsub->layers[layer_id];
> +
> + return zynqmp_disp_layer_formats(layer, num_input_fmts);
> +}
> +
> static const struct drm_bridge_funcs zynqmp_dp_bridge_fun
Re: [PATCH v2 2/8] drm: xlnx: zynqmp_dpsub: Update live format defines
Hi Anatoliy, Thank you for the patch. On Tue, Mar 12, 2024 at 05:54:59PM -0700, Anatoliy Klymenko wrote: > Update live format defines to match DPSUB AV_BUF_LIVE_VID_CONFIG register > layout. > > Signed-off-by: Anatoliy Klymenko Reviewed-by: Laurent Pinchart > --- > drivers/gpu/drm/xlnx/zynqmp_disp_regs.h | 8 > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h > b/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h > index f92a006d5070..fa3935384834 100644 > --- a/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h > +++ b/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h > @@ -165,10 +165,10 @@ > #define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_100x2 > #define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_120x3 > #define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_MASK GENMASK(2, 0) > -#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_RGB 0x0 > -#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV4440x1 > -#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV4220x2 > -#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YONLY 0x3 > +#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_RGB (0x0 << 4) > +#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV444(0x1 << 4) > +#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV422(0x2 << 4) > +#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YONLY (0x3 << 4) > #define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_MASK GENMASK(5, 4) > #define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_CB_FIRST BIT(8) > #define ZYNQMP_DISP_AV_BUF_PALETTE_MEMORY0x400 > -- Regards, Laurent Pinchart
Re: [PATCH] drm: bridge: thc63lvd1024: Print error message when DT parsing fails
Hi Sui,
On Tue, Mar 19, 2024 at 12:42:41AM +0800, Sui Jingfeng wrote:
> On 2024/3/19 00:06, Laurent Pinchart wrote:
> > Commit 00084f0c01bf ("drm: bridge: thc63lvd1024: Switch to use
> > of_graph_get_remote_node()") simplified the thc63lvd1024 driver by
> > replacing hand-rolled code with a helper function.
>
> [...]
>
> > While doing so, it
> > created an error code path at probe time without any error message,
>
> If this is a reason or a concern, then every drm bridges drivers will suffer
> from
> such a concern. Right?
Yes, bridge drivers (or any driver, really) should avoid failing probe
silently.
> > potentially causing probe issues that get annoying to debug.
>
> Sorry, let's keep it fair enough, it creates nothing annoyed.
>
> If there is a probe issues, then, it is caused by ill-behavioral DT.
> *NOT* my patch. And should be found during review stage.
Even before the review stage, in the DT development stage. My point is
that creating a silent failure path in probe will make it more difficult
for DT developers to debug issues.
> If the of_graph_get_remote_node() function is not good enough,
> I suggest to improve the of_graph_get_remote_node() function,
> then all callers of it will benefits.
>
> Well, the strong word here just terrifying new programmers to call
> core function helpers. Please use more *soft* description in the
> commit message.
Could you please propose a wording that you would consider more soft ?
> > Fix it by
> > adding an error message.
> >
> > Fixes: 00084f0c01bf ("drm: bridge: thc63lvd1024: Switch to use
> > of_graph_get_remote_node()")
>
> Please drop the fixes tag at here, append the tag to a real bug-fix patch
> will make more sense imo.
> I suggest to improve the of_graph_get_remote_node() function, then all
> callers of it will benefits.
> NOT a single implement like this.
Improving core helpers is certainly a good idea, and if we do so, we can
simplify drivers. What I'm concerned is that commit 00084f0c01bf creates
a silent probe failure path, which didn't exist before it. This is why
this patch references it in the Fixes: tag, making sure that this patch
will get backported to any stable kernel that includes commit
00084f0c01bf. As far as I understand, this is business as usual. There's
nothing personal here, and no judgement on the quality of your code.
> > Signed-off-by: Laurent Pinchart
> > ---
> > drivers/gpu/drm/bridge/thc63lvd1024.c | 5 -
> > 1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/thc63lvd1024.c
> > b/drivers/gpu/drm/bridge/thc63lvd1024.c
> > index 5f99f9724081..674efc489e3a 100644
> > --- a/drivers/gpu/drm/bridge/thc63lvd1024.c
> > +++ b/drivers/gpu/drm/bridge/thc63lvd1024.c
> > @@ -125,8 +125,11 @@ static int thc63_parse_dt(struct thc63_dev *thc63)
> >
> > remote = of_graph_get_remote_node(thc63->dev->of_node,
> > THC63_RGB_OUT0, -1);
> > - if (!remote)
> > + if (!remote) {
> > + dev_err(thc63->dev, "No remote endpoint for port@%u\n",
> > + THC63_RGB_OUT0);
> > return -ENODEV;
> > + }
> >
> > thc63->next = of_drm_find_bridge(remote);
> > of_node_put(remote);
> >
> > base-commit: 00084f0c01bf3a2591d007010b196e048281c455
--
Regards,
Laurent Pinchart
Re: [PATCH 3/6] drm: zynqmp_dp: Add locking
Hi Sean,
On Mon, Mar 18, 2024 at 01:29:12PM -0400, Sean Anderson wrote:
> On 3/18/24 13:16, Laurent Pinchart wrote:
> > On Fri, Mar 15, 2024 at 07:09:13PM -0400, Sean Anderson wrote:
> >> Add some locking, since none is provided by the drm subsystem. This will
> >
> > That's not quite right, the DRM core doesn't call bridge operations
> > concurrently.
>
> I figured something like this was going on.
>
> > We may need locking to protect against race conditions
> > between bridge operations and interrupts though.
>
> And of course this will only get worse once we let userspace get involved.
>
> >> prevent the IRQ/workers/bridge API calls from stepping on each other's
> >> toes.
> >>
> >> Signed-off-by: Sean Anderson
> >> ---
> >>
> >> drivers/gpu/drm/xlnx/zynqmp_dp.c | 59 +++-
> >> 1 file changed, 42 insertions(+), 17 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c
> >> b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> >> index 8635b5673386..d2dee58e7bf2 100644
> >> --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
> >> +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> >> @@ -279,6 +279,7 @@ struct zynqmp_dp_config {
> >> * @dpsub: Display subsystem
> >> * @iomem: device I/O memory for register access
> >> * @reset: reset controller
> >> + * @lock: Mutex protecting this struct and register access (but not AUX)
> >
> > This patch does two things at once, it defers link training from the IRQ
> > handler to a work queue, and covers everything with a big lock. The
> > scope is too large.
>
> OK, I can split this.
>
> > Please restrict the lock scope and document the
> > individual fields that need to be protected, and explain the locking
> > design in the commit message (or comments in the code).
>
> As said, this lock protects
>
> - Non-atomic registers configuring the link. That is, everything but the IRQ
> registers (since these are accessed in an atomic fashion), and the DP AUX
> registers (since these don't affect the link).
> - Link configuration. This is effectively everything in zynqmp_dp which isn't
> read-only after probe time. So from next_bridge onward.
>
> It's designed to protect configuration changes so we don't have to do anything
> tricky. Configuration should never be in the hot path, so I'm not worried
> about
> performance.
If userspace can control all this directly through debugfs, can you
guarantee that locks will be enough ? The driver doesn't expect direct
userspace access. I have a feeling this is really quite hacky.
> >> * @irq: irq
> >> * @bridge: DRM bridge for the DP encoder
> >> * @next_bridge: The downstream bridge
> >> @@ -299,6 +300,7 @@ struct zynqmp_dp {
> >>struct zynqmp_dpsub *dpsub;
> >>void __iomem *iomem;
> >>struct reset_control *reset;
> >> + struct mutex lock;
> >>int irq;
> >>
> >>struct drm_bridge bridge;
> >> @@ -308,7 +310,7 @@ struct zynqmp_dp {
> >>struct drm_dp_aux aux;
> >>struct phy *phy[ZYNQMP_DP_MAX_LANES];
> >>u8 num_lanes;
> >> - struct delayed_work hpd_work;
> >> + struct delayed_work hpd_work, hpd_irq_work;
> >
> > One variable per line please.
>
> OK
>
> >>enum drm_connector_status status;
> >>bool enabled;
> >>
> >> @@ -1371,8 +1373,10 @@ zynqmp_dp_bridge_mode_valid(struct drm_bridge
> >> *bridge,
> >>}
> >>
> >>/* Check with link rate and lane count */
> >> + mutex_lock(>lock);
> >>rate = zynqmp_dp_max_rate(dp->link_config.max_rate,
> >> dp->link_config.max_lanes, dp->config.bpp);
> >> + mutex_unlock(>lock);
> >>if (mode->clock > rate) {
> >>dev_dbg(dp->dev, "filtered mode %s for high pixel rate\n",
> >>mode->name);
> >> @@ -1399,6 +1403,7 @@ static void zynqmp_dp_bridge_atomic_enable(struct
> >> drm_bridge *bridge,
> >>
> >>pm_runtime_get_sync(dp->dev);
> >>
> >> + mutex_lock(>lock);
> >>zynqmp_dp_disp_enable(dp, old_bridge_state);
> >>
> >>/*
> >> @@ -1459,6 +1464,7 @@ static void zynqmp_dp_bridge_atomic_enable(struct
> >> drm_bridge *bridge,
> >>zynqmp_dp_write(dp, ZYNQMP_DP_SOFTWARE_RESET,
> >>ZYNQMP_DP_SOFTWARE_RESET_ALL);
> >>
Re: [PATCH 6/6] drm: zynqmp_dp: Add debugfs interface for compliance testing
fops_zynqmp_dp_preemphasis,
> > zynqmp_dp_preemphasis_get,
> > +zynqmp_dp_preemphasis_set, "%llu\n");
> > +
> > +static int zynqmp_dp_lanes_get(void *data, u64 *val)
> > +{
> > + struct zynqmp_dp *dp = data;
> > +
> > + mutex_lock(>lock);
> > + *val = dp->test.link_cnt;
> > + mutex_unlock(>lock);
> > + return 0;
> > +}
> > +
> > +static int zynqmp_dp_lanes_set(void *data, u64 val)
> > +{
> > + struct zynqmp_dp *dp = data;
> > + int ret = 0;
> > +
> > + if (val > ZYNQMP_DP_MAX_LANES)
> > + return -EINVAL;
> > +
> > + mutex_lock(>lock);
> > + if (val > dp->num_lanes) {
> > + ret = -EINVAL;
> > + } else {
> > + dp->test.link_cnt = val;
> > + if (dp->test.active)
> > + ret = zynqmp_dp_test_setup(dp);
> > + }
> > + mutex_unlock(>lock);
> > +
> > + return ret;
> > +}
> > +
> > +DEFINE_DEBUGFS_ATTRIBUTE(fops_zynqmp_dp_lanes, zynqmp_dp_lanes_get,
> > +zynqmp_dp_lanes_set, "%llu\n");
> > +
> > +static int zynqmp_dp_rate_get(void *data, u64 *val)
> > +{
> > + struct zynqmp_dp *dp = data;
> > +
> > + mutex_lock(>lock);
> > + *val = drm_dp_bw_code_to_link_rate(dp->test.bw_code) * 1;
> > + mutex_unlock(>lock);
> > + return 0;
> > +}
> > +
> > +static int zynqmp_dp_rate_set(void *data, u64 val)
> > +{
> > + struct zynqmp_dp *dp = data;
> > + u8 bw_code = drm_dp_link_rate_to_bw_code(val / 1);
> > + int link_rate = drm_dp_bw_code_to_link_rate(bw_code);
> > + int ret = 0;
> > +
> > + if (val / 1 != link_rate)
> > + return -EINVAL;
> > +
> > + if (bw_code != DP_LINK_BW_1_62 && bw_code != DP_LINK_BW_2_7 &&
> > + bw_code != DP_LINK_BW_5_4)
> > + return -EINVAL;
> > +
> > + mutex_lock(>lock);
> > + dp->test.bw_code = bw_code;
> > + if (dp->test.active)
> > + ret = zynqmp_dp_test_setup(dp);
> > + mutex_unlock(>lock);
> > +
> > + return ret;
> > +}
> > +
> > +DEFINE_DEBUGFS_ATTRIBUTE(fops_zynqmp_dp_rate, zynqmp_dp_rate_get,
> > +zynqmp_dp_rate_set, "%llu\n");
> > +
> > +static void zynqmp_dp_bridge_debugfs_init(struct drm_bridge *bridge,
> > + struct dentry *root)
> > +{
> > + struct zynqmp_dp *dp = bridge_to_dp(bridge);
> > + struct dentry *test;
> > + int i;
> > +
> > + dp->test.bw_code = DP_LINK_BW_5_4;
> > + dp->test.link_cnt = dp->num_lanes;
> > +
> > + test = debugfs_create_dir("test", root);
> > +#define CREATE_FILE(name) \
> > + debugfs_create_file(#name, 0600, test, dp, _zynqmp_dp_##name)
> > + CREATE_FILE(pattern);
> > + CREATE_FILE(enhanced);
> > + CREATE_FILE(downspread);
> > + CREATE_FILE(active);
> > + CREATE_FILE(custom);
> > + CREATE_FILE(rate);
> > + CREATE_FILE(lanes);
> > +
> > + for (i = 0; i < dp->num_lanes; i++) {
> > + static const char fmt[] = "lane%d_preemphasis";
> > + char name[sizeof(fmt)];
> > +
> > + dp->debugfs_train_set[i].dp = dp;
> > + dp->debugfs_train_set[i].lane = i;
> > +
> > + sprintf(name, fmt, i);
> > + debugfs_create_file(name, 0600, test,
> > + >debugfs_train_set[i],
> > + _zynqmp_dp_preemphasis);
> > +
> > + sprintf(name, "lane%d_swing", i);
> > + debugfs_create_file(name, 0600, test,
> > + >debugfs_train_set[i],
> > + _zynqmp_dp_swing);
> > + }
> > +}
> > +
> > static const struct drm_bridge_funcs zynqmp_dp_bridge_funcs = {
> > .attach = zynqmp_dp_bridge_attach,
> > .detach = zynqmp_dp_bridge_detach,
> > @@ -1611,6 +2189,7 @@ static const struct drm_bridge_funcs
> > zynqmp_dp_bridge_funcs = {
> > .atomic_check = zynqmp_dp_bridge_atomic_check,
> > .detect = zynqmp_dp_bridge_detect,
> > .get_edid = zynqmp_dp_bridge_get_edid,
> > + .debugfs_init = zynqmp_dp_bridge_debugfs_init,
> > };
> >
> > /*
> > -
> > @@ -1645,6 +2224,9 @@ static void zynqmp_dp_hpd_work_func(struct
> > work_struct *work)
> > hpd_work.work);
> > enum drm_connector_status status;
> >
> > + if (dp->test.active)
> > + return;
> > +
> > status = zynqmp_dp_bridge_detect(>bridge);
> > drm_bridge_hpd_notify(>bridge, status);
> > }
> > @@ -1666,7 +2248,14 @@ static void zynqmp_dp_hpd_irq_work_func(struct
> > work_struct *work)
> > if (status[4] & DP_LINK_STATUS_UPDATED ||
> > !drm_dp_clock_recovery_ok([2],
> > dp->mode.lane_cnt) ||
> > !drm_dp_channel_eq_ok([2], dp->mode.lane_cnt)) {
> > - zynqmp_dp_train_loop(dp);
> > + if (dp->test.active) {
> > + dev_dbg(dp->dev, "Ignoring HPD IRQ in test
> > mode\n");
> > + } else {
> > + dev_dbg(dp->dev,
> > + "Retraining due to HPD IRQ (status
> > is [%*ph])\n",
> > + (int)sizeof(status), status);
> > + zynqmp_dp_train_loop(dp);
> > + }
> > }
> > }
> > mutex_unlock(>lock);
--
Regards,
Laurent Pinchart
Re: [PATCH 6/6] drm: zynqmp_dp: Add debugfs interface for compliance testing
On Mon, Mar 18, 2024 at 11:06:40AM -0400, Sean Anderson wrote:
> On 3/16/24 06:14, kernel test robot wrote:
> > Hi Sean,
> >
> > kernel test robot noticed the following build warnings:
> >
> > [auto build test WARNING on v6.8]
> > [cannot apply to drm-misc/drm-misc-next linus/master next-20240315]
> > [If your patch is applied to the wrong git tree, kindly drop us a note.
> > And when submitting patch, we suggest to use '--base' as documented in
> > https://git-scm.com/docs/git-format-patch#_base_tree_information]
> >
> > url:
> > https://github.com/intel-lab-lkp/linux/commits/Sean-Anderson/drm-zynqmp_dp-Downgrade-log-level-for-aux-retries-message/20240316-071208
> > base: v6.8
> > patch link:
> > https://lore.kernel.org/r/20240315230916.1759060-7-sean.anderson%40linux.dev
> > patch subject: [PATCH 6/6] drm: zynqmp_dp: Add debugfs interface for
> > compliance testing
> > config: microblaze-allmodconfig
> > (https://download.01.org/0day-ci/archive/20240316/202403161704.achjdsjg-...@intel.com/config)
> > compiler: microblaze-linux-gcc (GCC) 13.2.0
> > reproduce (this is a W=1 build):
> > (https://download.01.org/0day-ci/archive/20240316/202403161704.achjdsjg-...@intel.com/reproduce)
> >
> > If you fix the issue in a separate patch/commit (i.e. not just a new
> > version of
> > the same patch/commit), kindly add following tags
> > | Reported-by: kernel test robot
> > | Closes:
> > https://lore.kernel.org/oe-kbuild-all/202403161704.achjdsjg-...@intel.com/
> >
> > All warnings (new ones prefixed by >>):
> >
> >drivers/gpu/drm/xlnx/zynqmp_dp.c: In function
> > 'zynqmp_dp_bridge_debugfs_init':
> >>> drivers/gpu/drm/xlnx/zynqmp_dp.c:2168:31: warning: 'sprintf' may write a
> >>> terminating nul past the end of the destination [-Wformat-overflow=]
> > 2168 | sprintf(name, fmt, i);
> > | ^~~
> >drivers/gpu/drm/xlnx/zynqmp_dp.c:2168:17: note: 'sprintf' output between
> > 18 and 20 bytes into a destination of size 19
> > 2168 | sprintf(name, fmt, i);
> > | ^
>
> Not a bug, as i will be at most 4, which uses 1 digit.
The compiler can't know that. Please fix this, there's a zero warning
policy.
> > vim +/sprintf +2168 drivers/gpu/drm/xlnx/zynqmp_dp.c
> >
> > 2136
> > 2137 DEFINE_DEBUGFS_ATTRIBUTE(fops_zynqmp_dp_rate,
> > zynqmp_dp_rate_get,
> > 2138 zynqmp_dp_rate_set, "%llu\n");
> > 2139
> > 2140 static void zynqmp_dp_bridge_debugfs_init(struct drm_bridge
> > *bridge,
> > 2141struct dentry *root)
> > 2142 {
> > 2143 struct zynqmp_dp *dp = bridge_to_dp(bridge);
> > 2144 struct dentry *test;
> > 2145 int i;
> > 2146
> > 2147 dp->test.bw_code = DP_LINK_BW_5_4;
> > 2148 dp->test.link_cnt = dp->num_lanes;
> > 2149
> > 2150 test = debugfs_create_dir("test", root);
> > 2151 #define CREATE_FILE(name) \
> > 2152 debugfs_create_file(#name, 0600, test, dp,
> > _zynqmp_dp_##name)
> > 2153 CREATE_FILE(pattern);
> > 2154 CREATE_FILE(enhanced);
> > 2155 CREATE_FILE(downspread);
> > 2156 CREATE_FILE(active);
> > 2157 CREATE_FILE(custom);
> > 2158 CREATE_FILE(rate);
> > 2159 CREATE_FILE(lanes);
> > 2160
> > 2161 for (i = 0; i < dp->num_lanes; i++) {
> > 2162 static const char fmt[] = "lane%d_preemphasis";
> > 2163 char name[sizeof(fmt)];
> > 2164
> > 2165 dp->debugfs_train_set[i].dp = dp;
> > 2166 dp->debugfs_train_set[i].lane = i;
> > 2167
> >> 2168 sprintf(name, fmt, i);
> > 2169 debugfs_create_file(name, 0600, test,
> > 2170 >debugfs_train_set[i],
> > 2171
> > _zynqmp_dp_preemphasis);
> > 2172
> > 2173 sprintf(name, "lane%d_swing", i);
> > 2174 debugfs_create_file(name, 0600, test,
> > 2175 >debugfs_train_set[i],
> > 2176 _zynqmp_dp_swing);
> > 2177 }
> > 2178 }
> > 2179
--
Regards,
Laurent Pinchart
Re: [PATCH 5/6] drm: zynqmp_dp: Optionally ignore DPCD errors
> + return ret;
> }
>
> ret = drm_dp_dpcd_writeb(>aux, DP_LINK_BW_SET, bw_code);
> if (ret < 0) {
> - dev_err(dp->dev, "failed to set DP bandwidth\n");
> - return ret;
> + dev_warn(dp->dev, "failed to set DP bandwidth\n");
> + if (!ignore_dpcd)
> + return ret;
> }
>
> zynqmp_dp_write(dp, ZYNQMP_DP_LINK_BW_SET, bw_code);
> @@ -860,7 +869,7 @@ static int zynqmp_dp_train(struct zynqmp_dp *dp)
>
> ret = zynqmp_dp_setup(dp, dp->mode.bw_code, dp->mode.lane_cnt,
> drm_dp_enhanced_frame_cap(dp->dpcd),
> - dp->dpcd[3] & 0x1);
> + dp->dpcd[3] & 0x1, false);
> if (ret)
> return ret;
>
> @@ -877,7 +886,7 @@ static int zynqmp_dp_train(struct zynqmp_dp *dp)
> ret = drm_dp_dpcd_writeb(>aux, DP_TRAINING_PATTERN_SET,
>DP_TRAINING_PATTERN_DISABLE);
> if (ret < 0) {
> - dev_err(dp->dev, "failed to disable training pattern\n");
> + dev_warn(dp->dev, "failed to disable training pattern\n");
> return ret;
> }
> zynqmp_dp_write(dp, ZYNQMP_DP_TRAINING_PATTERN_SET,
--
Regards,
Laurent Pinchart
Re: [PATCH 4/6] drm: zynqmp_dp: Split off several helper functions
if (ret < 0)
> + return zynqmp_dp_phy_ready(dp);
> +}
> +
> +
> +/**
> + * zynqmp_dp_train - Train the link
> + * @dp: DisplayPort IP core structure
> + *
> + * Return: 0 if all trains are done successfully, or corresponding error
> code.
> + */
> +static int zynqmp_dp_train(struct zynqmp_dp *dp)
> +{
> + int ret;
> +
> + ret = zynqmp_dp_setup(dp, dp->mode.bw_code, dp->mode.lane_cnt,
> + drm_dp_enhanced_frame_cap(dp->dpcd),
> + dp->dpcd[3] & 0x1);
This patch looks OK. Assuming you make correct use of the new functions
in the next patches,
Reviewed-by: Laurent Pinchart
On a side note, I think the above could be written
ret = zynqmp_dp_setup(dp, dp->mode.bw_code, dp->mode.lane_cnt,
drm_dp_enhanced_frame_cap(dp->dpcd),
dp->dpcd[DP_MAX_DOWNSPREAD] &
DP_MAX_DOWNSPREAD_0_5);
> + if (ret)
> return ret;
>
> zynqmp_dp_write(dp, ZYNQMP_DP_SCRAMBLING_DISABLE, 1);
--
Regards,
Laurent Pinchart
Re: [PATCH 3/6] drm: zynqmp_dp: Add locking
cted;
> }
>
> @@ -1611,6 +1623,29 @@ static void zynqmp_dp_hpd_work_func(struct work_struct
> *work)
> drm_bridge_hpd_notify(>bridge, status);
> }
>
> +static void zynqmp_dp_hpd_irq_work_func(struct work_struct *work)
> +{
> + struct zynqmp_dp *dp = container_of(work, struct zynqmp_dp,
> + hpd_irq_work.work);
> + u8 status[DP_LINK_STATUS_SIZE + 2];
> + int err;
> +
> + mutex_lock(>lock);
> + err = drm_dp_dpcd_read(>aux, DP_SINK_COUNT, status,
> +DP_LINK_STATUS_SIZE + 2);
> + if (err < 0) {
> + dev_dbg_ratelimited(dp->dev,
> + "could not read sink status: %d\n", err);
> + } else {
> + if (status[4] & DP_LINK_STATUS_UPDATED ||
> + !drm_dp_clock_recovery_ok([2], dp->mode.lane_cnt) ||
> + !drm_dp_channel_eq_ok([2], dp->mode.lane_cnt)) {
> + zynqmp_dp_train_loop(dp);
> + }
> + }
> + mutex_unlock(>lock);
> +}
> +
> static irqreturn_t zynqmp_dp_irq_handler(int irq, void *data)
> {
> struct zynqmp_dp *dp = (struct zynqmp_dp *)data;
> @@ -1635,23 +1670,9 @@ static irqreturn_t zynqmp_dp_irq_handler(int irq, void
> *data)
> if (status & ZYNQMP_DP_INT_HPD_EVENT)
> schedule_delayed_work(>hpd_work, 0);
>
> - if (status & ZYNQMP_DP_INT_HPD_IRQ) {
> - int ret;
> - u8 status[DP_LINK_STATUS_SIZE + 2];
> + if (status & ZYNQMP_DP_INT_HPD_IRQ)
> + schedule_delayed_work(>hpd_irq_work, 0);
>
> - ret = drm_dp_dpcd_read(>aux, DP_SINK_COUNT, status,
> -DP_LINK_STATUS_SIZE + 2);
> - if (ret < 0)
> - goto handled;
> -
> - if (status[4] & DP_LINK_STATUS_UPDATED ||
> - !drm_dp_clock_recovery_ok([2], dp->mode.lane_cnt) ||
> - !drm_dp_channel_eq_ok([2], dp->mode.lane_cnt)) {
> - zynqmp_dp_train_loop(dp);
> - }
> - }
> -
> -handled:
> return IRQ_HANDLED;
> }
>
> @@ -1674,8 +1695,10 @@ int zynqmp_dp_probe(struct zynqmp_dpsub *dpsub)
> dp->dev = >dev;
> dp->dpsub = dpsub;
> dp->status = connector_status_disconnected;
> + mutex_init(>lock);
>
> INIT_DELAYED_WORK(>hpd_work, zynqmp_dp_hpd_work_func);
> + INIT_DELAYED_WORK(>hpd_irq_work, zynqmp_dp_hpd_irq_work_func);
>
> /* Acquire all resources (IOMEM, IRQ and PHYs). */
> res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dp");
> @@ -1775,6 +1798,7 @@ void zynqmp_dp_remove(struct zynqmp_dpsub *dpsub)
> zynqmp_dp_write(dp, ZYNQMP_DP_INT_DS, ZYNQMP_DP_INT_ALL);
> disable_irq(dp->irq);
>
> + cancel_delayed_work_sync(>hpd_irq_work);
> cancel_delayed_work_sync(>hpd_work);
>
> zynqmp_dp_write(dp, ZYNQMP_DP_TRANSMITTER_ENABLE, 0);
> @@ -1782,4 +1806,5 @@ void zynqmp_dp_remove(struct zynqmp_dpsub *dpsub)
>
> zynqmp_dp_phy_exit(dp);
> zynqmp_dp_reset(dp, true);
> + mutex_destroy(>lock);
> }
--
Regards,
Laurent Pinchart
Re: [PATCH 2/6] drm: zynqmp_dp: Adjust training values per-lane
Hi Sean,
Thank you for the patch.
On Fri, Mar 15, 2024 at 07:09:12PM -0400, Sean Anderson wrote:
> The feedback we get from the DPRX is per-lane. Make changes using this
> information, instead of picking the maximum values from all lanes. This
> results in more-consistent training on marginal links.
>
> Signed-off-by: Sean Anderson
> ---
>
> drivers/gpu/drm/xlnx/zynqmp_dp.c | 23 ---
> 1 file changed, 8 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c
> b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> index 98a32e6a0459..8635b5673386 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> @@ -605,28 +605,21 @@ static void zynqmp_dp_adjust_train(struct zynqmp_dp *dp,
> u8 link_status[DP_LINK_STATUS_SIZE])
> {
> u8 *train_set = dp->train_set;
> - u8 voltage = 0, preemphasis = 0;
> u8 i;
>
> for (i = 0; i < dp->mode.lane_cnt; i++) {
> - u8 v = drm_dp_get_adjust_request_voltage(link_status, i);
> - u8 p = drm_dp_get_adjust_request_pre_emphasis(link_status, i);
> + u8 voltage = drm_dp_get_adjust_request_voltage(link_status, i);
> + u8 preemphasis =
> + drm_dp_get_adjust_request_pre_emphasis(link_status, i);
>
> - if (v > voltage)
> - voltage = v;
> + if (voltage >= DP_TRAIN_VOLTAGE_SWING_LEVEL_3)
> + voltage |= DP_TRAIN_MAX_SWING_REACHED;
>
> - if (p > preemphasis)
> - preemphasis = p;
> - }
> + if (preemphasis >= DP_TRAIN_PRE_EMPH_LEVEL_2)
> + preemphasis |= DP_TRAIN_MAX_PRE_EMPHASIS_REACHED;
>
> - if (voltage >= DP_TRAIN_VOLTAGE_SWING_LEVEL_3)
> - voltage |= DP_TRAIN_MAX_SWING_REACHED;
> -
> - if (preemphasis >= DP_TRAIN_PRE_EMPH_LEVEL_2)
> - preemphasis |= DP_TRAIN_MAX_PRE_EMPHASIS_REACHED;
> -
> - for (i = 0; i < dp->mode.lane_cnt; i++)
> train_set[i] = voltage | preemphasis;
> + }
I don't have enough DP knowledge to review this :-(
> }
>
> /**
--
Regards,
Laurent Pinchart
Re: [PATCH 1/6] drm: zynqmp_dp: Downgrade log level for aux retries message
Hi Sean,
Thank you for the patch.
On Fri, Mar 15, 2024 at 07:09:11PM -0400, Sean Anderson wrote:
> Enable this message for verbose debugging only as it is otherwise
> printed after every AUX message, quickly filling the log buffer.
>
> Signed-off-by: Sean Anderson
> ---
>
> drivers/gpu/drm/xlnx/zynqmp_dp.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c
> b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> index a0606fab0e22..98a32e6a0459 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> @@ -1006,7 +1006,7 @@ zynqmp_dp_aux_transfer(struct drm_dp_aux *aux, struct
> drm_dp_aux_msg *msg)
> msg->buffer, msg->size,
> >reply);
> if (!ret) {
> - dev_dbg(dp->dev, "aux %d retries\n", i);
> + dev_vdbg(dp->dev, "aux %d retries\n", i);
I didn't know about dev_vdbg(). I suppose this makes sense,
Reviewed-by: Laurent Pinchart
> return msg->size;
> }
>
--
Regards,
Laurent Pinchart
[PATCH] drm: bridge: thc63lvd1024: Print error message when DT parsing fails
Commit 00084f0c01bf ("drm: bridge: thc63lvd1024: Switch to use
of_graph_get_remote_node()") simplified the thc63lvd1024 driver by
replacing hand-rolled code with a helper function. While doing so, it
created an error code path at probe time without any error message,
potentially causing probe issues that get annoying to debug. Fix it by
adding an error message.
Fixes: 00084f0c01bf ("drm: bridge: thc63lvd1024: Switch to use
of_graph_get_remote_node()")
Signed-off-by: Laurent Pinchart
---
drivers/gpu/drm/bridge/thc63lvd1024.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/bridge/thc63lvd1024.c
b/drivers/gpu/drm/bridge/thc63lvd1024.c
index 5f99f9724081..674efc489e3a 100644
--- a/drivers/gpu/drm/bridge/thc63lvd1024.c
+++ b/drivers/gpu/drm/bridge/thc63lvd1024.c
@@ -125,8 +125,11 @@ static int thc63_parse_dt(struct thc63_dev *thc63)
remote = of_graph_get_remote_node(thc63->dev->of_node,
THC63_RGB_OUT0, -1);
- if (!remote)
+ if (!remote) {
+ dev_err(thc63->dev, "No remote endpoint for port@%u\n",
+ THC63_RGB_OUT0);
return -ENODEV;
+ }
thc63->next = of_drm_find_bridge(remote);
of_node_put(remote);
base-commit: 00084f0c01bf3a2591d007010b196e048281c455
--
Regards,
Laurent Pinchart
Re: [PATCH] drm: bridge: thc63lvd1024: Switch to use of_graph_get_remote_node()
On Mon, Mar 18, 2024 at 11:53:11PM +0800, Sui Jingfeng wrote:
> On 2024/3/18 23:33, Laurent Pinchart wrote:
> > On Sun, Mar 17, 2024 at 01:28:00AM +0800, Sui Jingfeng wrote:
> >> To reduce boilerplate, use of_graph_get_remote_node() helper instead of
> >> the hand-rolling code.
> >>
> >> Signed-off-by: Sui Jingfeng
> >> ---
> >> drivers/gpu/drm/bridge/thc63lvd1024.c | 24 +++-
> >> 1 file changed, 3 insertions(+), 21 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/bridge/thc63lvd1024.c
> >> b/drivers/gpu/drm/bridge/thc63lvd1024.c
> >> index d4c1a601bbb5..5f99f9724081 100644
> >> --- a/drivers/gpu/drm/bridge/thc63lvd1024.c
> >> +++ b/drivers/gpu/drm/bridge/thc63lvd1024.c
> >> @@ -123,28 +123,10 @@ static int thc63_parse_dt(struct thc63_dev *thc63)
> >>struct device_node *endpoint;
> >>struct device_node *remote;
> >>
> >> - endpoint = of_graph_get_endpoint_by_regs(thc63->dev->of_node,
> >> - THC63_RGB_OUT0, -1);
> >> - if (!endpoint) {
> >> - dev_err(thc63->dev, "Missing endpoint in port@%u\n",
> >> - THC63_RGB_OUT0);
> >> - return -ENODEV;
> >> - }
> >> -
> >> - remote = of_graph_get_remote_port_parent(endpoint);
> >> - of_node_put(endpoint);
> >> - if (!remote) {
> >> - dev_err(thc63->dev, "Endpoint in port@%u unconnected\n",
> >> - THC63_RGB_OUT0);
> >> + remote = of_graph_get_remote_node(thc63->dev->of_node,
> >> +THC63_RGB_OUT0, -1);
> >> + if (!remote)
> > The old logic is equivalent to of_graph_get_remote_node(), but now the
> > driver will fail probing without an error message. That's not very nice
> > as it leads to difficult to debug problems. I would keep one dev_err()
> > here:
> >
> > dev_err(thc63->dev, "No remote endpoint for port@%u\n",
> > THC63_RGB_OUT0);
> >
> > As your patch has been merged already, would you like to post a
> > follow-up patch to fix this ?
>
> Yes, this is indeed a difference. I will post a follow-up patch to fix this.
I'm actually build-testing a patch I already wrote :-) I'll post it in a
moment.
> >>return -ENODEV;
> >> - }
> >> -
> >> - if (!of_device_is_available(remote)) {
> >> - dev_err(thc63->dev, "port@%u remote endpoint is disabled\n",
> >> - THC63_RGB_OUT0);
> >> - of_node_put(remote);
> >> - return -ENODEV;
> >> - }
> >>
> >>thc63->next = of_drm_find_bridge(remote);
> >>of_node_put(remote);
--
Regards,
Laurent Pinchart
Re: [PATCH] drm: bridge: thc63lvd1024: Switch to use of_graph_get_remote_node()
Hi Sui,
Thank you for the patch.
On Sun, Mar 17, 2024 at 01:28:00AM +0800, Sui Jingfeng wrote:
> To reduce boilerplate, use of_graph_get_remote_node() helper instead of
> the hand-rolling code.
>
> Signed-off-by: Sui Jingfeng
> ---
> drivers/gpu/drm/bridge/thc63lvd1024.c | 24 +++-
> 1 file changed, 3 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/thc63lvd1024.c
> b/drivers/gpu/drm/bridge/thc63lvd1024.c
> index d4c1a601bbb5..5f99f9724081 100644
> --- a/drivers/gpu/drm/bridge/thc63lvd1024.c
> +++ b/drivers/gpu/drm/bridge/thc63lvd1024.c
> @@ -123,28 +123,10 @@ static int thc63_parse_dt(struct thc63_dev *thc63)
> struct device_node *endpoint;
> struct device_node *remote;
>
> - endpoint = of_graph_get_endpoint_by_regs(thc63->dev->of_node,
> - THC63_RGB_OUT0, -1);
> - if (!endpoint) {
> - dev_err(thc63->dev, "Missing endpoint in port@%u\n",
> - THC63_RGB_OUT0);
> - return -ENODEV;
> - }
> -
> - remote = of_graph_get_remote_port_parent(endpoint);
> - of_node_put(endpoint);
> - if (!remote) {
> - dev_err(thc63->dev, "Endpoint in port@%u unconnected\n",
> - THC63_RGB_OUT0);
> + remote = of_graph_get_remote_node(thc63->dev->of_node,
> + THC63_RGB_OUT0, -1);
> + if (!remote)
The old logic is equivalent to of_graph_get_remote_node(), but now the
driver will fail probing without an error message. That's not very nice
as it leads to difficult to debug problems. I would keep one dev_err()
here:
dev_err(thc63->dev, "No remote endpoint for port@%u\n",
THC63_RGB_OUT0);
As your patch has been merged already, would you like to post a
follow-up patch to fix this ?
> return -ENODEV;
> - }
> -
> - if (!of_device_is_available(remote)) {
> - dev_err(thc63->dev, "port@%u remote endpoint is disabled\n",
> - THC63_RGB_OUT0);
> - of_node_put(remote);
> - return -ENODEV;
> - }
>
> thc63->next = of_drm_find_bridge(remote);
> of_node_put(remote);
--
Regards,
Laurent Pinchart
Re: [PATCH] drm/panel: ilitek-ili9881c: Fix warning with GPIO controllers that sleep
Hi Neil,
On Mon, Mar 18, 2024 at 10:03:21AM +0100, Neil Armstrong wrote:
> On 17/03/2024 16:48, Laurent Pinchart wrote:
> > The ilitek-ili9881c controls the reset GPIO using the non-sleeping
> > gpiod_set_value() function. This complains loudly when the GPIO
> > controller needs to sleep. As the caller can sleep, use
> > gpiod_set_value_cansleep() to fix the issue.
>
> Seems something buggy happened to the patchset, this patch doesn't appear
> in the cover letter and insn't numbered...
I've sent it separately, as it's functionally independent from the small
series for the same driver.
> > Signed-off-by: Laurent Pinchart
> > ---
> > drivers/gpu/drm/panel/panel-ilitek-ili9881c.c | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/panel/panel-ilitek-ili9881c.c
> > b/drivers/gpu/drm/panel/panel-ilitek-ili9881c.c
> > index 80b386f91320..084c37fa7348 100644
> > --- a/drivers/gpu/drm/panel/panel-ilitek-ili9881c.c
> > +++ b/drivers/gpu/drm/panel/panel-ilitek-ili9881c.c
> > @@ -1276,10 +1276,10 @@ static int ili9881c_prepare(struct drm_panel *panel)
> > msleep(5);
> >
> > /* And reset it */
> > - gpiod_set_value(ctx->reset, 1);
> > + gpiod_set_value_cansleep(ctx->reset, 1);
> > msleep(20);
> >
> > - gpiod_set_value(ctx->reset, 0);
> > + gpiod_set_value_cansleep(ctx->reset, 0);
> > msleep(20);
> >
> > for (i = 0; i < ctx->desc->init_length; i++) {
> > @@ -1334,7 +1334,7 @@ static int ili9881c_unprepare(struct drm_panel *panel)
> >
> > mipi_dsi_dcs_enter_sleep_mode(ctx->dsi);
> > regulator_disable(ctx->power);
> > - gpiod_set_value(ctx->reset, 1);
> > + gpiod_set_value_cansleep(ctx->reset, 1);
> >
> > return 0;
> > }
>
> Anyway:
> Reviewed-by: Neil Armstrong
--
Regards,
Laurent Pinchart
[PATCH 1/2] dt-bindings: ili9881c: Add Startek KD050HDFIA020-C020A support
Document the compatible value for Startek KD050HDFIA020-C020A panels. Signed-off-by: Laurent Pinchart --- .../devicetree/bindings/display/panel/ilitek,ili9881c.yaml | 1 + 1 file changed, 1 insertion(+) diff --git a/Documentation/devicetree/bindings/display/panel/ilitek,ili9881c.yaml b/Documentation/devicetree/bindings/display/panel/ilitek,ili9881c.yaml index b1e624be3e33..a015dce72f60 100644 --- a/Documentation/devicetree/bindings/display/panel/ilitek,ili9881c.yaml +++ b/Documentation/devicetree/bindings/display/panel/ilitek,ili9881c.yaml @@ -19,6 +19,7 @@ properties: - ampire,am8001280g - bananapi,lhr050h41 - feixin,k101-im2byl02 + - startek,kd050hdfia020 - tdo,tl050hdv35 - wanchanglong,w552946aba - const: ilitek,ili9881c -- Regards, Laurent Pinchart
[PATCH 2/2] drm/panel: ilitek-ili9881c: Add Startek KD050HDFIA020-C020A support
Add support for the Startek KD050HDFIA020-C020A panel.
The init table comes from the Compulab BSP ([1]).
[1]
https://github.com/compulab-yokneam/meta-bsp-imx8mp/blob/ucm-imx8m-plus-r1.1/recipes-kernel/linux/compulab/imx8mp/-compulab-imx8m-plus-Add-boards-support.patch
Signed-off-by: Laurent Pinchart
---
drivers/gpu/drm/panel/panel-ilitek-ili9881c.c | 222 ++
1 file changed, 222 insertions(+)
diff --git a/drivers/gpu/drm/panel/panel-ilitek-ili9881c.c
b/drivers/gpu/drm/panel/panel-ilitek-ili9881c.c
index 2ffe5f68a890..80b386f91320 100644
--- a/drivers/gpu/drm/panel/panel-ilitek-ili9881c.c
+++ b/drivers/gpu/drm/panel/panel-ilitek-ili9881c.c
@@ -455,6 +455,202 @@ static const struct ili9881c_instr k101_im2byl02_init[] =
{
ILI9881C_COMMAND_INSTR(0xD3, 0x3F), /* VN0 */
};
+static const struct ili9881c_instr kd050hdfia020_init[] = {
+ ILI9881C_SWITCH_PAGE_INSTR(3),
+ ILI9881C_COMMAND_INSTR(0x01, 0x00),
+ ILI9881C_COMMAND_INSTR(0x02, 0x00),
+ ILI9881C_COMMAND_INSTR(0x03, 0x72),
+ ILI9881C_COMMAND_INSTR(0x04, 0x00),
+ ILI9881C_COMMAND_INSTR(0x05, 0x00),
+ ILI9881C_COMMAND_INSTR(0x06, 0x09),
+ ILI9881C_COMMAND_INSTR(0x07, 0x00),
+ ILI9881C_COMMAND_INSTR(0x08, 0x00),
+ ILI9881C_COMMAND_INSTR(0x09, 0x01),
+ ILI9881C_COMMAND_INSTR(0x0a, 0x00),
+ ILI9881C_COMMAND_INSTR(0x0b, 0x00),
+ ILI9881C_COMMAND_INSTR(0x0c, 0x01),
+ ILI9881C_COMMAND_INSTR(0x0d, 0x00),
+ ILI9881C_COMMAND_INSTR(0x0e, 0x00),
+ ILI9881C_COMMAND_INSTR(0x0f, 0x00),
+ ILI9881C_COMMAND_INSTR(0x10, 0x00),
+ ILI9881C_COMMAND_INSTR(0x11, 0x00),
+ ILI9881C_COMMAND_INSTR(0x12, 0x00),
+ ILI9881C_COMMAND_INSTR(0x13, 0x00),
+ ILI9881C_COMMAND_INSTR(0x14, 0x00),
+ ILI9881C_COMMAND_INSTR(0x15, 0x00),
+ ILI9881C_COMMAND_INSTR(0x16, 0x00),
+ ILI9881C_COMMAND_INSTR(0x17, 0x00),
+ ILI9881C_COMMAND_INSTR(0x18, 0x00),
+ ILI9881C_COMMAND_INSTR(0x19, 0x00),
+ ILI9881C_COMMAND_INSTR(0x1a, 0x00),
+ ILI9881C_COMMAND_INSTR(0x1b, 0x00),
+ ILI9881C_COMMAND_INSTR(0x1c, 0x00),
+ ILI9881C_COMMAND_INSTR(0x1d, 0x00),
+ ILI9881C_COMMAND_INSTR(0x1e, 0x40),
+ ILI9881C_COMMAND_INSTR(0x1f, 0x80),
+ ILI9881C_COMMAND_INSTR(0x20, 0x05),
+ ILI9881C_COMMAND_INSTR(0x20, 0x05),
+ ILI9881C_COMMAND_INSTR(0x21, 0x02),
+ ILI9881C_COMMAND_INSTR(0x22, 0x00),
+ ILI9881C_COMMAND_INSTR(0x23, 0x00),
+ ILI9881C_COMMAND_INSTR(0x24, 0x00),
+ ILI9881C_COMMAND_INSTR(0x25, 0x00),
+ ILI9881C_COMMAND_INSTR(0x26, 0x00),
+ ILI9881C_COMMAND_INSTR(0x27, 0x00),
+ ILI9881C_COMMAND_INSTR(0x28, 0x33),
+ ILI9881C_COMMAND_INSTR(0x29, 0x02),
+ ILI9881C_COMMAND_INSTR(0x2a, 0x00),
+ ILI9881C_COMMAND_INSTR(0x2b, 0x00),
+ ILI9881C_COMMAND_INSTR(0x2c, 0x00),
+ ILI9881C_COMMAND_INSTR(0x2d, 0x00),
+ ILI9881C_COMMAND_INSTR(0x2e, 0x00),
+ ILI9881C_COMMAND_INSTR(0x2f, 0x00),
+ ILI9881C_COMMAND_INSTR(0x30, 0x00),
+ ILI9881C_COMMAND_INSTR(0x31, 0x00),
+ ILI9881C_COMMAND_INSTR(0x32, 0x00),
+ ILI9881C_COMMAND_INSTR(0x32, 0x00),
+ ILI9881C_COMMAND_INSTR(0x33, 0x00),
+ ILI9881C_COMMAND_INSTR(0x34, 0x04),
+ ILI9881C_COMMAND_INSTR(0x35, 0x00),
+ ILI9881C_COMMAND_INSTR(0x36, 0x00),
+ ILI9881C_COMMAND_INSTR(0x37, 0x00),
+ ILI9881C_COMMAND_INSTR(0x38, 0x3C),
+ ILI9881C_COMMAND_INSTR(0x39, 0x00),
+ ILI9881C_COMMAND_INSTR(0x3a, 0x40),
+ ILI9881C_COMMAND_INSTR(0x3b, 0x40),
+ ILI9881C_COMMAND_INSTR(0x3c, 0x00),
+ ILI9881C_COMMAND_INSTR(0x3d, 0x00),
+ ILI9881C_COMMAND_INSTR(0x3e, 0x00),
+ ILI9881C_COMMAND_INSTR(0x3f, 0x00),
+ ILI9881C_COMMAND_INSTR(0x40, 0x00),
+ ILI9881C_COMMAND_INSTR(0x41, 0x00),
+ ILI9881C_COMMAND_INSTR(0x42, 0x00),
+ ILI9881C_COMMAND_INSTR(0x43, 0x00),
+ ILI9881C_COMMAND_INSTR(0x44, 0x00),
+ ILI9881C_COMMAND_INSTR(0x50, 0x01),
+ ILI9881C_COMMAND_INSTR(0x51, 0x23),
+ ILI9881C_COMMAND_INSTR(0x52, 0x45),
+ ILI9881C_COMMAND_INSTR(0x53, 0x67),
+ ILI9881C_COMMAND_INSTR(0x54, 0x89),
+ ILI9881C_COMMAND_INSTR(0x55, 0xab),
+ ILI9881C_COMMAND_INSTR(0x56, 0x01),
+ ILI9881C_COMMAND_INSTR(0x57, 0x23),
+ ILI9881C_COMMAND_INSTR(0x58, 0x45),
+ ILI9881C_COMMAND_INSTR(0x59, 0x67),
+ ILI9881C_COMMAND_INSTR(0x5a, 0x89),
+ ILI9881C_COMMAND_INSTR(0x5b, 0xab),
+ ILI9881C_COMMAND_INSTR(0x5c, 0xcd),
+ ILI9881C_COMMAND_INSTR(0x5d, 0xef),
+ ILI9881C_COMMAND_INSTR(0x5e, 0x11),
+ ILI9881C_COMMAND_INSTR(0x5f, 0x01),
+ ILI9881C_COMMAND_INSTR(0x60, 0x00),
+ ILI9881C_COMMAND_INSTR(0x61, 0x15),
+ ILI9881C_COMMAND_INSTR(0x62, 0x14),
+ ILI9881C_COMMAND_INSTR(0x63, 0x0E),
+ ILI9881C_COMMAND_INSTR(0x64, 0x0F),
+ ILI9881C_COMMAND_INSTR(0x65, 0x0C),
+ ILI9881C_COMMAND_INSTR
[PATCH 0/2] drm/panel: Add Startek KD050HDFIA020-C020A support
Hello, This small series adds support for the Startek KD050HDFIA020-C020A panel to the ilitek-ili9881c driver. There's not much special here, patch 1/2 addresses the DT binding and patch 2/2 the driver. Please see individual patches for details. The series has been tested witha Compulab SB-UCM-iMX8MPLUS carrier board. Laurent Pinchart (2): dt-bindings: ili9881c: Add Startek KD050HDFIA020-C020A support drm/panel: ilitek-ili9881c: Add Startek KD050HDFIA020-C020A support .../display/panel/ilitek,ili9881c.yaml| 1 + drivers/gpu/drm/panel/panel-ilitek-ili9881c.c | 222 ++ 2 files changed, 223 insertions(+) -- Regards, Laurent Pinchart
[PATCH] drm/panel: ilitek-ili9881c: Fix warning with GPIO controllers that sleep
The ilitek-ili9881c controls the reset GPIO using the non-sleeping
gpiod_set_value() function. This complains loudly when the GPIO
controller needs to sleep. As the caller can sleep, use
gpiod_set_value_cansleep() to fix the issue.
Signed-off-by: Laurent Pinchart
---
drivers/gpu/drm/panel/panel-ilitek-ili9881c.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/panel/panel-ilitek-ili9881c.c
b/drivers/gpu/drm/panel/panel-ilitek-ili9881c.c
index 80b386f91320..084c37fa7348 100644
--- a/drivers/gpu/drm/panel/panel-ilitek-ili9881c.c
+++ b/drivers/gpu/drm/panel/panel-ilitek-ili9881c.c
@@ -1276,10 +1276,10 @@ static int ili9881c_prepare(struct drm_panel *panel)
msleep(5);
/* And reset it */
- gpiod_set_value(ctx->reset, 1);
+ gpiod_set_value_cansleep(ctx->reset, 1);
msleep(20);
- gpiod_set_value(ctx->reset, 0);
+ gpiod_set_value_cansleep(ctx->reset, 0);
msleep(20);
for (i = 0; i < ctx->desc->init_length; i++) {
@@ -1334,7 +1334,7 @@ static int ili9881c_unprepare(struct drm_panel *panel)
mipi_dsi_dcs_enter_sleep_mode(ctx->dsi);
regulator_disable(ctx->power);
- gpiod_set_value(ctx->reset, 1);
+ gpiod_set_value_cansleep(ctx->reset, 1);
return 0;
}
--
Regards,
Laurent Pinchart
Re: [PATCH v3 2/2] drm/bridge: adv7511: get edid in hpd_work to update CEC phys address
struct drm_connector *connector)
> {
> - struct edid *edid;
> -
> - /* Reading the EDID only works if the device is powered */
> - if (!adv7511->powered) {
> - unsigned int edid_i2c_addr =
> - (adv7511->i2c_edid->addr << 1);
> -
> - __adv7511_power_on(adv7511);
> -
> - /* Reset the EDID_I2C_ADDR register as it might be cleared */
> - regmap_write(adv7511->regmap, ADV7511_REG_EDID_I2C_ADDR,
> - edid_i2c_addr);
> - }
> -
> - edid = drm_do_get_edid(connector, adv7511_get_edid_block, adv7511);
> -
> - if (!adv7511->powered)
> - __adv7511_power_off(adv7511);
> -
> - adv7511_set_config_csc(adv7511, connector, adv7511->rgb,
> -drm_detect_hdmi_monitor(edid));
> -
> - cec_s_phys_addr_from_edid(adv7511->cec_adap, edid);
> -
> - return edid;
> + return __adv7511_get_edid(adv7511, connector);
> }
>
> static int adv7511_get_modes(struct adv7511 *adv7511,
>
--
Regards,
Laurent Pinchart
Re: [PATCH v3 1/2] drm/bridge: adv7511: rearrange hotplug work code
Hi Alvin,
Thank you for the patch.
On Mon, Feb 19, 2024 at 09:12:58PM +0100, Alvin Šipraga wrote:
> From: Alvin Šipraga
>
> In preparation for calling EDID helpers from the hotplug work, move the
> hotplug work below the EDID helper section. No functional change.
>
> Reviewed-by: Robert Foss
> Signed-off-by: Alvin Šipraga
Reviewed-by: Laurent Pinchart
> ---
> drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 120
> ++-
> 1 file changed, 62 insertions(+), 58 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> index 8be235144f6d..5ffc5904bd59 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> @@ -406,64 +406,6 @@ static void adv7511_power_off(struct adv7511 *adv7511)
> * Interrupt and hotplug detection
> */
>
> -static bool adv7511_hpd(struct adv7511 *adv7511)
> -{
> - unsigned int irq0;
> - int ret;
> -
> - ret = regmap_read(adv7511->regmap, ADV7511_REG_INT(0), );
> - if (ret < 0)
> - return false;
> -
> - if (irq0 & ADV7511_INT0_HPD) {
> - regmap_write(adv7511->regmap, ADV7511_REG_INT(0),
> - ADV7511_INT0_HPD);
> - return true;
> - }
> -
> - return false;
> -}
> -
> -static void adv7511_hpd_work(struct work_struct *work)
> -{
> - struct adv7511 *adv7511 = container_of(work, struct adv7511, hpd_work);
> - enum drm_connector_status status;
> - unsigned int val;
> - int ret;
> -
> - ret = regmap_read(adv7511->regmap, ADV7511_REG_STATUS, );
> - if (ret < 0)
> - status = connector_status_disconnected;
> - else if (val & ADV7511_STATUS_HPD)
> - status = connector_status_connected;
> - else
> - status = connector_status_disconnected;
> -
> - /*
> - * The bridge resets its registers on unplug. So when we get a plug
> - * event and we're already supposed to be powered, cycle the bridge to
> - * restore its state.
> - */
> - if (status == connector_status_connected &&
> - adv7511->connector.status == connector_status_disconnected &&
> - adv7511->powered) {
> - regcache_mark_dirty(adv7511->regmap);
> - adv7511_power_on(adv7511);
> - }
> -
> - if (adv7511->connector.status != status) {
> - adv7511->connector.status = status;
> -
> - if (adv7511->connector.dev) {
> - if (status == connector_status_disconnected)
> - cec_phys_addr_invalidate(adv7511->cec_adap);
> - drm_kms_helper_hotplug_event(adv7511->connector.dev);
> - } else {
> - drm_bridge_hpd_notify(>bridge, status);
> - }
> - }
> -}
> -
> static int adv7511_irq_process(struct adv7511 *adv7511, bool process_hpd)
> {
> unsigned int irq0, irq1;
> @@ -600,6 +542,68 @@ static int adv7511_get_edid_block(void *data, u8 *buf,
> unsigned int block,
> return 0;
> }
>
> +/*
> -
> + * Hotplug handling
> + */
> +
> +static bool adv7511_hpd(struct adv7511 *adv7511)
> +{
> + unsigned int irq0;
> + int ret;
> +
> + ret = regmap_read(adv7511->regmap, ADV7511_REG_INT(0), );
> + if (ret < 0)
> + return false;
> +
> + if (irq0 & ADV7511_INT0_HPD) {
> + regmap_write(adv7511->regmap, ADV7511_REG_INT(0),
> + ADV7511_INT0_HPD);
> + return true;
> + }
> +
> + return false;
> +}
> +
> +static void adv7511_hpd_work(struct work_struct *work)
> +{
> + struct adv7511 *adv7511 = container_of(work, struct adv7511, hpd_work);
> + enum drm_connector_status status;
> + unsigned int val;
> + int ret;
> +
> + ret = regmap_read(adv7511->regmap, ADV7511_REG_STATUS, );
> + if (ret < 0)
> + status = connector_status_disconnected;
> + else if (val & ADV7511_STATUS_HPD)
> + status = connector_status_connected;
> + else
> + status = connector_status_disconnected;
> +
> + /*
> + * The bridge resets its registers on unplug. So when we get a plug
> + * event and we're already supposed to be powered, cycle the bridge to
> + * restore its state.
> + */
> + if (status == connector_status_connected &&
> +
Re: [PATCH V2 1/2] drm/bridge: adv7511: Allow IRQ to share GPIO pins
Hello Adam, Thank you for the patch. On Mon, Mar 04, 2024 at 06:48:57PM -0600, Adam Ford wrote: > The IRQ registration currently assumes that the GPIO is dedicated > to it, but that may not necessarily be the case. If the board has > another device sharing the GPIO, it won't be registered and the > hot-plug detect fails to function. > > Currently, the handler reads two registers and blindly > assumes one of them caused the interrupt and returns IRQ_HANDLED > unless there is an error. In order to properly do this, the IRQ > handler needs to check if it needs to handle the IRQ and return > IRQ_NONE if there is nothing to handle. With the check added > and the return code properly indicating whether or not it there > was an IRQ, the IRQF_SHARED can be set to share a GPIO IRQ. > > Signed-off-by: Adam Ford Reviewed-by: Laurent Pinchart > --- > V2: Add check to see if there is IRQ data to handle > > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > index b5518ff97165..f3b4616a8fb6 100644 > --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > @@ -477,6 +477,11 @@ static int adv7511_irq_process(struct adv7511 *adv7511, > bool process_hpd) > if (ret < 0) > return ret; > > + /* If there is no IRQ to handle, exit indicating no IRQ data */ > + if (!(irq0 & (ADV7511_INT0_HPD | ADV7511_INT0_EDID_READY)) && > + !(irq1 & ADV7511_INT1_DDC_ERROR)) > + return -ENODATA; > + > regmap_write(adv7511->regmap, ADV7511_REG_INT(0), irq0); > regmap_write(adv7511->regmap, ADV7511_REG_INT(1), irq1); > > @@ -1318,7 +1323,8 @@ static int adv7511_probe(struct i2c_client *i2c) > > ret = devm_request_threaded_irq(dev, i2c->irq, NULL, > adv7511_irq_handler, > - IRQF_ONESHOT, dev_name(dev), > + IRQF_ONESHOT | IRQF_SHARED, > + dev_name(dev), > adv7511); > if (ret) > goto err_unregister_audio; -- Regards, Laurent Pinchart
Re: [PATCH 1/2] drm/bridge: adv7511: Allow IRQ to share GPIO pins
On Sun, Mar 03, 2024 at 09:44:03AM -0600, Adam Ford wrote:
> On Wed, Feb 28, 2024 at 10:31 AM Laurent Pinchart wrote:
> > On Wed, Feb 28, 2024 at 05:37:35AM -0600, Adam Ford wrote:
> > > The IRQ registration currently assumes that the GPIO is
> > > dedicated to it, but that may not necessarily be the case.
> > > If the board has another device sharing the IRQ, it won't be
> > > registered and the hot-plug detect fails. This is easily
> > > fixed by add the IRQF_SHARED flag.
> > >
> > > Signed-off-by: Adam Ford
> > >
> > > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> > > b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> > > index b5518ff97165..21f08b2ae265 100644
> > > --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> > > +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> > > @@ -1318,7 +1318,8 @@ static int adv7511_probe(struct i2c_client *i2c)
> > >
> > > ret = devm_request_threaded_irq(dev, i2c->irq, NULL,
> > > adv7511_irq_handler,
> > > - IRQF_ONESHOT, dev_name(dev),
> > > + IRQF_ONESHOT | IRQF_SHARED,
> > > + dev_name(dev),
> >
> > This looks fine, but the IRQ handler doesn't.
>
> Thanks for the review.
>
> > static int adv7511_irq_process(struct adv7511 *adv7511, bool process_hpd)
> > {
> > unsigned int irq0, irq1;
> > int ret;
> >
> > ret = regmap_read(adv7511->regmap, ADV7511_REG_INT(0), );
> > if (ret < 0)
> > return ret;
> >
> > ret = regmap_read(adv7511->regmap, ADV7511_REG_INT(1), );
> > if (ret < 0)
> > return ret;
>
> If I did a check here and returned if there was no IRQ to handle,
> would that be sufficient?
>
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> @@ -477,6 +477,11 @@ static int adv7511_irq_process(struct adv7511
> *adv7511, bool process_hpd)
> if (ret < 0)
> return ret;
>
> + /* If there is no IRQ to handle, exit indicating no IRQ handled */
> + if (!(irq0 & (ADV7511_INT0_HPD | ADV7511_INT0_EDID_READY)) &&
> + !(irq1 & ADV7511_INT1_DDC_ERROR))
If these are the only interrupt sources that the driver enables, this is
fine.
> + return -1;
Maybe a defined error code instead ?
> +
> regmap_write(adv7511->regmap, ADV7511_REG_INT(0), irq0);
> regmap_write(adv7511->regmap, ADV7511_REG_INT(1), irq1);
>
> With this, I would expect adv7511_irq_handler to return IRQ_NONE. If
> you're OK with that approach, I can do that. If you want me to merge
> adv7511_irq_handler, and adv7511_irq_process, I can do that too to
> make the return codes a little more intuitive.
>
> >
> > regmap_write(adv7511->regmap, ADV7511_REG_INT(0), irq0);
> > regmap_write(adv7511->regmap, ADV7511_REG_INT(1), irq1);
> >
> > if (process_hpd && irq0 & ADV7511_INT0_HPD &&
> > adv7511->bridge.encoder)
> > schedule_work(>hpd_work);
> >
> > if (irq0 & ADV7511_INT0_EDID_READY || irq1 &
> > ADV7511_INT1_DDC_ERROR) {
> > adv7511->edid_read = true;
> >
> > if (adv7511->i2c_main->irq)
> > wake_up_all(>wq);
> > }
> >
> > #ifdef CONFIG_DRM_I2C_ADV7511_CEC
> > adv7511_cec_irq_process(adv7511, irq1);
> > #endif
> >
> > return 0;
> > }
> >
> > static irqreturn_t adv7511_irq_handler(int irq, void *devid)
> > {
> > struct adv7511 *adv7511 = devid;
> > int ret;
> >
> > ret = adv7511_irq_process(adv7511, true);
> > return ret < 0 ? IRQ_NONE : IRQ_HANDLED;
> > }
> >
> > The function will return IRQ_HANDLED as long as the registers can be
> > read, even if they don't report any interrupt.
> >
> > > adv7511);
> > > if (ret)
> > > goto err_unregister_audio;
--
Regards,
Laurent Pinchart
