Re: [PATCH 2/2] drm/i915: Set invert bit for hpd based on VBT

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Feb 16, 2016 at 07:29:03AM +0530, Thulasimani, Sivakumar wrote:
> 
> 
> 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@intel.com>
> >> Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
> >> Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivastava@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.

I think it is. We don't want to spread this stuff around all over the
place.

> 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 ?

I don't really see any point in doing it outside bxt_hpd_irq_setup().

> >
> >> +
> >>   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.

That's not how we roll generally. Besides we're alredy using the other
bits in that register on BXT.

> >> +#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.

I tried to do it globally, and while most of it turned out OK, there
were enough exceptions that I don't think it's necessarily worthwile.
It'll just make some of the bits parametrized and some not, so feels
a bit inconsistent.

> >> +#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.

Well, it'll tell you there's a bogus port in the VBT, but only if it
doesn't have invert==1, but it won't tell you about a bogus port with
invert==0. So seems like a somewhat half hearted attempt at verifying
the VBT to me.

> 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.

I tried to think how to do this more neatly and didn't really come up
with anything really elegant.

I suppose one thing we could try is to have this construct a bitmask
so it wouldn't have to loop through the child devs more than once.
And then the register setup could maybe do just something like:

if (invert_hpd & PORT_A)
	something |= BXT_DDIA_HPD_INVERT;
if (invert_hpd& PORT_B)
	something |= BXT_DDIB_HPD_INVERT;
if (invert_hpd & PORT_C)
	something |= BXT_DDIC_HPD_INVERT;

without looping since we don't loop there for the other bits either.

> 
> 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@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx