[Intel-gfx] [PATCH 2/2] drm/i915: Set invert bit for hpd based on VBT
Thulasimani, Sivakumar
sivakumar.thulasimani at intel.com
Tue Feb 16 01:59:03 UTC 2016
On 2/12/2016 7:38 PM, Ville Syrjälä wrote:
> On Fri, Feb 12, 2016 at 06:39:44PM +0530, Shubhangi Shrivastava wrote:
>> This patch sets the invert bit for hpd detection for each port
>> based on VBT configuration. Since each AOB can be designed to
>> depend on invert bit or not, it is expected if an AOB requires
>> invert bit, the user will set respective bit in VBT.
>>
>> v2: Separated VBT parsing from the rest of the logic. (Jani)
>>
>> Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani at intel.com>
>> Signed-off-by: Durgadoss R <durgadoss.r at intel.com>
>> Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivastava at intel.com>
>> ---
>> drivers/gpu/drm/i915/i915_drv.h | 1 +
>> drivers/gpu/drm/i915/i915_irq.c | 43 +++++++++++++++++++++++++++++++++++++++
>> drivers/gpu/drm/i915/i915_reg.h | 9 ++++++++
>> drivers/gpu/drm/i915/intel_bios.c | 42 ++++++++++++++++++++++++++++++++++++++
>> 4 files changed, 95 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 8216665..457f175 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -3393,6 +3393,7 @@ extern void intel_i2c_reset(struct drm_device *dev);
>> /* intel_bios.c */
>> int intel_bios_init(struct drm_i915_private *dev_priv);
>> bool intel_bios_is_valid_vbt(const void *buf, size_t size);
>> +bool intel_bios_is_port_hpd_inverted(struct drm_device *dev, enum port port);
>>
>> /* intel_opregion.c */
>> #ifdef CONFIG_ACPI
>> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
>> index 25a8937..fb95fb0 100644
>> --- a/drivers/gpu/drm/i915/i915_irq.c
>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> @@ -36,6 +36,7 @@
>> #include "i915_drv.h"
>> #include "i915_trace.h"
>> #include "intel_drv.h"
>> +#include "intel_bios.h"
>>
>> /**
>> * DOC: interrupt handling
>> @@ -3424,6 +3425,47 @@ static void ibx_hpd_irq_setup(struct drm_device *dev)
>> I915_WRITE(PCH_PORT_HOTPLUG, hotplug);
>> }
>>
>> +/*
>> + * For BXT invert bit has to be set based on AOB design
>> + * for HPD detection logic, update it based on VBT fields.
>> + */
>> +static void bxt_hpd_set_invert(struct drm_device *dev, u32 hotplug_port)
>> +{
>> + struct drm_i915_private *dev_priv = dev->dev_private;
>> + int reg_val, val = 0;
>> + enum port port;
>> +
>> + for (port = PORT_A; port <= PORT_C; port++) {
>> +
>> + /* Proceed only if invert bit is set */
>> + if (intel_bios_is_port_hpd_inverted(dev, port)) {
>> + switch (port) {
>> + case PORT_A:
>> + if (hotplug_port & BXT_DE_PORT_HP_DDIA)
>> + val |= BXT_DDIA_HPD_INVERT;
>> + break;
>> + case PORT_B:
>> + if (hotplug_port & BXT_DE_PORT_HP_DDIB)
>> + val |= BXT_DDIB_HPD_INVERT;
>> + break;
>> + case PORT_C:
>> + if (hotplug_port & BXT_DE_PORT_HP_DDIC)
>> + val |= BXT_DDIC_HPD_INVERT;
>> + break;
>> + default:
>> + DRM_ERROR("HPD invert set for invalid port %d\n",
>> + port);
>> + break;
>> + }
>> + }
>> + }
>> + reg_val = I915_READ(BXT_HOTPLUG_CTL);
>> + DRM_DEBUG_KMS("Invert bit setting: hp_ctl:%x hp_port:%x val:%x\n",
>> + reg_val, hotplug_port, val);
>> + reg_val &= ~BXT_DDI_HPD_INVERT_MASK;
>> + I915_WRITE(BXT_HOTPLUG_CTL, reg_val | val);
>> +}
> No RMW stuff please. Just set up the bits appropriately in bxt_hpd_irq_setup().
the problem is we need to query vbt for setting invert bit. so not sure
having this logic
inside bxt_hpd_irq_setup is good. if we want to avoid read/write to
registers we can
modify the input to be values written on register instead of
enabled_irqs. that way
we can write the value to register post call to this function and we
need not
worry about current values in register. is that fine ?
>
>> +
>> static void spt_hpd_irq_setup(struct drm_device *dev)
>> {
>> struct drm_i915_private *dev_priv = dev->dev_private;
>> @@ -3494,6 +3536,7 @@ static void bxt_hpd_irq_setup(struct drm_device *dev)
>> hotplug |= PORTC_HOTPLUG_ENABLE | PORTB_HOTPLUG_ENABLE |
>> PORTA_HOTPLUG_ENABLE;
>> I915_WRITE(PCH_PORT_HOTPLUG, hotplug);
>> + bxt_hpd_set_invert(dev, enabled_irqs);
>> }
>>
>> static void ibx_irq_postinstall(struct drm_device *dev)
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index 6732fc1..66cf92e 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -5940,6 +5940,15 @@ enum skl_disp_power_wells {
>> #define GEN8_PCU_IIR _MMIO(0x444e8)
>> #define GEN8_PCU_IER _MMIO(0x444ec)
>>
>> +/* BXT hotplug control */
>> +#define BXT_HOTPLUG_CTL _MMIO(0xC4030)
> We already have a name for that register.
yes, but as mentioned by you below the register bits are different.
we did not want to confuse by using an old register define when the
bits are different.
>> +#define BXT_DDIA_HPD_INVERT (1 << 27)
>> +#define BXT_DDIC_HPD_INVERT (1 << 11)
>> +#define BXT_DDIB_HPD_INVERT (1 << 3)
> I was going to suggest parametrizing this stuff, but the bits are in a
> fairly nasty order so not so easy, and we haven't parametrized the rest
> if the hpd bits either, so doing it just for these would be a bit out of
> place perhaps.
>
> We should perhaps try to parametrize all the hpd bits though, since that
> could result in some neater looking code. But that sort of stuff is
> better left for a separate patch. After a bit more though it's not
> actually hard at all: (((port) * 8 + 24 + whatever) & 31)
hmmm will check and get back on if this can be done.
>> +#define BXT_DDI_HPD_INVERT_MASK (BXT_DDIA_HPD_INVERT | \
>> + BXT_DDIB_HPD_INVERT | \
>> + BXT_DDIC_HPD_INVERT)
>> +
>> #define ILK_DISPLAY_CHICKEN2 _MMIO(0x42004)
>> /* Required on all Ironlake and Sandybridge according to the B-Spec. */
>> #define ILK_ELPIN_409_SELECT (1 << 25)
>> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
>> index a26d4b4..24d0077 100644
>> --- a/drivers/gpu/drm/i915/intel_bios.c
>> +++ b/drivers/gpu/drm/i915/intel_bios.c
>> @@ -105,6 +105,48 @@ find_section(const void *_bdb, int section_id)
>> return NULL;
>> }
>>
>> +bool
>> +intel_bios_is_port_hpd_inverted(struct drm_device *dev, enum port port)
>> +{
>> + struct drm_i915_private *dev_priv = dev->dev_private;
>> + int i;
>> +
>> + if (!IS_BROXTON(dev)) {
>> + DRM_ERROR("Bit inversion is not required in this platform\n");
>> + return false;
>> + }
>> +
>> + for (i = 0; i < dev_priv->vbt.child_dev_num; i++) {
>> +
>> + if (dev_priv->vbt.child_dev[i].common.hpd_invert == 1) {
>> +
>> + switch (dev_priv->vbt.child_dev[i].common.dvo_port) {
>> + case DVO_PORT_DPA:
>> + case DVO_PORT_HDMIA:
>> + if (port == PORT_A)
>> + return true;
>> + break;
>> + case DVO_PORT_DPB:
>> + case DVO_PORT_HDMIB:
>> + if (port == PORT_B)
>> + return true;
>> + break;
>> + case DVO_PORT_DPC:
>> + case DVO_PORT_HDMIC:
>> + if (port == PORT_C)
>> + return true;
>> + break;
>> + default:
>> + DRM_DEBUG_KMS("This dvo port %d doesn't need invert\n",
>> + dev_priv->vbt.child_dev[i].common.dvo_port);
> Why do we need a debug message for the "no invert" case if we don't
> need one for the invert case?
>
> The code structure feels a bit wonky. It's sort of expecting multiple
> child devs for each port. Can that actually happen?
the debug message is just a fail safe to let know if VBT programming is
incorrect.
it can be removed. but we already found and fixed a incorrect programming in
default VBT with this so i would recommend keeping it :).
regarding child devs, the invert bit can be set for HDMI or DP based on
the board
design so we need to handle this for both port types. hence we are
checking for
both here. i too felt it odd having to loop twice once here and again in
bxt_hpd_set_invert.
if we can come up with a simpler design then i would favor it as well
but for now
this seems to be required.
regards,
Sivakumar
>
>> + break;
>> + }
>> + }
>> + }
>> +
>> + return false;
>> +}
>> +
>> static void
>> fill_detail_timing_data(struct drm_display_mode *panel_fixed_mode,
>> const struct lvds_dvo_timing *dvo_timing)
>> --
>> 2.6.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
More information about the Intel-gfx
mailing list