Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1503809 > unrolled thread

[PATCH v2] drm/arcpgu: Accommodate adv7511 switch to DRM bridge

Back to article view | Back to linux.kernel

Contents

  [PATCH v2] drm/arcpgu: Accommodate adv7511 switch to DRM bridge Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com> - 2016-10-19 17:10 +0200
    Re: [PATCH v2] drm/arcpgu: Accommodate adv7511 switch to DRM bridge Archit Taneja <architt@codeaurora.org> - 2016-10-19 17:20 +0200

#1503809 — [PATCH v2] drm/arcpgu: Accommodate adv7511 switch to DRM bridge

ARC PGU driver starts crashing on initialization after
'commit e12c2f645557 ("drm/i2c: adv7511: Convert to drm_bridge")'
This happenes because in "arcpgu_drm_hdmi_init" function we get pointer
of "drm_i2c_encoder_driver" structure, which doesn't exist after
adv7511 hdmi encoder interface changed from slave encoder to drm bridge.
So, when we call "encoder_init" function from this structure driver
crashes.

Bootlog:
------------------------------------->8--------------------------------
[drm] Initialized drm 1.1.0 20060810
arcpgu e0017000.pgu: arc_pgu ID: 0xabbabaab
arcpgu e0017000.pgu: assigned reserved memory node frame_buffer@9e000000
Path: (null)
CPU: 0 PID: 1 Comm: swapper Not tainted 4.8.0-00001-gb5642252fa01-dirty #8
task: 9a058000 task.stack: 9a032000

[ECR   ]: 0x00220100 => Invalid Read @ 0x00000004 by insn @ 0x803934e8
[EFA   ]: 0x00000004
[BLINK ]: drm_atomic_helper_connector_dpms+0xa6/0x230
[ERET  ]: drm_atomic_helper_connector_dpms+0xa4/0x230
[STAT32]: 0x00000846 : K DE       E2 E1
BTA: 0x8016d949  SP: 0x9a033e34  FP: 0x00000000
LPS: 0x8036f6fc LPE: 0x8036f700 LPC: 0x00000000
r00: 0x8063c118 r01: 0x805b98ac r02: 0x00000b11
r03: 0x00000000 r04: 0x9a010f54 r05: 0x00000000
r06: 0x00000001 r07: 0x00000000 r08: 0x00000028
r09: 0x00000001 r10: 0x00000007 r11: 0x00000054
r12: 0x720a3033

Stack Trace:
  drm_atomic_helper_connector_dpms+0xa4/0x230
  arcpgu_drm_hdmi_init+0xbc/0x228
  arcpgu_probe+0x168/0x244
  platform_drv_probe+0x26/0x64
  really_probe+0x1f0/0x32c
  __driver_attach+0xa8/0xd0
  bus_for_each_dev+0x3c/0x74
  bus_add_driver+0xc2/0x184
  driver_register+0x50/0xec
  do_one_initcall+0x3a/0x120
  kernel_init_freeable+0x108/0x1a0
------------------------------------->8--------------------------------

Fix ARC PGU driver to be able work with drm bridge hdmi encoder
interface. The hdmi connector code isn't needed anymore as we expect
the adv7511 bridge driver to create/manage the connector.

Signed-off-by: Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com>
---
Changes for v2:
   - remove bridge functions call from kms driver
   - get rid of drm_encoder_slave interface
   - coding style cleanup

 drivers/gpu/drm/arc/arcpgu_hdmi.c | 159 ++++----------------------------------
 1 file changed, 17 insertions(+), 142 deletions(-)

diff --git a/drivers/gpu/drm/arc/arcpgu_hdmi.c b/drivers/gpu/drm/arc/arcpgu_hdmi.c
index b7a8b2a..b69c66b 100644
--- a/drivers/gpu/drm/arc/arcpgu_hdmi.c
+++ b/drivers/gpu/drm/arc/arcpgu_hdmi.c
@@ -14,170 +14,45 @@
  *
  */
 
-#include <drm/drm_crtc_helper.h>
+#include <drm/drm_crtc.h>
 #include <drm/drm_encoder_slave.h>
-#include <drm/drm_atomic_helper.h>
 
 #include "arcpgu.h"
 
-struct arcpgu_drm_connector {
-	struct drm_connector connector;
-	struct drm_encoder_slave *encoder_slave;
-};
-
-static int arcpgu_drm_connector_get_modes(struct drm_connector *connector)
-{
-	const struct drm_encoder_slave_funcs *sfuncs;
-	struct drm_encoder_slave *slave;
-	struct arcpgu_drm_connector *con =
-		container_of(connector, struct arcpgu_drm_connector, connector);
-
-	slave = con->encoder_slave;
-	if (slave == NULL) {
-		dev_err(connector->dev->dev,
-			"connector_get_modes: cannot find slave encoder for connector\n");
-		return 0;
-	}
-
-	sfuncs = slave->slave_funcs;
-	if (sfuncs->get_modes == NULL)
-		return 0;
-
-	return sfuncs->get_modes(&slave->base, connector);
-}
-
-static enum drm_connector_status
-arcpgu_drm_connector_detect(struct drm_connector *connector, bool force)
-{
-	enum drm_connector_status status = connector_status_unknown;
-	const struct drm_encoder_slave_funcs *sfuncs;
-	struct drm_encoder_slave *slave;
-
-	struct arcpgu_drm_connector *con =
-		container_of(connector, struct arcpgu_drm_connector, connector);
-
-	slave = con->encoder_slave;
-	if (slave == NULL) {
-		dev_err(connector->dev->dev,
-			"connector_detect: cannot find slave encoder for connector\n");
-		return status;
-	}
-
-	sfuncs = slave->slave_funcs;
-	if (sfuncs && sfuncs->detect)
-		return sfuncs->detect(&slave->base, connector);
-
-	dev_err(connector->dev->dev, "connector_detect: could not detect slave funcs\n");
-	return status;
-}
-
-static void arcpgu_drm_connector_destroy(struct drm_connector *connector)
-{
-	drm_connector_unregister(connector);
-	drm_connector_cleanup(connector);
-}
-
-static const struct drm_connector_helper_funcs
-arcpgu_drm_connector_helper_funcs = {
-	.get_modes = arcpgu_drm_connector_get_modes,
-};
-
-static const struct drm_connector_funcs arcpgu_drm_connector_funcs = {
-	.dpms = drm_helper_connector_dpms,
-	.reset = drm_atomic_helper_connector_reset,
-	.detect = arcpgu_drm_connector_detect,
-	.fill_modes = drm_helper_probe_single_connector_modes,
-	.destroy = arcpgu_drm_connector_destroy,
-	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
-	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
-};
-
-static struct drm_encoder_helper_funcs arcpgu_drm_encoder_helper_funcs = {
-	.dpms = drm_i2c_encoder_dpms,
-	.mode_fixup = drm_i2c_encoder_mode_fixup,
-	.mode_set = drm_i2c_encoder_mode_set,
-	.prepare = drm_i2c_encoder_prepare,
-	.commit = drm_i2c_encoder_commit,
-	.detect = drm_i2c_encoder_detect,
-};
-
 static struct drm_encoder_funcs arcpgu_drm_encoder_funcs = {
 	.destroy = drm_encoder_cleanup,
 };
 
 int arcpgu_drm_hdmi_init(struct drm_device *drm, struct device_node *np)
 {
-	struct arcpgu_drm_connector *arcpgu_connector;
-	struct drm_i2c_encoder_driver *driver;
-	struct drm_encoder_slave *encoder;
-	struct drm_connector *connector;
-	struct i2c_client *i2c_slave;
-	int ret;
+	struct drm_encoder *encoder;
+	struct drm_bridge *bridge;
+
+	int ret = 0;
 
 	encoder = devm_kzalloc(drm->dev, sizeof(*encoder), GFP_KERNEL);
 	if (encoder == NULL)
 		return -ENOMEM;
 
-	i2c_slave = of_find_i2c_device_by_node(np);
-	if (!i2c_slave || !i2c_get_clientdata(i2c_slave)) {
-		dev_err(drm->dev, "failed to find i2c slave encoder\n");
-		return -EPROBE_DEFER;
-	}
-
-	if (i2c_slave->dev.driver == NULL) {
-		dev_err(drm->dev, "failed to find i2c slave driver\n");
+	/* Locate drm bridge from the hdmi encoder DT node */
+	bridge = of_drm_find_bridge(np);
+	if (!bridge)
 		return -EPROBE_DEFER;
-	}
 
-	driver =
-	    to_drm_i2c_encoder_driver(to_i2c_driver(i2c_slave->dev.driver));
-	ret = driver->encoder_init(i2c_slave, drm, encoder);
-	if (ret) {
-		dev_err(drm->dev, "failed to initialize i2c encoder slave\n");
-		return ret;
-	}
-
-	encoder->base.possible_crtcs = 1;
-	encoder->base.possible_clones = 0;
-	ret = drm_encoder_init(drm, &encoder->base, &arcpgu_drm_encoder_funcs,
+	encoder->possible_crtcs = 1;
+	encoder->possible_clones = 0;
+	ret = drm_encoder_init(drm, encoder, &arcpgu_drm_encoder_funcs,
 			       DRM_MODE_ENCODER_TMDS, NULL);
 	if (ret)
 		return ret;
 
-	drm_encoder_helper_add(&encoder->base,
-			       &arcpgu_drm_encoder_helper_funcs);
-
-	arcpgu_connector = devm_kzalloc(drm->dev, sizeof(*arcpgu_connector),
-					GFP_KERNEL);
-	if (!arcpgu_connector) {
-		ret = -ENOMEM;
-		goto error_encoder_cleanup;
-	}
-
-	connector = &arcpgu_connector->connector;
-	drm_connector_helper_add(connector, &arcpgu_drm_connector_helper_funcs);
-	ret = drm_connector_init(drm, connector, &arcpgu_drm_connector_funcs,
-			DRM_MODE_CONNECTOR_HDMIA);
-	if (ret < 0) {
-		dev_err(drm->dev, "failed to initialize drm connector\n");
-		goto error_encoder_cleanup;
-	}
+	/* Link drm_bridge to encoder */
+	bridge->encoder = encoder;
+	encoder->bridge = bridge;
 
-	ret = drm_mode_connector_attach_encoder(connector, &encoder->base);
-	if (ret < 0) {
-		dev_err(drm->dev, "could not attach connector to encoder\n");
-		drm_connector_unregister(connector);
-		goto error_connector_cleanup;
-	}
-
-	arcpgu_connector->encoder_slave = encoder;
-
-	return 0;
-
-error_connector_cleanup:
-	drm_connector_cleanup(connector);
+	ret = drm_bridge_attach(drm, bridge);
+	if (ret)
+		drm_encoder_cleanup(encoder);
 
-error_encoder_cleanup:
-	drm_encoder_cleanup(&encoder->base);
 	return ret;
 }
-- 
2.5.5

[toc] | [next] | [standalone]

#1503862

On 10/19/2016 01:16 PM, Eugeniy Paltsev wrote:
> ARC PGU driver starts crashing on initialization after
> 'commit e12c2f645557 ("drm/i2c: adv7511: Convert to drm_bridge")'
> This happenes because in "arcpgu_drm_hdmi_init" function we get pointer
> of "drm_i2c_encoder_driver" structure, which doesn't exist after
> adv7511 hdmi encoder interface changed from slave encoder to drm bridge.
> So, when we call "encoder_init" function from this structure driver
> crashes.
>
> Bootlog:
> ------------------------------------->8--------------------------------
> [drm] Initialized drm 1.1.0 20060810
> arcpgu e0017000.pgu: arc_pgu ID: 0xabbabaab
> arcpgu e0017000.pgu: assigned reserved memory node frame_buffer@9e000000
> Path: (null)
> CPU: 0 PID: 1 Comm: swapper Not tainted 4.8.0-00001-gb5642252fa01-dirty #8
> task: 9a058000 task.stack: 9a032000
>
> [ECR   ]: 0x00220100 => Invalid Read @ 0x00000004 by insn @ 0x803934e8
> [EFA   ]: 0x00000004
> [BLINK ]: drm_atomic_helper_connector_dpms+0xa6/0x230
> [ERET  ]: drm_atomic_helper_connector_dpms+0xa4/0x230
> [STAT32]: 0x00000846 : K DE       E2 E1
> BTA: 0x8016d949  SP: 0x9a033e34  FP: 0x00000000
> LPS: 0x8036f6fc LPE: 0x8036f700 LPC: 0x00000000
> r00: 0x8063c118 r01: 0x805b98ac r02: 0x00000b11
> r03: 0x00000000 r04: 0x9a010f54 r05: 0x00000000
> r06: 0x00000001 r07: 0x00000000 r08: 0x00000028
> r09: 0x00000001 r10: 0x00000007 r11: 0x00000054
> r12: 0x720a3033
>
> Stack Trace:
>   drm_atomic_helper_connector_dpms+0xa4/0x230
>   arcpgu_drm_hdmi_init+0xbc/0x228
>   arcpgu_probe+0x168/0x244
>   platform_drv_probe+0x26/0x64
>   really_probe+0x1f0/0x32c
>   __driver_attach+0xa8/0xd0
>   bus_for_each_dev+0x3c/0x74
>   bus_add_driver+0xc2/0x184
>   driver_register+0x50/0xec
>   do_one_initcall+0x3a/0x120
>   kernel_init_freeable+0x108/0x1a0
> ------------------------------------->8--------------------------------
>
> Fix ARC PGU driver to be able work with drm bridge hdmi encoder
> interface. The hdmi connector code isn't needed anymore as we expect
> the adv7511 bridge driver to create/manage the connector.

Looks good now.

Reviewed-by: Archit Taneja <architt@codeaurora.org>

>
> Signed-off-by: Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com>
> ---
> Changes for v2:
>    - remove bridge functions call from kms driver
>    - get rid of drm_encoder_slave interface
>    - coding style cleanup
>
>  drivers/gpu/drm/arc/arcpgu_hdmi.c | 159 ++++----------------------------------
>  1 file changed, 17 insertions(+), 142 deletions(-)
>
> diff --git a/drivers/gpu/drm/arc/arcpgu_hdmi.c b/drivers/gpu/drm/arc/arcpgu_hdmi.c
> index b7a8b2a..b69c66b 100644
> --- a/drivers/gpu/drm/arc/arcpgu_hdmi.c
> +++ b/drivers/gpu/drm/arc/arcpgu_hdmi.c
> @@ -14,170 +14,45 @@
>   *
>   */
>
> -#include <drm/drm_crtc_helper.h>
> +#include <drm/drm_crtc.h>
>  #include <drm/drm_encoder_slave.h>
> -#include <drm/drm_atomic_helper.h>
>
>  #include "arcpgu.h"
>
> -struct arcpgu_drm_connector {
> -	struct drm_connector connector;
> -	struct drm_encoder_slave *encoder_slave;
> -};
> -
> -static int arcpgu_drm_connector_get_modes(struct drm_connector *connector)
> -{
> -	const struct drm_encoder_slave_funcs *sfuncs;
> -	struct drm_encoder_slave *slave;
> -	struct arcpgu_drm_connector *con =
> -		container_of(connector, struct arcpgu_drm_connector, connector);
> -
> -	slave = con->encoder_slave;
> -	if (slave == NULL) {
> -		dev_err(connector->dev->dev,
> -			"connector_get_modes: cannot find slave encoder for connector\n");
> -		return 0;
> -	}
> -
> -	sfuncs = slave->slave_funcs;
> -	if (sfuncs->get_modes == NULL)
> -		return 0;
> -
> -	return sfuncs->get_modes(&slave->base, connector);
> -}
> -
> -static enum drm_connector_status
> -arcpgu_drm_connector_detect(struct drm_connector *connector, bool force)
> -{
> -	enum drm_connector_status status = connector_status_unknown;
> -	const struct drm_encoder_slave_funcs *sfuncs;
> -	struct drm_encoder_slave *slave;
> -
> -	struct arcpgu_drm_connector *con =
> -		container_of(connector, struct arcpgu_drm_connector, connector);
> -
> -	slave = con->encoder_slave;
> -	if (slave == NULL) {
> -		dev_err(connector->dev->dev,
> -			"connector_detect: cannot find slave encoder for connector\n");
> -		return status;
> -	}
> -
> -	sfuncs = slave->slave_funcs;
> -	if (sfuncs && sfuncs->detect)
> -		return sfuncs->detect(&slave->base, connector);
> -
> -	dev_err(connector->dev->dev, "connector_detect: could not detect slave funcs\n");
> -	return status;
> -}
> -
> -static void arcpgu_drm_connector_destroy(struct drm_connector *connector)
> -{
> -	drm_connector_unregister(connector);
> -	drm_connector_cleanup(connector);
> -}
> -
> -static const struct drm_connector_helper_funcs
> -arcpgu_drm_connector_helper_funcs = {
> -	.get_modes = arcpgu_drm_connector_get_modes,
> -};
> -
> -static const struct drm_connector_funcs arcpgu_drm_connector_funcs = {
> -	.dpms = drm_helper_connector_dpms,
> -	.reset = drm_atomic_helper_connector_reset,
> -	.detect = arcpgu_drm_connector_detect,
> -	.fill_modes = drm_helper_probe_single_connector_modes,
> -	.destroy = arcpgu_drm_connector_destroy,
> -	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> -	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> -};
> -
> -static struct drm_encoder_helper_funcs arcpgu_drm_encoder_helper_funcs = {
> -	.dpms = drm_i2c_encoder_dpms,
> -	.mode_fixup = drm_i2c_encoder_mode_fixup,
> -	.mode_set = drm_i2c_encoder_mode_set,
> -	.prepare = drm_i2c_encoder_prepare,
> -	.commit = drm_i2c_encoder_commit,
> -	.detect = drm_i2c_encoder_detect,
> -};
> -
>  static struct drm_encoder_funcs arcpgu_drm_encoder_funcs = {
>  	.destroy = drm_encoder_cleanup,
>  };
>
>  int arcpgu_drm_hdmi_init(struct drm_device *drm, struct device_node *np)
>  {
> -	struct arcpgu_drm_connector *arcpgu_connector;
> -	struct drm_i2c_encoder_driver *driver;
> -	struct drm_encoder_slave *encoder;
> -	struct drm_connector *connector;
> -	struct i2c_client *i2c_slave;
> -	int ret;
> +	struct drm_encoder *encoder;
> +	struct drm_bridge *bridge;
> +
> +	int ret = 0;
>
>  	encoder = devm_kzalloc(drm->dev, sizeof(*encoder), GFP_KERNEL);
>  	if (encoder == NULL)
>  		return -ENOMEM;
>
> -	i2c_slave = of_find_i2c_device_by_node(np);
> -	if (!i2c_slave || !i2c_get_clientdata(i2c_slave)) {
> -		dev_err(drm->dev, "failed to find i2c slave encoder\n");
> -		return -EPROBE_DEFER;
> -	}
> -
> -	if (i2c_slave->dev.driver == NULL) {
> -		dev_err(drm->dev, "failed to find i2c slave driver\n");
> +	/* Locate drm bridge from the hdmi encoder DT node */
> +	bridge = of_drm_find_bridge(np);
> +	if (!bridge)
>  		return -EPROBE_DEFER;
> -	}
>
> -	driver =
> -	    to_drm_i2c_encoder_driver(to_i2c_driver(i2c_slave->dev.driver));
> -	ret = driver->encoder_init(i2c_slave, drm, encoder);
> -	if (ret) {
> -		dev_err(drm->dev, "failed to initialize i2c encoder slave\n");
> -		return ret;
> -	}
> -
> -	encoder->base.possible_crtcs = 1;
> -	encoder->base.possible_clones = 0;
> -	ret = drm_encoder_init(drm, &encoder->base, &arcpgu_drm_encoder_funcs,
> +	encoder->possible_crtcs = 1;
> +	encoder->possible_clones = 0;
> +	ret = drm_encoder_init(drm, encoder, &arcpgu_drm_encoder_funcs,
>  			       DRM_MODE_ENCODER_TMDS, NULL);
>  	if (ret)
>  		return ret;
>
> -	drm_encoder_helper_add(&encoder->base,
> -			       &arcpgu_drm_encoder_helper_funcs);
> -
> -	arcpgu_connector = devm_kzalloc(drm->dev, sizeof(*arcpgu_connector),
> -					GFP_KERNEL);
> -	if (!arcpgu_connector) {
> -		ret = -ENOMEM;
> -		goto error_encoder_cleanup;
> -	}
> -
> -	connector = &arcpgu_connector->connector;
> -	drm_connector_helper_add(connector, &arcpgu_drm_connector_helper_funcs);
> -	ret = drm_connector_init(drm, connector, &arcpgu_drm_connector_funcs,
> -			DRM_MODE_CONNECTOR_HDMIA);
> -	if (ret < 0) {
> -		dev_err(drm->dev, "failed to initialize drm connector\n");
> -		goto error_encoder_cleanup;
> -	}
> +	/* Link drm_bridge to encoder */
> +	bridge->encoder = encoder;
> +	encoder->bridge = bridge;
>
> -	ret = drm_mode_connector_attach_encoder(connector, &encoder->base);
> -	if (ret < 0) {
> -		dev_err(drm->dev, "could not attach connector to encoder\n");
> -		drm_connector_unregister(connector);
> -		goto error_connector_cleanup;
> -	}
> -
> -	arcpgu_connector->encoder_slave = encoder;
> -
> -	return 0;
> -
> -error_connector_cleanup:
> -	drm_connector_cleanup(connector);
> +	ret = drm_bridge_attach(drm, bridge);
> +	if (ret)
> +		drm_encoder_cleanup(encoder);
>
> -error_encoder_cleanup:
> -	drm_encoder_cleanup(&encoder->base);
>  	return ret;
>  }
>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web