Re: [Intel-gfx] [PATCH] drm/edid: Reduce horizontal timings for pixel replicated modes
On Thu, 04 Sep 2014, Daniel Vetter wrote: > More important is to Cc: all the people (in the commit message to make > sure it doesn't get lost, git send-email then takes care of > everything) who commented on that patch so that they'll see your > updated version. git send-email taking care of everything is subject to sendemail.suppresscc and other configuration values. With the git defaults, you need to be careful not to accidentally Cc people, typically when sending a patch privately/internally. BR, Jani. -- Jani Nikula, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 02/10] drm/i915: Decouple the stuck pageflip on modeset
On Thu, Sep 04, 2014 at 08:15:32PM +0300, Ville Syrjälä wrote: > On Tue, Sep 02, 2014 at 02:57:37PM +0100, Chris Wilson wrote: > > If we successfully confuse the hardware, and cause it to drop a queued > > pageflip, we wait for 60s and issue a warning before continuing on with > > the modeset. However, this leaves the pending pageflip still stuck > > indefinitely. Pretend to userspace that it does complete, and let us > > start afresh following the modeset. > > > > v2: Rebase after refactor This applied cleanly despite that I don't have that refactor. Should I be concerned? > > Signed-off-by: Chris Wilson > > Cc: Daniel Vetter > > Cc: Ville Syrjälä > > Signed-off-by: Daniel Vetter > > Yeah, seems better than having the flip stuck forever. > > Reviewed-by: Ville Syrjälä Queued for -next, thanks for the patch. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] Kick stuck flips before modeset
On Fri, Sep 05, 2014 at 07:13:23AM +0100, Chris Wilson wrote: > Hi Daniel, > > these are the first two patches in the flip boosting series pushed ahead > of the request rework. These are the interesting ones as they help paper > over oddities in our hardware and prevent the driver from wedging itself > with a stuck modeset. Ah, should read my entire inbox first next time around ;-) Thanks for the rebase, both applied to dinq. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 02/10] drm/i915: Decouple the stuck pageflip on modeset
On Fri, Sep 05, 2014 at 09:27:16AM +0200, Daniel Vetter wrote: > On Thu, Sep 04, 2014 at 08:15:32PM +0300, Ville Syrjälä wrote: > > On Tue, Sep 02, 2014 at 02:57:37PM +0100, Chris Wilson wrote: > > > If we successfully confuse the hardware, and cause it to drop a queued > > > pageflip, we wait for 60s and issue a warning before continuing on with > > > the modeset. However, this leaves the pending pageflip still stuck > > > indefinitely. Pretend to userspace that it does complete, and let us > > > start afresh following the modeset. > > > > > > v2: Rebase after refactor > > This applied cleanly despite that I don't have that refactor. Should I be > concerned? No, the refactoring here was about the pageflip checking itself in patch 1. I just kept moving these two in lockstep. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/5] tests/kms_psr_sink_crc: moving usleep around.
On Thu, Sep 04, 2014 at 06:22:47PM -0400, Rodrigo Vivi wrote:
> Lets just do this small sleep to allow human eyes to see what is happening,
> but let's avoid any kind of interference on the actual test.
>
> Signed-off-by: Rodrigo Vivi
> ---
> tests/kms_psr_sink_crc.c | 6 +-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/tests/kms_psr_sink_crc.c b/tests/kms_psr_sink_crc.c
> index 0917a7f..bbe18f0 100644
> --- a/tests/kms_psr_sink_crc.c
> +++ b/tests/kms_psr_sink_crc.c
> @@ -327,6 +327,11 @@ static void get_sink_crc(data_t *data, char *crc) {
> igt_require(ret > 0);
>
> fclose(file);
> +
> + /* The important value was already taken.
> + * Now give a time for human eyes
> + */
> + usleep(30);
We have igt_wait_for_keypress already, which is unfortunately not terribly
userfriendly (since the env-var is undocumented). Maybe we should apply a
bit of polish there. Have you considered using that one? Just fyi really.
-Daniel
> }
>
> static void test_crc(data_t *data)
> @@ -342,7 +347,6 @@ static void test_crc(data_t *data)
>1, 1) == 0);
> }
>
> - usleep(30);
> igt_assert(wait_psr_entry(data, 10));
> get_sink_crc(data, ref_crc);
>
> --
> 1.9.3
>
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/5] igt/kms_psr_sink_crc: Add debug for CRC
On Thu, Sep 04, 2014 at 06:22:49PM -0400, Rodrigo Vivi wrote:
> This helps to debug and check if the CRC you are reading correspond
> what you are seeing on the screen
>
>
> Signed-off-by: Rodrigo Vivi
> ---
> tests/kms_psr_sink_crc.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/tests/kms_psr_sink_crc.c b/tests/kms_psr_sink_crc.c
> index bbe18f0..51e54a7 100644
> --- a/tests/kms_psr_sink_crc.c
> +++ b/tests/kms_psr_sink_crc.c
> @@ -328,6 +328,8 @@ static void get_sink_crc(data_t *data, char *crc) {
>
> fclose(file);
>
> + igt_debug_wait(1, "%s\n", crc);
I think igt_wait_keypress here is the better option. Together with
igt_check_boolean_env_var it should be fairly pretty. I'll send out a
quick prep patch for this.
-Daniel
> +
> /* The important value was already taken.
>* Now give a time for human eyes
>*/
> --
> 1.9.3
>
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/edid: Reduce horizontal timings for pixel replicated modes
On Thu, Sep 04, 2014 at 08:31:07PM +0200, Daniel Vetter wrote: > Readding intel-gfx since this is interesting for everyone. > > On Thu, Sep 4, 2014 at 8:09 PM, Clint Taylor > wrote: > > I will attempt to improve my patch submissions process. I also need to start > > annotating version in the subject field as well. > > > > When using --in_reply_to= do I just use the Message-ID: from the original > > patch message header or do I use the Message-ID: from the reply asking for > > the change? > > Usually I reply to the previous patch version to avoid threads nesting > too badly, but other people reply to the last review comment. It > doesn't really matter. I tend to do the latter, but I must admit the threads sometimes get rather messy, so I think I'll change my ways. The changelog in the patch should anyway keep the story together. -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] lib/igt_aux: Improve wait_for_keypress helper a bit
- Use keys in just one env variable to enable/disable it.
- Add an informational message so that the users knows when to press
the key (more useful over ssh than when run on the terminal ofc).
- Improve the documentation so that it's clearer how to use this
when running tests.
Cc: Rodrigo Vivi
Cc: Damien Lespiau
Signed-off-by: Daniel Vetter
---
lib/igt_aux.c | 26 +-
lib/igt_aux.h | 2 +-
lib/igt_kms.c | 3 +--
3 files changed, 23 insertions(+), 8 deletions(-)
diff --git a/lib/igt_aux.c b/lib/igt_aux.c
index 5ddc8b610895..05cb4bd1a533 100644
--- a/lib/igt_aux.c
+++ b/lib/igt_aux.c
@@ -344,21 +344,37 @@ void igt_drop_root(void)
}
/**
- * igt_wait_for_keypress:
+ * igt_debug_wait_for_keypress:
+ * @key: env var lookup to to enable this wait
*
- * Waits for a key press when run interactively. When not connected to a
- * terminal immediately continues.
+ * Waits for a key press when run interactively and when the corresponding
debug
+ * key is set in the IGT_DEBUG_INTERACTIVE environment variable. Multiple keys
+ * can be specified as a comma-separated list or alternatively "all" if a wait
+ * should happen for all keys. When not connected to a terminal the enviroment
+ * setting is ignored and execution immediately continues.
*
* This is useful for display tests where under certain situation manual
- * inspection of the display is useful.
+ * inspection of the display is useful. Or when running a testcase in the
+ * background.
*/
-void igt_wait_for_keypress(void)
+void igt_debug_wait_for_keypress(const char *key)
{
struct termios oldt, newt;
+ const char *env;
if (!isatty(STDIN_FILENO))
return;
+ env = getenv("IGT_DEBUG_INTERACTIVE");
+
+ if (!env)
+ return;
+
+ if (!strstr(env, key) && !strstr(env, "all"))
+ return;
+
+ igt_info("Press any key to continue ...\n");
+
tcgetattr ( STDIN_FILENO, &oldt );
newt = oldt;
newt.c_lflag &= ~( ICANON | ECHO );
diff --git a/lib/igt_aux.h b/lib/igt_aux.h
index a90d8d9e6d95..d958abeb0e84 100644
--- a/lib/igt_aux.h
+++ b/lib/igt_aux.h
@@ -56,7 +56,7 @@ void igt_system_suspend_autoresume(void);
/* dropping priviledges */
void igt_drop_root(void);
-void igt_wait_for_keypress(void);
+void igt_debug_wait_for_keypress(const char *key);
enum igt_runtime_pm_status {
IGT_RUNTIME_PM_STATUS_ACTIVE,
diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index d763013cf15f..f483e2daf755 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -1401,8 +1401,7 @@ static int do_display_commit(igt_display_t *display,
LOG_UNINDENT(display);
- if (getenv("IGT_DISPLAY_WAIT_AT_COMMIT"))
- igt_wait_for_keypress();
+ igt_debug_wait_for_keypress("modeset");
return 0;
}
--
1.9.3
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 5/5] tests/kms_psr_sink_crc: Dry run with PSR disabled.
On Thu, Sep 04, 2014 at 06:22:51PM -0400, Rodrigo Vivi wrote:
> This allows to run tests with psr disabled and know what to expect when
> PSR is actually enabled.
>
> Signed-off-by: Rodrigo Vivi
I don't really follow what this is useful for ... Can you please elaborate
how this is used and how it helps debugging?
-Daniel
> ---
> tests/kms_psr_sink_crc.c | 8
> 1 file changed, 8 insertions(+)
>
> diff --git a/tests/kms_psr_sink_crc.c b/tests/kms_psr_sink_crc.c
> index 51e54a7..1380ca4 100644
> --- a/tests/kms_psr_sink_crc.c
> +++ b/tests/kms_psr_sink_crc.c
> @@ -72,6 +72,7 @@ typedef struct {
> igt_display_t display;
> struct igt_fb fb[2];
> igt_plane_t *plane[2];
> + bool running_with_psr_disabled;
> } data_t;
>
> static const char *tests_str(enum tests test)
> @@ -264,6 +265,9 @@ static bool psr_enabled(data_t *data)
> FILE *file;
> char str[4];
>
> + if (data->running_with_psr_disabled)
> + return true;
> +
> file = igt_debugfs_fopen("i915_edp_psr_status", "r");
> igt_require(file);
>
> @@ -284,6 +288,9 @@ static bool psr_active(data_t *data)
> FILE *file;
> char str[4];
>
> + if (data->running_with_psr_disabled)
> + return true;
> +
> file = igt_debugfs_fopen("i915_edp_psr_status", "r");
> igt_require(file);
>
> @@ -604,6 +611,7 @@ igt_main
> kmstest_set_vt_graphics_mode();
>
> data.devid = intel_get_drm_devid(data.drm_fd);
> + data.running_with_psr_disabled = igt_dry_run;
>
> igt_skip_on(!psr_enabled(&data));
>
> --
> 1.9.3
>
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 02/14] drm/i915: Reorganize vlv eDP reboot notifier
On Thu, Sep 04, 2014 at 10:47:41AM -0700, Clint Taylor wrote: > On 08/26/2014 07:06 AM, Ville Syrjälä wrote: > > On Tue, Aug 26, 2014 at 03:36:07PM +0200, Daniel Vetter wrote: > >> On Tue, Aug 26, 2014 at 04:21:00PM +0300, Jani Nikula wrote: > >>> On Tue, 26 Aug 2014, Ville Syrjälä wrote: > On Tue, Aug 19, 2014 at 10:00:55AM +0300, Jani Nikula wrote: > > On Mon, 18 Aug 2014, ville.syrj...@linux.intel.com wrote: > >> From: Ville Syrjälä > >> > >> Move the vlv_power_sequencer_pipe() after the IS_VALLEYVIEW() check > >> and flatten the rest of the function. > > > > Please imagine adding another platform there, and realize this just adds > > unnecessary churn. > > I'd just add another reboot notifier then. > >>> > >>> Fair enough; it should be vlv_edp_notify_handler then. (No, don't send a > >>> patch to change that! ;) > >>> > Frankly I don't understand the current one either. Why does it need to > set the delay to max for instance? And does this mean that the > PANEL_POWER_RESET bit doesn't actually work as advertised in the docs? > >>> > >>> *shrug* experimental evidence? > >>> > >>> commit 01527b3127997ef6370d5ad4fa25d96847fbf12a > >>> Author: Clint Taylor > >>> Date: Mon Jul 7 13:01:46 2014 -0700 > >>> > >>> drm/i915/vlv: T12 eDP panel timing enforcement during reboot > >>> > >>> The panel power sequencer on vlv doesn't appear to accept changes to > >>> its > >>> T12 power down duration during warm reboots. This change forces a > >>> delay > >>> for warm reboots to the T12 panel timing as defined in the VBT table > >>> for > >>> the connected panel. > >> > >> So if I remember this piece of lore correctly in the past the pp was > >> pessimistic, and enforced this delay on resume/boot-up, assuming you've > >> shut down _right_ before the machine was lit up again. Apparently people > >> where unhappy with that enforced delay and it was ditched on vlv, but then > >> it broke panels if you actually managed to reboot quickly enough. > > > > IIRC the way the reset bit is documented the hardware itself is supposed > > to initiate the power off cycle when it gets some reset notification and > > it should enforce the timing before allowing the panel power to be > > re-enabled. Although it does seem that it would also reset the > > "power cycle delay" so it would maybe only enforce some default delay in > > that case (300ms based on the documented default value of 0x4). So if the > > panel requires more than the 300ms then I understand the msleep() here. > > I guess use of the VDD force bit just after reset might also require that > > we do the power down + wait before reset. So that part does make sense > > to me, but I still don't understand the "power cycle delay"=0x1f part. > > > All eDP panels require at least 500ms according to the eDP > specifications T12 minimum of 500ms. The panel manufacturer's > specifications I have seen also have a 500ms minimum. Is the default > register value of 4 relevant for LVDS? IIRC I saw 400ms mentioned for LVDS somewhere, so I have no idea why someone made the default 300ms. Not that I actaully verified that the reset valus is indeed 4 on actual hardware, I just read it from the spec. > From our testing on VLV the PPS starts immediately upon ungate of the > display block. There is no way to stop or alter this sequence that > starts with the default value (4) in the register. The panel power will > assert at the end of the sequence. Without the msleep() adding time a > quick rebooting platform will not meet the 500ms minimum. Ok so I guess in theory we could do msleep(whatever-300) and we should still meet the panel timings, assuming we care to optimize a few hunder ms here. > > "power cycle delay" of 0x1f was added to prevent LCD_VDD from asserting > before the reboot actually completes which includes the msleep() time. > The "power cycle delay" value could be computed based on the T12 time in > the VBT. Just setting the highest value made sense for simplicity and > the fact the value is reset to default during the reboot. At that point we should have the correct t11_t12 delay programmed into the register already. Also it shouldn't really matter what we have in there because of the msleep(). By the time the msleep() is done and we reboot, the register gets reset to defaul anyway, and it should be OK to enable VDD again, even immediately rather than after the default 300ms since we already slept for the entire power cycle delay before rebooting. > The "PANEL_POWER_RESET bit doesn't actually work as advertised" > statement was probably in error based on my understanding of the PPS at > the time the patch was made. OK, so I guess it sort of works then, except since it resets the power cycle delay to 300ms it waits for less than it should. So yeah seems we do need the manual sleep to make sure we don't violate the panel timings. I guess we should then add the reboot
Re: [Intel-gfx] [PATCH] lib/igt_aux: Improve wait_for_keypress helper a bit
On Fri, Sep 05, 2014 at 08:52:31AM +0200, Daniel Vetter wrote:
> - Use keys in just one env variable to enable/disable it.
> - Add an informational message so that the users knows when to press
> the key (more useful over ssh than when run on the terminal ofc).
> - Improve the documentation so that it's clearer how to use this
> when running tests.
>
> Cc: Rodrigo Vivi
> Cc: Damien Lespiau
> Signed-off-by: Daniel Vetter
Looks good to me.
Acked-by: Damien Lespiau
--
Damien
> ---
> lib/igt_aux.c | 26 +-
> lib/igt_aux.h | 2 +-
> lib/igt_kms.c | 3 +--
> 3 files changed, 23 insertions(+), 8 deletions(-)
>
> diff --git a/lib/igt_aux.c b/lib/igt_aux.c
> index 5ddc8b610895..05cb4bd1a533 100644
> --- a/lib/igt_aux.c
> +++ b/lib/igt_aux.c
> @@ -344,21 +344,37 @@ void igt_drop_root(void)
> }
>
> /**
> - * igt_wait_for_keypress:
> + * igt_debug_wait_for_keypress:
> + * @key: env var lookup to to enable this wait
> *
> - * Waits for a key press when run interactively. When not connected to a
> - * terminal immediately continues.
> + * Waits for a key press when run interactively and when the corresponding
> debug
> + * key is set in the IGT_DEBUG_INTERACTIVE environment variable. Multiple
> keys
> + * can be specified as a comma-separated list or alternatively "all" if a
> wait
> + * should happen for all keys. When not connected to a terminal the
> enviroment
> + * setting is ignored and execution immediately continues.
> *
> * This is useful for display tests where under certain situation manual
> - * inspection of the display is useful.
> + * inspection of the display is useful. Or when running a testcase in the
> + * background.
> */
> -void igt_wait_for_keypress(void)
> +void igt_debug_wait_for_keypress(const char *key)
> {
> struct termios oldt, newt;
> + const char *env;
>
> if (!isatty(STDIN_FILENO))
> return;
>
> + env = getenv("IGT_DEBUG_INTERACTIVE");
> +
> + if (!env)
> + return;
> +
> + if (!strstr(env, key) && !strstr(env, "all"))
> + return;
> +
> + igt_info("Press any key to continue ...\n");
> +
> tcgetattr ( STDIN_FILENO, &oldt );
> newt = oldt;
> newt.c_lflag &= ~( ICANON | ECHO );
> diff --git a/lib/igt_aux.h b/lib/igt_aux.h
> index a90d8d9e6d95..d958abeb0e84 100644
> --- a/lib/igt_aux.h
> +++ b/lib/igt_aux.h
> @@ -56,7 +56,7 @@ void igt_system_suspend_autoresume(void);
> /* dropping priviledges */
> void igt_drop_root(void);
>
> -void igt_wait_for_keypress(void);
> +void igt_debug_wait_for_keypress(const char *key);
>
> enum igt_runtime_pm_status {
> IGT_RUNTIME_PM_STATUS_ACTIVE,
> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> index d763013cf15f..f483e2daf755 100644
> --- a/lib/igt_kms.c
> +++ b/lib/igt_kms.c
> @@ -1401,8 +1401,7 @@ static int do_display_commit(igt_display_t *display,
>
> LOG_UNINDENT(display);
>
> - if (getenv("IGT_DISPLAY_WAIT_AT_COMMIT"))
> - igt_wait_for_keypress();
> + igt_debug_wait_for_keypress("modeset");
>
> return 0;
> }
> --
> 1.9.3
>
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 43/89] drm/i915/skl: Read the Memory Latency Values for WM computation
On Thu, Sep 04, 2014 at 12:27:09PM +0100, Damien Lespiau wrote:
> From: Pradeep Bhat
>
> This patch reads the memory latency values for all the 8 levels for
> SKL. These values are needed for the Watermark computation.
>
> v2: Incorporated the review comments from Damien on register
> indentation.
>
> v3: Updated the code to use the sandybridge_pcode_read for reading
> memory latencies for GEN9.
>
> v4: Don't put gen 9 in the middle of an ordered list of ifs
> (Damien)
>
> v5 take the rps.hw_lock around sandybridge_pcode_read() (Damien)
>
> Signed-off-by: Pradeep Bhat
> Signed-off-by: Damien Lespiau
> ---
> drivers/gpu/drm/i915/i915_drv.h | 6
> drivers/gpu/drm/i915/i915_reg.h | 9 +
> drivers/gpu/drm/i915/intel_pm.c | 73
> +
> 3 files changed, 82 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index dcd1c72..32be299 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1665,6 +1665,12 @@ struct drm_i915_private {
> uint16_t spr_latency[5];
> /* cursor */
> uint16_t cur_latency[5];
> + /*
> + * Raw watermark memory latency values
> + * for SKL for all 8 levels
> + * in 1us units.
> + */
> + uint16_t skl_latency[8];
Not sure if we could unify these somehow to avoid wasted space. But that
can be left as a future exercise.
>
> /* current hardware state */
> struct ilk_wm_values hw;
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 0159f2d..bc55990 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -2018,6 +2018,15 @@ enum punit_power_well {
> #define MAD_DIMM_A_SIZE_SHIFT 0
> #define MAD_DIMM_A_SIZE_MASK (0xff << MAD_DIMM_A_SIZE_SHIFT)
>
> +/* SKL GT Driver Mailbox registers for reading memory latencies */
> +#define GEN9_MAILBOX_DATA1 0x13812C
> +#define GEN9_MAILBOX_READ_MEM_LAT (0x6)
> +#define GEN9_MAILBOX_READ_TIMEOUT 150
Timeout not used anywhere. Also spec says 100us.
> +#define GEN9_MEM_LAT_LEVEL_MASK0xFF
> +#define GEN9_MEM_LAT_LEVEL_1_5_SHIFT 8
> +#define GEN9_MEM_LAT_LEVEL_2_6_SHIFT 16
> +#define GEN9_MEM_LAT_LEVEL_3_7_SHIFT 24
This stuff should be grouped along the other pcode register defines.
Also according to Bspec the mailbox data1 register already existed since
snb. The hsw cdclk change sequence also mentions that it should be set
to 0, but eg. the bdw IPS sequence doesn't mention it. I guess in theory
some pcode command might cause it to be clobbered, so I'm thinking we
should just explicitly set it to 0 for all platforms in the pcode
read/write functions. That should be a separate patch though.
> +
> /* snb MCH registers for priority tuning */
> #define MCH_SSKPD(MCHBAR_MIRROR_BASE_SNB + 0x5d10)
> #define MCH_SSKPD_WM0_MASK 0x3f
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index a236e77..d8c8531 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2239,11 +2239,53 @@ hsw_compute_linetime_wm(struct drm_device *dev,
> struct drm_crtc *crtc)
> PIPE_WM_LINETIME_TIME(linetime);
> }
>
> -static void intel_read_wm_latency(struct drm_device *dev, uint16_t wm[5])
> +static void intel_read_wm_latency(struct drm_device *dev, uint16_t wm[8])
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
>
> - if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
> + if (IS_GEN9(dev)) {
> + uint32_t val;
> + int ret;
> +
> + /* read the first set of memory latencies[0:3] */
> + val = 0; /* data0 to be programmed to 0 for first set */
> + mutex_lock(&dev_priv->rps.hw_lock);
> + ret = sandybridge_pcode_read(dev_priv,
> + GEN9_MAILBOX_READ_MEM_LAT,
> + &val);
> + mutex_unlock(&dev_priv->rps.hw_lock);
> +
> + if (ret) {
> + DRM_ERROR("SKL Mailbox read error = %d\n", ret);
> + return;
> + }
> + wm[0] = val & GEN9_MEM_LAT_LEVEL_MASK;
> + wm[1] = (val >> GEN9_MEM_LAT_LEVEL_1_5_SHIFT) &
> + GEN9_MEM_LAT_LEVEL_MASK;
> + wm[2] = (val >> GEN9_MEM_LAT_LEVEL_2_6_SHIFT) &
> + GEN9_MEM_LAT_LEVEL_MASK;
> + wm[3] = (val >> GEN9_MEM_LAT_LEVEL_3_7_SHIFT) &
> + GEN9_MEM_LAT_LEVEL_MASK;
> +
> + /* read the second set of memory latencies[4:7] */
> + val = 1; /* data0 to be programmed to 1 for second set */
> + mutex_
Re: [Intel-gfx] [PATCH 43/89] drm/i915/skl: Read the Memory Latency Values for WM computation
On Fri, Sep 05, 2014 at 11:25:30AM +0300, Ville Syrjälä wrote: > > +/* SKL GT Driver Mailbox registers for reading memory latencies */ > > +#define GEN9_MAILBOX_DATA1 0x13812C > > +#define GEN9_MAILBOX_READ_MEM_LAT(0x6) > > +#define GEN9_MAILBOX_READ_TIMEOUT150 > > Timeout not used anywhere. Also spec says 100us. > > > +#define GEN9_MEM_LAT_LEVEL_MASK 0xFF > > +#define GEN9_MEM_LAT_LEVEL_1_5_SHIFT 8 > > +#define GEN9_MEM_LAT_LEVEL_2_6_SHIFT 16 > > +#define GEN9_MEM_LAT_LEVEL_3_7_SHIFT 24 > > This stuff should be grouped along the other pcode register defines. Funny you mention this, I fixed those two in the v6 sent as reply yesterday. -- Damien ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 09/12] tests/kms_psr_sink_crc: Fix all testcases.
On Thu, Sep 04, 2014 at 05:55:24PM -0700, Rodrigo Vivi wrote:
> adding suspend_autoresume on primary tests like this:
> @ -470,6 +472,8 @@ igt_main
> data.test_plane = PRIMARY;
> data.op = op;
> run_test(&data);
> + igt_system_suspend_autoresume();
> + run_test(&data);
>
> on BDW I got these results:
>
>
> vivijim rdvivi-seattle tests$ sudo ./kms_psr_sink_crc
> IGT-Version: 1.7-gd4b43f0 (x86_64) (Linux: 3.17.0-rc2+ x86_64)
> rtcwake: wakeup from "mem" using /dev/rtc0 at Fri Sep 5 00:44:03 2014
> Subtest primary_page_flip: SUCCESS
> rtcwake: wakeup from "mem" using /dev/rtc0 at Fri Sep 5 00:44:40 2014
> Subtest primary_mmap_gtt: SUCCESS
> Waiting 10s...
> rtcwake: wakeup from "mem" using /dev/rtc0 at Fri Sep 5 00:45:27 2014
> Waiting 10s...
> Subtest primary_mmap_gtt_waiting: SUCCESS
> rtcwake: wakeup from "mem" using /dev/rtc0 at Fri Sep 5 00:46:13 2014
> Subtest primary_mmap_cpu: SUCCESS
> rtcwake: wakeup from "mem" using /dev/rtc0 at Fri Sep 5 00:46:50 2014
> Subtest primary_blt: SUCCESS
> rtcwake: wakeup from "mem" using /dev/rtc0 at Fri Sep 5 00:47:27 2014
> Subtest primary_render: SUCCESS
>
> on HSW I couldn't test because suspend/resume breaks even with psr disabled.
> I'm going to check more tomorrow..
>
> But regarding the suspend resume test, how do you suggest to organize it?
> Extra loops for all current cases?
> suspend_{primary, sprite, cursor}_{page_flip, mmap_gtt, etc}? I believe the
> test will take so long to finish on this case what is bad for qa alghouth
> it is the complete one. What do you think?
I think we don't need the full set of tests also with system suspend. I
think just one test which catches the current bug is good enough, after
all if psr is set up correctly it should work the same at runtime than
over s/r. And since this is a test I'd just copypaste the relevant subtest
(if it doesn't integrate quickly into the existing code), not worth at all
to make a big fuzz.
And we have lots of resume tests already, they "only" take about
30 second. Only important to have "suspend" somewhere in the subtest name
so that all system suspend tests can easily be filtered out/selected.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/4] drm/i915: Increase PSR Idle Frame to 2.
On Thu, Sep 04, 2014 at 05:40:05PM -0700, Rodrigo Vivi wrote: > Here goes results on BDW with pure today's nightly (with idle_frame=1) > > # First run > > IGT-Version: 1.7-gd4b43f0 (x86_64) (Linux: 3.17.0-rc2+ x86_64) > Subtest primary_page_flip: SUCCESS > Subtest primary_mmap_gtt: SUCCESS > Test assertion failure function test_crc, file kms_psr_sink_crc.c:307: > Failed assertion: strcmp(ref_crc, CRC_GREEN) != 0 > Subtest primary_mmap_gtt_waiting: FAIL > Subtest primary_mmap_cpu: SUCCESS > Subtest primary_blt: SUCCESS > Subtest primary_render: SUCCESS > Subtest sprite_mmap_gtt: SUCCESS > Waiting 10s... > Subtest sprite_mmap_gtt_waiting: SUCCESS > Subtest sprite_mmap_cpu: SUCCESS > Subtest sprite_blt: SUCCESS > Subtest sprite_render: SUCCESS > Subtest sprite_plane_move: SUCCESS > Subtest sprite_plane_onoff: SUCCESS > Subtest cursor_mmap_gtt: SUCCESS > Waiting 10s... > Subtest cursor_mmap_gtt_waiting: SUCCESS > Subtest cursor_mmap_cpu: SUCCESS > Subtest cursor_blt: SUCCESS > Subtest cursor_render: SUCCESS > Subtest cursor_plane_move: SUCCESS > Subtest cursor_plane_onoff: SUCCESS > > #second run: > > IGT-Version: 1.7-gd4b43f0 (x86_64) (Linux: 3.17.0-rc2+ x86_64) > Subtest primary_page_flip: SUCCESS > Subtest primary_mmap_gtt: SUCCESS > Waiting 10s... > Subtest primary_mmap_gtt_waiting: SUCCESS > Subtest primary_mmap_cpu: SUCCESS > Subtest primary_blt: SUCCESS > Subtest primary_render: SUCCESS > Subtest sprite_mmap_gtt: SUCCESS > Waiting 10s... > Subtest sprite_mmap_gtt_waiting: SUCCESS > Subtest sprite_mmap_cpu: SUCCESS > Subtest sprite_blt: SUCCESS > Test assertion failure function test_crc, file kms_psr_sink_crc.c:307: > Failed assertion: strcmp(ref_crc, CRC_GREEN) != 0 > Subtest sprite_render: FAIL > Subtest sprite_plane_move: SUCCESS > Subtest sprite_plane_onoff: SUCCESS > Subtest cursor_mmap_gtt: SUCCESS > Waiting 10s... > Subtest cursor_mmap_gtt_waiting: SUCCESS > Subtest cursor_mmap_cpu: SUCCESS > Subtest cursor_blt: SUCCESS > Subtest cursor_render: SUCCESS > Subtest cursor_plane_move: SUCCESS > Subtest cursor_plane_onoff: SUCCESS > > random failures! but better than hsw at least. Ugh, this is painful :( > However the hardcoded color is indeed a mistake... Green on this panel is > different from the green on the other panel. > But I'm also not sure about drawing the color with cairo... How can we be > sure what is there is what we want anyway. Yeah, the crc value is implementation defined, but otoh we don't really depend upon that, as long as the same output results in the same crc. > So maybe it is better to fall back to old scheme where we just check if > changed when we want it changes... without checking for colors. > What do you think? You can't check for change with crc due to collisions, you really only (at least reliably) can check for matching outputs. And as long as we don't have any color correction enabled a given color on the sprite/cursor plane should exactly match the same color on the primary plane. For inspiration Ville's cursor crc testcase has all the ingredients I think you'll need for the sprite/cursor move tests. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/hdmi, pd: Do not dereference the encoder in the connector destroy
On Thu, Sep 04, 2014 at 09:43:45PM +0100, Chris Wilson wrote:
> Oops, apparently intel_hdmi/intel_dp is the encoder - an object with a
> distinct lifetime to the connector, and so we cannot simply reuse the
> common function to unset and free the edid.
>
> Signed-off-by: Chris Wilson
Queued for -next, thanks for the patch.
-Daniel
> ---
> drivers/gpu/drm/i915/intel_dp.c | 2 +-
> drivers/gpu/drm/i915/intel_hdmi.c | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index df477cd34b6b..082ef3eda5a5 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4415,7 +4415,7 @@ intel_dp_connector_destroy(struct drm_connector
> *connector)
> {
> struct intel_connector *intel_connector = to_intel_connector(connector);
>
> - intel_dp_unset_edid(intel_attached_dp(connector));
> + kfree(intel_connector->detect_edid);
>
> if (!IS_ERR_OR_NULL(intel_connector->edid))
> kfree(intel_connector->edid);
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c
> b/drivers/gpu/drm/i915/intel_hdmi.c
> index 867247b935f5..084a182bceb6 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -1495,7 +1495,7 @@ static void chv_hdmi_pre_enable(struct intel_encoder
> *encoder)
>
> static void intel_hdmi_destroy(struct drm_connector *connector)
> {
> - intel_hdmi_unset_edid(connector);
> + kfree(to_intel_connector(connector)->detect_edid);
> drm_connector_cleanup(connector);
> kfree(connector);
> }
> --
> 2.1.0
>
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 43/89] drm/i915/skl: Read the Memory Latency Values for WM computation
On Fri, Sep 05, 2014 at 09:29:33AM +0100, Damien Lespiau wrote: > On Fri, Sep 05, 2014 at 11:25:30AM +0300, Ville Syrjälä wrote: > > > +/* SKL GT Driver Mailbox registers for reading memory latencies */ > > > +#define GEN9_MAILBOX_DATA1 0x13812C > > > +#define GEN9_MAILBOX_READ_MEM_LAT (0x6) > > > +#define GEN9_MAILBOX_READ_TIMEOUT 150 > > > > Timeout not used anywhere. Also spec says 100us. > > > > > +#define GEN9_MEM_LAT_LEVEL_MASK0xFF > > > +#define GEN9_MEM_LAT_LEVEL_1_5_SHIFT 8 > > > +#define GEN9_MEM_LAT_LEVEL_2_6_SHIFT 16 > > > +#define GEN9_MEM_LAT_LEVEL_3_7_SHIFT 24 > > > > This stuff should be grouped along the other pcode register defines. > > Funny you mention this, I fixed those two in the v6 sent as reply > yesterday. Well that's two down then. I should stop composing mails the previous day and then forgetting to send them and firing them off the next day without checking if they still make sense :) -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/bdw: cancel the SW turbo tasks before runtime suspending
On Thu, Sep 04, 2014 at 06:07:02PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni
>
> If we don't cancel them, we may end up running them while the device
> is runtime suspended, which will trigger lots and lots of WARNs on
> dmesg.
>
> Regression introduced by:
> commit c76bb61a71083b2d90504cc6d0dda2047c5d63ca
> Author: Daisy Sun
> Date: Mon Aug 11 11:08:38 2014 -0700
> drm/i915/bdw: BDW Software Turbo
>
> Testcase: igt/pm_rpm/gem-execbuf (you may have to run it a few times)
> Signed-off-by: Paulo Zanoni
Please don't forget to cc relevant people when you fix a regression.
-Daniel
> ---
> drivers/gpu/drm/i915/i915_drv.c | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 8ff3755..4ce217b 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1448,6 +1448,10 @@ static int intel_runtime_suspend(struct device *device)
>* intel_mark_idle().
>*/
> cancel_work_sync(&dev_priv->rps.work);
> + if (dev_priv->rps.is_bdw_sw_turbo) {
> + del_timer_sync(&dev_priv->rps.sw_turbo.flip_timer);
> + cancel_work_sync(&dev_priv->rps.sw_turbo.work_max_freq);
> + }
> intel_runtime_pm_disable_interrupts(dev);
>
> ret = intel_suspend_complete(dev_priv);
> --
> 2.1.0
>
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 02/10] drm/i915: Decouple the stuck pageflip on modeset
On Fri, Sep 05, 2014 at 08:31:20AM +0100, Chris Wilson wrote: > On Fri, Sep 05, 2014 at 09:27:16AM +0200, Daniel Vetter wrote: > > On Thu, Sep 04, 2014 at 08:15:32PM +0300, Ville Syrjälä wrote: > > > On Tue, Sep 02, 2014 at 02:57:37PM +0100, Chris Wilson wrote: > > > > If we successfully confuse the hardware, and cause it to drop a queued > > > > pageflip, we wait for 60s and issue a warning before continuing on with > > > > the modeset. However, this leaves the pending pageflip still stuck > > > > indefinitely. Pretend to userspace that it does complete, and let us > > > > start afresh following the modeset. > > > > > > > > v2: Rebase after refactor > > > > This applied cleanly despite that I don't have that refactor. Should I be > > concerned? > > No, the refactoring here was about the pageflip checking itself in patch > 1. I just kept moving these two in lockstep. Yeah, serious lack of coffee when I tried to apply this ;-) -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/4] drm/i915: Remove PSR HW tracking.
On Thu, Sep 04, 2014 at 12:53:39PM -0700, Rodrigo Vivi wrote:
> On Thu, Sep 4, 2014 at 1:06 AM, Ville Syrjälä > wrote:
>
> > On Wed, Sep 03, 2014 at 10:49:57PM -0400, Rodrigo Vivi wrote:
> > > Now that we are tracking psr states on Software side we don't need HW
> > help anymore.
> > > So we can clean up the code a bit and avoid unecessary sets.
> > >
> > > Signed-off-by: Rodrigo Vivi
> > > ---
> > > drivers/gpu/drm/i915/intel_dp.c | 12
> > > 1 file changed, 12 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > b/drivers/gpu/drm/i915/intel_dp.c
> > > index a796831..9414e67 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -1778,10 +1778,7 @@ static void intel_edp_psr_enable_sink(struct
> > intel_dp *intel_dp)
> > > {
> > > struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> > > struct drm_device *dev = dig_port->base.base.dev;
> > > - struct drm_i915_private *dev_priv = dev->dev_private;
> > > uint32_t aux_clock_divider;
> > > - int precharge = 0x3;
> > > - int msg_size = 5; /* Header(4) + Message(1) */
> > > bool only_standby = false;
> > >
> > > aux_clock_divider = intel_dp->get_aux_clock_divider(intel_dp, 0);
> > > @@ -1796,15 +1793,6 @@ static void intel_edp_psr_enable_sink(struct
> > intel_dp *intel_dp)
> > > else
> > > drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG,
> > > DP_PSR_ENABLE |
> > DP_PSR_MAIN_LINK_ACTIVE);
> > > -
> > > - /* Setup AUX registers */
> > > - I915_WRITE(EDP_PSR_AUX_DATA1(dev), EDP_PSR_DPCD_COMMAND);
> > > - I915_WRITE(EDP_PSR_AUX_DATA2(dev), EDP_PSR_DPCD_NORMAL_OPERATION);
> > > - I915_WRITE(EDP_PSR_AUX_CTL(dev),
> > > -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));
> >
> > As I said I blieve FBC is the one that does the hw tracking. I think the
> > hardware will still do the entry/exit (including automagic link
> > re-train) but now you've not told it how to wake up the sink. So I don't
> > understand how this can even work.
> >
>
> Per spec we have to program aux "registers for HW assist PSR Exit Support."
And how do you exit w/o HW assist exactly? You never do the DPCD write
manually so I don't see how it can work.
>
> With the exit being made on SW side we don't need HW help anymore.
> Same reason why this works with VLV.
>
> But there is no way to claim full control. So automagically triggers still
> exist.
> So I couldn't remove the W/As that mask events on srd_debug. :(
>
> But withouth these aux sets we are fine and it just works.
Looks like these magic numbers are in fact just the SET_POWER(D0) DPCD
write. Might be good to clarify this with Art, but I think what is happening
is that you're using link standby, so just the PSR SDP sent inline on the
main link is enough to exit PSR. But if you'd use the main link off mode
then you'd need this AUX stuff.
We should probably kill the magic numbers here and just assemble the AUX
data like we would do for normal AUX. At least that way people would
understand what it's meant to do from just reading the code. Right now
it looks like black magic and the define names don't really help either.
>
>
> >
> > > }
> > >
> > > static void intel_edp_psr_enable_source(struct intel_dp *intel_dp)
> > > --
> > > 1.9.3
> > >
> > > ___
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> > --
> > Ville Syrjälä
> > Intel OTC
> > ___
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
>
>
>
> --
> Rodrigo Vivi
> Blog: http://blog.vivi.eng.br
--
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH i-g-t v2 6/6] tests/kms_3d: skip if connectors cannot be forced
Signed-off-by: Thomas Wood
---
tests/kms_3d.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/tests/kms_3d.c b/tests/kms_3d.c
index ddf4dc6..c11873b 100644
--- a/tests/kms_3d.c
+++ b/tests/kms_3d.c
@@ -60,7 +60,8 @@ igt_simple_main
&length);
kmstest_force_edid(drm_fd, connector, edid, length);
- kmstest_force_connector(drm_fd, connector, FORCE_CONNECTOR_ON);
+ if (!kmstest_force_connector(drm_fd, connector, FORCE_CONNECTOR_ON))
+ igt_skip("Could not force connector on\n");
connector_id = connector->connector_id;
--
1.9.3
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH i-g-t v2 0/6] 3D stereo mode testing
This updated and rebased series fixes various issues with the previous one and also skips testing on gen 7 and 8 where it is not currently possible to force the HDMI and DP connector states. There is also a small documentation fix for igt_create_fb. Thomas Wood (6): lib: add kmstest_edid_add_3d lib: move create_stereo_fb from testdisplay to igt_fb tests: add kms_3d test lib/igt_fb: ensure igt_create_fb parameters are consistent lib: don't force HDMI or DP connectors on gen 7 and 8 tests/kms_3d: skip if connectors cannot be forced lib/Makefile.am| 4 +- lib/igt_fb.c | 102 - lib/igt_fb.h | 4 +- lib/igt_kms.c | 92 + lib/igt_kms.h | 1 + tests/.gitignore | 1 + tests/Android.mk | 1 + tests/Makefile.sources | 1 + tests/kms_3d.c | 121 + tests/testdisplay.c| 118 ++- 10 files changed, 327 insertions(+), 118 deletions(-) create mode 100644 tests/kms_3d.c -- 1.9.3 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH i-g-t v2 5/6] lib: don't force HDMI or DP connectors on gen 7 and 8
Forcing HDMI or DP connectors on gen 7 and 8 doesn't currently work, so
fail early to allow the test to skip if required.
Signed-off-by: Thomas Wood
---
lib/igt_kms.c | 12
1 file changed, 12 insertions(+)
diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index 0dc46f9..e9455aa 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -41,6 +41,7 @@
#include "drmtest.h"
#include "igt_kms.h"
#include "igt_aux.h"
+#include "intel_chipset.h"
/*
* There hasn't been a release of libdrm containing these #define's yet, so
@@ -344,6 +345,17 @@ bool kmstest_force_connector(int drm_fd, drmModeConnector
*connector,
char *path;
const char *value;
int debugfs_fd, ret;
+ uint32_t devid;
+
+ devid = intel_get_drm_devid(drm_fd);
+
+ /* forcing hdmi or dp connectors on gen 7 and 8 doesn't currently work,
+* so fail early to allow the test to skip if required */
+ if ((connector->connector_type == DRM_MODE_CONNECTOR_HDMIA ||
+ connector->connector_type == DRM_MODE_CONNECTOR_HDMIB ||
+ connector->connector_type == DRM_MODE_CONNECTOR_DisplayPort)
+ && (IS_GEN7(devid) || IS_GEN8(devid)))
+ return false;
switch (state) {
case FORCE_CONNECTOR_ON:
--
1.9.3
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH i-g-t v2 1/6] lib: add kmstest_edid_add_3d
kmstest_edid_add_3d adds an EDID extension block with 3D support to a
copy of the specified EDID.
v2: Avoid using an invalid CEC SPA (Clint Taylor)
Signed-off-by: Thomas Wood
---
lib/igt_kms.c | 80 +++
lib/igt_kms.h | 1 +
2 files changed, 81 insertions(+)
diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index d763013..0dc46f9 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -649,6 +649,86 @@ kmstest_get_property(int drm_fd, uint32_t object_id,
uint32_t object_type,
}
/**
+ * kmstest_edid_add_3d:
+ * @edid: an existing valid edid block
+ * @length: length of @edid
+ * @new_edid_ptr: pointer to where the new edid will be placed
+ * @new_length: pointer to the size of the new edid
+ *
+ * Makes a copy of an existing edid block and adds an extension indicating
+ * stereo 3D capabilities.
+ */
+void kmstest_edid_add_3d(const unsigned char *edid, size_t length,
+unsigned char *new_edid_ptr[], size_t *new_length)
+{
+ unsigned char *new_edid;
+ int n_extensions;
+ char sum = 0;
+ int pos;
+ int i;
+ char cea_header_len = 4, video_block_len = 6, vsdb_block_len = 11;
+
+ igt_assert(new_edid_ptr != NULL && new_length != NULL);
+
+ *new_length = length + 128;
+
+ new_edid = calloc(*new_length, sizeof(char));
+ memcpy(new_edid, edid, length);
+ *new_edid_ptr = new_edid;
+
+ n_extensions = new_edid[126];
+ n_extensions++;
+ new_edid[126] = n_extensions;
+
+ /* recompute checksum */
+ for (i = 0; i < 127; i++) {
+ sum = sum + new_edid[i];
+ }
+ new_edid[127] = 256 - sum;
+
+ /* add a cea-861 extension block */
+ pos = length;
+ new_edid[pos++] = 0x2;
+ new_edid[pos++] = 0x3;
+ new_edid[pos++] = cea_header_len + video_block_len + vsdb_block_len;
+ new_edid[pos++] = 0x0;
+
+ /* video block (id | length) */
+ new_edid[pos++] = 2 << 5 | (video_block_len - 1);
+ new_edid[pos++] = 32 | 0x80; /* 1080p @ 24Hz | (native)*/
+ new_edid[pos++] = 5; /* 1080i @ 60Hz */
+ new_edid[pos++] = 20;/* 1080i @ 50Hz */
+ new_edid[pos++] = 4; /* 720p @ 60Hz*/
+ new_edid[pos++] = 19;/* 720p @ 50Hz*/
+
+ /* vsdb block ( id | length ) */
+ new_edid[pos++] = 3 << 5 | (vsdb_block_len - 1);
+ /* registration id */
+ new_edid[pos++] = 0x3;
+ new_edid[pos++] = 0xc;
+ new_edid[pos++] = 0x0;
+ /* source physical address */
+ new_edid[pos++] = 0x10;
+ new_edid[pos++] = 0x00;
+ /* Supports_AI ... etc */
+ new_edid[pos++] = 0x00;
+ /* Max TMDS Clock */
+ new_edid[pos++] = 0x00;
+ /* Latency present, HDMI Video Present */
+ new_edid[pos++] = 0x20;
+ /* HDMI Video */
+ new_edid[pos++] = 0x80;
+ new_edid[pos++] = 0x00;
+
+ /* checksum */
+ sum = 0;
+ for (i = 0; i < 127; i++) {
+ sum = sum + new_edid[length + i];
+ }
+ new_edid[length + 127] = 256 - sum;
+}
+
+/**
* kmstest_unset_all_crtcs:
* @drm_fd: the DRM fd
* @resources: libdrm resources pointer
diff --git a/lib/igt_kms.h b/lib/igt_kms.h
index 4263a01..921afef 100644
--- a/lib/igt_kms.h
+++ b/lib/igt_kms.h
@@ -149,6 +149,7 @@ enum kmstest_generic_edid {
bool kmstest_force_connector(int fd, drmModeConnector *connector,
enum kmstest_force_connector_state state);
+void kmstest_edid_add_3d(const unsigned char *edid, size_t length, unsigned
char *new_edid_ptr[], size_t *new_length);
void kmstest_force_edid(int drm_fd, drmModeConnector *connector,
const unsigned char *edid, size_t length);
--
1.9.3
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH i-g-t v2 4/6] lib/igt_fb: ensure igt_create_fb parameters are consistent
Make sure the parameters in the prototype and implementation of
igt_create_fb match and are complete so that the documentation is
correct.
Signed-off-by: Thomas Wood
---
lib/igt_fb.c | 2 +-
lib/igt_fb.h | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/lib/igt_fb.c b/lib/igt_fb.c
index f9f5de2..ce0dd6b 100644
--- a/lib/igt_fb.c
+++ b/lib/igt_fb.c
@@ -455,7 +455,7 @@ igt_create_fb_with_bo_size(int fd, int width, int height,
* The kms id of the created framebuffer.
*/
unsigned int igt_create_fb(int fd, int width, int height, uint32_t format,
- unsigned tiling, struct igt_fb *fb)
+ unsigned int tiling, struct igt_fb *fb)
{
return igt_create_fb_with_bo_size(fd, width, height, format, tiling,
fb, 0);
}
diff --git a/lib/igt_fb.h b/lib/igt_fb.h
index e6f72e9..d9fb6bb 100644
--- a/lib/igt_fb.h
+++ b/lib/igt_fb.h
@@ -70,7 +70,7 @@ igt_create_fb_with_bo_size(int fd, int width, int height,
uint32_t format, unsigned int tiling,
struct igt_fb *fb, unsigned bo_size);
unsigned int igt_create_fb(int fd, int width, int height, uint32_t format,
- unsigned int , struct igt_fb *fb);
+ unsigned int tiling, struct igt_fb *fb);
unsigned int igt_create_color_fb(int fd, int width, int height,
uint32_t format, unsigned int tiling,
double r, double g, double b,
--
1.9.3
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH i-g-t v2 3/6] tests: add kms_3d test
Add a test to verify creation and use of 3D stereo modes.
v2: update for API changes
Signed-off-by: Thomas Wood
---
tests/.gitignore | 1 +
tests/Android.mk | 1 +
tests/Makefile.sources | 1 +
tests/kms_3d.c | 120 +
4 files changed, 123 insertions(+)
create mode 100644 tests/kms_3d.c
diff --git a/tests/.gitignore b/tests/.gitignore
index efa0c92..1ea0681 100644
--- a/tests/.gitignore
+++ b/tests/.gitignore
@@ -117,6 +117,7 @@ igt_no_exit
igt_no_exit_list_only
igt_no_subtest
igt_simulation
+kms_3d
kms_addfb
kms_cursor_crc
kms_fbc_crc
diff --git a/tests/Android.mk b/tests/Android.mk
index 3644aa1..f28b400 100644
--- a/tests/Android.mk
+++ b/tests/Android.mk
@@ -55,6 +55,7 @@ ifeq ("${ANDROID_HAS_CAIRO}", "1")
else
# the following tests depend on cairo, so skip them
skip_tests_list += \
+kms_3d \
kms_plane \
kms_addfb \
kms_cursor_crc \
diff --git a/tests/Makefile.sources b/tests/Makefile.sources
index 551555f..a6677dd 100644
--- a/tests/Makefile.sources
+++ b/tests/Makefile.sources
@@ -142,6 +142,7 @@ TESTS_progs = \
gen3_render_tiledx_blits \
gen3_render_tiledy_blits \
gen7_forcewake_mt \
+ kms_3d \
kms_force_connector \
kms_sink_crc_basic \
kms_fence_pin_leak \
diff --git a/tests/kms_3d.c b/tests/kms_3d.c
new file mode 100644
index 000..ddf4dc6
--- /dev/null
+++ b/tests/kms_3d.c
@@ -0,0 +1,120 @@
+/*
+ * Copyright © 2014 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ */
+
+#include "igt_core.h"
+#include "igt_kms.h"
+#include "drmtest.h"
+#include "igt_edid.h"
+
+igt_simple_main
+{
+ int drm_fd;
+ drmModeRes *res;
+ drmModeConnector *connector;
+ unsigned char *edid;
+ size_t length;
+ int mode_count, connector_id;
+
+ drm_fd = drm_open_any();
+ res = drmModeGetResources(drm_fd);
+
+ igt_assert(drmSetClientCap(drm_fd, DRM_CLIENT_CAP_STEREO_3D, 1) >= 0);
+
+ /* find an hdmi connector */
+ for (int i = 0; i < res->count_connectors; i++) {
+
+ connector = drmModeGetConnector(drm_fd, res->connectors[i]);
+
+ if (connector->connector_type == DRM_MODE_CONNECTOR_HDMIA &&
+ connector->connection == DRM_MODE_DISCONNECTED)
+ break;
+
+ drmModeFreeConnector(connector);
+
+ connector = NULL;
+ }
+ igt_require(connector);
+
+ kmstest_edid_add_3d(generic_edid[EDID_FHD], EDID_LENGTH, &edid,
+ &length);
+
+ kmstest_force_edid(drm_fd, connector, edid, length);
+ kmstest_force_connector(drm_fd, connector, FORCE_CONNECTOR_ON);
+
+ connector_id = connector->connector_id;
+
+ /* check for 3D modes */
+ mode_count = 0;
+ connector = drmModeGetConnector(drm_fd, connector_id);
+ for (int i = 0; i < connector->count_modes; i++) {
+ if (connector->modes[i].flags & DRM_MODE_FLAG_3D_MASK)
+ mode_count++;
+ }
+
+ igt_assert(mode_count == 13);
+
+ /* set 3D modes */
+ igt_info("Testing:\n");
+ for (int i = 0; i < connector->count_modes; i++) {
+ int fb_id;
+ struct kmstest_connector_config config;
+ int crtc_mask = -1;
+ int ret;
+
+ if (!(connector->modes[i].flags & DRM_MODE_FLAG_3D_MASK))
+ continue;
+
+ /* create a configuration */
+ ret = kmstest_get_connector_config(drm_fd, connector_id,
+ crtc_mask, &config);
+ if (ret != true) {
+ igt_info("Error creating configuration for:\n ");
+ kmstest_dump_mode(&connector->modes[i]);
+
+
[Intel-gfx] [PATCH i-g-t v2 2/6] lib: move create_stereo_fb from testdisplay to igt_fb
Move create_stereo_fb from testdisplay to igt_create_stereo_fb in igt_fb
so that it can be used in other tests.
v2: update for new igt_create_fb API
add parameters for format and tiling
remove some old debug code
Signed-off-by: Thomas Wood
---
lib/Makefile.am | 4 +-
lib/igt_fb.c| 100
lib/igt_fb.h| 2 +
tests/testdisplay.c | 118 ++--
4 files changed, 108 insertions(+), 116 deletions(-)
diff --git a/lib/Makefile.am b/lib/Makefile.am
index 001ecab..36cf2d3 100644
--- a/lib/Makefile.am
+++ b/lib/Makefile.am
@@ -7,7 +7,9 @@ noinst_LTLIBRARIES = libintel_tools.la
noinst_HEADERS = check-ndebug.h
AM_CPPFLAGS = -I$(top_srcdir)
-AM_CFLAGS = $(DRM_CFLAGS) $(CWARNFLAGS)
+AM_CFLAGS = $(DRM_CFLAGS) $(CWARNFLAGS) \
+ -DIGT_DATADIR=\""$(abs_top_srcdir)/tests"\"
+
LDADD = $(CAIRO_LIBS)
AM_CFLAGS += $(CAIRO_CFLAGS)
diff --git a/lib/igt_fb.c b/lib/igt_fb.c
index 71d9a26..f9f5de2 100644
--- a/lib/igt_fb.c
+++ b/lib/igt_fb.c
@@ -502,6 +502,106 @@ unsigned int igt_create_color_fb(int fd, int width, int
height,
return fb_id;
}
+struct box {
+ int x, y, width, height;
+};
+
+struct stereo_fb_layout {
+ int fb_width, fb_height;
+ struct box left, right;
+};
+
+static void box_init(struct box *box, int x, int y, int bwidth, int bheight)
+{
+ box->x = x;
+ box->y = y;
+ box->width = bwidth;
+ box->height = bheight;
+}
+
+
+static void stereo_fb_layout_from_mode(struct stereo_fb_layout *layout,
+ drmModeModeInfo *mode)
+{
+ unsigned int format = mode->flags & DRM_MODE_FLAG_3D_MASK;
+ const int hdisplay = mode->hdisplay, vdisplay = mode->vdisplay;
+ int middle;
+
+ switch (format) {
+ case DRM_MODE_FLAG_3D_TOP_AND_BOTTOM:
+ layout->fb_width = hdisplay;
+ layout->fb_height = vdisplay;
+
+ middle = vdisplay / 2;
+ box_init(&layout->left, 0, 0, hdisplay, middle);
+ box_init(&layout->right,
+0, middle, hdisplay, vdisplay - middle);
+ break;
+ case DRM_MODE_FLAG_3D_SIDE_BY_SIDE_HALF:
+ layout->fb_width = hdisplay;
+ layout->fb_height = vdisplay;
+
+ middle = hdisplay / 2;
+ box_init(&layout->left, 0, 0, middle, vdisplay);
+ box_init(&layout->right,
+middle, 0, hdisplay - middle, vdisplay);
+ break;
+ case DRM_MODE_FLAG_3D_FRAME_PACKING:
+ {
+ int vactive_space = mode->vtotal - vdisplay;
+
+ layout->fb_width = hdisplay;
+ layout->fb_height = 2 * vdisplay + vactive_space;
+
+ box_init(&layout->left,
+0, 0, hdisplay, vdisplay);
+ box_init(&layout->right,
+0, vdisplay + vactive_space, hdisplay, vdisplay);
+ break;
+ }
+ default:
+ igt_assert(0);
+ }
+}
+
+/**
+ * igt_create_stereo_fb:
+ * @drm_fd: open i915 drm file descriptor
+ * @mode: A stereo 3D mode.
+ * @format: drm fourcc pixel format code
+ * @tiling: tiling layout of the framebuffer
+ *
+ * Create a framebuffer for use with the stereo 3D mode specified by @mode.
+ *
+ * Returns:
+ * The kms id of the created framebuffer on success or a negative error code on
+ * failure.
+ */
+unsigned int igt_create_stereo_fb(int drm_fd, drmModeModeInfo *mode,
+ uint32_t format, unsigned int tiling)
+{
+ struct stereo_fb_layout layout;
+ cairo_t *cr;
+ uint32_t fb_id;
+ struct igt_fb fb;
+
+ stereo_fb_layout_from_mode(&layout, mode);
+ fb_id = igt_create_fb(drm_fd, layout.fb_width, layout.fb_height, format,
+ tiling, &fb);
+ cr = igt_get_cairo_ctx(drm_fd, &fb);
+
+ igt_paint_image(cr, IGT_DATADIR"/1080p-left.png",
+ layout.left.x, layout.left.y,
+ layout.left.width, layout.left.height);
+ igt_paint_image(cr, IGT_DATADIR"/1080p-right.png",
+ layout.right.x, layout.right.y,
+ layout.right.width, layout.right.height);
+
+ cairo_destroy(cr);
+
+ return fb_id;
+}
+
static cairo_format_t drm_format_to_cairo(uint32_t drm_format)
{
struct format_desc_struct *f;
diff --git a/lib/igt_fb.h b/lib/igt_fb.h
index 4295df9..e6f72e9 100644
--- a/lib/igt_fb.h
+++ b/lib/igt_fb.h
@@ -75,6 +75,8 @@ unsigned int igt_create_color_fb(int fd, int width, int
height,
uint32_t format, unsigned int tiling,
double r, double g, double b,
struct igt_fb *fb /* out */);
+unsigned int igt_create_stereo_fb(int drm_fd, drmModeModeInfo *mode,
+
[Intel-gfx] [PATCH i-g-t] lib: keep a list of modified connectors to reset
Avoid calling functions in igt_reset_connectors that are not safe to use
in signal handlers by keeping a list of connectors that have been
modified, instead of enumerating all connectors.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=83498
Cc: Paulo Zanoni
Signed-off-by: Thomas Wood
---
lib/igt_kms.c | 50 +-
1 file changed, 33 insertions(+), 17 deletions(-)
diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index f483e2d..933e6fb 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -54,6 +54,9 @@
#define DRM_PLANE_TYPE_PRIMARY 1
#define DRM_PLANE_TYPE_CURSOR 2
+/* list of connectors that need resetting on exit */
+#define MAX_CONNECTORS 32
+static char *forced_connectors[MAX_CONNECTORS];
/**
@@ -341,9 +344,9 @@ static char* get_debugfs_connector_path(int drm_fd,
drmModeConnector *connector,
bool kmstest_force_connector(int drm_fd, drmModeConnector *connector,
enum kmstest_force_connector_state state)
{
- char *path;
+ char *path, **tmp;
const char *value;
- int debugfs_fd, ret;
+ int debugfs_fd, ret, len;
switch (state) {
case FORCE_CONNECTOR_ON:
@@ -364,7 +367,6 @@ bool kmstest_force_connector(int drm_fd, drmModeConnector
*connector,
path = get_debugfs_connector_path(drm_fd, connector, "force");
debugfs_fd = open(path, O_WRONLY | O_TRUNC);
- free(path);
if (debugfs_fd == -1) {
return false;
@@ -373,6 +375,27 @@ bool kmstest_force_connector(int drm_fd, drmModeConnector
*connector,
ret = write(debugfs_fd, value, strlen(value));
close(debugfs_fd);
+ for (len = 0, tmp = forced_connectors; *tmp; tmp++) {
+ /* check the connector is not already present */
+ if (strcmp(*tmp, path) == 0) {
+ len = -1;
+ break;
+ }
+ len++;
+ }
+
+ if (len != -1)
+ forced_connectors[len] = path;
+
+ igt_debug("Connector %s is now forced %s\n", path, value);
+ igt_debug("Current forced connectors:\n");
+ tmp = forced_connectors;
+ while (*tmp) {
+ igt_debug("\t%s\n", *tmp);
+ tmp++;
+ }
+
+
igt_assert(ret != -1);
return (ret == -1) ? false : true;
}
@@ -1643,21 +1666,14 @@ void igt_enable_connectors(void)
*/
void igt_reset_connectors(void)
{
- drmModeRes *res;
- drmModeConnector *c;
- int drm_fd;
-
- drm_fd = drm_open_any();
- res = drmModeGetResources(drm_fd);
-
- for (int i = 0; i < res->count_connectors; i++) {
-
- c = drmModeGetConnector(drm_fd, res->connectors[i]);
+ char **tmp;
- kmstest_force_connector(drm_fd, c, FORCE_CONNECTOR_UNSPECIFIED);
+ /* reset the connectors stored in forced_connectors, avoiding any
+* functions that are not safe to call in signal handlers */
- drmModeFreeConnector(c);
+ for (tmp = forced_connectors; *tmp; tmp++) {
+ int fd = open(*tmp, O_WRONLY | O_TRUNC);
+ write(fd, "unspecified", 11);
+ close(fd);
}
-
- close(drm_fd);
}
--
1.9.3
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH i-g-t] lib: keep a list of modified connectors to reset
On Fri, Sep 05, 2014 at 11:55:19AM +0100, Thomas Wood wrote:
> Avoid calling functions in igt_reset_connectors that are not safe to use
> in signal handlers by keeping a list of connectors that have been
> modified, instead of enumerating all connectors.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=83498
> Cc: Paulo Zanoni
> Signed-off-by: Thomas Wood
> ---
> lib/igt_kms.c | 50 +-
> 1 file changed, 33 insertions(+), 17 deletions(-)
>
> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> index f483e2d..933e6fb 100644
> --- a/lib/igt_kms.c
> +++ b/lib/igt_kms.c
> @@ -54,6 +54,9 @@
> #define DRM_PLANE_TYPE_PRIMARY 1
> #define DRM_PLANE_TYPE_CURSOR 2
>
> +/* list of connectors that need resetting on exit */
> +#define MAX_CONNECTORS 32
> +static char *forced_connectors[MAX_CONNECTORS];
Need to leave space for the sentinel NULL value.
static char *forced_connectors[MAX_CONNECTORS + 1];
> /**
> @@ -341,9 +344,9 @@ static char* get_debugfs_connector_path(int drm_fd,
> drmModeConnector *connector,
> bool kmstest_force_connector(int drm_fd, drmModeConnector *connector,
>enum kmstest_force_connector_state state)
> {
> - char *path;
> + char *path, **tmp;
> const char *value;
> - int debugfs_fd, ret;
> + int debugfs_fd, ret, len;
>
> switch (state) {
> case FORCE_CONNECTOR_ON:
> @@ -364,7 +367,6 @@ bool kmstest_force_connector(int drm_fd, drmModeConnector
> *connector,
>
> path = get_debugfs_connector_path(drm_fd, connector, "force");
> debugfs_fd = open(path, O_WRONLY | O_TRUNC);
> - free(path);
>
> if (debugfs_fd == -1) {
> return false;
> @@ -373,6 +375,27 @@ bool kmstest_force_connector(int drm_fd,
> drmModeConnector *connector,
> ret = write(debugfs_fd, value, strlen(value));
> close(debugfs_fd);
>
> + for (len = 0, tmp = forced_connectors; *tmp; tmp++) {
> + /* check the connector is not already present */
> + if (strcmp(*tmp, path) == 0) {
> + len = -1;
> + break;
> + }
> + len++;
> + }
if (len >= MAX_CONNECTORS)
len = -1;
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH i-g-t v2 5/6] lib: don't force HDMI or DP connectors on gen 7 and 8
On Fri, Sep 05, 2014 at 10:52:08AM +0100, Thomas Wood wrote:
> Forcing HDMI or DP connectors on gen 7 and 8 doesn't currently work, so
> fail early to allow the test to skip if required.
>
> Signed-off-by: Thomas Wood
> ---
> lib/igt_kms.c | 12
> 1 file changed, 12 insertions(+)
>
> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> index 0dc46f9..e9455aa 100644
> --- a/lib/igt_kms.c
> +++ b/lib/igt_kms.c
> @@ -41,6 +41,7 @@
> #include "drmtest.h"
> #include "igt_kms.h"
> #include "igt_aux.h"
> +#include "intel_chipset.h"
>
> /*
> * There hasn't been a release of libdrm containing these #define's yet, so
> @@ -344,6 +345,17 @@ bool kmstest_force_connector(int drm_fd,
> drmModeConnector *connector,
> char *path;
> const char *value;
> int debugfs_fd, ret;
> + uint32_t devid;
> +
> + devid = intel_get_drm_devid(drm_fd);
> +
> + /* forcing hdmi or dp connectors on gen 7 and 8 doesn't currently work,
> + * so fail early to allow the test to skip if required */
> + if ((connector->connector_type == DRM_MODE_CONNECTOR_HDMIA ||
> + connector->connector_type == DRM_MODE_CONNECTOR_HDMIB ||
> + connector->connector_type == DRM_MODE_CONNECTOR_DisplayPort)
> + && (IS_GEN7(devid) || IS_GEN8(devid)))
This catches too many platforms, since on ivb, vlv and chv we _can_
already use this. As well as on earlier platforms at least. And since
those platforms are under active testing by QA I think we really want
that. So the right check for now is IS_HSW || IS_BDW || IS_SKL (if Damien
pushed the igt/libdrm stuff already).
-Daniel
> + return false;
>
> switch (state) {
> case FORCE_CONNECTOR_ON:
> --
> 1.9.3
>
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 2/2] drm/i915: Clear PCODE_DATA1 on SNB+
Ville found out that the DATA1 register exists since SNB with some
scarce apparitions in the specs throughout the times. In his own words:
Also according to Bspec the mailbox data1 register already existed
since snb. The hsw cdclk change sequence also mentions that it should
be set to 0, but eg. the bdw IPS sequence doesn't mention it. I guess
in theory some pcode command might cause it to be clobbered, so I'm
thinking we should just explicitly set it to 0 for all platforms in
the pcode read/write functions
Suggested-by: Ville Syrjälä
Signed-off-by: Damien Lespiau
---
drivers/gpu/drm/i915/i915_reg.h | 2 +-
drivers/gpu/drm/i915/intel_pm.c | 3 +--
2 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 5a7adb1..56cccde 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -5952,8 +5952,8 @@ enum skl_disp_power_wells {
#define GEN6_PCODE_DATA0x138128
#define GEN6_PCODE_FREQ_IA_RATIO_SHIFT 8
#define GEN6_PCODE_FREQ_RING_RATIO_SHIFT 16
+#define GEN6_PCODE_DATA1 0x13812C
-#define GEN9_PCODE_DATA1 0x13812C
#define GEN9_PCODE_READ_MEM_LATENCY 0x6
#define GEN9_MEM_LATENCY_LEVEL_MASK 0xFF
#define GEN9_MEM_LATENCY_LEVEL_1_5_SHIFT 8
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 3f69f9a..7bc8f73 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -8716,8 +8716,7 @@ int sandybridge_pcode_read(struct drm_i915_private
*dev_priv, u8 mbox, u32 *val)
}
I915_WRITE(GEN6_PCODE_DATA, *val);
- if (INTEL_INFO(dev_priv)->gen >= 9)
- I915_WRITE(GEN9_PCODE_DATA1, 0);
+ I915_WRITE(GEN6_PCODE_DATA1, 0);
I915_WRITE(GEN6_PCODE_MAILBOX, GEN6_PCODE_READY | mbox);
if (wait_for((I915_READ(GEN6_PCODE_MAILBOX) & GEN6_PCODE_READY) == 0,
--
1.8.3.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 0/2] Couple of patches on top of the SKL latency retrieval
Ville had 2 comments on the SKL memory latency patch that are better addressed as separate patches on top. Patch 1: SKL unifies the plane hw and their latencies to fech data from memory is identical. So SKL only needs a single array. We save some room by using anymous union/structs. However, last time I tried that (shared DPLL state), Daniel didn't like it. So separate that into its own patch so it can be easily rejected. Patch 2: While it shouldn't cause any harm and DATA1 is likely unused by the pcode firmware, we don't actually know. So it's better to have as a separate patch, just in case a revert is needed (for testing or otherwise). -- Damien Damien Lespiau (2): drm/i915: Use anonymous union/struct to save space taken by latency values drm/i915: Clear PCODE_DATA1 on SNB+ drivers/gpu/drm/i915/i915_drv.h | 38 +- drivers/gpu/drm/i915/i915_reg.h | 2 +- drivers/gpu/drm/i915/intel_pm.c | 3 +-- 3 files changed, 23 insertions(+), 20 deletions(-) -- 1.8.3.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 1/2] drm/i915: Use anonymous union/struct to save space taken by latency values
Suggested-by: Ville Syrjälä
Signed-off-by: Damien Lespiau
---
drivers/gpu/drm/i915/i915_drv.h | 38 +-
1 file changed, 21 insertions(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 0baf7f3..8471d12 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1730,23 +1730,27 @@ struct drm_i915_private {
struct vlv_s0ix_state vlv_s0ix_state;
struct {
- /*
-* Raw watermark latency values:
-* in 0.1us units for WM0,
-* in 0.5us units for WM1+.
-*/
- /* primary */
- uint16_t pri_latency[5];
- /* sprite */
- uint16_t spr_latency[5];
- /* cursor */
- uint16_t cur_latency[5];
- /*
-* Raw watermark memory latency values
-* for SKL for all 8 levels
-* in 1us units.
-*/
- uint16_t skl_latency[8];
+ union {
+ /*
+* Raw watermark latency values:
+* in 0.1us units for WM0,
+* in 0.5us units for WM1+.
+*/
+ struct {
+ /* primary */
+ uint16_t pri_latency[5];
+ /* sprite */
+ uint16_t spr_latency[5];
+ /* cursor */
+ uint16_t cur_latency[5];
+ };
+
+ /*
+* Raw watermark memory latency values for SKL for all
+* 8 levels. In 1us units.
+*/
+ uint16_t skl_latency[8];
+ };
/*
* The skl_wm_values structure is a bit too big for stack
--
1.8.3.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 43/89] drm/i915/skl: Read the Memory Latency Values for WM computation
On Fri, Sep 05, 2014 at 11:42:32AM +0300, Ville Syrjälä wrote: > On Fri, Sep 05, 2014 at 09:29:33AM +0100, Damien Lespiau wrote: > > On Fri, Sep 05, 2014 at 11:25:30AM +0300, Ville Syrjälä wrote: > > > > +/* SKL GT Driver Mailbox registers for reading memory latencies */ > > > > +#define GEN9_MAILBOX_DATA1 0x13812C > > > > +#define GEN9_MAILBOX_READ_MEM_LAT(0x6) > > > > +#define GEN9_MAILBOX_READ_TIMEOUT150 > > > > > > Timeout not used anywhere. Also spec says 100us. > > > > > > > +#define GEN9_MEM_LAT_LEVEL_MASK 0xFF > > > > +#define GEN9_MEM_LAT_LEVEL_1_5_SHIFT 8 > > > > +#define GEN9_MEM_LAT_LEVEL_2_6_SHIFT 16 > > > > +#define GEN9_MEM_LAT_LEVEL_3_7_SHIFT 24 > > > > > > This stuff should be grouped along the other pcode register defines. > > > > Funny you mention this, I fixed those two in the v6 sent as reply > > yesterday. > > Well that's two down then. The other 2 items were marked as "patch on top is fine/better", so I guess it's now r-b material? In any case, addressed the 2 points you raised separately. http://lists.freedesktop.org/archives/intel-gfx/2014-September/052008.html -- Damien ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/bdw: cancel the SW turbo tasks before runtime suspending
(adding Daisy Sun to the conversation)
2014-09-04 18:07 GMT-03:00 Paulo Zanoni :
> From: Paulo Zanoni
>
> If we don't cancel them, we may end up running them while the device
> is runtime suspended, which will trigger lots and lots of WARNs on
> dmesg.
>
> Regression introduced by:
> commit c76bb61a71083b2d90504cc6d0dda2047c5d63ca
> Author: Daisy Sun
> Date: Mon Aug 11 11:08:38 2014 -0700
> drm/i915/bdw: BDW Software Turbo
>
> Testcase: igt/pm_rpm/gem-execbuf (you may have to run it a few times)
> Signed-off-by: Paulo Zanoni
> ---
> drivers/gpu/drm/i915/i915_drv.c | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 8ff3755..4ce217b 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1448,6 +1448,10 @@ static int intel_runtime_suspend(struct device *device)
> * intel_mark_idle().
> */
> cancel_work_sync(&dev_priv->rps.work);
> + if (dev_priv->rps.is_bdw_sw_turbo) {
> + del_timer_sync(&dev_priv->rps.sw_turbo.flip_timer);
> + cancel_work_sync(&dev_priv->rps.sw_turbo.work_max_freq);
> + }
> intel_runtime_pm_disable_interrupts(dev);
>
> ret = intel_suspend_complete(dev_priv);
> --
> 2.1.0
>
--
Paulo Zanoni
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: Enable full PPGTT on gen7
Use full PPGTT as the default option in gen7. Note that aliasing PPGTT is the default option for gen8 (see HAS_PPGTT). This may well come back to bite me later. Signed-off-by: Michel Thierry --- drivers/gpu/drm/i915/i915_gem_gtt.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index fc46647..30ab56a 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -59,7 +59,7 @@ static int sanitize_enable_ppgtt(struct drm_device *dev, int enable_ppgtt) return 0; } - return HAS_ALIASING_PPGTT(dev) ? 1 : 0; + return HAS_PPGTT(dev) ? 2 : HAS_ALIASING_PPGTT(dev) ? 1 : 0; } -- 2.0.3 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] Updated drm-intel-testing
Hi all, New -testing cycle with cool stuff: - final bits (again) for the rotation support (Sonika Jindal) - support bl_power in the intel backlight (Jani) - vdd handling improvements from Ville - i830M fixes from Ville - piles of prep work all over to make skl enabling just plug in (Damien, Sonika) - rename DP training defines to reflect latest edp standards, this touches all drm drivers supporting DP (Sonika Jindal) - cache edids during single detect cycle to avoid re-reading it for e.g. audio, from Chris - move w/a for registers which are stored in the hw context to the context init code (Arun&Damien) - edp panel power sequencer fixes, helps chv a lot (Ville) - piles of other chv fixes all over - much more paranoid pageflip handling with stall detection and better recovery from Chris - small things all over, as usual Happy testing! Cheers, Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915/dp: add missing \n in the TPS3 debug message
Signed-off-by: Jani Nikula
---
drivers/gpu/drm/i915/intel_dp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index f79473b3348b..ba96179eea0e 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3395,7 +3395,7 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
if (intel_dp->dpcd[DP_DPCD_REV] >= 0x12 &&
intel_dp->dpcd[DP_MAX_LANE_COUNT] & DP_TPS3_SUPPORTED) {
intel_dp->use_tps3 = true;
- DRM_DEBUG_KMS("Displayport TPS3 supported");
+ DRM_DEBUG_KMS("Displayport TPS3 supported\n");
} else
intel_dp->use_tps3 = false;
--
1.9.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 5/5] tests/kms_psr_sink_crc: Dry run with PSR disabled.
I really didn't like this implementation because I'm using the global
variable in the test case.
So I think also providing a define igt_skip_function_on_dryrun() if
(igt_dry_run) return 0 is more igt like.
For psr I need a way to run the testcases even when PSR is disabled to know
what to expect. So dryrun on psr test case means runs even with psr
disabled.
For any other feature could be something similar, run with feature
disabled. Or also it can be used for any other big testcase using a local
assert that do this plus igt_assert next so it can avoid fails and execute
the test to the end just to check all interactions.
So, what do you think? I could live with my old define on code though...
On Fri, Sep 5, 2014 at 1:23 AM, Daniel Vetter wrote:
> On Thu, Sep 04, 2014 at 06:22:51PM -0400, Rodrigo Vivi wrote:
> > This allows to run tests with psr disabled and know what to expect when
> > PSR is actually enabled.
> >
> > Signed-off-by: Rodrigo Vivi
>
> I don't really follow what this is useful for ... Can you please elaborate
> how this is used and how it helps debugging?
> -Daniel
>
> > ---
> > tests/kms_psr_sink_crc.c | 8
> > 1 file changed, 8 insertions(+)
> >
> > diff --git a/tests/kms_psr_sink_crc.c b/tests/kms_psr_sink_crc.c
> > index 51e54a7..1380ca4 100644
> > --- a/tests/kms_psr_sink_crc.c
> > +++ b/tests/kms_psr_sink_crc.c
> > @@ -72,6 +72,7 @@ typedef struct {
> > igt_display_t display;
> > struct igt_fb fb[2];
> > igt_plane_t *plane[2];
> > + bool running_with_psr_disabled;
> > } data_t;
> >
> > static const char *tests_str(enum tests test)
> > @@ -264,6 +265,9 @@ static bool psr_enabled(data_t *data)
> > FILE *file;
> > char str[4];
> >
> > + if (data->running_with_psr_disabled)
> > + return true;
> > +
> > file = igt_debugfs_fopen("i915_edp_psr_status", "r");
> > igt_require(file);
> >
> > @@ -284,6 +288,9 @@ static bool psr_active(data_t *data)
> > FILE *file;
> > char str[4];
> >
> > + if (data->running_with_psr_disabled)
> > + return true;
> > +
> > file = igt_debugfs_fopen("i915_edp_psr_status", "r");
> > igt_require(file);
> >
> > @@ -604,6 +611,7 @@ igt_main
> > kmstest_set_vt_graphics_mode();
> >
> > data.devid = intel_get_drm_devid(data.drm_fd);
> > + data.running_with_psr_disabled = igt_dry_run;
> >
> > igt_skip_on(!psr_enabled(&data));
> >
> > --
> > 1.9.3
> >
> > ___
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
--
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Fix irq enable tracking in driver load
On Wed, 27 Aug 2014 19:43:57 +0100 Chris Wilson wrote: > On Wed, Aug 27, 2014 at 10:11:34AM +0200, Daniel Vetter wrote: > > A bunch of warnings fire on some ->irq_postinstall hooks since those > > can enable interrupts (e.g. rps interrupts). And then our ordering > > self-checks fire and complain. > > > > To fix that set the tracking boolen before enabling the irqs with > > drm_irq_install. Quoting the discussion with Jesse why that's safe: > > > > On Tue, Aug 26, 2014 at 11:18 PM, Jesse Barnes > > wrote: > > > Yes, it might work, but if you look through the history, we set this > > > field carefully; first to true in the irq_init code, then to false only > > > after the irq_install completes. So I think your fragility arguments > > > apply to this change too. > > > > Well we've done it in 4 commits or so, but currently we have: > > > > - Set irqs_disabled to true early in driver load to make sure checks > > that. That's done in irq_init, which is totally not the function that > > enables interrupts, only the function that initializes all the vtables > > and similar things. We actually have a fairly sane naming scheme > > nowadays (not fully consistent ofc): _init is sw setup, > > _enable/_hw_init is the actual hw setup. That is done in > > 95f25beddba2ec9510b249740bacc11eca70cf75 > > > > - Set irqs_disabled to false right after the irqs are actually > > enabled. This is done in ed2e6df18935beb3d63613c50103bf9757b2aa85 > > > > So my change should only move the flag change over the ->preinstall > > and ->postinstall hooks. I've done a little audit and didn't spot > > anything amiss. Furthermore the runtime pm setup already clears > > irqs_disabled _before_ calling these two hooks. > > > > This regression has been introduced in > > > > commit ed2e6df18935beb3d63613c50103bf9757b2aa85 > > Author: Jesse Barnes > > Date: Fri Jun 20 09:39:36 2014 -0700 > > > > drm/i915: clear pm._irqs_disabled field after installing IRQs > > > > Cc: Jesse Barnes > > Cc: Oliver Hartkopp > > Tested-by: Oliver Hartkopp > > Signed-off-by: Daniel Vetter > Tested-by: Chris Wilson # gm45, ilk > > gm45 because it has been one of my troublesome machines in the past wrt > to modeset init. ilk because it exhibits this bug. > -Chris > Reviewed-by: Jesse Barnes -- Jesse Barnes, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: fix use-after-free in i915_drop_caches_set
With the new vma/ppgtt lifetime rules, the ppgtt (vm) could be removed
after i915_vma_unbind.
Use list_for_each_entry_safe() to prevent this use-after-free.
Found with gem_persistent_relocs and gem_evict_everything igt tests.
Cc: Daniel Vetter
Signed-off-by: Michel Thierry
---
drivers/gpu/drm/i915/i915_debugfs.c | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
b/drivers/gpu/drm/i915/i915_debugfs.c
index dd736c0..4b05cd8 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -3820,8 +3820,8 @@ i915_drop_caches_set(void *data, u64 val)
struct drm_device *dev = data;
struct drm_i915_private *dev_priv = dev->dev_private;
struct drm_i915_gem_object *obj, *next;
- struct i915_address_space *vm;
- struct i915_vma *vma, *x;
+ struct i915_address_space *vm, *x;
+ struct i915_vma *vma, *y;
int ret;
DRM_DEBUG("Dropping caches: 0x%08llx\n", val);
@@ -3842,8 +3842,9 @@ i915_drop_caches_set(void *data, u64 val)
i915_gem_retire_requests(dev);
if (val & DROP_BOUND) {
- list_for_each_entry(vm, &dev_priv->vm_list, global_link) {
- list_for_each_entry_safe(vma, x, &vm->inactive_list,
+ list_for_each_entry_safe(vm, x, &dev_priv->vm_list,
+ global_link) {
+ list_for_each_entry_safe(vma, y, &vm->inactive_list,
mm_list) {
if (vma->pin_count)
continue;
--
2.0.3
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] PCI: Add another ID for Intel GPU "spurious interrupt" quirk
On Fri, Aug 08, 2014 at 03:54:04PM +0200, Thomas Jarosch wrote: > New Intel G3258 CPU, new MSI board, same problem: > The GPU interrupt fired like crazy on monitor unplug. > > lspci output: > 00:02.0 VGA compatible controller: Intel Corporation Device 0402 (rev 06) > Subsystem: Micro-Star International Co., Ltd. Device 7817 > Flags: bus master, fast devsel, latency 0, IRQ 11 > > Signed-off-by: Thomas Jarosch > Tested-by: Thomas Jarosch > CC: sta...@vger.kernel.org # v3.4+ Dropping for now, please resend if necessary after resolving Daniel's question. > --- > drivers/pci/quirks.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c > index 80c2d01..d2ff39d 100644 > --- a/drivers/pci/quirks.c > +++ b/drivers/pci/quirks.c > @@ -2940,6 +2940,7 @@ static void disable_igfx_irq(struct pci_dev *dev) > DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x0102, disable_igfx_irq); > DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x010a, disable_igfx_irq); > DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x0152, disable_igfx_irq); > +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x0402, disable_igfx_irq); > > /* > * PCI devices which are on Intel chips can skip the 10ms delay > -- > 1.9.3 > ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: FBC flush nuke for BDW
2014-08-04 7:51 GMT-03:00 Rodrigo Vivi : > According to spec FBC on BDW and HSW are identical without any gaps. > So let's copy the nuke and let FBC really start compressing stuff. > > Without this patch we can verify with false color that nothing is being > compressed. With the nuke in place and false color it is possible > to see false color debugs. > > Unfortunatelly on some rings like BCS on BDW we have to avoid Bits 22:18 on > LRIs due to a high risk of hung. So, when using Blt ring for frontbuffer rend > cache would never been cleaned and FBC would stop compressing buffer. > One alternative is to cache clean on software frontbuffer tracking. > > v2: Fix rebase conflict. > v3: Do not clean cache on BCS ring. Instead use sw frontbuffer tracking. This patch causes lots of WARNs when running igt/pm_rpm/cursor and a few other similar test cases. Now the interesting this is that we're trying to do FBC flushing, but: - FBC is disabled by default - The screen is disabled - We're runtime suspended - We're touching cursors when we decide to flush FBC. This is the first WARN that happens: [ 123.581179] [drm:intel_runtime_suspend] Device suspended [ 123.680666] [ cut here ] [ 123.680697] WARNING: CPU: 1 PID: 1776 at drivers/gpu/drm/i915/intel_uncore.c:47 assert_device_not_suspended.isra.8+0x43/0x50 [i915]() [ 123.680699] Device suspended [ 123.680700] Modules linked in: intel_rapl x86_pkg_temp_thermal serio_raw intel_powerclamp efivars btusb iwlmvm iwlwifi mei_me mei int3403_thermal snd_hda_codec_hdmi snd_hda_intel snd_hda_controller i2c_designware_platform snd_hda_codec i2c_designware_core snd_hwdep snd_pcm_oss snd_mixer_oss acpi_pad snd_pcm snd_timer fuse nls_utf8 nls_cp437 vfat fat sd_mod ahci libahci i915 sdhci_pci e1000e drm_kms_helper drm sdhci_acpi sdhci [ 123.680733] CPU: 1 PID: 1776 Comm: pm_rpm Not tainted 3.17.0-rc2.1409051503pz+ #1100 [ 123.680736] Hardware name: Intel Corporation Broadwell Client platform/Wilson Beach SDS, BIOS BDW-E2R1.86C.0072.R03.1405072127 05/07/2014 [ 123.680738] 0009 88024174fab8 816f6d23 88024174fb00 [ 123.680742] 88024174faf0 8107b368 88003770 00050380 [ 123.680746] 00050380 880037700068 0001 88024174fb50 [ 123.680751] Call Trace: [ 123.680758] [] dump_stack+0x4d/0x66 [ 123.680763] [] warn_slowpath_common+0x78/0xa0 [ 123.680766] [] warn_slowpath_fmt+0x47/0x50 [ 123.680772] [] ? trace_hardirqs_on_caller+0x15d/0x200 [ 123.680791] [] assert_device_not_suspended.isra.8+0x43/0x50 [i915] [ 123.680809] [] gen8_write32+0x35/0x180 [i915] [ 123.680821] [] gen8_fbc_sw_flush+0x29/0x30 [i915] [ 123.680840] [] intel_frontbuffer_flush+0x7d/0x90 [i915] [ 123.680859] [] intel_cursor_plane_update+0x13b/0x150 [i915] [ 123.680876] [] setplane_internal+0x260/0x2a0 [drm] [ 123.680889] [] drm_mode_cursor_common+0x11e/0x310 [drm] [ 123.680904] [] drm_mode_cursor_ioctl+0x3c/0x40 [drm] [ 123.680914] [] drm_ioctl+0x1df/0x6a0 [drm] [ 123.680920] [] ? mutex_unlock+0x9/0x10 [ 123.680924] [] ? seq_read+0xb6/0x3e0 [ 123.680929] [] do_vfs_ioctl+0x2e0/0x4e0 [ 123.680934] [] ? sysret_check+0x1b/0x56 [ 123.680939] [] ? trace_hardirqs_on_caller+0x15d/0x200 [ 123.680943] [] SyS_ioctl+0x81/0xa0 [ 123.680947] [] system_call_fastpath+0x16/0x1b [ 123.680949] ---[ end trace 9f389682639d2eb9 ]--- [ 123.680953] [ cut here ] So, what we can/could do: 1 - Change gen8_fbc_sw_flush() do to nothing when FBC is disabled. 2 - At some point of the stack above, avoid calling everything else if the screen and/or plane and/or crtc is disabled. But when? At intel_cursor_plane_update? At intel_frontbuffer_flush? 3 - Don't call gen8_fbc_sw_flush if the frontbuffer_bits don't include the primary plane bits (since, AFAIR, FBC ignores the sprite and cursors, right?) Is anybody against any of the 3 items above? Also notice that the "cursor" subtest is not the only one that causes these WARNs, so I expect similar backtraces with other similar solutions for equivalent problems with the other planes. Thanks, Paulo > > Signed-off-by: Rodrigo Vivi > --- > drivers/gpu/drm/i915/i915_drv.h | 1 + > drivers/gpu/drm/i915/intel_display.c| 3 +++ > drivers/gpu/drm/i915/intel_pm.c | 10 ++ > drivers/gpu/drm/i915/intel_ringbuffer.c | 10 +- > 4 files changed, 23 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 2a372f2..25d7365 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -2713,6 +2713,7 @@ extern void intel_modeset_setup_hw_state(struct > drm_device *dev, > extern void i915_redisable_vga(struct drm_device *dev); > extern void i915_redisable_vga_power_on(struct drm_device *dev); > extern bool intel_fbc_enabled(struct drm_device *dev); > +extern void gen8_fbc_sw_flush(struct drm_device *dev, u32 valu
[Intel-gfx] [PATCH v2 13/16] drm/i915: Fix DVO 2x clock enable on 830M
From: Ville Syrjälä
The spec says:
"For the correct operation of the muxed DVO pins (GDEVSELB/ I2Cdata,
GIRDBY/I2CClk) and (GFRAMEB/DVI_Data, GTRDYB/DVI_Clk): Bit 31
(DPLL VCO Enable) and Bit 30 (2X Clock Enable) must be set to “1” in
both the DPLL A Control Register (06014h-06017h) and DPLL B Control
Register (06018h-0601Bh)."
The pipe A and B force quirks take care of DPLL_VCO_ENABLE, so we
just need a bit of special care to handle DPLL_DVO_2X_MODE.
v2: Recompute num_dvo_pipes on the spot, use PIPE_A/PIPE_B instead
of pipe/!pipe for the register offsets in disable (Daniel)
Add a comment about the ordering in enable and another one
about filtering out the DVO 2x bit in state readout
Signed-off-by: Ville Syrjälä
---
drivers/gpu/drm/i915/intel_display.c | 53 +---
1 file changed, 49 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index 842a5e1..6d6214a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1612,6 +1612,18 @@ static void chv_enable_pll(struct intel_crtc *crtc)
mutex_unlock(&dev_priv->dpio_lock);
}
+static int intel_num_dvo_pipes(struct drm_device *dev)
+{
+ struct intel_crtc *crtc;
+ int count = 0;
+
+ for_each_intel_crtc(dev, crtc)
+ count += crtc->active &&
+ intel_pipe_has_type(&crtc->base, INTEL_OUTPUT_DVO);
+
+ return count;
+}
+
static void i9xx_enable_pll(struct intel_crtc *crtc)
{
struct drm_device *dev = crtc->base.dev;
@@ -1628,7 +1640,18 @@ static void i9xx_enable_pll(struct intel_crtc *crtc)
if (IS_MOBILE(dev) && !IS_I830(dev))
assert_panel_unlocked(dev_priv, crtc->pipe);
- I915_WRITE(reg, dpll);
+ /* Enable DVO 2x clock on both PLLs if necessary */
+ if (IS_I830(dev) && intel_num_dvo_pipes(dev) > 0) {
+ /*
+* It appears to be important that we don't enable this
+* for the current pipe before otherwise configuring the
+* PLL. No idea how this should be handled if multiple
+* DVO outputs are enabled simultaneosly.
+*/
+ dpll |= DPLL_DVO_2X_MODE;
+ I915_WRITE(DPLL(!crtc->pipe),
+ I915_READ(DPLL(!crtc->pipe)) | DPLL_DVO_2X_MODE);
+ }
/* Wait for the clocks to stabilize. */
POSTING_READ(reg);
@@ -1667,8 +1690,22 @@ static void i9xx_enable_pll(struct intel_crtc *crtc)
*
* Note! This is for pre-ILK only.
*/
-static void i9xx_disable_pll(struct drm_i915_private *dev_priv, enum pipe pipe)
+static void i9xx_disable_pll(struct intel_crtc *crtc)
{
+ struct drm_device *dev = crtc->base.dev;
+ struct drm_i915_private *dev_priv = dev->dev_private;
+ enum pipe pipe = crtc->pipe;
+
+ /* Disable DVO 2x clock on both PLLs if necessary */
+ if (IS_I830(dev) &&
+ intel_pipe_has_type(&crtc->base, INTEL_OUTPUT_DVO) &&
+ intel_num_dvo_pipes(dev) == 1) {
+ I915_WRITE(DPLL(PIPE_B),
+ I915_READ(DPLL(PIPE_B)) & ~DPLL_DVO_2X_MODE);
+ I915_WRITE(DPLL(PIPE_A),
+ I915_READ(DPLL(PIPE_A)) & ~DPLL_DVO_2X_MODE);
+ }
+
/* Don't disable pipe or pipe PLLs if needed */
if ((pipe == PIPE_A && dev_priv->quirks & QUIRK_PIPEA_FORCE) ||
(pipe == PIPE_B && dev_priv->quirks & QUIRK_PIPEB_FORCE))
@@ -4940,7 +4977,7 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
else if (IS_VALLEYVIEW(dev))
vlv_disable_pll(dev_priv, pipe);
else
- i9xx_disable_pll(dev_priv, pipe);
+ i9xx_disable_pll(intel_crtc);
}
if (!IS_GEN2(dev))
@@ -5944,7 +5981,7 @@ static void i8xx_update_pll(struct intel_crtc *crtc,
dpll |= PLL_P2_DIVIDE_BY_4;
}
- if (intel_pipe_has_type(&crtc->base, INTEL_OUTPUT_DVO))
+ if (!IS_I830(dev) && intel_pipe_has_type(&crtc->base, INTEL_OUTPUT_DVO))
dpll |= DPLL_DVO_2X_MODE;
if (intel_pipe_has_type(&crtc->base, INTEL_OUTPUT_LVDS) &&
@@ -6450,6 +6487,14 @@ static bool i9xx_get_pipe_config(struct intel_crtc *crtc,
}
pipe_config->dpll_hw_state.dpll = I915_READ(DPLL(crtc->pipe));
if (!IS_VALLEYVIEW(dev)) {
+ /*
+* DPLL_DVO_2X_MODE must be enabled for both DPLLs
+* on 830. Filter it out here so that we don't
+* report errors due to that.
+*/
+ if (IS_I830(dev))
+ pipe_config->dpll_hw_state.dpll &= ~DPLL_DVO_2X_MODE;
+
pipe_config->dpll_hw_state.fp0 = I915_READ(FP0(crtc->pipe));
pipe_config->dpll_hw_state.fp1 = I915_READ(FP1(crtc->pipe));
} els
[Intel-gfx] [PATCH] drm/i915: Limit the watermark to at least 8 entries on gen2/3
From: Ville Syrjälä 830 is very unhappy of the watermark value is too low (indicating a very high watermark in fact, ie. memory fetch will occur with an almost full FIFO). Limit the watermark value to at least 8 cache lines. That also matches the burst size we use on most platforms. BSpec seems to indicate we should limit the watermark to 'burst size + 1'. But on gen4 we already use a hardcoded 8 as the watermark value (as the spec says we should), so just use 8 as the limit on gen2/3 as well. Signed-off-by: Ville Syrjälä --- drivers/gpu/drm/i915/intel_pm.c | 11 +++ 1 file changed, 11 insertions(+) diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 45f71e6..5b683e8 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -1070,6 +1070,17 @@ static unsigned long intel_calculate_wm(unsigned long clock_in_khz, wm_size = wm->max_wm; if (wm_size <= 0) wm_size = wm->default_wm; + + /* +* Bspec seems to indicate that the value shouldn't be lower than +* 'burst size + 1'. Certainly 830 is quite unhappy with low values. +* Lets go for 8 which is the burst size since certain platforms +* already use a hardcoded 8 (which is what the spec says should be +* done). +*/ + if (wm_size <= 8) + wm_size = 8; + return wm_size; } -- 1.8.5.5 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Limit the watermark to at least 8 entries on gen2/3
On 05.09.2014 20:54, ville.syrj...@linux.intel.com wrote: From: Ville Syrjälä 830 is very unhappy of the watermark value is too low (indicating a very high watermark in fact, ie. memory fetch will occur with an almost full FIFO). Limit the watermark value to at least 8 cache lines. That also matches the burst size we use on most platforms. BSpec seems to indicate we should limit the watermark to 'burst size + 1'. But on gen4 we already use a hardcoded 8 as the watermark value (as the spec says we should), so just use 8 as the limit on gen2/3 as well. /* snip */ That's certainly one way to fix it. Nothing against it. It might be more conservative to make this fix only in the calling context of the 830 and 845 specific functions as I don't know whether the other chipsets have similar issues. I.e. one level up in the call-chain. This being said, I can certainly confirm that the suggested modification works on i830. One way or another, it seems to me that drm-intel-nightly which I pulled had a couple of more serious issues. I didn't even get X running, and monitor detection did not seem to work reliable, too. I had the same kind of instability on the external monitor for the console Ville's patches fixed so nicely, so it seems to me that there is something else that is missing. The new DVO code was definitely there, I checked that. Thanks, Thomas ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: FBC flush nuke for BDW
On Fri, Sep 5, 2014 at 11:28 AM, Paulo Zanoni wrote: > 2014-08-04 7:51 GMT-03:00 Rodrigo Vivi : > > According to spec FBC on BDW and HSW are identical without any gaps. > > So let's copy the nuke and let FBC really start compressing stuff. > > > > Without this patch we can verify with false color that nothing is being > > compressed. With the nuke in place and false color it is possible > > to see false color debugs. > > > > Unfortunatelly on some rings like BCS on BDW we have to avoid Bits 22:18 > on > > LRIs due to a high risk of hung. So, when using Blt ring for frontbuffer > rend > > cache would never been cleaned and FBC would stop compressing buffer. > > One alternative is to cache clean on software frontbuffer tracking. > > > > v2: Fix rebase conflict. > > v3: Do not clean cache on BCS ring. Instead use sw frontbuffer tracking. > > This patch causes lots of WARNs when running igt/pm_rpm/cursor and a > few other similar test cases. Now the interesting this is that we're > trying to do FBC flushing, but: > As I told you on the IRC already, thank you for finding this. It is good to get reasons and arguments. > - FBC is disabled by default Yeah, we need to fix it although it just matter much > - The screen is disabled - We're runtime suspended > These are another error. Not caused by this patch. Why frontbuffer_flush is being called with screen disabled or on runtime suspend? > - We're touching cursors when we decide to flush FBC. > This is not actually a flush. But cleaning the cache to allow compression restart. I believe we could have some variable to indicate when we touched ring flush than on next frontbuffer we do the mmio cache clean. > > This is the first WARN that happens: > > [ 123.581179] [drm:intel_runtime_suspend] Device suspended > [ 123.680666] [ cut here ] > [ 123.680697] WARNING: CPU: 1 PID: 1776 at > drivers/gpu/drm/i915/intel_uncore.c:47 > assert_device_not_suspended.isra.8+0x43/0x50 [i915]() > [ 123.680699] Device suspended > [ 123.680700] Modules linked in: intel_rapl x86_pkg_temp_thermal > serio_raw intel_powerclamp efivars btusb iwlmvm iwlwifi mei_me mei > int3403_thermal snd_hda_codec_hdmi snd_hda_intel snd_hda_controller > i2c_designware_platform snd_hda_codec i2c_designware_core snd_hwdep > snd_pcm_oss snd_mixer_oss acpi_pad snd_pcm snd_timer fuse nls_utf8 > nls_cp437 vfat fat sd_mod ahci libahci i915 sdhci_pci e1000e > drm_kms_helper drm sdhci_acpi sdhci > [ 123.680733] CPU: 1 PID: 1776 Comm: pm_rpm Not tainted > 3.17.0-rc2.1409051503pz+ #1100 > [ 123.680736] Hardware name: Intel Corporation Broadwell Client > platform/Wilson Beach SDS, BIOS BDW-E2R1.86C.0072.R03.1405072127 > 05/07/2014 > [ 123.680738] 0009 88024174fab8 816f6d23 > 88024174fb00 > [ 123.680742] 88024174faf0 8107b368 88003770 > 00050380 > [ 123.680746] 00050380 880037700068 0001 > 88024174fb50 > [ 123.680751] Call Trace: > [ 123.680758] [] dump_stack+0x4d/0x66 > [ 123.680763] [] warn_slowpath_common+0x78/0xa0 > [ 123.680766] [] warn_slowpath_fmt+0x47/0x50 > [ 123.680772] [] ? trace_hardirqs_on_caller+0x15d/0x200 > [ 123.680791] [] > assert_device_not_suspended.isra.8+0x43/0x50 [i915] > [ 123.680809] [] gen8_write32+0x35/0x180 [i915] > [ 123.680821] [] gen8_fbc_sw_flush+0x29/0x30 [i915] > [ 123.680840] [] intel_frontbuffer_flush+0x7d/0x90 > [i915] > [ 123.680859] [] > intel_cursor_plane_update+0x13b/0x150 [i915] > [ 123.680876] [] setplane_internal+0x260/0x2a0 [drm] > [ 123.680889] [] drm_mode_cursor_common+0x11e/0x310 > [drm] > [ 123.680904] [] drm_mode_cursor_ioctl+0x3c/0x40 [drm] > [ 123.680914] [] drm_ioctl+0x1df/0x6a0 [drm] > [ 123.680920] [] ? mutex_unlock+0x9/0x10 > [ 123.680924] [] ? seq_read+0xb6/0x3e0 > [ 123.680929] [] do_vfs_ioctl+0x2e0/0x4e0 > [ 123.680934] [] ? sysret_check+0x1b/0x56 > [ 123.680939] [] ? trace_hardirqs_on_caller+0x15d/0x200 > [ 123.680943] [] SyS_ioctl+0x81/0xa0 > [ 123.680947] [] system_call_fastpath+0x16/0x1b > [ 123.680949] ---[ end trace 9f389682639d2eb9 ]--- > [ 123.680953] [ cut here ] > > > So, what we can/could do: > 1 - Change gen8_fbc_sw_flush() do to nothing when FBC is disabled. > Agreed! > 2 - At some point of the stack above, avoid calling everything else if > the screen and/or plane and/or crtc is disabled. But when? At > intel_cursor_plane_update? At intel_frontbuffer_flush? > Good question! > 3 - Don't call gen8_fbc_sw_flush if the frontbuffer_bits don't include > the primary plane bits (since, AFAIR, FBC ignores the sprite and > cursors, right?) > not sure. but we could try check primary bits + do only when we did a ring flush on blt ring. Actually other ring than render. > > Is anybody against any of the 3 items above? > should I change this patch itself or provide another one on top? > > Also notice that the "cursor" subtest is not the only one that c
[Intel-gfx] [PATCH -v4 1/4] drm/i915: create struct intel_plane_state
From: Gustavo Padovan
This new struct will be the storage of src and dst coordinates
between the check and commit stages of a plane update.
Signed-off-by: Gustavo Padovan
---
drivers/gpu/drm/i915/intel_drv.h | 12
1 file changed, 12 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index d727d20..be668ea 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -34,6 +34,7 @@
#include
#include
#include
+#include
/**
* _wait_for - magic (register) wait macro
@@ -237,6 +238,17 @@ typedef struct dpll {
int p;
} intel_clock_t;
+struct intel_plane_state {
+ struct drm_crtc *crtc;
+ struct drm_framebuffer *fb;
+ struct drm_rect src;
+ struct drm_rect dst;
+ struct drm_rect clip;
+ struct drm_rect orig_src;
+ struct drm_rect orig_dst;
+ bool visible;
+};
+
struct intel_plane_config {
bool tiled;
int size;
--
1.9.3
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH -v4 2/4] drm/i915: split intel_update_plane into check() and commit()
From: Gustavo Padovan
Due to the upcoming atomic modesetting feature we need to separate
some update functions into a check step that can fail and a commit
step that should, ideally, never fail.
This commit splits intel_update_plane() and its commit part can still
fail due to the fb pinning procedure.
Signed-off-by: Gustavo Padovan
---
drivers/gpu/drm/i915/intel_sprite.c | 233 ++--
1 file changed, 141 insertions(+), 92 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_sprite.c
b/drivers/gpu/drm/i915/intel_sprite.c
index 07a74ef..a4306cf 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -845,57 +845,24 @@ static bool colorkey_enabled(struct intel_plane
*intel_plane)
}
static int
-intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
- struct drm_framebuffer *fb, int crtc_x, int crtc_y,
- unsigned int crtc_w, unsigned int crtc_h,
- uint32_t src_x, uint32_t src_y,
- uint32_t src_w, uint32_t src_h)
+intel_check_sprite_plane(struct drm_plane *plane,
+struct intel_plane_state *state)
{
- struct drm_device *dev = plane->dev;
- struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+ struct intel_crtc *intel_crtc = to_intel_crtc(state->crtc);
struct intel_plane *intel_plane = to_intel_plane(plane);
- enum pipe pipe = intel_crtc->pipe;
+ struct drm_framebuffer *fb = state->fb;
struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
struct drm_i915_gem_object *obj = intel_fb->obj;
- struct drm_i915_gem_object *old_obj = intel_plane->obj;
- int ret;
- bool primary_enabled;
- bool visible;
+ int crtc_x, crtc_y;
+ unsigned int crtc_w, crtc_h;
+ uint32_t src_x, src_y, src_w, src_h;
+ struct drm_rect *src = &state->src;
+ struct drm_rect *dst = &state->dst;
+ struct drm_rect *orig_src = &state->orig_src;
+ const struct drm_rect *clip = &state->clip;
int hscale, vscale;
int max_scale, min_scale;
int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
- struct drm_rect src = {
- /* sample coordinates in 16.16 fixed point */
- .x1 = src_x,
- .x2 = src_x + src_w,
- .y1 = src_y,
- .y2 = src_y + src_h,
- };
- struct drm_rect dst = {
- /* integer pixels */
- .x1 = crtc_x,
- .x2 = crtc_x + crtc_w,
- .y1 = crtc_y,
- .y2 = crtc_y + crtc_h,
- };
- const struct drm_rect clip = {
- .x2 = intel_crtc->active ? intel_crtc->config.pipe_src_w : 0,
- .y2 = intel_crtc->active ? intel_crtc->config.pipe_src_h : 0,
- };
- const struct {
- int crtc_x, crtc_y;
- unsigned int crtc_w, crtc_h;
- uint32_t src_x, src_y, src_w, src_h;
- } orig = {
- .crtc_x = crtc_x,
- .crtc_y = crtc_y,
- .crtc_w = crtc_w,
- .crtc_h = crtc_h,
- .src_x = src_x,
- .src_y = src_y,
- .src_w = src_w,
- .src_h = src_h,
- };
/* Don't modify another pipe's plane */
if (intel_plane->pipe != intel_crtc->pipe) {
@@ -927,55 +894,55 @@ intel_update_plane(struct drm_plane *plane, struct
drm_crtc *crtc,
max_scale = intel_plane->max_downscale << 16;
min_scale = intel_plane->can_scale ? 1 : (1 << 16);
- drm_rect_rotate(&src, fb->width << 16, fb->height << 16,
+ drm_rect_rotate(src, fb->width << 16, fb->height << 16,
intel_plane->rotation);
- hscale = drm_rect_calc_hscale_relaxed(&src, &dst, min_scale, max_scale);
+ hscale = drm_rect_calc_hscale_relaxed(src, dst, min_scale, max_scale);
BUG_ON(hscale < 0);
- vscale = drm_rect_calc_vscale_relaxed(&src, &dst, min_scale, max_scale);
+ vscale = drm_rect_calc_vscale_relaxed(src, dst, min_scale, max_scale);
BUG_ON(vscale < 0);
- visible = drm_rect_clip_scaled(&src, &dst, &clip, hscale, vscale);
+ state->visible = drm_rect_clip_scaled(src, dst, clip, hscale, vscale);
- crtc_x = dst.x1;
- crtc_y = dst.y1;
- crtc_w = drm_rect_width(&dst);
- crtc_h = drm_rect_height(&dst);
+ crtc_x = dst->x1;
+ crtc_y = dst->y1;
+ crtc_w = drm_rect_width(dst);
+ crtc_h = drm_rect_height(dst);
- if (visible) {
+ if (state->visible) {
/* check again in case clipping clamped the results */
- hscale = drm_rect_calc_hscale(&src, &dst, min_scale, max_scale);
+ hscale = drm_rect_calc_hscale(src, dst, min_scale, max_scale);
if (hscale < 0) {
DRM_DEBUG_KMS("Horizontal scaling factor
[Intel-gfx] [PATCH -v4 0/4] split plane's updates functions into check() and commit()
From: Gustavo Padovan This is the beginning of the work to prepare i915 for the upcoming atomic modesetting API. Here we split the plane update fucntions in the check and commit states. v2: use struct intel_plane_state to keep states between check and commit stages. v3: take Ville's comments: - rename pstate to state - get rid of non-drm_rect coordinates in intel_plane_state - keep 'clip' const v4: take more Ville's comments: - populates orig_dst and orig_src too - use orig_dst coordinates to program the cursor plane Gustavo Padovan (4): drm/i915: create struct intel_plane_state drm/i915: split intel_update_plane into check() and commit() drm/i915: split intel_cursor_plane_update() into check() and commit() drm/i915: split intel_primary_plane_setplane() into check() and commit() drivers/gpu/drm/i915/intel_display.c | 240 +-- drivers/gpu/drm/i915/intel_drv.h | 12 ++ drivers/gpu/drm/i915/intel_sprite.c | 233 -- 3 files changed, 298 insertions(+), 187 deletions(-) -- 1.9.3 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH -v4 3/4] drm/i915: split intel_cursor_plane_update() into check() and commit()
From: Gustavo Padovan
Due to the upcoming atomic modesetting feature we need to separate
some update functions into a check step that can fail and a commit
step that should, ideally, never fail.
The commit part can still fail, but that should be solved in another
upcoming patch.
Signed-off-by: Gustavo Padovan
---
drivers/gpu/drm/i915/intel_display.c | 107 +++
1 file changed, 70 insertions(+), 37 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index 22d3902..0f036cd 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11896,51 +11896,42 @@ intel_cursor_plane_disable(struct drm_plane *plane)
}
static int
-intel_cursor_plane_update(struct drm_plane *plane, struct drm_crtc *crtc,
- struct drm_framebuffer *fb, int crtc_x, int crtc_y,
- unsigned int crtc_w, unsigned int crtc_h,
- uint32_t src_x, uint32_t src_y,
- uint32_t src_w, uint32_t src_h)
+intel_check_cursor_plane(struct drm_plane *plane,
+struct intel_plane_state *state)
{
- struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
- struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
- struct drm_i915_gem_object *obj = intel_fb->obj;
- struct drm_rect dest = {
- /* integer pixels */
- .x1 = crtc_x,
- .y1 = crtc_y,
- .x2 = crtc_x + crtc_w,
- .y2 = crtc_y + crtc_h,
- };
- struct drm_rect src = {
- /* 16.16 fixed point */
- .x1 = src_x,
- .y1 = src_y,
- .x2 = src_x + src_w,
- .y2 = src_y + src_h,
- };
- const struct drm_rect clip = {
- /* integer pixels */
- .x2 = intel_crtc->active ? intel_crtc->config.pipe_src_w : 0,
- .y2 = intel_crtc->active ? intel_crtc->config.pipe_src_h : 0,
- };
- bool visible;
- int ret;
+ struct drm_crtc *crtc = state->crtc;
+ struct drm_framebuffer *fb = state->fb;
+ struct drm_rect *dest = &state->dst;
+ struct drm_rect *src = &state->src;
+ const struct drm_rect *clip = &state->clip;
- ret = drm_plane_helper_check_update(plane, crtc, fb,
- &src, &dest, &clip,
+ return drm_plane_helper_check_update(plane, crtc, fb,
+ src, dest, clip,
DRM_PLANE_HELPER_NO_SCALING,
DRM_PLANE_HELPER_NO_SCALING,
- true, true, &visible);
- if (ret)
- return ret;
+ true, true, &state->visible);
+}
- crtc->cursor_x = crtc_x;
- crtc->cursor_y = crtc_y;
+static int
+intel_commit_cursor_plane(struct drm_plane *plane,
+ struct intel_plane_state *state)
+{
+ struct drm_crtc *crtc = state->crtc;
+ struct drm_framebuffer *fb = state->fb;
+ struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+ struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
+ struct drm_i915_gem_object *obj = intel_fb->obj;
+ struct drm_rect *dest = &state->dst;
+ int crtc_w, crtc_h;
+
+ crtc->cursor_x = state->orig_dst.x1;
+ crtc->cursor_y = state->orig_dst.y1;
if (fb != crtc->cursor->fb) {
+ crtc_w = drm_rect_width(&state->orig_dst);
+ crtc_h = drm_rect_height(&state->orig_dst);
return intel_crtc_cursor_set_obj(crtc, obj, crtc_w, crtc_h);
} else {
- intel_crtc_update_cursor(crtc, visible);
+ intel_crtc_update_cursor(crtc, state->visible);
intel_frontbuffer_flip(crtc->dev,
INTEL_FRONTBUFFER_CURSOR(intel_crtc->pipe));
@@ -11948,6 +11939,48 @@ intel_cursor_plane_update(struct drm_plane *plane,
struct drm_crtc *crtc,
return 0;
}
}
+
+static int
+intel_cursor_plane_update(struct drm_plane *plane, struct drm_crtc *crtc,
+ struct drm_framebuffer *fb, int crtc_x, int crtc_y,
+ unsigned int crtc_w, unsigned int crtc_h,
+ uint32_t src_x, uint32_t src_y,
+ uint32_t src_w, uint32_t src_h)
+{
+ struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+ struct intel_plane_state state;
+ int ret;
+
+ state.crtc = crtc;
+ state.fb = fb;
+
+ /* sample coordinates in 16.16 fixed point */
+ state.src.x1 = src_x;
+ state.src.x2 = src_x + src_w;
+ state.src.y1 = src_y;
+ state.src.y2 = src_y + src_h;
+
+ /* integer pixels */
+ state.dst.x1 = crtc_x;
+
[Intel-gfx] [PATCH -v4 4/4] drm/i915: split intel_primary_plane_setplane() into check() and commit()
From: Gustavo Padovan
As a preparation for atomic updates we need to split the code to check
everything we are going to commit first. This patch starts the work to
split intel_primary_plane_setplane() into check() and commit() parts.
More work is expected on this to get a better split of the two steps.
Ideally the commit() step should never fail.
Signed-off-by: Gustavo Padovan
---
drivers/gpu/drm/i915/intel_display.c | 133 ---
1 file changed, 75 insertions(+), 58 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index 0f036cd..39c57c2 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11663,63 +11663,37 @@ disable_unpin:
}
static int
-intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc,
-struct drm_framebuffer *fb, int crtc_x, int crtc_y,
-unsigned int crtc_w, unsigned int crtc_h,
-uint32_t src_x, uint32_t src_y,
-uint32_t src_w, uint32_t src_h)
+intel_check_primary_plane(struct drm_plane *plane,
+ struct intel_plane_state *state)
{
+ struct drm_crtc *crtc = state->crtc;
+ struct drm_framebuffer *fb = state->fb;
+ struct drm_rect *dest = &state->dst;
+ struct drm_rect *src = &state->src;
+ const struct drm_rect *clip = &state->clip;
+
+ return drm_plane_helper_check_update(plane, crtc, fb,
+ src, dest, clip,
+ DRM_PLANE_HELPER_NO_SCALING,
+ DRM_PLANE_HELPER_NO_SCALING,
+ false, true, &state->visible);
+}
+
+static int
+intel_commit_primary_plane(struct drm_plane *plane,
+ struct intel_plane_state *state)
+{
+ struct drm_crtc *crtc = state->crtc;
+ struct drm_framebuffer *fb = state->fb;
struct drm_device *dev = crtc->dev;
struct drm_i915_private *dev_priv = dev->dev_private;
struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
struct drm_i915_gem_object *obj = intel_fb_obj(fb);
struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb);
- struct drm_rect dest = {
- /* integer pixels */
- .x1 = crtc_x,
- .y1 = crtc_y,
- .x2 = crtc_x + crtc_w,
- .y2 = crtc_y + crtc_h,
- };
- struct drm_rect src = {
- /* 16.16 fixed point */
- .x1 = src_x,
- .y1 = src_y,
- .x2 = src_x + src_w,
- .y2 = src_y + src_h,
- };
- const struct drm_rect clip = {
- /* integer pixels */
- .x2 = intel_crtc->active ? intel_crtc->config.pipe_src_w : 0,
- .y2 = intel_crtc->active ? intel_crtc->config.pipe_src_h : 0,
- };
- const struct {
- int crtc_x, crtc_y;
- unsigned int crtc_w, crtc_h;
- uint32_t src_x, src_y, src_w, src_h;
- } orig = {
- .crtc_x = crtc_x,
- .crtc_y = crtc_y,
- .crtc_w = crtc_w,
- .crtc_h = crtc_h,
- .src_x = src_x,
- .src_y = src_y,
- .src_w = src_w,
- .src_h = src_h,
- };
struct intel_plane *intel_plane = to_intel_plane(plane);
- bool visible;
+ struct drm_rect *src = &state->src;
int ret;
- ret = drm_plane_helper_check_update(plane, crtc, fb,
- &src, &dest, &clip,
- DRM_PLANE_HELPER_NO_SCALING,
- DRM_PLANE_HELPER_NO_SCALING,
- false, true, &visible);
-
- if (ret)
- return ret;
-
/*
* If the CRTC isn't enabled, we're just pinning the framebuffer,
* updating the fb pointer, and returning without touching the
@@ -11754,7 +11728,7 @@ intel_primary_plane_setplane(struct drm_plane *plane,
struct drm_crtc *crtc,
* happens if userspace explicitly disables the plane by passing fb=0
* because plane->fb still gets set and pinned.
*/
- if (!visible) {
+ if (!state->visible) {
mutex_lock(&dev->struct_mutex);
/*
@@ -11801,7 +11775,7 @@ intel_primary_plane_setplane(struct drm_plane *plane,
struct drm_crtc *crtc,
intel_disable_fbc(dev);
}
}
- ret = intel_pipe_set_base(crtc, src.x1, src.y1, fb);
+ ret = intel_pipe_set_base(crtc, src->x1, src->y1, fb);
if (ret)
return ret;
@@ -11809,19 +11783,62 @@ intel_primary_plane
[Intel-gfx] [PATCH -v5 3/4] drm/i915: split intel_cursor_plane_update() into check() and commit()
From: Gustavo Padovan
Due to the upcoming atomic modesetting feature we need to separate
some update functions into a check step that can fail and a commit
step that should, ideally, never fail.
The commit part can still fail, but that should be solved in another
upcoming patch.
Signed-off-by: Gustavo Padovan
---
v5: fix compile warning of not used var
drivers/gpu/drm/i915/intel_display.c | 106 +++
1 file changed, 69 insertions(+), 37 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index 22d3902..f41d25c 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11896,51 +11896,41 @@ intel_cursor_plane_disable(struct drm_plane *plane)
}
static int
-intel_cursor_plane_update(struct drm_plane *plane, struct drm_crtc *crtc,
- struct drm_framebuffer *fb, int crtc_x, int crtc_y,
- unsigned int crtc_w, unsigned int crtc_h,
- uint32_t src_x, uint32_t src_y,
- uint32_t src_w, uint32_t src_h)
+intel_check_cursor_plane(struct drm_plane *plane,
+struct intel_plane_state *state)
{
- struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
- struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
- struct drm_i915_gem_object *obj = intel_fb->obj;
- struct drm_rect dest = {
- /* integer pixels */
- .x1 = crtc_x,
- .y1 = crtc_y,
- .x2 = crtc_x + crtc_w,
- .y2 = crtc_y + crtc_h,
- };
- struct drm_rect src = {
- /* 16.16 fixed point */
- .x1 = src_x,
- .y1 = src_y,
- .x2 = src_x + src_w,
- .y2 = src_y + src_h,
- };
- const struct drm_rect clip = {
- /* integer pixels */
- .x2 = intel_crtc->active ? intel_crtc->config.pipe_src_w : 0,
- .y2 = intel_crtc->active ? intel_crtc->config.pipe_src_h : 0,
- };
- bool visible;
- int ret;
+ struct drm_crtc *crtc = state->crtc;
+ struct drm_framebuffer *fb = state->fb;
+ struct drm_rect *dest = &state->dst;
+ struct drm_rect *src = &state->src;
+ const struct drm_rect *clip = &state->clip;
- ret = drm_plane_helper_check_update(plane, crtc, fb,
- &src, &dest, &clip,
+ return drm_plane_helper_check_update(plane, crtc, fb,
+ src, dest, clip,
DRM_PLANE_HELPER_NO_SCALING,
DRM_PLANE_HELPER_NO_SCALING,
- true, true, &visible);
- if (ret)
- return ret;
+ true, true, &state->visible);
+}
- crtc->cursor_x = crtc_x;
- crtc->cursor_y = crtc_y;
+static int
+intel_commit_cursor_plane(struct drm_plane *plane,
+ struct intel_plane_state *state)
+{
+ struct drm_crtc *crtc = state->crtc;
+ struct drm_framebuffer *fb = state->fb;
+ struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+ struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
+ struct drm_i915_gem_object *obj = intel_fb->obj;
+ int crtc_w, crtc_h;
+
+ crtc->cursor_x = state->orig_dst.x1;
+ crtc->cursor_y = state->orig_dst.y1;
if (fb != crtc->cursor->fb) {
+ crtc_w = drm_rect_width(&state->orig_dst);
+ crtc_h = drm_rect_height(&state->orig_dst);
return intel_crtc_cursor_set_obj(crtc, obj, crtc_w, crtc_h);
} else {
- intel_crtc_update_cursor(crtc, visible);
+ intel_crtc_update_cursor(crtc, state->visible);
intel_frontbuffer_flip(crtc->dev,
INTEL_FRONTBUFFER_CURSOR(intel_crtc->pipe));
@@ -11948,6 +11938,48 @@ intel_cursor_plane_update(struct drm_plane *plane,
struct drm_crtc *crtc,
return 0;
}
}
+
+static int
+intel_cursor_plane_update(struct drm_plane *plane, struct drm_crtc *crtc,
+ struct drm_framebuffer *fb, int crtc_x, int crtc_y,
+ unsigned int crtc_w, unsigned int crtc_h,
+ uint32_t src_x, uint32_t src_y,
+ uint32_t src_w, uint32_t src_h)
+{
+ struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+ struct intel_plane_state state;
+ int ret;
+
+ state.crtc = crtc;
+ state.fb = fb;
+
+ /* sample coordinates in 16.16 fixed point */
+ state.src.x1 = src_x;
+ state.src.x2 = src_x + src_w;
+ state.src.y1 = src_y;
+ state.src.y2 = src_y + src_h;
+
+ /* integer pixels */
+ state.dst.x1 = crtc_x;
+ st
[Intel-gfx] [PATCH 1/2] drm/i915: Only flush fbc on sw when fbc is enabled.
Avoid touching fbc register when fbc is disabled. Cc: Paulo Zanoni Signed-off-by: Rodrigo Vivi --- drivers/gpu/drm/i915/intel_pm.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 45f71e6..c3d3439 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -352,6 +352,9 @@ void gen8_fbc_sw_flush(struct drm_device *dev, u32 value) if (!IS_GEN8(dev)) return; + if (!intel_fbc_enabled(dev)) + return; + I915_WRITE(MSG_FBC_REND_STATE, value); } -- 1.9.3 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 2/2] drm/i915: Minimize the huge amount of unecessary fbc sw cache clean.
The sw cache clean on gen8 is a tempoorary workaround because we cannot
set cache clean on blt ring with risk of hungs. So we are doing the cache clean
on sw.
However we are doing much more than needed. Not only when using blt ring.
So, with this extra w/a we minimize the ammount of cache cleans and call it only
on same cases that it was being called on gen7.
fbc.need_sw_cache_clean works in the opposite information direction
of ring->fbc_dirty telling software on frontbuffer tracking to perform
the cache clean on sw side.
Cc: Paulo Zanoni
Signed-off-by: Rodrigo Vivi
---
drivers/gpu/drm/i915/i915_drv.h | 8
drivers/gpu/drm/i915/intel_display.c| 4 +++-
drivers/gpu/drm/i915/intel_ringbuffer.c | 9 +++--
3 files changed, 18 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 5706b8a..5acda40 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -657,6 +657,14 @@ struct i915_fbc {
bool false_color;
+ /* On gen8 some rings cannont perform fbc clean operation so for now
+* we are doing this on SW with mmio.
+* This variable works in the opposite information direction
+* of ring->fbc_dirty telling software on frontbuffer tracking
+* to perform the cache clean on sw side.
+*/
+ bool need_sw_cache_clean;
+
struct intel_fbc_work {
struct delayed_work work;
struct drm_crtc *crtc;
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index 965eb3c..731d925 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9137,8 +9137,10 @@ void intel_frontbuffer_flush(struct drm_device *dev,
* needs to be reworked into a proper frontbuffer tracking scheme like
* psr employs.
*/
- if (IS_BROADWELL(dev))
+ if (IS_BROADWELL(dev) && dev_priv->fbc.need_sw_cache_clean) {
+ dev_priv->fbc.need_sw_cache_clean = false;
gen8_fbc_sw_flush(dev, FBC_REND_CACHE_CLEAN);
+ }
}
/**
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c
b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 85fc2b1..02b43cd 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -2227,6 +2227,7 @@ static int gen6_ring_flush(struct intel_engine_cs *ring,
u32 invalidate, u32 flush)
{
struct drm_device *dev = ring->dev;
+ struct drm_i915_private *dev_priv = dev->dev_private;
uint32_t cmd;
int ret;
@@ -2257,8 +2258,12 @@ static int gen6_ring_flush(struct intel_engine_cs *ring,
}
intel_ring_advance(ring);
- if (IS_GEN7(dev) && !invalidate && flush)
- return gen7_ring_fbc_flush(ring, FBC_REND_CACHE_CLEAN);
+ if (!invalidate && flush) {
+ if (IS_GEN7(dev))
+ return gen7_ring_fbc_flush(ring, FBC_REND_CACHE_CLEAN);
+ else if (IS_GEN8(dev))
+ dev_priv->fbc.need_sw_cache_clean = true;
+ }
return 0;
}
--
1.9.3
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: FBC flush nuke for BDW
As Paulo told the part 2 of his proposal is still missing and we will get the WARN of reading fbc register at fbc_enabled when on runtime_suspend... But I couldn't find the propper place to check for intel_crtc->active. This was another issue reported by power team that on screen off there was to much activity on our driver. So we need to check it more carefully the proper place to avoid all unecessary inactivities with display off. Do you have any suggestion Daniel? On Fri, Sep 5, 2014 at 12:35 PM, Rodrigo Vivi wrote: > > > > On Fri, Sep 5, 2014 at 11:28 AM, Paulo Zanoni wrote: > >> 2014-08-04 7:51 GMT-03:00 Rodrigo Vivi : >> > According to spec FBC on BDW and HSW are identical without any gaps. >> > So let's copy the nuke and let FBC really start compressing stuff. >> > >> > Without this patch we can verify with false color that nothing is being >> > compressed. With the nuke in place and false color it is possible >> > to see false color debugs. >> > >> > Unfortunatelly on some rings like BCS on BDW we have to avoid Bits >> 22:18 on >> > LRIs due to a high risk of hung. So, when using Blt ring for >> frontbuffer rend >> > cache would never been cleaned and FBC would stop compressing buffer. >> > One alternative is to cache clean on software frontbuffer tracking. >> > >> > v2: Fix rebase conflict. >> > v3: Do not clean cache on BCS ring. Instead use sw frontbuffer tracking. >> >> This patch causes lots of WARNs when running igt/pm_rpm/cursor and a >> few other similar test cases. Now the interesting this is that we're >> trying to do FBC flushing, but: >> > > As I told you on the IRC already, thank you for finding this. It is good > to get reasons and arguments. > > >> - FBC is disabled by default > > > Yeah, we need to fix it although it just matter much > > >> - The screen is disabled > > - We're runtime suspended >> > > These are another error. Not caused by this patch. > Why frontbuffer_flush is being called with screen disabled or on runtime > suspend? > > > >> - We're touching cursors when we decide to flush FBC. >> > > This is not actually a flush. But cleaning the cache to allow compression > restart. > I believe we could have some variable to indicate when we touched ring > flush than on next frontbuffer we do the mmio cache clean. > > >> >> This is the first WARN that happens: >> >> [ 123.581179] [drm:intel_runtime_suspend] Device suspended >> [ 123.680666] [ cut here ] >> [ 123.680697] WARNING: CPU: 1 PID: 1776 at >> drivers/gpu/drm/i915/intel_uncore.c:47 >> assert_device_not_suspended.isra.8+0x43/0x50 [i915]() >> [ 123.680699] Device suspended >> [ 123.680700] Modules linked in: intel_rapl x86_pkg_temp_thermal >> serio_raw intel_powerclamp efivars btusb iwlmvm iwlwifi mei_me mei >> int3403_thermal snd_hda_codec_hdmi snd_hda_intel snd_hda_controller >> i2c_designware_platform snd_hda_codec i2c_designware_core snd_hwdep >> snd_pcm_oss snd_mixer_oss acpi_pad snd_pcm snd_timer fuse nls_utf8 >> nls_cp437 vfat fat sd_mod ahci libahci i915 sdhci_pci e1000e >> drm_kms_helper drm sdhci_acpi sdhci >> [ 123.680733] CPU: 1 PID: 1776 Comm: pm_rpm Not tainted >> 3.17.0-rc2.1409051503pz+ #1100 >> [ 123.680736] Hardware name: Intel Corporation Broadwell Client >> platform/Wilson Beach SDS, BIOS BDW-E2R1.86C.0072.R03.1405072127 >> 05/07/2014 >> [ 123.680738] 0009 88024174fab8 816f6d23 >> 88024174fb00 >> [ 123.680742] 88024174faf0 8107b368 88003770 >> 00050380 >> [ 123.680746] 00050380 880037700068 0001 >> 88024174fb50 >> [ 123.680751] Call Trace: >> [ 123.680758] [] dump_stack+0x4d/0x66 >> [ 123.680763] [] warn_slowpath_common+0x78/0xa0 >> [ 123.680766] [] warn_slowpath_fmt+0x47/0x50 >> [ 123.680772] [] ? >> trace_hardirqs_on_caller+0x15d/0x200 >> [ 123.680791] [] >> assert_device_not_suspended.isra.8+0x43/0x50 [i915] >> [ 123.680809] [] gen8_write32+0x35/0x180 [i915] >> [ 123.680821] [] gen8_fbc_sw_flush+0x29/0x30 [i915] >> [ 123.680840] [] intel_frontbuffer_flush+0x7d/0x90 >> [i915] >> [ 123.680859] [] >> intel_cursor_plane_update+0x13b/0x150 [i915] >> [ 123.680876] [] setplane_internal+0x260/0x2a0 [drm] >> [ 123.680889] [] drm_mode_cursor_common+0x11e/0x310 >> [drm] >> [ 123.680904] [] drm_mode_cursor_ioctl+0x3c/0x40 [drm] >> [ 123.680914] [] drm_ioctl+0x1df/0x6a0 [drm] >> [ 123.680920] [] ? mutex_unlock+0x9/0x10 >> [ 123.680924] [] ? seq_read+0xb6/0x3e0 >> [ 123.680929] [] do_vfs_ioctl+0x2e0/0x4e0 >> [ 123.680934] [] ? sysret_check+0x1b/0x56 >> [ 123.680939] [] ? >> trace_hardirqs_on_caller+0x15d/0x200 >> [ 123.680943] [] SyS_ioctl+0x81/0xa0 >> [ 123.680947] [] system_call_fastpath+0x16/0x1b >> [ 123.680949] ---[ end trace 9f389682639d2eb9 ]--- >> [ 123.680953] [ cut here ] >> >> >> So, what we can/could do: >> 1 - Change gen8_fbc_sw_flush() do to nothing when FBC is disabled. >> > > Agreed! > > >> 2 - At some po
Re: [Intel-gfx] [PATCH 2/2] drm/i915: Minimize the huge amount of unecessary fbc sw cache clean.
Hey Daniel,
funny story: Remember that with idle_frames=1 on BDW it was working but it
was faililng on HSW?
So, with these 2 patches applied now BDW PSR fails like HSW!!!
Ville, any thoughts on this?
On Fri, Sep 5, 2014 at 1:57 PM, Rodrigo Vivi wrote:
> The sw cache clean on gen8 is a tempoorary workaround because we cannot
> set cache clean on blt ring with risk of hungs. So we are doing the cache
> clean on sw.
> However we are doing much more than needed. Not only when using blt ring.
> So, with this extra w/a we minimize the ammount of cache cleans and call
> it only
> on same cases that it was being called on gen7.
>
> fbc.need_sw_cache_clean works in the opposite information direction
> of ring->fbc_dirty telling software on frontbuffer tracking to perform
> the cache clean on sw side.
>
> Cc: Paulo Zanoni
> Signed-off-by: Rodrigo Vivi
> ---
> drivers/gpu/drm/i915/i915_drv.h | 8
> drivers/gpu/drm/i915/intel_display.c| 4 +++-
> drivers/gpu/drm/i915/intel_ringbuffer.c | 9 +++--
> 3 files changed, 18 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> index 5706b8a..5acda40 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -657,6 +657,14 @@ struct i915_fbc {
>
> bool false_color;
>
> + /* On gen8 some rings cannont perform fbc clean operation so for
> now
> +* we are doing this on SW with mmio.
> +* This variable works in the opposite information direction
> +* of ring->fbc_dirty telling software on frontbuffer tracking
> +* to perform the cache clean on sw side.
> +*/
> + bool need_sw_cache_clean;
> +
> struct intel_fbc_work {
> struct delayed_work work;
> struct drm_crtc *crtc;
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 965eb3c..731d925 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -9137,8 +9137,10 @@ void intel_frontbuffer_flush(struct drm_device *dev,
> * needs to be reworked into a proper frontbuffer tracking scheme
> like
> * psr employs.
> */
> - if (IS_BROADWELL(dev))
> + if (IS_BROADWELL(dev) && dev_priv->fbc.need_sw_cache_clean) {
> + dev_priv->fbc.need_sw_cache_clean = false;
> gen8_fbc_sw_flush(dev, FBC_REND_CACHE_CLEAN);
> + }
> }
>
> /**
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c
> b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 85fc2b1..02b43cd 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -2227,6 +2227,7 @@ static int gen6_ring_flush(struct intel_engine_cs
> *ring,
>u32 invalidate, u32 flush)
> {
> struct drm_device *dev = ring->dev;
> + struct drm_i915_private *dev_priv = dev->dev_private;
> uint32_t cmd;
> int ret;
>
> @@ -2257,8 +2258,12 @@ static int gen6_ring_flush(struct intel_engine_cs
> *ring,
> }
> intel_ring_advance(ring);
>
> - if (IS_GEN7(dev) && !invalidate && flush)
> - return gen7_ring_fbc_flush(ring, FBC_REND_CACHE_CLEAN);
> + if (!invalidate && flush) {
> + if (IS_GEN7(dev))
> + return gen7_ring_fbc_flush(ring,
> FBC_REND_CACHE_CLEAN);
> + else if (IS_GEN8(dev))
> + dev_priv->fbc.need_sw_cache_clean = true;
> + }
>
> return 0;
> }
> --
> 1.9.3
>
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
--
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
