Hi Laurent,

On 04/12/2020 22:01, Laurent Pinchart wrote:

On D3 and E3 platforms, the LVDS encoder includes a PLL that can generate a clock for the corresponding CRTC, used even when the CRTC output to a non-LVDS port. This mechanism is supported by the driver, but the implementation is broken in dual-link LVDS mode. In that case, the LVDS1 drm_encoder is skipped, which causes a crash when trying to access its bridge later on.

Fix this by storing bridge pointers internally instead of retrieving them from the encoder.

This looks cleaner too IMO. Win win, bug fix and nicer code.

Reviewed-by: Kieran Bingham kieran.bingham+renesas@ideasonboard.com

Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com

drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 10 ++-------- drivers/gpu/drm/rcar-du/rcar_du_drv.h | 3 +++ drivers/gpu/drm/rcar-du/rcar_du_encoder.c | 4 ++++ 3 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c index b5fb941e0f53..e23b9c7b4afe 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c @@ -730,13 +730,10 @@ static void rcar_du_crtc_atomic_enable(struct drm_crtc *crtc, */ if (rcdu->info->lvds_clk_mask & BIT(rcrtc->index) && rstate->outputs == BIT(RCAR_DU_OUTPUT_DPAD0)) {

• struct rcar_du_encoder *encoder =
  

• 	rcdu->encoders[RCAR_DU_OUTPUT_LVDS0 + rcrtc->index];
  

• struct drm_bridge *bridge = rcdu->lvds[rcrtc->index];
  

  const struct drm_display_mode *mode = &crtc->state->adjusted_mode;

• struct drm_bridge *bridge;
  
  

• bridge = drm_bridge_chain_get_first_bridge(&encoder->base);
  

  rcar_lvds_clk_enable(bridge, mode->clock * 1000); }

@@ -764,15 +761,12 @@ static void rcar_du_crtc_atomic_disable(struct drm_crtc *crtc,

if (rcdu->info->lvds_clk_mask & BIT(rcrtc->index) && rstate->outputs == BIT(RCAR_DU_OUTPUT_DPAD0)) {

• struct rcar_du_encoder *encoder =
  

• 	rcdu->encoders[RCAR_DU_OUTPUT_LVDS0 + rcrtc->index];
  

• struct drm_bridge *bridge;
  

• struct drm_bridge *bridge = rcdu->lvds[rcrtc->index];
  
  

  /*

  • Disable the LVDS clock output, see
  • rcar_du_crtc_atomic_enable().

  */

• bridge = drm_bridge_chain_get_first_bridge(&encoder->base);
  

  rcar_lvds_clk_disable(bridge); }

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.h b/drivers/gpu/drm/rcar-du/rcar_du_drv.h index 61504c54e2ec..71732fc5df8f 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.h +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.h @@ -20,6 +20,7 @@

struct clk; struct device; +struct drm_bridge; struct drm_device; struct drm_property; struct rcar_du_device; @@ -71,6 +72,7 @@ struct rcar_du_device_info { #define RCAR_DU_MAX_CRTCS 4 #define RCAR_DU_MAX_GROUPS DIV_ROUND_UP(RCAR_DU_MAX_CRTCS, 2) #define RCAR_DU_MAX_VSPS 4 +#define RCAR_DU_MAX_LVDS 2

struct rcar_du_device { struct device *dev; @@ -88,6 +90,7 @@ struct rcar_du_device { struct rcar_du_group groups[RCAR_DU_MAX_GROUPS]; struct platform_device *cmms[RCAR_DU_MAX_CRTCS]; struct rcar_du_vsp vsps[RCAR_DU_MAX_VSPS];

• struct drm_bridge *lvds[RCAR_DU_MAX_LVDS];

  struct { struct drm_property *colorkey;

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c index b0335da0c161..2d40da98144b 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c @@ -91,6 +91,10 @@ int rcar_du_encoder_init(struct rcar_du_device *rcdu, ret = -EPROBE_DEFER; goto done; }

• if (output == RCAR_DU_OUTPUT_LVDS0 ||
  

•     output == RCAR_DU_OUTPUT_LVDS1)
  

• 	rcdu->lvds[output - RCAR_DU_OUTPUT_LVDS0] = bridge;
  

  }

  /*

Hi Laurent,

On Sat, Dec 05, 2020 at 12:01:32AM +0200, Laurent Pinchart wrote:

Use drmm_add_action_or_reset() instead of drmm_add_action() to ensure the vsp device reference is released in case the function call fails.

Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com

Just in case... better safe than sorry

Reviewed-by: Jacopo Mondi jacopo+renesas@jmondi.org

---

drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c index f6a69aa116e6..4dcb1bfbe201 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c @@ -364,7 +364,7 @@ int rcar_du_vsp_init(struct rcar_du_vsp *vsp, struct device_node *np,

vsp->vsp = &pdev->dev;

• ret = drmm_add_action(rcdu->ddev, rcar_du_vsp_cleanup, vsp);

• ret = drmm_add_action_or_reset(rcdu->ddev, rcar_du_vsp_cleanup, vsp); if (ret < 0) return ret;

-- Regards,

Laurent Pinchart

Hi Jacopo,

On Mon, Dec 14, 2020 at 11:11:08AM +0100, Jacopo Mondi wrote:

On Sat, Dec 05, 2020 at 12:01:33AM +0200, Laurent Pinchart wrote:

...

The encoder->name field can never be non-null in the error path, as that can only be possible after a successful call to drm_simple_encoder_init(). Drop the cleanup.

Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com

drivers/gpu/drm/rcar-du/rcar_du_encoder.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c index 2d40da98144b..0edce24f2053 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c @@ -124,11 +124,8 @@ int rcar_du_encoder_init(struct rcar_du_device *rcdu, }

done:

• if (ret < 0) {

• if (encoder->name)
  

• 	encoder->funcs->destroy(encoder);
  

This is probably worth a Fixes tag, as accessing encoder->func if drm_simple_encoder_init() has not completed might lead to a NULL pointer dereference.

But in that case encoder->name would be NULL, so encoder->funcs won't be dereferenced. And encoder->name can never be non-NULL here, as explained in the commit message, so this is dead code. I don't think it requires a Fixes: tag.

Apart from this, patch looks good Reviewed-by: Jacopo Mondi jacopo@jmondi.org

...

• if (ret < 0) devm_kfree(rcdu->dev, renc);

• }

  return ret;

}

Hi Kieran,

On Mon, Dec 14, 2020 at 04:20:17PM +0000, Kieran Bingham wrote:

On 04/12/2020 22:01, Laurent Pinchart wrote:

...

devm_kcalloc() is the wrong API to allocate planes, as the lifetime of the planes is tied to the DRM device, not the device to driver binding. drmm_kcalloc() isn't a good option either, as it would result in the planes being freed before being unregistered during the managed cleanup of the DRM objects. Use a plain kcalloc(), and cleanup the planes and free the memory in the existing rcar_du_vsp_cleanup() handler.

Managed memory always seems to hurt - which is a shame, because it should be better throughout.

It's like we need a way to arbitrarily specify the lifetimes of objects correctly against another object... without being tied to a dev ...

I've been saying for years that devm_kzalloc() is a major regression. We've traded a memory leak for a use-after-free. The function has its use cases, there are objects that need to match the lifetime of the binding between a device and its driver, but that's a small minority.

Reviewed-by: Kieran Bingham kieran.bingham+renesas@ideasonboard.com

...

Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com

drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c index 4dcb1bfbe201..78a886651d9f 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c @@ -21,6 +21,7 @@ #include <linux/dma-mapping.h> #include <linux/of_platform.h> #include <linux/scatterlist.h> +#include <linux/slab.h> #include <linux/videodev2.h>

#include <media/vsp1.h> @@ -344,6 +345,15 @@ static const struct drm_plane_funcs rcar_du_vsp_plane_funcs = { static void rcar_du_vsp_cleanup(struct drm_device *dev, void *res) { struct rcar_du_vsp *vsp = res;

• unsigned int i;

• for (i = 0; i < vsp->num_planes; ++i) {

• struct rcar_du_vsp_plane *plane = &vsp->planes[i];
  

• drm_plane_cleanup(&plane->plane);
  

• }

• kfree(vsp->planes);

  put_device(vsp->vsp);

} @@ -354,6 +364,7 @@ int rcar_du_vsp_init(struct rcar_du_vsp *vsp, struct device_node *np, struct rcar_du_device *rcdu = vsp->dev; struct platform_device *pdev; unsigned int num_crtcs = hweight32(crtcs);

• unsigned int num_planes; unsigned int i; int ret;

@@ -376,14 +387,13 @@ int rcar_du_vsp_init(struct rcar_du_vsp *vsp, struct device_node *np, * The VSP2D (Gen3) has 5 RPFs, but the VSP1D (Gen2) is limited to * 4 RPFs. */

• vsp->num_planes = rcdu->info->gen >= 3 ? 5 : 4;

• num_planes = rcdu->info->gen >= 3 ? 5 : 4;

• vsp->planes = devm_kcalloc(rcdu->dev, vsp->num_planes,

• 		   sizeof(*vsp->planes), GFP_KERNEL);
  

• vsp->planes = kcalloc(num_planes, sizeof(*vsp->planes), GFP_KERNEL); if (!vsp->planes) return -ENOMEM;

• for (i = 0; i < vsp->num_planes; ++i) {

• for (i = 0; i < num_planes; ++i) { enum drm_plane_type type = i < num_crtcs ? DRM_PLANE_TYPE_PRIMARY : DRM_PLANE_TYPE_OVERLAY;

@@ -409,8 +419,10 @@ int rcar_du_vsp_init(struct rcar_du_vsp *vsp, struct device_node *np, } else { drm_plane_create_alpha_property(&plane->plane); drm_plane_create_zpos_property(&plane->plane, 1, 1,

• 				       vsp->num_planes - 1);
  

• 				       num_planes - 1);
  

  }

• vsp->num_planes++;>  	}
  
  

  return 0;

Hi Jacopo,

On Mon, Dec 14, 2020 at 11:37:50AM +0100, Jacopo Mondi wrote:

On Sat, Dec 05, 2020 at 12:01:35AM +0200, Laurent Pinchart wrote:

...

devm_kzalloc() is the wrong API to allocate encoders, as the lifetime of the encoders is tied to the DRM device, not the device to driver binding. drmm_kzalloc() isn't a good option either, as it would result in the encoder being freed before being unregistered during the managed cleanup of the DRM objects. Use a plain kzalloc(), and register a drmm action to cleanup the encoder.

Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com

drivers/gpu/drm/rcar-du/rcar_du_encoder.c | 47 ++++++++++++++--------- 1 file changed, 29 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c index 0edce24f2053..7c491ff72cd2 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c @@ -8,12 +8,13 @@ */

#include <linux/export.h> +#include <linux/slab.h>

#include <drm/drm_bridge.h> #include <drm/drm_crtc.h> +#include <drm/drm_managed.h> #include <drm/drm_modeset_helper_vtables.h> #include <drm/drm_panel.h> -#include <drm/drm_simple_kms_helper.h>

#include "rcar_du_drv.h" #include "rcar_du_encoder.h" @@ -44,6 +45,17 @@ static unsigned int rcar_du_encoder_count_ports(struct device_node *node) return num_ports; }

+static const struct drm_encoder_funcs rcar_du_encoder_funcs = { +};

+static void rcar_du_encoder_release(struct drm_device *dev, void *res) +{

• struct rcar_du_encoder *renc = res;
• drm_encoder_cleanup(&renc->base);
• kfree(renc);

+}

int rcar_du_encoder_init(struct rcar_du_device *rcdu, enum rcar_du_output output, struct device_node *enc_node) @@ -53,7 +65,7 @@ int rcar_du_encoder_init(struct rcar_du_device *rcdu, struct drm_bridge *bridge; int ret;

• renc = devm_kzalloc(rcdu->dev, sizeof(*renc), GFP_KERNEL);

• renc = kzalloc(sizeof(*renc), GFP_KERNEL); if (renc == NULL) return -ENOMEM;

@@ -76,20 +88,20 @@ int rcar_du_encoder_init(struct rcar_du_device *rcdu,

if (IS_ERR(panel)) {
	ret = PTR_ERR(panel);

• 	goto done;
  

• 	goto error;
  

  }

  bridge = devm_drm_panel_bridge_add_typed(rcdu->dev, panel, DRM_MODE_CONNECTOR_DPI); if (IS_ERR(bridge)) { ret = PTR_ERR(bridge);

• 	goto done;
  

• 	goto error;
  

  } } else { bridge = of_drm_find_bridge(enc_node); if (!bridge) { ret = -EPROBE_DEFER;

• 	goto done;
  

• 	goto error;
  

  }

  if (output == RCAR_DU_OUTPUT_LVDS0 ||

@@ -104,28 +116,27 @@ int rcar_du_encoder_init(struct rcar_du_device *rcdu, if (rcdu->info->gen >= 3 && output == RCAR_DU_OUTPUT_LVDS1) { if (rcar_lvds_dual_link(bridge)) { ret = -ENOLINK;

• 	goto done;
  

• 	goto error;
  

  } }

• ret = drm_simple_encoder_init(rcdu->ddev, encoder,

• 		      DRM_MODE_ENCODER_NONE);
  

• ret = drm_encoder_init(rcdu->ddev, encoder, &rcar_du_encoder_funcs,

• 	       DRM_MODE_ENCODER_NONE, NULL);
  

I might have missed the reason for this. Why add an action later instead of making rcar_du_encoder_release part of rcar_du_encoder_funcs ?

Because the subsystem is moving towards drmm_* instead of manual release callbacks (I don't mind the manual release callbacks personally). Note that there's "[PATCH v4 02/19] drm: add drmm_encoder_alloc()" that should be merged soon, which will allow simplifying this.

...

if (ret < 0)

• goto done;
  

• goto error;
  

• ret = drmm_add_action_or_reset(rcdu->ddev, rcar_du_encoder_release,

• 		       renc);
  

• if (ret)

• return ret;
  
  

  /*

  • Attach the bridge to the encoder. The bridge will create the
  • connector.

  */

• ret = drm_bridge_attach(encoder, bridge, NULL, 0);
• if (ret) {

• drm_encoder_cleanup(encoder);
  

• return ret;
  

• }

-done:

• if (ret < 0)

• devm_kfree(rcdu->dev, renc);
  

• return drm_bridge_attach(encoder, bridge, NULL, 0);

+error:

• kfree(renc); return ret;

}

Hi Laurent,

On Sat, Dec 05, 2020 at 12:01:36AM +0200, Laurent Pinchart wrote:

Embedding drm_device in rcar_du_device allows usage of the DRM managed API to allocate both structures in one go, simplifying error handling.

Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com

Looks good to me (re-sorting the headers rcar_du_drv without mentioning it too).

Reviewed-by: Jacopo Mondi jacopo+renesas@jmondi.org

---

drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 2 +- drivers/gpu/drm/rcar-du/rcar_du_drv.c | 33 +++++++++------------ drivers/gpu/drm/rcar-du/rcar_du_drv.h | 5 ++-- drivers/gpu/drm/rcar-du/rcar_du_encoder.c | 4 +-- drivers/gpu/drm/rcar-du/rcar_du_kms.c | 4 +-- drivers/gpu/drm/rcar-du/rcar_du_plane.c | 6 ++-- drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 6 ++-- drivers/gpu/drm/rcar-du/rcar_du_writeback.c | 2 +- 8 files changed, 29 insertions(+), 33 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c index e23b9c7b4afe..9a099c0fe1d4 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c @@ -1251,7 +1251,7 @@ int rcar_du_crtc_create(struct rcar_du_group *rgrp, unsigned int swindex, else primary = &rgrp->planes[swindex % 2].plane;

• ret = drm_crtc_init_with_planes(rcdu->ddev, crtc, primary, NULL,

• ret = drm_crtc_init_with_planes(&rcdu->ddev, crtc, primary, NULL, rcdu->info->gen <= 2 ? &crtc_funcs_gen2 : &crtc_funcs_gen3, NULL);

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c b/drivers/gpu/drm/rcar-du/rcar_du_drv.c index 1490ec182646..4ab99ac49891 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c @@ -18,10 +18,11 @@ #include <linux/wait.h>

#include <drm/drm_atomic_helper.h> +#include <drm/drm_drv.h> #include <drm/drm_fb_cma_helper.h> #include <drm/drm_fb_helper.h> -#include <drm/drm_drv.h> #include <drm/drm_gem_cma_helper.h> +#include <drm/drm_managed.h> #include <drm/drm_probe_helper.h>

#include "rcar_du_drv.h" @@ -529,14 +530,14 @@ static int rcar_du_pm_suspend(struct device *dev) { struct rcar_du_device *rcdu = dev_get_drvdata(dev);

• return drm_mode_config_helper_suspend(rcdu->ddev);

• return drm_mode_config_helper_suspend(&rcdu->ddev);

}

static int rcar_du_pm_resume(struct device *dev) { struct rcar_du_device *rcdu = dev_get_drvdata(dev);

• return drm_mode_config_helper_resume(rcdu->ddev);

• return drm_mode_config_helper_resume(&rcdu->ddev);

} #endif

@@ -551,7 +552,7 @@ static const struct dev_pm_ops rcar_du_pm_ops = { static int rcar_du_remove(struct platform_device *pdev) { struct rcar_du_device *rcdu = platform_get_drvdata(pdev);

• struct drm_device *ddev = rcdu->ddev;

• struct drm_device *ddev = &rcdu->ddev;

  drm_dev_unregister(ddev);

@@ -565,14 +566,14 @@ static int rcar_du_remove(struct platform_device *pdev) static int rcar_du_probe(struct platform_device *pdev) { struct rcar_du_device *rcdu;

• struct drm_device *ddev; struct resource *mem; int ret;

  /* Allocate and initialize the R-Car device structure. */

• rcdu = devm_kzalloc(&pdev->dev, sizeof(*rcdu), GFP_KERNEL);

• if (rcdu == NULL)

• return -ENOMEM;
  

• rcdu = devm_drm_dev_alloc(&pdev->dev, &rcar_du_driver,

• 		  struct rcar_du_device, ddev);
  

• if (IS_ERR(rcdu))

• return PTR_ERR(rcdu);
  
  

  rcdu->dev = &pdev->dev; rcdu->info = of_device_get_match_data(rcdu->dev);

@@ -586,12 +587,7 @@ static int rcar_du_probe(struct platform_device *pdev) return PTR_ERR(rcdu->mmio);

/* DRM/KMS objects */

• ddev = drm_dev_alloc(&rcar_du_driver, &pdev->dev);
• if (IS_ERR(ddev))

• return PTR_ERR(ddev);
  

• rcdu->ddev = ddev;
• ddev->dev_private = rcdu;

• rcdu->ddev.dev_private = rcdu;

  ret = rcar_du_modeset_init(rcdu); if (ret < 0) {

@@ -601,25 +597,24 @@ static int rcar_du_probe(struct platform_device *pdev) goto error; }

• ddev->irq_enabled = 1;

• rcdu->ddev.irq_enabled = 1;

  /*

  • Register the DRM device with the core and the connectors with
  • sysfs.

  */

• ret = drm_dev_register(ddev, 0);

• ret = drm_dev_register(&rcdu->ddev, 0); if (ret) goto error;

  DRM_INFO("Device %s probed\n", dev_name(&pdev->dev));

• drm_fbdev_generic_setup(ddev, 32);

• drm_fbdev_generic_setup(&rcdu->ddev, 32);

  return 0;

error:

• rcar_du_remove(pdev);

• drm_kms_helper_poll_fini(&rcdu->ddev); return ret;

}

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.h b/drivers/gpu/drm/rcar-du/rcar_du_drv.h index 71732fc5df8f..e5b6f456357e 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.h +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.h @@ -13,6 +13,8 @@ #include <linux/kernel.h> #include <linux/wait.h>

+#include <drm/drm_device.h>

#include "rcar_cmm.h" #include "rcar_du_crtc.h" #include "rcar_du_group.h" @@ -21,7 +23,6 @@ struct clk; struct device; struct drm_bridge; -struct drm_device; struct drm_property; struct rcar_du_device; struct rcar_du_encoder; @@ -80,7 +81,7 @@ struct rcar_du_device {

void __iomem *mmio;

• struct drm_device *ddev;

• struct drm_device ddev;

  struct rcar_du_crtc crtcs[RCAR_DU_MAX_CRTCS]; unsigned int num_crtcs;

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c index 7c491ff72cd2..e4f35a88d00f 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c @@ -120,12 +120,12 @@ int rcar_du_encoder_init(struct rcar_du_device *rcdu, } }

• ret = drm_encoder_init(rcdu->ddev, encoder, &rcar_du_encoder_funcs,

• ret = drm_encoder_init(&rcdu->ddev, encoder, &rcar_du_encoder_funcs, DRM_MODE_ENCODER_NONE, NULL); if (ret < 0) goto error;

• ret = drmm_add_action_or_reset(rcdu->ddev, rcar_du_encoder_release,

• ret = drmm_add_action_or_reset(&rcdu->ddev, rcar_du_encoder_release, renc); if (ret) return ret;

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c index 72dda446355f..57bb0dc22807 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c @@ -583,7 +583,7 @@ static int rcar_du_properties_init(struct rcar_du_device *rcdu) * or enable source color keying (1). */ rcdu->props.colorkey =

• drm_property_create_range(rcdu->ddev, 0, "colorkey",
  

• drm_property_create_range(&rcdu->ddev, 0, "colorkey",
  		  0, 0x01ffffff);
  

  if (rcdu->props.colorkey == NULL) return -ENOMEM;

@@ -752,7 +752,7 @@ int rcar_du_modeset_init(struct rcar_du_device *rcdu) DU0_REG_OFFSET, DU2_REG_OFFSET };

• struct drm_device *dev = rcdu->ddev;

• struct drm_device *dev = &rcdu->ddev; struct drm_encoder *encoder; unsigned int dpad0_sources; unsigned int num_encoders;

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_plane.c b/drivers/gpu/drm/rcar-du/rcar_du_plane.c index a0021fc25b27..5f69ff4502c1 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_plane.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_plane.c @@ -773,9 +773,9 @@ int rcar_du_planes_init(struct rcar_du_group *rgrp)

plane->group = rgrp;

• ret = drm_universal_plane_init(rcdu->ddev, &plane->plane, crtcs,
  

• 			       &rcar_du_plane_funcs, formats,
  

• 			       ARRAY_SIZE(formats),
  

• ret = drm_universal_plane_init(&rcdu->ddev, &plane->plane,
  

• 			       crtcs, &rcar_du_plane_funcs,
  

• 			       formats, ARRAY_SIZE(formats),
  		       NULL, type, NULL);
  

  if (ret < 0) return ret;

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c index 78a886651d9f..53221d8473c1 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c @@ -375,7 +375,7 @@ int rcar_du_vsp_init(struct rcar_du_vsp *vsp, struct device_node *np,

vsp->vsp = &pdev->dev;

• ret = drmm_add_action_or_reset(rcdu->ddev, rcar_du_vsp_cleanup, vsp);

• ret = drmm_add_action_or_reset(&rcdu->ddev, rcar_du_vsp_cleanup, vsp); if (ret < 0) return ret;

@@ -402,8 +402,8 @@ int rcar_du_vsp_init(struct rcar_du_vsp *vsp, struct device_node *np, plane->vsp = vsp; plane->index = i;

• ret = drm_universal_plane_init(rcdu->ddev, &plane->plane, crtcs,
  

• 			       &rcar_du_vsp_plane_funcs,
  

• ret = drm_universal_plane_init(&rcdu->ddev, &plane->plane,
  

• 			       crtcs, &rcar_du_vsp_plane_funcs,
  		       rcar_du_vsp_formats,
  		       ARRAY_SIZE(rcar_du_vsp_formats),
  		       NULL, type, NULL);
  

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c index 04efa78d70b6..c79d1259e49b 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c @@ -204,7 +204,7 @@ int rcar_du_writeback_init(struct rcar_du_device *rcdu, drm_connector_helper_add(&wb_conn->base, &rcar_du_wb_conn_helper_funcs);

• return drm_writeback_connector_init(rcdu->ddev, wb_conn,

• return drm_writeback_connector_init(&rcdu->ddev, wb_conn, &rcar_du_wb_conn_funcs, &rcar_du_wb_enc_helper_funcs, writeback_formats,

-- Regards,

Laurent Pinchart

Hi Laurent,

On 14/12/2020 10:58, Jacopo Mondi wrote:

Hi Laurent,

On Sat, Dec 05, 2020 at 12:01:37AM +0200, Laurent Pinchart wrote:

...

Now that drm_device is embedded in rcar_du_device, we can use container_of to get the rcar_du_device pointer from the drm_device, instead of using the drm_device.dev_private field.

Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com

De gustibus non est disputandum

Err ... um ... what he said - whatever that is ... maybe ...

/me checks if google is still working to translate ...

Oh ok ;-)

Well - Yes, I like to_*(from) conversion macros ... so

Reviewed-by: Kieran Bingham kieran.bingham+renesas@ideasonboard.com

Reviewed-by: Jacopo Mondi jacopo+renesas@jmondi.org

Thanks j

...

---

drivers/gpu/drm/rcar-du/rcar_du_drv.c | 2 -- drivers/gpu/drm/rcar-du/rcar_du_drv.h | 5 +++++ drivers/gpu/drm/rcar-du/rcar_du_kms.c | 8 ++++---- drivers/gpu/drm/rcar-du/rcar_du_plane.c | 2 +- 4 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c b/drivers/gpu/drm/rcar-du/rcar_du_drv.c index 4ab99ac49891..d6a8b7899952 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c @@ -587,8 +587,6 @@ static int rcar_du_probe(struct platform_device *pdev) return PTR_ERR(rcdu->mmio);

/* DRM/KMS objects */

• rcdu->ddev.dev_private = rcdu;
• ret = rcar_du_modeset_init(rcdu); if (ret < 0) { if (ret != -EPROBE_DEFER)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.h b/drivers/gpu/drm/rcar-du/rcar_du_drv.h index e5b6f456357e..98d6bac3f2fa 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.h +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.h @@ -102,6 +102,11 @@ struct rcar_du_device { unsigned int vspd1_sink; };

+static inline struct rcar_du_device *to_rcar_du_device(struct drm_device *dev) +{

• return container_of(dev, struct rcar_du_device, ddev);

+}

static inline bool rcar_du_has(struct rcar_du_device *rcdu, unsigned int feature) { diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c index 57bb0dc22807..d6b71a9361ca 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c @@ -327,7 +327,7 @@ const struct rcar_du_format_info *rcar_du_format_info(u32 fourcc) int rcar_du_dumb_create(struct drm_file *file, struct drm_device *dev, struct drm_mode_create_dumb *args) {

• struct rcar_du_device *rcdu = dev->dev_private;

• struct rcar_du_device *rcdu = to_rcar_du_device(dev); unsigned int min_pitch = DIV_ROUND_UP(args->width * args->bpp, 8); unsigned int align;

@@ -349,7 +349,7 @@ static struct drm_framebuffer * rcar_du_fb_create(struct drm_device *dev, struct drm_file *file_priv, const struct drm_mode_fb_cmd2 *mode_cmd) {

• struct rcar_du_device *rcdu = dev->dev_private;

• struct rcar_du_device *rcdu = to_rcar_du_device(dev); const struct rcar_du_format_info *format; unsigned int chroma_pitch; unsigned int max_pitch;

@@ -421,7 +421,7 @@ rcar_du_fb_create(struct drm_device *dev, struct drm_file *file_priv, static int rcar_du_atomic_check(struct drm_device *dev, struct drm_atomic_state *state) {

• struct rcar_du_device *rcdu = dev->dev_private;

• struct rcar_du_device *rcdu = to_rcar_du_device(dev); int ret;

  ret = drm_atomic_helper_check(dev, state);

@@ -437,7 +437,7 @@ static int rcar_du_atomic_check(struct drm_device *dev, static void rcar_du_atomic_commit_tail(struct drm_atomic_state *old_state) { struct drm_device *dev = old_state->dev;

• struct rcar_du_device *rcdu = dev->dev_private;

• struct rcar_du_device *rcdu = to_rcar_du_device(dev); struct drm_crtc_state *crtc_state; struct drm_crtc *crtc; unsigned int i;

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_plane.c b/drivers/gpu/drm/rcar-du/rcar_du_plane.c index 5f69ff4502c1..02e5f11f38eb 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_plane.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_plane.c @@ -128,7 +128,7 @@ static int rcar_du_plane_hwalloc(struct rcar_du_plane *plane, int rcar_du_atomic_check_planes(struct drm_device *dev, struct drm_atomic_state *state) {

• struct rcar_du_device *rcdu = dev->dev_private;

• struct rcar_du_device *rcdu = to_rcar_du_device(dev); unsigned int group_freed_planes[RCAR_DU_MAX_GROUPS] = { 0, }; unsigned int group_free_planes[RCAR_DU_MAX_GROUPS] = { 0, }; bool needs_realloc = false;

-- Regards,

Laurent Pinchart

Hi Laurent,

On Sat, Dec 05, 2020 at 12:01:38AM +0200, Laurent Pinchart wrote:

The rcar-du driver skips registration of the encoder for the LVDS1 output when LVDS is used in dual-link mode, as the LVDS0 and LVDS1 links are bundled and handled through the LVDS0 output. It however still allocates the encoder and immediately destroys it, which is pointless. Skip allocation of the encoder altogether in that case.

Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com

drivers/gpu/drm/rcar-du/rcar_du_encoder.c | 51 ++++++++++------------- 1 file changed, 22 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c index e4f35a88d00f..49c0b27e2f5a 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c @@ -65,17 +65,6 @@ int rcar_du_encoder_init(struct rcar_du_device *rcdu, struct drm_bridge *bridge; int ret;

• renc = kzalloc(sizeof(*renc), GFP_KERNEL);
• if (renc == NULL)

• return -ENOMEM;
  

• rcdu->encoders[output] = renc;
• renc->output = output;
• encoder = rcar_encoder_to_drm_encoder(renc);
• dev_dbg(rcdu->dev, "initializing encoder %pOF for output %u\n",

• enc_node, output);
  

• /*
  • Locate the DRM bridge from the DT node. For the DPAD outputs, if the
  • DT node has a single port, assume that it describes a panel and

@@ -86,23 +75,17 @@ int rcar_du_encoder_init(struct rcar_du_device *rcdu, rcar_du_encoder_count_ports(enc_node) == 1) { struct drm_panel *panel = of_drm_find_panel(enc_node);

• if (IS_ERR(panel)) {
  

• 	ret = PTR_ERR(panel);
  

• 	goto error;
  

• }
  

• if (IS_ERR(panel))
  

• 	return PTR_ERR(panel);
  
  

  bridge = devm_drm_panel_bridge_add_typed(rcdu->dev, panel, DRM_MODE_CONNECTOR_DPI);

• if (IS_ERR(bridge)) {
  

• 	ret = PTR_ERR(bridge);
  

• 	goto error;
  

• }
  

• if (IS_ERR(bridge))
  

• 	return PTR_ERR(bridge);
  

  } else { bridge = of_drm_find_bridge(enc_node);

• if (!bridge) {
  

• 	ret = -EPROBE_DEFER;
  

• 	goto error;
  

• }
  

• if (!bridge)
  

• 	return -EPROBE_DEFER;
  
  

  if (output == RCAR_DU_OUTPUT_LVDS0 || output == RCAR_DU_OUTPUT_LVDS1)

@@ -110,16 +93,26 @@ int rcar_du_encoder_init(struct rcar_du_device *rcdu, }

/*

• * On Gen3 skip the LVDS1 output if the LVDS1 encoder is used as a
  

• * companion for LVDS0 in dual-link mode.
  

• * Create and initialize the encoder. On Gen3 skip the LVDS1 output if
  

• * the LVDS1 encoder is used as a companion for LVDS0 in dual-link
  

• * mode.
  

Oh, here's the answer to my question on 1/9, I should have not looked at DTS but to the driver

*/

if (rcdu->info->gen >= 3 && output == RCAR_DU_OUTPUT_LVDS1) {

• if (rcar_lvds_dual_link(bridge)) {
  

• 	ret = -ENOLINK;
  

• 	goto error;
  

• }
  

• if (rcar_lvds_dual_link(bridge))
  

• 	return -ENOLINK;
  

  }

• renc = kzalloc(sizeof(*renc), GFP_KERNEL);

• if (renc == NULL)

• return -ENOMEM;
  

• rcdu->encoders[output] = renc;

• renc->output = output;

• encoder = rcar_encoder_to_drm_encoder(renc);

• dev_dbg(rcdu->dev, "initializing encoder %pOF for output %u\n",

• enc_node, output);
  

• ret = drm_encoder_init(&rcdu->ddev, encoder, &rcar_du_encoder_funcs, DRM_MODE_ENCODER_NONE, NULL); if (ret < 0)

Do you have any other caller of the 'done:' label left apart from the one after this last line ? In case you don't you can call devm_kfree() here

With or without this addressed: Reviewed-by: Jacopo Mondi jacopo+renesas@jmondi.org

-- Regards,

Laurent Pinchart

Hi Laurent,

On 04/12/2020 22:01, Laurent Pinchart wrote:

The rcar-du driver skips registration of the encoder for the LVDS1 output when LVDS is used in dual-link mode, as the LVDS0 and LVDS1 links are bundled and handled through the LVDS0 output. It however still allocates the encoder and immediately destroys it, which is pointless. Skip allocation of the encoder altogether in that case.

Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com

I love it when re-ordering code simplifies all the error conditions.

Reviewed-by: Kieran Bingham kieran.bingham+renesas@ideasonboard.com

---

drivers/gpu/drm/rcar-du/rcar_du_encoder.c | 51 ++++++++++------------- 1 file changed, 22 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c index e4f35a88d00f..49c0b27e2f5a 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c @@ -65,17 +65,6 @@ int rcar_du_encoder_init(struct rcar_du_device *rcdu, struct drm_bridge *bridge; int ret;

• renc = kzalloc(sizeof(*renc), GFP_KERNEL);
• if (renc == NULL)

• return -ENOMEM;
  

• rcdu->encoders[output] = renc;
• renc->output = output;
• encoder = rcar_encoder_to_drm_encoder(renc);
• dev_dbg(rcdu->dev, "initializing encoder %pOF for output %u\n",

• enc_node, output);
  

• /*
  • Locate the DRM bridge from the DT node. For the DPAD outputs, if the
  • DT node has a single port, assume that it describes a panel and

@@ -86,23 +75,17 @@ int rcar_du_encoder_init(struct rcar_du_device *rcdu, rcar_du_encoder_count_ports(enc_node) == 1) { struct drm_panel *panel = of_drm_find_panel(enc_node);

• if (IS_ERR(panel)) {
  

• 	ret = PTR_ERR(panel);
  

• 	goto error;
  

• }
  

• if (IS_ERR(panel))
  

• 	return PTR_ERR(panel);
  
  

  bridge = devm_drm_panel_bridge_add_typed(rcdu->dev, panel, DRM_MODE_CONNECTOR_DPI);

• if (IS_ERR(bridge)) {
  

• 	ret = PTR_ERR(bridge);
  

• 	goto error;
  

• }
  

• if (IS_ERR(bridge))
  

• 	return PTR_ERR(bridge);
  

  } else { bridge = of_drm_find_bridge(enc_node);

• if (!bridge) {
  

• 	ret = -EPROBE_DEFER;
  

• 	goto error;
  

• }
  

• if (!bridge)
  

• 	return -EPROBE_DEFER;
  
  

  if (output == RCAR_DU_OUTPUT_LVDS0 || output == RCAR_DU_OUTPUT_LVDS1)

@@ -110,16 +93,26 @@ int rcar_du_encoder_init(struct rcar_du_device *rcdu, }

/*

• * On Gen3 skip the LVDS1 output if the LVDS1 encoder is used as a
  

• * companion for LVDS0 in dual-link mode.
  

• * Create and initialize the encoder. On Gen3 skip the LVDS1 output if
  

• * the LVDS1 encoder is used as a companion for LVDS0 in dual-link
  

• * mode.
  

  */ if (rcdu->info->gen >= 3 && output == RCAR_DU_OUTPUT_LVDS1) {

• if (rcar_lvds_dual_link(bridge)) {
  

• 	ret = -ENOLINK;
  

• 	goto error;
  

• }
  

• if (rcar_lvds_dual_link(bridge))
  

• 	return -ENOLINK;
  

  }

• renc = kzalloc(sizeof(*renc), GFP_KERNEL);

• if (renc == NULL)

• return -ENOMEM;
  

• rcdu->encoders[output] = renc;

• renc->output = output;

• encoder = rcar_encoder_to_drm_encoder(renc);

• dev_dbg(rcdu->dev, "initializing encoder %pOF for output %u\n",

• enc_node, output);
  

• ret = drm_encoder_init(&rcdu->ddev, encoder, &rcar_du_encoder_funcs, DRM_MODE_ENCODER_NONE, NULL); if (ret < 0)

Hi Laurent, On Sat, Dec 05, 2020 at 12:01:39AM +0200, Laurent Pinchart wrote:

The local encoder variable is an alias for &renc->base, and is only use twice. It doesn't help much, drop it, along with the rcar_encoder_to_drm_encoder() macro that is then unused.

Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com

Reviewed-by: Jacopo Mondi jacopo+renesas@jmondi.org

Thanks j

---

drivers/gpu/drm/rcar-du/rcar_du_encoder.c | 6 ++---- drivers/gpu/drm/rcar-du/rcar_du_encoder.h | 2 -- 2 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c index 49c0b27e2f5a..9a565bd3380d 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c @@ -61,7 +61,6 @@ int rcar_du_encoder_init(struct rcar_du_device *rcdu, struct device_node *enc_node) { struct rcar_du_encoder *renc;

• struct drm_encoder *encoder; struct drm_bridge *bridge; int ret;

@@ -108,12 +107,11 @@ int rcar_du_encoder_init(struct rcar_du_device *rcdu,

rcdu->encoders[output] = renc; renc->output = output;

• encoder = rcar_encoder_to_drm_encoder(renc);

  dev_dbg(rcdu->dev, "initializing encoder %pOF for output %u\n", enc_node, output);

• ret = drm_encoder_init(&rcdu->ddev, encoder, &rcar_du_encoder_funcs,

• ret = drm_encoder_init(&rcdu->ddev, &renc->base, &rcar_du_encoder_funcs, DRM_MODE_ENCODER_NONE, NULL); if (ret < 0) goto error;

@@ -127,7 +125,7 @@ int rcar_du_encoder_init(struct rcar_du_device *rcdu, * Attach the bridge to the encoder. The bridge will create the * connector. */

• return drm_bridge_attach(encoder, bridge, NULL, 0);

• return drm_bridge_attach(&renc->base, bridge, NULL, 0);

error: kfree(renc); diff --git a/drivers/gpu/drm/rcar-du/rcar_du_encoder.h b/drivers/gpu/drm/rcar-du/rcar_du_encoder.h index df9be4524301..73560563fb31 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_encoder.h +++ b/drivers/gpu/drm/rcar-du/rcar_du_encoder.h @@ -22,8 +22,6 @@ struct rcar_du_encoder { #define to_rcar_encoder(e) \ container_of(e, struct rcar_du_encoder, base)

-#define rcar_encoder_to_drm_encoder(e) (&(e)->base)

int rcar_du_encoder_init(struct rcar_du_device *rcdu, enum rcar_du_output output, struct device_node *enc_node); -- Regards,

Laurent Pinchart