[PATCH 0/4] Some initial tidy-ups and refactoring

[PATCH 0/4] Some initial tidy-ups and refactoring

[Joel Holdsworth]

The patches contain some tidy-up work, and refactoring that has arisen
as a by-product of my initial work on adding TV support to nv50.

Joel Holdsworth (4):
  drm/nouveau: Removed unneeded include in nvc0_fence.c
  drm/nouveau: Replaced magic numbers with defines from nouveau_reg
  drm/nouveau: Replaced copy-pasted nv_wait with nv50_wait_dpms_ctrl
    helper
  drm/nouveau: Refactored encoder create functions into common
    nv50_encoder_create helper

 drivers/gpu/drm/nouveau/core/engine/disp/dacnv50.c |  33 ++--
 drivers/gpu/drm/nouveau/nv50_display.c             | 178 ++++++++-------------
 drivers/gpu/drm/nouveau/nvc0_fence.c               |   2 -
 3 files changed, 93 insertions(+), 120 deletions(-)

-- 
1.9.1

[PATCH 1/4] drm/nouveau: Removed unneeded include in nvc0_fence.c

[Joel Holdsworth]

---
 drivers/gpu/drm/nouveau/nvc0_fence.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nvc0_fence.c b/drivers/gpu/drm/nouveau/nvc0_fence.c
index becf19a..c9fc6f7 100644
--- a/drivers/gpu/drm/nouveau/nvc0_fence.c
+++ b/drivers/gpu/drm/nouveau/nvc0_fence.c
@@ -26,8 +26,6 @@
 #include "nouveau_dma.h"
 #include "nouveau_fence.h"
 
-#include "nv50_display.h"
-
 static int
 nvc0_fence_emit32(struct nouveau_channel *chan, u64 virtual, u32 sequence)
 {
-- 
1.9.1

[PATCH 2/4] drm/nouveau: Replaced magic numbers with defines from nouveau_reg

[Joel Holdsworth]

---
 drivers/gpu/drm/nouveau/core/engine/disp/dacnv50.c | 27 +++++++++++++---------
 1 file changed, 16 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/core/engine/disp/dacnv50.c b/drivers/gpu/drm/nouveau/core/engine/disp/dacnv50.c
index b36addf..61e29d6 100644
--- a/drivers/gpu/drm/nouveau/core/engine/disp/dacnv50.c
+++ b/drivers/gpu/drm/nouveau/core/engine/disp/dacnv50.c
@@ -31,11 +31,12 @@
 #include <subdev/timer.h>
 
 #include "nv50.h"
+#include "nouveau_reg.h"
 
 int
 nv50_dac_power(NV50_DISP_MTHD_V1)
 {
-	const u32 doff = outp->or * 0x800;
+	const u32 dpms_ctrl = NV50_PDISPLAY_DAC_DPMS_CTRL(outp->or);
 	union {
 		struct nv50_disp_dac_pwr_v0 v0;
 	} *args = data;
@@ -55,19 +56,21 @@ nv50_dac_power(NV50_DISP_MTHD_V1)
 	} else
 		return ret;
 
-	nv_wait(priv, 0x61a004 + doff, 0x80000000, 0x00000000);
-	nv_mask(priv, 0x61a004 + doff, 0xc000007f, 0x80000000 | stat);
-	nv_wait(priv, 0x61a004 + doff, 0x80000000, 0x00000000);
+	nv_wait(priv, dpms_ctrl, NV50_PDISPLAY_DAC_DPMS_CTRL_PENDING, 0);
+	nv_mask(priv, dpms_ctrl, NV50_PDISPLAY_DAC_DPMS_CTRL_PENDING | 0x4000007f,
+		NV50_PDISPLAY_DAC_DPMS_CTRL_PENDING | stat);
+	nv_wait(priv, dpms_ctrl, NV50_PDISPLAY_DAC_DPMS_CTRL_PENDING, 0);
 	return 0;
 }
 
 int
 nv50_dac_sense(NV50_DISP_MTHD_V1)
 {
+	const u32 dpms_ctrl = NV50_PDISPLAY_DAC_DPMS_CTRL(outp->or);
+	const u32 load_ctrl = NV50_PDISPLAY_DAC_LOAD_CTRL(outp->or);
 	union {
 		struct nv50_disp_dac_load_v0 v0;
 	} *args = data;
-	const u32 doff = outp->or * 0x800;
 	u32 loadval;
 	int ret;
 
@@ -81,16 +84,18 @@ nv50_dac_sense(NV50_DISP_MTHD_V1)
 	} else
 		return ret;
 
-	nv_mask(priv, 0x61a004 + doff, 0x807f0000, 0x80150000);
-	nv_wait(priv, 0x61a004 + doff, 0x80000000, 0x00000000);
+	nv_mask(priv, dpms_ctrl, NV50_PDISPLAY_DAC_DPMS_CTRL_PENDING | 0x007f0000,
+		NV50_PDISPLAY_DAC_DPMS_CTRL_PENDING | 0x00150000);
+	nv_wait(priv, dpms_ctrl, NV50_PDISPLAY_DAC_DPMS_CTRL_PENDING, 0);
 
-	nv_wr32(priv, 0x61a00c + doff, 0x00100000 | loadval);
+	nv_wr32(priv, load_ctrl, 0x00100000 | loadval);
 	mdelay(9);
 	udelay(500);
-	loadval = nv_mask(priv, 0x61a00c + doff, 0xffffffff, 0x00000000);
+	loadval = nv_mask(priv, load_ctrl, 0xffffffff, 0x00000000);
 
-	nv_mask(priv, 0x61a004 + doff, 0x807f0000, 0x80550000);
-	nv_wait(priv, 0x61a004 + doff, 0x80000000, 0x00000000);
+	nv_mask(priv, dpms_ctrl, NV50_PDISPLAY_DAC_DPMS_CTRL_PENDING | 0x007f0000,
+		NV50_PDISPLAY_DAC_DPMS_CTRL_PENDING | 0x00550000);
+	nv_wait(priv, dpms_ctrl, NV50_PDISPLAY_DAC_DPMS_CTRL_PENDING, 0);
 
 	nv_debug(priv, "DAC%d sense: 0x%08x\n", outp->or, loadval);
 	if (!(loadval & 0x80000000))
-- 
1.9.1

[PATCH 3/4] drm/nouveau: Replaced copy-pasted nv_wait with nv50_wait_dpms_ctrl helper

[Joel Holdsworth]

---
 drivers/gpu/drm/nouveau/core/engine/disp/dacnv50.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/core/engine/disp/dacnv50.c b/drivers/gpu/drm/nouveau/core/engine/disp/dacnv50.c
index 61e29d6..98bded6 100644
--- a/drivers/gpu/drm/nouveau/core/engine/disp/dacnv50.c
+++ b/drivers/gpu/drm/nouveau/core/engine/disp/dacnv50.c
@@ -33,6 +33,12 @@
 #include "nv50.h"
 #include "nouveau_reg.h"
 
+static void
+nv50_wait_dpms_ctrl(struct nv50_disp_priv *priv, const u32 dpms_ctrl)
+{
+	nv_wait(priv, dpms_ctrl, NV50_PDISPLAY_DAC_DPMS_CTRL_PENDING, 0);
+}
+
 int
 nv50_dac_power(NV50_DISP_MTHD_V1)
 {
@@ -56,10 +62,10 @@ nv50_dac_power(NV50_DISP_MTHD_V1)
 	} else
 		return ret;
 
-	nv_wait(priv, dpms_ctrl, NV50_PDISPLAY_DAC_DPMS_CTRL_PENDING, 0);
+	nv50_wait_dpms_ctrl(priv, dpms_ctrl);
 	nv_mask(priv, dpms_ctrl, NV50_PDISPLAY_DAC_DPMS_CTRL_PENDING | 0x4000007f,
 		NV50_PDISPLAY_DAC_DPMS_CTRL_PENDING | stat);
-	nv_wait(priv, dpms_ctrl, NV50_PDISPLAY_DAC_DPMS_CTRL_PENDING, 0);
+	nv50_wait_dpms_ctrl(priv, dpms_ctrl);
 	return 0;
 }
 
@@ -86,7 +92,7 @@ nv50_dac_sense(NV50_DISP_MTHD_V1)
 
 	nv_mask(priv, dpms_ctrl, NV50_PDISPLAY_DAC_DPMS_CTRL_PENDING | 0x007f0000,
 		NV50_PDISPLAY_DAC_DPMS_CTRL_PENDING | 0x00150000);
-	nv_wait(priv, dpms_ctrl, NV50_PDISPLAY_DAC_DPMS_CTRL_PENDING, 0);
+	nv50_wait_dpms_ctrl(priv, dpms_ctrl);
 
 	nv_wr32(priv, load_ctrl, 0x00100000 | loadval);
 	mdelay(9);
@@ -95,7 +101,7 @@ nv50_dac_sense(NV50_DISP_MTHD_V1)
 
 	nv_mask(priv, dpms_ctrl, NV50_PDISPLAY_DAC_DPMS_CTRL_PENDING | 0x007f0000,
 		NV50_PDISPLAY_DAC_DPMS_CTRL_PENDING | 0x00550000);
-	nv_wait(priv, dpms_ctrl, NV50_PDISPLAY_DAC_DPMS_CTRL_PENDING, 0);
+	nv50_wait_dpms_ctrl(priv, dpms_ctrl);
 
 	nv_debug(priv, "DAC%d sense: 0x%08x\n", outp->or, loadval);
 	if (!(loadval & 0x80000000))
-- 
1.9.1

[PATCH 4/4] drm/nouveau: Refactored encoder create functions into common nv50_encoder_create helper

[Joel Holdsworth]

---
 drivers/gpu/drm/nouveau/nv50_display.c | 178 +++++++++++++--------------------
 1 file changed, 71 insertions(+), 107 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nv50_display.c b/drivers/gpu/drm/nouveau/nv50_display.c
index 03949ea..048e03e 100644
--- a/drivers/gpu/drm/nouveau/nv50_display.c
+++ b/drivers/gpu/drm/nouveau/nv50_display.c
@@ -1442,6 +1442,37 @@ out:
 }
 
 /******************************************************************************
+ * Encoder Helpers
+ *****************************************************************************/
+
+static int
+nv50_encoder_create(struct drm_connector *connector, struct dcb_output *dcbe,
+	int type, int init_dpms, struct nouveau_i2c_port *i2c_port,
+	const struct drm_encoder_funcs *func,
+	const struct drm_encoder_helper_funcs *hfunc)
+{
+	struct nouveau_encoder *nv_encoder;
+	struct drm_encoder *encoder;
+
+	nv_encoder = kzalloc(sizeof(*nv_encoder), GFP_KERNEL);
+	if (!nv_encoder)
+		return -ENOMEM;
+	nv_encoder->dcb = dcbe;
+	nv_encoder->or = ffs(dcbe->or) - 1;
+	nv_encoder->i2c = i2c_port;
+	nv_encoder->last_dpms = init_dpms;
+
+	encoder = to_drm_encoder(nv_encoder);
+	encoder->possible_crtcs = dcbe->heads;
+	encoder->possible_clones = 0;
+	drm_encoder_init(connector->dev, encoder, func, type);
+	drm_encoder_helper_add(encoder, hfunc);
+
+	drm_mode_connector_attach_encoder(connector, encoder);
+	return 0;
+}
+
+/******************************************************************************
  * DAC
  *****************************************************************************/
 static void
@@ -1618,32 +1649,6 @@ static const struct drm_encoder_funcs nv50_dac_func = {
 	.destroy = nv50_dac_destroy,
 };
 
-static int
-nv50_dac_create(struct drm_connector *connector, struct dcb_output *dcbe)
-{
-	struct nouveau_drm *drm = nouveau_drm(connector->dev);
-	struct nouveau_i2c *i2c = nvkm_i2c(&drm->device);
-	struct nouveau_encoder *nv_encoder;
-	struct drm_encoder *encoder;
-	int type = DRM_MODE_ENCODER_DAC;
-
-	nv_encoder = kzalloc(sizeof(*nv_encoder), GFP_KERNEL);
-	if (!nv_encoder)
-		return -ENOMEM;
-	nv_encoder->dcb = dcbe;
-	nv_encoder->or = ffs(dcbe->or) - 1;
-	nv_encoder->i2c = i2c->find(i2c, dcbe->i2c_index);
-
-	encoder = to_drm_encoder(nv_encoder);
-	encoder->possible_crtcs = dcbe->heads;
-	encoder->possible_clones = 0;
-	drm_encoder_init(connector->dev, encoder, &nv50_dac_func, type);
-	drm_encoder_helper_add(encoder, &nv50_dac_hfunc);
-
-	drm_mode_connector_attach_encoder(connector, encoder);
-	return 0;
-}
-
 /******************************************************************************
  * Audio
  *****************************************************************************/
@@ -2017,42 +2022,6 @@ static const struct drm_encoder_funcs nv50_sor_func = {
 	.destroy = nv50_sor_destroy,
 };
 
-static int
-nv50_sor_create(struct drm_connector *connector, struct dcb_output *dcbe)
-{
-	struct nouveau_drm *drm = nouveau_drm(connector->dev);
-	struct nouveau_i2c *i2c = nvkm_i2c(&drm->device);
-	struct nouveau_encoder *nv_encoder;
-	struct drm_encoder *encoder;
-	int type;
-
-	switch (dcbe->type) {
-	case DCB_OUTPUT_LVDS: type = DRM_MODE_ENCODER_LVDS; break;
-	case DCB_OUTPUT_TMDS:
-	case DCB_OUTPUT_DP:
-	default:
-		type = DRM_MODE_ENCODER_TMDS;
-		break;
-	}
-
-	nv_encoder = kzalloc(sizeof(*nv_encoder), GFP_KERNEL);
-	if (!nv_encoder)
-		return -ENOMEM;
-	nv_encoder->dcb = dcbe;
-	nv_encoder->or = ffs(dcbe->or) - 1;
-	nv_encoder->i2c = i2c->find(i2c, dcbe->i2c_index);
-	nv_encoder->last_dpms = DRM_MODE_DPMS_OFF;
-
-	encoder = to_drm_encoder(nv_encoder);
-	encoder->possible_crtcs = dcbe->heads;
-	encoder->possible_clones = 0;
-	drm_encoder_init(connector->dev, encoder, &nv50_sor_func, type);
-	drm_encoder_helper_add(encoder, &nv50_sor_hfunc);
-
-	drm_mode_connector_attach_encoder(connector, encoder);
-	return 0;
-}
-
 /******************************************************************************
  * PIOR
  *****************************************************************************/
@@ -2198,46 +2167,6 @@ static const struct drm_encoder_funcs nv50_pior_func = {
 	.destroy = nv50_pior_destroy,
 };
 
-static int
-nv50_pior_create(struct drm_connector *connector, struct dcb_output *dcbe)
-{
-	struct nouveau_drm *drm = nouveau_drm(connector->dev);
-	struct nouveau_i2c *i2c = nvkm_i2c(&drm->device);
-	struct nouveau_i2c_port *ddc = NULL;
-	struct nouveau_encoder *nv_encoder;
-	struct drm_encoder *encoder;
-	int type;
-
-	switch (dcbe->type) {
-	case DCB_OUTPUT_TMDS:
-		ddc  = i2c->find_type(i2c, NV_I2C_TYPE_EXTDDC(dcbe->extdev));
-		type = DRM_MODE_ENCODER_TMDS;
-		break;
-	case DCB_OUTPUT_DP:
-		ddc  = i2c->find_type(i2c, NV_I2C_TYPE_EXTAUX(dcbe->extdev));
-		type = DRM_MODE_ENCODER_TMDS;
-		break;
-	default:
-		return -ENODEV;
-	}
-
-	nv_encoder = kzalloc(sizeof(*nv_encoder), GFP_KERNEL);
-	if (!nv_encoder)
-		return -ENOMEM;
-	nv_encoder->dcb = dcbe;
-	nv_encoder->or = ffs(dcbe->or) - 1;
-	nv_encoder->i2c = ddc;
-
-	encoder = to_drm_encoder(nv_encoder);
-	encoder->possible_crtcs = dcbe->heads;
-	encoder->possible_clones = 0;
-	drm_encoder_init(connector->dev, encoder, &nv50_pior_func, type);
-	drm_encoder_helper_add(encoder, &nv50_pior_hfunc);
-
-	drm_mode_connector_attach_encoder(connector, encoder);
-	return 0;
-}
-
 /******************************************************************************
  * Framebuffer
  *****************************************************************************/
@@ -2437,10 +2366,12 @@ nv50_display_create(struct drm_device *dev)
 {
 	struct nvif_device *device = &nouveau_drm(dev)->device;
 	struct nouveau_drm *drm = nouveau_drm(dev);
+	struct nouveau_i2c *i2c = nvkm_i2c(&drm->device);
 	struct dcb_table *dcb = &drm->vbios.dcb;
 	struct drm_connector *connector, *tmp;
 	struct nv50_disp *disp;
 	struct dcb_output *dcbe;
+	struct nouveau_i2c_port *i2c_port;
 	int crtcs, ret, i;
 
 	disp = kzalloc(sizeof(*disp), GFP_KERNEL);
@@ -2498,21 +2429,54 @@ nv50_display_create(struct drm_device *dev)
 			continue;
 
 		if (dcbe->location == DCB_LOC_ON_CHIP) {
+			i2c_port = i2c->find(i2c, dcbe->i2c_index);
+
 			switch (dcbe->type) {
+			case DCB_OUTPUT_ANALOG:
+				ret = nv50_encoder_create(connector, dcbe,
+					DRM_MODE_ENCODER_DAC, DRM_MODE_DPMS_ON,
+					i2c_port, &nv50_dac_func,
+					&nv50_dac_hfunc);
+				break;
 			case DCB_OUTPUT_TMDS:
-			case DCB_OUTPUT_LVDS:
 			case DCB_OUTPUT_DP:
-				ret = nv50_sor_create(connector, dcbe);
+				ret = nv50_encoder_create(connector, dcbe,
+					DRM_MODE_ENCODER_TMDS,
+					DRM_MODE_DPMS_OFF,
+					i2c_port, &nv50_sor_func,
+					&nv50_sor_hfunc);
 				break;
-			case DCB_OUTPUT_ANALOG:
-				ret = nv50_dac_create(connector, dcbe);
+			case DCB_OUTPUT_LVDS:
+				ret = nv50_encoder_create(connector, dcbe,
+					DRM_MODE_ENCODER_LVDS, DRM_MODE_DPMS_ON,
+					i2c_port, &nv50_sor_func,
+					&nv50_sor_hfunc);
 				break;
 			default:
 				ret = -ENODEV;
 				break;
 			}
 		} else {
-			ret = nv50_pior_create(connector, dcbe);
+			switch (dcbe->type) {
+			case DCB_OUTPUT_TMDS:
+				i2c_port = i2c->find_type(i2c,
+					NV_I2C_TYPE_EXTDDC(dcbe->extdev));
+				ret = nv50_encoder_create(connector, dcbe,
+					DRM_MODE_ENCODER_TMDS, DRM_MODE_DPMS_ON,
+					i2c_port, &nv50_pior_func,
+					&nv50_pior_hfunc);
+				break;
+			case DCB_OUTPUT_DP:
+				i2c_port = i2c->find_type(i2c,
+					NV_I2C_TYPE_EXTAUX(dcbe->extdev));
+				ret = nv50_encoder_create(connector, dcbe,
+					DRM_MODE_ENCODER_TMDS, DRM_MODE_DPMS_ON,
+					i2c_port, &nv50_pior_func,
+					&nv50_pior_hfunc);
+				break;
+			default:
+				ret = -ENODEV;
+			}
 		}
 
 		if (ret) {
-- 
1.9.1

Re: [PATCH 2/4] drm/nouveau: Replaced magic numbers with defines from nouveau_reg

[Ilia Mirkin]

As you've no doubt noticed, using "magic" numbers is the convention
used throughout the driver (except dispnv04 which is in a world of its
own), rather than the (occasional) macros. It's a trade-off, since the
magic numbers are a little harder to read, but they're much easier to
match up in traces. Eventually you start to remember what the various
numbers mean... or you can look them up with the "lookup" utility in
envytools.

On Tue, Aug 12, 2014 at 5:05 AM, Joel Holdsworth
<joel.holdsworth-EQPyqoE4rJiYoAmmQSGB5AC/G2K4zDHf@public.gmane.org> wrote:
> ---
>  drivers/gpu/drm/nouveau/core/engine/disp/dacnv50.c | 27 +++++++++++++---------
>  1 file changed, 16 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/nouveau/core/engine/disp/dacnv50.c b/drivers/gpu/drm/nouveau/core/engine/disp/dacnv50.c
> index b36addf..61e29d6 100644
> --- a/drivers/gpu/drm/nouveau/core/engine/disp/dacnv50.c
> +++ b/drivers/gpu/drm/nouveau/core/engine/disp/dacnv50.c
> @@ -31,11 +31,12 @@
>  #include <subdev/timer.h>
>
>  #include "nv50.h"
> +#include "nouveau_reg.h"
>
>  int
>  nv50_dac_power(NV50_DISP_MTHD_V1)
>  {
> -       const u32 doff = outp->or * 0x800;
> +       const u32 dpms_ctrl = NV50_PDISPLAY_DAC_DPMS_CTRL(outp->or);
>         union {
>                 struct nv50_disp_dac_pwr_v0 v0;
>         } *args = data;
> @@ -55,19 +56,21 @@ nv50_dac_power(NV50_DISP_MTHD_V1)
>         } else
>                 return ret;
>
> -       nv_wait(priv, 0x61a004 + doff, 0x80000000, 0x00000000);
> -       nv_mask(priv, 0x61a004 + doff, 0xc000007f, 0x80000000 | stat);
> -       nv_wait(priv, 0x61a004 + doff, 0x80000000, 0x00000000);
> +       nv_wait(priv, dpms_ctrl, NV50_PDISPLAY_DAC_DPMS_CTRL_PENDING, 0);
> +       nv_mask(priv, dpms_ctrl, NV50_PDISPLAY_DAC_DPMS_CTRL_PENDING | 0x4000007f,
> +               NV50_PDISPLAY_DAC_DPMS_CTRL_PENDING | stat);
> +       nv_wait(priv, dpms_ctrl, NV50_PDISPLAY_DAC_DPMS_CTRL_PENDING, 0);
>         return 0;
>  }
>
>  int
>  nv50_dac_sense(NV50_DISP_MTHD_V1)
>  {
> +       const u32 dpms_ctrl = NV50_PDISPLAY_DAC_DPMS_CTRL(outp->or);
> +       const u32 load_ctrl = NV50_PDISPLAY_DAC_LOAD_CTRL(outp->or);
>         union {
>                 struct nv50_disp_dac_load_v0 v0;
>         } *args = data;
> -       const u32 doff = outp->or * 0x800;
>         u32 loadval;
>         int ret;
>
> @@ -81,16 +84,18 @@ nv50_dac_sense(NV50_DISP_MTHD_V1)
>         } else
>                 return ret;
>
> -       nv_mask(priv, 0x61a004 + doff, 0x807f0000, 0x80150000);
> -       nv_wait(priv, 0x61a004 + doff, 0x80000000, 0x00000000);
> +       nv_mask(priv, dpms_ctrl, NV50_PDISPLAY_DAC_DPMS_CTRL_PENDING | 0x007f0000,
> +               NV50_PDISPLAY_DAC_DPMS_CTRL_PENDING | 0x00150000);
> +       nv_wait(priv, dpms_ctrl, NV50_PDISPLAY_DAC_DPMS_CTRL_PENDING, 0);
>
> -       nv_wr32(priv, 0x61a00c + doff, 0x00100000 | loadval);
> +       nv_wr32(priv, load_ctrl, 0x00100000 | loadval);
>         mdelay(9);
>         udelay(500);
> -       loadval = nv_mask(priv, 0x61a00c + doff, 0xffffffff, 0x00000000);
> +       loadval = nv_mask(priv, load_ctrl, 0xffffffff, 0x00000000);
>
> -       nv_mask(priv, 0x61a004 + doff, 0x807f0000, 0x80550000);
> -       nv_wait(priv, 0x61a004 + doff, 0x80000000, 0x00000000);
> +       nv_mask(priv, dpms_ctrl, NV50_PDISPLAY_DAC_DPMS_CTRL_PENDING | 0x007f0000,
> +               NV50_PDISPLAY_DAC_DPMS_CTRL_PENDING | 0x00550000);
> +       nv_wait(priv, dpms_ctrl, NV50_PDISPLAY_DAC_DPMS_CTRL_PENDING, 0);
>
>         nv_debug(priv, "DAC%d sense: 0x%08x\n", outp->or, loadval);
>         if (!(loadval & 0x80000000))
> --
> 1.9.1
>
> _______________________________________________
> Nouveau mailing list
> Nouveau-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> http://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [PATCH 3/4] drm/nouveau: Replaced copy-pasted nv_wait with nv50_wait_dpms_ctrl helper

[Ilia Mirkin]

On Tue, Aug 12, 2014 at 5:05 AM, Joel Holdsworth
<joel.holdsworth-EQPyqoE4rJiYoAmmQSGB5AC/G2K4zDHf@public.gmane.org> wrote:
> ---
>  drivers/gpu/drm/nouveau/core/engine/disp/dacnv50.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/nouveau/core/engine/disp/dacnv50.c b/drivers/gpu/drm/nouveau/core/engine/disp/dacnv50.c
> index 61e29d6..98bded6 100644
> --- a/drivers/gpu/drm/nouveau/core/engine/disp/dacnv50.c
> +++ b/drivers/gpu/drm/nouveau/core/engine/disp/dacnv50.c
> @@ -33,6 +33,12 @@
>  #include "nv50.h"
>  #include "nouveau_reg.h"
>
> +static void
> +nv50_wait_dpms_ctrl(struct nv50_disp_priv *priv, const u32 dpms_ctrl)
> +{
> +       nv_wait(priv, dpms_ctrl, NV50_PDISPLAY_DAC_DPMS_CTRL_PENDING, 0);
> +}
> +
>  int
>  nv50_dac_power(NV50_DISP_MTHD_V1)
>  {
> @@ -56,10 +62,10 @@ nv50_dac_power(NV50_DISP_MTHD_V1)
>         } else
>                 return ret;
>
> -       nv_wait(priv, dpms_ctrl, NV50_PDISPLAY_DAC_DPMS_CTRL_PENDING, 0);
> +       nv50_wait_dpms_ctrl(priv, dpms_ctrl);
>         nv_mask(priv, dpms_ctrl, NV50_PDISPLAY_DAC_DPMS_CTRL_PENDING | 0x4000007f,
>                 NV50_PDISPLAY_DAC_DPMS_CTRL_PENDING | stat);
> -       nv_wait(priv, dpms_ctrl, NV50_PDISPLAY_DAC_DPMS_CTRL_PENDING, 0);
> +       nv50_wait_dpms_ctrl(priv, dpms_ctrl);

This feels like it's hiding what's going on. If it was some complex
procedure done over and over -- fine, but in this case, it's just one
line...

>         return 0;
>  }
>
> @@ -86,7 +92,7 @@ nv50_dac_sense(NV50_DISP_MTHD_V1)
>
>         nv_mask(priv, dpms_ctrl, NV50_PDISPLAY_DAC_DPMS_CTRL_PENDING | 0x007f0000,
>                 NV50_PDISPLAY_DAC_DPMS_CTRL_PENDING | 0x00150000);
> -       nv_wait(priv, dpms_ctrl, NV50_PDISPLAY_DAC_DPMS_CTRL_PENDING, 0);
> +       nv50_wait_dpms_ctrl(priv, dpms_ctrl);
>
>         nv_wr32(priv, load_ctrl, 0x00100000 | loadval);
>         mdelay(9);
> @@ -95,7 +101,7 @@ nv50_dac_sense(NV50_DISP_MTHD_V1)
>
>         nv_mask(priv, dpms_ctrl, NV50_PDISPLAY_DAC_DPMS_CTRL_PENDING | 0x007f0000,
>                 NV50_PDISPLAY_DAC_DPMS_CTRL_PENDING | 0x00550000);
> -       nv_wait(priv, dpms_ctrl, NV50_PDISPLAY_DAC_DPMS_CTRL_PENDING, 0);
> +       nv50_wait_dpms_ctrl(priv, dpms_ctrl);
>
>         nv_debug(priv, "DAC%d sense: 0x%08x\n", outp->or, loadval);
>         if (!(loadval & 0x80000000))
> --
> 1.9.1
>
> _______________________________________________
> Nouveau mailing list
> Nouveau-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> http://lists.freedesktop.org/mailman/listinfo/nouveau