[PATCH 8/8] drm/i915: clarify confusion between SDVO and HDMI registers

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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


[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux