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
[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
[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 suggestionCan'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
[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
[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
Apparently Analagous Threads
- [PATCH v2 5/9] drm/nouveau: Use drm_connector_for_each_possible_encoder()
- [PATCH v2] drm/connector: Allow max possible encoders to attach to a connector
- [PATCH CI 2/2] drm/connector: Allow max possible encoders to attach to a connector
- [PATCH v2] drm/nouveau: Fix nouveau_connector_ddc_detect()
- [PATCH 6/8] drm/nouveau: Use drm_for_each_connector_encoder_ids()