[PATCH v2 0/3] Fixes to bridge/panel and ingenic-drm

[PATCH v2 0/3] Fixes to bridge/panel and ingenic-drm

[Paul Cercueil]

Hi,

Here are three independent fixes. The first one addresses a
use-after-free in bridge/panel.c; the second one addresses a
use-after-free in the ingenic-drm driver; finally, the third one makes
the ingenic-drm driver work again on older Ingenic SoCs.

Changes from v1:
- patch [1/3]: the connector is cleaned up only if it was initialized in
  the first place;
- patch [2/3]: use __drmm_simple_encoder_alloc;
- patch [3/3] is unmodified.

Note to linux-stable guys: patch [v2 2/3] will only apply on the current
drm-misc-next branch, to fix it for v5.11 and older kernels, use the V1
of that patch.

Cheers,
-Paul

Paul Cercueil (3):
  drm: bridge/panel: Cleanup connector on bridge detach
  drm/ingenic: Register devm action to cleanup encoders
  drm/ingenic: Fix non-OSD mode

 drivers/gpu/drm/bridge/panel.c            |  6 +++++
 drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 27 ++++++++++++-----------
 2 files changed, 20 insertions(+), 13 deletions(-)

-- 
2.29.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 1/3] drm: bridge/panel: Cleanup connector on bridge detach

[Paul Cercueil]

If we don't call drm_connector_cleanup() manually in
panel_bridge_detach(), the connector will be cleaned up with the other
DRM objects in the call to drm_mode_config_cleanup(). However, since our
drm_connector is devm-allocated, by the time drm_mode_config_cleanup()
will be called, our connector will be long gone. Therefore, the
connector must be cleaned up when the bridge is detached to avoid
use-after-free conditions.

v2: Cleanup connector only if it was created

Fixes: 13dfc0540a57 ("drm/bridge: Refactor out the panel wrapper from the lvds-encoder bridge.")
Cc: <stable@vger.kernel.org> # 4.12+
Cc: Andrzej Hajda <a.hajda@samsung.com>
Cc: Neil Armstrong <narmstrong@baylibre.com>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Jonas Karlman <jonas@kwiboo.se>
Cc: Jernej Skrabec <jernej.skrabec@siol.net>
Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 drivers/gpu/drm/bridge/panel.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c
index 0ddc37551194..df86b0ee0549 100644
--- a/drivers/gpu/drm/bridge/panel.c
+++ b/drivers/gpu/drm/bridge/panel.c
@@ -87,6 +87,12 @@ static int panel_bridge_attach(struct drm_bridge *bridge,
 
 static void panel_bridge_detach(struct drm_bridge *bridge)
 {
+	struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge);
+	struct drm_connector *connector = &panel_bridge->connector;
+
+	/* Cleanup the connector if we know it was initialized */
+	if (!!panel_bridge->connector.dev)
+		drm_connector_cleanup(connector);
 }
 
 static void panel_bridge_pre_enable(struct drm_bridge *bridge)
-- 
2.29.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 2/3] drm/ingenic: Register devm action to cleanup encoders

[Paul Cercueil]

Since the encoders have been devm-allocated, they will be freed way
before drm_mode_config_cleanup() is called. To avoid use-after-free
conditions, we then must ensure that drm_encoder_cleanup() is called
before the encoders are freed.

v2: Use the new __drmm_simple_encoder_alloc() function

Fixes: c369cb27c267 ("drm/ingenic: Support multiple panels/bridges")
Cc: <stable@vger.kernel.org> # 5.8+
Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---

Notes:
    Use the V1 of this patch to fix v5.11 and older kernels. This V2 only
    applies on the current drm-misc-next branch.

 drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
index 7bb31fbee29d..158433b4c084 100644
--- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
+++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
@@ -1014,20 +1014,18 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
 			bridge = devm_drm_panel_bridge_add_typed(dev, panel,
 								 DRM_MODE_CONNECTOR_DPI);
 
-		encoder = devm_kzalloc(dev, sizeof(*encoder), GFP_KERNEL);
-		if (!encoder)
-			return -ENOMEM;
+		encoder = __drmm_simple_encoder_alloc(drm, sizeof(*encoder), 0,
+						      DRM_MODE_ENCODER_DPI);
+		if (IS_ERR(encoder)) {
+			ret = PTR_ERR(encoder);
+			dev_err(dev, "Failed to init encoder: %d\n", ret);
+			return ret;
+		}
 
 		encoder->possible_crtcs = 1;
 
 		drm_encoder_helper_add(encoder, &ingenic_drm_encoder_helper_funcs);
 
-		ret = drm_simple_encoder_init(drm, encoder, DRM_MODE_ENCODER_DPI);
-		if (ret) {
-			dev_err(dev, "Failed to init encoder: %d\n", ret);
-			return ret;
-		}
-
 		ret = drm_bridge_attach(encoder, bridge, NULL, 0);
 		if (ret) {
 			dev_err(dev, "Unable to attach bridge\n");
-- 
2.29.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 3/3] drm/ingenic: Fix non-OSD mode

[Paul Cercueil]

Even though the JZ4740 did not have the OSD mode, it had (according to
the documentation) two DMA channels, but there is absolutely no
information about how to select the second DMA channel.

Make the ingenic-drm driver work in non-OSD mode by using the
foreground0 plane (which is bound to the DMA0 channel) as the primary
plane, instead of the foreground1 plane, which is the primary plane
when in OSD mode.

Fixes: 3c9bea4ef32b ("drm/ingenic: Add support for OSD mode")
Cc: <stable@vger.kernel.org> # v5.8+
Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
index 158433b4c084..963dcbfeaba2 100644
--- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
+++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
@@ -554,7 +554,7 @@ static void ingenic_drm_plane_atomic_update(struct drm_plane *plane,
 		height = state->src_h >> 16;
 		cpp = state->fb->format->cpp[0];
 
-		if (priv->soc_info->has_osd && plane->type == DRM_PLANE_TYPE_OVERLAY)
+		if (!priv->soc_info->has_osd || plane->type == DRM_PLANE_TYPE_OVERLAY)
 			hwdesc = &priv->dma_hwdescs->hwdesc_f0;
 		else
 			hwdesc = &priv->dma_hwdescs->hwdesc_f1;
@@ -826,6 +826,7 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
 	const struct jz_soc_info *soc_info;
 	struct ingenic_drm *priv;
 	struct clk *parent_clk;
+	struct drm_plane *primary;
 	struct drm_bridge *bridge;
 	struct drm_panel *panel;
 	struct drm_encoder *encoder;
@@ -940,9 +941,11 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
 	if (soc_info->has_osd)
 		priv->ipu_plane = drm_plane_from_index(drm, 0);
 
-	drm_plane_helper_add(&priv->f1, &ingenic_drm_plane_helper_funcs);
+	primary = priv->soc_info->has_osd ? &priv->f1 : &priv->f0;
 
-	ret = drm_universal_plane_init(drm, &priv->f1, 1,
+	drm_plane_helper_add(primary, &ingenic_drm_plane_helper_funcs);
+
+	ret = drm_universal_plane_init(drm, primary, 1,
 				       &ingenic_drm_primary_plane_funcs,
 				       priv->soc_info->formats_f1,
 				       priv->soc_info->num_formats_f1,
@@ -954,7 +957,7 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
 
 	drm_crtc_helper_add(&priv->crtc, &ingenic_drm_crtc_helper_funcs);
 
-	ret = drm_crtc_init_with_planes(drm, &priv->crtc, &priv->f1,
+	ret = drm_crtc_init_with_planes(drm, &priv->crtc, primary,
 					NULL, &ingenic_drm_crtc_funcs, NULL);
 	if (ret) {
 		dev_err(dev, "Failed to init CRTC: %i\n", ret);
-- 
2.29.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 2/3] drm/ingenic: Register devm action to cleanup encoders

[Daniel Vetter]

On Wed, Jan 20, 2021 at 1:36 PM Paul Cercueil <paul@crapouillou.net> wrote:
>
> Since the encoders have been devm-allocated, they will be freed way
> before drm_mode_config_cleanup() is called. To avoid use-after-free
> conditions, we then must ensure that drm_encoder_cleanup() is called
> before the encoders are freed.
>
> v2: Use the new __drmm_simple_encoder_alloc() function
>
> Fixes: c369cb27c267 ("drm/ingenic: Support multiple panels/bridges")
> Cc: <stable@vger.kernel.org> # 5.8+
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>
> Notes:
>     Use the V1 of this patch to fix v5.11 and older kernels. This V2 only
>     applies on the current drm-misc-next branch.
>
>  drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 16 +++++++---------
>  1 file changed, 7 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> index 7bb31fbee29d..158433b4c084 100644
> --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> @@ -1014,20 +1014,18 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
>                         bridge = devm_drm_panel_bridge_add_typed(dev, panel,
>                                                                  DRM_MODE_CONNECTOR_DPI);
>
> -               encoder = devm_kzalloc(dev, sizeof(*encoder), GFP_KERNEL);
> -               if (!encoder)
> -                       return -ENOMEM;
> +               encoder = __drmm_simple_encoder_alloc(drm, sizeof(*encoder), 0,

Please don't use the __ prefixed functions, those are the internal
ones. The official one comes with type checking and all that included.
Otherwise lgtm.
-Daniel

> +                                                     DRM_MODE_ENCODER_DPI);
> +               if (IS_ERR(encoder)) {
> +                       ret = PTR_ERR(encoder);
> +                       dev_err(dev, "Failed to init encoder: %d\n", ret);
> +                       return ret;
> +               }
>
>                 encoder->possible_crtcs = 1;
>
>                 drm_encoder_helper_add(encoder, &ingenic_drm_encoder_helper_funcs);
>
> -               ret = drm_simple_encoder_init(drm, encoder, DRM_MODE_ENCODER_DPI);
> -               if (ret) {
> -                       dev_err(dev, "Failed to init encoder: %d\n", ret);
> -                       return ret;
> -               }
> -
>                 ret = drm_bridge_attach(encoder, bridge, NULL, 0);
>                 if (ret) {
>                         dev_err(dev, "Unable to attach bridge\n");
> --
> 2.29.2
>


-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 2/3] drm/ingenic: Register devm action to cleanup encoders

[Paul Cercueil]

Le mer. 20 janv. 2021 à 14:01, Daniel Vetter <daniel@ffwll.ch> a 
écrit :
> On Wed, Jan 20, 2021 at 1:36 PM Paul Cercueil <paul@crapouillou.net> 
> wrote:
>> 
>>  Since the encoders have been devm-allocated, they will be freed way
>>  before drm_mode_config_cleanup() is called. To avoid use-after-free
>>  conditions, we then must ensure that drm_encoder_cleanup() is called
>>  before the encoders are freed.
>> 
>>  v2: Use the new __drmm_simple_encoder_alloc() function
>> 
>>  Fixes: c369cb27c267 ("drm/ingenic: Support multiple panels/bridges")
>>  Cc: <stable@vger.kernel.org> # 5.8+
>>  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>  ---
>> 
>>  Notes:
>>      Use the V1 of this patch to fix v5.11 and older kernels. This 
>> V2 only
>>      applies on the current drm-misc-next branch.
>> 
>>   drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 16 +++++++---------
>>   1 file changed, 7 insertions(+), 9 deletions(-)
>> 
>>  diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c 
>> b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>>  index 7bb31fbee29d..158433b4c084 100644
>>  --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>>  +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>>  @@ -1014,20 +1014,18 @@ static int ingenic_drm_bind(struct device 
>> *dev, bool has_components)
>>                          bridge = 
>> devm_drm_panel_bridge_add_typed(dev, panel,
>>                                                                   
>> DRM_MODE_CONNECTOR_DPI);
>> 
>>  -               encoder = devm_kzalloc(dev, sizeof(*encoder), 
>> GFP_KERNEL);
>>  -               if (!encoder)
>>  -                       return -ENOMEM;
>>  +               encoder = __drmm_simple_encoder_alloc(drm, 
>> sizeof(*encoder), 0,
> 
> Please don't use the __ prefixed functions, those are the internal
> ones. The official one comes with type checking and all that included.
> Otherwise lgtm.
> -Daniel

The non-prefixed one assumes that I want to allocate a struct that 
contains the encoder, not just the drm_encoder itself.

-Paul

>>  +                                                     
>> DRM_MODE_ENCODER_DPI);
>>  +               if (IS_ERR(encoder)) {
>>  +                       ret = PTR_ERR(encoder);
>>  +                       dev_err(dev, "Failed to init encoder: 
>> %d\n", ret);
>>  +                       return ret;
>>  +               }
>> 
>>                  encoder->possible_crtcs = 1;
>> 
>>                  drm_encoder_helper_add(encoder, 
>> &ingenic_drm_encoder_helper_funcs);
>> 
>>  -               ret = drm_simple_encoder_init(drm, encoder, 
>> DRM_MODE_ENCODER_DPI);
>>  -               if (ret) {
>>  -                       dev_err(dev, "Failed to init encoder: 
>> %d\n", ret);
>>  -                       return ret;
>>  -               }
>>  -
>>                  ret = drm_bridge_attach(encoder, bridge, NULL, 0);
>>                  if (ret) {
>>                          dev_err(dev, "Unable to attach bridge\n");
>>  --
>>  2.29.2
>> 
> 
> 
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 2/3] drm/ingenic: Register devm action to cleanup encoders

[Daniel Vetter]

On Wed, Jan 20, 2021 at 2:21 PM Paul Cercueil <paul@crapouillou.net> wrote:
>
>
>
> Le mer. 20 janv. 2021 à 14:01, Daniel Vetter <daniel@ffwll.ch> a
> écrit :
> > On Wed, Jan 20, 2021 at 1:36 PM Paul Cercueil <paul@crapouillou.net>
> > wrote:
> >>
> >>  Since the encoders have been devm-allocated, they will be freed way
> >>  before drm_mode_config_cleanup() is called. To avoid use-after-free
> >>  conditions, we then must ensure that drm_encoder_cleanup() is called
> >>  before the encoders are freed.
> >>
> >>  v2: Use the new __drmm_simple_encoder_alloc() function
> >>
> >>  Fixes: c369cb27c267 ("drm/ingenic: Support multiple panels/bridges")
> >>  Cc: <stable@vger.kernel.org> # 5.8+
> >>  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> >>  ---
> >>
> >>  Notes:
> >>      Use the V1 of this patch to fix v5.11 and older kernels. This
> >> V2 only
> >>      applies on the current drm-misc-next branch.
> >>
> >>   drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 16 +++++++---------
> >>   1 file changed, 7 insertions(+), 9 deletions(-)
> >>
> >>  diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> >> b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> >>  index 7bb31fbee29d..158433b4c084 100644
> >>  --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> >>  +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> >>  @@ -1014,20 +1014,18 @@ static int ingenic_drm_bind(struct device
> >> *dev, bool has_components)
> >>                          bridge =
> >> devm_drm_panel_bridge_add_typed(dev, panel,
> >>
> >> DRM_MODE_CONNECTOR_DPI);
> >>
> >>  -               encoder = devm_kzalloc(dev, sizeof(*encoder),
> >> GFP_KERNEL);
> >>  -               if (!encoder)
> >>  -                       return -ENOMEM;
> >>  +               encoder = __drmm_simple_encoder_alloc(drm,
> >> sizeof(*encoder), 0,
> >
> > Please don't use the __ prefixed functions, those are the internal
> > ones. The official one comes with type checking and all that included.
> > Otherwise lgtm.
> > -Daniel
>
> The non-prefixed one assumes that I want to allocate a struct that
> contains the encoder, not just the drm_encoder itself.

Hm, but using the internal one is also a bit too ugly. A
drm_plain_simple_enocder_alloc(drm, type) wrapper would be the right
thing here I think? Setting the offsets and struct sizes directly in
these in drivers really doesn't feel like a good idea. I think simple
encoder is the only case where we really have a need for a
non-embeddable struct.
-Daniel

>
> -Paul
>
> >>  +
> >> DRM_MODE_ENCODER_DPI);
> >>  +               if (IS_ERR(encoder)) {
> >>  +                       ret = PTR_ERR(encoder);
> >>  +                       dev_err(dev, "Failed to init encoder:
> >> %d\n", ret);
> >>  +                       return ret;
> >>  +               }
> >>
> >>                  encoder->possible_crtcs = 1;
> >>
> >>                  drm_encoder_helper_add(encoder,
> >> &ingenic_drm_encoder_helper_funcs);
> >>
> >>  -               ret = drm_simple_encoder_init(drm, encoder,
> >> DRM_MODE_ENCODER_DPI);
> >>  -               if (ret) {
> >>  -                       dev_err(dev, "Failed to init encoder:
> >> %d\n", ret);
> >>  -                       return ret;
> >>  -               }
> >>  -
> >>                  ret = drm_bridge_attach(encoder, bridge, NULL, 0);
> >>                  if (ret) {
> >>                          dev_err(dev, "Unable to attach bridge\n");
> >>  --
> >>  2.29.2
> >>
> >
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
>
>


-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 2/3] drm/ingenic: Register devm action to cleanup encoders

[Paul Cercueil]

Le mer. 20 janv. 2021 à 15:04, Daniel Vetter <daniel@ffwll.ch> a 
écrit :
> On Wed, Jan 20, 2021 at 2:21 PM Paul Cercueil <paul@crapouillou.net> 
> wrote:
>> 
>> 
>> 
>>  Le mer. 20 janv. 2021 à 14:01, Daniel Vetter <daniel@ffwll.ch> a
>>  écrit :
>>  > On Wed, Jan 20, 2021 at 1:36 PM Paul Cercueil 
>> <paul@crapouillou.net>
>>  > wrote:
>>  >>
>>  >>  Since the encoders have been devm-allocated, they will be freed 
>> way
>>  >>  before drm_mode_config_cleanup() is called. To avoid 
>> use-after-free
>>  >>  conditions, we then must ensure that drm_encoder_cleanup() is 
>> called
>>  >>  before the encoders are freed.
>>  >>
>>  >>  v2: Use the new __drmm_simple_encoder_alloc() function
>>  >>
>>  >>  Fixes: c369cb27c267 ("drm/ingenic: Support multiple 
>> panels/bridges")
>>  >>  Cc: <stable@vger.kernel.org> # 5.8+
>>  >>  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>  >>  ---
>>  >>
>>  >>  Notes:
>>  >>      Use the V1 of this patch to fix v5.11 and older kernels. 
>> This
>>  >> V2 only
>>  >>      applies on the current drm-misc-next branch.
>>  >>
>>  >>   drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 16 +++++++---------
>>  >>   1 file changed, 7 insertions(+), 9 deletions(-)
>>  >>
>>  >>  diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>>  >> b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>>  >>  index 7bb31fbee29d..158433b4c084 100644
>>  >>  --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>>  >>  +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>>  >>  @@ -1014,20 +1014,18 @@ static int ingenic_drm_bind(struct 
>> device
>>  >> *dev, bool has_components)
>>  >>                          bridge =
>>  >> devm_drm_panel_bridge_add_typed(dev, panel,
>>  >>
>>  >> DRM_MODE_CONNECTOR_DPI);
>>  >>
>>  >>  -               encoder = devm_kzalloc(dev, sizeof(*encoder),
>>  >> GFP_KERNEL);
>>  >>  -               if (!encoder)
>>  >>  -                       return -ENOMEM;
>>  >>  +               encoder = __drmm_simple_encoder_alloc(drm,
>>  >> sizeof(*encoder), 0,
>>  >
>>  > Please don't use the __ prefixed functions, those are the internal
>>  > ones. The official one comes with type checking and all that 
>> included.
>>  > Otherwise lgtm.
>>  > -Daniel
>> 
>>  The non-prefixed one assumes that I want to allocate a struct that
>>  contains the encoder, not just the drm_encoder itself.
> 
> Hm, but using the internal one is also a bit too ugly. A
> drm_plain_simple_enocder_alloc(drm, type) wrapper would be the right
> thing here I think? Setting the offsets and struct sizes directly in
> these in drivers really doesn't feel like a good idea. I think simple
> encoder is the only case where we really have a need for a
> non-embeddable struct.
> -Daniel

Alright, I will add a wrapper.

Cheers,
-Paul

>> 
>>  >>  +
>>  >> DRM_MODE_ENCODER_DPI);
>>  >>  +               if (IS_ERR(encoder)) {
>>  >>  +                       ret = PTR_ERR(encoder);
>>  >>  +                       dev_err(dev, "Failed to init encoder:
>>  >> %d\n", ret);
>>  >>  +                       return ret;
>>  >>  +               }
>>  >>
>>  >>                  encoder->possible_crtcs = 1;
>>  >>
>>  >>                  drm_encoder_helper_add(encoder,
>>  >> &ingenic_drm_encoder_helper_funcs);
>>  >>
>>  >>  -               ret = drm_simple_encoder_init(drm, encoder,
>>  >> DRM_MODE_ENCODER_DPI);
>>  >>  -               if (ret) {
>>  >>  -                       dev_err(dev, "Failed to init encoder:
>>  >> %d\n", ret);
>>  >>  -                       return ret;
>>  >>  -               }
>>  >>  -
>>  >>                  ret = drm_bridge_attach(encoder, bridge, NULL, 
>> 0);
>>  >>                  if (ret) {
>>  >>                          dev_err(dev, "Unable to attach 
>> bridge\n");
>>  >>  --
>>  >>  2.29.2
>>  >>
>>  >
>>  >
>>  > --
>>  > Daniel Vetter
>>  > Software Engineer, Intel Corporation
>>  > http://blog.ffwll.ch
>> 
>> 
> 
> 
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 1/3] drm: bridge/panel: Cleanup connector on bridge detach

[Daniel Vetter]

On Wed, Jan 20, 2021 at 1:35 PM Paul Cercueil <paul@crapouillou.net> wrote:
>
> If we don't call drm_connector_cleanup() manually in
> panel_bridge_detach(), the connector will be cleaned up with the other
> DRM objects in the call to drm_mode_config_cleanup(). However, since our
> drm_connector is devm-allocated, by the time drm_mode_config_cleanup()
> will be called, our connector will be long gone. Therefore, the
> connector must be cleaned up when the bridge is detached to avoid
> use-after-free conditions.

For -fixes this sounds ok, but for -next I think switching to drmm_
would be much better.
-Daniel

> v2: Cleanup connector only if it was created
>
> Fixes: 13dfc0540a57 ("drm/bridge: Refactor out the panel wrapper from the lvds-encoder bridge.")
> Cc: <stable@vger.kernel.org> # 4.12+
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Cc: Neil Armstrong <narmstrong@baylibre.com>
> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> Cc: Jonas Karlman <jonas@kwiboo.se>
> Cc: Jernej Skrabec <jernej.skrabec@siol.net>
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>  drivers/gpu/drm/bridge/panel.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c
> index 0ddc37551194..df86b0ee0549 100644
> --- a/drivers/gpu/drm/bridge/panel.c
> +++ b/drivers/gpu/drm/bridge/panel.c
> @@ -87,6 +87,12 @@ static int panel_bridge_attach(struct drm_bridge *bridge,
>
>  static void panel_bridge_detach(struct drm_bridge *bridge)
>  {
> +       struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge);
> +       struct drm_connector *connector = &panel_bridge->connector;
> +
> +       /* Cleanup the connector if we know it was initialized */
> +       if (!!panel_bridge->connector.dev)
> +               drm_connector_cleanup(connector);
>  }
>
>  static void panel_bridge_pre_enable(struct drm_bridge *bridge)
> --
> 2.29.2
>


-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 1/3] drm: bridge/panel: Cleanup connector on bridge detach

[Paul Cercueil]

Le mer. 20 janv. 2021 à 17:03, Daniel Vetter <daniel@ffwll.ch> a 
écrit :
> On Wed, Jan 20, 2021 at 1:35 PM Paul Cercueil <paul@crapouillou.net> 
> wrote:
>> 
>>  If we don't call drm_connector_cleanup() manually in
>>  panel_bridge_detach(), the connector will be cleaned up with the 
>> other
>>  DRM objects in the call to drm_mode_config_cleanup(). However, 
>> since our
>>  drm_connector is devm-allocated, by the time 
>> drm_mode_config_cleanup()
>>  will be called, our connector will be long gone. Therefore, the
>>  connector must be cleaned up when the bridge is detached to avoid
>>  use-after-free conditions.
> 
> For -fixes this sounds ok, but for -next I think switching to drmm_
> would be much better.

The API would need to change to have access to the drm_device struct, 
though. That would be quite a big patch, there are a few dozens source 
files that use this API already.

Cheers,
-Paul

> 
>>  v2: Cleanup connector only if it was created
>> 
>>  Fixes: 13dfc0540a57 ("drm/bridge: Refactor out the panel wrapper 
>> from the lvds-encoder bridge.")
>>  Cc: <stable@vger.kernel.org> # 4.12+
>>  Cc: Andrzej Hajda <a.hajda@samsung.com>
>>  Cc: Neil Armstrong <narmstrong@baylibre.com>
>>  Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
>>  Cc: Jonas Karlman <jonas@kwiboo.se>
>>  Cc: Jernej Skrabec <jernej.skrabec@siol.net>
>>  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>  ---
>>   drivers/gpu/drm/bridge/panel.c | 6 ++++++
>>   1 file changed, 6 insertions(+)
>> 
>>  diff --git a/drivers/gpu/drm/bridge/panel.c 
>> b/drivers/gpu/drm/bridge/panel.c
>>  index 0ddc37551194..df86b0ee0549 100644
>>  --- a/drivers/gpu/drm/bridge/panel.c
>>  +++ b/drivers/gpu/drm/bridge/panel.c
>>  @@ -87,6 +87,12 @@ static int panel_bridge_attach(struct drm_bridge 
>> *bridge,
>> 
>>   static void panel_bridge_detach(struct drm_bridge *bridge)
>>   {
>>  +       struct panel_bridge *panel_bridge = 
>> drm_bridge_to_panel_bridge(bridge);
>>  +       struct drm_connector *connector = &panel_bridge->connector;
>>  +
>>  +       /* Cleanup the connector if we know it was initialized */
>>  +       if (!!panel_bridge->connector.dev)
>>  +               drm_connector_cleanup(connector);
>>   }
>> 
>>   static void panel_bridge_pre_enable(struct drm_bridge *bridge)
>>  --
>>  2.29.2
>> 
> 
> 
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 3/3] drm/ingenic: Fix non-OSD mode

[Daniel Vetter]

On Wed, Jan 20, 2021 at 1:36 PM Paul Cercueil <paul@crapouillou.net> wrote:
>
> Even though the JZ4740 did not have the OSD mode, it had (according to
> the documentation) two DMA channels, but there is absolutely no
> information about how to select the second DMA channel.
>
> Make the ingenic-drm driver work in non-OSD mode by using the
> foreground0 plane (which is bound to the DMA0 channel) as the primary
> plane, instead of the foreground1 plane, which is the primary plane
> when in OSD mode.
>
> Fixes: 3c9bea4ef32b ("drm/ingenic: Add support for OSD mode")
> Cc: <stable@vger.kernel.org> # v5.8+
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>

Does what it says on the tin^Wcommit message.

Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> index 158433b4c084..963dcbfeaba2 100644
> --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> @@ -554,7 +554,7 @@ static void ingenic_drm_plane_atomic_update(struct drm_plane *plane,
>                 height = state->src_h >> 16;
>                 cpp = state->fb->format->cpp[0];
>
> -               if (priv->soc_info->has_osd && plane->type == DRM_PLANE_TYPE_OVERLAY)
> +               if (!priv->soc_info->has_osd || plane->type == DRM_PLANE_TYPE_OVERLAY)
>                         hwdesc = &priv->dma_hwdescs->hwdesc_f0;
>                 else
>                         hwdesc = &priv->dma_hwdescs->hwdesc_f1;
> @@ -826,6 +826,7 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
>         const struct jz_soc_info *soc_info;
>         struct ingenic_drm *priv;
>         struct clk *parent_clk;
> +       struct drm_plane *primary;
>         struct drm_bridge *bridge;
>         struct drm_panel *panel;
>         struct drm_encoder *encoder;
> @@ -940,9 +941,11 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
>         if (soc_info->has_osd)
>                 priv->ipu_plane = drm_plane_from_index(drm, 0);
>
> -       drm_plane_helper_add(&priv->f1, &ingenic_drm_plane_helper_funcs);
> +       primary = priv->soc_info->has_osd ? &priv->f1 : &priv->f0;
>
> -       ret = drm_universal_plane_init(drm, &priv->f1, 1,
> +       drm_plane_helper_add(primary, &ingenic_drm_plane_helper_funcs);
> +
> +       ret = drm_universal_plane_init(drm, primary, 1,
>                                        &ingenic_drm_primary_plane_funcs,
>                                        priv->soc_info->formats_f1,
>                                        priv->soc_info->num_formats_f1,
> @@ -954,7 +957,7 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
>
>         drm_crtc_helper_add(&priv->crtc, &ingenic_drm_crtc_helper_funcs);
>
> -       ret = drm_crtc_init_with_planes(drm, &priv->crtc, &priv->f1,
> +       ret = drm_crtc_init_with_planes(drm, &priv->crtc, primary,
>                                         NULL, &ingenic_drm_crtc_funcs, NULL);
>         if (ret) {
>                 dev_err(dev, "Failed to init CRTC: %i\n", ret);
> --
> 2.29.2
>


-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 1/3] drm: bridge/panel: Cleanup connector on bridge detach

[Daniel Vetter]

On Wed, Jan 20, 2021 at 6:12 PM Paul Cercueil <paul@crapouillou.net> wrote:
>
>
>
> Le mer. 20 janv. 2021 à 17:03, Daniel Vetter <daniel@ffwll.ch> a
> écrit :
> > On Wed, Jan 20, 2021 at 1:35 PM Paul Cercueil <paul@crapouillou.net>
> > wrote:
> >>
> >>  If we don't call drm_connector_cleanup() manually in
> >>  panel_bridge_detach(), the connector will be cleaned up with the
> >> other
> >>  DRM objects in the call to drm_mode_config_cleanup(). However,
> >> since our
> >>  drm_connector is devm-allocated, by the time
> >> drm_mode_config_cleanup()
> >>  will be called, our connector will be long gone. Therefore, the
> >>  connector must be cleaned up when the bridge is detached to avoid
> >>  use-after-free conditions.
> >
> > For -fixes this sounds ok, but for -next I think switching to drmm_
> > would be much better.
>
> The API would need to change to have access to the drm_device struct,
> though. That would be quite a big patch, there are a few dozens source
> files that use this API already.

Hm right pure drmm_ doesn't work for panel or bridge since it's
usually a separate driver. But devm_ also doesn't work. I think what
we need here is two-stage: first kmalloc the panel (or bridge, it's
really the same) in the panel/bridge driver load. Then when we bind it
to the drm_device we can tie it into the managed resources with
drmm_add_action_or_reset. Passing the drm_device to the point where we
allocate the panel/bridge doesn't work for these.

I think minimally we need a FIXME here and ack from Laurent on how
this should be solved at least, since panel bridge is used rather
widely.
-Daniel

>
> Cheers,
> -Paul
>
> >
> >>  v2: Cleanup connector only if it was created
> >>
> >>  Fixes: 13dfc0540a57 ("drm/bridge: Refactor out the panel wrapper
> >> from the lvds-encoder bridge.")
> >>  Cc: <stable@vger.kernel.org> # 4.12+
> >>  Cc: Andrzej Hajda <a.hajda@samsung.com>
> >>  Cc: Neil Armstrong <narmstrong@baylibre.com>
> >>  Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> >>  Cc: Jonas Karlman <jonas@kwiboo.se>
> >>  Cc: Jernej Skrabec <jernej.skrabec@siol.net>
> >>  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> >>  ---
> >>   drivers/gpu/drm/bridge/panel.c | 6 ++++++
> >>   1 file changed, 6 insertions(+)
> >>
> >>  diff --git a/drivers/gpu/drm/bridge/panel.c
> >> b/drivers/gpu/drm/bridge/panel.c
> >>  index 0ddc37551194..df86b0ee0549 100644
> >>  --- a/drivers/gpu/drm/bridge/panel.c
> >>  +++ b/drivers/gpu/drm/bridge/panel.c
> >>  @@ -87,6 +87,12 @@ static int panel_bridge_attach(struct drm_bridge
> >> *bridge,
> >>
> >>   static void panel_bridge_detach(struct drm_bridge *bridge)
> >>   {
> >>  +       struct panel_bridge *panel_bridge =
> >> drm_bridge_to_panel_bridge(bridge);
> >>  +       struct drm_connector *connector = &panel_bridge->connector;
> >>  +
> >>  +       /* Cleanup the connector if we know it was initialized */
> >>  +       if (!!panel_bridge->connector.dev)
> >>  +               drm_connector_cleanup(connector);
> >>   }
> >>
> >>   static void panel_bridge_pre_enable(struct drm_bridge *bridge)
> >>  --
> >>  2.29.2
> >>
> >
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
>
>


-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 1/3] drm: bridge/panel: Cleanup connector on bridge detach

[Laurent Pinchart]

On Wed, Jan 20, 2021 at 06:38:03PM +0100, Daniel Vetter wrote:
> On Wed, Jan 20, 2021 at 6:12 PM Paul Cercueil wrote:
> > Le mer. 20 janv. 2021 à 17:03, Daniel Vetter a écrit :
> > > On Wed, Jan 20, 2021 at 1:35 PM Paul Cercueil wrote:
> > >>
> > >>  If we don't call drm_connector_cleanup() manually in
> > >>  panel_bridge_detach(), the connector will be cleaned up with the other
> > >>  DRM objects in the call to drm_mode_config_cleanup(). However, since our
> > >>  drm_connector is devm-allocated, by the time drm_mode_config_cleanup()
> > >>  will be called, our connector will be long gone. Therefore, the
> > >>  connector must be cleaned up when the bridge is detached to avoid
> > >>  use-after-free conditions.
> > >
> > > For -fixes this sounds ok, but for -next I think switching to drmm_
> > > would be much better.
> >
> > The API would need to change to have access to the drm_device struct,
> > though. That would be quite a big patch, there are a few dozens source
> > files that use this API already.
> 
> Hm right pure drmm_ doesn't work for panel or bridge since it's
> usually a separate driver. But devm_ also doesn't work. I think what
> we need here is two-stage: first kmalloc the panel (or bridge, it's
> really the same) in the panel/bridge driver load. Then when we bind it
> to the drm_device we can tie it into the managed resources with
> drmm_add_action_or_reset. Passing the drm_device to the point where we
> allocate the panel/bridge doesn't work for these.
> 
> I think minimally we need a FIXME here and ack from Laurent on how
> this should be solved at least, since panel bridge is used rather
> widely.

Bridge removal is completely broken. If you unbind a bridge driver from
the device, the bridge will be unregistered and resources freed, without
the display driver knowing about this. The lifetime of the drm_bridge
structure itself isn't the only issue to be addressed here, it's broader
than that, and needs to consider that the display driver could be
calling the bridge operations concurrently to the removal.

We need a volunteer with enough motivation to solve this subsystem-wide
:-) In the meantime, whatever shortcut addresses immediate issues is
probably fine, as yak-shaving in this area would definitely not be
reasonable.

> > >> v2: Cleanup connector only if it was created
> > >>
> > >> Fixes: 13dfc0540a57 ("drm/bridge: Refactor out the panel wrapper from the lvds-encoder bridge.")
> > >> Cc: <stable@vger.kernel.org> # 4.12+
> > >> Cc: Andrzej Hajda <a.hajda@samsung.com>
> > >> Cc: Neil Armstrong <narmstrong@baylibre.com>
> > >> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> > >> Cc: Jonas Karlman <jonas@kwiboo.se>
> > >> Cc: Jernej Skrabec <jernej.skrabec@siol.net>
> > >> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> > >> ---
> > >>  drivers/gpu/drm/bridge/panel.c | 6 ++++++
> > >>  1 file changed, 6 insertions(+)
> > >>
> > >> diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c
> > >> index 0ddc37551194..df86b0ee0549 100644
> > >> --- a/drivers/gpu/drm/bridge/panel.c
> > >> +++ b/drivers/gpu/drm/bridge/panel.c
> > >> @@ -87,6 +87,12 @@ static int panel_bridge_attach(struct drm_bridge *bridge,
> > >>
> > >>  static void panel_bridge_detach(struct drm_bridge *bridge)
> > >>  {
> > >> +	struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge);
> > >> +	struct drm_connector *connector = &panel_bridge->connector;
> > >> +
> > >> +	/* Cleanup the connector if we know it was initialized */
> > >> +	if (!!panel_bridge->connector.dev)
> > >> +		drm_connector_cleanup(connector);
> > >>  }
> > >>
> > >>  static void panel_bridge_pre_enable(struct drm_bridge *bridge)

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 1/3] drm: bridge/panel: Cleanup connector on bridge detach

[Daniel Vetter]

On Wed, Mar 24, 2021 at 04:15:37AM +0200, Laurent Pinchart wrote:
> On Wed, Jan 20, 2021 at 06:38:03PM +0100, Daniel Vetter wrote:
> > On Wed, Jan 20, 2021 at 6:12 PM Paul Cercueil wrote:
> > > Le mer. 20 janv. 2021 à 17:03, Daniel Vetter a écrit :
> > > > On Wed, Jan 20, 2021 at 1:35 PM Paul Cercueil wrote:
> > > >>
> > > >>  If we don't call drm_connector_cleanup() manually in
> > > >>  panel_bridge_detach(), the connector will be cleaned up with the other
> > > >>  DRM objects in the call to drm_mode_config_cleanup(). However, since our
> > > >>  drm_connector is devm-allocated, by the time drm_mode_config_cleanup()
> > > >>  will be called, our connector will be long gone. Therefore, the
> > > >>  connector must be cleaned up when the bridge is detached to avoid
> > > >>  use-after-free conditions.
> > > >
> > > > For -fixes this sounds ok, but for -next I think switching to drmm_
> > > > would be much better.
> > >
> > > The API would need to change to have access to the drm_device struct,
> > > though. That would be quite a big patch, there are a few dozens source
> > > files that use this API already.
> > 
> > Hm right pure drmm_ doesn't work for panel or bridge since it's
> > usually a separate driver. But devm_ also doesn't work. I think what
> > we need here is two-stage: first kmalloc the panel (or bridge, it's
> > really the same) in the panel/bridge driver load. Then when we bind it
> > to the drm_device we can tie it into the managed resources with
> > drmm_add_action_or_reset. Passing the drm_device to the point where we
> > allocate the panel/bridge doesn't work for these.
> > 
> > I think minimally we need a FIXME here and ack from Laurent on how
> > this should be solved at least, since panel bridge is used rather
> > widely.
> 
> Bridge removal is completely broken. If you unbind a bridge driver from
> the device, the bridge will be unregistered and resources freed, without
> the display driver knowing about this. The lifetime of the drm_bridge
> structure itself isn't the only issue to be addressed here, it's broader
> than that, and needs to consider that the display driver could be
> calling the bridge operations concurrently to the removal.

So for the "unloading bridge should first unload display" problem that was
supposed to get fixed with device links. There was at least a patch for
that, and I Rafel from pm side did all the core changes to make it work.
But it didn't land I think, so things keep on sucking.

Ofc the lifetime of the bridge structure is then an additional problem on
top here.

> We need a volunteer with enough motivation to solve this subsystem-wide
> :-) In the meantime, whatever shortcut addresses immediate issues is
> probably fine, as yak-shaving in this area would definitely not be
> reasonable.

I guess drm/bridge keeps on disappointing :-/
-Daniel

> 
> > > >> v2: Cleanup connector only if it was created
> > > >>
> > > >> Fixes: 13dfc0540a57 ("drm/bridge: Refactor out the panel wrapper from the lvds-encoder bridge.")
> > > >> Cc: <stable@vger.kernel.org> # 4.12+
> > > >> Cc: Andrzej Hajda <a.hajda@samsung.com>
> > > >> Cc: Neil Armstrong <narmstrong@baylibre.com>
> > > >> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> > > >> Cc: Jonas Karlman <jonas@kwiboo.se>
> > > >> Cc: Jernej Skrabec <jernej.skrabec@siol.net>
> > > >> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> > > >> ---
> > > >>  drivers/gpu/drm/bridge/panel.c | 6 ++++++
> > > >>  1 file changed, 6 insertions(+)
> > > >>
> > > >> diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c
> > > >> index 0ddc37551194..df86b0ee0549 100644
> > > >> --- a/drivers/gpu/drm/bridge/panel.c
> > > >> +++ b/drivers/gpu/drm/bridge/panel.c
> > > >> @@ -87,6 +87,12 @@ static int panel_bridge_attach(struct drm_bridge *bridge,
> > > >>
> > > >>  static void panel_bridge_detach(struct drm_bridge *bridge)
> > > >>  {
> > > >> +	struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge);
> > > >> +	struct drm_connector *connector = &panel_bridge->connector;
> > > >> +
> > > >> +	/* Cleanup the connector if we know it was initialized */
> > > >> +	if (!!panel_bridge->connector.dev)
> > > >> +		drm_connector_cleanup(connector);
> > > >>  }
> > > >>
> > > >>  static void panel_bridge_pre_enable(struct drm_bridge *bridge)
> 
> -- 
> Regards,
> 
> Laurent Pinchart

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 1/3] drm: bridge/panel: Cleanup connector on bridge detach

[Laurent Pinchart]

On Wed, Mar 24, 2021 at 10:39:52AM +0100, Daniel Vetter wrote:
> On Wed, Mar 24, 2021 at 04:15:37AM +0200, Laurent Pinchart wrote:
> > On Wed, Jan 20, 2021 at 06:38:03PM +0100, Daniel Vetter wrote:
> > > On Wed, Jan 20, 2021 at 6:12 PM Paul Cercueil wrote:
> > > > Le mer. 20 janv. 2021 à 17:03, Daniel Vetter a écrit :
> > > > > On Wed, Jan 20, 2021 at 1:35 PM Paul Cercueil wrote:
> > > > >>
> > > > >>  If we don't call drm_connector_cleanup() manually in
> > > > >>  panel_bridge_detach(), the connector will be cleaned up with the other
> > > > >>  DRM objects in the call to drm_mode_config_cleanup(). However, since our
> > > > >>  drm_connector is devm-allocated, by the time drm_mode_config_cleanup()
> > > > >>  will be called, our connector will be long gone. Therefore, the
> > > > >>  connector must be cleaned up when the bridge is detached to avoid
> > > > >>  use-after-free conditions.
> > > > >
> > > > > For -fixes this sounds ok, but for -next I think switching to drmm_
> > > > > would be much better.
> > > >
> > > > The API would need to change to have access to the drm_device struct,
> > > > though. That would be quite a big patch, there are a few dozens source
> > > > files that use this API already.
> > > 
> > > Hm right pure drmm_ doesn't work for panel or bridge since it's
> > > usually a separate driver. But devm_ also doesn't work. I think what
> > > we need here is two-stage: first kmalloc the panel (or bridge, it's
> > > really the same) in the panel/bridge driver load. Then when we bind it
> > > to the drm_device we can tie it into the managed resources with
> > > drmm_add_action_or_reset. Passing the drm_device to the point where we
> > > allocate the panel/bridge doesn't work for these.
> > > 
> > > I think minimally we need a FIXME here and ack from Laurent on how
> > > this should be solved at least, since panel bridge is used rather
> > > widely.
> > 
> > Bridge removal is completely broken. If you unbind a bridge driver from
> > the device, the bridge will be unregistered and resources freed, without
> > the display driver knowing about this. The lifetime of the drm_bridge
> > structure itself isn't the only issue to be addressed here, it's broader
> > than that, and needs to consider that the display driver could be
> > calling the bridge operations concurrently to the removal.
> 
> So for the "unloading bridge should first unload display" problem that was
> supposed to get fixed with device links. There was at least a patch for
> that, and I Rafel from pm side did all the core changes to make it work.
> But it didn't land I think, so things keep on sucking.
> 
> Ofc the lifetime of the bridge structure is then an additional problem on
> top here.

There's a set of interesting problems. I don't think it's impossible,
but it will require someone with a good understanding of the problem (as
that person would really need to see the big picture, and take all use
cases into account), and a large amount of time and motivation.

> > We need a volunteer with enough motivation to solve this subsystem-wide
> > :-) In the meantime, whatever shortcut addresses immediate issues is
> > probably fine, as yak-shaving in this area would definitely not be
> > reasonable.
> 
> I guess drm/bridge keeps on disappointing :-/

I usually blame the x86 folks for not caring enough about bridges
initially, resulting in it being a second class citizen ;-)

> > > > >> v2: Cleanup connector only if it was created
> > > > >>
> > > > >> Fixes: 13dfc0540a57 ("drm/bridge: Refactor out the panel wrapper from the lvds-encoder bridge.")
> > > > >> Cc: <stable@vger.kernel.org> # 4.12+
> > > > >> Cc: Andrzej Hajda <a.hajda@samsung.com>
> > > > >> Cc: Neil Armstrong <narmstrong@baylibre.com>
> > > > >> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> > > > >> Cc: Jonas Karlman <jonas@kwiboo.se>
> > > > >> Cc: Jernej Skrabec <jernej.skrabec@siol.net>
> > > > >> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> > > > >> ---
> > > > >>  drivers/gpu/drm/bridge/panel.c | 6 ++++++
> > > > >>  1 file changed, 6 insertions(+)
> > > > >>
> > > > >> diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c
> > > > >> index 0ddc37551194..df86b0ee0549 100644
> > > > >> --- a/drivers/gpu/drm/bridge/panel.c
> > > > >> +++ b/drivers/gpu/drm/bridge/panel.c
> > > > >> @@ -87,6 +87,12 @@ static int panel_bridge_attach(struct drm_bridge *bridge,
> > > > >>
> > > > >>  static void panel_bridge_detach(struct drm_bridge *bridge)
> > > > >>  {
> > > > >> +	struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge);
> > > > >> +	struct drm_connector *connector = &panel_bridge->connector;
> > > > >> +
> > > > >> +	/* Cleanup the connector if we know it was initialized */
> > > > >> +	if (!!panel_bridge->connector.dev)
> > > > >> +		drm_connector_cleanup(connector);
> > > > >>  }
> > > > >>
> > > > >>  static void panel_bridge_pre_enable(struct drm_bridge *bridge)

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel