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@xxxxxxxxx>
> >> Signed-off-by: Durgadoss R <durgadoss.r@xxxxxxxxx>
> >> Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivastava@xxxxxxxxx>
> >> ---
> >> 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@xxxxxxxxxxxxxxxxxxxxx
> >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
