drm/i915: disable IPS while getting the sink CRCs
Subsystems (2)
PATCH 1/1 drm/i915: disable IPS while getting the sink CRCs
From: Paulo Zanoni <[email protected]>
This commit is the "sink CRC" version of:
commit 8c740dcea254a1472df2c0ac5ac585412a2507ec
Author: Paulo Zanoni <[email protected]> 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)
--- drivers/gpu/drm/i915/intel_dp.c | 66 ++++++++++++++++++++++++++++------------- 1 file changed, 45 insertions(+), 21 deletions(-)
drivers/gpu/drm/i915/intel_dp.c +45 -21
@@ -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
Signature · 6 lines
2.1.4 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
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:
Quoted 133 lines · Paulo Zanoni
From: Paulo Zanoni <[email protected]>
This commit is the "sink CRC" version of:
commit
8c740dcea254a1472df2c0ac5ac585412a2507ecAuthor: Paulo Zanoni <[email protected]> Date: Fri Oct 17 18:42:03 2014 -0300drm/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
8c740dceatoo). 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 <[email protected]>Signed-off-by Paulo Zanoni <[email protected]>--- drivers/gpu/drm/i915/intel_dp.c | 66 ++++++++++++++++++++++++++++------------- 1 file changed, 45 insertions(+), 21 deletions(-)drivers/gpu/drm/i915/intel_dp.c +45
-22@@ -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
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Signature · 6 lines
Rodrigo Vivi Blog: http://blog.vivi.eng.br _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
2015-05-27 16:35 GMT-03:00 Rodrigo Vivi <[email protected]>:
Quoted 11 lines
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.
Quoted 3 lines
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 <[email protected]>
We do have the sysfs interface:
echo 0 > /sys/module/i915/parameters/enable_ips
I think we should have consistency between both source and sink CRCs. 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?
Quoted 143 lines · Paulo Zanoni
Quoted 129 lines
From: Paulo Zanoni <[email protected]>
This commit is the "sink CRC" version of:
commit
8c740dcea254a1472df2c0ac5ac585412a2507ecAuthor: Paulo Zanoni <[email protected]> Date: Fri Oct 17 18:42:03 2014 -0300drm/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
8c740dceatoo). 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 <[email protected]>Signed-off-by Paulo Zanoni <[email protected]>--- drivers/gpu/drm/i915/intel_dp.c | 66 ++++++++++++++++++++++++++++------------- 1 file changed, 45 insertions(+), 21 deletions(-)drivers/gpu/drm/i915/intel_dp.c +45
-22@@ -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
Quoted 4 lines
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx--
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
Signature · 5 lines
Paulo Zanoni _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoted 12 lines · Paulo Zanoni
2015-05-27 16:35 GMT-03:00 Rodrigo Vivi <[email protected]>:
Quoted 11 lines
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.
Quoted 6 lines
Quoted 3 lines
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 <[email protected]>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.
Quoted 1 line
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...
Quoted 8 lines
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.
Quoted 149 lines
Quoted 134 lines · Paulo Zanoni
Quoted 129 lines
From: Paulo Zanoni <[email protected]>
This commit is the "sink CRC" version of:
commit
8c740dcea254a1472df2c0ac5ac585412a2507ecAuthor: Paulo Zanoni <[email protected]> Date: Fri Oct 17 18:42:03 2014 -0300drm/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
8c740dceatoo). 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 <[email protected]>Signed-off-by Paulo Zanoni <[email protected]>--- drivers/gpu/drm/i915/intel_dp.c | 66 ++++++++++++++++++++++++++++------------- 1 file changed, 45 insertions(+), 21 deletions(-)drivers/gpu/drm/i915/intel_dp.c +45
-22@@ -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
Quoted 10 lines
Quoted 4 lines
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx--
Rodrigo Vivi
Blog: http://blog.vivi.eng.br--
Paulo Zanoni
Signature · 6 lines
Rodrigo Vivi Blog: http://blog.vivi.eng.br _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
2015-05-27 17:06 GMT-03:00 Rodrigo Vivi <[email protected]>:
Quoted 17 lines · Paulo Zanoni
Quoted 16 lines
2015-05-27 16:35 GMT-03:00 Rodrigo Vivi <[email protected]>:
Quoted 14 lines
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 <[email protected]>
Quoted 5 lines
Quoted 2 lines
We do have the sysfs interface:
echo 0 >/sys/module/i915/parameters/enable_ipsI believe the parameter is not enough. It prevents to get enabled next time, but didn't disable ips immediately.
Unset all modes, change parameter, do modeset. This was my first solution, and it worked. I'm also using the same approach for FBC and PSR at kms_frontbuffer_tracking.
Quoted 8 lines
Quoted 2 lines
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...
I agree we should probably debug this, since, somehow, some pixels are different. Any suggestions on how to debug this?
Quoted 5 lines
Quoted 5 lines
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.
Quoted 8 lines
Quoted 4 lines
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?
If the debugfs information shows that PSR is enabled, then it should be enabled, right? My testcase is relying on this.
Quoted 3 lines
We
would never be really testing our PSR SW tracking corner cases with sink CRC.
kms_frontbuffer_tracking checks the pipe CRC, the sink CRC and the debufgs information
Quoted 156 lines
Quoted 148 lines
Quoted 135 lines · Paulo Zanoni
Quoted 129 lines
From: Paulo Zanoni <[email protected]>
This commit is the "sink CRC" version of:
commit
8c740dcea254a1472df2c0ac5ac585412a2507ecAuthor: Paulo Zanoni <[email protected]> Date: Fri Oct 17 18:42:03 2014 -0300drm/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
8c740dceatoo). 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 <[email protected]>Signed-off-by Paulo Zanoni <[email protected]>--- drivers/gpu/drm/i915/intel_dp.c | 66 ++++++++++++++++++++++++++++------------- 1 file changed, 45 insertions(+), 21 deletions(-)drivers/gpu/drm/i915/intel_dp.c +45
-22@@ -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
Quoted 10 lines
Quoted 4 lines
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx--
Rodrigo Vivi
Blog: http://blog.vivi.eng.brQuoted 2 lines
--
Paulo Zanoni--
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
Signature · 5 lines
Paulo Zanoni _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoted 21 lines · Paulo Zanoni
2015-05-27 17:06 GMT-03:00 Rodrigo Vivi <[email protected]>:
Quoted 20 lines · Paulo Zanoni
Quoted 18 lines
2015-05-27 16:35 GMT-03:00 Rodrigo Vivi <[email protected]>:
Quoted 11 lines
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.Quoted 3 lines
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 <[email protected]>We do have the sysfs interface:
echo 0 >/sys/module/i915/parameters/enable_ips
Quoted 6 lines
Quoted 2 lines
I believe the parameter is not enough. It prevents to get enabled next time, but didn't disable ips immediately.
Unset all modes, change parameter, do modeset. This was my first solution, and it worked. I'm also using the same approach for FBC and PSR at
kms_frontbuffer_tracking.
oh indeed... this works on test case... I was thinking about something more generic and immediate to be used anywhere... but this is a another discussion for later ;)
Quoted 4 lines
Quoted 4 lines
Quoted 3 lines
I think we should have consistency between both source and sink CRCs.
Quoted 8 lines
Quoted 5 lines
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...I agree we should probably debug this, since, somehow, some pixels are different. Any suggestions on how to debug this?
no idea, sorry :(
Quoted 11 lines
Quoted 11 lines
Quoted 10 lines
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?
Quoted 6 lines
Quoted 3 lines
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?
If the debugfs information shows that PSR is enabled, then it should be enabled, right? My testcase is relying on this.
Yes, it is. But the HW and frontbuffer tracking can be triggering PSR exit that forces RFB update.
Our testcases intend to cover all cases to see if our frontbuffer tracking is working correctly, but what if IPS disable makes HW trigger PSR exit by itself. You would never be testing the frontbuffer tracking actually...
Quoted 169 lines
Quoted 4 lines
We
would never be really testing our PSR SW tracking corner cases with sink CRC.
kms_frontbuffer_trackingchecks the pipe CRC, the sink CRC and the debufgs informationQuoted 148 lines
Quoted 148 lines
Quoted 145 lines · Paulo Zanoni
Quoted 129 lines
From: Paulo Zanoni <[email protected]>
This commit is the "sink CRC" version of:
commit
8c740dcea254a1472df2c0ac5ac585412a2507ecAuthor: Paulo Zanoni <[email protected]> Date: Fri Oct 17 18:42:03 2014 -0300drm/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
8c740dceatoo). 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 <[email protected]>Signed-off-by Paulo Zanoni <[email protected]>--- drivers/gpu/drm/i915/intel_dp.c | 66 ++++++++++++++++++++++++++++------------- 1 file changed, 45 insertions(+), 21 deletions(-)drivers/gpu/drm/i915/intel_dp.c +45
-22@@ -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
Quoted 4 lines
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx--
Rodrigo Vivi
Blog: http://blog.vivi.eng.brQuoted 8 lines
Quoted 2 lines
--
Paulo Zanoni--
Rodrigo Vivi
Blog: http://blog.vivi.eng.br--
Paulo Zanoni
Signature · 6 lines
Rodrigo Vivi Blog: http://blog.vivi.eng.br _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Daniel Vetter
AppliedQuoted 4 lines · Paulo Zanoni
2015-05-27 16:35 GMT-03:00 Rodrigo Vivi <[email protected]>:
Quoted 3 lines
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.
Bummer, I was hopeful this would help. I guess we need to dig more and need to try to figure out what more is broken here. Maybe some panels just have broken CRCs?
Quoted 19 lines
Quoted 10 lines
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 <[email protected]>We do have the sysfs interface:
echo 0 >/sys/module/i915/parameters/enable_ipsI think we should have consistency between both source and sink CRCs. 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.
Quoted 4 lines
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 agree with Paulo that the kernel solution seems to be the best. We already have similar tricks on some platforms for crcs where we e.g. need to put the scrambler into a non-compliant mode or switch to a different output routing/power well config for edp.
If this kind of magic ever becomes a problem then we can look into adding more raw crc interfaces. But I honestly don't expect this to happen - we want to make sure that the right picture gets composited, validating individual parts of the hw pipeline (like whether ips or a different routing or whatever works correctly) is a job for SV, not us.
Queued for -next, thanks for the patch.
-Daniel
Quoted 153 lines
Quoted 144 lines · Paulo Zanoni
Quoted 129 lines
From: Paulo Zanoni <[email protected]>
This commit is the "sink CRC" version of:
commit
8c740dcea254a1472df2c0ac5ac585412a2507ecAuthor: Paulo Zanoni <[email protected]> Date: Fri Oct 17 18:42:03 2014 -0300drm/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
8c740dceatoo). 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 <[email protected]>Signed-off-by Paulo Zanoni <[email protected]>--- drivers/gpu/drm/i915/intel_dp.c | 66 ++++++++++++++++++++++++++++------------- 1 file changed, 45 insertions(+), 21 deletions(-)drivers/gpu/drm/i915/intel_dp.c +45
-22@@ -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
Quoted 4 lines
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx--
Rodrigo Vivi
Blog: http://blog.vivi.eng.brSignature · 5 lines
Paulo Zanoni _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Signature · 7 lines
Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx