On Fri, Jul 5, 2013 at 6:58 PM, Paulo Zanoni <przanoni at gmail.com> wrote:
> Hi
>
> Sorry for the delay.
>
> 2013/7/1 Rodrigo Vivi <rodrigo.vivi at gmail.com>:
>> On Fri, Jun 28, 2013 at 4:31 PM, Paulo Zanoni <przanoni at gmail.com> wrote:
>>> Hi
>>>
>>> 2013/6/28 Rodrigo Vivi <rodrigo.vivi at gmail.com>:
>>>> Adding Enable and Disable PSR functionalities. This includes setting the
>>>> PSR configuration over AUX, sending SDP VSC DIP over the eDP PIPE config,
>>>> enabling PSR in the sink via DPCD register and finally enabling PSR on
>>>> the host.
>>>>
>>>> This patch is based on initial PSR code by Sateesh Kavuri and Kumar Shobhit
>>>> but in a different implementation.
>>>>
>>>> v2: * moved functions around and changed its names.
>>>> * removed VSC DIP unset from disable.
>>>> * remove FBC wa.
>>>> * don't mask LSPS anymore.
>>>> * incorporate new crtc usage after a rebase.
>>>> v3: Make a clear separation between Sink (Panel) and Source (HW) enabling.
>>>> v4: Fix identation and other style issues raised by checkpatch (by Paulo).
>>>>
>>>
>>> A few of the comments here were already present in previous reviews.
>>> If you think they're not needed, please reply saying why.
>>
>> Thank you very much for reviewing it and for including old comments I
>> had missed.
>> I'm replying this email saying what I changed and explaining what I didn't.
>> updated patch coming later.
>>
>>>
>>>
>>>> Credits-by: Sateesh Kavuri <sateesh.kavuri at intel.com>
>>>> Credits-by: Shobhit Kumar <shobhit.kumar at intel.com>
>>>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi at gmail.com>
>>>> ---
>>>> drivers/gpu/drm/i915/i915_reg.h | 42 +++++++++++
>>>> drivers/gpu/drm/i915/intel_dp.c | 151 +++++++++++++++++++++++++++++++++++++++
>>>> drivers/gpu/drm/i915/intel_drv.h | 3 +
>>>> 3 files changed, 196 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>>>> index 137be4c..caf57d8 100644
>>>> --- a/drivers/gpu/drm/i915/i915_reg.h
>>>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>>>> @@ -1777,6 +1777,47 @@
>>>> #define BCLRPAT(pipe) _PIPE(pipe, _BCLRPAT_A, _BCLRPAT_B)
>>>> #define VSYNCSHIFT(trans) _TRANSCODER(trans, _VSYNCSHIFT_A, _VSYNCSHIFT_B)
>>>>
>>>> +/* HSW eDP PSR registers */
>>>> +#define EDP_PSR_CTL 0x64800
>>>> +#define EDP_PSR_ENABLE (1<<31)
>>>> +#define EDP_PSR_LINK_DISABLE (0<<27)
>>>> +#define EDP_PSR_LINK_STANDBY (1<<27)
>>>> +#define EDP_PSR_MIN_LINK_ENTRY_TIME_MASK (3<<25)
>>>> +#define EDP_PSR_MIN_LINK_ENTRY_TIME_8_LINES (0<<25)
>>>> +#define EDP_PSR_MIN_LINK_ENTRY_TIME_4_LINES (1<<25)
>>>> +#define EDP_PSR_MIN_LINK_ENTRY_TIME_2_LINES (2<<25)
>>>> +#define EDP_PSR_MIN_LINK_ENTRY_TIME_0_LINES (3<<25)
>>>> +#define EDP_PSR_MAX_SLEEP_TIME_SHIFT 20
>>>> +#define EDP_PSR_SKIP_AUX_EXIT (1<<12)
>>>> +#define EDP_PSR_TP1_TP2_SEL (0<<11)
>>>> +#define EDP_PSR_TP1_TP3_SEL (1<<11)
>>>> +#define EDP_PSR_TP2_TP3_TIME_500us (0<<8)
>>>> +#define EDP_PSR_TP2_TP3_TIME_100us (1<<8)
>>>> +#define EDP_PSR_TP2_TP3_TIME_2500us (2<<8)
>>>> +#define EDP_PSR_TP2_TP3_TIME_0us (3<<8)
>>>> +#define EDP_PSR_TP1_TIME_500us (0<<4)
>>>> +#define EDP_PSR_TP1_TIME_100us (1<<4)
>>>> +#define EDP_PSR_TP1_TIME_2500us (2<<4)
>>>> +#define EDP_PSR_TP1_TIME_0us (3<<4)
>>>> +#define EDP_PSR_IDLE_FRAME_SHIFT 0
>>>> +
>>>> +#define EDP_PSR_AUX_CTL 0x64810
>>>> +#define EDP_PSR_AUX_DATA1 0x64814
>>>> +#define EDP_PSR_DPCD_COMMAND 0x80060000
>>>> +#define EDP_PSR_AUX_DATA2 0x64818
>>>> +#define EDP_PSR_DPCD_NORMAL_OPERATION (1<<24)
>>>
>>> I know our documentation explicitly says "0x80060000" and
>>> "0x01000000", but to me these magic values are just magic... I think
>>> we could try to reuse the same mechanism we use for the other aux
>>> messages. Check the usage of pack_aux inside intel_dp_aux_ch and also
>>> intel_dp_aux_native_write. Maybe this could be done in a follow-up
>>> patch. Jani gave a suggestion on how to implement this, see email "Re:
>>> [PATCH 5/9] drm/i915: Setup EDP PSR AUX Registers" from January 31.
>>
>> I know this is just magic, but writing some values in some specific
>> aux dst is obfuscated magic.
>> At least the magic implemented here is documented somewhere and more
>> easy to understand.
>>
>>>
>>>
>>>> +#define EDP_PSR_AUX_DATA3 0x6481c
>>>> +#define EDP_PSR_AUX_DATA4 0x64820
>>>> +#define EDP_PSR_AUX_DATA5 0x64824
>>>> +
>>>> +#define EDP_PSR_STATUS_CTL 0x64840
>>>> +#define EDP_PSR_STATUS_STATE_MASK (7<<29)
>>>> +
>>>> +#define EDP_PSR_DEBUG_CTL 0x64860
>>>> +#define EDP_PSR_DEBUG_MASK_LPSP (1<<27)
>>>> +#define EDP_PSR_DEBUG_MASK_MEMUP (1<<26)
>>>> +#define EDP_PSR_DEBUG_MASK_HPD (1<<25)
>>>> +
>>>> /* VGA port control */
>>>> #define ADPA 0x61100
>>>> #define PCH_ADPA 0xe1100
>>>> @@ -2046,6 +2087,7 @@
>>>> * (Haswell and newer) to see which VIDEO_DIP_DATA byte corresponds to each byte
>>>> * of the infoframe structure specified by CEA-861. */
>>>> #define VIDEO_DIP_DATA_SIZE 32
>>>> +#define VIDEO_DIP_VSC_DATA_SIZE 36
>>>> #define VIDEO_DIP_CTL 0x61170
>>>> /* Pre HSW: */
>>>> #define VIDEO_DIP_ENABLE (1 << 31)
>>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>>>> index dca8fa6..c10be94 100644
>>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>>> @@ -1356,6 +1356,157 @@ static bool is_edp_psr(struct intel_dp *intel_dp)
>>>> intel_dp->psr_dpcd[0] & DP_PSR_IS_SUPPORTED;
>>>> }
>>>>
>>>> +static bool intel_edp_is_psr_enabled(struct drm_device *dev)
>>>> +{
>>>> + struct drm_i915_private *dev_priv = dev->dev_private;
>>>> +
>>>> + if (!IS_HASWELL(dev))
>>>> + return false;
>>>> +
>>>> + return I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE;
>>>> +}
>>>> +
>>>> +void intel_edp_psr_write_vsc(struct intel_dp *intel_dp,
>>>> + struct edp_vsc_psr *vsc_psr)
>>>
>>> This function should be static (or included in a .h file).
>>
>> done.
>>
>>>
>>>
>>>> +{
>>>> + struct drm_device *dev = intel_dp_to_dev(intel_dp);
>>>
>>> Function intel_dp_to_dev calls dp_to_dig_port, but you're already
>>> calling dp_to_dig_port below. You could remove this intel_dp_to_dev
>>> call by reordering the definitions.
>>
>> done.
>>
>>>
>>>
>>>> + struct drm_i915_private *dev_priv = dev->dev_private;
>>>> + struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
>>>> + struct intel_crtc *crtc = to_intel_crtc(dig_port->base.base.crtc);
>>>> +
>>>> + u32 ctl_reg = HSW_TVIDEO_DIP_CTL(crtc->config.cpu_transcoder);
>>>> + u32 data_reg = HSW_TVIDEO_DIP_VSC_DATA(crtc->config.cpu_transcoder);
>>>> + uint32_t *data = (uint32_t *) vsc_psr;
>>>> + unsigned int i;
>>>> + u32 val = I915_READ(ctl_reg);
>>>
>>> We don't use this register for anything else on eDP, so instead of
>>> preserving its contents with this read we should just set the bits we
>>> want, zeroing everything else. We don't want to preserve bogus values.
>>
>> done.
>>
>>> Also, read below: if we move this code to mode_set time it will make
>>> even more sense.
>>>
>>>
>>>> +
>>>> + /* As per eDP spec, wait for vblank to send SDP VSC packet */
>>>> + intel_wait_for_vblank(dev, crtc->pipe);
>>>
>>> The spec says the SDP VSC packets should be sent during the vblank,
>>> but the HW does this automatically and intel_wait_for_vblank doesn't
>>> guarantee us anything regarding that. So that wait seems useless.
>>>
>>>
>>>> +
>>>> + /* As per BSPec (Pipe Video Data Island Packet), besides wait for
>>>> + vsync we need to disable the video DIP being updated before program
>>>> + video DIP data buffer registers for DIP being updated.*/
>>>
>>> My interpretation of the spec is that we need to wait until exactly
>>> after the VSync so we don't send incomplete packets, but we don't have
>>> a good way to do this, and intel_wait_for_vblank doesn't help us with
>>> that. If you take a look at the HDMI code you'll notice that we set
>>> the video DIP registers inside intel_hdmi_mode_set, because at that
>>> point the pipe is stopped and we don't need to worry about waiting for
>>> the exact vblank period. Perhaps we should do the same here: load the
>>> contents of the video dip registers at mode_set time? Is there any
>>> problem in keeping those values there even when PSR or the pipe is
>>> disabled?
>>
>>
>> By moving it to mode_set time I started to get some other bugs when
>> exiting PSR state.
>> Also on mode_set time you don't know if psr will be enabled or not and
>> maybe writting this vsc will be useless.
>> I understand vblank is not ideal, but it works and since we don't have
>> a wait_for_vsync implemented I prefer to stay with this version that
>> works.
>
> What if we just remove the wait_for_vblank line since it doesn't
> really seem to help us and may make us skip a few frames? The best
> thing is probably to write the wait_for_vsync function anyway.
ok, I'll try that
>
>>
>>>
>>>
>>>> + I915_WRITE(ctl_reg, val & ~VIDEO_DIP_ENABLE_VSC_HSW);
>>>> + POSTING_READ(ctl_reg);
>>>> +
>>>> + for (i = 0; i < VIDEO_DIP_VSC_DATA_SIZE; i += 4) {
>>>> + if (i < sizeof(struct edp_vsc_psr))
>>>> + I915_WRITE(data_reg + i, *data++);
>>>> + else
>>>> + I915_WRITE(data_reg + i, 0);
>>>> + }
>>>> +
>>>> + I915_WRITE(ctl_reg, val | VIDEO_DIP_ENABLE_VSC_HSW);
>>>
>>> The PSR enabling documentation says that the "HW also enables VSC DIP
>>> when required", so maybe we should not turn the
>>> VIDEO_DIP_ENABLE_VSC_HSW bit and see if the HW does that for us? My
>>> fear is that setting this unconditionally may make some panels
>>> confused. But I'm not really sure if the HW really sets this bit for
>>> us or just sends/stops-sending the DIP in case this bit is on.
>>
>> unfortunately it didn't worked. we have to manually enable it back
>> when manually disabling it.
>>
>>>
>>>
>>>> + POSTING_READ(ctl_reg);
>>>> +}
>>>> +
>>>> +static void intel_edp_psr_enable_sink(struct intel_dp *intel_dp)
>>>> +{
>>>> + struct drm_device *dev = intel_dp_to_dev(intel_dp);
>>>> + struct drm_i915_private *dev_priv = dev->dev_private;
>>>> + struct edp_vsc_psr psr_vsc;
>>>> + uint32_t aux_clock_divider = get_aux_clock_divider(intel_dp);
>>>> + int precharge = 0x3;
>>>> + int msg_size = 5; /* Header(4) + Message(1) */
>>>
>>> If you implement our suggestion of replacing the magic 0x80060000
>>> value with code this magic msg_size will also disappear.
>>
>> I don't see how. This is aux ctl, not aux data. Besides it is the same
>> used on dp_aux.
>>
>>>
>>>
>>>> +
>>>> + /* Prepare VSC packet as per EDP 1.3 spec, Table 3.10 */
>>>> + memset(&psr_vsc, 0, sizeof(psr_vsc));
>>>> + psr_vsc.sdp_header.HB0 = 0;
>>>> + psr_vsc.sdp_header.HB1 = 0x7;
>>>> + psr_vsc.sdp_header.HB2 = 0x2;
>>>> + psr_vsc.sdp_header.HB3 = 0x8;
>>>> + intel_edp_psr_write_vsc(intel_dp, &psr_vsc);
>>>> +
>>>> + /* Enable PSR in sink */
>>>> + if (intel_dp->psr_dpcd[1] & DP_PSR_NO_TRAIN_ON_EXIT)
>>>> + intel_dp_aux_native_write_1(intel_dp, DP_PSR_EN_CFG,
>>>> + DP_PSR_ENABLE &
>>>> + ~DP_PSR_MAIN_LINK_ACTIVE);
>>>> + else
>>>> + intel_dp_aux_native_write_1(intel_dp, DP_PSR_EN_CFG,
>>>> + DP_PSR_ENABLE |
>>>> + DP_PSR_MAIN_LINK_ACTIVE);
>>>> +
>>>> + /* Setup AUX registers */
>>>> + I915_WRITE(EDP_PSR_AUX_DATA1, EDP_PSR_DPCD_COMMAND);
>>>> + I915_WRITE(EDP_PSR_AUX_DATA2, EDP_PSR_DPCD_NORMAL_OPERATION);
>>>> + I915_WRITE(EDP_PSR_AUX_CTL,
>>>> + DP_AUX_CH_CTL_TIME_OUT_400us |
>>>> + (msg_size << DP_AUX_CH_CTL_MESSAGE_SIZE_SHIFT) |
>>>> + (precharge << DP_AUX_CH_CTL_PRECHARGE_2US_SHIFT) |
>>>> + (aux_clock_divider << DP_AUX_CH_CTL_BIT_CLOCK_2X_SHIFT));
>>>> +}
>>>> +
>>>> +static void intel_edp_psr_enable_source(struct intel_dp *intel_dp)
>>>> +{
>>>> + struct drm_device *dev = intel_dp_to_dev(intel_dp);
>>>> + struct drm_i915_private *dev_priv = dev->dev_private;
>>>> + uint32_t max_sleep_time = 0x1f;
>>>> + uint32_t idle_frames = 1;
>>>> + uint32_t val = 0x0;
>>>> +
>>>> + if (intel_dp->psr_dpcd[1] & DP_PSR_NO_TRAIN_ON_EXIT) {
>>>> + val |= EDP_PSR_LINK_STANDBY;
>>>> + val |= EDP_PSR_TP2_TP3_TIME_0us;
>>>> + val |= EDP_PSR_TP1_TIME_0us;
>>>> + val |= EDP_PSR_SKIP_AUX_EXIT;
>>>> + } else {
>>>> + val |= EDP_PSR_LINK_DISABLE;
>>>> + val |= EDP_PSR_TP1_TIME_500us;
>>>> + val |= EDP_PSR_TP2_TP3_TIME_500us;
>>>
>>> Why are we using these 500us values? I couldn't find a place that
>>> tells us which ones to use.
>>
>> just the default... useless and already removed.
>
> I think we should try to discover what is the correct value to write here.
The correct are the default ones. The one that works already.
At some point in the future after we can finally get this feature
accepted I intend to rewrite that VBT patch and get this info from
VBT. I just gave up on the patch that gets this value from vbt
because vbt was inconsistent regarding versions... that one you had
seem...
>
>
>>
>>>
>>>
>>>> + }
>>>> +
>>>> + /* Avoid continuous PSR exit by masking memup and hpd */
>>>> + I915_WRITE(EDP_PSR_DEBUG_CTL, EDP_PSR_DEBUG_MASK_MEMUP |
>>>> + EDP_PSR_DEBUG_MASK_HPD);
>>>
>>> Do we have a workaround name for the line above? Damien recently made
>>> a nice effort to document all the WA names we implement. We should at
>>> least say this is a WA.
>>
>> Unfortunately it is not documented on WA database.
>> MEM up mask came only from PM guide doc and HPD came from the hardest path...
>> I had to mask everything and unmasking one by one to find out why we
>> were getting the continuous psr exit.
>> I tried to unsed long and short hpd pulse as described in pm guide,
>> but it refuses to change. So I decided to mask that.
>
> Did you check if we had a pending interrupt to clear?
>
>>
>>>
>>>
>>>> +
>>>> + /* Disable unused interrupts */
>>>> + I915_WRITE(GEN6_PMINTRMSK, GEN6_PM_RP_UP_EI_EXPIRED |
>>>> + GEN6_PM_RP_DOWN_EI_EXPIRED);
>>>
>>> The line above needs a very big comment explaining it. Also, I'm not
>>> sure the code is correct. Don't we need irq_lock or rps_lock, also
>>> adjust some dev_priv->xyz_iir variable? I'll leave the review of this
>>> line to Ben and Daniel since they're currently touching these things.
>>
>> I checked this is useless in our case. removed.
>>
>
> Ok, looks like it's just in case PSR is continuously existing...
>
>>>
>>>
>>>> +
>>>> + I915_WRITE(EDP_PSR_CTL, val |
>>>> + EDP_PSR_MIN_LINK_ENTRY_TIME_8_LINES |
>>>> + max_sleep_time << EDP_PSR_MAX_SLEEP_TIME_SHIFT |
>>>> + idle_frames << EDP_PSR_IDLE_FRAME_SHIFT |
>>>> + EDP_PSR_ENABLE);
>>>> +}
>>>> +
>>>> +void intel_edp_psr_enable(struct intel_dp *intel_dp)
>>>> +{
>>>> + struct drm_device *dev = intel_dp_to_dev(intel_dp);
>>>> +
>>>> + if (!is_edp_psr(intel_dp) || intel_edp_is_psr_enabled(dev))
>>>
>>> One of the nice things about the modeset rework is that, for most of
>>> the display code, we don't call "enable" twice of "disable" twice. On
>>> a brief look at patch 11 it seems we do respect this, so how about you
>>> turn this check for intel_edp_is_psr_enabled into a WARN?
>>
>> I tried to put WARN_ON but started to get warnings... not sure if this
>> is working.
>> Anyway, after I created update_psr, this can be enabled by disabled at
>> any time we need.
>> So I prefer to let it as it is.
>>
>>>
>>>
>>>> + return;
>>>> +
>>>> + /* Enable PSR on the panel */
>>>> + intel_edp_psr_enable_sink(intel_dp);
>>>> +
>>>> + /* Enable PSR on the host */
>>>> + intel_edp_psr_enable_source(intel_dp);
>>>> +}
>>>> +
>>>> +void intel_edp_psr_disable(struct intel_dp *intel_dp)
>>>> +{
>>>> + struct drm_device *dev = intel_dp_to_dev(intel_dp);
>>>> + struct drm_i915_private *dev_priv = dev->dev_private;
>>>> + struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
>>>
>>> Same as above: you could avoid intel_dp_to_dev since you're already
>>> calling dp_to_dig_port.
>>
>> done.
>>
>>>
>>>
>>>> + struct intel_crtc *intel_crtc = to_intel_crtc(dig_port->base.base.crtc);
>>>> + uint32_t val;
>>>> +
>>>> + if (!intel_edp_is_psr_enabled(dev))
>>>> + return;
>>>
>>> Same as above: replace the check above with a WARN?
>>
>> same as above.
>>
>>>
>>>
>>>> +
>>>> + val = I915_READ(EDP_PSR_CTL);
>>>> + I915_WRITE(EDP_PSR_CTL, val & ~EDP_PSR_ENABLE);
>>>> +
>>>> + /* Wait till PSR is idle */
>>>> + if (_wait_for((I915_READ(EDP_PSR_STATUS_CTL) &
>>>> + EDP_PSR_STATUS_STATE_MASK) == 0, 2000, 10))
>>>> + DRM_ERROR("Timed out waiting for PSR Idle State\n");
>>>> +
>>>> + intel_wait_for_vblank(dev, intel_crtc->pipe);
>>>
>>> The spec says we need to wait for a vblank and then disable the VSC
>>> DIP. As stated above, it is not clear whether the hardware does this
>>> automatically or not. Also, do we need this at all? I imagine the spec
>>> says we need to wait for a vblank just to avoid sending incomplete
>>> DIPs, but on this case the intel_wait_for_vblank won't help us here.
>>> Maybe we could fully just enable/disable the bit at mode_set time and
>>> not worry about it later?
>>
>> checked and removed. useless wait.
>
> But now you're never disabling the VSC DIP bit.
>
> Anyway, I re-applied your latest patches and had a few small conflicts
> on many patches. Most of the conflicts were due to changes in your own
> patches. Maybe it should be good to rebase everything and resend...
Uhm dam... I tried to resend the one that I got conflict here but
cleared I missed few..
Next versions I'll resend the full series again to avoid loosing some patch
> By the way, things are still working on my machine :)
Thanks for checking that! ;)
>
>>
>>>
>>>
>>>> +}
>>>> +
>>>> static void intel_disable_dp(struct intel_encoder *encoder)
>>>> {
>>>> struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
>>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>>>> index 9b264ee..ff09c4c 100644
>>>> --- a/drivers/gpu/drm/i915/intel_drv.h
>>>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>>>> @@ -840,4 +840,7 @@ extern bool intel_set_pch_fifo_underrun_reporting(struct drm_device *dev,
>>>> enum transcoder pch_transcoder,
>>>> bool enable);
>>>>
>>>> +extern void intel_edp_psr_enable(struct intel_dp *intel_dp);
>>>> +extern void intel_edp_psr_disable(struct intel_dp *intel_dp);
>>>> +
>>>> #endif /* __INTEL_DRV_H__ */
>>>> --
>>>> 1.8.1.4
>>>>
>>>> _______________________________________________
>>>> Intel-gfx mailing list
>>>> Intel-gfx at lists.freedesktop.org
>>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>>
>>>
>>>
>>> --
>>> Paulo Zanoni
>>
>>
>>
>> --
>> Rodrigo Vivi
>> Blog: http://blog.vivi.eng.br
>
>
>
> --
> Paulo Zanoni
--
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
