[Intel-gfx] [PATCH] drm/i915: disable IPS while getting the sink CRCs
Rodrigo Vivi
rodrigo.vivi at gmail.com
Wed May 27 13:06:23 PDT 2015
On Wed, May 27, 2015 at 12:42 PM, Paulo Zanoni <przanoni at gmail.com> wrote:
> 2015-05-27 16:35 GMT-03:00 Rodrigo Vivi <rodrigo.vivi at gmail.com>:
>> I didn't face this since I was letting IPS disabled on my tests here
>> and the platforms that I'm mostly concerned about sink CRC not working
>> well are the ones that doesn't have IPS.
>>
>> I'm not sure this is the right way to solve this issue. Maybe we want
>> to know the sink CRC when IPS is on.
>> I believe the right approach is to provide a sysfs interface to toggle
>> ips off so testcase can disabled ips to not get impacted by it when
>> reading sink CRC. and than, enable it back if it was enabled after
>> test is concluded.
>>
>> But anyway, this is a simple solution that we have right now and we
>> can change it once we have the sysfs interface, so:
>> Reviewed-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
>
> We do have the sysfs interface:
> echo 0 > /sys/module/i915/parameters/enable_ips
I believe the parameter is not enough. It prevents to get enabled next
time, but didn't disable ips immediately.
> I think we should have consistency between both source and sink CRCs.
I'm not sure actually... We do need consistency of what is going
through the pipe and being showed, but not necessarily on the
usability.
If IPS is impacting sink CRC it is actually impacting what is being
displayed and we might be interested to know the differences...
> So if we decide to go for the sysfs solution we need to revert the
> "disable IPS while getting the source CRCs" patch and then fix IGT so
> it always disables IPS for all CRC tests.
>
> I prefer the Kernel solution since IMHO it is much simpler and makes
> sure IGT is always correct, but if we decide to go for the sysfs
> solution I can write the patches. I guess a third (and fourth and
> fifth) opinion from someone else here would help. Anybody?
I was going to write here that I was ok with the kernel solution while
we aren't actually interested on this ips usecase with sink CRC, but
what if this disable ips force HW to exit PSR with HW tracking? We
would never be really testing our PSR SW tracking corner cases with
sink CRC.
>
>>
>>
>>
>> On Mon, May 25, 2015 at 2:52 PM, Paulo Zanoni <przanoni at gmail.com> wrote:
>>> From: Paulo Zanoni <paulo.r.zanoni at intel.com>
>>>
>>> This commit is the "sink CRC" version of:
>>>
>>> commit 8c740dcea254a1472df2c0ac5ac585412a2507ec
>>> Author: Paulo Zanoni <paulo.r.zanoni at intel.com>
>>> Date: Fri Oct 17 18:42:03 2014 -0300
>>> drm/i915: disable IPS while getting the pipe CRCs.
>>>
>>> For some unknown reason, when IPS gets enabled, the sink CRC changes.
>>> Since hsw_enable_ips() doesn't really guarantee to enable IPS (it
>>> depends on package C-states), we can't really predict if IPS is
>>> enabled or disabled while running our CRC tests, so let's just
>>> completely disable IPS while sink CRCs are being used.
>>>
>>> If we find a way to make IPS not change the pipe CRC result, we may
>>> want to fix IPS and then revert this patch (and 8c740dcea too). While
>>> this doesn't happen, let's merge this patch, so the IGT tests relying
>>> on sink CRCs can work properly.
>>>
>>> This was discovered while developing a new IGT test, which will
>>> probably be called kms_frontbuffer_tracking.
>>>
>>> Testcase: igt/kms_frontbuffer_tracking (not on upstream IGT yet)
>>> Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
>>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
>>> ---
>>> drivers/gpu/drm/i915/intel_dp.c | 66 ++++++++++++++++++++++++++++-------------
>>> 1 file changed, 45 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>>> index abd442a..62662de 100644
>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>> @@ -4041,46 +4041,70 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
>>> u8 buf;
>>> int test_crc_count;
>>> int attempts = 6;
>>> + int ret = 0;
>>>
>>> - if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK_MISC, &buf) < 0)
>>> - return -EIO;
>>> + hsw_disable_ips(intel_crtc);
>>>
>>> - if (!(buf & DP_TEST_CRC_SUPPORTED))
>>> - return -ENOTTY;
>>> + if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK_MISC, &buf) < 0) {
>>> + ret = -EIO;
>>> + goto out;
>>> + }
>>> +
>>> + if (!(buf & DP_TEST_CRC_SUPPORTED)) {
>>> + ret = -ENOTTY;
>>> + goto out;
>>> + }
>>>
>>> - if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) < 0)
>>> - return -EIO;
>>> + if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) < 0) {
>>> + ret = -EIO;
>>> + goto out;
>>> + }
>>>
>>> if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK,
>>> - buf | DP_TEST_SINK_START) < 0)
>>> - return -EIO;
>>> + buf | DP_TEST_SINK_START) < 0) {
>>> + ret = -EIO;
>>> + goto out;
>>> + }
>>> +
>>> + if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK_MISC, &buf) < 0) {
>>> + ret = -EIO;
>>> + goto out;
>>> + }
>>>
>>> - if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK_MISC, &buf) < 0)
>>> - return -EIO;
>>> test_crc_count = buf & DP_TEST_COUNT_MASK;
>>>
>>> do {
>>> if (drm_dp_dpcd_readb(&intel_dp->aux,
>>> - DP_TEST_SINK_MISC, &buf) < 0)
>>> - return -EIO;
>>> + DP_TEST_SINK_MISC, &buf) < 0) {
>>> + ret = -EIO;
>>> + goto out;
>>> + }
>>> intel_wait_for_vblank(dev, intel_crtc->pipe);
>>> } while (--attempts && (buf & DP_TEST_COUNT_MASK) == test_crc_count);
>>>
>>> if (attempts == 0) {
>>> DRM_DEBUG_KMS("Panel is unable to calculate CRC after 6 vblanks\n");
>>> - return -ETIMEDOUT;
>>> + ret = -ETIMEDOUT;
>>> + goto out;
>>> }
>>>
>>> - if (drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_CRC_R_CR, crc, 6) < 0)
>>> - return -EIO;
>>> + if (drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_CRC_R_CR, crc, 6) < 0) {
>>> + ret = -EIO;
>>> + goto out;
>>> + }
>>>
>>> - if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) < 0)
>>> - return -EIO;
>>> + if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) < 0) {
>>> + ret = -EIO;
>>> + goto out;
>>> + }
>>> if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK,
>>> - buf & ~DP_TEST_SINK_START) < 0)
>>> - return -EIO;
>>> -
>>> - return 0;
>>> + buf & ~DP_TEST_SINK_START) < 0) {
>>> + ret = -EIO;
>>> + goto out;
>>> + }
>>> +out:
>>> + hsw_enable_ips(intel_crtc);
>>> + return ret;
>>> }
>>>
>>> static bool
>>> --
>>> 2.1.4
>>>
>>> _______________________________________________
>>> Intel-gfx mailing list
>>> Intel-gfx at lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>
>>
>>
>> --
>> Rodrigo Vivi
>> Blog: http://blog.vivi.eng.br
>
>
>
> --
> Paulo Zanoni
--
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
More information about the Intel-gfx
mailing list