Hi
2013/2/19 Daniel Vetter <daniel at ffwll.ch>:
> On Mon, Feb 18, 2013 at 07:00:27PM -0300, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni at intel.com>
>>
>> Some HDMI registers can be used for SDVO, so saying "HDMIB" should be
>> the same as saying "SDVOB" for a given HW generation. This was not
>> true and led to confusions and even a regression.
>>
>> Previously we had:
>> - SDVO{B,C} defined as the Gen3+ registers
>> - HDMI{B,C,D} and PCH_SDVOB defined as the PCH registers
>>
>> But now:
>> - SDVO{B,C} became GEN3_SDVO{B,C} on SDVO code
>> - SDVO{B,C} became GEN4_HDMI{B,C} on HDMI code
>> - HDMI{B,C,D} became PCH_HDMI{B,C,D}
>> - PCH_SDVOB is still the same thing
>>
>> v2: Rebase (v1 was sent in May 2012).
>>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
>
> I think we still have a bit of ugly left in here, especially that the
> register bit definitions are splattered all over irks me a bit. What about
> moving the HDMI stuff up to the SDVO definitions and giving the HDMI bits
> consisten HDMI_ prefixes? Imo there's no point in adding duplicate
> #defines for all the SDVO_ bits we use in intel_hdmi.c ...
I agree, there's more to clean up. I thought about amending your
suggestions to this patch, but I don't think this will be a good idea,
so I will send 3 additional patches on top of this one. Feel free to
merge them as a single patch if you want.
> -Daniel
>
>> ---
>> drivers/gpu/drm/i915/i915_reg.h | 19 ++++++++-------
>> drivers/gpu/drm/i915/intel_display.c | 42 ++++++++++++++++++----------------
>> drivers/gpu/drm/i915/intel_sdvo.c | 22 +++++++++---------
>> 3 files changed, 42 insertions(+), 41 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index 9e5844b..cd31af2 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -1680,8 +1680,9 @@
>> #define SDVOB_HOTPLUG_INT_STATUS_I915 (1 << 6)
>>
>> /* SDVO port control */
>> -#define SDVOB 0x61140
>> -#define SDVOC 0x61160
>> +#define GEN3_SDVOB 0x61140
>> +#define GEN3_SDVOC 0x61160
>> +#define PCH_SDVOB 0xe1140
>> #define SDVO_ENABLE (1 << 31)
>> #define SDVO_PIPE_B_SELECT (1 << 30)
>> #define SDVO_STALL_SELECT (1 << 29)
>> @@ -3979,8 +3980,12 @@
>> #define FDI_PLL_CTL_1 0xfe000
>> #define FDI_PLL_CTL_2 0xfe004
>>
>> -/* or SDVOB */
>> -#define HDMIB 0xe1140
>> +/* The same register may be used for SDVO or HDMI */
>> +#define GEN4_HDMIB GEN3_SDVOB
>> +#define GEN4_HDMIC GEN3_SDVOC
>> +#define PCH_HDMIB PCH_SDVOB
>> +#define PCH_HDMIC 0xe1150
>> +#define PCH_HDMID 0xe1160
>> #define PORT_ENABLE (1 << 31)
>> #define TRANSCODER(pipe) ((pipe) << 30)
>> #define TRANSCODER_CPT(pipe) ((pipe) << 29)
>> @@ -4001,12 +4006,6 @@
>> #define HSYNC_ACTIVE_HIGH (1 << 3)
>> #define PORT_DETECTED (1 << 2)
>>
>> -/* PCH SDVOB multiplex with HDMIB */
>> -#define PCH_SDVOB HDMIB
>> -
>> -#define HDMIC 0xe1150
>> -#define HDMID 0xe1160
>> -
>> #define PCH_LVDS 0xe1180
>> #define LVDS_DETECTED (1 << 1)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 6eb3882..744db70 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -1419,9 +1419,9 @@ static void assert_pch_ports_disabled(struct drm_i915_private *dev_priv,
>> "PCH LVDS enabled on transcoder %c, should be disabled\n",
>> pipe_name(pipe));
>>
>> - assert_pch_hdmi_disabled(dev_priv, pipe, HDMIB);
>> - assert_pch_hdmi_disabled(dev_priv, pipe, HDMIC);
>> - assert_pch_hdmi_disabled(dev_priv, pipe, HDMID);
>> + assert_pch_hdmi_disabled(dev_priv, pipe, PCH_HDMIB);
>> + assert_pch_hdmi_disabled(dev_priv, pipe, PCH_HDMIC);
>> + assert_pch_hdmi_disabled(dev_priv, pipe, PCH_HDMID);
>> }
>>
>> /**
>> @@ -8323,20 +8323,20 @@ static void intel_setup_outputs(struct drm_device *dev)
>> if (has_edp_a(dev))
>> intel_dp_init(dev, DP_A, PORT_A);
>>
>> - if (I915_READ(HDMIB) & PORT_DETECTED) {
>> + if (I915_READ(PCH_HDMIB) & PORT_DETECTED) {
>> /* PCH SDVOB multiplex with HDMIB */
>> found = intel_sdvo_init(dev, PCH_SDVOB, true);
>> if (!found)
>> - intel_hdmi_init(dev, HDMIB, PORT_B);
>> + intel_hdmi_init(dev, PCH_HDMIB, PORT_B);
>> if (!found && (I915_READ(PCH_DP_B) & DP_DETECTED))
>> intel_dp_init(dev, PCH_DP_B, PORT_B);
>> }
>>
>> - if (I915_READ(HDMIC) & PORT_DETECTED)
>> - intel_hdmi_init(dev, HDMIC, PORT_C);
>> + if (I915_READ(PCH_HDMIC) & PORT_DETECTED)
>> + intel_hdmi_init(dev, PCH_HDMIC, PORT_C);
>>
>> - if (!dpd_is_edp && I915_READ(HDMID) & PORT_DETECTED)
>> - intel_hdmi_init(dev, HDMID, PORT_D);
>> + if (!dpd_is_edp && I915_READ(PCH_HDMID) & PORT_DETECTED)
>> + intel_hdmi_init(dev, PCH_HDMID, PORT_D);
>>
>> if (I915_READ(PCH_DP_C) & DP_DETECTED)
>> intel_dp_init(dev, PCH_DP_C, PORT_C);
>> @@ -8348,24 +8348,26 @@ static void intel_setup_outputs(struct drm_device *dev)
>> if (I915_READ(VLV_DISPLAY_BASE + DP_C) & DP_DETECTED)
>> intel_dp_init(dev, VLV_DISPLAY_BASE + DP_C, PORT_C);
>>
>> - if (I915_READ(VLV_DISPLAY_BASE + SDVOB) & PORT_DETECTED) {
>> - intel_hdmi_init(dev, VLV_DISPLAY_BASE + SDVOB, PORT_B);
>> + if (I915_READ(VLV_DISPLAY_BASE + GEN4_HDMIB) & PORT_DETECTED) {
>> + intel_hdmi_init(dev, VLV_DISPLAY_BASE + GEN4_HDMIB,
>> + PORT_B);
>> if (I915_READ(VLV_DISPLAY_BASE + DP_B) & DP_DETECTED)
>> intel_dp_init(dev, VLV_DISPLAY_BASE + DP_B, PORT_B);
>> }
>>
>> - if (I915_READ(VLV_DISPLAY_BASE + SDVOC) & PORT_DETECTED)
>> - intel_hdmi_init(dev, VLV_DISPLAY_BASE + SDVOC, PORT_C);
>> + if (I915_READ(VLV_DISPLAY_BASE + GEN4_HDMIC) & PORT_DETECTED)
>> + intel_hdmi_init(dev, VLV_DISPLAY_BASE + GEN4_HDMIC,
>> + PORT_C);
>>
>> } else if (SUPPORTS_DIGITAL_OUTPUTS(dev)) {
>> bool found = false;
>>
>> - if (I915_READ(SDVOB) & SDVO_DETECTED) {
>> + if (I915_READ(GEN3_SDVOB) & SDVO_DETECTED) {
>> DRM_DEBUG_KMS("probing SDVOB\n");
>> - found = intel_sdvo_init(dev, SDVOB, true);
>> + found = intel_sdvo_init(dev, GEN3_SDVOB, true);
>> if (!found && SUPPORTS_INTEGRATED_HDMI(dev)) {
>> DRM_DEBUG_KMS("probing HDMI on SDVOB\n");
>> - intel_hdmi_init(dev, SDVOB, PORT_B);
>> + intel_hdmi_init(dev, GEN4_HDMIB, PORT_B);
>> }
>>
>> if (!found && SUPPORTS_INTEGRATED_DP(dev)) {
>> @@ -8376,16 +8378,16 @@ static void intel_setup_outputs(struct drm_device *dev)
>>
>> /* Before G4X SDVOC doesn't have its own detect register */
>>
>> - if (I915_READ(SDVOB) & SDVO_DETECTED) {
>> + if (I915_READ(GEN3_SDVOB) & SDVO_DETECTED) {
>> DRM_DEBUG_KMS("probing SDVOC\n");
>> - found = intel_sdvo_init(dev, SDVOC, false);
>> + found = intel_sdvo_init(dev, GEN3_SDVOC, false);
>> }
>>
>> - if (!found && (I915_READ(SDVOC) & SDVO_DETECTED)) {
>> + if (!found && (I915_READ(GEN3_SDVOC) & SDVO_DETECTED)) {
>>
>> if (SUPPORTS_INTEGRATED_HDMI(dev)) {
>> DRM_DEBUG_KMS("probing HDMI on SDVOC\n");
>> - intel_hdmi_init(dev, SDVOC, PORT_C);
>> + intel_hdmi_init(dev, GEN4_HDMIC, PORT_C);
>> }
>> if (SUPPORTS_INTEGRATED_DP(dev)) {
>> DRM_DEBUG_KMS("probing DP_C\n");
>> diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
>> index f01063a..7d94db8 100644
>> --- a/drivers/gpu/drm/i915/intel_sdvo.c
>> +++ b/drivers/gpu/drm/i915/intel_sdvo.c
>> @@ -246,11 +246,11 @@ static void intel_sdvo_write_sdvox(struct intel_sdvo *intel_sdvo, u32 val)
>> return;
>> }
>>
>> - if (intel_sdvo->sdvo_reg == SDVOB) {
>> - cval = I915_READ(SDVOC);
>> - } else {
>> - bval = I915_READ(SDVOB);
>> - }
>> + if (intel_sdvo->sdvo_reg == GEN3_SDVOB)
>> + cval = I915_READ(GEN3_SDVOC);
>> + else
>> + bval = I915_READ(GEN3_SDVOB);
>> +
>> /*
>> * Write the registers twice for luck. Sometimes,
>> * writing them only once doesn't appear to 'stick'.
>> @@ -258,10 +258,10 @@ static void intel_sdvo_write_sdvox(struct intel_sdvo *intel_sdvo, u32 val)
>> */
>> for (i = 0; i < 2; i++)
>> {
>> - I915_WRITE(SDVOB, bval);
>> - I915_READ(SDVOB);
>> - I915_WRITE(SDVOC, cval);
>> - I915_READ(SDVOC);
>> + I915_WRITE(GEN3_SDVOB, bval);
>> + I915_READ(GEN3_SDVOB);
>> + I915_WRITE(GEN3_SDVOC, cval);
>> + I915_READ(GEN3_SDVOC);
>> }
>> }
>>
>> @@ -1182,10 +1182,10 @@ static void intel_sdvo_mode_set(struct drm_encoder *encoder,
>> } else {
>> sdvox = I915_READ(intel_sdvo->sdvo_reg);
>> switch (intel_sdvo->sdvo_reg) {
>> - case SDVOB:
>> + case GEN3_SDVOB:
>> sdvox &= SDVOB_PRESERVE_MASK;
>> break;
>> - case SDVOC:
>> + case GEN3_SDVOC:
>> sdvox &= SDVOC_PRESERVE_MASK;
>> break;
>> }
>> --
>> 1.7.10.4
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
Paulo Zanoni
