thr3ads.net - Nouveau - [Nouveau] [PATCH v2 0/9] drm: Third attempt at fixing the fb-helper .best

If this information is useful, please help other people find it:
Share via:

Ville Syrjala

2018-Jun-28 13:13 UTC

[Nouveau] [PATCH v2 0/9] drm: Third attempt at fixing the fb-helper .best_encoder() mess

From: Ville Syrjälä <ville.syrjala at linux.intel.com>

Changes from the previous version mainly involve Danoie's suggestion
of hiding the drm_encoder_find() in the iterator macro. I also polished
the msm and tilcdc cases a bit more with another small helper.

Cc: Alex Deucher <alexander.deucher at amd.com>
Cc: amd-gfx at lists.freedesktop.org
Cc: Ben Skeggs <bskeggs at redhat.com>
Cc: "Christian König" <christian.koenig at amd.com>
Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
Cc: "David (ChunMing) Zhou" <David1.Zhou at amd.com>
Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan at intel.com>
Cc: freedreno at lists.freedesktop.org
Cc: Harry Wentland <harry.wentland at amd.com>
Cc: Jyri Sarha <jsarha at ti.com>
Cc: linux-arm-msm at vger.kernel.org
Cc: nouveau at lists.freedesktop.org
Cc: Rob Clark <robdclark at gmail.com>
Cc: Tomi Valkeinen <tomi.valkeinen at ti.com>

Ville Syrjälä (9):
  drm/fb-helper: Eliminate the .best_encoder() usage
  drm/i915: Nuke intel_mst_best_encoder()
  drm: Add drm_connector_for_each_possible_encoder()
  drm/amdgpu: Use drm_connector_for_each_possible_encoder()
  drm/nouveau: Use drm_connector_for_each_possible_encoder()
  drm/radeon: Use drm_connector_for_each_possible_encoder()
  drm: Add drm_connector_has_possible_encoder()
  drm/msm: Use drm_connector_has_possible_encoder()
  drm/tilcdc: Use drm_connector_has_possible_encoder()

 drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c | 81 ++++++-----------------
 drivers/gpu/drm/amd/amdgpu/dce_virtual.c       | 15 ++---
 drivers/gpu/drm/drm_connector.c                | 44 +++++++++----
 drivers/gpu/drm/drm_fb_helper.c                | 34 +++++-----
 drivers/gpu/drm/drm_probe_helper.c             | 10 +--
 drivers/gpu/drm/i915/intel_dp_mst.c            | 10 ---
 drivers/gpu/drm/msm/dsi/dsi_manager.c          |  8 +--
 drivers/gpu/drm/nouveau/nouveau_connector.c    | 21 +-----
 drivers/gpu/drm/radeon/radeon_connectors.c     | 90 ++++++++------------------
 drivers/gpu/drm/tilcdc/tilcdc_external.c       |  9 ++-
 include/drm/drm_connector.h                    | 16 +++++
 11 files changed, 128 insertions(+), 210 deletions(-)

-- 
2.16.4

Ville Syrjala

2018-Jun-28 13:13 UTC

head link

[Nouveau] [PATCH v2 5/9] drm/nouveau: Use drm_connector_for_each_possible_encoder()

From: Ville Syrjälä <ville.syrjala at linux.intel.com>

Use drm_connector_for_each_possible_encoder() for iterating
connector->encoder_ids[]. A bit more convenient not having
to deal with the implementation details.

v2: Replace drm_for_each_connector_encoder_ids() with
    drm_connector_for_each_possible_encoder() (Daniel)

Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
Cc: Ben Skeggs <bskeggs at redhat.com>
Cc: nouveau at lists.freedesktop.org
Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
Reviewed-by: Alex Deucher <alexander.deucher at amd.com> #v1
---
 drivers/gpu/drm/nouveau/nouveau_connector.c | 21 +++------------------
 1 file changed, 3 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.c
b/drivers/gpu/drm/nouveau/nouveau_connector.c
index 7b557c354307..28d7b42cd666 100644
--- a/drivers/gpu/drm/nouveau/nouveau_connector.c
+++ b/drivers/gpu/drm/nouveau/nouveau_connector.c
@@ -363,19 +363,11 @@ module_param_named(hdmimhz, nouveau_hdmimhz, int, 0400);
 struct nouveau_encoder *
 find_encoder(struct drm_connector *connector, int type)
 {
-	struct drm_device *dev = connector->dev;
 	struct nouveau_encoder *nv_encoder;
 	struct drm_encoder *enc;
-	int i, id;
-
-	for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) {
-		id = connector->encoder_ids[i];
-		if (!id)
-			break;
+	int i;
 
-		enc = drm_encoder_find(dev, NULL, id);
-		if (!enc)
-			continue;
+	drm_connector_for_each_possible_encoder(connector, enc, i) {
 		nv_encoder = nouveau_encoder(enc);
 
 		if (type == DCB_OUTPUT_ANY ||
@@ -436,14 +428,7 @@ nouveau_connector_ddc_detect(struct drm_connector
*connector)
 		}
 	}
 
-	for (i = 0; nv_encoder = NULL, i < DRM_CONNECTOR_MAX_ENCODER; i++) {
-		int id = connector->encoder_ids[i];
-		if (id == 0)
-			break;
-
-		encoder = drm_encoder_find(dev, NULL, id);
-		if (!encoder)
-			continue;
+	drm_connector_for_each_possible_encoder(connector, encoder, i) {
 		nv_encoder = nouveau_encoder(encoder);
 
 		if (nv_encoder->dcb->type == DCB_OUTPUT_DP) {
-- 
2.16.4

Ville Syrjälä

2018-Jun-28 13:47 UTC

head link

[Nouveau] [PATCH v2 0/9] drm: Third attempt at fixing the fb-helper .best_encoder() mess

On Thu, Jun 28, 2018 at 04:13:06PM +0300, Ville Syrjala
wrote:> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> 
> Changes from the previous version mainly involve Danoie's suggestion
Can't type today either: "Daniel's"
> of hiding the drm_encoder_find() in the iterator macro. I also polished
> the msm and tilcdc cases a bit more with another small helper.
> 
> Cc: Alex Deucher <alexander.deucher at amd.com>
> Cc: amd-gfx at lists.freedesktop.org
> Cc: Ben Skeggs <bskeggs at redhat.com>
> Cc: "Christian König" <christian.koenig at amd.com>
> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> Cc: "David (ChunMing) Zhou" <David1.Zhou at amd.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan at intel.com>
> Cc: freedreno at lists.freedesktop.org
> Cc: Harry Wentland <harry.wentland at amd.com>
> Cc: Jyri Sarha <jsarha at ti.com>
> Cc: linux-arm-msm at vger.kernel.org
> Cc: nouveau at lists.freedesktop.org
> Cc: Rob Clark <robdclark at gmail.com>
> Cc: Tomi Valkeinen <tomi.valkeinen at ti.com>
> 
> Ville Syrjälä (9):
>   drm/fb-helper: Eliminate the .best_encoder() usage
>   drm/i915: Nuke intel_mst_best_encoder()
>   drm: Add drm_connector_for_each_possible_encoder()
>   drm/amdgpu: Use drm_connector_for_each_possible_encoder()
>   drm/nouveau: Use drm_connector_for_each_possible_encoder()
>   drm/radeon: Use drm_connector_for_each_possible_encoder()
>   drm: Add drm_connector_has_possible_encoder()
>   drm/msm: Use drm_connector_has_possible_encoder()
>   drm/tilcdc: Use drm_connector_has_possible_encoder()
> 
>  drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c | 81
++++++-----------------
>  drivers/gpu/drm/amd/amdgpu/dce_virtual.c       | 15 ++---
>  drivers/gpu/drm/drm_connector.c                | 44 +++++++++----
>  drivers/gpu/drm/drm_fb_helper.c                | 34 +++++-----
>  drivers/gpu/drm/drm_probe_helper.c             | 10 +--
>  drivers/gpu/drm/i915/intel_dp_mst.c            | 10 ---
>  drivers/gpu/drm/msm/dsi/dsi_manager.c          |  8 +--
>  drivers/gpu/drm/nouveau/nouveau_connector.c    | 21 +-----
>  drivers/gpu/drm/radeon/radeon_connectors.c     | 90
++++++++------------------
>  drivers/gpu/drm/tilcdc/tilcdc_external.c       |  9 ++-
>  include/drm/drm_connector.h                    | 16 +++++
>  11 files changed, 128 insertions(+), 210 deletions(-)
> 
> -- 
> 2.16.4
-- 
Ville Syrjälä
Intel

Dan Carpenter

2018-Jun-30 19:12 UTC

head link

[Nouveau] [PATCH v2 5/9] drm/nouveau: Use drm_connector_for_each_possible_encoder()

Hi Ville,

Thank you for the patch! Perhaps something to improve:

url:   
https://github.com/0day-ci/linux/commits/Ville-Syrjala/drm-Third-attempt-at-fixing-the-fb-helper-best_encoder-mess/20180629-014202
base:   git://people.freedesktop.org/~airlied/linux.git drm-next

smatch warnings:
drivers/gpu/drm/nouveau/nouveau_connector.c:461 nouveau_connector_ddc_detect()
error: uninitialized symbol 'nv_encoder'.

#
https://github.com/0day-ci/linux/commit/7ec8bb65386edfb0b2bdc8e8391eb5e6eac44c06
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout 7ec8bb65386edfb0b2bdc8e8391eb5e6eac44c06
vim +/nv_encoder +461 drivers/gpu/drm/nouveau/nouveau_connector.c

6ee738610 Ben Skeggs      2009-12-11  407  
8777c5c11 Ben Skeggs      2014-06-06  408  static struct nouveau_encoder *
8777c5c11 Ben Skeggs      2014-06-06  409  nouveau_connector_ddc_detect(struct
drm_connector *connector)
6ee738610 Ben Skeggs      2009-12-11  410  {
6ee738610 Ben Skeggs      2009-12-11  411  	struct drm_device *dev =
connector->dev;
1a1841d30 Ben Skeggs      2012-12-10  412  	struct nouveau_connector
*nv_connector = nouveau_connector(connector);
77145f1cb Ben Skeggs      2012-07-31  413  	struct nouveau_drm *drm =
nouveau_drm(dev);
1167c6bc5 Ben Skeggs      2016-05-18  414  	struct nvkm_gpio *gpio =
nvxx_gpio(&drm->client.device);
8777c5c11 Ben Skeggs      2014-06-06  415  	struct nouveau_encoder *nv_encoder;
6d385c0aa Rob Clark       2014-07-17  416  	struct drm_encoder *encoder;
1a1841d30 Ben Skeggs      2012-12-10  417  	int i, panel = -ENODEV;
1a1841d30 Ben Skeggs      2012-12-10  418  
1a1841d30 Ben Skeggs      2012-12-10  419  	/* eDP panels need powering on by us
(if the VBIOS doesn't default it
1a1841d30 Ben Skeggs      2012-12-10  420  	 * to on) before doing any AUX
channel transactions.  LVDS panel power
1a1841d30 Ben Skeggs      2012-12-10  421  	 * is handled by the SOR itself, and
not required for LVDS DDC.
1a1841d30 Ben Skeggs      2012-12-10  422  	 */
1a1841d30 Ben Skeggs      2012-12-10  423  	if (nv_connector->type ==
DCB_CONNECTOR_eDP) {
2ea7249fe Ben Skeggs      2015-08-20  424  		panel = nvkm_gpio_get(gpio, 0,
DCB_GPIO_PANEL_POWER, 0xff);
1a1841d30 Ben Skeggs      2012-12-10  425  		if (panel == 0) {
2ea7249fe Ben Skeggs      2015-08-20  426  			nvkm_gpio_set(gpio, 0,
DCB_GPIO_PANEL_POWER, 0xff, 1);
1a1841d30 Ben Skeggs      2012-12-10  427  			msleep(300);
1a1841d30 Ben Skeggs      2012-12-10  428  		}
1a1841d30 Ben Skeggs      2012-12-10  429  	}
6ee738610 Ben Skeggs      2009-12-11  430  
7ec8bb653 Ville Syrjälä   2018-06-28  431  
drm_connector_for_each_possible_encoder(connector, encoder, i) {
6d385c0aa Rob Clark       2014-07-17  432  		nv_encoder =
nouveau_encoder(encoder);
                                                       
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
If we enter the loop that means nv_encoder is non-NULL.  Smatch can't
prove that we always enter the loop in this case for whatever reason but
my guess is that we always do.

4ca2b7120 Francisco Jerez 2010-08-08  433  
8777c5c11 Ben Skeggs      2014-06-06  434  		if (nv_encoder->dcb->type ==
DCB_OUTPUT_DP) {
8777c5c11 Ben Skeggs      2014-06-06  435  			int ret =
nouveau_dp_detect(nv_encoder);
52aa30f25 Ben Skeggs      2016-11-04  436  			if (ret == NOUVEAU_DP_MST)
52aa30f25 Ben Skeggs      2016-11-04  437  				return NULL;
52aa30f25 Ben Skeggs      2016-11-04  438  			if (ret == NOUVEAU_DP_SST)
8777c5c11 Ben Skeggs      2014-06-06  439  				break;
8777c5c11 Ben Skeggs      2014-06-06  440  		} else
39c1c9011 Lukas Wunner    2016-01-11  441  		if ((vga_switcheroo_handler_flags()
&
39c1c9011 Lukas Wunner    2016-01-11  442  		     VGA_SWITCHEROO_CAN_SWITCH_DDC)
&&
39c1c9011 Lukas Wunner    2016-01-11  443  		    nv_encoder->dcb->type ==
DCB_OUTPUT_LVDS &&
39c1c9011 Lukas Wunner    2016-01-11  444  		    nv_encoder->i2c) {
39c1c9011 Lukas Wunner    2016-01-11  445  			int ret;
39c1c9011 Lukas Wunner    2016-01-11  446  		
vga_switcheroo_lock_ddc(dev->pdev);
39c1c9011 Lukas Wunner    2016-01-11  447  			ret =
nvkm_probe_i2c(nv_encoder->i2c, 0x50);
39c1c9011 Lukas Wunner    2016-01-11  448  		
vga_switcheroo_unlock_ddc(dev->pdev);
39c1c9011 Lukas Wunner    2016-01-11  449  			if (ret)
39c1c9011 Lukas Wunner    2016-01-11  450  				break;
39c1c9011 Lukas Wunner    2016-01-11  451  		} else
8777c5c11 Ben Skeggs      2014-06-06  452  		if (nv_encoder->i2c) {
2aa5eac51 Ben Skeggs      2015-08-20  453  			if
(nvkm_probe_i2c(nv_encoder->i2c, 0x50))
1a1841d30 Ben Skeggs      2012-12-10  454  				break;
6ee738610 Ben Skeggs      2009-12-11  455  		}
6ee738610 Ben Skeggs      2009-12-11  456  	}
6ee738610 Ben Skeggs      2009-12-11  457  
1a1841d30 Ben Skeggs      2012-12-10  458  	/* eDP panel not detected, restore
panel power GPIO to previous
1a1841d30 Ben Skeggs      2012-12-10  459  	 * state to avoid confusing the SOR
for other output types.
1a1841d30 Ben Skeggs      2012-12-10  460  	 */
8777c5c11 Ben Skeggs      2014-06-06 @461  	if (!nv_encoder && panel ==
0)
                                                    ^^^^^^^^^^^
So testing for NULL doesn't make sense.  It's either non-NULL if we
entered the loop at least once or it's uninitialized if we didn't enter
the loop.

2ea7249fe Ben Skeggs      2015-08-20  462  		nvkm_gpio_set(gpio, 0,
DCB_GPIO_PANEL_POWER, 0xff, panel);
1a1841d30 Ben Skeggs      2012-12-10  463  
8777c5c11 Ben Skeggs      2014-06-06  464  	return nv_encoder;
6ee738610 Ben Skeggs      2009-12-11  465  }
6ee738610 Ben Skeggs      2009-12-11  466  


---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

Ville Syrjala

2018-Jul-02 15:29 UTC

head link

[Nouveau] [PATCH v3 5/9] drm/nouveau: Use drm_connector_for_each_possible_encoder()

From: Ville Syrjälä <ville.syrjala at linux.intel.com>

Use drm_connector_for_each_possible_encoder() for iterating
connector->encoder_ids[]. A bit more convenient not having
to deal with the implementation details.

v2: Replace drm_for_each_connector_encoder_ids() with
    drm_connector_for_each_possible_encoder() (Daniel)
v3: Initialize nv_encoder to NULL to shut up gcc/smatch

Cc: Dan Carpenter <dan.carpenter at oracle.com>
Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
Cc: Ben Skeggs <bskeggs at redhat.com>
Cc: nouveau at lists.freedesktop.org
Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
Reviewed-by: Alex Deucher <alexander.deucher at amd.com> #v1
---
 drivers/gpu/drm/nouveau/nouveau_connector.c | 23 ++++-------------------
 1 file changed, 4 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.c
b/drivers/gpu/drm/nouveau/nouveau_connector.c
index 7b557c354307..bb46c1d489cf 100644
--- a/drivers/gpu/drm/nouveau/nouveau_connector.c
+++ b/drivers/gpu/drm/nouveau/nouveau_connector.c
@@ -363,19 +363,11 @@ module_param_named(hdmimhz, nouveau_hdmimhz, int, 0400);
 struct nouveau_encoder *
 find_encoder(struct drm_connector *connector, int type)
 {
-	struct drm_device *dev = connector->dev;
 	struct nouveau_encoder *nv_encoder;
 	struct drm_encoder *enc;
-	int i, id;
-
-	for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) {
-		id = connector->encoder_ids[i];
-		if (!id)
-			break;
+	int i;
 
-		enc = drm_encoder_find(dev, NULL, id);
-		if (!enc)
-			continue;
+	drm_connector_for_each_possible_encoder(connector, enc, i) {
 		nv_encoder = nouveau_encoder(enc);
 
 		if (type == DCB_OUTPUT_ANY ||
@@ -420,7 +412,7 @@ nouveau_connector_ddc_detect(struct drm_connector
*connector)
 	struct nouveau_connector *nv_connector = nouveau_connector(connector);
 	struct nouveau_drm *drm = nouveau_drm(dev);
 	struct nvkm_gpio *gpio = nvxx_gpio(&drm->client.device);
-	struct nouveau_encoder *nv_encoder;
+	struct nouveau_encoder *nv_encoder = NULL;
 	struct drm_encoder *encoder;
 	int i, panel = -ENODEV;
 
@@ -436,14 +428,7 @@ nouveau_connector_ddc_detect(struct drm_connector
*connector)
 		}
 	}
 
-	for (i = 0; nv_encoder = NULL, i < DRM_CONNECTOR_MAX_ENCODER; i++) {
-		int id = connector->encoder_ids[i];
-		if (id == 0)
-			break;
-
-		encoder = drm_encoder_find(dev, NULL, id);
-		if (!encoder)
-			continue;
+	drm_connector_for_each_possible_encoder(connector, encoder, i) {
 		nv_encoder = nouveau_encoder(encoder);
 
 		if (nv_encoder->dcb->type == DCB_OUTPUT_DP) {
-- 
2.16.4

Possibly Parallel Threads

Search for more apparently analagous threads

Nouveau - Jun 2018 - [PATCH v2 0/9] drm: Third attempt at fixing the fb-helper .best_encoder() mess

[Nouveau] [PATCH v2 0/9] drm: Third attempt at fixing the fb-helper .best_encoder() mess

[Nouveau] [PATCH v2 5/9] drm/nouveau: Use drm_connector_for_each_possible_encoder()

[Nouveau] [PATCH v2 0/9] drm: Third attempt at fixing the fb-helper .best_encoder() mess

[Nouveau] [PATCH v2 5/9] drm/nouveau: Use drm_connector_for_each_possible_encoder()

[Nouveau] [PATCH v3 5/9] drm/nouveau: Use drm_connector_for_each_possible_encoder()

Possibly Parallel Threads