drm/i915: hdmi/dp audio rework

Const is good.

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/intel_audio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/gpu/drm/i915/intel_audio.c +1 -2

index 167903b5e3ff..e761f2c8d1ae 100644
@@ -28,7 +28,7 @@
 #include "intel_drv.h"
 #include "i915_drv.h"
 
-static struct {
+static const struct {
        int clock;
        u32 config;
 } hdmi_audio_clock[] = {
--

2.1.1

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Most importantly, "i" need not be the universal variable used for everything. No functional changes.

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/intel_audio.c | 115 ++++++++++++++++++-------------------
 1 file changed, 57 insertions(+), 58 deletions(-)

drivers/gpu/drm/i915/intel_audio.c +57 -59

index e761f2c8d1ae..00e9bfcd1e8d 100644
@@ -73,20 +73,21 @@ static bool intel_eld_uptodate(struct drm_connector *connector,
 {
        struct drm_i915_private *dev_priv = connector->dev->dev_private;
        uint8_t *eld = connector->eld;
-       uint32_t i;
+       uint32_t tmp;
+       int i;
 
-       i = I915_READ(reg_eldv);
-       i &= bits_eldv;
+       tmp = I915_READ(reg_eldv);
+       tmp &= bits_eldv;
 
        if (!eld[0])
-               return !i;
+               return !tmp;
 
-       if (!i)
+       if (!tmp)
                return false;
 
-       i = I915_READ(reg_elda);
-       i &= ~bits_elda;
-       I915_WRITE(reg_elda, i);
+       tmp = I915_READ(reg_elda);
+       tmp &= ~bits_elda;
+       I915_WRITE(reg_elda, tmp);
 
        for (i = 0; i < eld[2]; i++)
                if (I915_READ(reg_edid) != *((uint32_t *)eld + i))

@@ -102,12 +103,11 @@ static void g4x_write_eld(struct drm_connector *connector,
        struct drm_i915_private *dev_priv = connector->dev->dev_private;
        uint8_t *eld = connector->eld;
        uint32_t eldv;
-       uint32_t len;
-       uint32_t i;
-
-       i = I915_READ(G4X_AUD_VID_DID);
+       uint32_t tmp;
+       int len, i;
 
-       if (i == INTEL_AUDIO_DEVBLC || i == INTEL_AUDIO_DEVCL)
+       tmp = I915_READ(G4X_AUD_VID_DID);
+       if (tmp == INTEL_AUDIO_DEVBLC || tmp == INTEL_AUDIO_DEVCL)
                eldv = G4X_ELDV_DEVCL_DEVBLC;
        else
                eldv = G4X_ELDV_DEVCTG;

@@ -118,22 +118,22 @@ static void g4x_write_eld(struct drm_connector *connector,
                               G4X_HDMIW_HDMIEDID))
                return;
 
-       i = I915_READ(G4X_AUD_CNTL_ST);
-       i &= ~(eldv | G4X_ELD_ADDR);
-       len = (i >> 9) & 0x1f;          /* ELD buffer size */
-       I915_WRITE(G4X_AUD_CNTL_ST, i);
+       tmp = I915_READ(G4X_AUD_CNTL_ST);
+       tmp &= ~(eldv | G4X_ELD_ADDR);
+       len = (tmp >> 9) & 0x1f;                /* ELD buffer size */
+       I915_WRITE(G4X_AUD_CNTL_ST, tmp);
 
        if (!eld[0])
                return;
 
-       len = min_t(uint8_t, eld[2], len);
+       len = min_t(int, eld[2], len);
        DRM_DEBUG_DRIVER("ELD size %d\n", len);
        for (i = 0; i < len; i++)
                I915_WRITE(G4X_HDMIW_HDMIEDID, *((uint32_t *)eld + i));
 
-       i = I915_READ(G4X_AUD_CNTL_ST);
-       i |= eldv;
-       I915_WRITE(G4X_AUD_CNTL_ST, i);
+       tmp = I915_READ(G4X_AUD_CNTL_ST);
+       tmp |= eldv;
+       I915_WRITE(G4X_AUD_CNTL_ST, tmp);
 }
 
 static void haswell_write_eld(struct drm_connector *connector,

@@ -144,11 +144,10 @@ static void haswell_write_eld(struct drm_connector *connector,
        struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
        uint8_t *eld = connector->eld;
        uint32_t eldv;
-       uint32_t i;
-       int len;
-       int pipe = to_intel_crtc(crtc)->pipe;
-       int tmp;
-
+       uint32_t tmp;
+       int len, i;
+       enum pipe pipe = to_intel_crtc(crtc)->pipe;
+       enum port port;
        int hdmiw_hdmiedid = HSW_AUD_EDID_DATA(pipe);
        int aud_cntl_st = HSW_AUD_DIP_ELD_CTRL(pipe);
        int aud_config = HSW_AUD_CFG(pipe);

@@ -196,28 +195,27 @@ static void haswell_write_eld(struct drm_connector *connector,
                               hdmiw_hdmiedid))
                return;
 
-       i = I915_READ(aud_cntrl_st2);
-       i &= ~eldv;
-       I915_WRITE(aud_cntrl_st2, i);
+       tmp = I915_READ(aud_cntrl_st2);
+       tmp &= ~eldv;
+       I915_WRITE(aud_cntrl_st2, tmp);
 
        if (!eld[0])
                return;
 
-       i = I915_READ(aud_cntl_st);
-       i &= ~IBX_ELD_ADDRESS;
-       I915_WRITE(aud_cntl_st, i);
-       i = (i >> 29) & DIP_PORT_SEL_MASK;              /* DIP_Port_Select, 0x1 = PortB */
-       DRM_DEBUG_DRIVER("port num:%d\n", i);
+       tmp = I915_READ(aud_cntl_st);
+       tmp &= ~IBX_ELD_ADDRESS;
+       I915_WRITE(aud_cntl_st, tmp);
+       port = (tmp >> 29) & DIP_PORT_SEL_MASK;         /* DIP_Port_Select, 0x1 = PortB */
+       DRM_DEBUG_DRIVER("port num:%d\n", port);
 
-       len = min_t(uint8_t, eld[2], 21);       /* 84 bytes of hw ELD buffer */
+       len = min_t(int, eld[2], 21);   /* 84 bytes of hw ELD buffer */
        DRM_DEBUG_DRIVER("ELD size %d\n", len);
        for (i = 0; i < len; i++)
                I915_WRITE(hdmiw_hdmiedid, *((uint32_t *)eld + i));
 
-       i = I915_READ(aud_cntrl_st2);
-       i |= eldv;
-       I915_WRITE(aud_cntrl_st2, i);
-
+       tmp = I915_READ(aud_cntrl_st2);
+       tmp |= eldv;
+       I915_WRITE(aud_cntrl_st2, tmp);
 }
 
 static void ironlake_write_eld(struct drm_connector *connector,

@@ -228,13 +226,14 @@ static void ironlake_write_eld(struct drm_connector *connector,
        struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
        uint8_t *eld = connector->eld;
        uint32_t eldv;
-       uint32_t i;
-       int len;
+       uint32_t tmp;
+       int len, i;
        int hdmiw_hdmiedid;
        int aud_config;
        int aud_cntl_st;
        int aud_cntrl_st2;
-       int pipe = to_intel_crtc(crtc)->pipe;
+       enum pipe pipe = to_intel_crtc(crtc)->pipe;
+       enum port port;
 
        if (HAS_PCH_IBX(connector->dev)) {
                hdmiw_hdmiedid = IBX_HDMIW_HDMIEDID(pipe);

@@ -261,22 +260,22 @@ static void ironlake_write_eld(struct drm_connector *connector,
 
                intel_encoder = intel_attached_encoder(connector);
                intel_dig_port = enc_to_dig_port(&intel_encoder->base);
-               i = intel_dig_port->port;
+               port = intel_dig_port->port;
        } else {
-               i = I915_READ(aud_cntl_st);
-               i = (i >> 29) & DIP_PORT_SEL_MASK;
+               tmp = I915_READ(aud_cntl_st);
+               port = (tmp >> 29) & DIP_PORT_SEL_MASK;
                /* DIP_Port_Select, 0x1 = PortB */
        }
 
-       if (!i) {
+       if (!port) {
                DRM_DEBUG_DRIVER("Audio directed to unknown port\n");
                /* operate blindly on all ports */
                eldv = IBX_ELD_VALIDB;
                eldv |= IBX_ELD_VALIDB << 4;
                eldv |= IBX_ELD_VALIDB << 8;
        } else {
-               DRM_DEBUG_DRIVER("ELD on port %c\n", port_name(i));
-               eldv = IBX_ELD_VALIDB << ((i - 1) * 4);
+               DRM_DEBUG_DRIVER("ELD on port %c\n", port_name(port));
+               eldv = IBX_ELD_VALIDB << ((port - 1) * 4);
        }
 
        if (intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DISPLAYPORT)) {

@@ -293,25 +292,25 @@ static void ironlake_write_eld(struct drm_connector *connector,
                               hdmiw_hdmiedid))
                return;
 
-       i = I915_READ(aud_cntrl_st2);
-       i &= ~eldv;
-       I915_WRITE(aud_cntrl_st2, i);
+       tmp = I915_READ(aud_cntrl_st2);
+       tmp &= ~eldv;
+       I915_WRITE(aud_cntrl_st2, tmp);
 
        if (!eld[0])
                return;
 
-       i = I915_READ(aud_cntl_st);
-       i &= ~IBX_ELD_ADDRESS;
-       I915_WRITE(aud_cntl_st, i);
+       tmp = I915_READ(aud_cntl_st);
+       tmp &= ~IBX_ELD_ADDRESS;
+       I915_WRITE(aud_cntl_st, tmp);
 
-       len = min_t(uint8_t, eld[2], 21);       /* 84 bytes of hw ELD buffer */
+       len = min_t(int, eld[2], 21);   /* 84 bytes of hw ELD buffer */
        DRM_DEBUG_DRIVER("ELD size %d\n", len);
        for (i = 0; i < len; i++)
                I915_WRITE(hdmiw_hdmiedid, *((uint32_t *)eld + i));
 
-       i = I915_READ(aud_cntrl_st2);
-       i |= eldv;
-       I915_WRITE(aud_cntrl_st2, i);
+       tmp = I915_READ(aud_cntrl_st2);
+       tmp |= eldv;
+       I915_WRITE(aud_cntrl_st2, tmp);
 }
 
 void intel_write_eld(struct drm_encoder *encoder,
--

2.1.1

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Everything else can be derived from that. No functional changes.

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/intel_audio.c | 9 +++++----
 drivers/gpu/drm/i915/intel_ddi.c   | 2 +-
 drivers/gpu/drm/i915/intel_dp.c    | 2 +-
 drivers/gpu/drm/i915/intel_drv.h   | 3 +--
 drivers/gpu/drm/i915/intel_hdmi.c  | 2 +-
 5 files changed, 9 insertions(+), 9 deletions(-)

drivers/gpu/drm/i915/intel_audio.c +5 -4

index 00e9bfcd1e8d..829afd5305d1 100644
@@ -313,10 +313,11 @@ static void ironlake_write_eld(struct drm_connector *connector,
        I915_WRITE(aud_cntrl_st2, tmp);
 }
 
-void intel_write_eld(struct drm_encoder *encoder,
-                    struct drm_display_mode *mode)
+void intel_write_eld(struct intel_encoder *intel_encoder)
 {
-       struct drm_crtc *crtc = encoder->crtc;
+       struct drm_encoder *encoder = &intel_encoder->base;
+       struct intel_crtc *crtc = to_intel_crtc(encoder->crtc);
+       struct drm_display_mode *mode = &crtc->config.adjusted_mode;
        struct drm_connector *connector;
        struct drm_device *dev = encoder->dev;
        struct drm_i915_private *dev_priv = dev->dev_private;

@@ -334,7 +335,7 @@ void intel_write_eld(struct drm_encoder *encoder,
        connector->eld[6] = drm_av_sync_delay(connector, mode) / 2;
 
        if (dev_priv->display.write_eld)
-               dev_priv->display.write_eld(connector, crtc, mode);
+               dev_priv->display.write_eld(connector, encoder->crtc, mode);
 }
 
 /**

drivers/gpu/drm/i915/intel_ddi.c +1 -1

index cb5367c6f95a..2688bc940879 100644
@@ -1126,7 +1126,7 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)
 
                /* write eld */
                DRM_DEBUG_DRIVER("DDI audio: write eld information\n");
-               intel_write_eld(encoder, &crtc->config.adjusted_mode);
+               intel_write_eld(intel_encoder);
        }
 
        if (type == INTEL_OUTPUT_EDP) {

drivers/gpu/drm/i915/intel_dp.c +1 -1

index 4455009fb471..eafe9f598a03 100644
@@ -1250,7 +1250,7 @@ static void intel_dp_prepare(struct intel_encoder *encoder)
                DRM_DEBUG_DRIVER("Enabling DP audio on pipe %c\n",
                                 pipe_name(crtc->pipe));
                intel_dp->DP |= DP_AUDIO_OUTPUT_ENABLE;
-               intel_write_eld(&encoder->base, adjusted_mode);
+               intel_write_eld(encoder);
        }
 
        /* Split out the IBX/CPU vs CPT settings */

drivers/gpu/drm/i915/intel_drv.h +1 -2

index 3bbc4fe817ff..bf72a9201a15 100644
@@ -850,8 +850,7 @@ void intel_fb_obj_flush(struct drm_i915_gem_object *obj, bool retire);
 
 /* intel_audio.c */
 void intel_init_audio(struct drm_device *dev);
-void intel_write_eld(struct drm_encoder *encoder,
-                    struct drm_display_mode *mode);
+void intel_write_eld(struct intel_encoder *encoder);
 
 /* intel_display.c */
 const char *intel_output_name(int output);

drivers/gpu/drm/i915/intel_hdmi.c +1 -2

index 8b5f3aa027f3..07b5ebd65d41 100644
@@ -666,7 +666,7 @@ static void intel_hdmi_prepare(struct intel_encoder *encoder)
                DRM_DEBUG_DRIVER("Enabling HDMI audio on pipe %c\n",
                                 pipe_name(crtc->pipe));
                hdmi_val |= SDVO_AUDIO_ENABLE;
-               intel_write_eld(&encoder->base, adjusted_mode);
+               intel_write_eld(encoder);
        }
 
        if (HAS_PCH_CPT(dev))
--

2.1.1

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

This will simplify things later on. No functional changes.

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/i915_drv.h    |  4 ++--
 drivers/gpu/drm/i915/intel_audio.c | 22 ++++++++++------------
 2 files changed, 12 insertions(+), 14 deletions(-)

drivers/gpu/drm/i915/i915_drv.h +2 -2

index 627b7e71f168..6a73803482cb 100644
@@ -434,6 +434,7 @@ struct drm_i915_error_state {
 };
 
 struct intel_connector;
+struct intel_encoder;
 struct intel_crtc_config;
 struct intel_plane_config;
 struct intel_crtc;

@@ -483,7 +484,7 @@ struct drm_i915_display_funcs {
        void (*crtc_disable)(struct drm_crtc *crtc);
        void (*off)(struct drm_crtc *crtc);
        void (*write_eld)(struct drm_connector *connector,
-                         struct drm_crtc *crtc,
+                         struct intel_encoder *encoder,
                          struct drm_display_mode *mode);
        void (*fdi_link_train)(struct drm_crtc *crtc);
        void (*init_clock_gating)(struct drm_device *dev);

@@ -2798,7 +2799,6 @@ static inline bool intel_gmbus_is_forced_bit(struct i2c_adapter *adapter)
 extern void intel_i2c_reset(struct drm_device *dev);
 
 /* intel_opregion.c */
-struct intel_encoder;
 #ifdef CONFIG_ACPI
 extern int intel_opregion_setup(struct drm_device *dev);
 extern void intel_opregion_init(struct drm_device *dev);

drivers/gpu/drm/i915/intel_audio.c +4 -4

index 829afd5305d1..4a384d780b20 100644
@@ -97,7 +97,7 @@ static bool intel_eld_uptodate(struct drm_connector *connector,
 }
 
 static void g4x_write_eld(struct drm_connector *connector,
-                         struct drm_crtc *crtc,
+                         struct intel_encoder *encoder,
                          struct drm_display_mode *mode)
 {
        struct drm_i915_private *dev_priv = connector->dev->dev_private;

@@ -137,16 +137,16 @@ static void g4x_write_eld(struct drm_connector *connector,
 }
 
 static void haswell_write_eld(struct drm_connector *connector,
-                             struct drm_crtc *crtc,
+                             struct intel_encoder *encoder,
                              struct drm_display_mode *mode)
 {
        struct drm_i915_private *dev_priv = connector->dev->dev_private;
-       struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+       struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
        uint8_t *eld = connector->eld;
        uint32_t eldv;
        uint32_t tmp;
        int len, i;
-       enum pipe pipe = to_intel_crtc(crtc)->pipe;
+       enum pipe pipe = intel_crtc->pipe;
        enum port port;
        int hdmiw_hdmiedid = HSW_AUD_EDID_DATA(pipe);
        int aud_cntl_st = HSW_AUD_DIP_ELD_CTRL(pipe);

drivers/gpu/drm/i915/intel_audio.c +5 -8

        /* Set ELD valid state */
        tmp = I915_READ(aud_cntrl_st2);

@@ -219,11 +219,11 @@ static void haswell_write_eld(struct drm_connector *connector,
 }
 
 static void ironlake_write_eld(struct drm_connector *connector,
-                              struct drm_crtc *crtc,
+                              struct intel_encoder *encoder,
                               struct drm_display_mode *mode)
 {
        struct drm_i915_private *dev_priv = connector->dev->dev_private;
-       struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+       struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
        uint8_t *eld = connector->eld;
        uint32_t eldv;
        uint32_t tmp;

@@ -232,7 +232,7 @@ static void ironlake_write_eld(struct drm_connector *connector,
        int aud_config;
        int aud_cntl_st;
        int aud_cntrl_st2;
-       enum pipe pipe = to_intel_crtc(crtc)->pipe;
+       enum pipe pipe = intel_crtc->pipe;
        enum port port;
 
        if (HAS_PCH_IBX(connector->dev)) {

@@ -255,11 +255,9 @@ static void ironlake_write_eld(struct drm_connector *connector,
        DRM_DEBUG_DRIVER("ELD on pipe %c\n", pipe_name(pipe));
 
        if (IS_VALLEYVIEW(connector->dev))  {
-               struct intel_encoder *intel_encoder;
                struct intel_digital_port *intel_dig_port;
 
-               intel_encoder = intel_attached_encoder(connector);
-               intel_dig_port = enc_to_dig_port(&intel_encoder->base);
+               intel_dig_port = enc_to_dig_port(&encoder->base);
                port = intel_dig_port->port;
        } else {
                tmp = I915_READ(aud_cntl_st);

@@ -335,7 +333,7 @@ void intel_write_eld(struct intel_encoder *intel_encoder)
        connector->eld[6] = drm_av_sync_delay(connector, mode) / 2;
 
        if (dev_priv->display.write_eld)
-               dev_priv->display.write_eld(connector, encoder->crtc, mode);
+               dev_priv->display.write_eld(connector, intel_encoder, mode);
 }
 
 /**
--

2.1.1

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 74 lines · Jani Nikula

This will simplify things later on. No functional changes.

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/i915_drv.h    |  4 ++--
 drivers/gpu/drm/i915/intel_audio.c | 22 ++++++++++------------
 2 files changed, 12 insertions(+), 14 deletions(-)

drivers/gpu/drm/i915/i915_drv.h +2 -2

index 627b7e71f168..6a73803482cb 100644
@@ -434,6 +434,7 @@ struct drm_i915_error_state {
 };
 
 struct intel_connector;
+struct intel_encoder;
 struct intel_crtc_config;
 struct intel_plane_config;
 struct intel_crtc;

@@ -483,7 +484,7 @@ struct drm_i915_display_funcs {
        void (*crtc_disable)(struct drm_crtc *crtc);
        void (*off)(struct drm_crtc *crtc);
        void (*write_eld)(struct drm_connector *connector,
-                         struct drm_crtc *crtc,
+                         struct intel_encoder *encoder,
                          struct drm_display_mode *mode);
        void (*fdi_link_train)(struct drm_crtc *crtc);
        void (*init_clock_gating)(struct drm_device *dev);

@@ -2798,7 +2799,6 @@ static inline bool intel_gmbus_is_forced_bit(struct i2c_adapter *adapter)
 extern void intel_i2c_reset(struct drm_device *dev);
 
 /* intel_opregion.c */
-struct intel_encoder;
 #ifdef CONFIG_ACPI
 extern int intel_opregion_setup(struct drm_device *dev);
 extern void intel_opregion_init(struct drm_device *dev);

drivers/gpu/drm/i915/intel_audio.c +4 -4

index 829afd5305d1..4a384d780b20 100644
@@ -97,7 +97,7 @@ static bool intel_eld_uptodate(struct drm_connector *connector,
 }
 
 static void g4x_write_eld(struct drm_connector *connector,
-                         struct drm_crtc *crtc,
+                         struct intel_encoder *encoder,
                          struct drm_display_mode *mode)
 {
        struct drm_i915_private *dev_priv = connector->dev->dev_private;

@@ -137,16 +137,16 @@ static void g4x_write_eld(struct drm_connector *connector,
 }
 
 static void haswell_write_eld(struct drm_connector *connector,
-                             struct drm_crtc *crtc,
+                             struct intel_encoder *encoder,
                              struct drm_display_mode *mode)
 {
        struct drm_i915_private *dev_priv = connector->dev->dev_private;
-       struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+       struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
        uint8_t *eld = connector->eld;
        uint32_t eldv;
        uint32_t tmp;
        int len, i;
-       enum pipe pipe = to_intel_crtc(crtc)->pipe;
+       enum pipe pipe = intel_crtc->pipe;
        enum port port;
        int hdmiw_hdmiedid = HSW_AUD_EDID_DATA(pipe);
        int aud_cntl_st = HSW_AUD_DIP_ELD_CTRL(pipe);

@@ -160,7 +160,7 @@ static void haswell_write_eld(struct drm_connector *connector,
        I915_WRITE(aud_cntrl_st2, tmp);
        POSTING_READ(aud_cntrl_st2);
 

drivers/gpu/drm/i915/intel_audio.c static void haswell_write_eld(struct drm_connector *connector, · line 163

Quoted 2 lines

drivers/gpu/drm/i915/intel_audio.c +1 -1

-       assert_pipe_disabled(dev_priv, to_intel_crtc(crtc)->pipe);
+       assert_pipe_disabled(dev_priv, pipe);

maybe this line could be on previous patch, but nevermind

Reviewed-by Rodrigo Vivi <[email protected]>

Quoted 55 lines

drivers/gpu/drm/i915/intel_audio.c +5 -8

        /* Set ELD valid state */
        tmp = I915_READ(aud_cntrl_st2);

@@ -219,11 +219,11 @@ static void haswell_write_eld(struct drm_connector *connector,
 }
 
 static void ironlake_write_eld(struct drm_connector *connector,
-                              struct drm_crtc *crtc,
+                              struct intel_encoder *encoder,
                               struct drm_display_mode *mode)
 {
        struct drm_i915_private *dev_priv = connector->dev->dev_private;
-       struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+       struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
        uint8_t *eld = connector->eld;
        uint32_t eldv;
        uint32_t tmp;

@@ -232,7 +232,7 @@ static void ironlake_write_eld(struct drm_connector *connector,
        int aud_config;
        int aud_cntl_st;
        int aud_cntrl_st2;
-       enum pipe pipe = to_intel_crtc(crtc)->pipe;
+       enum pipe pipe = intel_crtc->pipe;
        enum port port;
 
        if (HAS_PCH_IBX(connector->dev)) {

@@ -255,11 +255,9 @@ static void ironlake_write_eld(struct drm_connector *connector,
        DRM_DEBUG_DRIVER("ELD on pipe %c\n", pipe_name(pipe));
 
        if (IS_VALLEYVIEW(connector->dev))  {
-               struct intel_encoder *intel_encoder;
                struct intel_digital_port *intel_dig_port;
 
-               intel_encoder = intel_attached_encoder(connector);
-               intel_dig_port = enc_to_dig_port(&intel_encoder->base);
+               intel_dig_port = enc_to_dig_port(&encoder->base);
                port = intel_dig_port->port;
        } else {
                tmp = I915_READ(aud_cntl_st);

@@ -335,7 +333,7 @@ void intel_write_eld(struct intel_encoder *intel_encoder)
        connector->eld[6] = drm_av_sync_delay(connector, mode) / 2;
 
        if (dev_priv->display.write_eld)
-               dev_priv->display.write_eld(connector, encoder->crtc, mode);
+               dev_priv->display.write_eld(connector, intel_encoder, mode);
 }
 
 /**
--

2.1.1

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Signature · 6 lines

Rodrigo Vivi
Blog: http://blog.vivi.eng.br
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 75 lines · Rodrigo Vivi

Quoted 74 lines · Jani Nikula

This will simplify things later on. No functional changes.

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/i915_drv.h    |  4 ++--
 drivers/gpu/drm/i915/intel_audio.c | 22 ++++++++++------------
 2 files changed, 12 insertions(+), 14 deletions(-)

drivers/gpu/drm/i915/i915_drv.h +2 -2

index 627b7e71f168..6a73803482cb 100644
@@ -434,6 +434,7 @@ struct drm_i915_error_state {
 };
 
 struct intel_connector;
+struct intel_encoder;
 struct intel_crtc_config;
 struct intel_plane_config;
 struct intel_crtc;

@@ -483,7 +484,7 @@ struct drm_i915_display_funcs {
        void (*crtc_disable)(struct drm_crtc *crtc);
        void (*off)(struct drm_crtc *crtc);
        void (*write_eld)(struct drm_connector *connector,
-                         struct drm_crtc *crtc,
+                         struct intel_encoder *encoder,
                          struct drm_display_mode *mode);
        void (*fdi_link_train)(struct drm_crtc *crtc);
        void (*init_clock_gating)(struct drm_device *dev);

@@ -2798,7 +2799,6 @@ static inline bool intel_gmbus_is_forced_bit(struct i2c_adapter *adapter)
 extern void intel_i2c_reset(struct drm_device *dev);
 
 /* intel_opregion.c */
-struct intel_encoder;
 #ifdef CONFIG_ACPI
 extern int intel_opregion_setup(struct drm_device *dev);
 extern void intel_opregion_init(struct drm_device *dev);

drivers/gpu/drm/i915/intel_audio.c +4 -4

index 829afd5305d1..4a384d780b20 100644
@@ -97,7 +97,7 @@ static bool intel_eld_uptodate(struct drm_connector *connector,
 }
 
 static void g4x_write_eld(struct drm_connector *connector,
-                         struct drm_crtc *crtc,
+                         struct intel_encoder *encoder,
                          struct drm_display_mode *mode)
 {
        struct drm_i915_private *dev_priv = connector->dev->dev_private;

@@ -137,16 +137,16 @@ static void g4x_write_eld(struct drm_connector *connector,
 }
 
 static void haswell_write_eld(struct drm_connector *connector,
-                             struct drm_crtc *crtc,
+                             struct intel_encoder *encoder,
                              struct drm_display_mode *mode)
 {
        struct drm_i915_private *dev_priv = connector->dev->dev_private;
-       struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+       struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
        uint8_t *eld = connector->eld;
        uint32_t eldv;
        uint32_t tmp;
        int len, i;
-       enum pipe pipe = to_intel_crtc(crtc)->pipe;
+       enum pipe pipe = intel_crtc->pipe;
        enum port port;
        int hdmiw_hdmiedid = HSW_AUD_EDID_DATA(pipe);
        int aud_cntl_st = HSW_AUD_DIP_ELD_CTRL(pipe);

@@ -160,7 +160,7 @@ static void haswell_write_eld(struct drm_connector *connector,
        I915_WRITE(aud_cntrl_st2, tmp);
        POSTING_READ(aud_cntrl_st2);
 

Quoted 5 lines

drivers/gpu/drm/i915/intel_audio.c static void haswell_write_eld(struct drm_connector *connector, · line 163

Quoted 2 lines

drivers/gpu/drm/i915/intel_audio.c +1 -1

-       assert_pipe_disabled(dev_priv, to_intel_crtc(crtc)->pipe);
+       assert_pipe_disabled(dev_priv, pipe);

maybe this line could be on previous patch, but nevermind

Reviewed-by Rodrigo Vivi <[email protected]>

Merged up to this patch. Later patches with r-b don't apply any more due to some in-between that are still under discussion. So I've stopped here.

Thanks, Daniel

Quoted 51 lines

Quoted 51 lines

drivers/gpu/drm/i915/intel_audio.c +5 -8

        /* Set ELD valid state */
        tmp = I915_READ(aud_cntrl_st2);

@@ -219,11 +219,11 @@ static void haswell_write_eld(struct drm_connector *connector,
 }
 
 static void ironlake_write_eld(struct drm_connector *connector,
-                              struct drm_crtc *crtc,
+                              struct intel_encoder *encoder,
                               struct drm_display_mode *mode)
 {
        struct drm_i915_private *dev_priv = connector->dev->dev_private;
-       struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+       struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
        uint8_t *eld = connector->eld;
        uint32_t eldv;
        uint32_t tmp;

@@ -232,7 +232,7 @@ static void ironlake_write_eld(struct drm_connector *connector,
        int aud_config;
        int aud_cntl_st;
        int aud_cntrl_st2;
-       enum pipe pipe = to_intel_crtc(crtc)->pipe;
+       enum pipe pipe = intel_crtc->pipe;
        enum port port;
 
        if (HAS_PCH_IBX(connector->dev)) {

@@ -255,11 +255,9 @@ static void ironlake_write_eld(struct drm_connector *connector,
        DRM_DEBUG_DRIVER("ELD on pipe %c\n", pipe_name(pipe));
 
        if (IS_VALLEYVIEW(connector->dev))  {
-               struct intel_encoder *intel_encoder;
                struct intel_digital_port *intel_dig_port;
 
-               intel_encoder = intel_attached_encoder(connector);
-               intel_dig_port = enc_to_dig_port(&intel_encoder->base);
+               intel_dig_port = enc_to_dig_port(&encoder->base);
                port = intel_dig_port->port;
        } else {
                tmp = I915_READ(aud_cntl_st);

@@ -335,7 +333,7 @@ void intel_write_eld(struct intel_encoder *intel_encoder)
        connector->eld[6] = drm_av_sync_delay(connector, mode) / 2;
 
        if (dev_priv->display.write_eld)
-               dev_priv->display.write_eld(connector, encoder->crtc, mode);
+               dev_priv->display.write_eld(connector, intel_encoder, mode);
 }
 
 /**
--

2.1.1

Quoted 14 lines

Quoted 4 lines

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

--
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Oh, I was going to review the rest... but based on this comment I guess I might wait for a new v2 series right?

Quoted 156 lines · Rodrigo Vivi

Quoted 75 lines · Jani Nikula

Quoted 74 lines

This will simplify things later on. No functional changes.

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/i915_drv.h    |  4 ++--
 drivers/gpu/drm/i915/intel_audio.c | 22 ++++++++++------------
 2 files changed, 12 insertions(+), 14 deletions(-)

drivers/gpu/drm/i915/i915_drv.h +2 -2

index 627b7e71f168..6a73803482cb 100644
@@ -434,6 +434,7 @@ struct drm_i915_error_state {
 };
 
 struct intel_connector;
+struct intel_encoder;
 struct intel_crtc_config;
 struct intel_plane_config;
 struct intel_crtc;

@@ -483,7 +484,7 @@ struct drm_i915_display_funcs {
        void (*crtc_disable)(struct drm_crtc *crtc);
        void (*off)(struct drm_crtc *crtc);
        void (*write_eld)(struct drm_connector *connector,
-                         struct drm_crtc *crtc,
+                         struct intel_encoder *encoder,
                          struct drm_display_mode *mode);
        void (*fdi_link_train)(struct drm_crtc *crtc);
        void (*init_clock_gating)(struct drm_device *dev);

@@ -2798,7 +2799,6 @@ static inline bool intel_gmbus_is_forced_bit(struct i2c_adapter *adapter)
 extern void intel_i2c_reset(struct drm_device *dev);
 
 /* intel_opregion.c */
-struct intel_encoder;
 #ifdef CONFIG_ACPI
 extern int intel_opregion_setup(struct drm_device *dev);
 extern void intel_opregion_init(struct drm_device *dev);

drivers/gpu/drm/i915/intel_audio.c +4 -4

index 829afd5305d1..4a384d780b20 100644
@@ -97,7 +97,7 @@ static bool intel_eld_uptodate(struct drm_connector *connector,
 }
 
 static void g4x_write_eld(struct drm_connector *connector,
-                         struct drm_crtc *crtc,
+                         struct intel_encoder *encoder,
                          struct drm_display_mode *mode)
 {
        struct drm_i915_private *dev_priv = connector->dev->dev_private;

@@ -137,16 +137,16 @@ static void g4x_write_eld(struct drm_connector *connector,
 }
 
 static void haswell_write_eld(struct drm_connector *connector,
-                             struct drm_crtc *crtc,
+                             struct intel_encoder *encoder,
                              struct drm_display_mode *mode)
 {
        struct drm_i915_private *dev_priv = connector->dev->dev_private;
-       struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+       struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
        uint8_t *eld = connector->eld;
        uint32_t eldv;
        uint32_t tmp;
        int len, i;
-       enum pipe pipe = to_intel_crtc(crtc)->pipe;
+       enum pipe pipe = intel_crtc->pipe;
        enum port port;
        int hdmiw_hdmiedid = HSW_AUD_EDID_DATA(pipe);
        int aud_cntl_st = HSW_AUD_DIP_ELD_CTRL(pipe);

@@ -160,7 +160,7 @@ static void haswell_write_eld(struct drm_connector *connector,
        I915_WRITE(aud_cntrl_st2, tmp);
        POSTING_READ(aud_cntrl_st2);
 

Quoted 5 lines

drivers/gpu/drm/i915/intel_audio.c static void haswell_write_eld(struct drm_connector *connector, · line 163

Quoted 2 lines

drivers/gpu/drm/i915/intel_audio.c +1 -1

-       assert_pipe_disabled(dev_priv, to_intel_crtc(crtc)->pipe);
+       assert_pipe_disabled(dev_priv, pipe);

maybe this line could be on previous patch, but nevermind

Reviewed-by Rodrigo Vivi <[email protected]>

Merged up to this patch. Later patches with r-b don't apply any more due to some in-between that are still under discussion. So I've stopped here.

Thanks, Daniel

Quoted 51 lines

Quoted 51 lines

drivers/gpu/drm/i915/intel_audio.c +5 -8

        /* Set ELD valid state */
        tmp = I915_READ(aud_cntrl_st2);

@@ -219,11 +219,11 @@ static void haswell_write_eld(struct drm_connector *connector,
 }
 
 static void ironlake_write_eld(struct drm_connector *connector,
-                              struct drm_crtc *crtc,
+                              struct intel_encoder *encoder,
                               struct drm_display_mode *mode)
 {
        struct drm_i915_private *dev_priv = connector->dev->dev_private;
-       struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+       struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
        uint8_t *eld = connector->eld;
        uint32_t eldv;
        uint32_t tmp;

@@ -232,7 +232,7 @@ static void ironlake_write_eld(struct drm_connector *connector,
        int aud_config;
        int aud_cntl_st;
        int aud_cntrl_st2;
-       enum pipe pipe = to_intel_crtc(crtc)->pipe;
+       enum pipe pipe = intel_crtc->pipe;
        enum port port;
 
        if (HAS_PCH_IBX(connector->dev)) {

@@ -255,11 +255,9 @@ static void ironlake_write_eld(struct drm_connector *connector,
        DRM_DEBUG_DRIVER("ELD on pipe %c\n", pipe_name(pipe));
 
        if (IS_VALLEYVIEW(connector->dev))  {
-               struct intel_encoder *intel_encoder;
                struct intel_digital_port *intel_dig_port;
 
-               intel_encoder = intel_attached_encoder(connector);
-               intel_dig_port = enc_to_dig_port(&intel_encoder->base);
+               intel_dig_port = enc_to_dig_port(&encoder->base);
                port = intel_dig_port->port;
        } else {
                tmp = I915_READ(aud_cntl_st);

@@ -335,7 +333,7 @@ void intel_write_eld(struct intel_encoder *intel_encoder)
        connector->eld[6] = drm_av_sync_delay(connector, mode) / 2;
 
        if (dev_priv->display.write_eld)
-               dev_priv->display.write_eld(connector, encoder->crtc, mode);
+               dev_priv->display.write_eld(connector, intel_encoder, mode);
 }
 
 /**
--

2.1.1

Quoted 14 lines

Quoted 4 lines

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

--
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Signature · 2 lines

Rodrigo Vivi
Blog: http://blog.vivi.eng.br

Quoted 2 lines · Rodrigo Vivi

Oh, I was going to review the rest... but based on this comment I guess I might wait for a new v2 series right?

No, I think it was in reference to *your* comments!

Quoted 5 lines · Jani Nikula

Quoted 2 lines · Rodrigo Vivi

Oh, I was going to review the rest... but based on this comment I guess I might wait for a new v2 series right?

No, I think it was in reference to *your* comments!

Yeah that was just my explanations for why I haven't merged everything - later patches lacked r-bs.

-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Keep the driver modifications to ELD together. This also sets the Conn_Type for G4X DP which wasn't done before.

Clean up the debugs while at it; this is all obvious from the connector name.

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/intel_audio.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

drivers/gpu/drm/i915/intel_audio.c +4 -10

index 4a384d780b20..4d644efde608 100644
@@ -181,13 +181,10 @@ static void haswell_write_eld(struct drm_connector *connector,
 
        eldv = AUDIO_ELD_VALID_A << (pipe * 4);
 
-       if (intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DISPLAYPORT)) {
-               DRM_DEBUG_DRIVER("ELD: DisplayPort detected\n");
-               eld[5] |= (1 << 2);     /* Conn_Type, 0x1 = DisplayPort */
+       if (intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DISPLAYPORT))
                I915_WRITE(aud_config, AUD_CONFIG_N_VALUE_INDEX); /* 0x1 = DP */
-       } else {
+       else
                I915_WRITE(aud_config, audio_config_hdmi_pixel_clock(mode));
-       }
 
        if (intel_eld_uptodate(connector,
                               aud_cntrl_st2, eldv,

@@ -276,13 +273,10 @@ static void ironlake_write_eld(struct drm_connector *connector,
                eldv = IBX_ELD_VALIDB << ((port - 1) * 4);
        }
 
-       if (intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DISPLAYPORT)) {
-               DRM_DEBUG_DRIVER("ELD: DisplayPort detected\n");
-               eld[5] |= (1 << 2);     /* Conn_Type, 0x1 = DisplayPort */
+       if (intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DISPLAYPORT))
                I915_WRITE(aud_config, AUD_CONFIG_N_VALUE_INDEX); /* 0x1 = DP */
-       } else {
+       else
                I915_WRITE(aud_config, audio_config_hdmi_pixel_clock(mode));
-       }
 
        if (intel_eld_uptodate(connector,
                               aud_cntrl_st2, eldv,

drivers/gpu/drm/i915/intel_audio.c +3 -1

+       if (intel_pipe_has_type(crtc, INTEL_OUTPUT_DISPLAYPORT))
+               connector->eld[5] |= (1 << 2);
+
        connector->eld[6] = drm_av_sync_delay(connector, mode) / 2;
 
        if (dev_priv->display.write_eld)
--

2.1.1

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 51 lines · Jani Nikula

Keep the driver modifications to ELD together. This also sets the Conn_Type for G4X DP which wasn't done before.

Clean up the debugs while at it; this is all obvious from the connector name.

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/intel_audio.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

drivers/gpu/drm/i915/intel_audio.c +4 -10

index 4a384d780b20..4d644efde608 100644
@@ -181,13 +181,10 @@ static void haswell_write_eld(struct drm_connector *connector,
 
        eldv = AUDIO_ELD_VALID_A << (pipe * 4);
 
-       if (intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DISPLAYPORT)) {
-               DRM_DEBUG_DRIVER("ELD: DisplayPort detected\n");
-               eld[5] |= (1 << 2);     /* Conn_Type, 0x1 = DisplayPort */
+       if (intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DISPLAYPORT))
                I915_WRITE(aud_config, AUD_CONFIG_N_VALUE_INDEX); /* 0x1 = DP */
-       } else {
+       else
                I915_WRITE(aud_config, audio_config_hdmi_pixel_clock(mode));
-       }
 
        if (intel_eld_uptodate(connector,
                               aud_cntrl_st2, eldv,

@@ -276,13 +273,10 @@ static void ironlake_write_eld(struct drm_connector *connector,
                eldv = IBX_ELD_VALIDB << ((port - 1) * 4);
        }
 
-       if (intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DISPLAYPORT)) {
-               DRM_DEBUG_DRIVER("ELD: DisplayPort detected\n");
-               eld[5] |= (1 << 2);     /* Conn_Type, 0x1 = DisplayPort */
+       if (intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DISPLAYPORT))
                I915_WRITE(aud_config, AUD_CONFIG_N_VALUE_INDEX); /* 0x1 = DP */
-       } else {
+       else
                I915_WRITE(aud_config, audio_config_hdmi_pixel_clock(mode));
-       }
 
        if (intel_eld_uptodate(connector,
                               aud_cntrl_st2, eldv,

@@ -330,6 +324,11 @@ void intel_write_eld(struct intel_encoder *intel_encoder)
                         connector->encoder->base.id,
                         connector->encoder->name);
 

drivers/gpu/drm/i915/intel_audio.c void intel_write_eld(struct intel_encoder *intel_encoder) · line 328

Quoted 2 lines

drivers/gpu/drm/i915/intel_audio.c +2 -0

+       /* ELD Conn_Type */
+       connector->eld[5] &= (3 << 2);

Where this came from? Isn't a "~" missing here?

Quoted 14 lines

drivers/gpu/drm/i915/intel_audio.c +3 -1

+       if (intel_pipe_has_type(crtc, INTEL_OUTPUT_DISPLAYPORT))
+               connector->eld[5] |= (1 << 2);
+
        connector->eld[6] = drm_av_sync_delay(connector, mode) / 2;
 
        if (dev_priv->display.write_eld)
--

2.1.1

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Signature · 6 lines

Rodrigo Vivi
Blog: http://blog.vivi.eng.br
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Keep the driver modifications to ELD together. This also sets the Conn_Type for G4X DP which wasn't done before.

Clean up the debugs while at it; this is all obvious from the connector name.

v3: add missing ~ (Rodrigo)

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/intel_audio.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

drivers/gpu/drm/i915/intel_audio.c +9 -11

index 4a384d780b20..537f6d8927f1 100644
@@ -181,13 +181,10 @@ static void haswell_write_eld(struct drm_connector *connector,
 
        eldv = AUDIO_ELD_VALID_A << (pipe * 4);
 
-       if (intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DISPLAYPORT)) {
-               DRM_DEBUG_DRIVER("ELD: DisplayPort detected\n");
-               eld[5] |= (1 << 2);     /* Conn_Type, 0x1 = DisplayPort */
+       if (intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DISPLAYPORT))
                I915_WRITE(aud_config, AUD_CONFIG_N_VALUE_INDEX); /* 0x1 = DP */
-       } else {
+       else
                I915_WRITE(aud_config, audio_config_hdmi_pixel_clock(mode));
-       }
 
        if (intel_eld_uptodate(connector,
                               aud_cntrl_st2, eldv,

@@ -276,13 +273,10 @@ static void ironlake_write_eld(struct drm_connector *connector,
                eldv = IBX_ELD_VALIDB << ((port - 1) * 4);
        }
 
-       if (intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DISPLAYPORT)) {
-               DRM_DEBUG_DRIVER("ELD: DisplayPort detected\n");
-               eld[5] |= (1 << 2);     /* Conn_Type, 0x1 = DisplayPort */
+       if (intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DISPLAYPORT))
                I915_WRITE(aud_config, AUD_CONFIG_N_VALUE_INDEX); /* 0x1 = DP */
-       } else {
+       else
                I915_WRITE(aud_config, audio_config_hdmi_pixel_clock(mode));
-       }
 
        if (intel_eld_uptodate(connector,
                               aud_cntrl_st2, eldv,

@@ -330,6 +324,11 @@ void intel_write_eld(struct intel_encoder *intel_encoder)
                         connector->encoder->base.id,
                         connector->encoder->name);
 
+       /* ELD Conn_Type */
+       connector->eld[5] &= ~(3 << 2);
+       if (intel_pipe_has_type(crtc, INTEL_OUTPUT_DISPLAYPORT))
+               connector->eld[5] |= (1 << 2);
+
        connector->eld[6] = drm_av_sync_delay(connector, mode) / 2;
 
        if (dev_priv->display.write_eld)
--

2.1.1

The audio programming sequence states that the ELD must be written and enabled after the pipe is ready. Indeed, this should clarify the situation with

commit c79057922ed6c2c6df1214e6ab4414fea1b23db2

Author: Daniel Vetter <[email protected]>
Date:   Wed Apr 16 16:56:09 2014 +0200

drm/i915: Remove vblank wait from haswell_write_eld

and Ville's review of it [1].

Moreover, we should not touch the relevant registers before we get the audio power domain.

[1] http://mid.gmane.org/[email protected]

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/intel_audio.c |  2 --
 drivers/gpu/drm/i915/intel_ddi.c   | 11 ++---------
 2 files changed, 2 insertions(+), 11 deletions(-)

drivers/gpu/drm/i915/intel_audio.c +0 -2

index 4d644efde608..b9d3a143dd8c 100644
@@ -160,8 +160,6 @@ static void haswell_write_eld(struct drm_connector *connector,
        I915_WRITE(aud_cntrl_st2, tmp);
        POSTING_READ(aud_cntrl_st2);
 
-       assert_pipe_disabled(dev_priv, pipe);
-
        /* Set ELD valid state */
        tmp = I915_READ(aud_cntrl_st2);
        DRM_DEBUG_DRIVER("HDMI audio: pin eld vld status=0x%08x\n", tmp);

drivers/gpu/drm/i915/intel_ddi.c +2 -10

index 2688bc940879..56e7cb1ddc75 100644
@@ -1120,15 +1120,6 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)
        enum port port = intel_ddi_get_encoder_port(intel_encoder);
        int type = intel_encoder->type;
 
-       if (crtc->config.has_audio) {
-               DRM_DEBUG_DRIVER("Audio on pipe %c on DDI\n",
-                                pipe_name(crtc->pipe));
-
-               /* write eld */
-               DRM_DEBUG_DRIVER("DDI audio: write eld information\n");
-               intel_write_eld(intel_encoder);
-       }
-
        if (type == INTEL_OUTPUT_EDP) {
                struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
                intel_edp_panel_on(intel_dp);

@@ -1225,6 +1216,8 @@ static void intel_enable_ddi(struct intel_encoder *intel_encoder)
 
        if (intel_crtc->config.has_audio) {
                intel_display_power_get(dev_priv, POWER_DOMAIN_AUDIO);
+               intel_write_eld(intel_encoder);
+
                tmp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD);
                tmp |= ((AUDIO_OUTPUT_ENABLE_A | AUDIO_ELD_VALID_A) << (pipe * 4));
                I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, tmp);
--

2.1.1

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 3 lines · Jani Nikula

The audio programming sequence states that the ELD must be written and enabled after the pipe is ready. Indeed, this should clarify the situation with

Where/what doc can I confirm this?

Quoted 70 lines

commit c79057922ed6c2c6df1214e6ab4414fea1b23db2

Author: Daniel Vetter <[email protected]>
Date:   Wed Apr 16 16:56:09 2014 +0200

drm/i915: Remove vblank wait from haswell_write_eld

and Ville's review of it [1].

Moreover, we should not touch the relevant registers before we get the audio power domain.

[1] http://mid.gmane.org/[email protected]

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/intel_audio.c |  2 --
 drivers/gpu/drm/i915/intel_ddi.c   | 11 ++---------
 2 files changed, 2 insertions(+), 11 deletions(-)

drivers/gpu/drm/i915/intel_audio.c +0 -2

index 4d644efde608..b9d3a143dd8c 100644
@@ -160,8 +160,6 @@ static void haswell_write_eld(struct drm_connector *connector,
        I915_WRITE(aud_cntrl_st2, tmp);
        POSTING_READ(aud_cntrl_st2);
 
-       assert_pipe_disabled(dev_priv, pipe);
-
        /* Set ELD valid state */
        tmp = I915_READ(aud_cntrl_st2);
        DRM_DEBUG_DRIVER("HDMI audio: pin eld vld status=0x%08x\n", tmp);

drivers/gpu/drm/i915/intel_ddi.c +2 -10

index 2688bc940879..56e7cb1ddc75 100644
@@ -1120,15 +1120,6 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)
        enum port port = intel_ddi_get_encoder_port(intel_encoder);
        int type = intel_encoder->type;
 
-       if (crtc->config.has_audio) {
-               DRM_DEBUG_DRIVER("Audio on pipe %c on DDI\n",
-                                pipe_name(crtc->pipe));
-
-               /* write eld */
-               DRM_DEBUG_DRIVER("DDI audio: write eld information\n");
-               intel_write_eld(intel_encoder);
-       }
-
        if (type == INTEL_OUTPUT_EDP) {
                struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
                intel_edp_panel_on(intel_dp);

@@ -1225,6 +1216,8 @@ static void intel_enable_ddi(struct intel_encoder *intel_encoder)
 
        if (intel_crtc->config.has_audio) {
                intel_display_power_get(dev_priv, POWER_DOMAIN_AUDIO);
+               intel_write_eld(intel_encoder);
+
                tmp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD);
                tmp |= ((AUDIO_OUTPUT_ENABLE_A | AUDIO_ELD_VALID_A) << (pipe * 4));
                I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, tmp);
--

2.1.1

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Signature · 2 lines

Rodrigo Vivi
Blog: http://blog.vivi.eng.br

Quoted 6 lines · Rodrigo Vivi

Quoted 3 lines · Jani Nikula

The audio programming sequence states that the ELD must be written and enabled after the pipe is ready. Indeed, this should clarify the situation with

Where/what doc can I confirm this?

Bspec -> Display -> North Display Engine Registers -> North Display Engine Audio -> Audio Programming Sequence etc.

Jani.

Quoted 66 lines

Quoted 66 lines

commit c79057922ed6c2c6df1214e6ab4414fea1b23db2

Author: Daniel Vetter <[email protected]>
Date:   Wed Apr 16 16:56:09 2014 +0200

drm/i915: Remove vblank wait from haswell_write_eld

and Ville's review of it [1].

Moreover, we should not touch the relevant registers before we get the audio power domain.

[1] http://mid.gmane.org/[email protected]

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/intel_audio.c |  2 --
 drivers/gpu/drm/i915/intel_ddi.c   | 11 ++---------
 2 files changed, 2 insertions(+), 11 deletions(-)

drivers/gpu/drm/i915/intel_audio.c +0 -2

index 4d644efde608..b9d3a143dd8c 100644
@@ -160,8 +160,6 @@ static void haswell_write_eld(struct drm_connector *connector,
        I915_WRITE(aud_cntrl_st2, tmp);
        POSTING_READ(aud_cntrl_st2);
 
-       assert_pipe_disabled(dev_priv, pipe);
-
        /* Set ELD valid state */
        tmp = I915_READ(aud_cntrl_st2);
        DRM_DEBUG_DRIVER("HDMI audio: pin eld vld status=0x%08x\n", tmp);

drivers/gpu/drm/i915/intel_ddi.c +2 -10

index 2688bc940879..56e7cb1ddc75 100644
@@ -1120,15 +1120,6 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)
        enum port port = intel_ddi_get_encoder_port(intel_encoder);
        int type = intel_encoder->type;
 
-       if (crtc->config.has_audio) {
-               DRM_DEBUG_DRIVER("Audio on pipe %c on DDI\n",
-                                pipe_name(crtc->pipe));
-
-               /* write eld */
-               DRM_DEBUG_DRIVER("DDI audio: write eld information\n");
-               intel_write_eld(intel_encoder);
-       }
-
        if (type == INTEL_OUTPUT_EDP) {
                struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
                intel_edp_panel_on(intel_dp);

@@ -1225,6 +1216,8 @@ static void intel_enable_ddi(struct intel_encoder *intel_encoder)
 
        if (intel_crtc->config.has_audio) {
                intel_display_power_get(dev_priv, POWER_DOMAIN_AUDIO);
+               intel_write_eld(intel_encoder);
+
                tmp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD);
                tmp |= ((AUDIO_OUTPUT_ENABLE_A | AUDIO_ELD_VALID_A) << (pipe * 4));
                I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, tmp);
--

2.1.1

Quoted 10 lines

Quoted 4 lines

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

--
Rodrigo Vivi
Blog: http://blog.vivi.eng.br

--
Jani Nikula, Intel Open Source Technology Center

Introduce functions to enable/disable the audio codec, incorporating the ELD setup within enable. The disable is initially limited to HSW, covering exactly what was done previously.

The only functional difference is that ELD valid is no longer set if there is no connector with ELD, which should be the right thing to do anyway. Otherwise the sequence remains the same, with warts and all, in preparation for applying more sanity.

v2: add kernel doc.

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/i915_drv.h    |  7 ++--
 drivers/gpu/drm/i915/intel_audio.c | 83 +++++++++++++++++++++++++++++---------
 drivers/gpu/drm/i915/intel_ddi.c   | 17 +-------
 drivers/gpu/drm/i915/intel_dp.c    |  2 +-
 drivers/gpu/drm/i915/intel_drv.h   |  3 +-
 drivers/gpu/drm/i915/intel_hdmi.c  |  2 +-
 6 files changed, 73 insertions(+), 41 deletions(-)

drivers/gpu/drm/i915/i915_drv.h +4 -3

index 6a73803482cb..0344fd561789 100644
@@ -483,9 +483,10 @@ struct drm_i915_display_funcs {
        void (*crtc_enable)(struct drm_crtc *crtc);
        void (*crtc_disable)(struct drm_crtc *crtc);
        void (*off)(struct drm_crtc *crtc);
-       void (*write_eld)(struct drm_connector *connector,
-                         struct intel_encoder *encoder,
-                         struct drm_display_mode *mode);
+       void (*audio_codec_enable)(struct drm_connector *connector,
+                                  struct intel_encoder *encoder,
+                                  struct drm_display_mode *mode);
+       void (*audio_codec_disable)(struct intel_encoder *encoder);
        void (*fdi_link_train)(struct drm_crtc *crtc);
        void (*init_clock_gating)(struct drm_device *dev);
        int (*queue_flip)(struct drm_device *dev, struct drm_crtc *crtc,

drivers/gpu/drm/i915/intel_audio.c +63 -20

index b9d3a143dd8c..c38c62eaebad 100644
@@ -96,9 +96,9 @@ static bool intel_eld_uptodate(struct drm_connector *connector,
        return true;
 }
 
-static void g4x_write_eld(struct drm_connector *connector,
-                         struct intel_encoder *encoder,
-                         struct drm_display_mode *mode)
+static void g4x_audio_codec_enable(struct drm_connector *connector,
+                                  struct intel_encoder *encoder,
+                                  struct drm_display_mode *mode)
 {
        struct drm_i915_private *dev_priv = connector->dev->dev_private;
        uint8_t *eld = connector->eld;

@@ -136,9 +136,22 @@ static void g4x_write_eld(struct drm_connector *connector,
        I915_WRITE(G4X_AUD_CNTL_ST, tmp);
 }
 
-static void haswell_write_eld(struct drm_connector *connector,
-                             struct intel_encoder *encoder,
-                             struct drm_display_mode *mode)
+static void hsw_audio_codec_disable(struct intel_encoder *encoder)
+{
+       struct drm_device *dev = encoder->base.dev;
+       struct drm_i915_private *dev_priv = dev->dev_private;
+       struct drm_crtc *crtc = encoder->base.crtc;
+       enum pipe pipe = to_intel_crtc(crtc)->pipe;
+       uint32_t tmp;
+
+       tmp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD);
+       tmp &= ~((AUDIO_OUTPUT_ENABLE_A | AUDIO_ELD_VALID_A) << (pipe * 4));
+       I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, tmp);
+}
+
+static void hsw_audio_codec_enable(struct drm_connector *connector,
+                                  struct intel_encoder *encoder,
+                                  struct drm_display_mode *mode)
 {
        struct drm_i915_private *dev_priv = connector->dev->dev_private;
        struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);

@@ -211,11 +224,16 @@ static void haswell_write_eld(struct drm_connector *connector,
        tmp = I915_READ(aud_cntrl_st2);
        tmp |= eldv;
        I915_WRITE(aud_cntrl_st2, tmp);
+
+       /* XXX: Transitional */
+       tmp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD);
+       tmp |= ((AUDIO_OUTPUT_ENABLE_A | AUDIO_ELD_VALID_A) << (pipe * 4));
+       I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, tmp);
 }
 
-static void ironlake_write_eld(struct drm_connector *connector,
-                              struct intel_encoder *encoder,
-                              struct drm_display_mode *mode)
+static void ilk_audio_codec_enable(struct drm_connector *connector,
+                                  struct intel_encoder *encoder,
+                                  struct drm_display_mode *mode)
 {
        struct drm_i915_private *dev_priv = connector->dev->dev_private;
        struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);

@@ -303,7 +321,14 @@ static void ironlake_write_eld(struct drm_connector *connector,
        I915_WRITE(aud_cntrl_st2, tmp);
 }
 
-void intel_write_eld(struct intel_encoder *intel_encoder)
+/**
+ * intel_audio_codec_enable - Enable the audio codec for HD audio
+ * @intel_encoder: encoder on which to enable audio
+ *
+ * The enable sequences may only be performed after enabling the transcoder and
+ * port, and after completed link training.
+ */
+void intel_audio_codec_enable(struct intel_encoder *intel_encoder)
 {
        struct drm_encoder *encoder = &intel_encoder->base;
        struct intel_crtc *crtc = to_intel_crtc(encoder->crtc);

@@ -329,8 +354,24 @@ void intel_write_eld(struct intel_encoder *intel_encoder)
 
        connector->eld[6] = drm_av_sync_delay(connector, mode) / 2;
 
-       if (dev_priv->display.write_eld)
-               dev_priv->display.write_eld(connector, intel_encoder, mode);
+       if (dev_priv->display.audio_codec_enable)
+               dev_priv->display.audio_codec_enable(connector, intel_encoder, mode);
+}
+
+/**
+ * intel_audio_codec_disable - Disable the audio codec for HD audio
+ * @encoder: encoder on which to disable audio
+ *
+ * The disable sequences must be performed before disabling the transcoder or
+ * port.
+ */
+void intel_audio_codec_disable(struct intel_encoder *encoder)
+{
+       struct drm_device *dev = encoder->base.dev;
+       struct drm_i915_private *dev_priv = dev->dev_private;
+
+       if (dev_priv->display.audio_codec_disable)
+               dev_priv->display.audio_codec_disable(encoder);
 }
 
 /**

@@ -341,12 +382,14 @@ void intel_init_audio(struct drm_device *dev)
 {
        struct drm_i915_private *dev_priv = dev->dev_private;
 
-       if (IS_G4X(dev))
-               dev_priv->display.write_eld = g4x_write_eld;
-       else if (IS_VALLEYVIEW(dev))
-               dev_priv->display.write_eld = ironlake_write_eld;
-       else if (IS_HASWELL(dev) || INTEL_INFO(dev)->gen >= 8)
-               dev_priv->display.write_eld = haswell_write_eld;
-       else if (HAS_PCH_SPLIT(dev))
-               dev_priv->display.write_eld = ironlake_write_eld;
+       if (IS_G4X(dev)) {
+               dev_priv->display.audio_codec_enable = g4x_audio_codec_enable;
+       } else if (IS_VALLEYVIEW(dev)) {
+               dev_priv->display.audio_codec_enable = ilk_audio_codec_enable;
+       } else if (IS_HASWELL(dev) || INTEL_INFO(dev)->gen >= 8) {
+               dev_priv->display.audio_codec_enable = hsw_audio_codec_enable;
+               dev_priv->display.audio_codec_disable = hsw_audio_codec_disable;
+       } else if (HAS_PCH_SPLIT(dev)) {
+               dev_priv->display.audio_codec_enable = ilk_audio_codec_enable;
+       }
 }

drivers/gpu/drm/i915/intel_ddi.c +2 -15

index 56e7cb1ddc75..b182b9b80461 100644
@@ -1186,12 +1186,10 @@ static void intel_enable_ddi(struct intel_encoder *intel_encoder)
        struct drm_encoder *encoder = &intel_encoder->base;
        struct drm_crtc *crtc = encoder->crtc;
        struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-       int pipe = intel_crtc->pipe;
        struct drm_device *dev = encoder->dev;
        struct drm_i915_private *dev_priv = dev->dev_private;
        enum port port = intel_ddi_get_encoder_port(intel_encoder);
        int type = intel_encoder->type;
-       uint32_t tmp;
 
        if (type == INTEL_OUTPUT_HDMI) {
                struct intel_digital_port *intel_dig_port =

@@ -1216,11 +1214,7 @@ static void intel_enable_ddi(struct intel_encoder *intel_encoder)
 
        if (intel_crtc->config.has_audio) {
                intel_display_power_get(dev_priv, POWER_DOMAIN_AUDIO);
-               intel_write_eld(intel_encoder);
-
-               tmp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD);
-               tmp |= ((AUDIO_OUTPUT_ENABLE_A | AUDIO_ELD_VALID_A) << (pipe * 4));
-               I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, tmp);
+               intel_audio_codec_enable(intel_encoder);
        }
 }
 

@@ -1229,19 +1223,12 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder)
        struct drm_encoder *encoder = &intel_encoder->base;
        struct drm_crtc *crtc = encoder->crtc;
        struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-       int pipe = intel_crtc->pipe;
        int type = intel_encoder->type;
        struct drm_device *dev = encoder->dev;
        struct drm_i915_private *dev_priv = dev->dev_private;
-       uint32_t tmp;
 
-       /* We can't touch HSW_AUD_PIN_ELD_CP_VLD uncionditionally because this
-        * register is part of the power well on Haswell. */
        if (intel_crtc->config.has_audio) {
-               tmp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD);
-               tmp &= ~((AUDIO_OUTPUT_ENABLE_A | AUDIO_ELD_VALID_A) <<
-                        (pipe * 4));
-               I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, tmp);
+               intel_audio_codec_disable(intel_encoder);
                intel_display_power_put(dev_priv, POWER_DOMAIN_AUDIO);
        }
 

drivers/gpu/drm/i915/intel_dp.c +1 -1

index eafe9f598a03..299c606a9b3c 100644
@@ -1250,7 +1250,7 @@ static void intel_dp_prepare(struct intel_encoder *encoder)
                DRM_DEBUG_DRIVER("Enabling DP audio on pipe %c\n",
                                 pipe_name(crtc->pipe));
                intel_dp->DP |= DP_AUDIO_OUTPUT_ENABLE;
-               intel_write_eld(encoder);
+               intel_audio_codec_enable(encoder);
        }
 
        /* Split out the IBX/CPU vs CPT settings */

drivers/gpu/drm/i915/intel_drv.h +2 -1

index bf72a9201a15..e4deb9cbc6bc 100644
@@ -850,7 +850,8 @@ void intel_fb_obj_flush(struct drm_i915_gem_object *obj, bool retire);
 
 /* intel_audio.c */
 void intel_init_audio(struct drm_device *dev);
-void intel_write_eld(struct intel_encoder *encoder);
+void intel_audio_codec_enable(struct intel_encoder *encoder);
+void intel_audio_codec_disable(struct intel_encoder *encoder);
 
 /* intel_display.c */
 const char *intel_output_name(int output);

drivers/gpu/drm/i915/intel_hdmi.c +1 -2

index 07b5ebd65d41..f29026a1157d 100644
@@ -666,7 +666,7 @@ static void intel_hdmi_prepare(struct intel_encoder *encoder)
                DRM_DEBUG_DRIVER("Enabling HDMI audio on pipe %c\n",
                                 pipe_name(crtc->pipe));
                hdmi_val |= SDVO_AUDIO_ENABLE;
-               intel_write_eld(encoder);
+               intel_audio_codec_enable(encoder);
        }
 
        if (HAS_PCH_CPT(dev))
--

2.1.1

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

We'll never end up in the hooks with eld[0] unset, as that's checked by drm_select_eld().

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/intel_audio.c | 12 ------------
 1 file changed, 12 deletions(-)

drivers/gpu/drm/i915/intel_audio.c +0 -13

index c38c62eaebad..076377f43a49 100644
@@ -79,9 +79,6 @@ static bool intel_eld_uptodate(struct drm_connector *connector,
        tmp = I915_READ(reg_eldv);
        tmp &= bits_eldv;
 
-       if (!eld[0])
-               return !tmp;
-
        if (!tmp)
                return false;
 

@@ -123,9 +120,6 @@ static void g4x_audio_codec_enable(struct drm_connector *connector,
        len = (tmp >> 9) & 0x1f;                /* ELD buffer size */
        I915_WRITE(G4X_AUD_CNTL_ST, tmp);
 
-       if (!eld[0])
-               return;
-
        len = min_t(int, eld[2], len);
        DRM_DEBUG_DRIVER("ELD size %d\n", len);
        for (i = 0; i < len; i++)

@@ -207,9 +201,6 @@ static void hsw_audio_codec_enable(struct drm_connector *connector,
        tmp &= ~eldv;
        I915_WRITE(aud_cntrl_st2, tmp);
 
-       if (!eld[0])
-               return;
-
        tmp = I915_READ(aud_cntl_st);
        tmp &= ~IBX_ELD_ADDRESS;
        I915_WRITE(aud_cntl_st, tmp);

@@ -304,9 +295,6 @@ static void ilk_audio_codec_enable(struct drm_connector *connector,
        tmp &= ~eldv;
        I915_WRITE(aud_cntrl_st2, tmp);
 
-       if (!eld[0])
-               return;
-
        tmp = I915_READ(aud_cntl_st);
        tmp &= ~IBX_ELD_ADDRESS;
        I915_WRITE(aud_cntl_st, tmp);
--

2.1.1

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

I'm not 100% convinced drm_select_eld will always cover this check... so What do you think about changing it to a BUG_ON or at least a WARN_ON?

Quoted 55 lines · Jani Nikula

We'll never end up in the hooks with eld[0] unset, as that's checked by drm_select_eld().

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/intel_audio.c | 12 ------------
 1 file changed, 12 deletions(-)

drivers/gpu/drm/i915/intel_audio.c +0 -13

index c38c62eaebad..076377f43a49 100644
@@ -79,9 +79,6 @@ static bool intel_eld_uptodate(struct drm_connector *connector,
        tmp = I915_READ(reg_eldv);
        tmp &= bits_eldv;
 
-       if (!eld[0])
-               return !tmp;
-
        if (!tmp)
                return false;
 

@@ -123,9 +120,6 @@ static void g4x_audio_codec_enable(struct drm_connector *connector,
        len = (tmp >> 9) & 0x1f;                /* ELD buffer size */
        I915_WRITE(G4X_AUD_CNTL_ST, tmp);
 
-       if (!eld[0])
-               return;
-
        len = min_t(int, eld[2], len);
        DRM_DEBUG_DRIVER("ELD size %d\n", len);
        for (i = 0; i < len; i++)

@@ -207,9 +201,6 @@ static void hsw_audio_codec_enable(struct drm_connector *connector,
        tmp &= ~eldv;
        I915_WRITE(aud_cntrl_st2, tmp);
 
-       if (!eld[0])
-               return;
-
        tmp = I915_READ(aud_cntl_st);
        tmp &= ~IBX_ELD_ADDRESS;
        I915_WRITE(aud_cntl_st, tmp);

@@ -304,9 +295,6 @@ static void ilk_audio_codec_enable(struct drm_connector *connector,
        tmp &= ~eldv;
        I915_WRITE(aud_cntrl_st2, tmp);
 
-       if (!eld[0])
-               return;
-
        tmp = I915_READ(aud_cntl_st);
        tmp &= ~IBX_ELD_ADDRESS;
        I915_WRITE(aud_cntl_st, tmp);
--

2.1.1

Quoted 4 lines

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

--
Rodrigo Vivi
Blog: http://blog.vivi.eng.br

I'm not 100% convinced drm_select_eld will always cover this check... so What do you think about changing it to a BUG_ON or at least a WARN_ON?

Make audio related register defines conform to existing style: Add _MASK where relevant, indent the defines for register contents, don't indent the defines for register addresses, prefix pipe specific register address defines with underscores, drop self explanatory comments.

No functional changes.

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/i915_reg.h    | 165 +++++++++++++++++++------------------
 drivers/gpu/drm/i915/intel_audio.c |  12 +--
 2 files changed, 89 insertions(+), 88 deletions(-)

drivers/gpu/drm/i915/i915_reg.h +83 -82

index 77fce96b54a5..6880f8ac452c 100644
@@ -5955,57 +5955,58 @@ enum punit_power_well {
 #define   GEN8_CENTROID_PIXEL_OPT_DIS  (1<<8)
 #define   GEN8_SAMPLER_POWER_BYPASS_DIS        (1<<1)
 
+/* Audio */
 #define G4X_AUD_VID_DID                        (dev_priv->info.display_mmio_offset + 0x62020)
-#define INTEL_AUDIO_DEVCL              0x808629FB
-#define INTEL_AUDIO_DEVBLC             0x80862801
-#define INTEL_AUDIO_DEVCTG             0x80862802
+#define   INTEL_AUDIO_DEVCL            0x808629FB
+#define   INTEL_AUDIO_DEVBLC           0x80862801
+#define   INTEL_AUDIO_DEVCTG           0x80862802
 
 #define G4X_AUD_CNTL_ST                        0x620B4
-#define G4X_ELDV_DEVCL_DEVBLC          (1 << 13)
-#define G4X_ELDV_DEVCTG                        (1 << 14)
-#define G4X_ELD_ADDR                   (0xf << 5)
-#define G4X_ELD_ACK                    (1 << 4)
+#define   G4X_ELDV_DEVCL_DEVBLC                (1 << 13)
+#define   G4X_ELDV_DEVCTG              (1 << 14)
+#define   G4X_ELD_ADDR_MASK            (0xf << 5)
+#define   G4X_ELD_ACK                  (1 << 4)
 #define G4X_HDMIW_HDMIEDID             0x6210C
 
-#define IBX_HDMIW_HDMIEDID_A           0xE2050
-#define IBX_HDMIW_HDMIEDID_B           0xE2150
+#define _IBX_HDMIW_HDMIEDID_A          0xE2050
+#define _IBX_HDMIW_HDMIEDID_B          0xE2150
 #define IBX_HDMIW_HDMIEDID(pipe) _PIPE(pipe, \
-                                       IBX_HDMIW_HDMIEDID_A, \
-                                       IBX_HDMIW_HDMIEDID_B)
-#define IBX_AUD_CNTL_ST_A              0xE20B4
-#define IBX_AUD_CNTL_ST_B              0xE21B4
+                                       _IBX_HDMIW_HDMIEDID_A, \
+                                       _IBX_HDMIW_HDMIEDID_B)
+#define _IBX_AUD_CNTL_ST_A             0xE20B4
+#define _IBX_AUD_CNTL_ST_B             0xE21B4
 #define IBX_AUD_CNTL_ST(pipe) _PIPE(pipe, \
-                                       IBX_AUD_CNTL_ST_A, \
-                                       IBX_AUD_CNTL_ST_B)
-#define IBX_ELD_BUFFER_SIZE            (0x1f << 10)
-#define IBX_ELD_ADDRESS                        (0x1f << 5)
-#define IBX_ELD_ACK                    (1 << 4)
+                                       _IBX_AUD_CNTL_ST_A, \
+                                       _IBX_AUD_CNTL_ST_B)
+#define   IBX_ELD_BUFFER_SIZE_MASK     (0x1f << 10)
+#define   IBX_ELD_ADDRESS_MASK         (0x1f << 5)
+#define   IBX_ELD_ACK                  (1 << 4)
 #define IBX_AUD_CNTL_ST2               0xE20C0
-#define IBX_ELD_VALIDB                 (1 << 0)
-#define IBX_CP_READYB                  (1 << 1)
+#define   IBX_ELD_VALIDB               (1 << 0)
+#define   IBX_CP_READYB                        (1 << 1)
 
-#define CPT_HDMIW_HDMIEDID_A           0xE5050
-#define CPT_HDMIW_HDMIEDID_B           0xE5150
+#define _CPT_HDMIW_HDMIEDID_A          0xE5050
+#define _CPT_HDMIW_HDMIEDID_B          0xE5150
 #define CPT_HDMIW_HDMIEDID(pipe) _PIPE(pipe, \
-                                       CPT_HDMIW_HDMIEDID_A, \
-                                       CPT_HDMIW_HDMIEDID_B)
-#define CPT_AUD_CNTL_ST_A              0xE50B4
-#define CPT_AUD_CNTL_ST_B              0xE51B4
+                                       _CPT_HDMIW_HDMIEDID_A, \
+                                       _CPT_HDMIW_HDMIEDID_B)
+#define _CPT_AUD_CNTL_ST_A             0xE50B4
+#define _CPT_AUD_CNTL_ST_B             0xE51B4
 #define CPT_AUD_CNTL_ST(pipe) _PIPE(pipe, \
-                                       CPT_AUD_CNTL_ST_A, \
-                                       CPT_AUD_CNTL_ST_B)
+                                       _CPT_AUD_CNTL_ST_A, \
+                                       _CPT_AUD_CNTL_ST_B)
 #define CPT_AUD_CNTRL_ST2              0xE50C0
 
-#define VLV_HDMIW_HDMIEDID_A           (VLV_DISPLAY_BASE + 0x62050)
-#define VLV_HDMIW_HDMIEDID_B           (VLV_DISPLAY_BASE + 0x62150)
+#define _VLV_HDMIW_HDMIEDID_A          (VLV_DISPLAY_BASE + 0x62050)
+#define _VLV_HDMIW_HDMIEDID_B          (VLV_DISPLAY_BASE + 0x62150)
 #define VLV_HDMIW_HDMIEDID(pipe) _PIPE(pipe, \
-                                       VLV_HDMIW_HDMIEDID_A, \
-                                       VLV_HDMIW_HDMIEDID_B)
-#define VLV_AUD_CNTL_ST_A              (VLV_DISPLAY_BASE + 0x620B4)
-#define VLV_AUD_CNTL_ST_B              (VLV_DISPLAY_BASE + 0x621B4)
+                                       _VLV_HDMIW_HDMIEDID_A, \
+                                       _VLV_HDMIW_HDMIEDID_B)
+#define _VLV_AUD_CNTL_ST_A             (VLV_DISPLAY_BASE + 0x620B4)
+#define _VLV_AUD_CNTL_ST_B             (VLV_DISPLAY_BASE + 0x621B4)
 #define VLV_AUD_CNTL_ST(pipe) _PIPE(pipe, \
-                                       VLV_AUD_CNTL_ST_A, \
-                                       VLV_AUD_CNTL_ST_B)
+                                       _VLV_AUD_CNTL_ST_A, \
+                                       _VLV_AUD_CNTL_ST_B)
 #define VLV_AUD_CNTL_ST2               (VLV_DISPLAY_BASE + 0x620C0)
 
 /* These are the 4 32-bit write offset registers for each stream

@@ -6014,28 +6015,28 @@ enum punit_power_well {
  */
 #define GEN7_SO_WRITE_OFFSET(n)                (0x5280 + (n) * 4)
 
-#define IBX_AUD_CONFIG_A                       0xe2000
-#define IBX_AUD_CONFIG_B                       0xe2100
+#define _IBX_AUD_CONFIG_A              0xe2000
+#define _IBX_AUD_CONFIG_B              0xe2100
 #define IBX_AUD_CFG(pipe) _PIPE(pipe, \
-                                       IBX_AUD_CONFIG_A, \
-                                       IBX_AUD_CONFIG_B)
-#define CPT_AUD_CONFIG_A                       0xe5000
-#define CPT_AUD_CONFIG_B                       0xe5100
+                                       _IBX_AUD_CONFIG_A, \
+                                       _IBX_AUD_CONFIG_B)
+#define _CPT_AUD_CONFIG_A              0xe5000
+#define _CPT_AUD_CONFIG_B              0xe5100
 #define CPT_AUD_CFG(pipe) _PIPE(pipe, \
-                                       CPT_AUD_CONFIG_A, \
-                                       CPT_AUD_CONFIG_B)
-#define VLV_AUD_CONFIG_A               (VLV_DISPLAY_BASE + 0x62000)
-#define VLV_AUD_CONFIG_B               (VLV_DISPLAY_BASE + 0x62100)
+                                       _CPT_AUD_CONFIG_A, \
+                                       _CPT_AUD_CONFIG_B)
+#define _VLV_AUD_CONFIG_A              (VLV_DISPLAY_BASE + 0x62000)
+#define _VLV_AUD_CONFIG_B              (VLV_DISPLAY_BASE + 0x62100)
 #define VLV_AUD_CFG(pipe) _PIPE(pipe, \
-                                       VLV_AUD_CONFIG_A, \
-                                       VLV_AUD_CONFIG_B)
+                                       _VLV_AUD_CONFIG_A, \
+                                       _VLV_AUD_CONFIG_B)
 
 #define   AUD_CONFIG_N_VALUE_INDEX             (1 << 29)
 #define   AUD_CONFIG_N_PROG_ENABLE             (1 << 28)
 #define   AUD_CONFIG_UPPER_N_SHIFT             20
-#define   AUD_CONFIG_UPPER_N_VALUE             (0xff << 20)
+#define   AUD_CONFIG_UPPER_N_MASK              (0xff << 20)
 #define   AUD_CONFIG_LOWER_N_SHIFT             4
-#define   AUD_CONFIG_LOWER_N_VALUE             (0xfff << 4)
+#define   AUD_CONFIG_LOWER_N_MASK              (0xfff << 4)
 #define   AUD_CONFIG_PIXEL_CLOCK_HDMI_SHIFT    16
 #define   AUD_CONFIG_PIXEL_CLOCK_HDMI_MASK     (0xf << 16)
 #define   AUD_CONFIG_PIXEL_CLOCK_HDMI_25175    (0 << 16)

@@ -6051,40 +6052,40 @@ enum punit_power_well {
 #define   AUD_CONFIG_DISABLE_NCTS              (1 << 3)
 
 /* HSW Audio */
-#define   HSW_AUD_CONFIG_A             0x65000 /* Audio Configuration Transcoder A */
-#define   HSW_AUD_CONFIG_B             0x65100 /* Audio Configuration Transcoder B */
-#define   HSW_AUD_CFG(pipe) _PIPE(pipe, \
-                                       HSW_AUD_CONFIG_A, \
-                                       HSW_AUD_CONFIG_B)
-
-#define   HSW_AUD_MISC_CTRL_A          0x65010 /* Audio Misc Control Convert 1 */
-#define   HSW_AUD_MISC_CTRL_B          0x65110 /* Audio Misc Control Convert 2 */
-#define   HSW_AUD_MISC_CTRL(pipe) _PIPE(pipe, \
-                                       HSW_AUD_MISC_CTRL_A, \
-                                       HSW_AUD_MISC_CTRL_B)
-
-#define   HSW_AUD_DIP_ELD_CTRL_ST_A    0x650b4 /* Audio DIP and ELD Control State Transcoder A */
-#define   HSW_AUD_DIP_ELD_CTRL_ST_B    0x651b4 /* Audio DIP and ELD Control State Transcoder B */
-#define   HSW_AUD_DIP_ELD_CTRL(pipe) _PIPE(pipe, \
-                                       HSW_AUD_DIP_ELD_CTRL_ST_A, \
-                                       HSW_AUD_DIP_ELD_CTRL_ST_B)
+#define _HSW_AUD_CONFIG_A              0x65000
+#define _HSW_AUD_CONFIG_B              0x65100
+#define HSW_AUD_CFG(pipe) _PIPE(pipe, \
+                                       _HSW_AUD_CONFIG_A, \
+                                       _HSW_AUD_CONFIG_B)
+
+#define _HSW_AUD_MISC_CTRL_A           0x65010
+#define _HSW_AUD_MISC_CTRL_B           0x65110
+#define HSW_AUD_MISC_CTRL(pipe) _PIPE(pipe, \
+                                       _HSW_AUD_MISC_CTRL_A, \
+                                       _HSW_AUD_MISC_CTRL_B)
+
+#define _HSW_AUD_DIP_ELD_CTRL_ST_A     0x650b4
+#define _HSW_AUD_DIP_ELD_CTRL_ST_B     0x651b4
+#define HSW_AUD_DIP_ELD_CTRL(pipe) _PIPE(pipe, \
+                                       _HSW_AUD_DIP_ELD_CTRL_ST_A, \
+                                       _HSW_AUD_DIP_ELD_CTRL_ST_B)
 
 /* Audio Digital Converter */
-#define   HSW_AUD_DIG_CNVT_1           0x65080 /* Audio Converter 1 */
-#define   HSW_AUD_DIG_CNVT_2           0x65180 /* Audio Converter 1 */
-#define   AUD_DIG_CNVT(pipe) _PIPE(pipe, \
-                                       HSW_AUD_DIG_CNVT_1, \
-                                       HSW_AUD_DIG_CNVT_2)
-#define   DIP_PORT_SEL_MASK            0x3
-
-#define   HSW_AUD_EDID_DATA_A          0x65050
-#define   HSW_AUD_EDID_DATA_B          0x65150
-#define   HSW_AUD_EDID_DATA(pipe) _PIPE(pipe, \
-                                       HSW_AUD_EDID_DATA_A, \
-                                       HSW_AUD_EDID_DATA_B)
-
-#define   HSW_AUD_PIPE_CONV_CFG                0x6507c /* Audio pipe and converter configs */
-#define   HSW_AUD_PIN_ELD_CP_VLD       0x650c0 /* Audio ELD and CP Ready Status */
+#define _HSW_AUD_DIG_CNVT_1            0x65080
+#define _HSW_AUD_DIG_CNVT_2            0x65180
+#define AUD_DIG_CNVT(pipe) _PIPE(pipe, \
+                                       _HSW_AUD_DIG_CNVT_1, \
+                                       _HSW_AUD_DIG_CNVT_2)
+#define DIP_PORT_SEL_MASK              0x3
+
+#define _HSW_AUD_EDID_DATA_A           0x65050
+#define _HSW_AUD_EDID_DATA_B           0x65150
+#define HSW_AUD_EDID_DATA(pipe) _PIPE(pipe, \
+                                       _HSW_AUD_EDID_DATA_A, \
+                                       _HSW_AUD_EDID_DATA_B)
+
+#define HSW_AUD_PIPE_CONV_CFG          0x6507c
+#define HSW_AUD_PIN_ELD_CP_VLD         0x650c0
 #define   AUDIO_INACTIVE_C             (1<<11)
 #define   AUDIO_INACTIVE_B             (1<<7)
 #define   AUDIO_INACTIVE_A             (1<<3)

drivers/gpu/drm/i915/intel_audio.c +6 -7

index 076377f43a49..8070414b50ad 100644
@@ -111,12 +111,12 @@ static void g4x_audio_codec_enable(struct drm_connector *connector,
 
        if (intel_eld_uptodate(connector,
                               G4X_AUD_CNTL_ST, eldv,
-                              G4X_AUD_CNTL_ST, G4X_ELD_ADDR,
+                              G4X_AUD_CNTL_ST, G4X_ELD_ADDR_MASK,
                               G4X_HDMIW_HDMIEDID))
                return;
 
        tmp = I915_READ(G4X_AUD_CNTL_ST);
-       tmp &= ~(eldv | G4X_ELD_ADDR);
+       tmp &= ~(eldv | G4X_ELD_ADDR_MASK);
        len = (tmp >> 9) & 0x1f;                /* ELD buffer size */
        I915_WRITE(G4X_AUD_CNTL_ST, tmp);
 

@@ -193,7 +193,7 @@ static void hsw_audio_codec_enable(struct drm_connector *connector,
 
        if (intel_eld_uptodate(connector,
                               aud_cntrl_st2, eldv,
-                              aud_cntl_st, IBX_ELD_ADDRESS,
+                              aud_cntl_st, IBX_ELD_ADDRESS_MASK,
                               hdmiw_hdmiedid))
                return;
 

@@ -202,7 +202,7 @@ static void hsw_audio_codec_enable(struct drm_connector *connector,
        I915_WRITE(aud_cntrl_st2, tmp);
 
        tmp = I915_READ(aud_cntl_st);
-       tmp &= ~IBX_ELD_ADDRESS;
+       tmp &= ~IBX_ELD_ADDRESS_MASK;
        I915_WRITE(aud_cntl_st, tmp);
        port = (tmp >> 29) & DIP_PORT_SEL_MASK;         /* DIP_Port_Select, 0x1 = PortB */
        DRM_DEBUG_DRIVER("port num:%d\n", port);

@@ -287,7 +287,7 @@ static void ilk_audio_codec_enable(struct drm_connector *connector,
 
        if (intel_eld_uptodate(connector,
                               aud_cntrl_st2, eldv,
-                              aud_cntl_st, IBX_ELD_ADDRESS,
+                              aud_cntl_st, IBX_ELD_ADDRESS_MASK,
                               hdmiw_hdmiedid))
                return;
 

@@ -296,7 +296,7 @@ static void ilk_audio_codec_enable(struct drm_connector *connector,
        I915_WRITE(aud_cntrl_st2, tmp);
 
        tmp = I915_READ(aud_cntl_st);
-       tmp &= ~IBX_ELD_ADDRESS;
+       tmp &= ~IBX_ELD_ADDRESS_MASK;
        I915_WRITE(aud_cntl_st, tmp);
 
        len = min_t(int, eld[2], 21);   /* 84 bytes of hw ELD buffer */
--

2.1.1

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx