Re: [Intel-gfx] [PATCH i-g-t] igt/kms_rotation_crc: Add horizontal flip subtest.
>-Original Message- >From: Vivi, Rodrigo >Sent: Tuesday, December 19, 2017 1:50 PM >To: Srivatsa, Anusha>Cc: Daniel Vetter ; Strano, Luis ; >Latvala, Petri ; Lofstedt, Marta > ; Saarinen, Jani ; intel-gfx > ; Joseph Garvey >Subject: Re: [Intel-gfx] [PATCH i-g-t] igt/kms_rotation_crc: Add horizontal >flip >subtest. > >On Tue, Dec 19, 2017 at 12:20:18AM +, Srivatsa, Anusha wrote: >> >> >> >-Original Message- >> >From: daniel.vet...@ffwll.ch [mailto:daniel.vet...@ffwll.ch] On >> >Behalf Of Daniel Vetter >> >Sent: Thursday, December 14, 2017 2:14 AM >> >To: Srivatsa, Anusha ; Strano, Luis >> > ; Latvala, Petri ; >> >Lofstedt, Marta ; Saarinen, Jani >> > >> >Cc: intel-gfx ; Joseph Garvey >> > >> >Subject: Re: [Intel-gfx] [PATCH i-g-t] igt/kms_rotation_crc: Add >> >horizontal flip subtest. >> > >> >On Thu, Nov 23, 2017 at 12:05 AM, Anusha Srivatsa >> > >> >wrote: >> >> From: Joseph Garvey >> >> >> >> Test that horizontal flip works with supported rotations. Includes >> >> a fix for the unrotated fb which was not being positioned correctly >> >> with portrait and landscape rectangles. >> >> >> >> v2:(from Anusha) >> >> - Change 180 degree rotation to follow the rest, use igt_swap(), >> >> make flip variable a bool. Format the patch correctly (Ville, Petri >> >> Latvala) >> >> >> >> v3: (From Anusha) >> >> - Correct the name of subtests in order to avoid duplication of >> >> names >> >> (Arek) >> >> >> >> Signed-off-by: Anusha Srivatsa >> >> Signed-off-by: Joseph Garvey >> >> Cc: Ville Syrjälä >> >> Cc: Petri Latvala >> >> Cc: Arkadiusz Hiler >> > >> >I didn't see this patch fly by originally, but now Marta pointed out >> >that this skips everywhere. We need to rework it. >> > >> >General principle is that in kms tests we should _not_ have any >> >platform/feature checks encoded in the test. Instead, the testcase >> >should check properties to figure out whether something should work or not. >> > >> > >> >> --- >> >> lib/igt_kms.c| 2 +- >> >> lib/igt_kms.h| 5 ++ >> >> tests/kms_rotation_crc.c | 198 >> >> +-- >> >> 3 files changed, 164 insertions(+), 41 deletions(-) >> >> >> >> diff --git a/lib/igt_kms.c b/lib/igt_kms.c index a572fc6..3034e44 >> >> 100644 >> >> --- a/lib/igt_kms.c >> >> +++ b/lib/igt_kms.c >> >> @@ -3050,7 +3050,7 @@ void igt_fb_set_size(struct igt_fb *fb, >> >> igt_plane_t *plane, >> >> >> >> static const char *rotation_name(igt_rotation_t rotation) { >> >> - switch (rotation) { >> >> + switch (rotation & IGT_ROTATION_MASK) { >> >> case IGT_ROTATION_0: >> >> return "0°"; >> >> case IGT_ROTATION_90: >> >> diff --git a/lib/igt_kms.h b/lib/igt_kms.h index 8dc118c..b83a828 >> >> 100644 >> >> --- a/lib/igt_kms.h >> >> +++ b/lib/igt_kms.h >> >> @@ -281,8 +281,13 @@ typedef enum { >> >> IGT_ROTATION_90 = 1 << 1, >> >> IGT_ROTATION_180 = 1 << 2, >> >> IGT_ROTATION_270 = 1 << 3, >> >> + IGT_REFLECT_X= 1 << 4, >> >> + IGT_REFLECT_Y= 1 << 5, >> >> } igt_rotation_t; >> >> >> >> +#define IGT_ROTATION_MASK \ >> >> + (IGT_ROTATION_0 | IGT_ROTATION_90 | IGT_ROTATION_180 | >> >> +IGT_ROTATION_270) >> >> + >> >> typedef struct { >> >> /*< private >*/ >> >> igt_pipe_t *pipe; >> >> diff --git a/tests/kms_rotation_crc.c b/tests/kms_rotation_crc.c >> >> index >> >> 5aec8fa..9e13667 100644 >> >> --- a/tests/kms_rotation_crc.c >> >> +++ b/tests/kms_rotation_crc.c >> >> @@ -32,6 +32,7 @@ typedef struct { >> >> igt_display_t display; >> >> struct igt_fb fb; >> >> struct igt_fb fb_reference; >> >> + struct igt_fb fb_unrotated; >> >> struct igt_fb fb_modeset; >> >> struct igt_fb fb_flip; >> >> igt_crc_t ref_crc; >> >> @@ -43,8 +44,59 @@ typedef struct { >> >> uint32_t override_fmt; >> >> uint64_t override_tiling; >> >> bool flips; >> >> + int devid; >> >> } data_t; >> >> >> >> +typedef struct { >> >> + float r; >> >> + float g; >> >> + float b; >> >> +} rgb_color_t; >> >> + >> >> +static void set_color(rgb_color_t *color, float r, float g, float >> >> +b) { >> >> + color->r = r; >> >> + color->g = g; >> >> + color->b = b; >> >> +} >> >> + >> >> +static void rotate_colors(rgb_color_t *tl, rgb_color_t *tr, rgb_color_t >> >>
Re: [Intel-gfx] [PATCH v4 7/7] drm/i915: Add YCBCR 4:2:0/4:4:4 support for LSPCON
Regards Shashank On 2/2/2018 12:39 AM, Ville Syrjälä wrote: On Tue, Jan 30, 2018 at 03:06:03PM +0530, Shashank Sharma wrote: From: "Sharma, Shashank"LSPCON chips can generate YCBCR outputs, if asked nicely :). In order to generate YCBCR 4:2:0 outputs, a source must: - send YCBCR 4:4:4 signals to LSPCON - program color space as 4:2:0 in AVI infoframes Whereas for YCBCR 4:4:4 outputs, the source must: - send YCBCR 4:4:4 signals to LSPCON - program color space as 4:4:4 in AVI infoframes So for both 4:2:0 as well as 4:4:4 outputs, we are driving the pipe for YCBCR 4:4:4 output, but AVI infoframe's color space information indicates LSPCON FW to start scaling down from YCBCR 4:4:4 and generate YCBCR 4:2:0 output. As the scaling is done by LSPCON device, we need not to reserve a scaler for 4:2:0 outputs. V2: rebase V3: Addressed review comments from Ville - add enum crtc_output_format instead of bool ycbcr420 - use crtc_output_format=4:4:4 for modeset of LSPCON 4:2:0 output cases in this way we will have YCBCR 4:4:4 framework ready (except the ABI part) V4: Added r-b from Maarten (for v3) Addressed review comments from Ville: - Do not add a non-atomic state variable to determine lspcon output. Instead add bool in CRTC state to indicate lspcon based scaling. Cc: Ville Syrjala Cc: Maarten Lankhorst Reviewed-by: Maarten Lankhorst Signed-off-by: Shashank Sharma --- drivers/gpu/drm/i915/i915_reg.h | 2 ++ drivers/gpu/drm/i915/intel_ddi.c | 7 +++ drivers/gpu/drm/i915/intel_display.c | 11 ++- drivers/gpu/drm/i915/intel_dp.c | 10 ++ drivers/gpu/drm/i915/intel_drv.h | 5 + drivers/gpu/drm/i915/intel_lspcon.c | 30 ++ 6 files changed, 64 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 64933fd..e383b05 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -8721,6 +8721,8 @@ enum skl_power_gate { #define TRANS_MSA_MISC(tran) _MMIO_TRANS2(tran, _TRANSA_MSA_MISC) #define TRANS_MSA_SYNC_CLK (1<<0) +#define TRANS_MSA_SAMPLING_444(2<<1) +#define TRANS_MSA_CLRSP_YCBCR (2<<3) #define TRANS_MSA_6_BPC (0<<5) #define TRANS_MSA_8_BPC (1<<5) #define TRANS_MSA_10_BPC (2<<5) diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c index 510bb77..db7d940 100644 --- a/drivers/gpu/drm/i915/intel_ddi.c +++ b/drivers/gpu/drm/i915/intel_ddi.c @@ -1499,6 +1499,13 @@ void intel_ddi_set_pipe_settings(const struct intel_crtc_state *crtc_state) break; } + /* +* As per DP 1.2 spec section 2.3.4.3 while sending +* YCBCR 444 signals we should program MSA MISC1/0 fields with +* colorspace information. The output colorspace encoding is BT601. +*/ + if (crtc_state->output_format == CRTC_OUTPUT_YCBCR444) + temp |= TRANS_MSA_SAMPLING_444 | TRANS_MSA_CLRSP_YCBCR; I915_WRITE(TRANS_MSA_MISC(cpu_transcoder), temp); } diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 4a40de9..b26f4a9 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -9228,6 +9228,7 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc, pipe_config->gamma_mode = I915_READ(GAMMA_MODE(crtc->pipe)) & GAMMA_MODE_MODE_MASK; + pipe_config->external_scaling = false; I really don't like the name of that. Makes me think we're actually scaling the entire output or something. 'external_ycbcr_420_downsample' or something like that would be better. ok, let me rename this to something in those lines. if (IS_BROADWELL(dev_priv) || INTEL_GEN(dev_priv) >= 9) { u32 tmp = I915_READ(PIPEMISC(crtc->pipe)); enum intel_output_format output_format = CRTC_OUTPUT_RGB; @@ -9247,8 +9248,16 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc, output_format = CRTC_OUTPUT_YCBCR420; } else { output_format = CRTC_OUTPUT_YCBCR444; - } +/* +* As there is no interface defined (yet) +* to get the user's preference for output +* format, YCBCR444 output format is only +* possible with LSPCON YCBCR420 output, +* which uses LSPCON's (external )scaler. +*/ + pipe_config->external_scaling = true; + }
Re: [Intel-gfx] [PATCH v4 1/7] drm/i915: Add CRTC output format YCBCR 4:2:0
Thanks for the comments, mine inline. Regards Shashank On 2/2/2018 12:39 AM, Ville Syrjälä wrote: On Tue, Jan 30, 2018 at 03:05:57PM +0530, Shashank Sharma wrote: From: "Sharma, Shashank"Currently, we are using a bool in CRTC state (state->ycbcr420), to indicate modeset, that the output format is YCBCR 4:2:0. Now in order to support other YCBCR formats, we will need more such flags. The idea behind this patch is to replace this bool with an enum, and plug this in in the existing YCBCR 4:2:0 framework in such a way that this can be re-used for YCBCR 4:4:4 and other output formats too. This patch adds a new enum for CRTC output formats, and then plugs it in the existing modeset framework. V3: Added this patch in the series, to address review comments from second patchset. V4: Added r-b from Maarten (on v3) Addressed review comments from Ville: - Change the enum name to intel_output_format from crtc_output_format. - Start the enum value (INVALID) from 0 instaed of 1. - Set the crtc's output_format to RGB in encoder's compute_config. Cc: Ville Syrjala Cc: Maarten Lankhorst Signed-off-by: Shashank Sharma Reviewed-by: Maarten Lankhorst --- drivers/gpu/drm/i915/intel_color.c | 2 +- drivers/gpu/drm/i915/intel_ddi.c | 4 ++- drivers/gpu/drm/i915/intel_display.c | 66 drivers/gpu/drm/i915/intel_drv.h | 10 -- drivers/gpu/drm/i915/intel_hdmi.c| 6 ++-- drivers/gpu/drm/i915/intel_panel.c | 2 +- 6 files changed, 61 insertions(+), 29 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c index aa66e95..99e32cb 100644 --- a/drivers/gpu/drm/i915/intel_color.c +++ b/drivers/gpu/drm/i915/intel_color.c @@ -141,7 +141,7 @@ static void i9xx_load_csc_matrix(struct drm_crtc_state *crtc_state) uint16_t coeffs[9] = { 0, }; struct intel_crtc_state *intel_crtc_state = to_intel_crtc_state(crtc_state); - if (intel_crtc_state->ycbcr420) { + if (intel_crtc_state->output_format == CRTC_OUTPUT_YCBCR420) { i9xx_load_ycbcr_conversion_matrix(intel_crtc); return; } else if (crtc_state->ctm) { diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c index e51559b..d565e28 100644 --- a/drivers/gpu/drm/i915/intel_ddi.c +++ b/drivers/gpu/drm/i915/intel_ddi.c @@ -1264,7 +1264,7 @@ static void ddi_dotclock_get(struct intel_crtc_state *pipe_config) else dotclock = pipe_config->port_clock; - if (pipe_config->ycbcr420) + if (pipe_config->output_format == CRTC_OUTPUT_YCBCR420) dotclock *= 2; if (pipe_config->pixel_multiplier) @@ -2759,6 +2759,8 @@ static bool intel_ddi_compute_config(struct intel_encoder *encoder, if (port == PORT_A) pipe_config->cpu_transcoder = TRANSCODER_EDP; + pipe_config->output_format = CRTC_OUTPUT_RGB; We seem to be missing this for most platforms/encoder types. I thought this would be required only for DDI interfaces, do you suggest we should set this per encoder basis ? + if (intel_crtc_has_type(pipe_config, INTEL_OUTPUT_HDMI)) ret = intel_hdmi_compute_config(encoder, pipe_config, conn_state); else diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 83de43c..877336d 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -4650,7 +4650,8 @@ skl_update_scaler(struct intel_crtc_state *crtc_state, bool force_detach, */ need_scaling = src_w != dst_w || src_h != dst_h; - if (crtc_state->ycbcr420 && scaler_user == SKL_CRTC_INDEX) + if (crtc_state->output_format == CRTC_OUTPUT_YCBCR420 && + scaler_user == SKL_CRTC_INDEX) need_scaling = true; /* @@ -6362,7 +6363,8 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc, return -EINVAL; } - if (pipe_config->ycbcr420 && pipe_config->base.ctm) { + if (pipe_config->output_format == CRTC_OUTPUT_YCBCR420 && + pipe_config->base.ctm) { /* * There is only one pipe CSC unit per pipe, and we need that * for output conversion from RGB->YCBCR. So if CTM is already @@ -8192,10 +8194,10 @@ static void haswell_set_pipemisc(struct drm_crtc *crtc) if (intel_crtc->config->dither) val |= PIPEMISC_DITHER_ENABLE | PIPEMISC_DITHER_TYPE_SP; - if (config->ycbcr420) { - val |= PIPEMISC_OUTPUT_COLORSPACE_YUV | - PIPEMISC_YUV420_ENABLE | - PIPEMISC_YUV420_MODE_FULL_BLEND; + if (config->output_format ==
Re: [Intel-gfx] [PATCH v7 3/6] drm/i915/guc: Implement dynamic GuC WOPCM offset and size
On 2/2/2018 3:21 AM, Yaodong Li wrote: On 02/01/2018 12:38 AM, Sagar Arun Kamble wrote: On 1/19/2018 6:59 AM, Jackie Li wrote: Hardware may have specific restrictions on GuC WOPCM size It would be good if you can tell about Gen9/CNL restriction briefly here. versus HuC firmware size. With static GuC WOPCM size, there's no way to adjust the GuC WOPCM partition size based on the actual HuC firmware size, so that GuC/HuC loading failure would occur even if there was enough WOPCM space for both GuC and HuC firmware. This patch enables the dynamic calculation of the GuC WOPCM aperture size used by GuC and HuC firmware. we are also calculating for HuC? Strictly speaking, we are calculating the GuC WOPCM offset based on GuC & HuC firmware sizes. Will make it clearer. Thanks. + offset = huc_fw_size + WOPCM_RESERVED_SIZE; + if (offset >= WOPCM_DEFAULT_SIZE) + return -E2BIG; + + /* Hardware requires GuC WOPCM offset needs to be 16K aligned. */ + offset = ALIGN(offset, WOPCM_OFFSET_ALIGNMENT); + if ((offset + reserved) >= WOPCM_DEFAULT_SIZE) + return -E2BIG; + + top = WOPCM_DEFAULT_SIZE - offset; + size = top - reserved; + + /* GuC WOPCM size must be 4K aligned. */ + size &= GUC_WOPCM_SIZE_MASK; + ALIGN(size, PAGE_SIZE)? Can avoid this new macro. If you want to stay with GUC_WOPCM_SIZE_MASK, then that macro needs to be in guc_wopcm.h I'd like to go with GUC_WOPCM_SIZE_MASK here since there's no sign that it should be related to page size. *Align* seems not accurate here since I actually wanted to trim the size to 4k boundary, will update the comments. Thanks! Need to update comment though. /* GuC WOPCM size must be multiple of 4K pages */ Align does not suit here. We could have used "& ~PAGE_MASK" but okay with G_W_S_M too. size being absolute here helps. had it been offset starting from 0 we would have needed extra logic. -- Thanks, Sagar ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] i915 PSR test results and cursor lag
On Thu, Feb 1, 2018 at 9:20 PM, Chris Wilsonwrote: > Quoting Andy Lutomirski (2018-02-01 21:04:30) >> I got this after a recent suspend/resume: >> >> Feb 01 09:44:34 laptop systemd-logind[2412]: Lid closed. >> Feb 01 09:44:34 laptop systemd-logind[2412]: device-enumerator: scan all dirs >> Feb 01 09:44:34 laptop systemd-logind[2412]: device-enumerator: >> scanning /sys/bus >> Feb 01 09:44:34 laptop systemd-logind[2412]: device-enumerator: >> scanning /sys/class >> Feb 01 09:44:34 laptop systemd-logind[2412]: Failed to open >> configuration file '/etc/systemd/sleep.conf': No such file or >> directory >> Feb 01 09:44:34 laptop systemd-logind[2412]: Suspending... >> Feb 01 09:44:34 laptop systemd-logind[2412]: Sent message type=signal >> sender=n/a destination=n/a object=/org/freedesktop/login1 >> interface=org.freedesktop.login1.Manager member=PrepareForSleep >> cookie=570 reply >> Feb 01 09:44:34 laptop systemd-logind[2412]: Got message >> type=method_call sender=:1.46 destination=:1.1 >> object=/org/freedesktop/login1/session/_32 >> interface=org.freedesktop.login1.Session member=ReleaseDevice >> Feb 01 09:44:34 laptop systemd-logind[2412]: Sent message type=signal >> sender=n/a destination=:1.46 >> object=/org/freedesktop/login1/session/_32 >> interface=org.freedesktop.login1.Session member=PauseDevice cookie >> Feb 01 09:44:34 laptop gnome-shell[2630]: Failed to apply DRM plane >> transform 0: Permission denied >> Feb 01 09:44:34 laptop gnome-shell[2630]: drmModeSetCursor2 failed >> with (Permission denied), drawing cursor with OpenGL from now on >> >> But I don't see the word "cursor" in my system logs before the first >> suspend. What am I looking for? This is Fedora 27 running a Gnome >> Wayland session, but it hasn't been reinstalled in some time, so it's >> possible that there are some weird settings sitting around. But I did >> check and I have no weird i915 parameters. > > You are using gnome-shell as the display server. From that it appears to > have started off with a HW cursor and switched to a SW cursor after > suspend. Did you notice a change in behaviour? After rebooting or just > restarting gnome-shell? I think it's less consistently bad after a reboot before suspending. > >> Also, are these things potentially related: >> >> [ 3067.702527] [drm:intel_pipe_update_start [i915]] *ERROR* Potential >> atomic update failure on pipe A > > They are just "missed the immediate vblank for the screen update" > messages. Should not be related to PSR, but may cause jitter by delaying > the odd screen update. I just got this one, and the timestamp is at least reasonably close to a giant latency spike: [ 288.799654] [drm:intel_pipe_update_end [i915]] *ERROR* Atomic update failure on pipe A (start=31 end=32) time 15 us, min 1073, max 1079, scanline start 1087, end 1088 > >> As I'm typing this, I've seen a couple instances of what seems like a >> full *second* of cursor latency, but I've only gotten the potential >> atomic update failure once. >> >> And is there any straightforward tracing to do to distinguish between >> PSR exit latency and other potential sources of latency? > > It looks plausible that we could at least report how long it takes the > registers to reflect the change in state (but we don't). The best source > of information atm is /sys/kernel/debug/dri/0/i915_edp_psr_status. Hmm. I went and looked at the code, and I noticed what could be bugs or could (more likely) be my confusion since I don't know this code at all: intel_single_frame_update() does something inscrutable to me, but I imagine it does something that causes the next page flip to get noticed by the panel even with PSR on. But how does the code that calls it know that anything happened? (Looking at the commit history, maybe this is something special that's only needed on some platforms but doesn't replace the normal PSR exit sequence.) Perhaps more interestingly, intel_psr_flush() does this: /* By definition flush = invalidate + flush */ if (frontbuffer_bits) intel_psr_exit(dev_priv); if (!dev_priv->psr.active && !dev_priv->psr.busy_frontbuffer_bits) if (!work_busy(_priv->psr.work.work)) schedule_delayed_work(_priv->psr.work, msecs_to_jiffies(100)); I'm guessing that the idea is that we're turning off PSR because we want the panel to update and we expect that, in 100ms, the update will have hit the panel and we'll have been idle long enough for it to make sense to re-enter PSR. IOW, the code wants PSR to be off for at least 100ms and then to turn back on. But this code actually says "turn PSR back on in at *most* 100ms". What happens if there are two screen updates 99ms apart? The first one should work fine, but the next one will hit with 1ms left on the delayed work, and intel_psr_work() will get called in 1ms. There's some magic with busy_frontbuffer_bits, but it seems questionable to me that
Re: [Intel-gfx] clang warning: implicit conversion in intel_ddi.c:1481
Hi Greg, On Thu, 1 Feb 2018, Greg KH wrote: > On Thu, Feb 01, 2018 at 06:33:30PM +0100, Ozan Alpay wrote: > > Dear Rodrigo Vivi, Ville Syrjälä, > > > > My name is Ozan Alpay, and I am a student mentored by Lukas Bulwahn. We > > intend to use static analysis tools on the kernel source to identify, > > analyze and report issues. As a very first step, we are looking into > > clang compiler warnings and will then move to more sophisticated tools. > > > > [...] > > > > Linux 4.15 is shipped with this clang warning, but we don't see the > > crucial need to provide a backport commit to the stable branch for 4.15. > > We just wanted to inform you about our analysis of this clang warning. > > Ultimately the final call if you would like to address this clang warning > > in 4.15 is yours. > > Note, I have not taken "clang warning fixes" for stable kernel updates > in the past, and I doubt I will in the future, unless the tree "builds > clean" with clang. If it eventually gets there, then yes, I will do > that. > > Note, if you are going to email this out to everyone who fixes a warning > message, you might want to reconsider it. That's going to be a lot of > work, and for people who have already fixed an issue, it's kind of > pointless to just remind them of work they have done in the past, right? > > What is the goal of these types of emails? > We are interested in providing useful information on potential bugs or bug patterns that we get from static analysis tools, after we pre-assess them and manually select them to send to the review process of the patch submission. For me, the clang warnings were an easy starting point for a student to set up and look at, compared to much more sophisticated tools, such as coccinelle, sparse or new tools for the kernel development, such as CMBC or facebook's Infer. Once we really understand which tools are useful and which information can be quickly pre-assessed and sent out in a useful way without just creating more noise in the discussion, I would have contacted the 0-day infrastructure team or the kernelci.org team to continue the discussion how to include our first setup into a proper semi-automated service. Using the clang warnings, I wanted to explore how this would even potentially work. Considering clang, of course, currently, we cannot compile the whole kernel with all possible kernel configurations with clang, but I believe Nick Desaulniers, Matthias Kaehlcke and others are already working on that and are getting close to this goal. Hence, assuming they will be successful soon, I wanted to explore the next step of using static analysis tools around the clang/LLVM compiler. Actually, v4.15 builds almost "cleanly" with clang: For defconfig, there are only two clang compiler warnings and the one that we looked into deeper here is already resolved in linux-next, so chances are actually high that we might get to this "builds clean" soon-ish, at least for defconfig. Concerning clang warnings and how to progress towards that goal of building cleanly, my strategy is to identify when new clang compiler warnings are introduced and just point these warnings out as code smells during the review discussion before they are merged into the first maintainer tree. If we manually inspect these clang warnings to make sure that they are genuine code smells of some "imprecise implementation" before we send them to the mailing list, I would hope that they are of some value for the developer in the submission process and he/she could address the warning in a patch v2 while he/she is reacting to the other review comments from the human reviewers. At best, I always considered them as useful information during the review process, but I certainly DO NOT want to start patching the kernel due to clang warnings. The chances/risk that we just break more due to naively fixing warnings without proper understanding is much higher than the benefit of actually improving the situation. If I recall correctly, I think this is also one of the lessons learned from motivating newcomers to address warnings in previous kernel newbies activities. Greg, do you think it is worthwhile to invest some effort to move towards the goal "kernel builds cleanly with clang"? Would you agree that providing information during the patch review is a good way to move forward to this goal if we find a suitable manner to provide this feedback quickly and cleanly at this very early stage of development? If not, we will immediately stop to move in this direction and this is the first and last email that you have seen of this kind, and we will have to come up with better/other ideas around work on the Linux kernel. If so, we will continue in the direction sketched above, and I think I just have to point out and apologize for the two obvious things that we did wrong in this specific case here: - We noticed that there were further changes in linux-next, but we
Re: [Intel-gfx] clang warning: implicit conversion in intel_ddi.c:1481
01.02.2018, 21:03, "Greg KH": > On Thu, Feb 01, 2018 at 06:33:30PM +0100, Ozan Alpay wrote: >> Dear Rodrigo Vivi, Ville Syrjälä, >> >> My name is Ozan Alpay, and I am a student mentored by Lukas Bulwahn. We Hi Ozan, why did you send e-mail to kernel development e-mail list? >> intend to use static analysis tools on the kernel source to identify, >> analyze and report issues. As a very first step, we are looking into >> clang compiler warnings and will then move to more sophisticated tools. >> >> When compiling Linux 4.15 with clang, we have discovered that your commit >> 2952cd6fb4cc ("drm/i915: Let's use more enum intel_dpll_id pll_id.") >> introduced the following warning: >> >> drivers/gpu/drm/i915/intel_ddi.c:1481:30: warning: implicit conversion from >> enumeration type 'enum port' to different enumeration type 'enum >> intel_dpll_id' [-Wenum-conversion] >> enum intel_dpll_id pll_id = port; >> >> To reproduce it, you can compile Linux 4.15 with clang with this command: >> >> make HOSTCC=clang-5.0 defconfig && make -j32 HOSTCC=clang-5.0 CC=clang-5.0 >> >> If you don't have clang installed in your system, you can use this simple >> docker setup to compile the kernel with clang: >> >> wget >> https://raw.githubusercontent.com/bulwahn/linux-kernel-analysis/master/docker/kernel-clang/Dockerfile >> && \ >> docker build -t kernel-clang . && \ >> docker run -v :/linux/ kernel-clang /bin/sh >> -c "cd linux && make CC=clang-5.0 clean && make HOSTCC=clang-5.0 defconfig >> && make -j32 HOSTCC=clang-5.0 CC=clang-5.0" >> >> While we were doing our analysis on 4.15, we noticed that you already >> resolved this warning on linux-next with your work in commit bb911536f07e >> ("drm/i915: Eliminate pll->state usage from bxt_calc_pll_link()"). So, >> since it is resolved on linux-next and we expect that this commit will be >> merged in the merge window for 4.16, there is probably nothing further to >> do. >> >> Linux 4.15 is shipped with this clang warning, but we don't see the >> crucial need to provide a backport commit to the stable branch for 4.15. >> We just wanted to inform you about our analysis of this clang warning. >> Ultimately the final call if you would like to address this clang warning >> in 4.15 is yours. > > Note, I have not taken "clang warning fixes" for stable kernel updates > in the past, and I doubt I will in the future, unless the tree "builds > clean" with clang. If it eventually gets there, then yes, I will do > that. > > Note, if you are going to email this out to everyone who fixes a warning > message, you might want to reconsider it. That's going to be a lot of > work, and for people who have already fixed an issue, it's kind of > pointless to just remind them of work they have done in the past, right? > > What is the goal of these types of emails? > > thanks, > > greg k-h Ozgur ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] clang warning: implicit conversion in intel_ddi.c:1481
Dear Rodrigo Vivi, Ville Syrjälä, My name is Ozan Alpay, and I am a student mentored by Lukas Bulwahn. We intend to use static analysis tools on the kernel source to identify, analyze and report issues. As a very first step, we are looking into clang compiler warnings and will then move to more sophisticated tools. When compiling Linux 4.15 with clang, we have discovered that your commit 2952cd6fb4cc ("drm/i915: Let's use more enum intel_dpll_id pll_id.") introduced the following warning: drivers/gpu/drm/i915/intel_ddi.c:1481:30: warning: implicit conversion from enumeration type 'enum port' to different enumeration type 'enum intel_dpll_id' [-Wenum-conversion] enum intel_dpll_id pll_id = port; To reproduce it, you can compile Linux 4.15 with clang with this command: make HOSTCC=clang-5.0 defconfig && make -j32 HOSTCC=clang-5.0 CC=clang-5.0 If you don't have clang installed in your system, you can use this simple docker setup to compile the kernel with clang: wget https://raw.githubusercontent.com/bulwahn/linux-kernel-analysis/master/docker/kernel-clang/Dockerfile && \ docker build -t kernel-clang . && \ docker run -v :/linux/ kernel-clang /bin/sh -c "cd linux && make CC=clang-5.0 clean && make HOSTCC=clang-5.0 defconfig && make -j32 HOSTCC=clang-5.0 CC=clang-5.0" While we were doing our analysis on 4.15, we noticed that you already resolved this warning on linux-next with your work in commit bb911536f07e ("drm/i915: Eliminate pll->state usage from bxt_calc_pll_link()"). So, since it is resolved on linux-next and we expect that this commit will be merged in the merge window for 4.16, there is probably nothing further to do. Linux 4.15 is shipped with this clang warning, but we don't see the crucial need to provide a backport commit to the stable branch for 4.15. We just wanted to inform you about our analysis of this clang warning. Ultimately the final call if you would like to address this clang warning in 4.15 is yours. Best regards, Ozan & Lukas___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PULL] drm-intel-next-fixes
Hi Dave, I didn't send sooner because I wasn't happy with the CI results running on drm-intel-next-fixes, but now everything looks greener there. Here goes drm-intel-next-fixes-2018-02-01: Fixes for GPU hangs and other bugs around hangcheck and result; Fix for regression on suspend case with vgaswitcheroo; Fixes for eDP and HDMI blank screens Fix for protecting WC allocation to avoid overflow on page vec; Cleanup around unpublished GLK firmware blobs, and other small fixes. This also contains GVT pull request mostly with regression fixes on vGPU display dmabuf, mmio switch and other misc changes. Thanks, Rodrigo. The following changes since commit 24b8ef699e8221d2b7f813adaab13eec053e1507: drm/ast: Load lut in crtc_commit (2018-02-01 11:35:46 +1000) are available in the git repository at: git://anongit.freedesktop.org/drm/drm-intel tags/drm-intel-next-fixes-2018-02-01 for you to fetch changes up to 751b01cb07ebf0dbbe4a4fbfeaa509388e4a2b0a: drm/i915/ppgtt: Pin page directories before allocation (2018-02-01 07:33:04 -0800) Fixes for GPU hangs and other bugs around hangcheck and result; Fix for regression on suspend case with vgaswitcheroo; Fixes for eDP and HDMI blank screens Fix for protecting WC allocation to avoid overflow on page vec; Cleanup around unpublished GLK firmware blobs, and other small fixes. This also contains GVT pull request mostly with regression fixes on vGPU display dmabuf, mmio switch and other misc changes. Anusha Srivatsa (1): drm/i915/glk: Disable Guc and HuC on GLK Chris Wilson (5): drm/i915/pmu: Reconstruct active state on starting busy-stats drm/i915: Only attempt to scan the requested number of shrinker slabs drm/i915: Protect WC stash allocation against direct reclaim drm/i915: Always run hangcheck while the GPU is busy drm/i915/ppgtt: Pin page directories before allocation Hang Yuan (1): drm/i915/gvt: validate gfn before set shadow page entry Imre Deak (1): drm/i915: Fix using BIT_ULL() vs. BIT() for power domain masks Lionel Landwerlin (1): Revert "drm/i915: mark all device info struct with __initconst" Maarten Lankhorst (1): drm/i915: Always call to intel_display_set_init_power() in resume_early. Manasi Navare (1): drm/i915/edp: Do not do link training fallback or prune modes on EDP Michel Thierry (1): drm/i915/gvt: Do not use I915_NUM_ENGINES to iterate over the mocs regs array Mika Kahola (1): drm/i915: Check for fused or unused pipes Oscar Mateo (1): drm/i915: Stop getting the fault address from RING_FAULT_REG Pei Zhang (1): drm/i915/gvt: add PLANE_KEYMAX regs to mmio track list Sagar Arun Kamble (1): drm/i915/guc: Add uc_fini_wq in gem_init unwind path Stefan Brüns (1): drm/i915: Try EDID bitbanging on HDMI after failed read Tina Zhang (1): drm/i915/gvt: Keep obj->dma_buf link NULL during exporting Xiong Zhang (1): drm/i915/gvt: Fix gen8/9_render_mmio_list[0] don't take effect Zhenyu Wang (2): drm/i915/gvt: cancel virtual vblank timer when no vGPU exists drm/i915/gvt: cancel scheduler timer when no vGPU exists drivers/gpu/drm/i915/gvt/dmabuf.c | 1 - drivers/gpu/drm/i915/gvt/gtt.c| 24 +-- drivers/gpu/drm/i915/gvt/handlers.c | 3 + drivers/gpu/drm/i915/gvt/hypercall.h | 1 + drivers/gpu/drm/i915/gvt/kvmgt.c | 16 + drivers/gpu/drm/i915/gvt/mmio_context.c | 10 +-- drivers/gpu/drm/i915/gvt/mpt.h| 17 + drivers/gpu/drm/i915/gvt/sched_policy.c | 7 ++ drivers/gpu/drm/i915/gvt/vgpu.c | 2 + drivers/gpu/drm/i915/i915_drv.c | 2 + drivers/gpu/drm/i915/i915_gem.c | 9 +-- drivers/gpu/drm/i915/i915_gem_gtt.c | 61 + drivers/gpu/drm/i915/i915_gem_request.c | 2 + drivers/gpu/drm/i915/i915_gem_shrinker.c | 4 +- drivers/gpu/drm/i915/i915_pci.c | 94 +-- drivers/gpu/drm/i915/i915_reg.h | 2 + drivers/gpu/drm/i915/intel_audio.c| 2 +- drivers/gpu/drm/i915/intel_breadcrumbs.c | 11 drivers/gpu/drm/i915/intel_display.c | 6 +- drivers/gpu/drm/i915/intel_dp_link_training.c | 26 +--- drivers/gpu/drm/i915/intel_engine_cs.c| 16 - drivers/gpu/drm/i915/intel_guc_fw.c | 9 --- drivers/gpu/drm/i915/intel_hangcheck.c| 7 +- drivers/gpu/drm/i915/intel_hdmi.c | 14 +++- drivers/gpu/drm/i915/intel_huc.c | 11 drivers/gpu/drm/i915/intel_uc.c | 2 - 26 files changed, 226 insertions(+), 133 deletions(-) ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 08/27] drm/i915/icl: Ringbuffer interrupt handling
On 2/1/2018 3:58 PM, Belgaumkar, Vinay wrote: On 1/15/2018 2:38 AM, Tvrtko Ursulin wrote: On 11/01/2018 19:17, Daniele Ceraolo Spurio wrote: On 10/01/18 02:12, Chris Wilson wrote: Quoting Paulo Zanoni (2018-01-09 23:23:17) From: Tvrtko UrsulinSince it is not possible to mask individual engine instances and they are all permanently unmasked we do not need to do anything for engine interrupt management. This scares me as we will more than double our interrupt generation rate as we have a breadcrumb interrupt after every request, just in case we need to synchronize with the request. We've been relying on the ability to mask those interrupts off, as historically we have been able to saturate cpus with the amount of interrupts we could generate. -Chris We do have masks per instance, but they're in gunit and not in the CS. they're defined in patch 4 of this series: +#define GEN11_RCS0_RSVD_INTR_MASK _MMIO(0x190090) +#define GEN11_BCS_RSVD_INTR_MASK _MMIO(0x1900a0) +#define GEN11_VCS0_VCS1_INTR_MASK _MMIO(0x1900a8) +#define GEN11_VCS2_VCS3_INTR_MASK _MMIO(0x1900ac) +#define GEN11_VECS0_VECS1_INTR_MASK _MMIO(0x1900d0) Each instance gets half of a register for a 16 bits vector. I think there was some gotcha with the masks being in gunit but can't remember exactly what. Tvrtko, do you remember anything on this? I remember the message was to not use them since they were pencilled in for removal from the HW. There was some issue with them in general which made that decision attractive, coupled with the fact other OS did not have a need for them. If that removal did not materialize we could perhaps revisit the plans. It would require a locked RMW cycle as minimum since there are two instances in each reg but that probably isn't such a big deal. So yeah, I think summary would be someone needs to pull on sleeves of a HW designer, or someone close, to check what happened with the removal plans, and what exactly was the problematic bit with frequent (un)masking. I asked the hardware team about the removal, and that does not seem to be the case. I have added you to the email if you want more clarification. Please disregard that comment, looks like there was already a separate discussion on this. Thanks, Vinay. Thanks, Vinay. Regards, Tvrtko ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 08/27] drm/i915/icl: Ringbuffer interrupt handling
On 1/15/2018 2:38 AM, Tvrtko Ursulin wrote: On 11/01/2018 19:17, Daniele Ceraolo Spurio wrote: On 10/01/18 02:12, Chris Wilson wrote: Quoting Paulo Zanoni (2018-01-09 23:23:17) From: Tvrtko UrsulinSince it is not possible to mask individual engine instances and they are all permanently unmasked we do not need to do anything for engine interrupt management. This scares me as we will more than double our interrupt generation rate as we have a breadcrumb interrupt after every request, just in case we need to synchronize with the request. We've been relying on the ability to mask those interrupts off, as historically we have been able to saturate cpus with the amount of interrupts we could generate. -Chris We do have masks per instance, but they're in gunit and not in the CS. they're defined in patch 4 of this series: +#define GEN11_RCS0_RSVD_INTR_MASK _MMIO(0x190090) +#define GEN11_BCS_RSVD_INTR_MASK _MMIO(0x1900a0) +#define GEN11_VCS0_VCS1_INTR_MASK _MMIO(0x1900a8) +#define GEN11_VCS2_VCS3_INTR_MASK _MMIO(0x1900ac) +#define GEN11_VECS0_VECS1_INTR_MASK _MMIO(0x1900d0) Each instance gets half of a register for a 16 bits vector. I think there was some gotcha with the masks being in gunit but can't remember exactly what. Tvrtko, do you remember anything on this? I remember the message was to not use them since they were pencilled in for removal from the HW. There was some issue with them in general which made that decision attractive, coupled with the fact other OS did not have a need for them. If that removal did not materialize we could perhaps revisit the plans. It would require a locked RMW cycle as minimum since there are two instances in each reg but that probably isn't such a big deal. So yeah, I think summary would be someone needs to pull on sleeves of a HW designer, or someone close, to check what happened with the removal plans, and what exactly was the problematic bit with frequent (un)masking. I asked the hardware team about the removal, and that does not seem to be the case. I have added you to the email if you want more clarification. Thanks, Vinay. Regards, Tvrtko ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] ✗ Fi.CI.IGT: failure for DRM management via cgroups (rev2)
== Series Details == Series: DRM management via cgroups (rev2) URL : https://patchwork.freedesktop.org/series/36837/ State : failure == Summary == Test perf_pmu: Subgroup busy-check-all-vecs0: pass -> DMESG-WARN (shard-hsw) Subgroup busy-start-rcs0: pass -> DMESG-WARN (shard-snb) Subgroup other-read-0: pass -> DMESG-WARN (shard-snb) pass -> DMESG-WARN (shard-hsw) pass -> DMESG-WARN (shard-apl) Subgroup idle-vcs0: pass -> DMESG-WARN (shard-snb) Subgroup busy-no-semaphores-vcs0: pass -> DMESG-WARN (shard-hsw) pass -> DMESG-WARN (shard-apl) Subgroup render-node-busy-rcs0: pass -> DMESG-WARN (shard-apl) Subgroup busy-no-semaphores-rcs0: pass -> DMESG-WARN (shard-snb) pass -> DMESG-WARN (shard-hsw) Subgroup most-busy-check-all-vcs0: pass -> DMESG-WARN (shard-snb) pass -> DMESG-WARN (shard-hsw) pass -> DMESG-WARN (shard-apl) Subgroup semaphore-wait-vecs0: pass -> DMESG-WARN (shard-apl) Subgroup interrupts: pass -> DMESG-WARN (shard-snb) pass -> DMESG-WARN (shard-hsw) Subgroup busy-vecs0: pass -> DMESG-WARN (shard-hsw) pass -> DMESG-WARN (shard-apl) Subgroup rc6: pass -> DMESG-WARN (shard-snb) pass -> DMESG-WARN (shard-apl) Subgroup busy-no-semaphores-vecs0: pass -> DMESG-WARN (shard-hsw) Subgroup semaphore-wait-vcs0: pass -> DMESG-WARN (shard-apl) Test kms_atomic_transition: Subgroup 1x-modeset-transitions-nonblocking: fail -> PASS (shard-apl) fdo#103336 Subgroup plane-toggle-modeset-transition: skip -> PASS (shard-snb) Test kms_vblank: Subgroup pipe-a-ts-continuation-suspend: pass -> FAIL (shard-hsw) Test kms_cursor_legacy: Subgroup basic-busy-flip-before-cursor-atomic: skip -> PASS (shard-snb) Test perf: Subgroup buffer-fill: fail -> PASS (shard-apl) fdo#103755 fdo#103336 https://bugs.freedesktop.org/show_bug.cgi?id=103336 fdo#103755 https://bugs.freedesktop.org/show_bug.cgi?id=103755 shard-apltotal:2836 pass:1742 dwarn:9 dfail:0 fail:21 skip:1064 time:11404s shard-hswtotal:2836 pass:1724 dwarn:9 dfail:0 fail:12 skip:1090 time:11212s shard-snbtotal:2836 pass:1321 dwarn:8 dfail:0 fail:10 skip:1497 time:6157s == Logs == For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7856/shards.html ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [IGT PATCH RFC] tools: Introduce intel_cgroup tool
+cgroups list since this discussion goes back to the general design On Thu, Feb 01, 2018 at 08:27:33PM +, Chris Wilson wrote: > Quoting Matt Roper (2018-02-01 19:56:15) > > intel_cgroup is used to modify i915 cgroup parameters. At the moment only a > > single parameter is supported (GPU priority offset). In the future the > > driver > > may support additional parameters as well (e.g., limits on memory usage). > > > > Signed-off-by: Matt Roper> > +#define I915_CGROUP_PARAM_PRIORITY_OFFSET 0x1 > > Hmm. Could we not avoid drm_ioctl + well known param names and use a > more generic tool to set cgroup attributes? Just feels wrong that a > such a generic interface boils down to a driver specific ioctl. > -Chris There are a few different design choices here, so feedback like this is definitely what I'm looking for with this series. A few goals we should keep in mind as we consider options: * I'd like to keep the attributes we set tied to a driver/device in some manner. E.g., if we have a machine with multiple GPU's, we should be able to set card0 priority independently of card1 priority A DRM or driver ioctl makes this easy, but it certainly isn't the only approach. * Some of the attributes we want to manage don't fall under the design goals of formal cgroup controllers. I.e., they don't care about the hierarchy layout of cgroups at all; they only care about the details of the process' final leaf node. GPU priority as implemented in this series is an example of that. * Other attributes we'll eventually want to control (such as graphics memory usage), probably _will_ want to take the hierarchy design into account, the same way real cgroup controllers like the mem controller do. * The drivers that want to make use of this functionality may be built as modules rather than compiled directly into the kernel. This is important because the cgroups subsystem removed the ability to have cgroup controllers in modules a few years ago. While it might be possible to resurrect module-based cgroup controller support, my impression is that the cgroups community would prefer a separate non-controller mechanism for doing what we want to do. E.g., see https://www.spinics.net/lists/cgroups/msg18674.html for some brief discussion on this topic. I think the nicest interface for setting cgroup attributes would be to find a way to add our own kernfs entries to the cgroup filesystem, the same way real cgroup controllers do. Then we could do something like "echo 123 > /cgroup2/apps/highprio/i915.card0.priority" and not need to call any ioctl's at all. Without creating an actual cgroup controller, I think this might be challenging, but I'm investigating it on the side right now to see if it's a viable option. There are other options that might be suitable as well, but I haven't throught through them in details: * Hang the ioctl() off the cgroup filesystem entry rather than the DRM device node. Not sure if this gains us anything given that we still want to track data on a per-device basis. * Add a cgroup filesystem entry that just handles write() events of the form "drivername:key:value" to trigger driver callbacks. This might be a decent compromise. E.g., we could issue a command like: echo "i915.card0:prio_base:123" > /cgroup2/foo/cgroup.driver_attr and the kernfs handler for that file would looks for a cgroup_driver registered under the name "i915.card0" to figure out what driver callback to call for the key/value data. The second bullet above is growing on me as I think more about it, so I might try implementing that approach in the third version of my series if nobody has other suggestions or feedback. Matt -- Matt Roper Graphics Software Engineer IoTG Platform Enabling & Development Intel Corporation (916) 356-2795 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v7 2/6] drm/i915/guc: Rename guc_ggtt_offset to intel_guc_ggtt_offset
On 02/01/2018 02:35 PM, Chris Wilson wrote: Quoting Yaodong Li (2018-02-01 19:47:53) On 01/31/2018 11:38 PM, Chris Wilson wrote: Quoting Jackie Li (2018-01-19 01:29:28) GuC related exported functions should start with "intel_guc_" prefix and pass intel_guc as the first parameter since its guc related. Current guc_ggtt_offset() failed to follow this code convention. But it was not, and still does not operate on the guc. Is that changing? this problem is that it's guc related and the following patches do need to access the data from intel_guc. Do you think it's getting better if I add a sentence like "the future patches will need to access the intel_guc to verify the offset"? That's the idea. You need to explain _why_ you need a particular change, in some cases like this where it's not clear from the context of the patch, you need to fill in the missing details for the reader. In patch series like this where there is upfront refactoring required, remember the reader is starting at the beginning with no idea of what's coming next, so a bit^Wlot of foreshadowing is required in the story you tell. Got it. Will keep that in mind. Thank you Chris:-) Regards, -Jackie ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v7 2/6] drm/i915/guc: Rename guc_ggtt_offset to intel_guc_ggtt_offset
Quoting Yaodong Li (2018-02-01 19:47:53) > > On 01/31/2018 11:38 PM, Chris Wilson wrote: > > Quoting Jackie Li (2018-01-19 01:29:28) > >> GuC related exported functions should start with "intel_guc_" > >> prefix and pass intel_guc as the first parameter since its guc > >> related. Current guc_ggtt_offset() failed to follow this code > >> convention. > > But it was not, and still does not operate on the guc. Is that changing? > this problem is that it's guc related and the following patches do need > to access the data from intel_guc. Do you think it's getting better > if I add a sentence like "the future patches will need to access > the intel_guc to verify the offset"? That's the idea. You need to explain _why_ you need a particular change, in some cases like this where it's not clear from the context of the patch, you need to fill in the missing details for the reader. In patch series like this where there is upfront refactoring required, remember the reader is starting at the beginning with no idea of what's coming next, so a bit^Wlot of foreshadowing is required in the story you tell. -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v7 5/6] drm/i915/guc: Check the locking status of GuC WOPCM registers
On 01/31/2018 11:41 PM, Chris Wilson wrote: - Fixed patch format issues Cc: Michal WajdeczkoCc: Chris Wilson Cc: Joonas Lahtinen Signed-off-by: Jackie Li --- +static inline bool __reg_locked(struct drm_i915_private *dev_priv, + i915_reg_t reg) +{ + return !!(I915_READ(reg) & GUC_WOPCM_REG_LOCKED); Why the double cast to bool? My thought was the code would be clearer to use bool as the return type and have a explicit cast to bool. Are you suggesting I should use a return type such as *int* and remove the double cast? Regards, -Jackie ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] ✗ Fi.CI.IGT: failure for drm/i915: Enable inject_load_failure only in DEBUG config
== Series Details == Series: drm/i915: Enable inject_load_failure only in DEBUG config URL : https://patchwork.freedesktop.org/series/37495/ State : failure == Summary == Test perf: Subgroup oa-exponents: pass -> FAIL (shard-apl) fdo#102254 Subgroup enable-disable: pass -> FAIL (shard-apl) fdo#103715 Test gem_eio: Subgroup in-flight-external: fail -> PASS (shard-hsw) fdo#104676 Test kms_sysfs_edid_timing: warn -> PASS (shard-apl) fdo#100047 Test kms_flip: Subgroup flip-vs-panning-vs-hang-interruptible: dmesg-warn -> PASS (shard-snb) fdo#103821 Subgroup 2x-plain-flip-ts-check: pass -> FAIL (shard-hsw) Subgroup plain-flip-fb-recreate: fail -> PASS (shard-hsw) fdo#100368 Test drv_suspend: Subgroup forcewake: pass -> SKIP (shard-snb) k.org#196691 Test perf_pmu: Subgroup semaphore-wait-vecs0: pass -> FAIL (shard-apl) fdo#102254 https://bugs.freedesktop.org/show_bug.cgi?id=102254 fdo#103715 https://bugs.freedesktop.org/show_bug.cgi?id=103715 fdo#104676 https://bugs.freedesktop.org/show_bug.cgi?id=104676 fdo#100047 https://bugs.freedesktop.org/show_bug.cgi?id=100047 fdo#103821 https://bugs.freedesktop.org/show_bug.cgi?id=103821 fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368 k.org#196691 https://bugzilla.kernel.org/show_bug.cgi?id=196691 shard-apltotal:2836 pass:1747 dwarn:1 dfail:0 fail:24 skip:1064 time:12430s shard-hswtotal:2836 pass:1733 dwarn:1 dfail:0 fail:11 skip:1090 time:11611s shard-snbtotal:2836 pass:1327 dwarn:1 dfail:0 fail:10 skip:1498 time:6467s Blacklisted hosts: shard-kbltotal:2828 pass:1835 dwarn:28 dfail:1 fail:22 skip:941 time:9077s == Logs == For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7854/shards.html ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v7 3/6] drm/i915/guc: Implement dynamic GuC WOPCM offset and size
On 02/01/2018 12:38 AM, Sagar Arun Kamble wrote: On 1/19/2018 6:59 AM, Jackie Li wrote: Hardware may have specific restrictions on GuC WOPCM size It would be good if you can tell about Gen9/CNL restriction briefly here. versus HuC firmware size. With static GuC WOPCM size, there's no way to adjust the GuC WOPCM partition size based on the actual HuC firmware size, so that GuC/HuC loading failure would occur even if there was enough WOPCM space for both GuC and HuC firmware. This patch enables the dynamic calculation of the GuC WOPCM aperture size used by GuC and HuC firmware. we are also calculating for HuC? Strictly speaking, we are calculating the GuC WOPCM offset based on GuC & HuC firmware sizes. Will make it clearer. Thanks. + offset = huc_fw_size + WOPCM_RESERVED_SIZE; + if (offset >= WOPCM_DEFAULT_SIZE) + return -E2BIG; + + /* Hardware requires GuC WOPCM offset needs to be 16K aligned. */ + offset = ALIGN(offset, WOPCM_OFFSET_ALIGNMENT); + if ((offset + reserved) >= WOPCM_DEFAULT_SIZE) + return -E2BIG; + + top = WOPCM_DEFAULT_SIZE - offset; + size = top - reserved; + + /* GuC WOPCM size must be 4K aligned. */ + size &= GUC_WOPCM_SIZE_MASK; + ALIGN(size, PAGE_SIZE)? Can avoid this new macro. If you want to stay with GUC_WOPCM_SIZE_MASK, then that macro needs to be in guc_wopcm.h I'd like to go with GUC_WOPCM_SIZE_MASK here since there's no sign that it should be related to page size. *Align* seems not accurate here since I actually wanted to trim the size to 4k boundary, will update the comments. Thanks! ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH RFC v2 3/7] cgroup: Add interface to allow drivers to lookup process cgroup membership
On Thu, Feb 01, 2018 at 08:49:32PM +, Chris Wilson wrote: > Quoting Matt Roper (2018-02-01 19:53:11) ... > > +struct cgroup * > > +cgroup_for_driver_process(struct pid *pid) > > +{ > > + struct task_struct *task = pid_task(pid, PIDTYPE_PID); > > + > > + mutex_lock(_mutex); > > + spin_lock_irq(_set_lock); > > + task_cgroup_from_root(task, _dfl_root); > > + spin_unlock_irq(_set_lock); > > + mutex_unlock(_mutex); > > Gut feeling says use task = get_pid_task(); if (!task) return NULL and > put_task_struct(task); for safety. > > Shouldn't there be a return here? Woops, looks like I mis-squashed some of my fixes for this patch in to patch #4 instead. But yeah, get_pid_task() is better in general. Matt > > +} > -Chris > -- > To unsubscribe from this list: send the line "unsubscribe cgroups" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- Matt Roper Graphics Software Engineer IoTG Platform Enabling & Development Intel Corporation (916) 356-2795 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] CI igt-all on drm-intel-next-fixes gem_exec_* and other gem_*
Quoting Rodrigo Vivi (2018-02-01 21:00:45) > Ok, things look much cleaner(greener) on last run after I rebased on > top of a more recent drm-next. > > https://intel-gfx-ci.01.org/tree/drm-intel-next-fixes/shards.html > > My only concern right now is this one: > > https://intel-gfx-ci.01.org/tree/drm-intel-next-fixes/CI_DINF_97/shard-kbl5/igt@gem_soft...@noreloc-s3.html > > <7>[ 256.521580] [IGT] gem_softpin: starting subtest noreloc-S3 > <7>[ 256.561583] [drm:gmbus_xfer [i915]] GMBUS [i915 gmbus dpc] timed > out, falling back to bit banging on pin 4 > <7>[ 256.564321] [drm:drm_dp_dual_mode_detect] DP dual mode HDMI ID: (err > -6) > <7>[ 256.564326] [drm:drm_helper_hpd_irq_event] > [CONNECTOR:70:HDMI-A-1] status updated from disconnected to > disconnected > ��� > > Could this be caused by?: > > commit 90024a5951029685acc5396258f1b0de9b23cf4a > Author: Stefan Brüns> Date: Sun Dec 31 23:34:54 2017 +0100 > > drm/i915: Try EDID bitbanging on HDMI after failed read Indeterminate. The loss of the log does suggest that something went catastrophically wrong on suspend, and we have the circumspect evidence that hpd was active at that time. But we do flush those workqueues, right? So it may just be the grue that sometimes likes to eat machines for no reason. -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] i915 PSR test results and cursor lag
Quoting Andy Lutomirski (2018-02-01 21:04:30) > I got this after a recent suspend/resume: > > Feb 01 09:44:34 laptop systemd-logind[2412]: Lid closed. > Feb 01 09:44:34 laptop systemd-logind[2412]: device-enumerator: scan all dirs > Feb 01 09:44:34 laptop systemd-logind[2412]: device-enumerator: > scanning /sys/bus > Feb 01 09:44:34 laptop systemd-logind[2412]: device-enumerator: > scanning /sys/class > Feb 01 09:44:34 laptop systemd-logind[2412]: Failed to open > configuration file '/etc/systemd/sleep.conf': No such file or > directory > Feb 01 09:44:34 laptop systemd-logind[2412]: Suspending... > Feb 01 09:44:34 laptop systemd-logind[2412]: Sent message type=signal > sender=n/a destination=n/a object=/org/freedesktop/login1 > interface=org.freedesktop.login1.Manager member=PrepareForSleep > cookie=570 reply > Feb 01 09:44:34 laptop systemd-logind[2412]: Got message > type=method_call sender=:1.46 destination=:1.1 > object=/org/freedesktop/login1/session/_32 > interface=org.freedesktop.login1.Session member=ReleaseDevice > Feb 01 09:44:34 laptop systemd-logind[2412]: Sent message type=signal > sender=n/a destination=:1.46 > object=/org/freedesktop/login1/session/_32 > interface=org.freedesktop.login1.Session member=PauseDevice cookie > Feb 01 09:44:34 laptop gnome-shell[2630]: Failed to apply DRM plane > transform 0: Permission denied > Feb 01 09:44:34 laptop gnome-shell[2630]: drmModeSetCursor2 failed > with (Permission denied), drawing cursor with OpenGL from now on > > But I don't see the word "cursor" in my system logs before the first > suspend. What am I looking for? This is Fedora 27 running a Gnome > Wayland session, but it hasn't been reinstalled in some time, so it's > possible that there are some weird settings sitting around. But I did > check and I have no weird i915 parameters. You are using gnome-shell as the display server. From that it appears to have started off with a HW cursor and switched to a SW cursor after suspend. Did you notice a change in behaviour? After rebooting or just restarting gnome-shell? > Also, are these things potentially related: > > [ 3067.702527] [drm:intel_pipe_update_start [i915]] *ERROR* Potential > atomic update failure on pipe A They are just "missed the immediate vblank for the screen update" messages. Should not be related to PSR, but may cause jitter by delaying the odd screen update. > As I'm typing this, I've seen a couple instances of what seems like a > full *second* of cursor latency, but I've only gotten the potential > atomic update failure once. > > And is there any straightforward tracing to do to distinguish between > PSR exit latency and other potential sources of latency? It looks plausible that we could at least report how long it takes the registers to reflect the change in state (but we don't). The best source of information atm is /sys/kernel/debug/dri/0/i915_edp_psr_status. -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] i915 PSR test results and cursor lag
On Thu, Feb 1, 2018 at 9:53 AM, Chris Wilsonwrote: > Quoting Andy Lutomirski (2018-02-01 17:40:22) >> *However*, I do see one unfortunate side effect of turning on PSR. It >> seems that, when I move my cursor a little bit after a few seconds of >> doing nothing, there seems to be a little bit of lag, as if either a >> few frames are dropped at the beginning of the motion or maybe the >> entire motion is delayed a bit. I don't notice a similar delay when >> typing, so I'm wondering if maybe there's a minor driver bug in which >> the driver doesn't kick the panel out of PSR quite as quickly when the >> cursor is updated as it does when the framebuffer is updated. > > One thing that's important know regarding the cursor is whether the > display server is using a HW cursor or SW cursor. Could you please attach > the log from the display server (or if you are using a stock > distribution that's probably enough to work out what it is using)? > -Chris Looking at the logs, I see a few things. First, I have a few of these: Feb 01 09:24:24 laptop kernel: [drm:intel_pipe_update_start [i915]] *ERROR* Potential atomic update failure on pipe A Feb 01 09:24:48 laptop org.gnome.Shell.desktop[3261]: libinput error: event15 - libinput error: DLL0704:01 06CB:76AE Touchpad: libinput error: kernel bug: Touch jump detected and discarded. Feb 01 09:24:48 laptop org.gnome.Shell.desktop[3261]: See https://wayland.freedesktop.org/libinput/doc/1.9.3/touchpad_jumping_cursor.html for details Feb 01 09:24:50 laptop org.gnome.Shell.desktop[3261]: libinput error: event15 - libinput error: DLL0704:01 06CB:76AE Touchpad: libinput error: kernel bug: Touch jump detected and discarded. Feb 01 09:24:50 laptop org.gnome.Shell.desktop[3261]: See https://wayland.freedesktop.org/libinput/doc/1.9.3/touchpad_jumping_cursor.html for details (Hi, Peter!) So it's entirely possible that what I'm seeing is actually an input issue that's exacerbated by PSR for some bizarre reason. I got this after a recent suspend/resume: Feb 01 09:44:34 laptop systemd-logind[2412]: Lid closed. Feb 01 09:44:34 laptop systemd-logind[2412]: device-enumerator: scan all dirs Feb 01 09:44:34 laptop systemd-logind[2412]: device-enumerator: scanning /sys/bus Feb 01 09:44:34 laptop systemd-logind[2412]: device-enumerator: scanning /sys/class Feb 01 09:44:34 laptop systemd-logind[2412]: Failed to open configuration file '/etc/systemd/sleep.conf': No such file or directory Feb 01 09:44:34 laptop systemd-logind[2412]: Suspending... Feb 01 09:44:34 laptop systemd-logind[2412]: Sent message type=signal sender=n/a destination=n/a object=/org/freedesktop/login1 interface=org.freedesktop.login1.Manager member=PrepareForSleep cookie=570 reply Feb 01 09:44:34 laptop systemd-logind[2412]: Got message type=method_call sender=:1.46 destination=:1.1 object=/org/freedesktop/login1/session/_32 interface=org.freedesktop.login1.Session member=ReleaseDevice Feb 01 09:44:34 laptop systemd-logind[2412]: Sent message type=signal sender=n/a destination=:1.46 object=/org/freedesktop/login1/session/_32 interface=org.freedesktop.login1.Session member=PauseDevice cookie Feb 01 09:44:34 laptop gnome-shell[2630]: Failed to apply DRM plane transform 0: Permission denied Feb 01 09:44:34 laptop gnome-shell[2630]: drmModeSetCursor2 failed with (Permission denied), drawing cursor with OpenGL from now on But I don't see the word "cursor" in my system logs before the first suspend. What am I looking for? This is Fedora 27 running a Gnome Wayland session, but it hasn't been reinstalled in some time, so it's possible that there are some weird settings sitting around. But I did check and I have no weird i915 parameters. Also, are these things potentially related: [ 3067.702527] [drm:intel_pipe_update_start [i915]] *ERROR* Potential atomic update failure on pipe A As I'm typing this, I've seen a couple instances of what seems like a full *second* of cursor latency, but I've only gotten the potential atomic update failure once. And is there any straightforward tracing to do to distinguish between PSR exit latency and other potential sources of latency? ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] CI igt-all on drm-intel-next-fixes gem_exec_* and other gem_*
Ok, things look much cleaner(greener) on last run after I rebased on top of a more recent drm-next. https://intel-gfx-ci.01.org/tree/drm-intel-next-fixes/shards.html My only concern right now is this one: https://intel-gfx-ci.01.org/tree/drm-intel-next-fixes/CI_DINF_97/shard-kbl5/igt@gem_soft...@noreloc-s3.html <7>[ 256.521580] [IGT] gem_softpin: starting subtest noreloc-S3 <7>[ 256.561583] [drm:gmbus_xfer [i915]] GMBUS [i915 gmbus dpc] timed out, falling back to bit banging on pin 4 <7>[ 256.564321] [drm:drm_dp_dual_mode_detect] DP dual mode HDMI ID: (err -6) <7>[ 256.564326] [drm:drm_helper_hpd_irq_event] [CONNECTOR:70:HDMI-A-1] status updated from disconnected to disconnected ��� Could this be caused by?: commit 90024a5951029685acc5396258f1b0de9b23cf4a Author: Stefan BrünsDate: Sun Dec 31 23:34:54 2017 +0100 drm/i915: Try EDID bitbanging on HDMI after failed read On Wed, Jan 31, 2018 at 10:10 PM, Rodrigo Vivi wrote: > Hi guys, > > As we run more rounds of CI on drm-intel-next-fixes > a standard gets visible there. > > This round of dinf started with CI_DINF_90. > > Specially gem_exec_* but also other gem_* tests fails randomly > on gen9 platforms: > > https://intel-gfx-ci.01.org/tree/drm-intel-next-fixes/shards.html > (last 5 columns of gen9 platforms) > > while it is pure clean green on drm-tip: > > https://intel-gfx-ci.01.org/tree/drm-tip/shards-all.html > > From the logs I looked it seems that it is always during hibernation > and most of the times some strange page faults and also not > only i915: > > <6>[ 36.647941] PM: Image saving progress: 70% > <3>[ 36.707234] e1000e :00:1f.6 eno1: Detected Hardware Unit Hang: > > Do you know what could justify this difference? > > Is there anything we could we be missing on our side? > > Thanks, > Rodrigo. > ___ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Rodrigo Vivi Blog: http://blog.vivi.eng.br ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH RFC v2 3/7] cgroup: Add interface to allow drivers to lookup process cgroup membership
Quoting Matt Roper (2018-02-01 19:53:11) > Drivers will need to save/restore the appropriate cgroup data for the process > submitting a driver request. Add an interface for drivers to determine the > cgroup for the userspace process submitting a driver request. > > Cc: Tejun Heo> Cc: cgro...@vger.kernel.org > Signed-off-by: Matt Roper > --- > include/linux/cgroup.h| 6 ++ > kernel/cgroup/cgroup_driver.c | 24 > 2 files changed, 30 insertions(+) > > diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h > index 0ba1374122c7..05ebfb97bcde 100644 > --- a/include/linux/cgroup.h > +++ b/include/linux/cgroup.h > @@ -842,6 +842,7 @@ void cgroup_driver_release(struct cgroup_driver *drv); > struct cgroup_driver_data * cgroup_driver_get_data(struct cgroup_driver *drv, >struct cgroup *cgrp, >bool *is_new); > +struct cgroup *cgroup_for_driver_process(struct pid *pid); > > #else /* !CONFIG_CGROUP_DRIVER */ > > @@ -857,6 +858,11 @@ cgroup_driver_get_data(struct cgroup_driver *drv, > { > return ERR_PTR(-EINVAL); > } > +static inline struct cgroup * > +cgroup_for_driver_process(struct pid *pid) > +{ > + return NULL; > +} > > #endif /* !CONFIG_CGROUP_DRIVER */ > > diff --git a/kernel/cgroup/cgroup_driver.c b/kernel/cgroup/cgroup_driver.c > index 0d893395dc7b..4f870cbb9212 100644 > --- a/kernel/cgroup/cgroup_driver.c > +++ b/kernel/cgroup/cgroup_driver.c > @@ -128,3 +128,27 @@ cgroup_driver_get_data(struct cgroup_driver *drv, > return data; > } > EXPORT_SYMBOL(cgroup_driver_get_data); > + > +/** > + * cgroup_for_driver_process - return the cgroup for a process > + * @pid: process to lookup cgroup for > + * > + * Returns the cgroup from the v2 hierarchy that a process belongs to. > + * This function is intended to be called from drivers and will obtain > + * the necessary cgroup locks. > + * > + * RETURNS: > + * Process' cgroup in the default (v2) hierarchy > + */ > +struct cgroup * > +cgroup_for_driver_process(struct pid *pid) > +{ > + struct task_struct *task = pid_task(pid, PIDTYPE_PID); > + > + mutex_lock(_mutex); > + spin_lock_irq(_set_lock); > + task_cgroup_from_root(task, _dfl_root); > + spin_unlock_irq(_set_lock); > + mutex_unlock(_mutex); Gut feeling says use task = get_pid_task(); if (!task) return NULL and put_task_struct(task); for safety. Shouldn't there be a return here? > +} -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 01/17] drm/i915/icl: add the main CDCLK functions
On Thu, Feb 01, 2018 at 06:08:29PM -0200, Paulo Zanoni wrote: > Em Seg, 2018-01-29 às 12:51 +0200, Imre Deak escreveu: > > On Tue, Jan 23, 2018 at 05:05:20PM -0200, Paulo Zanoni wrote: > > > This commit adds the basic CDCLK functions, but it's still missing > > > pieces of the display initialization sequence. > > > > > > v2: > > > - Implement the voltage levels. > > > - Rebase. > > > > > > Signed-off-by: Paulo Zanoni> > > --- > > > drivers/gpu/drm/i915/i915_reg.h| 10 +- > > > drivers/gpu/drm/i915/intel_cdclk.c | 253 > > > - > > > drivers/gpu/drm/i915/intel_drv.h | 2 + > > > 3 files changed, 261 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h > > > b/drivers/gpu/drm/i915/i915_reg.h > > > index abd9ee876186..d72e206b2b9f 100644 > > > --- a/drivers/gpu/drm/i915/i915_reg.h > > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > > @@ -7113,8 +7113,12 @@ enum { > > > #define SKL_DFSM_PIPE_B_DISABLE (1 << 21) > > > #define SKL_DFSM_PIPE_C_DISABLE (1 << 28) > > > > > > -#define SKL_DSSM _MMIO(0x51004) > > > -#define CNL_DSSM_CDCLK_PLL_REFCLK_24MHz (1 << 31) > > > +#define SKL_DSSM _MMIO(0x51004) > > > +#define CNL_DSSM_CDCLK_PLL_REFCLK_24MHz (1 << 31) > > > +#define ICL_DSSM_CDCLK_PLL_REFCLK_MASK (7 << 29) > > > +#define ICL_DSSM_CDCLK_PLL_REFCLK_24MHz (0 << 29) > > > +#define ICL_DSSM_CDCLK_PLL_REFCLK_19_2MHz(1 << 29) > > > +#define ICL_DSSM_CDCLK_PLL_REFCLK_38_4MHz(2 << 29) > > > > > > #define GEN7_FF_SLICE_CS_CHICKEN1_MMIO(0x20e0) > > > #define GEN9_FFSC_PERCTX_PREEMPT_CTRL (1<<14) > > > @@ -8760,6 +8764,8 @@ enum skl_power_gate { > > > #define BXT_CDCLK_CD2X_PIPE_NONEBXT_CDCLK_CD2X_PIPE(3) > > > #define BXT_CDCLK_SSA_PRECHARGE_ENABLE (1<<16) > > > #define CDCLK_FREQ_DECIMAL_MASK (0x7ff) > > > +#define ICL_CDCLK_CD2X_PIPE(pipe) ((pipe) << 19) > > > +#define ICL_CDCLK_CD2X_PIPE_NONEICL_CDCLK_CD2X_PIPE(7) > > > > > > /* LCPLL_CTL */ > > > #define LCPLL1_CTL _MMIO(0x46010) > > > diff --git a/drivers/gpu/drm/i915/intel_cdclk.c > > > b/drivers/gpu/drm/i915/intel_cdclk.c > > > index c4392ea34a3d..d867956d5a9f 100644 > > > --- a/drivers/gpu/drm/i915/intel_cdclk.c > > > +++ b/drivers/gpu/drm/i915/intel_cdclk.c > > > @@ -1766,6 +1766,215 @@ static void cnl_sanitize_cdclk(struct > > > drm_i915_private *dev_priv) > > > dev_priv->cdclk.hw.vco = -1; > > > } > > > > > > +static int icl_calc_cdclk(int min_cdclk, unsigned int ref) > > > +{ > > > + int ranges_24[] = { 312000, 552000, 648000 }; > > > + int ranges_19_38[] = { 307200, 556800, 652800 }; > > > + int *ranges; > > > + > > > + switch (ref) { > > > + default: > > > + MISSING_CASE(ref); > > > + case 24000: > > > + ranges = ranges_24; > > > + break; > > > + case 19200: > > > + case 38400: > > > + ranges = ranges_19_38; > > > + break; > > > + } > > > + > > > + if (min_cdclk > ranges[1]) > > > + return ranges[2]; > > > + else if (min_cdclk > ranges[0]) > > > + return ranges[1]; > > > + else > > > + return ranges[0]; > > > +} > > > + > > > +static int icl_calc_cdclk_pll_vco(struct drm_i915_private > > > *dev_priv, int cdclk) > > > +{ > > > + int ratio; > > > + > > > + /* 50MHz == CDCLK PLL disabled. */ > > > + if (cdclk == 5) > > > > Here and everywhere else cdclk.hw.bypass should be used instead of > > hard > > coding it. > > I really don't understand your comment, and I also looked at "drm/i915: > Add tracking for CDCLK bypass frequency" and don't understand it > either. > > On our documentation I can't find anything named "bypass clock", and > there's no explanation in our code nor the commit message. The 50MHz is > the CDCLK value (not "bypass clock") when the CDCLK PLL is disabled. cdclk.hw.bypass is the CDCLK frequency when the CDCLK PLL is disabled. The clock source for this has different names on different platforms, on ICL it's called croclk or slow-clock by BSpec which is a 100MHz clock. This is divided by 2, resulting in the 50MHz bypass frequency, as shown under "Icelake Clocks"/"CD Clock" in BSpec. > In the driver, I see the "bypass" clock is set to the refclk we read > from the hardware, and that has nothing to do with the 50MHz we're > talking about at that point. We can't even read that 50MHz value from > the hardware. On ICL, the refclk is either 24, 19.2 or 38.4, we can't > read the 50 anywhere. Yes, on ICL the bypass frequency is not refclk based on the above, but the croclk/2 or 50MHz, so that's what cdclk.hw.bypass needs to be set to. > I'll really need a better explanation on what exactly you meant with > this "bypass" clock thing if I need to implement it on ICL. I just > can't see those "5" references going away. I can't see what's wrong > with this code. We want to handle this part of
Re: [Intel-gfx] i915 PSR test results and cursor lag
Quoting Kristian Høgsberg (2018-02-01 20:22:40) > On Thu, Feb 1, 2018 at 9:53 AM Chris Wilson> wrote: > > > Quoting Andy Lutomirski (2018-02-01 17:40:22) > > > *However*, I do see one unfortunate side effect of turning on PSR. It > > > seems that, when I move my cursor a little bit after a few seconds of > > > doing nothing, there seems to be a little bit of lag, as if either a > > > few frames are dropped at the beginning of the motion or maybe the > > > entire motion is delayed a bit. I don't notice a similar delay when > > > typing, so I'm wondering if maybe there's a min > > or driver bug in which > > > the driver doesn't kick the panel out of PSR quite as quickly when the > > > cursor is updated as it does when the framebuffer is updated. > > > One thing that's important know regarding the cursor is whether the > > display server is using a HW cursor or SW cursor. Could you please attach > > the log from the display server (or if you are using a stock > > distribution that's probably enough to work out what it is using)? > > -Chris > > We had a similar problem for Rockchip in ChromeOS and ended up using an > input handler to let us start the PSR exit as early as possible: Reminds me of mutter devs suggesting that we may like to kick the gpu to max clocks high frequency on any input activity as well. (I'm still not convinced that's a good idea, for mundane typing we barely need to wake up the gpu. :) I guess it all depends on expected wakeup latencies, but I didn't think PSR had multi-frame lag? -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [IGT PATCH RFC] tools: Introduce intel_cgroup tool
Quoting Matt Roper (2018-02-01 19:56:15) > intel_cgroup is used to modify i915 cgroup parameters. At the moment only a > single parameter is supported (GPU priority offset). In the future the driver > may support additional parameters as well (e.g., limits on memory usage). > > Signed-off-by: Matt Roper> --- > tools/Makefile.sources | 1 + > tools/intel_cgroup.c | 103 > + > 2 files changed, 104 insertions(+) > create mode 100644 tools/intel_cgroup.c > > diff --git a/tools/Makefile.sources b/tools/Makefile.sources > index abd23a0f..b30216c4 100644 > --- a/tools/Makefile.sources > +++ b/tools/Makefile.sources > @@ -11,6 +11,7 @@ tools_prog_lists =\ > intel_reg \ > intel_backlight \ > intel_bios_dumper \ > + intel_cgroup\ > intel_display_crc \ > intel_display_poller\ > intel_forcewaked\ > diff --git a/tools/intel_cgroup.c b/tools/intel_cgroup.c > new file mode 100644 > index ..ce781b08 > --- /dev/null > +++ b/tools/intel_cgroup.c > @@ -0,0 +1,103 @@ > +/* > + * Copyright © 2018 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 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 > +#include > +#include > +#include > +#include > +#include > + > +#include "igt.h" > +#include "xf86drm.h" > +#include "xf86drmMode.h" > + > +#define I915_CGROUP_PARAM_PRIORITY_OFFSET 0x1 Hmm. Could we not avoid drm_ioctl + well known param names and use a more generic tool to set cgroup attributes? Just feels wrong that a such a generic interface boils down to a driver specific ioctl. -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH RFC v2 7/7] drm/i915: Add context priority & priority offset to debugfs
Quoting Matt Roper (2018-02-01 19:53:15) > Update i915_context_status to include priority information. > > Signed-off-by: Matt Roper> --- > drivers/gpu/drm/i915/i915_debugfs.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c > b/drivers/gpu/drm/i915/i915_debugfs.c > index 3849ded354e3..e9d8e20155b1 100644 > --- a/drivers/gpu/drm/i915/i915_debugfs.c > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > @@ -1915,6 +1915,9 @@ static int i915_context_status(struct seq_file *m, void > *unused) > seq_putc(m, ctx->remap_slice ? 'R' : 'r'); > seq_putc(m, '\n'); > > + seq_printf(m, "Priority %d (prio offset = %d)\n", > + ctx->priority, ctx->priority_offset); s/prio offset/cgroup offset/ ? "prio" itself I think is redundant given that we are providing further details of "Priority" -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] i915 PSR test results and cursor lag
On Thu, Feb 1, 2018 at 9:53 AM Chris Wilsonwrote: > Quoting Andy Lutomirski (2018-02-01 17:40:22) > > *However*, I do see one unfortunate side effect of turning on PSR. It > > seems that, when I move my cursor a little bit after a few seconds of > > doing nothing, there seems to be a little bit of lag, as if either a > > few frames are dropped at the beginning of the motion or maybe the > > entire motion is delayed a bit. I don't notice a similar delay when > > typing, so I'm wondering if maybe there's a min > or driver bug in which > > the driver doesn't kick the panel out of PSR quite as quickly when the > > cursor is updated as it does when the framebuffer is updated. > One thing that's important know regarding the cursor is whether the > display server is using a HW cursor or SW cursor. Could you please attach > the log from the display server (or if you are using a stock > distribution that's probably enough to work out what it is using)? > -Chris We had a similar problem for Rockchip in ChromeOS and ended up using an input handler to let us start the PSR exit as early as possible: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-4.4/drivers/gpu/drm/rockchip/rockchip_drm_psr.c#345 It's similar in spirit to the interactive cpufreq governor: https://lwn.net/Articles/662209/ Kristian ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH RFC v2 6/7] drm/i915: Introduce 'priority offset' for GPU contexts
Quoting Matt Roper (2018-02-01 19:53:14) > There are cases where a system integrator may wish to raise/lower the > priority of GPU workloads being submitted by specific OS process(es), > independently of how the software self-classifies its own priority. > Exposing "priority offset" as an i915-specific cgroup parameter will > enable such system-level configuration. > > Normally GPU contexts start with a priority value of 0 > (I915_CONTEXT_DEFAULT_PRIORITY) and then may be adjusted up/down from > there via other mechanisms. We'd like to provide a system-level input > to the priority decision that will be taken into consideration, even > when userspace later attempts to set an absolute priority value via > I915_CONTEXT_PARAM_PRIORITY. The priority offset introduced here > provides a base value that will always be added to (or subtracted from) > the software's self-assigned priority value. > > This patch makes priority offset a cgroup-specific value; contexts will > be created with a priority offset based on the cgroup membership of the > process creating the context at the time the context is created. Note > that priority offset is assigned at context creation time; migrating a > process to a different cgroup or changing the offset associated with a > cgroup will only affect new context creation and will not alter the > behavior of existing contexts previously created by the process. > > Signed-off-by: Matt Roper> --- > drivers/gpu/drm/i915/i915_cgroup.c | 46 > + > drivers/gpu/drm/i915/i915_drv.h | 7 + > drivers/gpu/drm/i915/i915_gem_context.c | 6 +++-- > drivers/gpu/drm/i915/i915_gem_context.h | 9 +++ > include/uapi/drm/i915_drm.h | 1 + > 5 files changed, 67 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_cgroup.c > b/drivers/gpu/drm/i915/i915_cgroup.c > index 778af895fc00..73f3cfe31d66 100644 > --- a/drivers/gpu/drm/i915/i915_cgroup.c > +++ b/drivers/gpu/drm/i915/i915_cgroup.c > @@ -11,6 +11,8 @@ > > struct i915_cgroup_data { > struct cgroup_driver_data base; > + > + int priority_offset; > }; > > static inline struct i915_cgroup_data * > @@ -72,6 +74,8 @@ i915_cgroup_setparam_ioctl(struct drm_device *dev, void > *data, > struct cgroup *cgrp; > struct file *f; > struct inode *inode = NULL; > + struct cgroup_driver_data *cgrpdata; > + struct i915_cgroup_data *i915data; > int ret; > > if (!dev_priv->i915_cgroups) { > @@ -125,6 +129,17 @@ i915_cgroup_setparam_ioctl(struct drm_device *dev, void > *data, > return ret; > > switch (req->param) { > + case I915_CGROUP_PARAM_PRIORITY_OFFSET: > + cgrpdata = cgroup_driver_get_data(dev_priv->i915_cgroups, > cgrp, > + NULL); dev_priv is i915. So i915->i915_cgroups? Could you just have a i915_get_cgroup_data() returning struct i915_croup_data? > + if (IS_ERR(cgrpdata)) > + return PTR_ERR(cgrpdata); > + > + DRM_DEBUG_DRIVER("Setting cgroup priority offset to %lld\n", > +req->value); > + i915data = cgrp_to_i915(cgrpdata); > + i915data->priority_offset = req->value; We'll have to consider what bounds we think are sensible, and how internal policy interacts. Presumably another cgroup-param :) > + break; > default: > DRM_DEBUG_DRIVER("Invalid cgroup parameter %lld\n", > req->param); > return -EINVAL; > @@ -132,3 +147,34 @@ i915_cgroup_setparam_ioctl(struct drm_device *dev, void > *data, > > return 0; > } > + > +/** > + * i915_cgroup_get_prio_offset() - get prio offset for current proc's cgroup > + * @dev_priv: drm device > + * @file_priv: file handle for calling process > + * > + * RETURNS: > + * Priority offset associated with the calling process' cgroup in the default > + * (v2) hierarchy, otherwise 0 if no explicit priority has been assigned. > + */ > +int > +i915_cgroup_get_prio_offset(struct drm_i915_private *dev_priv, > + struct drm_i915_file_private *file_priv) > +{ > + struct cgroup *cgrp; > + struct cgroup_driver_data *cgrpdata; > + > + /* Ignore internally-created contexts not associated with a process */ > + if (!file_priv) > + return 0; > + > + cgrp = drm_file_get_cgroup(file_priv->file); > + if (WARN_ON(!cgrp)) > + return 0; Ah, this needs to pull from current then not drm_file->pid. > + > + cgrpdata = cgroup_driver_get_data(dev_priv->i915_cgroups, cgrp, NULL); > + if (IS_ERR(cgrpdata)) > + return 0; > + > + return cgrp_to_i915(cgrpdata)->priority_offset; > +} > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index
Re: [Intel-gfx] [PATCH RFC v2 5/7] drm/i915: cgroup integration
Quoting Matt Roper (2018-02-01 19:53:13) > +#include > + > +#include "i915_drv.h" > + > +struct i915_cgroup_data { > + struct cgroup_driver_data base; > +}; > + > +static inline struct i915_cgroup_data * > +cgrp_to_i915(struct cgroup_driver_data *data) > +{ Document your requirement that base is offset 0 (required for both i915_cgroup_alloc and i915_cgroup_free). BUILD_BUG_ON(offsetof(struct i915_cgroup_data, base)); (or make said code flexible) > + return container_of(data, struct i915_cgroup_data, base); > +} > + > +static struct cgroup_driver_data * > +i915_cgroup_alloc(struct cgroup_driver *drv) > +{ > + struct i915_cgroup_data *data; > + > + data = kzalloc(sizeof *data, GFP_KERNEL); > + return >base; > +} > + > +static void > +i915_cgroup_free(struct cgroup_driver_data *data) > +{ > + kfree(data); > +} ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] ✗ Fi.CI.BAT: failure for tools: Introduce intel_cgroup tool
== Series Details == Series: tools: Introduce intel_cgroup tool URL : https://patchwork.freedesktop.org/series/37505/ State : failure == Summary == IGT patchset build failed on latest successful build a20a69e25a18ec63236633b804d89cc0c0cea259 overlay: fix invalid pointer access make all-recursive Making all in lib make all-recursive Making all in . Making all in tests make[4]: Nothing to be done for 'all'. Making all in man make[2]: Nothing to be done for 'all'. Making all in tools Making all in null_state_gen make[3]: Nothing to be done for 'all'. Making all in registers make[3]: Nothing to be done for 'all'. CCLD intel_aubdump.la CCLD intel_audio_dump CCLD intel_reg CC intel_cgroup.o intel_cgroup.c: In function ‘main’: intel_cgroup.c:52:31: error: storage size of ‘req’ isn’t known struct drm_i915_cgroup_param req; ^~~ intel_cgroup.c:94:26: error: ‘DRM_IOCTL_I915_CGROUP_SETPARAM’ undeclared (first use in this function) ret = drmIoctl(drm_fd, DRM_IOCTL_I915_CGROUP_SETPARAM, ); ^~ intel_cgroup.c:94:26: note: each undeclared identifier is reported only once for each function it appears in intel_cgroup.c:52:31: warning: unused variable ‘req’ [-Wunused-variable] struct drm_i915_cgroup_param req; ^~~ Makefile:1170: recipe for target 'intel_cgroup.o' failed make[3]: *** [intel_cgroup.o] Error 1 Makefile:1234: recipe for target 'all-recursive' failed make[2]: *** [all-recursive] Error 1 Makefile:532: recipe for target 'all-recursive' failed make[1]: *** [all-recursive] Error 1 Makefile:464: recipe for target 'all' failed make: *** [all] Error 2 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] ✓ Fi.CI.BAT: success for DRM management via cgroups (rev2)
== Series Details == Series: DRM management via cgroups (rev2) URL : https://patchwork.freedesktop.org/series/36837/ State : success == Summary == Series 36837v2 DRM management via cgroups https://patchwork.freedesktop.org/api/1.0/series/36837/revisions/2/mbox/ Test gem_exec_reloc: Subgroup basic-cpu-gtt-active: incomplete -> PASS (fi-byt-j1900) fi-bdw-5557u total:288 pass:267 dwarn:0 dfail:0 fail:0 skip:21 time:421s fi-bdw-gvtdvmtotal:288 pass:264 dwarn:0 dfail:0 fail:0 skip:24 time:420s fi-blb-e6850 total:288 pass:223 dwarn:1 dfail:0 fail:0 skip:64 time:370s fi-bsw-n3050 total:288 pass:242 dwarn:0 dfail:0 fail:0 skip:46 time:483s fi-bwr-2160 total:288 pass:183 dwarn:0 dfail:0 fail:0 skip:105 time:282s fi-bxt-dsi total:288 pass:258 dwarn:0 dfail:0 fail:0 skip:30 time:484s fi-bxt-j4205 total:288 pass:259 dwarn:0 dfail:0 fail:0 skip:29 time:479s fi-byt-j1900 total:288 pass:253 dwarn:0 dfail:0 fail:0 skip:35 time:465s fi-byt-n2820 total:288 pass:249 dwarn:0 dfail:0 fail:0 skip:39 time:451s fi-cfl-s2total:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:568s fi-elk-e7500 total:288 pass:229 dwarn:0 dfail:0 fail:0 skip:59 time:413s fi-gdg-551 total:288 pass:179 dwarn:0 dfail:0 fail:1 skip:108 time:275s fi-glk-1 total:288 pass:260 dwarn:0 dfail:0 fail:0 skip:28 time:508s fi-hsw-4770 total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:387s fi-hsw-4770r total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:396s fi-ilk-650 total:288 pass:228 dwarn:0 dfail:0 fail:0 skip:60 time:409s fi-ivb-3520m total:288 pass:259 dwarn:0 dfail:0 fail:0 skip:29 time:451s fi-ivb-3770 total:288 pass:255 dwarn:0 dfail:0 fail:0 skip:33 time:412s fi-kbl-7500u total:288 pass:263 dwarn:1 dfail:0 fail:0 skip:24 time:455s fi-kbl-7560u total:288 pass:269 dwarn:0 dfail:0 fail:0 skip:19 time:496s fi-kbl-7567u total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:452s fi-kbl-r total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:500s fi-pnv-d510 total:288 pass:222 dwarn:1 dfail:0 fail:0 skip:65 time:579s fi-skl-6260u total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:422s fi-skl-6600u total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:506s fi-skl-6700hqtotal:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:527s fi-skl-6700k2total:288 pass:264 dwarn:0 dfail:0 fail:0 skip:24 time:484s fi-skl-6770hqtotal:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:475s fi-skl-guc total:288 pass:260 dwarn:0 dfail:0 fail:0 skip:28 time:413s fi-skl-gvtdvmtotal:288 pass:265 dwarn:0 dfail:0 fail:0 skip:23 time:430s fi-snb-2520m total:245 pass:211 dwarn:0 dfail:0 fail:0 skip:33 Blacklisted hosts: fi-glk-dsi total:288 pass:258 dwarn:0 dfail:0 fail:0 skip:30 time:470s abd8913d5ab0f40061166da741a2969f6fc8206a drm-tip: 2018y-02m-01d-19h-16m-13s UTC integration manifest efbe51d8915b drm/i915: Add context priority & priority offset to debugfs 2ad672fc3337 drm/i915: Introduce 'priority offset' for GPU contexts 955860954ef5 drm/i915: cgroup integration 86fff2c23f8a drm: Add helper to obtain cgroup of drm_file's owning process 33b5f92f9c44 cgroup: Add interface to allow drivers to lookup process cgroup membership 1b617cd2d0ec kernfs: Export kernfs_get_inode 09c9a771a688 cgroup: Allow drivers to store data associated with a cgroup == Logs == For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7856/issues.html ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH RFC v2 5/7] drm/i915: cgroup integration
Quoting Matt Roper (2018-02-01 19:53:13) > Register i915 as a consumer of the cgroup_driver framework and add an ioctl to > allow userspace to set i915-specific parameters associated with cgroups. > > Signed-off-by: Matt Roper> --- > drivers/gpu/drm/i915/Kconfig | 1 + > drivers/gpu/drm/i915/Makefile | 1 + > drivers/gpu/drm/i915/i915_cgroup.c | 134 > + > drivers/gpu/drm/i915/i915_drv.c| 7 ++ > drivers/gpu/drm/i915/i915_drv.h| 24 +++ > include/uapi/drm/i915_drm.h| 12 > 6 files changed, 179 insertions(+) > create mode 100644 drivers/gpu/drm/i915/i915_cgroup.c > > diff --git a/drivers/gpu/drm/i915/Kconfig b/drivers/gpu/drm/i915/Kconfig > index dfd95889f4b7..1c6b53ee76cd 100644 > --- a/drivers/gpu/drm/i915/Kconfig > +++ b/drivers/gpu/drm/i915/Kconfig > @@ -23,6 +23,7 @@ config DRM_I915 > select SYNC_FILE > select IOSF_MBI > select CRC32 > + select CGROUP_DRIVER if CGROUPS > help > Choose this option if you have a system that has "Intel Graphics > Media Accelerator" or "HD Graphics" integrated graphics, > diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile > index 3bddd8a06806..5f4a21f1a9df 100644 > --- a/drivers/gpu/drm/i915/Makefile > +++ b/drivers/gpu/drm/i915/Makefile > @@ -47,6 +47,7 @@ i915-y := i915_drv.o \ > i915-$(CONFIG_COMPAT) += i915_ioc32.o > i915-$(CONFIG_DEBUG_FS) += i915_debugfs.o intel_pipe_crc.o > i915-$(CONFIG_PERF_EVENTS) += i915_pmu.o > +i915-$(CONFIG_CGROUPS) += i915_cgroup.o CONFIG_CGROUP_DRIVER since that's strictly what the code depends on? -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH RFC v2 5/7] drm/i915: cgroup integration
Quoting Matt Roper (2018-02-01 19:53:13) > diff --git a/drivers/gpu/drm/i915/Kconfig b/drivers/gpu/drm/i915/Kconfig > index dfd95889f4b7..1c6b53ee76cd 100644 > --- a/drivers/gpu/drm/i915/Kconfig > +++ b/drivers/gpu/drm/i915/Kconfig > @@ -23,6 +23,7 @@ config DRM_I915 > select SYNC_FILE > select IOSF_MBI > select CRC32 > + select CGROUP_DRIVER if CGROUPS Hmm, I was expecting a DRM_CGROUP around here? Was it too small to be worth building conditionally? -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH RFC v2 4/7] drm: Add helper to obtain cgroup of drm_file's owning process
Quoting Matt Roper (2018-02-01 19:53:12) > +/** > + * drm_file_get_cgroup - obtain cgroup for drm_file's owning process > + * @file_priv: DRM file > + * > + * Obtains the cgroup from a specific hierarchy that the drm_file's owning > + * process belongs to. The cgroup may be used to set driver-specific > + * policy (priority, vram usage, etc.). > + */ > +static inline struct cgroup * > +drm_file_get_cgroup(struct drm_file *file_priv) > +{ > + return cgroup_for_driver_process(file_priv->pid); Just a word of warning file_priv->pid isn't a true indicator of the attached process's pid. E.g. DRI3 passes the struct drm_file over a UNIX socket, so pid is Xorg but the actual process is the GL client. Not sure if we can detect the transfer over the socket, but one suggestion has been to update file->pid based on who creates a context. -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 01/17] drm/i915/icl: add the main CDCLK functions
Em Sex, 2018-01-26 às 15:14 -0800, James Ausmus escreveu: > On Tue, Jan 23, 2018 at 05:05:20PM -0200, Paulo Zanoni wrote: > > This commit adds the basic CDCLK functions, but it's still missing > > pieces of the display initialization sequence. > > > > v2: > > - Implement the voltage levels. > > - Rebase. > > > > Signed-off-by: Paulo Zanoni> > --- > > drivers/gpu/drm/i915/i915_reg.h| 10 +- > > drivers/gpu/drm/i915/intel_cdclk.c | 253 > > - > > drivers/gpu/drm/i915/intel_drv.h | 2 + > > 3 files changed, 261 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h > > b/drivers/gpu/drm/i915/i915_reg.h > > index abd9ee876186..d72e206b2b9f 100644 > > --- a/drivers/gpu/drm/i915/i915_reg.h > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > @@ -7113,8 +7113,12 @@ enum { > > #define SKL_DFSM_PIPE_B_DISABLE(1 << 21) > > #define SKL_DFSM_PIPE_C_DISABLE(1 << 28) > > > > -#define SKL_DSSM _MMIO(0x51004) > > -#define CNL_DSSM_CDCLK_PLL_REFCLK_24MHz(1 << 31) > > +#define SKL_DSSM _MMIO(0x51004) > > +#define CNL_DSSM_CDCLK_PLL_REFCLK_24MHz(1 << 31) > > +#define ICL_DSSM_CDCLK_PLL_REFCLK_MASK (7 << 29) > > +#define ICL_DSSM_CDCLK_PLL_REFCLK_24MHz(0 << 29) > > +#define ICL_DSSM_CDCLK_PLL_REFCLK_19_2MHz (1 << 29) > > +#define ICL_DSSM_CDCLK_PLL_REFCLK_38_4MHz (2 << 29) > > > > #define GEN7_FF_SLICE_CS_CHICKEN1 _MMIO(0x20e0) > > #define GEN9_FFSC_PERCTX_PREEMPT_CTRL(1<<14) > > @@ -8760,6 +8764,8 @@ enum skl_power_gate { > > #define BXT_CDCLK_CD2X_PIPE_NONE BXT_CDCLK_CD2X_PIPE(3) > > #define BXT_CDCLK_SSA_PRECHARGE_ENABLE(1<<16) > > #define CDCLK_FREQ_DECIMAL_MASK (0x7ff) > > +#define ICL_CDCLK_CD2X_PIPE(pipe) ((pipe) << 19) > > This isn't right - pipe A is (0 << 19), but pipe B is (2 << 19), and > C > is (6 << 19). Right. I'll fix that once the other review comments on this patch are clarified. > > > +#define ICL_CDCLK_CD2X_PIPE_NONE ICL_CDCLK_CD2X_PIPE(7) > > > > /* LCPLL_CTL */ > > #define LCPLL1_CTL _MMIO(0x46010) > > diff --git a/drivers/gpu/drm/i915/intel_cdclk.c > > b/drivers/gpu/drm/i915/intel_cdclk.c > > index c4392ea34a3d..d867956d5a9f 100644 > > --- a/drivers/gpu/drm/i915/intel_cdclk.c > > +++ b/drivers/gpu/drm/i915/intel_cdclk.c > > @@ -1766,6 +1766,215 @@ static void cnl_sanitize_cdclk(struct > > drm_i915_private *dev_priv) > > dev_priv->cdclk.hw.vco = -1; > > } > > > > +static int icl_calc_cdclk(int min_cdclk, unsigned int ref) > > +{ > > + int ranges_24[] = { 312000, 552000, 648000 }; > > + int ranges_19_38[] = { 307200, 556800, 652800 }; > > + int *ranges; > > + > > + switch (ref) { > > + default: > > + MISSING_CASE(ref); > > + case 24000: > > + ranges = ranges_24; > > + break; > > + case 19200: > > + case 38400: > > + ranges = ranges_19_38; > > + break; > > + } > > + > > + if (min_cdclk > ranges[1]) > > + return ranges[2]; > > + else if (min_cdclk > ranges[0]) > > + return ranges[1]; > > + else > > + return ranges[0]; > > +} > > + > > +static int icl_calc_cdclk_pll_vco(struct drm_i915_private > > *dev_priv, int cdclk) > > +{ > > + int ratio; > > + > > + /* 50MHz == CDCLK PLL disabled. */ > > + if (cdclk == 5) > > + return 0; > > + > > + switch (cdclk) { > > + default: > > + MISSING_CASE(cdclk); > > + case 307200: > > + case 556800: > > + case 652800: > > + WARN_ON(dev_priv->cdclk.hw.ref != 19200 && > > + dev_priv->cdclk.hw.ref != 38400); > > + break; > > + case 312000: > > + case 552000: > > + case 648000: > > + WARN_ON(dev_priv->cdclk.hw.ref != 24000); > > + } > > + > > + ratio = cdclk / (dev_priv->cdclk.hw.ref / 2); > > + > > + return dev_priv->cdclk.hw.ref * ratio; > > +} > > + > > +static void icl_set_cdclk(struct drm_i915_private *dev_priv, > > + const struct intel_cdclk_state > > *cdclk_state) > > +{ > > + unsigned int cdclk = cdclk_state->cdclk; > > + unsigned int vco = cdclk_state->vco; > > + int ret; > > + u32 voltage_level; > > + > > + mutex_lock(_priv->pcu_lock); > > + ret = skl_pcode_request(dev_priv, SKL_PCODE_CDCLK_CONTROL, > > + SKL_CDCLK_PREPARE_FOR_CHANGE, > > + SKL_CDCLK_READY_FOR_CHANGE, > > + SKL_CDCLK_READY_FOR_CHANGE, 3); > > + mutex_unlock(_priv->pcu_lock); > > + if (ret) { > > + DRM_ERROR("Failed to inform PCU about cdclk change > > (%d)\n", > > + ret); > > + return; > > + } > > + > > + /* FIXME: We should also consider the DDI clock here. */ > > + switch (cdclk) { > > + case 307200: > > + case 312000: > > + voltage_level = 0; > > + break; > >
Re: [Intel-gfx] [PATCH 01/17] drm/i915/icl: add the main CDCLK functions
Em Seg, 2018-01-29 às 12:51 +0200, Imre Deak escreveu: > On Tue, Jan 23, 2018 at 05:05:20PM -0200, Paulo Zanoni wrote: > > This commit adds the basic CDCLK functions, but it's still missing > > pieces of the display initialization sequence. > > > > v2: > > - Implement the voltage levels. > > - Rebase. > > > > Signed-off-by: Paulo Zanoni> > --- > > drivers/gpu/drm/i915/i915_reg.h| 10 +- > > drivers/gpu/drm/i915/intel_cdclk.c | 253 > > - > > drivers/gpu/drm/i915/intel_drv.h | 2 + > > 3 files changed, 261 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h > > b/drivers/gpu/drm/i915/i915_reg.h > > index abd9ee876186..d72e206b2b9f 100644 > > --- a/drivers/gpu/drm/i915/i915_reg.h > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > @@ -7113,8 +7113,12 @@ enum { > > #define SKL_DFSM_PIPE_B_DISABLE(1 << 21) > > #define SKL_DFSM_PIPE_C_DISABLE(1 << 28) > > > > -#define SKL_DSSM _MMIO(0x51004) > > -#define CNL_DSSM_CDCLK_PLL_REFCLK_24MHz(1 << 31) > > +#define SKL_DSSM _MMIO(0x51004) > > +#define CNL_DSSM_CDCLK_PLL_REFCLK_24MHz(1 << 31) > > +#define ICL_DSSM_CDCLK_PLL_REFCLK_MASK (7 << 29) > > +#define ICL_DSSM_CDCLK_PLL_REFCLK_24MHz(0 << 29) > > +#define ICL_DSSM_CDCLK_PLL_REFCLK_19_2MHz (1 << 29) > > +#define ICL_DSSM_CDCLK_PLL_REFCLK_38_4MHz (2 << 29) > > > > #define GEN7_FF_SLICE_CS_CHICKEN1 _MMIO(0x20e0) > > #define GEN9_FFSC_PERCTX_PREEMPT_CTRL(1<<14) > > @@ -8760,6 +8764,8 @@ enum skl_power_gate { > > #define BXT_CDCLK_CD2X_PIPE_NONE BXT_CDCLK_CD2X_PIPE(3) > > #define BXT_CDCLK_SSA_PRECHARGE_ENABLE(1<<16) > > #define CDCLK_FREQ_DECIMAL_MASK (0x7ff) > > +#define ICL_CDCLK_CD2X_PIPE(pipe) ((pipe) << 19) > > +#define ICL_CDCLK_CD2X_PIPE_NONE ICL_CDCLK_CD2X_PIPE(7) > > > > /* LCPLL_CTL */ > > #define LCPLL1_CTL _MMIO(0x46010) > > diff --git a/drivers/gpu/drm/i915/intel_cdclk.c > > b/drivers/gpu/drm/i915/intel_cdclk.c > > index c4392ea34a3d..d867956d5a9f 100644 > > --- a/drivers/gpu/drm/i915/intel_cdclk.c > > +++ b/drivers/gpu/drm/i915/intel_cdclk.c > > @@ -1766,6 +1766,215 @@ static void cnl_sanitize_cdclk(struct > > drm_i915_private *dev_priv) > > dev_priv->cdclk.hw.vco = -1; > > } > > > > +static int icl_calc_cdclk(int min_cdclk, unsigned int ref) > > +{ > > + int ranges_24[] = { 312000, 552000, 648000 }; > > + int ranges_19_38[] = { 307200, 556800, 652800 }; > > + int *ranges; > > + > > + switch (ref) { > > + default: > > + MISSING_CASE(ref); > > + case 24000: > > + ranges = ranges_24; > > + break; > > + case 19200: > > + case 38400: > > + ranges = ranges_19_38; > > + break; > > + } > > + > > + if (min_cdclk > ranges[1]) > > + return ranges[2]; > > + else if (min_cdclk > ranges[0]) > > + return ranges[1]; > > + else > > + return ranges[0]; > > +} > > + > > +static int icl_calc_cdclk_pll_vco(struct drm_i915_private > > *dev_priv, int cdclk) > > +{ > > + int ratio; > > + > > + /* 50MHz == CDCLK PLL disabled. */ > > + if (cdclk == 5) > > Here and everywhere else cdclk.hw.bypass should be used instead of > hard > coding it. I really don't understand your comment, and I also looked at "drm/i915: Add tracking for CDCLK bypass frequency" and don't understand it either. On our documentation I can't find anything named "bypass clock", and there's no explanation in our code nor the commit message. The 50MHz is the CDCLK value (not "bypass clock") when the CDCLK PLL is disabled. In the driver, I see the "bypass" clock is set to the refclk we read from the hardware, and that has nothing to do with the 50MHz we're talking about at that point. We can't even read that 50MHz value from the hardware. On ICL, the refclk is either 24, 19.2 or 38.4, we can't read the 50 anywhere. I'll really need a better explanation on what exactly you meant with this "bypass" clock thing if I need to implement it on ICL. I just can't see those "5" references going away. I can't see what's wrong with this code. > > > + return 0; > > + > > + switch (cdclk) { > > + default: > > + MISSING_CASE(cdclk); > > + case 307200: > > + case 556800: > > + case 652800: > > + WARN_ON(dev_priv->cdclk.hw.ref != 19200 && > > + dev_priv->cdclk.hw.ref != 38400); > > + break; > > + case 312000: > > + case 552000: > > + case 648000: > > + WARN_ON(dev_priv->cdclk.hw.ref != 24000); > > + } > > + > > + ratio = cdclk / (dev_priv->cdclk.hw.ref / 2); > > + > > + return dev_priv->cdclk.hw.ref * ratio; > > +} > > + > > +static void icl_set_cdclk(struct drm_i915_private *dev_priv, > > + const struct intel_cdclk_state > > *cdclk_state) > > +{ > > + unsigned int cdclk =
Re: [Intel-gfx] [PATCH RFC v2 2/7] kernfs: Export kernfs_get_inode
Quoting Matt Roper (2018-02-01 19:53:10) > Drivers may wish to access a cgroup's inode to perform permission checks on > driver-specific operations. > > Cc: Tejun Heo> Cc: cgro...@vger.kernel.org > Signed-off-by: Matt Roper > --- > fs/kernfs/inode.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/fs/kernfs/inode.c b/fs/kernfs/inode.c > index a34303981deb..e1e8a0404c5b 100644 > --- a/fs/kernfs/inode.c > +++ b/fs/kernfs/inode.c > @@ -272,6 +272,7 @@ struct inode *kernfs_get_inode(struct super_block *sb, > struct kernfs_node *kn) > > return inode; > } > +EXPORT_SYMBOL(kernfs_get_inode); Will want the _GPL suffix to be in keeping with the other kernfs exports. -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [IGT PATCH RFC] tools: Introduce intel_cgroup tool
intel_cgroup is used to modify i915 cgroup parameters. At the moment only a single parameter is supported (GPU priority offset). In the future the driver may support additional parameters as well (e.g., limits on memory usage). Signed-off-by: Matt Roper--- tools/Makefile.sources | 1 + tools/intel_cgroup.c | 103 + 2 files changed, 104 insertions(+) create mode 100644 tools/intel_cgroup.c diff --git a/tools/Makefile.sources b/tools/Makefile.sources index abd23a0f..b30216c4 100644 --- a/tools/Makefile.sources +++ b/tools/Makefile.sources @@ -11,6 +11,7 @@ tools_prog_lists =\ intel_reg \ intel_backlight \ intel_bios_dumper \ + intel_cgroup\ intel_display_crc \ intel_display_poller\ intel_forcewaked\ diff --git a/tools/intel_cgroup.c b/tools/intel_cgroup.c new file mode 100644 index ..ce781b08 --- /dev/null +++ b/tools/intel_cgroup.c @@ -0,0 +1,103 @@ +/* + * Copyright © 2018 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 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 +#include +#include +#include +#include +#include + +#include "igt.h" +#include "xf86drm.h" +#include "xf86drmMode.h" + +#define I915_CGROUP_PARAM_PRIORITY_OFFSET 0x1 + +char short_opts[] = "p:"; +struct option long_opts[] = { + { "set-prio", required_argument, NULL, 'p'}, + { 0 }, +}; + +static void usage(void) +{ + puts("Usage:"); + printf(" intel_cgroup --set-prio= \n"); +} + +int main(int argc, char **argv) +{ + int drm_fd, cgrp_fd; + struct drm_i915_cgroup_param req; + int opt, ret; + struct { + bool do_prio; + int prio_val; + } updates = { 0 }; + + while ((opt = getopt_long(argc, argv, short_opts, long_opts, NULL)) != -1) { + switch (opt) { + case 'p': + updates.do_prio = true; + updates.prio_val = atoi(optarg); + break; + default: + igt_assert(false); + } + } + + if (argc - optind != 1) { + usage(); + return 1; + } + + drm_fd = drm_open_driver(DRIVER_INTEL); + if (drm_fd < 0) { + perror("Invalid DRM device"); + return 1; + } + + cgrp_fd = open(argv[optind], O_RDONLY|O_DIRECTORY, 0); + if (cgrp_fd < 0) { + perror("Invalid cgroup directory"); + return 1; + } + + req.cgroup_fd = cgrp_fd; + req.flags = 0; + + if (updates.do_prio) { + req.param = I915_CGROUP_PARAM_PRIORITY_OFFSET; + req.value = updates.prio_val; + + ret = drmIoctl(drm_fd, DRM_IOCTL_I915_CGROUP_SETPARAM, ); + if (ret) + perror("Failed to set cgroup parameter"); + } + + close(cgrp_fd); + close(drm_fd); + + return ret; +} -- 2.14.3 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH RFC v2 4/7] drm: Add helper to obtain cgroup of drm_file's owning process
Signed-off-by: Matt Roper--- include/drm/drm_file.h| 20 kernel/cgroup/cgroup_driver.c | 12 ++-- 2 files changed, 30 insertions(+), 2 deletions(-) diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h index 0e0c868451a5..72ac40530ad3 100644 --- a/include/drm/drm_file.h +++ b/include/drm/drm_file.h @@ -32,6 +32,7 @@ #include #include +#include #include @@ -378,4 +379,23 @@ void drm_event_cancel_free(struct drm_device *dev, void drm_send_event_locked(struct drm_device *dev, struct drm_pending_event *e); void drm_send_event(struct drm_device *dev, struct drm_pending_event *e); +#ifdef CONFIG_CGROUPS +/** + * drm_file_get_cgroup - obtain cgroup for drm_file's owning process + * @file_priv: DRM file + * + * Obtains the cgroup from a specific hierarchy that the drm_file's owning + * process belongs to. The cgroup may be used to set driver-specific + * policy (priority, vram usage, etc.). + */ +static inline struct cgroup * +drm_file_get_cgroup(struct drm_file *file_priv) +{ + return cgroup_for_driver_process(file_priv->pid); +} +#else +static inline struct cgroup * +drm_file_get_cgroup(struct drm_file *file_priv) { return NULL; } +#endif + #endif /* _DRM_FILE_H_ */ diff --git a/kernel/cgroup/cgroup_driver.c b/kernel/cgroup/cgroup_driver.c index 4f870cbb9212..db0e268b9546 100644 --- a/kernel/cgroup/cgroup_driver.c +++ b/kernel/cgroup/cgroup_driver.c @@ -4,6 +4,7 @@ #include #include +#include /* * General data structure returned by cgroup_driver_init() and used as a @@ -143,12 +144,19 @@ EXPORT_SYMBOL(cgroup_driver_get_data); struct cgroup * cgroup_for_driver_process(struct pid *pid) { - struct task_struct *task = pid_task(pid, PIDTYPE_PID); + struct task_struct *task; + struct cgroup *cgrp; + + read_lock(_lock); + task = pid_task(pid, PIDTYPE_PID); + read_unlock(_lock); mutex_lock(_mutex); spin_lock_irq(_set_lock); - task_cgroup_from_root(task, _dfl_root); + cgrp = task_cgroup_from_root(task, _dfl_root); spin_unlock_irq(_set_lock); mutex_unlock(_mutex); + + return cgrp; } EXPORT_SYMBOL(cgroup_for_driver_process); -- 2.14.3 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH RFC v2 7/7] drm/i915: Add context priority & priority offset to debugfs
Update i915_context_status to include priority information. Signed-off-by: Matt Roper--- drivers/gpu/drm/i915/i915_debugfs.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index 3849ded354e3..e9d8e20155b1 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -1915,6 +1915,9 @@ static int i915_context_status(struct seq_file *m, void *unused) seq_putc(m, ctx->remap_slice ? 'R' : 'r'); seq_putc(m, '\n'); + seq_printf(m, "Priority %d (prio offset = %d)\n", + ctx->priority, ctx->priority_offset); + for_each_engine(engine, dev_priv, id) { struct intel_context *ce = >engine[engine->id]; -- 2.14.3 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH RFC v2 6/7] drm/i915: Introduce 'priority offset' for GPU contexts
There are cases where a system integrator may wish to raise/lower the priority of GPU workloads being submitted by specific OS process(es), independently of how the software self-classifies its own priority. Exposing "priority offset" as an i915-specific cgroup parameter will enable such system-level configuration. Normally GPU contexts start with a priority value of 0 (I915_CONTEXT_DEFAULT_PRIORITY) and then may be adjusted up/down from there via other mechanisms. We'd like to provide a system-level input to the priority decision that will be taken into consideration, even when userspace later attempts to set an absolute priority value via I915_CONTEXT_PARAM_PRIORITY. The priority offset introduced here provides a base value that will always be added to (or subtracted from) the software's self-assigned priority value. This patch makes priority offset a cgroup-specific value; contexts will be created with a priority offset based on the cgroup membership of the process creating the context at the time the context is created. Note that priority offset is assigned at context creation time; migrating a process to a different cgroup or changing the offset associated with a cgroup will only affect new context creation and will not alter the behavior of existing contexts previously created by the process. Signed-off-by: Matt Roper--- drivers/gpu/drm/i915/i915_cgroup.c | 46 + drivers/gpu/drm/i915/i915_drv.h | 7 + drivers/gpu/drm/i915/i915_gem_context.c | 6 +++-- drivers/gpu/drm/i915/i915_gem_context.h | 9 +++ include/uapi/drm/i915_drm.h | 1 + 5 files changed, 67 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_cgroup.c b/drivers/gpu/drm/i915/i915_cgroup.c index 778af895fc00..73f3cfe31d66 100644 --- a/drivers/gpu/drm/i915/i915_cgroup.c +++ b/drivers/gpu/drm/i915/i915_cgroup.c @@ -11,6 +11,8 @@ struct i915_cgroup_data { struct cgroup_driver_data base; + + int priority_offset; }; static inline struct i915_cgroup_data * @@ -72,6 +74,8 @@ i915_cgroup_setparam_ioctl(struct drm_device *dev, void *data, struct cgroup *cgrp; struct file *f; struct inode *inode = NULL; + struct cgroup_driver_data *cgrpdata; + struct i915_cgroup_data *i915data; int ret; if (!dev_priv->i915_cgroups) { @@ -125,6 +129,17 @@ i915_cgroup_setparam_ioctl(struct drm_device *dev, void *data, return ret; switch (req->param) { + case I915_CGROUP_PARAM_PRIORITY_OFFSET: + cgrpdata = cgroup_driver_get_data(dev_priv->i915_cgroups, cgrp, + NULL); + if (IS_ERR(cgrpdata)) + return PTR_ERR(cgrpdata); + + DRM_DEBUG_DRIVER("Setting cgroup priority offset to %lld\n", +req->value); + i915data = cgrp_to_i915(cgrpdata); + i915data->priority_offset = req->value; + break; default: DRM_DEBUG_DRIVER("Invalid cgroup parameter %lld\n", req->param); return -EINVAL; @@ -132,3 +147,34 @@ i915_cgroup_setparam_ioctl(struct drm_device *dev, void *data, return 0; } + +/** + * i915_cgroup_get_prio_offset() - get prio offset for current proc's cgroup + * @dev_priv: drm device + * @file_priv: file handle for calling process + * + * RETURNS: + * Priority offset associated with the calling process' cgroup in the default + * (v2) hierarchy, otherwise 0 if no explicit priority has been assigned. + */ +int +i915_cgroup_get_prio_offset(struct drm_i915_private *dev_priv, + struct drm_i915_file_private *file_priv) +{ + struct cgroup *cgrp; + struct cgroup_driver_data *cgrpdata; + + /* Ignore internally-created contexts not associated with a process */ + if (!file_priv) + return 0; + + cgrp = drm_file_get_cgroup(file_priv->file); + if (WARN_ON(!cgrp)) + return 0; + + cgrpdata = cgroup_driver_get_data(dev_priv->i915_cgroups, cgrp, NULL); + if (IS_ERR(cgrpdata)) + return 0; + + return cgrp_to_i915(cgrpdata)->priority_offset; +} diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 60e3ff6a48bb..313191e5278a 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -2920,6 +2920,8 @@ int i915_cgroup_init(struct drm_i915_private *dev_priv); int i915_cgroup_setparam_ioctl(struct drm_device *dev, void *data, struct drm_file *file); void i915_cgroup_shutdown(struct drm_i915_private *dev_priv); +int i915_cgroup_get_prio_offset(struct drm_i915_private *dev_priv, + struct drm_i915_file_private *file_priv); #else static inline int i915_cgroup_init(struct drm_i915_private *dev_priv) @@
[Intel-gfx] [PATCH RFC v2 5/7] drm/i915: cgroup integration
Register i915 as a consumer of the cgroup_driver framework and add an ioctl to allow userspace to set i915-specific parameters associated with cgroups. Signed-off-by: Matt Roper--- drivers/gpu/drm/i915/Kconfig | 1 + drivers/gpu/drm/i915/Makefile | 1 + drivers/gpu/drm/i915/i915_cgroup.c | 134 + drivers/gpu/drm/i915/i915_drv.c| 7 ++ drivers/gpu/drm/i915/i915_drv.h| 24 +++ include/uapi/drm/i915_drm.h| 12 6 files changed, 179 insertions(+) create mode 100644 drivers/gpu/drm/i915/i915_cgroup.c diff --git a/drivers/gpu/drm/i915/Kconfig b/drivers/gpu/drm/i915/Kconfig index dfd95889f4b7..1c6b53ee76cd 100644 --- a/drivers/gpu/drm/i915/Kconfig +++ b/drivers/gpu/drm/i915/Kconfig @@ -23,6 +23,7 @@ config DRM_I915 select SYNC_FILE select IOSF_MBI select CRC32 + select CGROUP_DRIVER if CGROUPS help Choose this option if you have a system that has "Intel Graphics Media Accelerator" or "HD Graphics" integrated graphics, diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile index 3bddd8a06806..5f4a21f1a9df 100644 --- a/drivers/gpu/drm/i915/Makefile +++ b/drivers/gpu/drm/i915/Makefile @@ -47,6 +47,7 @@ i915-y := i915_drv.o \ i915-$(CONFIG_COMPAT) += i915_ioc32.o i915-$(CONFIG_DEBUG_FS) += i915_debugfs.o intel_pipe_crc.o i915-$(CONFIG_PERF_EVENTS) += i915_pmu.o +i915-$(CONFIG_CGROUPS) += i915_cgroup.o # GEM code i915-y += i915_cmd_parser.o \ diff --git a/drivers/gpu/drm/i915/i915_cgroup.c b/drivers/gpu/drm/i915/i915_cgroup.c new file mode 100644 index ..778af895fc00 --- /dev/null +++ b/drivers/gpu/drm/i915/i915_cgroup.c @@ -0,0 +1,134 @@ +/* SPDX-License-Identifier: MIT */ +/* + * i915_cgroup.c - Linux cgroups integration for i915 + * + * Copyright (C) 2018 Intel Corporation + */ + +#include + +#include "i915_drv.h" + +struct i915_cgroup_data { + struct cgroup_driver_data base; +}; + +static inline struct i915_cgroup_data * +cgrp_to_i915(struct cgroup_driver_data *data) +{ + return container_of(data, struct i915_cgroup_data, base); +} + +static struct cgroup_driver_data * +i915_cgroup_alloc(struct cgroup_driver *drv) +{ + struct i915_cgroup_data *data; + + data = kzalloc(sizeof *data, GFP_KERNEL); + return >base; +} + +static void +i915_cgroup_free(struct cgroup_driver_data *data) +{ + kfree(data); +} + +static struct cgroup_driver_funcs i915_cgroup_funcs = { + .alloc_data = i915_cgroup_alloc, + .free_data = i915_cgroup_free, +}; + +int +i915_cgroup_init(struct drm_i915_private *dev_priv) +{ + dev_priv->i915_cgroups = cgroup_driver_init(_cgroup_funcs); + if (IS_ERR(dev_priv->i915_cgroups)) + return PTR_ERR(dev_priv->i915_cgroups); + + return 0; +} + +void +i915_cgroup_shutdown(struct drm_i915_private *dev_priv) +{ + cgroup_driver_release(dev_priv->i915_cgroups); +} + +/** + * i915_cgroup_setparam_ioctl - ioctl to alter i915 settings for a cgroup + * @dev: DRM device + * @data: data pointer for the ioctl + * @file_priv: DRM file handle for the ioctl call + * + * Allows i915-specific parameters to be set for a Linux cgroup. + */ +int +i915_cgroup_setparam_ioctl(struct drm_device *dev, void *data, + struct drm_file *file) +{ + struct drm_i915_private *dev_priv = to_i915(dev); + struct drm_i915_cgroup_param *req = data; + struct cgroup *cgrp; + struct file *f; + struct inode *inode = NULL; + int ret; + + if (!dev_priv->i915_cgroups) { + DRM_DEBUG_DRIVER("No support for driver-specific cgroup data\n"); + return -EINVAL; + } + + /* We don't actually support any flags yet. */ + if (req->flags) { + DRM_DEBUG_DRIVER("Invalid flags\n"); + return -EINVAL; + } + + /* +* Make sure the file descriptor really is a cgroup fd and is on the +* v2 hierarchy. +*/ + cgrp = cgroup_get_from_fd(req->cgroup_fd); + if (IS_ERR(cgrp)) { + DRM_DEBUG_DRIVER("Invalid cgroup file descriptor\n"); + return PTR_ERR(cgrp); + } + + /* +* Access control: The strategy for using cgroups in a given +* environment is generally determined by the system integrator +* and/or OS vendor, so the specific policy about who can/can't +* manipulate them tends to be domain-specific (and may vary +* depending on the location in the cgroup hierarchy). Rather than +* trying to tie permission on this ioctl to a DRM-specific concepts +* like DRM master, we'll allow cgroup parameters to be set by any +* process that has been granted write access on the cgroup's +* virtual file system (i.e., the same permissions that would +* generally be needed to update the virtual files
[Intel-gfx] [PATCH RFC v2 3/7] cgroup: Add interface to allow drivers to lookup process cgroup membership
Drivers will need to save/restore the appropriate cgroup data for the process submitting a driver request. Add an interface for drivers to determine the cgroup for the userspace process submitting a driver request. Cc: Tejun HeoCc: cgro...@vger.kernel.org Signed-off-by: Matt Roper --- include/linux/cgroup.h| 6 ++ kernel/cgroup/cgroup_driver.c | 24 2 files changed, 30 insertions(+) diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h index 0ba1374122c7..05ebfb97bcde 100644 --- a/include/linux/cgroup.h +++ b/include/linux/cgroup.h @@ -842,6 +842,7 @@ void cgroup_driver_release(struct cgroup_driver *drv); struct cgroup_driver_data * cgroup_driver_get_data(struct cgroup_driver *drv, struct cgroup *cgrp, bool *is_new); +struct cgroup *cgroup_for_driver_process(struct pid *pid); #else /* !CONFIG_CGROUP_DRIVER */ @@ -857,6 +858,11 @@ cgroup_driver_get_data(struct cgroup_driver *drv, { return ERR_PTR(-EINVAL); } +static inline struct cgroup * +cgroup_for_driver_process(struct pid *pid) +{ + return NULL; +} #endif /* !CONFIG_CGROUP_DRIVER */ diff --git a/kernel/cgroup/cgroup_driver.c b/kernel/cgroup/cgroup_driver.c index 0d893395dc7b..4f870cbb9212 100644 --- a/kernel/cgroup/cgroup_driver.c +++ b/kernel/cgroup/cgroup_driver.c @@ -128,3 +128,27 @@ cgroup_driver_get_data(struct cgroup_driver *drv, return data; } EXPORT_SYMBOL(cgroup_driver_get_data); + +/** + * cgroup_for_driver_process - return the cgroup for a process + * @pid: process to lookup cgroup for + * + * Returns the cgroup from the v2 hierarchy that a process belongs to. + * This function is intended to be called from drivers and will obtain + * the necessary cgroup locks. + * + * RETURNS: + * Process' cgroup in the default (v2) hierarchy + */ +struct cgroup * +cgroup_for_driver_process(struct pid *pid) +{ + struct task_struct *task = pid_task(pid, PIDTYPE_PID); + + mutex_lock(_mutex); + spin_lock_irq(_set_lock); + task_cgroup_from_root(task, _dfl_root); + spin_unlock_irq(_set_lock); + mutex_unlock(_mutex); +} +EXPORT_SYMBOL(cgroup_for_driver_process); -- 2.14.3 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH RFC v2 2/7] kernfs: Export kernfs_get_inode
Drivers may wish to access a cgroup's inode to perform permission checks on driver-specific operations. Cc: Tejun HeoCc: cgro...@vger.kernel.org Signed-off-by: Matt Roper --- fs/kernfs/inode.c | 1 + 1 file changed, 1 insertion(+) diff --git a/fs/kernfs/inode.c b/fs/kernfs/inode.c index a34303981deb..e1e8a0404c5b 100644 --- a/fs/kernfs/inode.c +++ b/fs/kernfs/inode.c @@ -272,6 +272,7 @@ struct inode *kernfs_get_inode(struct super_block *sb, struct kernfs_node *kn) return inode; } +EXPORT_SYMBOL(kernfs_get_inode); /* * The kernfs_node serves as both an inode and a directory entry for -- 2.14.3 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH RFC v2 1/7] cgroup: Allow drivers to store data associated with a cgroup
There are cases where drivers need to adjust behavior or control device-specific resources according to the type of clients (processes) submitting requests. Linux cgroups are a natural fit for this type of resource control and policy management, so we need a way for drivers to associate their own driver-specific and device-specific data with individual cgroups. This is different than the cgroup controller support that exists today in several important ways: * Drivers may be built as modules (and unloaded/reloaded) which is not something cgroup controllers support today. * Drivers may wish to provide their own interface to allow userspace to adjust driver-specific settings (e.g., via a driver ioctl rather than via the kernfs filesystem). * A single driver may be managing multiple devices and wish to maintain different driver-specific cgroup data for each. To use this mechanism, drivers should call cgroup_driver_init() to register themselves and provide provide handler functions for allocating driver-specific data structures; this call will return a handle that can be used to lookup the driver-specific data associated with the device. Drivers managing multiple devices that wish to track separate data for each device may register themselves multiple times (e.g., a graphics driver might call cgroup_driver_init() for each drm_device it manages). At runtime, drivers may call cgroup_driver_get_data() to fetch the driver-specific data associated with a cgroup. The driver-specific data (which should be a driver-defined subclass of 'struct cgroup_driver_data') will be allocated if one doesn't already exist. Management of driver-specific data by the cgroups framework is protected by cgroup_mutex, but drivers are responsible for performing their own synchronization on the per-cgroup data they receive, if necessary. Note that driver-specific data for a cgroup will only be allocated if/when the driver first requests data for that cgroup. The driver data will also be automatically destroyed if the cgroup it belongs to is removed. Finally, drivers should cgroup_driver_release() on driver unload to destroy all of their driver-specific data. Note that technically these interfaces aren't restricted to drivers (other non-driver parts of the kernel could make use of them as well). I expect drivers to be the primary consumers of this interface and couldn't think of a more appropriate generic name (the term "subsystem" would probably be more accurate, but that's already used by cgroup controllers). Cc: Tejun HeoCc: cgro...@vger.kernel.org Signed-off-by: Matt Roper --- include/linux/cgroup-defs.h | 37 include/linux/cgroup.h| 27 + init/Kconfig | 4 ++ kernel/cgroup/Makefile| 1 + kernel/cgroup/cgroup.c| 1 + kernel/cgroup/cgroup_driver.c | 130 ++ 6 files changed, 200 insertions(+) create mode 100644 kernel/cgroup/cgroup_driver.c diff --git a/include/linux/cgroup-defs.h b/include/linux/cgroup-defs.h index 8b7fd8eeccee..5728e3afc95f 100644 --- a/include/linux/cgroup-defs.h +++ b/include/linux/cgroup-defs.h @@ -8,6 +8,7 @@ #ifndef _LINUX_CGROUP_DEFS_H #define _LINUX_CGROUP_DEFS_H +#include #include #include #include @@ -27,6 +28,7 @@ struct cgroup; struct cgroup_root; struct cgroup_subsys; struct cgroup_taskset; +struct cgroup_driver; struct kernfs_node; struct kernfs_ops; struct kernfs_open_file; @@ -307,6 +309,22 @@ struct cgroup_stat { struct prev_cputime prev_cputime; }; +/* + * Driver-specific cgroup data. Drivers should subclass this structure with + * their own fields for data that should be stored alongside individual + * cgroups. + */ +struct cgroup_driver_data { + /* Driver this data structure is associated with */ + struct cgroup_driver *drv; + + /* Node in cgroup's data hashtable */ + struct hlist_node cgroupnode; + + /* Node in driver's data list; used to cleanup on driver unload */ + struct list_head drivernode; +}; + struct cgroup { /* self css with NULL ->ss, points back to this cgroup */ struct cgroup_subsys_state self; @@ -427,6 +445,12 @@ struct cgroup { /* used to store eBPF programs */ struct cgroup_bpf bpf; + /* +* list of cgroup_driver_data structures; used to manage +* driver-specific data associated with individual cgroups +*/ + DECLARE_HASHTABLE(driver_data, 4); + /* ids of the ancestors at each level including self */ int ancestor_ids[]; }; @@ -662,6 +686,19 @@ struct cgroup_subsys { unsigned int depends_on; }; +/* Function table for handling driver-specific cgroup data */ +struct cgroup_driver_funcs { + /* Allocates driver-specific data for a cgroup */ + struct cgroup_driver_data *(*alloc_data)(struct cgroup_driver *drv); + + /* +*
[Intel-gfx] [PATCH RFC v2 0/7] DRM management via cgroups
This is a second iteration of the patches originally posted here: https://lists.freedesktop.org/archives/intel-gfx/2018-January/153156.html Please see the original cover letter to understand the general goal and justification for this work. This series makes the following important changes to the design from the first version: * The "cgroup helper library" has been removed from the DRM core; the early patches of this series introduce an enhancement to the cgroup subsystem itself which will grant drivers the ability to save/restore their own driver-specific data structures associated with individual cgroups. Drivers may subclass the cgroup_driver_data structure to store any kind of per-cgroup data they want, not just integer key/value pairs as the helper library from version 1 of this series did. Moving this functionality directly into cgroups will also allow other parts of the kernel to potentially benefit from this functionality, not just DRM drivers. * The general graphics-specific ioctl to set cgroup parameters has moved from the DRM core to i915. This was pretty much an arbitrary choice I made while re-writing the new version of the series; I don't have a strong opinion over whether the ioctl goes in the DRM core or stays i915-specific. I'd appreciate feedback from other driver teams as to whether they anticipate cgroup integration being useful for their drivers and use cases (e.g., for controlling things like GPU memory usage, priority settings, etc.). I'll move the ioctl back to the DRM core in the next iteration if there's interest from other driver teams. * The i915-specific usage of this functionality is to adjust GPU priority for groups of processes. Based on Chris' feedback, I've made this control a "priority offset" rather than just a starting priority, so that the change applied by cgroup will be added to the explicitly-set context priority rather than being overwritten by it. At the moment i915 still just assigns the priority offset one time at context creation and doesn't alter it if the process migrates between cgroups at runtime or if the cgroup has a new value assigned to it; such changes only affect the priority offset of new contexts created by a process. I'm open to feedback on whether we should also make cgroup changes affect existing GPU context(s) from a process. As noted on v1 of my patch series, cgroup control of graphics driver concepts is expected to be done mostly during system startup (e.g., a sysv-init script or a systemd recipe written by a system integrator and custom to their own setup), so we don't really need a terribly complicated userspace, just a simple tool that can be called by appropriate scripts/recipes to set the desired policy. I've re-written the tool from v1 of the patch series as an i-g-t tool this time and will send it as followup email shortly. Still on my TODO list (once I gather some more general feedback and move out of the early RFC stage): * Document this more fully in both the cgroups documentation and the i915 kerneldoc. * Write some i-g-t tests to exercise the ioctl and all the corner cases. Matt Roper (7): cgroup: Allow drivers to store data associated with a cgroup kernfs: Export kernfs_get_inode cgroup: Add interface to allow drivers to lookup process cgroup membership drm: Add helper to obtain cgroup of drm_file's owning process drm/i915: cgroup integration drm/i915: Introduce 'priority offset' for GPU contexts drm/i915: Add context priority & priority offset to debugfs drivers/gpu/drm/i915/Kconfig| 1 + drivers/gpu/drm/i915/Makefile | 1 + drivers/gpu/drm/i915/i915_cgroup.c | 180 drivers/gpu/drm/i915/i915_debugfs.c | 3 + drivers/gpu/drm/i915/i915_drv.c | 7 ++ drivers/gpu/drm/i915/i915_drv.h | 31 ++ drivers/gpu/drm/i915/i915_gem_context.c | 6 +- drivers/gpu/drm/i915/i915_gem_context.h | 9 ++ fs/kernfs/inode.c | 1 + include/drm/drm_file.h | 20 include/linux/cgroup-defs.h | 37 +++ include/linux/cgroup.h | 33 ++ include/uapi/drm/i915_drm.h | 13 +++ init/Kconfig| 4 + kernel/cgroup/Makefile | 1 + kernel/cgroup/cgroup.c | 1 + kernel/cgroup/cgroup_driver.c | 162 17 files changed, 508 insertions(+), 2 deletions(-) create mode 100644 drivers/gpu/drm/i915/i915_cgroup.c create mode 100644 kernel/cgroup/cgroup_driver.c -- 2.14.3 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v7 2/6] drm/i915/guc: Rename guc_ggtt_offset to intel_guc_ggtt_offset
On 01/31/2018 11:38 PM, Chris Wilson wrote: Quoting Jackie Li (2018-01-19 01:29:28) GuC related exported functions should start with "intel_guc_" prefix and pass intel_guc as the first parameter since its guc related. Current guc_ggtt_offset() failed to follow this code convention. But it was not, and still does not operate on the guc. Is that changing? this problem is that it's guc related and the following patches do need to access the data from intel_guc. Do you think it's getting better if I add a sentence like "the future patches will need to access the intel_guc to verify the offset"? Regards, -Jackie ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v7 1/6] drm/i915/guc: Move GuC WOPCM related code into separate files
On 01/31/2018 11:37 PM, Chris Wilson wrote: Quoting Jackie Li (2018-01-19 01:29:27) intel_guc_reg.h should only include definition for GuC registers and related register bits. GuC WOPCM related values should not be defined in intel_guc_reg.h GuC registers does not include GuC WOPCM? The code does seem to suggest they are related ;) I should say "Non-register related GuC WOPCM macros should not be defined in intel_guc_reg.h"? This patch creates a better file structure by moving GuC WOPCM related definitions int to a new header intel_guc_wopcm.h and moving GuC WOPCM related functions to a new source file intel_guc_wopcm.c Just needs one more sentence to sell this, perhaps "as future patches will increase the complexity of determining the WOPCM layout". One function per file is just crying out for LTO ;) Thanks Chris! I will add it. This is why I need your help and learn how to sell this:-) Really appreciate the comments! Regards, -Jackie ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v7 2/6] drm/i915/guc: Rename guc_ggtt_offset to intel_guc_ggtt_offset
diff --git a/drivers/gpu/drm/i915/intel_guc_ads.c b/drivers/gpu/drm/i915/intel_guc_ads.c index ac62753..7215594 100644 --- a/drivers/gpu/drm/i915/intel_guc_ads.c +++ b/drivers/gpu/drm/i915/intel_guc_ads.c @@ -113,17 +113,6 @@ int intel_guc_ads_create(struct intel_guc *guc) blob->reg_state.white_list[engine->guc_id].count = 0; } - /* - * The GuC requires a "Golden Context" when it reinitialises - * engines after a reset. Here we use the Render ring default - * context, which must already exist and be pinned in the GGTT, - * so its address won't change after we've told the GuC where - * to find it. Note that we have to skip our header (1 page), - * because our GuC shared data is there. - */ - blob->ads.golden_context_lrca = - guc_ggtt_offset(dev_priv->kernel_context->engine[RCS].state) + - skipped_offset; This move seems unnecessary This move is mainly for reusing "vma" variable so that the line won't get too long. Besides, this move can also make sure the assignment will get closer to the code that actually uses the value from "blob->ads.golden_context_lrca":-) /* * The GuC expects us to exclude the portion of the context image that @@ -135,11 +124,23 @@ int intel_guc_ads_create(struct intel_guc *guc) blob->ads.eng_state_size[engine->guc_id] = engine->context_size - skipped_size; - base = guc_ggtt_offset(vma); + base = intel_guc_ggtt_offset(guc, vma); blob->ads.scheduler_policies = base + ptr_offset(blob, policies); blob->ads.reg_state_buffer = base + ptr_offset(blob, reg_state_buffer); blob->ads.reg_state_addr = base + ptr_offset(blob, reg_state); + /* + * The GuC requires a "Golden Context" when it reinitialises + * engines after a reset. Here we use the Render ring default + * context, which must already exist and be pinned in the GGTT, + * so its address won't change after we've told the GuC where + * to find it. Note that we have to skip our header (1 page), + * because our GuC shared data is there. + */ + vma = dev_priv->kernel_context->engine[RCS].state; + blob->ads.golden_context_lrca = intel_guc_ggtt_offset(guc, vma) + + skipped_offset; + kunmap(page); return 0; ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] ✗ Fi.CI.BAT: failure for series starting with [v2] drm/i915/guc: Allow preempt-client to be NULL (rev3)
== Series Details == Series: series starting with [v2] drm/i915/guc: Allow preempt-client to be NULL (rev3) URL : https://patchwork.freedesktop.org/series/37395/ State : failure == Summary == Series 37395v3 series starting with [v2] drm/i915/guc: Allow preempt-client to be NULL https://patchwork.freedesktop.org/api/1.0/series/37395/revisions/3/mbox/ Test gem_mmap_gtt: Subgroup basic-small-bo-tiledx: pass -> FAIL (fi-gdg-551) fdo#102575 Test gem_sync: Subgroup basic-many-each: pass -> DMESG-FAIL (fi-bsw-n3050) Subgroup basic-store-all: pass -> SKIP (fi-bsw-n3050) Subgroup basic-store-each: pass -> SKIP (fi-bsw-n3050) Test gem_tiled_blits: Subgroup basic: pass -> SKIP (fi-bsw-n3050) Test gem_tiled_fence_blits: Subgroup basic: pass -> SKIP (fi-bsw-n3050) Test gem_wait: Subgroup basic-busy-all: pass -> SKIP (fi-bsw-n3050) Subgroup basic-wait-all: pass -> SKIP (fi-bsw-n3050) Subgroup basic-await-all: pass -> SKIP (fi-bsw-n3050) Test gem_workarounds: Subgroup basic-read: pass -> SKIP (fi-bsw-n3050) Test kms_busy: Subgroup basic-flip-c: pass -> SKIP (fi-bsw-n3050) Test kms_cursor_legacy: Subgroup basic-busy-flip-before-cursor-atomic: pass -> SKIP (fi-bsw-n3050) Subgroup basic-busy-flip-before-cursor-legacy: pass -> SKIP (fi-bsw-n3050) Test kms_frontbuffer_tracking: Subgroup basic: pass -> SKIP (fi-bsw-n3050) fdo#104571 Test kms_pipe_crc_basic: Subgroup hang-read-crc-pipe-c: pass -> DMESG-FAIL (fi-bsw-n3050) Subgroup suspend-read-crc-pipe-c: pass -> DMESG-WARN (fi-bsw-n3050) fdo#102575 https://bugs.freedesktop.org/show_bug.cgi?id=102575 fdo#104571 https://bugs.freedesktop.org/show_bug.cgi?id=104571 fi-bdw-5557u total:288 pass:267 dwarn:0 dfail:0 fail:0 skip:21 time:417s fi-bdw-gvtdvmtotal:288 pass:264 dwarn:0 dfail:0 fail:0 skip:24 time:425s fi-blb-e6850 total:288 pass:223 dwarn:1 dfail:0 fail:0 skip:64 time:374s fi-bsw-n3050 total:288 pass:227 dwarn:1 dfail:2 fail:0 skip:58 time:479s fi-bwr-2160 total:288 pass:183 dwarn:0 dfail:0 fail:0 skip:105 time:282s fi-bxt-dsi total:288 pass:258 dwarn:0 dfail:0 fail:0 skip:30 time:480s fi-bxt-j4205 total:288 pass:259 dwarn:0 dfail:0 fail:0 skip:29 time:490s fi-byt-j1900 total:288 pass:253 dwarn:0 dfail:0 fail:0 skip:35 time:474s fi-byt-n2820 total:288 pass:249 dwarn:0 dfail:0 fail:0 skip:39 time:460s fi-cfl-s2total:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:566s fi-elk-e7500 total:288 pass:229 dwarn:0 dfail:0 fail:0 skip:59 time:413s fi-gdg-551 total:288 pass:179 dwarn:0 dfail:0 fail:1 skip:108 time:277s fi-glk-1 total:288 pass:260 dwarn:0 dfail:0 fail:0 skip:28 time:510s fi-hsw-4770 total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:390s fi-hsw-4770r total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:404s fi-ilk-650 total:288 pass:228 dwarn:0 dfail:0 fail:0 skip:60 time:412s fi-ivb-3520m total:288 pass:259 dwarn:0 dfail:0 fail:0 skip:29 time:452s fi-ivb-3770 total:288 pass:255 dwarn:0 dfail:0 fail:0 skip:33 time:413s fi-kbl-7500u total:288 pass:263 dwarn:1 dfail:0 fail:0 skip:24 time:457s fi-kbl-7560u total:288 pass:269 dwarn:0 dfail:0 fail:0 skip:19 time:498s fi-kbl-7567u total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:453s fi-kbl-r total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:498s fi-pnv-d510 total:288 pass:222 dwarn:1 dfail:0 fail:0 skip:65 time:576s fi-skl-6260u total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:431s fi-skl-6600u total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:510s fi-skl-6700hqtotal:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:531s fi-skl-6700k2total:288 pass:264 dwarn:0 dfail:0 fail:0 skip:24 time:492s fi-skl-6770hqtotal:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:496s fi-skl-guc total:288 pass:260 dwarn:0 dfail:0 fail:0 skip:28 time:415s fi-skl-gvtdvmtotal:288 pass:265 dwarn:0 dfail:0 fail:0 skip:23 time:432s fi-snb-2520m total:288 pass:248 dwarn:0 dfail:0 fail:0 skip:40 time:526s Blacklisted hosts: fi-glk-dsi total:288 pass:258 dwarn:0 dfail:0 fail:0 skip:30 time:467s
Re: [Intel-gfx] ✗ Fi.CI.IGT: failure for series starting with [v2,1/2] drm/i915/bxt, glk: Increase PCODE timeouts during CDCLK freq changing
On Tue, Jan 30, 2018 at 10:11:49PM +, Patchwork wrote: > == Series Details == > > Series: series starting with [v2,1/2] drm/i915/bxt, glk: Increase PCODE > timeouts during CDCLK freq changing > URL : https://patchwork.freedesktop.org/series/37344/ > State : failure > > == Summary == > > Test kms_rotation_crc: > Subgroup sprite-rotation-180: > pass -> FAIL (shard-apl) This is a CRC error, but without any pcode timeout, so not related. The rest of failures below are on unrelated platforms. Thanks for the reviews, pushed both patches to -dinq. > Test kms_vblank: > Subgroup pipe-b-ts-continuation-dpms-suspend: > pass -> INCOMPLETE (shard-hsw) > Test perf: > Subgroup oa-exponents: > fail -> PASS (shard-apl) fdo#102254 > Subgroup polling: > fail -> PASS (shard-hsw) fdo#102252 > Test kms_flip: > Subgroup 2x-plain-flip-fb-recreate: > fail -> PASS (shard-hsw) fdo#100368 +1 > Subgroup 2x-flip-vs-expired-vblank-interruptible: > pass -> FAIL (shard-hsw) fdo#102887 > Test kms_plane: > Subgroup plane-panning-bottom-right-suspend-pipe-b-planes: > dmesg-warn -> PASS (shard-snb) > Test kms_cursor_legacy: > Subgroup cursorb-vs-flipa-atomic: > pass -> SKIP (shard-hsw) > > fdo#102254 https://bugs.freedesktop.org/show_bug.cgi?id=102254 > fdo#102252 https://bugs.freedesktop.org/show_bug.cgi?id=102252 > fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368 > fdo#102887 https://bugs.freedesktop.org/show_bug.cgi?id=102887 > > shard-apltotal:2838 pass:1750 dwarn:1 dfail:0 fail:23 skip:1064 > time:12657s > shard-hswtotal:2826 pass:1726 dwarn:1 dfail:0 fail:11 skip:1086 > time:11649s > shard-snbtotal:2838 pass:1330 dwarn:1 dfail:0 fail:10 skip:1497 > time:6619s > > == Logs == > > For more details see: > https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7823/shards.html ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v4 1/7] drm/i915: Add CRTC output format YCBCR 4:2:0
On Tue, Jan 30, 2018 at 03:05:57PM +0530, Shashank Sharma wrote: > From: "Sharma, Shashank"> > Currently, we are using a bool in CRTC state (state->ycbcr420), > to indicate modeset, that the output format is YCBCR 4:2:0. Now in > order to support other YCBCR formats, we will need more such flags. > > The idea behind this patch is to replace this bool with an enum, > and plug this in in the existing YCBCR 4:2:0 framework in such a > way that this can be re-used for YCBCR 4:4:4 and other output formats too. > > This patch adds a new enum for CRTC output formats, and then plugs it > in the existing modeset framework. > > V3: Added this patch in the series, to address review comments from > second patchset. > V4: Added r-b from Maarten (on v3) > Addressed review comments from Ville: > - Change the enum name to intel_output_format from crtc_output_format. > - Start the enum value (INVALID) from 0 instaed of 1. > - Set the crtc's output_format to RGB in encoder's compute_config. > > Cc: Ville Syrjala > Cc: Maarten Lankhorst > Signed-off-by: Shashank Sharma > Reviewed-by: Maarten Lankhorst > --- > drivers/gpu/drm/i915/intel_color.c | 2 +- > drivers/gpu/drm/i915/intel_ddi.c | 4 ++- > drivers/gpu/drm/i915/intel_display.c | 66 > > drivers/gpu/drm/i915/intel_drv.h | 10 -- > drivers/gpu/drm/i915/intel_hdmi.c| 6 ++-- > drivers/gpu/drm/i915/intel_panel.c | 2 +- > 6 files changed, 61 insertions(+), 29 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_color.c > b/drivers/gpu/drm/i915/intel_color.c > index aa66e95..99e32cb 100644 > --- a/drivers/gpu/drm/i915/intel_color.c > +++ b/drivers/gpu/drm/i915/intel_color.c > @@ -141,7 +141,7 @@ static void i9xx_load_csc_matrix(struct drm_crtc_state > *crtc_state) > uint16_t coeffs[9] = { 0, }; > struct intel_crtc_state *intel_crtc_state = > to_intel_crtc_state(crtc_state); > > - if (intel_crtc_state->ycbcr420) { > + if (intel_crtc_state->output_format == CRTC_OUTPUT_YCBCR420) { > i9xx_load_ycbcr_conversion_matrix(intel_crtc); > return; > } else if (crtc_state->ctm) { > diff --git a/drivers/gpu/drm/i915/intel_ddi.c > b/drivers/gpu/drm/i915/intel_ddi.c > index e51559b..d565e28 100644 > --- a/drivers/gpu/drm/i915/intel_ddi.c > +++ b/drivers/gpu/drm/i915/intel_ddi.c > @@ -1264,7 +1264,7 @@ static void ddi_dotclock_get(struct intel_crtc_state > *pipe_config) > else > dotclock = pipe_config->port_clock; > > - if (pipe_config->ycbcr420) > + if (pipe_config->output_format == CRTC_OUTPUT_YCBCR420) > dotclock *= 2; > > if (pipe_config->pixel_multiplier) > @@ -2759,6 +2759,8 @@ static bool intel_ddi_compute_config(struct > intel_encoder *encoder, > if (port == PORT_A) > pipe_config->cpu_transcoder = TRANSCODER_EDP; > > + pipe_config->output_format = CRTC_OUTPUT_RGB; We seem to be missing this for most platforms/encoder types. > + > if (intel_crtc_has_type(pipe_config, INTEL_OUTPUT_HDMI)) > ret = intel_hdmi_compute_config(encoder, pipe_config, > conn_state); > else > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c > index 83de43c..877336d 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -4650,7 +4650,8 @@ skl_update_scaler(struct intel_crtc_state *crtc_state, > bool force_detach, >*/ > need_scaling = src_w != dst_w || src_h != dst_h; > > - if (crtc_state->ycbcr420 && scaler_user == SKL_CRTC_INDEX) > + if (crtc_state->output_format == CRTC_OUTPUT_YCBCR420 && > + scaler_user == SKL_CRTC_INDEX) > need_scaling = true; > > /* > @@ -6362,7 +6363,8 @@ static int intel_crtc_compute_config(struct intel_crtc > *crtc, > return -EINVAL; > } > > - if (pipe_config->ycbcr420 && pipe_config->base.ctm) { > + if (pipe_config->output_format == CRTC_OUTPUT_YCBCR420 && > + pipe_config->base.ctm) { > /* >* There is only one pipe CSC unit per pipe, and we need that >* for output conversion from RGB->YCBCR. So if CTM is already > @@ -8192,10 +8194,10 @@ static void haswell_set_pipemisc(struct drm_crtc > *crtc) > if (intel_crtc->config->dither) > val |= PIPEMISC_DITHER_ENABLE | PIPEMISC_DITHER_TYPE_SP; > > - if (config->ycbcr420) { > - val |= PIPEMISC_OUTPUT_COLORSPACE_YUV | > - PIPEMISC_YUV420_ENABLE | > - PIPEMISC_YUV420_MODE_FULL_BLEND; > + if (config->output_format == CRTC_OUTPUT_YCBCR420) { > +
Re: [Intel-gfx] [PATCH v4 7/7] drm/i915: Add YCBCR 4:2:0/4:4:4 support for LSPCON
On Tue, Jan 30, 2018 at 03:06:03PM +0530, Shashank Sharma wrote: > From: "Sharma, Shashank"> > LSPCON chips can generate YCBCR outputs, if asked nicely :). > > In order to generate YCBCR 4:2:0 outputs, a source must: > - send YCBCR 4:4:4 signals to LSPCON > - program color space as 4:2:0 in AVI infoframes > > Whereas for YCBCR 4:4:4 outputs, the source must: > - send YCBCR 4:4:4 signals to LSPCON > - program color space as 4:4:4 in AVI infoframes > > So for both 4:2:0 as well as 4:4:4 outputs, we are driving the > pipe for YCBCR 4:4:4 output, but AVI infoframe's color space > information indicates LSPCON FW to start scaling down from YCBCR > 4:4:4 and generate YCBCR 4:2:0 output. As the scaling is done by > LSPCON device, we need not to reserve a scaler for 4:2:0 outputs. > > V2: rebase > V3: Addressed review comments from Ville > - add enum crtc_output_format instead of bool ycbcr420 > - use crtc_output_format=4:4:4 for modeset of LSPCON 4:2:0 output > cases in this way we will have YCBCR 4:4:4 framework ready (except > the ABI part) > V4: Added r-b from Maarten (for v3) > Addressed review comments from Ville: > - Do not add a non-atomic state variable to determine lspcon output. > Instead add bool in CRTC state to indicate lspcon based scaling. > > Cc: Ville Syrjala > Cc: Maarten Lankhorst > Reviewed-by: Maarten Lankhorst > Signed-off-by: Shashank Sharma > --- > drivers/gpu/drm/i915/i915_reg.h | 2 ++ > drivers/gpu/drm/i915/intel_ddi.c | 7 +++ > drivers/gpu/drm/i915/intel_display.c | 11 ++- > drivers/gpu/drm/i915/intel_dp.c | 10 ++ > drivers/gpu/drm/i915/intel_drv.h | 5 + > drivers/gpu/drm/i915/intel_lspcon.c | 30 ++ > 6 files changed, 64 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index 64933fd..e383b05 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -8721,6 +8721,8 @@ enum skl_power_gate { > #define TRANS_MSA_MISC(tran) _MMIO_TRANS2(tran, _TRANSA_MSA_MISC) > > #define TRANS_MSA_SYNC_CLK (1<<0) > +#define TRANS_MSA_SAMPLING_444(2<<1) > +#define TRANS_MSA_CLRSP_YCBCR (2<<3) > #define TRANS_MSA_6_BPC (0<<5) > #define TRANS_MSA_8_BPC (1<<5) > #define TRANS_MSA_10_BPC(2<<5) > diff --git a/drivers/gpu/drm/i915/intel_ddi.c > b/drivers/gpu/drm/i915/intel_ddi.c > index 510bb77..db7d940 100644 > --- a/drivers/gpu/drm/i915/intel_ddi.c > +++ b/drivers/gpu/drm/i915/intel_ddi.c > @@ -1499,6 +1499,13 @@ void intel_ddi_set_pipe_settings(const struct > intel_crtc_state *crtc_state) > break; > } > > + /* > + * As per DP 1.2 spec section 2.3.4.3 while sending > + * YCBCR 444 signals we should program MSA MISC1/0 fields with > + * colorspace information. The output colorspace encoding is BT601. > + */ > + if (crtc_state->output_format == CRTC_OUTPUT_YCBCR444) > + temp |= TRANS_MSA_SAMPLING_444 | TRANS_MSA_CLRSP_YCBCR; > I915_WRITE(TRANS_MSA_MISC(cpu_transcoder), temp); > } > > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c > index 4a40de9..b26f4a9 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -9228,6 +9228,7 @@ static bool haswell_get_pipe_config(struct intel_crtc > *crtc, > pipe_config->gamma_mode = > I915_READ(GAMMA_MODE(crtc->pipe)) & GAMMA_MODE_MODE_MASK; > > + pipe_config->external_scaling = false; I really don't like the name of that. Makes me think we're actually scaling the entire output or something. 'external_ycbcr_420_downsample' or something like that would be better. > if (IS_BROADWELL(dev_priv) || INTEL_GEN(dev_priv) >= 9) { > u32 tmp = I915_READ(PIPEMISC(crtc->pipe)); > enum intel_output_format output_format = CRTC_OUTPUT_RGB; > @@ -9247,8 +9248,16 @@ static bool haswell_get_pipe_config(struct intel_crtc > *crtc, > output_format = CRTC_OUTPUT_YCBCR420; > } else { > output_format = CRTC_OUTPUT_YCBCR444; > - } > > + /* > + * As there is no interface defined (yet) > + * to get the user's preference for output > + * format, YCBCR444 output format is only > + * possible with LSPCON YCBCR420 output, > + * which uses LSPCON's (external )scaler. > + */ > +
[Intel-gfx] [PATCH v2] drm/i915: Move the scheduler feature bits into the purview of the engines
Rather than having the high level ioctl interface guess the underlying implementation details, having the implementation declare what capabilities it exports. We define an intel_driver_caps, similar to the intel_device_info, which instead of trying to describe the HW gives details on what the driver itself supports. This is then populated by the engine backend for the new scheduler capability field for use elsewhere. v2: Use caps.scheduler for validating CONTEXT_PARAM_SET_PRIORITY (Mika) One less assumption of engine[RCS] \o/ Signed-off-by: Chris WilsonCc: Tomasz Lis Cc: Joonas Lahtinen Cc: Tvrtko Ursulin Cc: Michal Wajdeczko Cc: Mika Kuoppala --- drivers/gpu/drm/i915/i915_debugfs.c | 1 + drivers/gpu/drm/i915/i915_drv.c | 8 +--- drivers/gpu/drm/i915/i915_drv.h | 2 ++ drivers/gpu/drm/i915/i915_gem.c | 3 +++ drivers/gpu/drm/i915/i915_gem_context.c | 2 +- drivers/gpu/drm/i915/i915_gpu_error.c| 7 +-- drivers/gpu/drm/i915/intel_device_info.c | 6 ++ drivers/gpu/drm/i915/intel_device_info.h | 7 +++ drivers/gpu/drm/i915/intel_lrc.c | 6 ++ 9 files changed, 32 insertions(+), 10 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index 9e44adef30f0..2bdce9fea671 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -49,6 +49,7 @@ static int i915_capabilities(struct seq_file *m, void *data) intel_device_info_dump_flags(info, ); intel_device_info_dump_runtime(info, ); + intel_driver_caps_print(_priv->caps, ); kernel_param_lock(THIS_MODULE); i915_params_dump(_modparams, ); diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 1ec12add34b2..733f71637914 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -372,13 +372,7 @@ static int i915_getparam(struct drm_device *dev, void *data, value = i915_gem_mmap_gtt_version(); break; case I915_PARAM_HAS_SCHEDULER: - value = 0; - if (dev_priv->engine[RCS] && dev_priv->engine[RCS]->schedule) { - value |= I915_SCHEDULER_CAP_ENABLED; - value |= I915_SCHEDULER_CAP_PRIORITY; - if (HAS_LOGICAL_RING_PREEMPTION(dev_priv)) - value |= I915_SCHEDULER_CAP_PREEMPTION; - } + value = dev_priv->caps.scheduler; break; case I915_PARAM_MMAP_VERSION: diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index c676269ed843..24333042e1e9 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -468,6 +468,7 @@ struct i915_gpu_state { u32 reset_count; u32 suspend_count; struct intel_device_info device_info; + struct intel_driver_caps driver_caps; struct i915_params params; struct i915_error_uc { @@ -1809,6 +1810,7 @@ struct drm_i915_private { struct kmem_cache *priorities; const struct intel_device_info info; + struct intel_driver_caps caps; /** * Data Stolen Memory - aka "i915 stolen memory" gives us the start and diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index c049496e8757..8d732f97e23e 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -3222,8 +3222,11 @@ void i915_gem_set_wedged(struct drm_i915_private *i915) * start to complete all requests. */ engine->submit_request = nop_complete_submit_request; + engine->schedule = NULL; } + i915->caps.scheduler = 0; + /* * Make sure no request can slip through without getting completed by * either this call here to intel_engine_init_global_seqno, or the one diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c index 648e7536ff51..8337d15bb0e5 100644 --- a/drivers/gpu/drm/i915/i915_gem_context.c +++ b/drivers/gpu/drm/i915/i915_gem_context.c @@ -807,7 +807,7 @@ int i915_gem_context_setparam_ioctl(struct drm_device *dev, void *data, if (args->size) ret = -EINVAL; - else if (!to_i915(dev)->engine[RCS]->schedule) + else if (!(to_i915(dev)->caps.scheduler & I915_SCHEDULER_CAP_PRIORITY)) ret = -ENODEV; else if (priority > I915_CONTEXT_MAX_USER_PRIORITY || priority < I915_CONTEXT_MIN_USER_PRIORITY) diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c
Re: [Intel-gfx] [PATCH v2 2/3] drm/i915: Move the scheduler feature bits into the purview of the engines
Quoting Mika Kuoppala (2018-01-31 13:58:17) > Chris Wilsonwrites: > > > Rather than having the high level ioctl interface guess the underlying > > implementation details, having the implementation declare what > > capabilities it exports. We define an intel_driver_caps, similar to the > > intel_device_info, which instead of trying to describe the HW gives > > details on what the driver itself supports. This is then populated by > > the engine backend for the new scheduler capability field for use > > elsewhere. > > > > Signed-off-by: Chris Wilson > > Cc: Tomasz Lis > > Cc: Joonas Lahtinen > > Cc: Tvrtko Ursulin > > Cc: Michal Wajdeczko > > --- > > drivers/gpu/drm/i915/i915_drv.c | 8 +--- > > drivers/gpu/drm/i915/i915_drv.h | 2 ++ > > drivers/gpu/drm/i915/i915_gem.c | 3 +++ > > drivers/gpu/drm/i915/i915_gpu_error.c| 7 +-- > > drivers/gpu/drm/i915/intel_device_info.c | 6 ++ > > drivers/gpu/drm/i915/intel_device_info.h | 7 +++ > > drivers/gpu/drm/i915/intel_lrc.c | 6 ++ > > 7 files changed, 30 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c > > b/drivers/gpu/drm/i915/i915_drv.c > > index 1ec12add34b2..733f71637914 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.c > > +++ b/drivers/gpu/drm/i915/i915_drv.c > > @@ -372,13 +372,7 @@ static int i915_getparam(struct drm_device *dev, void > > *data, > > value = i915_gem_mmap_gtt_version(); > > break; > > case I915_PARAM_HAS_SCHEDULER: > > - value = 0; > > - if (dev_priv->engine[RCS] && dev_priv->engine[RCS]->schedule) > > { > > - value |= I915_SCHEDULER_CAP_ENABLED; > > - value |= I915_SCHEDULER_CAP_PRIORITY; > > - if (HAS_LOGICAL_RING_PREEMPTION(dev_priv)) > > - value |= I915_SCHEDULER_CAP_PREEMPTION; > > - } > > + value = dev_priv->caps.scheduler; > > Use the shiny CAP_PRIORITY instead of rcs->schedule on the > I915_CONTEXT_PARAM_PRIORITY validation? Yeah, that makes sense. -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] ✓ Fi.CI.BAT: success for drm/i915: Enable inject_load_failure only in DEBUG config
== Series Details == Series: drm/i915: Enable inject_load_failure only in DEBUG config URL : https://patchwork.freedesktop.org/series/37495/ State : success == Summary == Series 37495v1 drm/i915: Enable inject_load_failure only in DEBUG config https://patchwork.freedesktop.org/api/1.0/series/37495/revisions/1/mbox/ Test kms_pipe_crc_basic: Subgroup suspend-read-crc-pipe-b: pass -> INCOMPLETE (fi-snb-2520m) fdo#103713 fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713 fi-bdw-5557u total:288 pass:267 dwarn:0 dfail:0 fail:0 skip:21 time:419s fi-bdw-gvtdvmtotal:288 pass:264 dwarn:0 dfail:0 fail:0 skip:24 time:423s fi-blb-e6850 total:288 pass:223 dwarn:1 dfail:0 fail:0 skip:64 time:373s fi-bsw-n3050 total:288 pass:242 dwarn:0 dfail:0 fail:0 skip:46 time:493s fi-bwr-2160 total:288 pass:183 dwarn:0 dfail:0 fail:0 skip:105 time:282s fi-bxt-dsi total:288 pass:258 dwarn:0 dfail:0 fail:0 skip:30 time:483s fi-bxt-j4205 total:288 pass:259 dwarn:0 dfail:0 fail:0 skip:29 time:487s fi-byt-j1900 total:288 pass:253 dwarn:0 dfail:0 fail:0 skip:35 time:469s fi-byt-n2820 total:288 pass:249 dwarn:0 dfail:0 fail:0 skip:39 time:463s fi-cfl-s2total:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:571s fi-elk-e7500 total:288 pass:229 dwarn:0 dfail:0 fail:0 skip:59 time:414s fi-gdg-551 total:288 pass:180 dwarn:0 dfail:0 fail:0 skip:108 time:280s fi-glk-1 total:288 pass:260 dwarn:0 dfail:0 fail:0 skip:28 time:512s fi-hsw-4770 total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:391s fi-hsw-4770r total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:400s fi-ilk-650 total:288 pass:228 dwarn:0 dfail:0 fail:0 skip:60 time:414s fi-ivb-3520m total:288 pass:259 dwarn:0 dfail:0 fail:0 skip:29 time:458s fi-ivb-3770 total:288 pass:255 dwarn:0 dfail:0 fail:0 skip:33 time:410s fi-kbl-7500u total:288 pass:263 dwarn:1 dfail:0 fail:0 skip:24 time:454s fi-kbl-7560u total:288 pass:269 dwarn:0 dfail:0 fail:0 skip:19 time:497s fi-kbl-7567u total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:454s fi-kbl-r total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:501s fi-pnv-d510 total:288 pass:222 dwarn:1 dfail:0 fail:0 skip:65 time:574s fi-skl-6260u total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:432s fi-skl-6600u total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:508s fi-skl-6700hqtotal:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:536s fi-skl-6700k2total:288 pass:264 dwarn:0 dfail:0 fail:0 skip:24 time:488s fi-skl-6770hqtotal:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:499s fi-skl-guc total:288 pass:260 dwarn:0 dfail:0 fail:0 skip:28 time:417s fi-skl-gvtdvmtotal:288 pass:265 dwarn:0 dfail:0 fail:0 skip:23 time:431s fi-snb-2520m total:245 pass:211 dwarn:0 dfail:0 fail:0 skip:33 Blacklisted hosts: fi-glk-dsi total:288 pass:258 dwarn:0 dfail:0 fail:0 skip:30 time:479s cc6c1df8bef5c07d1b05bebf56e66a9f10a4702b drm-tip: 2018y-02m-01d-16h-46m-26s UTC integration manifest e203cb386b4b drm/i915: Enable inject_load_failure only in DEBUG config == Logs == For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7854/issues.html ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 1/2] drm/i915/bxt, glk: Increase PCODE timeouts during CDCLK freq changing
On Tue, Jan 30, 2018 at 04:29:38PM +0200, Imre Deak wrote: > Currently we see sporadic timeouts during CDCLK changing both on BXT and > GLK as reported by the Bugzilla: ticket. It's easy to reproduce this by > changing the frequency in a tight loop after blanking the display. The > upper bound for the completion time is 800us based on my tests, so > increase it from the current 500us to 2ms; with that I couldn't trigger > the problem either on BXT or GLK. > > Note that timeouts happened during both the change notification and the > voltage level setting PCODE request. (For the latter one BSpec doesn't > require us to wait for completion before further HW programming.) > > This issue is similar to > 2c7d0602c815 ("drm/i915/gen9: Fix PCODE polling during CDCLK change > notification") > but there the PCODE request does complete (as shown by the mbox > busy flag), only the reply we get from PCODE indicates a failure. > So there we keep resending the request until a success reply, here we > just have to increase the timeout for the one PCODE request we send. > > v2: > - s/snb_pcode_request/sandybridge_pcode_write_timeout/ (Ville) > > Cc: Chris Wilson> Cc: Ville Syrjälä > Cc: # v4.4+ > Acked-by: Chris Wilson (v1) > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103326 > Signed-off-by: Imre Deak Reviewed-by: Ville Syrjälä > --- > drivers/gpu/drm/i915/i915_drv.h| 6 +- > drivers/gpu/drm/i915/intel_cdclk.c | 22 +- > drivers/gpu/drm/i915/intel_pm.c| 6 +++--- > 3 files changed, 25 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 454d8f937fae..7e83d0d3e632 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -3723,7 +3723,11 @@ extern void intel_display_print_error_state(struct > drm_i915_error_state_buf *e, > struct intel_display_error_state > *error); > > int sandybridge_pcode_read(struct drm_i915_private *dev_priv, u32 mbox, u32 > *val); > -int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u32 mbox, u32 > val); > +int sandybridge_pcode_write_timeout(struct drm_i915_private *dev_priv, u32 > mbox, > + u32 val, int timeout_us); > +#define sandybridge_pcode_write(dev_priv, mbox, val) \ > + sandybridge_pcode_write_timeout(dev_priv, mbox, val, 500) > + > int skl_pcode_request(struct drm_i915_private *dev_priv, u32 mbox, u32 > request, > u32 reply_mask, u32 reply, int timeout_base_ms); > > diff --git a/drivers/gpu/drm/i915/intel_cdclk.c > b/drivers/gpu/drm/i915/intel_cdclk.c > index c4392ea34a3d..a423b674fcec 100644 > --- a/drivers/gpu/drm/i915/intel_cdclk.c > +++ b/drivers/gpu/drm/i915/intel_cdclk.c > @@ -1370,10 +1370,15 @@ static void bxt_set_cdclk(struct drm_i915_private > *dev_priv, > break; > } > > - /* Inform power controller of upcoming frequency change */ > + /* > + * Inform power controller of upcoming frequency change. BSpec > + * requires us to wait up to 150usec, but that leads to timeouts; > + * the 2ms used here is based on experiment. > + */ > mutex_lock(_priv->pcu_lock); > - ret = sandybridge_pcode_write(dev_priv, HSW_PCODE_DE_WRITE_FREQ_REQ, > - 0x8000); > + ret = sandybridge_pcode_write_timeout(dev_priv, > + HSW_PCODE_DE_WRITE_FREQ_REQ, > + 0x8000, 2000); > mutex_unlock(_priv->pcu_lock); > > if (ret) { > @@ -1404,8 +1409,15 @@ static void bxt_set_cdclk(struct drm_i915_private > *dev_priv, > I915_WRITE(CDCLK_CTL, val); > > mutex_lock(_priv->pcu_lock); > - ret = sandybridge_pcode_write(dev_priv, HSW_PCODE_DE_WRITE_FREQ_REQ, > - cdclk_state->voltage_level); > + /* > + * The timeout isn't specified, the 2ms used here is based on > + * experiment. > + * FIXME: Waiting for the request completion could be delayed until > + * the next PCODE request based on BSpec. > + */ > + ret = sandybridge_pcode_write_timeout(dev_priv, > + HSW_PCODE_DE_WRITE_FREQ_REQ, > + cdclk_state->voltage_level, 2000); > mutex_unlock(_priv->pcu_lock); > > if (ret) { > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 0b92ea1dbd40..a6098932be5a 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -9169,8 +9169,8 @@ int sandybridge_pcode_read(struct drm_i915_private > *dev_priv, u32 mbox, u32 *val > return 0; > } > > -int
Re: [Intel-gfx] ✓ Fi.CI.IGT: success for series starting with [1/6] disable-gem-trace (rev2)
Quoting Patchwork (2018-02-01 17:52:06) > shard-apltotal:2838 pass:1750 dwarn:1 dfail:0 fail:23 skip:1064 > time:12757s > shard-hswtotal:2838 pass:1735 dwarn:1 dfail:0 fail:11 skip:1090 > time:12008s > shard-snbtotal:2838 pass:1328 dwarn:2 dfail:0 fail:10 skip:1498 > time:6582s So no regressions, but no kbl to show its almighty power! Review welcome! -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] clang warning: implicit conversion in intel_ddi.c:1481
On Thu, Feb 01, 2018 at 06:33:30PM +0100, Ozan Alpay wrote: > Dear Rodrigo Vivi, Ville Syrjälä, > > My name is Ozan Alpay, and I am a student mentored by Lukas Bulwahn. We > intend to use static analysis tools on the kernel source to identify, > analyze and report issues. As a very first step, we are looking into > clang compiler warnings and will then move to more sophisticated tools. > > When compiling Linux 4.15 with clang, we have discovered that your commit > 2952cd6fb4cc ("drm/i915: Let's use more enum intel_dpll_id pll_id.") > introduced the following warning: > > drivers/gpu/drm/i915/intel_ddi.c:1481:30: warning: implicit conversion from > enumeration type 'enum port' to different enumeration type 'enum > intel_dpll_id' [-Wenum-conversion] > enum intel_dpll_id pll_id = port; > > To reproduce it, you can compile Linux 4.15 with clang with this command: > > make HOSTCC=clang-5.0 defconfig && make -j32 HOSTCC=clang-5.0 CC=clang-5.0 > > If you don't have clang installed in your system, you can use this simple > docker setup to compile the kernel with clang: > > wget > https://raw.githubusercontent.com/bulwahn/linux-kernel-analysis/master/docker/kernel-clang/Dockerfile > && \ > docker build -t kernel-clang . && \ > docker run -v :/linux/ kernel-clang /bin/sh -c > "cd linux && make CC=clang-5.0 clean && make HOSTCC=clang-5.0 defconfig && > make -j32 HOSTCC=clang-5.0 CC=clang-5.0" > > While we were doing our analysis on 4.15, we noticed that you already > resolved this warning on linux-next with your work in commit bb911536f07e > ("drm/i915: Eliminate pll->state usage from bxt_calc_pll_link()"). So, > since it is resolved on linux-next and we expect that this commit will be > merged in the merge window for 4.16, there is probably nothing further to > do. > > Linux 4.15 is shipped with this clang warning, but we don't see the > crucial need to provide a backport commit to the stable branch for 4.15. > We just wanted to inform you about our analysis of this clang warning. > Ultimately the final call if you would like to address this clang warning > in 4.15 is yours. Note, I have not taken "clang warning fixes" for stable kernel updates in the past, and I doubt I will in the future, unless the tree "builds clean" with clang. If it eventually gets there, then yes, I will do that. Note, if you are going to email this out to everyone who fixes a warning message, you might want to reconsider it. That's going to be a lot of work, and for people who have already fixed an issue, it's kind of pointless to just remind them of work they have done in the past, right? What is the goal of these types of emails? thanks, greg k-h ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] i915 PSR test results and cursor lag
Quoting Andy Lutomirski (2018-02-01 17:40:22) > *However*, I do see one unfortunate side effect of turning on PSR. It > seems that, when I move my cursor a little bit after a few seconds of > doing nothing, there seems to be a little bit of lag, as if either a > few frames are dropped at the beginning of the motion or maybe the > entire motion is delayed a bit. I don't notice a similar delay when > typing, so I'm wondering if maybe there's a minor driver bug in which > the driver doesn't kick the panel out of PSR quite as quickly when the > cursor is updated as it does when the framebuffer is updated. One thing that's important know regarding the cursor is whether the display server is using a HW cursor or SW cursor. Could you please attach the log from the display server (or if you are using a stock distribution that's probably enough to work out what it is using)? -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] ✓ Fi.CI.IGT: success for series starting with [1/6] disable-gem-trace (rev2)
== Series Details == Series: series starting with [1/6] disable-gem-trace (rev2) URL : https://patchwork.freedesktop.org/series/37473/ State : success == Summary == Test kms_sysfs_edid_timing: warn -> PASS (shard-apl) fdo#100047 Test gem_eio: Subgroup in-flight-contexts: pass -> DMESG-WARN (shard-snb) fdo#104058 Test kms_setmode: Subgroup basic: pass -> FAIL (shard-hsw) fdo#99912 Test kms_flip: Subgroup plain-flip-ts-check: fail -> PASS (shard-hsw) fdo#100368 Test kms_plane_multiple: Subgroup atomic-pipe-a-tiling-x: pass -> FAIL (shard-apl) fdo#103831 fdo#100047 https://bugs.freedesktop.org/show_bug.cgi?id=100047 fdo#104058 https://bugs.freedesktop.org/show_bug.cgi?id=104058 fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912 fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368 fdo#103831 https://bugs.freedesktop.org/show_bug.cgi?id=103831 shard-apltotal:2838 pass:1750 dwarn:1 dfail:0 fail:23 skip:1064 time:12757s shard-hswtotal:2838 pass:1735 dwarn:1 dfail:0 fail:11 skip:1090 time:12008s shard-snbtotal:2838 pass:1328 dwarn:2 dfail:0 fail:10 skip:1498 time:6582s == Logs == For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7852/shards.html ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Enable inject_load_failure only in DEBUG config
Quoting Michal Wajdeczko (2018-02-01 17:45:52) > On Thu, 01 Feb 2018 18:42:00 +0100, Chris Wilson >wrote: > > > Quoting Michal Wajdeczko (2018-02-01 17:32:48) > >> We're using i915_inject_load_failure() to inject dummy > >> faults during driver load, but since this is debug utility > >> we shouldn't expose it in default config as it consumes > >> both code and data. > >> > >> add/remove: 0/1 grow/shrink: 0/2 up/down: 0/-302 (-302) > >> Function old new delta > >> __i915_inject_load_failure61 - -61 > >> i915_gem_init 13311268 -63 > >> i915_driver_load59235745-178 > >> Total: Before=1177454, After=1177152, chg -0.03% > >> > >> add/remove: 0/1 grow/shrink: 0/0 up/down: 0/-4 (-4) > >> Data old new delta > >> i915_load_fail_count 4 - -4 > >> Total: Before=56762, After=56758, chg -0.01% > >> > >> add/remove: 4/8 grow/shrink: 0/1 up/down: 245/-591 (-346) > >> RO Data old new delta > >> __param_str_inject_load_failure 20 - -20 > >> __UNIQUE_ID_inject_load_failuretype20034 - -34 > >> __param_inject_load_failure 40 - -40 > >> __func__49984896-102 > >> __UNIQUE_ID_inject_load_failure201 150 --150 > >> Total: Before=119095, After=118749, chg -0.29% > >> > >> Signed-off-by: Michal Wajdeczko > >> Cc: Chris Wilson > >> --- > >> drivers/gpu/drm/i915/i915_drv.c| 6 ++ > >> drivers/gpu/drm/i915/i915_drv.h| 4 > >> drivers/gpu/drm/i915/i915_params.c | 2 ++ > > > > git add for i915_params.h ? ;) > > No, > > It looks that we leave corresponding internal member unchanged. > See "error_capture" param that is also guarded by the config. Hmm, macros within macros. Ah yes. Fair enough, everything else looked good and I was just checking if there might be any other possible changes, Reviewed-by: Chris Wilson -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Enable inject_load_failure only in DEBUG config
On Thu, 01 Feb 2018 18:42:00 +0100, Chris Wilsonwrote: Quoting Michal Wajdeczko (2018-02-01 17:32:48) We're using i915_inject_load_failure() to inject dummy faults during driver load, but since this is debug utility we shouldn't expose it in default config as it consumes both code and data. add/remove: 0/1 grow/shrink: 0/2 up/down: 0/-302 (-302) Function old new delta __i915_inject_load_failure61 - -61 i915_gem_init 13311268 -63 i915_driver_load59235745-178 Total: Before=1177454, After=1177152, chg -0.03% add/remove: 0/1 grow/shrink: 0/0 up/down: 0/-4 (-4) Data old new delta i915_load_fail_count 4 - -4 Total: Before=56762, After=56758, chg -0.01% add/remove: 4/8 grow/shrink: 0/1 up/down: 245/-591 (-346) RO Data old new delta __param_str_inject_load_failure 20 - -20 __UNIQUE_ID_inject_load_failuretype20034 - -34 __param_inject_load_failure 40 - -40 __func__49984896-102 __UNIQUE_ID_inject_load_failure201 150 --150 Total: Before=119095, After=118749, chg -0.29% Signed-off-by: Michal Wajdeczko Cc: Chris Wilson --- drivers/gpu/drm/i915/i915_drv.c| 6 ++ drivers/gpu/drm/i915/i915_drv.h| 4 drivers/gpu/drm/i915/i915_params.c | 2 ++ git add for i915_params.h ? ;) No, It looks that we leave corresponding internal member unchanged. See "error_capture" param that is also guarded by the config. Michal ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] i915 PSR test results and cursor lag
On Thu, Feb 1, 2018 at 9:40 AM, Andy Lutomirskiwrote: > Hi- > > As requested in your blog post, I tested PSR. I see something like > 2.69W with PSR off and 2.17W with PSR on. Screen blanking, > suspend/resume, and the contents of the screen all seem okay. This is > a Dell XPS 13 9350, i.e.: > > System Information > Manufacturer: Dell Inc. > Product Name: XPS 13 9350 > > EDID is attached. > > *However*, I do see one unfortunate side effect of turning on PSR. It > seems that, when I move my cursor a little bit after a few seconds of > doing nothing, there seems to be a little bit of lag, as if either a > few frames are dropped at the beginning of the motion or maybe the > entire motion is delayed a bit. I don't notice a similar delay when > typing, so I'm wondering if maybe there's a minor driver bug in which > the driver doesn't kick the panel out of PSR quite as quickly when the > cursor is updated as it does when the framebuffer is updated. > I'm also getting occasional messages like: [ 2675.574486] [drm:intel_pipe_update_start [i915]] *ERROR* Potential atomic update failure on pipe A with PSR on. But there is nowhere near one of these messages per tiny lag incident. ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Enable inject_load_failure only in DEBUG config
Quoting Michal Wajdeczko (2018-02-01 17:32:48) > We're using i915_inject_load_failure() to inject dummy > faults during driver load, but since this is debug utility > we shouldn't expose it in default config as it consumes > both code and data. > > add/remove: 0/1 grow/shrink: 0/2 up/down: 0/-302 (-302) > Function old new delta > __i915_inject_load_failure61 - -61 > i915_gem_init 13311268 -63 > i915_driver_load59235745-178 > Total: Before=1177454, After=1177152, chg -0.03% > > add/remove: 0/1 grow/shrink: 0/0 up/down: 0/-4 (-4) > Data old new delta > i915_load_fail_count 4 - -4 > Total: Before=56762, After=56758, chg -0.01% > > add/remove: 4/8 grow/shrink: 0/1 up/down: 245/-591 (-346) > RO Data old new delta > __param_str_inject_load_failure 20 - -20 > __UNIQUE_ID_inject_load_failuretype20034 - -34 > __param_inject_load_failure 40 - -40 > __func__49984896-102 > __UNIQUE_ID_inject_load_failure201 150 --150 > Total: Before=119095, After=118749, chg -0.29% > > Signed-off-by: Michal Wajdeczko> Cc: Chris Wilson > --- > drivers/gpu/drm/i915/i915_drv.c| 6 ++ > drivers/gpu/drm/i915/i915_drv.h| 4 > drivers/gpu/drm/i915/i915_params.c | 2 ++ git add for i915_params.h ? ;) -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] i915 PSR test results and cursor lag
Hi- As requested in your blog post, I tested PSR. I see something like 2.69W with PSR off and 2.17W with PSR on. Screen blanking, suspend/resume, and the contents of the screen all seem okay. This is a Dell XPS 13 9350, i.e.: System Information Manufacturer: Dell Inc. Product Name: XPS 13 9350 EDID is attached. *However*, I do see one unfortunate side effect of turning on PSR. It seems that, when I move my cursor a little bit after a few seconds of doing nothing, there seems to be a little bit of lag, as if either a few frames are dropped at the beginning of the motion or maybe the entire motion is delayed a bit. I don't notice a similar delay when typing, so I'm wondering if maybe there's a minor driver bug in which the driver doesn't kick the panel out of PSR quite as quickly when the cursor is updated as it does when the framebuffer is updated. (A couple of lists are cc'd BTW, switching PSR on and off using /sys/module/i915/parameters/enable_psr seems to work fine, although it seems like I may need to suspend/resume to get it to kick in. But, if there's really going to be a blacklist or whitelist of panels in userspace, shouldn't there be an option in sysfs in /sys/class/drm/card0-eDP-1/ or similar? --Andy panel-edid Description: Binary data ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: Enable inject_load_failure only in DEBUG config
We're using i915_inject_load_failure() to inject dummy faults during driver load, but since this is debug utility we shouldn't expose it in default config as it consumes both code and data. add/remove: 0/1 grow/shrink: 0/2 up/down: 0/-302 (-302) Function old new delta __i915_inject_load_failure61 - -61 i915_gem_init 13311268 -63 i915_driver_load59235745-178 Total: Before=1177454, After=1177152, chg -0.03% add/remove: 0/1 grow/shrink: 0/0 up/down: 0/-4 (-4) Data old new delta i915_load_fail_count 4 - -4 Total: Before=56762, After=56758, chg -0.01% add/remove: 4/8 grow/shrink: 0/1 up/down: 245/-591 (-346) RO Data old new delta __param_str_inject_load_failure 20 - -20 __UNIQUE_ID_inject_load_failuretype20034 - -34 __param_inject_load_failure 40 - -40 __func__49984896-102 __UNIQUE_ID_inject_load_failure201 150 --150 Total: Before=119095, After=118749, chg -0.29% Signed-off-by: Michal WajdeczkoCc: Chris Wilson --- drivers/gpu/drm/i915/i915_drv.c| 6 ++ drivers/gpu/drm/i915/i915_drv.h| 4 drivers/gpu/drm/i915/i915_params.c | 2 ++ 3 files changed, 12 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 1ec12ad..6e7e933 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -55,6 +55,7 @@ static struct drm_driver driver; +#if IS_ENABLED(CONFIG_DRM_I915_DEBUG) static unsigned int i915_load_fail_count; bool __i915_inject_load_failure(const char *func, int line) @@ -70,6 +71,7 @@ bool __i915_inject_load_failure(const char *func, int line) return false; } +#endif #define FDO_BUG_URL "https://bugs.freedesktop.org/enter_bug.cgi?product=DRI; #define FDO_BUG_MSG "Please file a bug at " FDO_BUG_URL " against DRM/Intel " \ @@ -107,8 +109,12 @@ bool __i915_inject_load_failure(const char *func, int line) static bool i915_error_injected(struct drm_i915_private *dev_priv) { +#if IS_ENABLED(CONFIG_DRM_I915_DEBUG) return i915_modparams.inject_load_failure && i915_load_fail_count == i915_modparams.inject_load_failure; +#else + return false; +#endif } #define i915_load_error(dev_priv, fmt, ...) \ diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index c676269..8344ecb 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -104,9 +104,13 @@ #define I915_STATE_WARN_ON(x) \ I915_STATE_WARN((x), "%s", "WARN_ON(" __stringify(x) ")") +#if IS_ENABLED(CONFIG_DRM_I915_DEBUG) bool __i915_inject_load_failure(const char *func, int line); #define i915_inject_load_failure() \ __i915_inject_load_failure(__func__, __LINE__) +#else +#define i915_inject_load_failure() false +#endif typedef struct { uint32_t val; diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c index 0b553a8..08108ce 100644 --- a/drivers/gpu/drm/i915/i915_params.c +++ b/drivers/gpu/drm/i915/i915_params.c @@ -167,8 +167,10 @@ struct i915_params i915_modparams __read_mostly = { i915_param_named_unsafe(enable_dp_mst, bool, 0600, "Enable multi-stream transport (MST) for new DisplayPort sinks. (default: true)"); +#if IS_ENABLED(CONFIG_DRM_I915_DEBUG) i915_param_named_unsafe(inject_load_failure, uint, 0400, "Force an error after a number of failure check points (0:disabled (default), N:force failure at the Nth failure check point)"); +#endif i915_param_named(enable_dpcd_backlight, bool, 0600, "Enable support for DPCD backlight control (default:false)"); -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] ✗ Fi.CI.BAT: failure for drm/i915: Fix plane regression
== Series Details == Series: drm/i915: Fix plane regression URL : https://patchwork.freedesktop.org/series/37492/ State : failure == Summary == Applying: drm/i915: Add .get_hw_state() method for planes error: sha1 information is lacking or useless (drivers/gpu/drm/i915/intel_display.c). error: could not build fake ancestor Patch failed at 0001 drm/i915: Add .get_hw_state() method for planes The copy of the patch that failed is found in: .git/rebase-apply/patch When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort". ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH stable-4.14 0/3] drm/i915: Fix plane regression
From: Ville SyrjäläThese backports fix a plane related regression causing a corrupted screen and bunch of WARNs from the kernel on some pre-i965 era hardware. Cc: # 4.14 Ville Syrjälä (3): drm/i915: Add .get_hw_state() method for planes drm/i915: Redo plane sanitation during readout drm/i915: Fix deadlock in i830_disable_pipe() drivers/gpu/drm/i915/intel_display.c | 303 ++- drivers/gpu/drm/i915/intel_drv.h | 2 + drivers/gpu/drm/i915/intel_sprite.c | 83 ++ 3 files changed, 242 insertions(+), 146 deletions(-) -- 2.13.6 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH stable-4.14 3/3] drm/i915: Fix deadlock in i830_disable_pipe()
From: Ville Syrjäläi830_disable_pipe() gets called from the power well code, and thus we're already holding the power domain mutex. That means we can't call plane->get_hw_state() as it will also try to grab the same mutex and will thus deadlock. Replace the assert_plane() calls (which calls ->get_hw_state()) with just raw register reads in i830_disable_pipe(). As a bonus we can now get a warning if plane C is enabled even though we don't even expose it as a drm plane. v2: Do a separate WARN_ON() for each plane (Chris) Cc: # 4.14 Cc: Chris Wilson Reviewed-by: Chris Wilson Fixes: d87ce7640295 ("drm/i915: Add .get_hw_state() method for planes") Signed-off-by: Ville Syrjälä Link: https://patchwork.freedesktop.org/patch/msgid/20171129125411.29055-1-ville.syrj...@linux.intel.com (cherry picked from commit 5816d9cbc0a0fbf232fe297cefcb85361a3cde90) Signed-off-by: Jani Nikula (cherry picked from commit 4488496d58200c7511842e049a4cc891d928da56) --- drivers/gpu/drm/i915/intel_display.c | 7 +-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 751b0f0582bc..46485692db48 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -14717,8 +14717,11 @@ void i830_disable_pipe(struct drm_i915_private *dev_priv, enum pipe pipe) DRM_DEBUG_KMS("disabling pipe %c due to force quirk\n", pipe_name(pipe)); - assert_planes_disabled(intel_get_crtc_for_pipe(dev_priv, PIPE_A)); - assert_planes_disabled(intel_get_crtc_for_pipe(dev_priv, PIPE_B)); + WARN_ON(I915_READ(DSPCNTR(PLANE_A)) & DISPLAY_PLANE_ENABLE); + WARN_ON(I915_READ(DSPCNTR(PLANE_B)) & DISPLAY_PLANE_ENABLE); + WARN_ON(I915_READ(DSPCNTR(PLANE_C)) & DISPLAY_PLANE_ENABLE); + WARN_ON(I915_READ(CURCNTR(PIPE_A)) & CURSOR_MODE); + WARN_ON(I915_READ(CURCNTR(PIPE_B)) & CURSOR_MODE); I915_WRITE(PIPECONF(pipe), 0); POSTING_READ(PIPECONF(pipe)); -- 2.13.6 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH stable-4.14 2/3] drm/i915: Redo plane sanitation during readout
From: Ville SyrjäläUnify the plane disabling during state readout by pulling the code into a new helper intel_plane_disable_noatomic(). We'll also read out the state of all planes, so that we know which planes really need to be diabled. Additonally we change the plane<->pipe mapping sanitation to work by simply disabling the offending planes instead of entire pipes. And we do it before we otherwise sanitize the crtcs, which means we don't have to worry about misassigned planes during crtc sanitation anymore. v2: Reoder patches to not depend on enum old_plane_id v3: s/for_each_pipe/for_each_intel_crtc/ Cc: # 4.14 Cc: Thierry Reding Cc: Alex Villacís Lasso Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103223 Reviewed-by: Daniel Vetter Tested-by: Thierry Reding Link: https://patchwork.freedesktop.org/patch/msgid/20171117191917.11506-3-ville.syrj...@linux.intel.com Signed-off-by: Ville Syrjälä (cherry picked from commit b1e01595a66dc206a2c75401ec4c285740537f3f) Signed-off-by: Jani Nikula (cherry picked from commit 23ac12732825901b3fc6ac720958d8bff9a0d6ec) --- drivers/gpu/drm/i915/intel_display.c | 114 --- 1 file changed, 65 insertions(+), 49 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 47094d4a8f0e..751b0f0582bc 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -2756,6 +2756,23 @@ intel_set_plane_visible(struct intel_crtc_state *crtc_state, crtc_state->active_planes); } +static void intel_plane_disable_noatomic(struct intel_crtc *crtc, +struct intel_plane *plane) +{ + struct intel_crtc_state *crtc_state = + to_intel_crtc_state(crtc->base.state); + struct intel_plane_state *plane_state = + to_intel_plane_state(plane->base.state); + + intel_set_plane_visible(crtc_state, plane_state, false); + + if (plane->id == PLANE_PRIMARY) + intel_pre_disable_primary_noatomic(>base); + + trace_intel_disable_plane(>base, crtc); + plane->disable_plane(plane, crtc); +} + static void intel_find_initial_plane_obj(struct intel_crtc *intel_crtc, struct intel_initial_plane_config *plane_config) @@ -2813,12 +2830,7 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc, * simplest solution is to just disable the primary plane now and * pretend the BIOS never had it enabled. */ - intel_set_plane_visible(to_intel_crtc_state(crtc_state), - to_intel_plane_state(plane_state), - false); - intel_pre_disable_primary_noatomic(_crtc->base); - trace_intel_disable_plane(primary, intel_crtc); - intel_plane->disable_plane(intel_plane, intel_crtc); + intel_plane_disable_noatomic(intel_crtc, intel_plane); return; @@ -5954,6 +5966,7 @@ static void intel_crtc_disable_noatomic(struct drm_crtc *crtc, struct intel_crtc *intel_crtc = to_intel_crtc(crtc); struct drm_i915_private *dev_priv = to_i915(crtc->dev); enum intel_display_power_domain domain; + struct intel_plane *plane; u64 domains; struct drm_atomic_state *state; struct intel_crtc_state *crtc_state; @@ -5962,11 +5975,12 @@ static void intel_crtc_disable_noatomic(struct drm_crtc *crtc, if (!intel_crtc->active) return; - if (crtc->primary->state->visible) { - intel_pre_disable_primary_noatomic(crtc); + for_each_intel_plane_on_crtc(_priv->drm, intel_crtc, plane) { + const struct intel_plane_state *plane_state = + to_intel_plane_state(plane->base.state); - intel_crtc_disable_planes(crtc, 1 << drm_plane_index(crtc->primary)); - crtc->primary->state->visible = false; + if (plane_state->base.visible) + intel_plane_disable_noatomic(intel_crtc, plane); } state = drm_atomic_state_alloc(crtc->dev); @@ -14715,22 +14729,36 @@ void i830_disable_pipe(struct drm_i915_private *dev_priv, enum pipe pipe) POSTING_READ(DPLL(pipe)); } -static bool -intel_check_plane_mapping(struct intel_crtc *crtc) +static bool intel_plane_mapping_ok(struct intel_crtc *crtc, + struct intel_plane *primary) { struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); - u32 val; + enum plane plane = primary->plane; + u32 val = I915_READ(DSPCNTR(plane)); - if (INTEL_INFO(dev_priv)->num_pipes == 1) - return true; + return (val &
[Intel-gfx] [PATCH stable-4.14 1/3] drm/i915: Add .get_hw_state() method for planes
From: Ville SyrjäläAdd a .get_hw_state() method for planes, returning true or false depending on whether the plane is enabled. Use it to rewrite the plane enabled/disabled asserts in platform agnostic fashion. We do lose the pre-gen4 plane<->pipe mapping checks, but since we're supposed sanitize that anyway it doesn't really matter. v2: Reoder patches to not depend on enum old_plane_id Just call assert_plane_disabled() from assert_planes_disabled() v3: Deal with disabled power wells in .get_hw_state() v4: Rebase due skl primary plane code removal Cc: # 4.14 Cc: Thierry Reding Cc: Alex Villacís Lasso Reviewed-by: Daniel Vetter #v2 Tested-by: Thierry Reding #v2 Link: https://patchwork.freedesktop.org/patch/msgid/20171117191917.11506-2-ville.syrj...@linux.intel.com Signed-off-by: Ville Syrjälä (cherry picked from commit 51f5a096398433a881e845d3685a2c1dac756019) Signed-off-by: Jani Nikula (cherry picked from commit d87ce76402950b8e4d5117276d44465658e886a4) --- drivers/gpu/drm/i915/intel_display.c | 188 +-- drivers/gpu/drm/i915/intel_drv.h | 2 + drivers/gpu/drm/i915/intel_sprite.c | 83 3 files changed, 175 insertions(+), 98 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 095a2240af4f..47094d4a8f0e 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -1211,23 +1211,6 @@ void assert_panel_unlocked(struct drm_i915_private *dev_priv, enum pipe pipe) pipe_name(pipe)); } -static void assert_cursor(struct drm_i915_private *dev_priv, - enum pipe pipe, bool state) -{ - bool cur_state; - - if (IS_I845G(dev_priv) || IS_I865G(dev_priv)) - cur_state = I915_READ(CURCNTR(PIPE_A)) & CURSOR_ENABLE; - else - cur_state = I915_READ(CURCNTR(pipe)) & CURSOR_MODE; - - I915_STATE_WARN(cur_state != state, -"cursor on pipe %c assertion failure (expected %s, current %s)\n", - pipe_name(pipe), onoff(state), onoff(cur_state)); -} -#define assert_cursor_enabled(d, p) assert_cursor(d, p, true) -#define assert_cursor_disabled(d, p) assert_cursor(d, p, false) - void assert_pipe(struct drm_i915_private *dev_priv, enum pipe pipe, bool state) { @@ -1255,77 +1238,25 @@ void assert_pipe(struct drm_i915_private *dev_priv, pipe_name(pipe), onoff(state), onoff(cur_state)); } -static void assert_plane(struct drm_i915_private *dev_priv, -enum plane plane, bool state) +static void assert_plane(struct intel_plane *plane, bool state) { - u32 val; - bool cur_state; + bool cur_state = plane->get_hw_state(plane); - val = I915_READ(DSPCNTR(plane)); - cur_state = !!(val & DISPLAY_PLANE_ENABLE); I915_STATE_WARN(cur_state != state, -"plane %c assertion failure (expected %s, current %s)\n", - plane_name(plane), onoff(state), onoff(cur_state)); + "%s assertion failure (expected %s, current %s)\n", + plane->base.name, onoff(state), onoff(cur_state)); } -#define assert_plane_enabled(d, p) assert_plane(d, p, true) -#define assert_plane_disabled(d, p) assert_plane(d, p, false) - -static void assert_planes_disabled(struct drm_i915_private *dev_priv, - enum pipe pipe) -{ - int i; - - /* Primary planes are fixed to pipes on gen4+ */ - if (INTEL_GEN(dev_priv) >= 4) { - u32 val = I915_READ(DSPCNTR(pipe)); - I915_STATE_WARN(val & DISPLAY_PLANE_ENABLE, -"plane %c assertion failure, should be disabled but not\n", -plane_name(pipe)); - return; - } - - /* Need to check both planes against the pipe */ - for_each_pipe(dev_priv, i) { - u32 val = I915_READ(DSPCNTR(i)); - enum pipe cur_pipe = (val & DISPPLANE_SEL_PIPE_MASK) >> - DISPPLANE_SEL_PIPE_SHIFT; - I915_STATE_WARN((val & DISPLAY_PLANE_ENABLE) && pipe == cur_pipe, -"plane %c assertion failure, should be off on pipe %c but is still active\n", -plane_name(i), pipe_name(pipe)); - } -} +#define assert_plane_enabled(p) assert_plane(p, true) +#define assert_plane_disabled(p) assert_plane(p, false) -static void assert_sprites_disabled(struct drm_i915_private *dev_priv, - enum pipe pipe) +static void assert_planes_disabled(struct intel_crtc *crtc) { - int sprite; + struct drm_i915_private *dev_priv =
Re: [Intel-gfx] [PATCH] drm/i915: Add option to list load failure checkpoints
Quoting Michal Wajdeczko (2018-02-01 15:51:58) > I was assuming these steps: > > 1. add new failure checkpoint(s) to the code > 2. boot driver with inject modparam=-1 to list active checkpoints > 3. note the number of checkpoint that we want to trigger > 4. boot driver with inject modparam=n to trigger selected checkpoint > 5. repeat steps 2-4 in case of rebase/code refactor/new checkpoints > > > how to make it more useful than drv_module_reload and to keep it working > > as you intend. A pure fault-counter doesn't seem very robust or > > intuitive for me when you are trying to develop a new fault point. Otoh, > > Currently we identify fault checkpoint by pair [function:line] so if we > want > to invest more in this mechanism we can change modparam to char* and then > check against function and line instead of the counter. Then no iteration > (or listing checkpoints) would be needed. So I've no strong objection to the overall approach. (If time were infinite, I would push the fault-counters into a .data section so we could probe them from the i915.ko without loading.) I think though to make it really useful would be to add a tag to each fault-point (NULL for boring existing ones unless you're feeling inventive). Then the developer need only say ./tests/drv_reload_fault my-tag and that will then be stable across different kernels. The minimum bar for adding this dev feature would be the corresponding tool for igt, with a small integrated test. (Hmm, if we could push the tags to .data, we could even tell how many fault-injection sites were hit.) Long term desire would be kprobe/ebpf hookup, or some [soon-to-be] existing infrastructure for fault-injection along those lines. -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v11] drm/i915/icl: Gen11 forcewake support
From: Daniele Ceraolo SpurioThe main difference with previous GENs is that starting from Gen11 each VCS and VECS engine has its own power well, which only exist if the related engine exists in the HW. The fallback forcewake request workaround is only needed on gen9 according to the HSDES WA entry (1604254524), so we can go back to using the simpler fw_domains_get/put functions. BSpec: 18331 v2: fix fwtable, use array to test shadow tables, create new accessors to avoid check on every access (Tvrtko) v3 (from Paulo): Rebase. v4: - Range 09400-097FF should be FORCEWAKE_ALL (Daniele) - Use the BIT macro for forcewake domains (Daniele) - Add a comment about the range ordering (Oscar) - Updated commit message (Oscar) v5: Rebased v6: Use I915_MAX_VCS/VECS (Michal) v7: translate FORCEWAKE_ALL to available domains v8: rebase, add clarification on fallback ack in commit message. v9: fix rebase issue, change check in fw_domains_init from IS_GEN11 to GEN >= 11 v10: Generate is_genX_shadowed with a macro (Daniele) Include gen11_fw_ranges in the selftest (Michel) v11: Simplify FORCEWAKE_ALL, new line between NEEDS_FORCEWAKEs (Tvrtko) Cc: Michal Wajdeczko Cc: Tvrtko Ursulin Cc: Paulo Zanoni Acked-by: Michel Thierry Signed-off-by: Daniele Ceraolo Spurio Signed-off-by: Oscar Mateo Signed-off-by: Michel Thierry Reviewed-by: Tvrtko Ursulin --- drivers/gpu/drm/i915/i915_reg.h | 4 + drivers/gpu/drm/i915/intel_uncore.c | 157 -- drivers/gpu/drm/i915/intel_uncore.h | 23 +++- drivers/gpu/drm/i915/selftests/intel_uncore.c | 31 +++-- 4 files changed, 189 insertions(+), 26 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index d29e8a0e2ca3..eaca12292ffe 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -8015,9 +8015,13 @@ enum { #define VLV_GTLC_PW_RENDER_STATUS_MASK (1 << 7) #define FORCEWAKE_MT _MMIO(0xa188) /* multi-threaded */ #define FORCEWAKE_MEDIA_GEN9 _MMIO(0xa270) +#define FORCEWAKE_MEDIA_VDBOX_GEN11(n)_MMIO(0xa540 + (n) * 4) +#define FORCEWAKE_MEDIA_VEBOX_GEN11(n)_MMIO(0xa560 + (n) * 4) #define FORCEWAKE_RENDER_GEN9 _MMIO(0xa278) #define FORCEWAKE_BLITTER_GEN9_MMIO(0xa188) #define FORCEWAKE_ACK_MEDIA_GEN9 _MMIO(0x0D88) +#define FORCEWAKE_ACK_MEDIA_VDBOX_GEN11(n)_MMIO(0x0D50 + (n) * 4) +#define FORCEWAKE_ACK_MEDIA_VEBOX_GEN11(n)_MMIO(0x0D70 + (n) * 4) #define FORCEWAKE_ACK_RENDER_GEN9 _MMIO(0x0D84) #define FORCEWAKE_ACK_BLITTER_GEN9_MMIO(0x130044) #define FORCEWAKE_KERNEL BIT(0) diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c index 164dbb8cfa36..abe3e2d44a25 100644 --- a/drivers/gpu/drm/i915/intel_uncore.c +++ b/drivers/gpu/drm/i915/intel_uncore.c @@ -37,6 +37,12 @@ static const char * const forcewake_domain_names[] = { "render", "blitter", "media", + "vdbox0", + "vdbox1", + "vdbox2", + "vdbox3", + "vebox0", + "vebox1", }; const char * @@ -774,6 +780,9 @@ void assert_forcewakes_active(struct drm_i915_private *dev_priv, /* We give fast paths for the really cool registers */ #define NEEDS_FORCE_WAKE(reg) ((reg) < 0x4) +#define GEN11_NEEDS_FORCE_WAKE(reg) \ + ((reg) < 0x4 || ((reg) >= 0x1c && (reg) < 0x1dc000)) + #define __gen6_reg_read_fw_domains(offset) \ ({ \ enum forcewake_domains __fwd; \ @@ -826,6 +835,14 @@ find_fw_domain(struct drm_i915_private *dev_priv, u32 offset) if (!entry) return 0; + /* +* The list of FW domains depends on the SKU in gen11+ so we +* can't determine it statically. We use FORCEWAKE_ALL and +* translate it here to the list of available domains. +*/ + if (entry->domains == FORCEWAKE_ALL) + return dev_priv->uncore.fw_domains; + WARN(entry->domains & ~dev_priv->uncore.fw_domains, "Uninitialized forcewake domain(s) 0x%x accessed at 0x%x\n", entry->domains & ~dev_priv->uncore.fw_domains, offset); @@ -860,6 +877,14 @@ static const struct intel_forcewake_range __vlv_fw_ranges[] = { __fwd; \ }) +#define __gen11_fwtable_reg_read_fw_domains(offset) \ +({ \ + enum forcewake_domains __fwd = 0; \ + if (GEN11_NEEDS_FORCE_WAKE((offset))) \ + __fwd = find_fw_domain(dev_priv, offset); \ + __fwd; \ +}) + /* *Must* be sorted by offset! See intel_shadow_table_check(). */ static const i915_reg_t
Re: [Intel-gfx] [PATCH v10] drm/i915/icl: Gen11 forcewake support
On 2/1/2018 2:25 AM, Tvrtko Ursulin wrote: On 01/02/2018 00:52, Michel Thierry wrote: From: Daniele Ceraolo SpurioThe main difference with previous GENs is that starting from Gen11 each VCS and VECS engine has its own power well, which only exist if the related engine exists in the HW. The fallback forcewake request workaround is only needed on gen9 according to the HSDES WA entry (1604254524), so we can go back to using the simpler fw_domains_get/put functions. BSpec: 18331 v2: fix fwtable, use array to test shadow tables, create new accessors to avoid check on every access (Tvrtko) v3 (from Paulo): Rebase. v4: - Range 09400-097FF should be FORCEWAKE_ALL (Daniele) - Use the BIT macro for forcewake domains (Daniele) - Add a comment about the range ordering (Oscar) - Updated commit message (Oscar) v5: Rebased v6: Use I915_MAX_VCS/VECS (Michal) v7: translate FORCEWAKE_ALL to available domains v8: rebase, add clarification on fallback ack in commit message. v9: fix rebase issue, change check in fw_domains_init from IS_GEN11 to GEN >= 11 v10: Generate is_genX_shadowed with a macro (Daniele) Include gen11_fw_ranges in the selftest (Michel) Cc: Michal Wajdeczko Cc: Tvrtko Ursulin Cc: Paulo Zanoni Acked-by: Michel Thierry Signed-off-by: Daniele Ceraolo Spurio Signed-off-by: Oscar Mateo Signed-off-by: Michel Thierry --- drivers/gpu/drm/i915/i915_reg.h | 4 + drivers/gpu/drm/i915/intel_uncore.c | 155 -- drivers/gpu/drm/i915/intel_uncore.h | 27 - drivers/gpu/drm/i915/selftests/intel_uncore.c | 31 -- 4 files changed, 193 insertions(+), 24 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index d29e8a0e2ca3..eaca12292ffe 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -8015,9 +8015,13 @@ enum { #define VLV_GTLC_PW_RENDER_STATUS_MASK (1 << 7) #define FORCEWAKE_MT _MMIO(0xa188) /* multi-threaded */ #define FORCEWAKE_MEDIA_GEN9 _MMIO(0xa270) +#define FORCEWAKE_MEDIA_VDBOX_GEN11(n) _MMIO(0xa540 + (n) * 4) +#define FORCEWAKE_MEDIA_VEBOX_GEN11(n) _MMIO(0xa560 + (n) * 4) #define FORCEWAKE_RENDER_GEN9 _MMIO(0xa278) #define FORCEWAKE_BLITTER_GEN9 _MMIO(0xa188) #define FORCEWAKE_ACK_MEDIA_GEN9 _MMIO(0x0D88) +#define FORCEWAKE_ACK_MEDIA_VDBOX_GEN11(n) _MMIO(0x0D50 + (n) * 4) +#define FORCEWAKE_ACK_MEDIA_VEBOX_GEN11(n) _MMIO(0x0D70 + (n) * 4) #define FORCEWAKE_ACK_RENDER_GEN9 _MMIO(0x0D84) #define FORCEWAKE_ACK_BLITTER_GEN9 _MMIO(0x130044) #define FORCEWAKE_KERNEL BIT(0) diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c index 164dbb8cfa36..c1953043604b 100644 --- a/drivers/gpu/drm/i915/intel_uncore.c +++ b/drivers/gpu/drm/i915/intel_uncore.c @@ -37,6 +37,12 @@ static const char * const forcewake_domain_names[] = { "render", "blitter", "media", + "vdbox0", + "vdbox1", + "vdbox2", + "vdbox3", + "vebox0", + "vebox1", }; const char * @@ -773,6 +779,8 @@ void assert_forcewakes_active(struct drm_i915_private *dev_priv, /* We give fast paths for the really cool registers */ #define NEEDS_FORCE_WAKE(reg) ((reg) < 0x4) +#define GEN11_NEEDS_FORCE_WAKE(reg) \ + ((reg) < 0x4 || ((reg) >= 0x1c && (reg) < 0x1dc000)) Nitpick - I'd perhaps at least have a blank line between the two defines, or even moved the GEN11 lower in file, just before the first mention of GEN11 specific code starts appearing. I'd go for a new blank line, it makes it obvious something changed between gens. #define __gen6_reg_read_fw_domains(offset) \ ({ \ @@ -826,6 +834,14 @@ find_fw_domain(struct drm_i915_private *dev_priv, u32 offset) if (!entry) return 0; + /* + * The list of FW domains depends on the SKU in gen11+ so we + * can't determine it statically. We use FORCEWAKE_ALL and + * translate it here to the list of available domains. + */ + if (entry->domains == FORCEWAKE_ALL) + return dev_priv->uncore.fw_domains; + WARN(entry->domains & ~dev_priv->uncore.fw_domains, "Uninitialized forcewake domain(s) 0x%x accessed at 0x%x\n", entry->domains & ~dev_priv->uncore.fw_domains, offset); @@ -860,6 +876,14 @@ static const struct intel_forcewake_range __vlv_fw_ranges[] = { __fwd; \ }) +#define __gen11_fwtable_reg_read_fw_domains(offset) \ +({ \ + enum forcewake_domains __fwd = 0; \ + if (GEN11_NEEDS_FORCE_WAKE((offset))) \ + __fwd = find_fw_domain(dev_priv, offset); \ + __fwd; \ +}) + /* *Must* be sorted
Re: [Intel-gfx] [PATCH] drm/i915: Add option to list load failure checkpoints
On Thu, 01 Feb 2018 16:28:31 +0100, Chris Wilsonwrote: Quoting Michal Wajdeczko (2018-02-01 14:47:05) On Wed, 31 Jan 2018 21:48:42 +0100, Chris Wilson wrote: > Quoting Michal Wajdeczko (2018-01-31 18:23:47) >> Our inject_load_failure functionality allows to insert one >> failure during driver load, but it is hard to guess which >> number should passed as modparam to select specific checkpoint. >> >> Use negative number as option to list all available failure >> checkpoints without triggering any failure. > > Hmm, it was only intended for use with the coupled igt test. Mind > expanding upon the use case you have? Could you not use that iterative > search for finding the injection value you want for repeated runs? For > the bisect case, do you not want to keep it iterating over all in case > the value changes? How stable do you want the modparam? Iterative approach is good for validation team to verify that all existing failure points are correctly handled (ie. we don't cause crash/panic), but it is less useful when you are interested in adding new checkpoints just for your code, as it requires both time and luck as you may be hit by earlier checkpoint that no longer works ;) Btw, IMHO this modparam should only be exposed in DEBUG config (as it introduces some code and text), and maybe we should also consider extending it to support more than one failure (as then it will be easy to check several code paths (like failure in fallback) Sure, let's get it under the IS_ENABLED(CONFIG_DRM_I915_DEBUG). I'll send separate patch for this. This patch was trying to keep definition of modparam unchanged (extra -1 value should not be noticed by any existing use case) but since it is purely debug feature I'm not against making bigger changes here. From my pov, it's very useful to define how you expect it to work, and I was assuming these steps: 1. add new failure checkpoint(s) to the code 2. boot driver with inject modparam=-1 to list active checkpoints 3. note the number of checkpoint that we want to trigger 4. boot driver with inject modparam=n to trigger selected checkpoint 5. repeat steps 2-4 in case of rebase/code refactor/new checkpoints how to make it more useful than drv_module_reload and to keep it working as you intend. A pure fault-counter doesn't seem very robust or intuitive for me when you are trying to develop a new fault point. Otoh, Currently we identify fault checkpoint by pair [function:line] so if we want to invest more in this mechanism we can change modparam to char* and then check against function and line instead of the counter. Then no iteration (or listing checkpoints) would be needed. I don't mind if the developer has to fix all the earlier fails when testing his (those fails are all our responsibility) -- or at least file bugs so that awareness is raised. Agree. Michal ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Add option to list load failure checkpoints
Quoting Michal Wajdeczko (2018-02-01 14:47:05) > On Wed, 31 Jan 2018 21:48:42 +0100, Chris Wilson >wrote: > > > Quoting Michal Wajdeczko (2018-01-31 18:23:47) > >> Our inject_load_failure functionality allows to insert one > >> failure during driver load, but it is hard to guess which > >> number should passed as modparam to select specific checkpoint. > >> > >> Use negative number as option to list all available failure > >> checkpoints without triggering any failure. > > > > Hmm, it was only intended for use with the coupled igt test. Mind > > expanding upon the use case you have? Could you not use that iterative > > search for finding the injection value you want for repeated runs? For > > the bisect case, do you not want to keep it iterating over all in case > > the value changes? How stable do you want the modparam? > > Iterative approach is good for validation team to verify that all > existing failure points are correctly handled (ie. we don't cause > crash/panic), but it is less useful when you are interested in adding > new checkpoints just for your code, as it requires both time and luck > as you may be hit by earlier checkpoint that no longer works ;) > > Btw, IMHO this modparam should only be exposed in DEBUG config (as it > introduces some code and text), and maybe we should also consider > extending it to support more than one failure (as then it will be > easy to check several code paths (like failure in fallback) Sure, let's get it under the IS_ENABLED(CONFIG_DRM_I915_DEBUG). > This patch was trying to keep definition of modparam unchanged (extra > -1 value should not be noticed by any existing use case) but since > it is purely debug feature I'm not against making bigger changes here. From my pov, it's very useful to define how you expect it to work, and how to make it more useful than drv_module_reload and to keep it working as you intend. A pure fault-counter doesn't seem very robust or intuitive for me when you are trying to develop a new fault point. Otoh, I don't mind if the developer has to fix all the earlier fails when testing his (those fails are all our responsibility) -- or at least file bugs so that awareness is raised. -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 0/3] drm/i915/dp: refactoring + respect vbt max dp rate
On Thu, Feb 01, 2018 at 11:03:40AM +, Jani Nikula wrote: > Hi Rodrigo, this is what I had in mind for the DP CNL and VBT rate limiting, I > hope you don't mind me writing the patches. It was easier to express myself > in C > than English. of course I don't mind. Thanks for the clean version. > > BR, > Jani. > > Cc: Ville Syrjälä> Cc: Rodrigo Vivi > Cc: Dhinakaran Pandiyan > > Jani Nikula (3): > drm/i915/dp: abstract rate array length limiting > drm/i915/dp: clean up source rate limiting for cnl > drm/i915/dp: limit DP link rate based on VBT on CNL+ > > drivers/gpu/drm/i915/i915_drv.h | 1 + > drivers/gpu/drm/i915/intel_bios.c | 21 > drivers/gpu/drm/i915/intel_dp.c | 63 > ++- > drivers/gpu/drm/i915/intel_vbt_defs.h | 5 +++ > 4 files changed, 67 insertions(+), 23 deletions(-) > > -- > 2.11.0 > ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/3] drm/i915/dp: limit DP link rate based on VBT on CNL+
On Thu, Feb 01, 2018 at 11:03:43AM +, Jani Nikula wrote: > We have the max DP link rate info available in VBT since BDB version > 216, included in child device config since commit c4fb60b9aba9 > ("drm/i915/bios: add DP max link rate to VBT child device > struct"). Parse it and use it. > > Cc: Ville Syrjälä> Cc: Rodrigo Vivi > Cc: Dhinakaran Pandiyan > Signed-off-by: Jani Nikula > --- > drivers/gpu/drm/i915/i915_drv.h | 1 + > drivers/gpu/drm/i915/intel_bios.c | 21 + > drivers/gpu/drm/i915/intel_dp.c | 9 - > drivers/gpu/drm/i915/intel_vbt_defs.h | 5 + > 4 files changed, 35 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index c676269ed843..26b91c25b8a0 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1282,6 +1282,7 @@ struct ddi_vbt_port_info { > > uint8_t dp_boost_level; > uint8_t hdmi_boost_level; > + int dp_max_link_rate; /* 0 for not limited by VBT */ > }; > > enum psr_lines_to_wait { > diff --git a/drivers/gpu/drm/i915/intel_bios.c > b/drivers/gpu/drm/i915/intel_bios.c > index cf3f8f1ba6f7..4e74aa2f16bc 100644 > --- a/drivers/gpu/drm/i915/intel_bios.c > +++ b/drivers/gpu/drm/i915/intel_bios.c > @@ -1274,6 +1274,27 @@ static void parse_ddi_port(struct drm_i915_private > *dev_priv, enum port port, > DRM_DEBUG_KMS("VBT HDMI boost level for port %c: %d\n", > port_name(port), info->hdmi_boost_level); > } > + > + /* DP max link rate for CNL+ */ > + if (bdb_version >= 216) { > + switch (child->dp_max_link_rate) { > + default: > + case VBT_DP_MAX_LINK_RATE_HBR3: > + info->dp_max_link_rate = 81; > + break; > + case VBT_DP_MAX_LINK_RATE_HBR2: > + info->dp_max_link_rate = 54; > + break; > + case VBT_DP_MAX_LINK_RATE_HBR: > + info->dp_max_link_rate = 27; > + break; > + case VBT_DP_MAX_LINK_RATE_LBR: > + info->dp_max_link_rate = 162000; > + break; oh! I was missing this conversion Reviewed-by: Rodrigo Vivi > + } > + DRM_DEBUG_KMS("VBT DP max link rate for port %c: %d\n", > + port_name(port), info->dp_max_link_rate); > + } > } > > static void parse_ddi_ports(struct drm_i915_private *dev_priv, u8 > bdb_version) > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 8bef858919c8..9a610d4783d8 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -270,8 +270,10 @@ intel_dp_set_source_rates(struct intel_dp *intel_dp) > { > struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp); > struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev); > + const struct ddi_vbt_port_info *info = > + _priv->vbt.ddi_port_info[dig_port->base.port]; > const int *source_rates; > - int size, max_rate = 0; > + int size, max_rate = 0, vbt_max_rate = info->dp_max_link_rate; > > /* This should only be done once */ > WARN_ON(intel_dp->source_rates || intel_dp->num_source_rates); > @@ -295,6 +297,11 @@ intel_dp_set_source_rates(struct intel_dp *intel_dp) > size = ARRAY_SIZE(default_rates) - 1; > } > > + if (max_rate && vbt_max_rate) > + max_rate = min(max_rate, vbt_max_rate); > + else if (vbt_max_rate) > + max_rate = vbt_max_rate; > + > if (max_rate) > size = intel_dp_rate_limit_len(source_rates, size, max_rate); > > diff --git a/drivers/gpu/drm/i915/intel_vbt_defs.h > b/drivers/gpu/drm/i915/intel_vbt_defs.h > index 3d3feee9b5dd..458468237b5f 100644 > --- a/drivers/gpu/drm/i915/intel_vbt_defs.h > +++ b/drivers/gpu/drm/i915/intel_vbt_defs.h > @@ -320,6 +320,11 @@ enum vbt_gmbus_ddi { > DDC_BUS_DDI_F, > }; > > +#define VBT_DP_MAX_LINK_RATE_HBR30 > +#define VBT_DP_MAX_LINK_RATE_HBR21 > +#define VBT_DP_MAX_LINK_RATE_HBR 2 > +#define VBT_DP_MAX_LINK_RATE_LBR 3 > + > /* > * The child device config, aka the display device data structure, provides a > * description of a port and its configuration on the platform. > -- > 2.11.0 > ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/3] drm/i915/dp: clean up source rate limiting for cnl
On Thu, Feb 01, 2018 at 11:03:42AM +, Jani Nikula wrote: > Make the limiting rate based instead of messing with the array size. > > Cc: Ville Syrjälä> Cc: Rodrigo Vivi > Cc: Dhinakaran Pandiyan > Signed-off-by: Jani Nikula Reviewed-by: Rodrigo Vivi > --- > drivers/gpu/drm/i915/intel_dp.c | 18 +++--- > 1 file changed, 11 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 3c1c11c1cd30..8bef858919c8 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -242,7 +242,7 @@ intel_dp_downstream_max_dotclock(struct intel_dp > *intel_dp) > return max_dotclk; > } > > -static int cnl_adjusted_max_rate(struct intel_dp *intel_dp, int size) > +static int cnl_max_source_rate(struct intel_dp *intel_dp) > { > struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp); > struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev); > @@ -252,17 +252,17 @@ static int cnl_adjusted_max_rate(struct intel_dp > *intel_dp, int size) > > /* Low voltage SKUs are limited to max of 5.4G */ > if (voltage == VOLTAGE_INFO_0_85V) > - return size - 2; > + return 54; > > /* For this SKU 8.1G is supported in all ports */ > if (IS_CNL_WITH_PORT_F(dev_priv)) > - return size; > + return 81; > > /* For other SKUs, max rate on ports A and B is 5.4G */ > if (port == PORT_A || port == PORT_D) > - return size - 2; > + return 54; > > - return size; > + return 81; > } > > static void > @@ -271,7 +271,7 @@ intel_dp_set_source_rates(struct intel_dp *intel_dp) > struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp); > struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev); > const int *source_rates; > - int size; > + int size, max_rate = 0; > > /* This should only be done once */ > WARN_ON(intel_dp->source_rates || intel_dp->num_source_rates); > @@ -281,7 +281,8 @@ intel_dp_set_source_rates(struct intel_dp *intel_dp) > size = ARRAY_SIZE(bxt_rates); > } else if (IS_CANNONLAKE(dev_priv)) { > source_rates = cnl_rates; > - size = cnl_adjusted_max_rate(intel_dp, ARRAY_SIZE(cnl_rates)); > + size = ARRAY_SIZE(cnl_rates); > + max_rate = cnl_max_source_rate(intel_dp); > } else if (IS_GEN9_BC(dev_priv)) { > source_rates = skl_rates; > size = ARRAY_SIZE(skl_rates); > @@ -294,6 +295,9 @@ intel_dp_set_source_rates(struct intel_dp *intel_dp) > size = ARRAY_SIZE(default_rates) - 1; > } > > + if (max_rate) > + size = intel_dp_rate_limit_len(source_rates, size, max_rate); > + > intel_dp->source_rates = source_rates; > intel_dp->num_source_rates = size; > } > -- > 2.11.0 > ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/3] drm/i915/dp: abstract rate array length limiting
On Thu, Feb 01, 2018 at 11:03:41AM +, Jani Nikula wrote: > This will be useful later on. Also move the functions around to not need > forward declarations in subsequent patches. No functional changes. > > Cc: Ville Syrjälä> Cc: Rodrigo Vivi > Cc: Dhinakaran Pandiyan > Signed-off-by: Jani Nikula Reviewed-by: Rodrigo Vivi > --- > drivers/gpu/drm/i915/intel_dp.c | 38 ++ > 1 file changed, 22 insertions(+), 16 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 03d86ff9b805..3c1c11c1cd30 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -157,6 +157,28 @@ static void intel_dp_set_sink_rates(struct intel_dp > *intel_dp) > intel_dp->num_sink_rates = i; > } > > +/* Get length of rates array potentially limited by max_rate. */ > +static int intel_dp_rate_limit_len(const int *rates, int len, int max_rate) > +{ > + int i; > + > + /* Limit results by potentially reduced max rate */ > + for (i = 0; i < len; i++) { > + if (rates[len - i - 1] <= max_rate) > + return len - i; > + } > + > + return 0; > +} > + > +/* Get length of common rates array potentially limited by max_rate. */ > +static int intel_dp_common_len_rate_limit(const struct intel_dp *intel_dp, > + int max_rate) > +{ > + return intel_dp_rate_limit_len(intel_dp->common_rates, > +intel_dp->num_common_rates, max_rate); > +} > + > /* Theoretical max between source and sink */ > static int intel_dp_max_common_rate(struct intel_dp *intel_dp) > { > @@ -328,22 +350,6 @@ static void intel_dp_set_common_rates(struct intel_dp > *intel_dp) > } > } > > -/* get length of common rates potentially limited by max_rate */ > -static int intel_dp_common_len_rate_limit(struct intel_dp *intel_dp, > - int max_rate) > -{ > - const int *common_rates = intel_dp->common_rates; > - int i, common_len = intel_dp->num_common_rates; > - > - /* Limit results by potentially reduced max rate */ > - for (i = 0; i < common_len; i++) { > - if (common_rates[common_len - i - 1] <= max_rate) > - return common_len - i; > - } > - > - return 0; > -} > - > static bool intel_dp_link_params_valid(struct intel_dp *intel_dp, int > link_rate, > uint8_t lane_count) > { > -- > 2.11.0 > ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/5] drm/i915/guc: Use correct error code for GuC initialization failure
On 2/1/2018 7:53 PM, Michal Wajdeczko wrote: On Thu, 01 Feb 2018 13:35:15 +0100, Chris Wilsonwrote: Quoting Sagar Arun Kamble (2018-02-01 11:48:54) On 1/31/2018 11:02 PM, Michal Wajdeczko wrote: > Since commit 6ca9a2beb5 ("drm/i915: Unwind i915_gem_init() failure") > we believed that we correctly handle all errors encountered during > GuC initialization, including special one that indicates request to > run driver with disabled GPU submission (-EIO). > > Unfortunately since commit 121981fafe ("drm/i915/guc: Combine > enable_guc_loading|submission modparams") we stopped using that > error code to avoid unwanted fallback to execlist submission mode. > > In result any GuC initialization failure was treated as non-recoverable > error leading to driver load abort, so we could not even read related > GuC error log to investigate cause of the problem. > > Fix that by always returning -EIO on uC hardware related failure. > > Signed-off-by: Michal Wajdeczko > Cc: Chris Wilson > Cc: Michal Winiarski > Cc: Daniele Ceraolo Spurio > Cc: Sagar Arun Kamble > --- > drivers/gpu/drm/i915/intel_uc.c | 5 + > 1 file changed, 1 insertion(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c > index 3a13cbb..1547e16 100644 > --- a/drivers/gpu/drm/i915/intel_uc.c > +++ b/drivers/gpu/drm/i915/intel_uc.c > @@ -432,11 +432,8 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv) > * Note that there is no fallback as either user explicitly asked for > * the GuC or driver default option was to run with the GuC enabled. > */ > - if (GEM_WARN_ON(ret == -EIO)) > - ret = -EINVAL; > - > dev_err(dev_priv->drm.dev, "GuC initialization failed %d\n", ret); > - return ret; > + return -EIO; /* We want to disable GPU submission but keep KMS alive */ > } We'll need to avoid uc_fini_hw/ in unload path since GPU will be marked wedged after this? We should be avoiding uc_fini_hw in unload because we haven't completed uc_init_hw. Such failure should already be handled? It's even worst. In case of uc_init_hw() failure we always call uc_fini() and uc_fini_misc() regardless of EIO, and then in case of running in wedged more we will try to call them again in unload path ... While it is easy to postpone uc_fini[_misc] calls to unload stage, it is tricky to avoid calling uc_fini_hw there without adding extra check (earlier there was discussion that we should not have any conditions in _fini functions) Any ideas how to solve that ? Earlier we could rely on status of enable_guc_loading/submission. Now I think we need to have status per uc, uc_misc, uc_hw as many internal steps can't rely on like pointer check. Michal -- Thanks, Sagar ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/5] drm/i915/uc: Add few more load failure checkpoints
Thanks for clarification. On 2/1/2018 7:57 PM, Michal Wajdeczko wrote: On Thu, 01 Feb 2018 12:43:29 +0100, Sagar Arun Kamblewrote: On 1/31/2018 11:02 PM, Michal Wajdeczko wrote: Additional load failure checkpoints added into uC initialization sequence should help us verify correctness of error handling. Signed-off-by: Michal Wajdeczko Cc: Chris Wilson Reviewed-by: Sagar Arun Kamble --- drivers/gpu/drm/i915/intel_uc.c | 11 +++ 1 file changed, 11 insertions(+) diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c index e3f3509..2f80cab 100644 --- a/drivers/gpu/drm/i915/intel_uc.c +++ b/drivers/gpu/drm/i915/intel_uc.c @@ -287,6 +287,9 @@ int intel_uc_init(struct drm_i915_private *dev_priv) if (!HAS_GUC(dev_priv)) return -ENODEV; + if (i915_inject_load_failure()) + return -ENODEV; + ret = intel_guc_init(guc); if (ret) return ret; @@ -330,6 +333,11 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv) if (!USES_GUC(dev_priv)) return 0; + if (i915_inject_load_failure()) { + ret = -ECANCELED; In patch 3 we will be returning -EIO always from this function so just goto here "ret" will be uninitialized, but it is used there and below? well, I wanted to include path that captures GuC log ... + goto err_out; + } + GEM_BUG_ON(!HAS_GUC(dev_priv)); guc_disable_communication(guc); @@ -371,6 +379,9 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv) "retry %d more time(s)\n", ret, attempts); } + if (i915_inject_load_failure()) + ret = -ECANCELED; + /* Did we succeded or run out of retries? */ if (ret) goto err_log_capture; -- Thanks, Sagar ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Add option to list load failure checkpoints
On Wed, 31 Jan 2018 21:48:42 +0100, Chris Wilsonwrote: Quoting Michal Wajdeczko (2018-01-31 18:23:47) Our inject_load_failure functionality allows to insert one failure during driver load, but it is hard to guess which number should passed as modparam to select specific checkpoint. Use negative number as option to list all available failure checkpoints without triggering any failure. Hmm, it was only intended for use with the coupled igt test. Mind expanding upon the use case you have? Could you not use that iterative search for finding the injection value you want for repeated runs? For the bisect case, do you not want to keep it iterating over all in case the value changes? How stable do you want the modparam? Iterative approach is good for validation team to verify that all existing failure points are correctly handled (ie. we don't cause crash/panic), but it is less useful when you are interested in adding new checkpoints just for your code, as it requires both time and luck as you may be hit by earlier checkpoint that no longer works ;) Btw, IMHO this modparam should only be exposed in DEBUG config (as it introduces some code and text), and maybe we should also consider extending it to support more than one failure (as then it will be easy to check several code paths (like failure in fallback) This patch was trying to keep definition of modparam unchanged (extra -1 value should not be noticed by any existing use case) but since it is purely debug feature I'm not against making bigger changes here. Michal ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] ✓ Fi.CI.IGT: success for drm/i915: Less verbose engine startup
== Series Details == Series: drm/i915: Less verbose engine startup URL : https://patchwork.freedesktop.org/series/37476/ State : success == Summary == Test kms_cursor_legacy: Subgroup flip-vs-cursor-crc-atomic: pass -> FAIL (shard-apl) fdo#102670 Test perf: Subgroup buffer-fill: fail -> PASS (shard-apl) fdo#103755 Subgroup oa-exponents: fail -> PASS (shard-apl) fdo#102254 Test kms_cursor_crc: Subgroup cursor-128x128-suspend: pass -> SKIP (shard-snb) fdo#103880 Test kms_sysfs_edid_timing: pass -> WARN (shard-apl) fdo#100047 Test gem_exec_schedule: Subgroup preempt-other-vebox: pass -> FAIL (shard-apl) fdo#102848 Test kms_frontbuffer_tracking: Subgroup fbc-1p-primscrn-pri-indfb-draw-render: pass -> FAIL (shard-snb) fdo#101623 Test kms_flip: Subgroup 2x-flip-vs-expired-vblank-interruptible: pass -> FAIL (shard-hsw) fdo#102887 Test gem_eio: Subgroup in-flight-contexts: pass -> DMESG-WARN (shard-snb) fdo#104058 fdo#102670 https://bugs.freedesktop.org/show_bug.cgi?id=102670 fdo#103755 https://bugs.freedesktop.org/show_bug.cgi?id=103755 fdo#102254 https://bugs.freedesktop.org/show_bug.cgi?id=102254 fdo#103880 https://bugs.freedesktop.org/show_bug.cgi?id=103880 fdo#100047 https://bugs.freedesktop.org/show_bug.cgi?id=100047 fdo#102848 https://bugs.freedesktop.org/show_bug.cgi?id=102848 fdo#101623 https://bugs.freedesktop.org/show_bug.cgi?id=101623 fdo#102887 https://bugs.freedesktop.org/show_bug.cgi?id=102887 fdo#104058 https://bugs.freedesktop.org/show_bug.cgi?id=104058 shard-apltotal:2838 pass:1748 dwarn:1 dfail:0 fail:24 skip:1064 time:12715s shard-hswtotal:2838 pass:1734 dwarn:1 dfail:0 fail:12 skip:1090 time:11990s shard-snbtotal:2838 pass:1327 dwarn:2 dfail:0 fail:11 skip:1498 time:6580s Blacklisted hosts: shard-kbltotal:2817 pass:1856 dwarn:1 dfail:0 fail:22 skip:937 time:9472s == Logs == For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7851/shards.html ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/atomic: Fix memleak on ERESTARTSYS during non-blocking commits
On 2018-02-01 05:30 AM, Maarten Lankhorst wrote: > Op 31-01-18 om 20:57 schreef Harry Wentland: >> On 2018-01-30 05:28 AM, Maarten Lankhorst wrote: >>> Op 29-01-18 om 16:41 schreef Leo Li: Updated IGT results seem sane: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7698/shards.html Would someone be able to apply this patch? >>> Thanks for the reminder, pushed. >>> >> Thanks, Maarten. I see it in drm-misc-next. >> >> Would someone be able to pull this into drm-misc-fixes as well, or can I >> just I apply this myself with the following dim commands? >> >> dim checkout drm-misc-fixes >> dim cherry-pick 1c6c6ebb >> dim push-branch > My bad, pushed to the right branch. :) > Thanks, Maarten. Harry ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] ✓ Fi.CI.BAT: success for series starting with [1/6] disable-gem-trace (rev2)
== Series Details == Series: series starting with [1/6] disable-gem-trace (rev2) URL : https://patchwork.freedesktop.org/series/37473/ State : success == Summary == Series 37473v2 series starting with [1/6] disable-gem-trace https://patchwork.freedesktop.org/api/1.0/series/37473/revisions/2/mbox/ Test gem_mmap_gtt: Subgroup basic-small-bo-tiledx: fail -> PASS (fi-gdg-551) fdo#102575 fdo#102575 https://bugs.freedesktop.org/show_bug.cgi?id=102575 fi-bdw-5557u total:288 pass:267 dwarn:0 dfail:0 fail:0 skip:21 time:419s fi-bdw-gvtdvmtotal:288 pass:264 dwarn:0 dfail:0 fail:0 skip:24 time:422s fi-blb-e6850 total:288 pass:223 dwarn:1 dfail:0 fail:0 skip:64 time:372s fi-bsw-n3050 total:288 pass:242 dwarn:0 dfail:0 fail:0 skip:46 time:492s fi-bwr-2160 total:288 pass:183 dwarn:0 dfail:0 fail:0 skip:105 time:282s fi-bxt-dsi total:288 pass:258 dwarn:0 dfail:0 fail:0 skip:30 time:485s fi-bxt-j4205 total:288 pass:259 dwarn:0 dfail:0 fail:0 skip:29 time:486s fi-byt-j1900 total:288 pass:253 dwarn:0 dfail:0 fail:0 skip:35 time:471s fi-byt-n2820 total:288 pass:249 dwarn:0 dfail:0 fail:0 skip:39 time:455s fi-cfl-s2total:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:575s fi-elk-e7500 total:288 pass:229 dwarn:0 dfail:0 fail:0 skip:59 time:415s fi-gdg-551 total:288 pass:180 dwarn:0 dfail:0 fail:0 skip:108 time:280s fi-glk-1 total:288 pass:260 dwarn:0 dfail:0 fail:0 skip:28 time:511s fi-hsw-4770 total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:389s fi-hsw-4770r total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:397s fi-ilk-650 total:288 pass:228 dwarn:0 dfail:0 fail:0 skip:60 time:414s fi-ivb-3520m total:288 pass:259 dwarn:0 dfail:0 fail:0 skip:29 time:457s fi-ivb-3770 total:288 pass:255 dwarn:0 dfail:0 fail:0 skip:33 time:414s fi-kbl-7500u total:288 pass:263 dwarn:1 dfail:0 fail:0 skip:24 time:456s fi-kbl-7560u total:288 pass:269 dwarn:0 dfail:0 fail:0 skip:19 time:498s fi-kbl-7567u total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:458s fi-kbl-r total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:500s fi-pnv-d510 total:288 pass:222 dwarn:1 dfail:0 fail:0 skip:65 time:580s fi-skl-6260u total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:426s fi-skl-6600u total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:507s fi-skl-6700hqtotal:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:531s fi-skl-6700k2total:288 pass:264 dwarn:0 dfail:0 fail:0 skip:24 time:488s fi-skl-6770hqtotal:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:474s fi-skl-guc total:288 pass:260 dwarn:0 dfail:0 fail:0 skip:28 time:415s fi-skl-gvtdvmtotal:288 pass:265 dwarn:0 dfail:0 fail:0 skip:23 time:432s fi-snb-2520m total:288 pass:248 dwarn:0 dfail:0 fail:0 skip:40 time:529s fi-snb-2600 total:288 pass:248 dwarn:0 dfail:0 fail:0 skip:40 time:398s Blacklisted hosts: fi-glk-dsi total:288 pass:258 dwarn:0 dfail:0 fail:0 skip:30 time:477s 4244f989341c83c90d1fdcc35b8cb6785fc57ef6 drm-tip: 2018y-02m-01d-12h-39m-55s UTC integration manifest 65d8831b2ae7 drm/i915: Skip post-reset request emission if the engine is not idle 8eb0cb9b8a1d drm/i915/execlists: Move the reset bits to a more natural home 8c7b0eb0981e drm/i915/selftests: Use a sacrificial context for hang testing 7a10dcdd208a drm/i915: Remove unbannable context spam from reset ede20d30f2ac drm/i915/execlists: Remove the startup spam 6af6741fe22a disable-gem-trace == Logs == For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7852/issues.html ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/5] drm/i915/uc: Add few more load failure checkpoints
On Thu, 01 Feb 2018 12:43:29 +0100, Sagar Arun Kamblewrote: On 1/31/2018 11:02 PM, Michal Wajdeczko wrote: Additional load failure checkpoints added into uC initialization sequence should help us verify correctness of error handling. Signed-off-by: Michal Wajdeczko Cc: Chris Wilson --- drivers/gpu/drm/i915/intel_uc.c | 11 +++ 1 file changed, 11 insertions(+) diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c index e3f3509..2f80cab 100644 --- a/drivers/gpu/drm/i915/intel_uc.c +++ b/drivers/gpu/drm/i915/intel_uc.c @@ -287,6 +287,9 @@ int intel_uc_init(struct drm_i915_private *dev_priv) if (!HAS_GUC(dev_priv)) return -ENODEV; + if (i915_inject_load_failure()) + return -ENODEV; + ret = intel_guc_init(guc); if (ret) return ret; @@ -330,6 +333,11 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv) if (!USES_GUC(dev_priv)) return 0; + if (i915_inject_load_failure()) { + ret = -ECANCELED; In patch 3 we will be returning -EIO always from this function so just goto here "ret" will be uninitialized, but it is used there and below? well, I wanted to include path that captures GuC log ... + goto err_out; + } + GEM_BUG_ON(!HAS_GUC(dev_priv)); guc_disable_communication(guc); @@ -371,6 +379,9 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv) "retry %d more time(s)\n", ret, attempts); } + if (i915_inject_load_failure()) + ret = -ECANCELED; + /* Did we succeded or run out of retries? */ if (ret) goto err_log_capture; ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/5] drm/i915/guc: Use correct error code for GuC initialization failure
On Thu, 01 Feb 2018 13:35:15 +0100, Chris Wilsonwrote: Quoting Sagar Arun Kamble (2018-02-01 11:48:54) On 1/31/2018 11:02 PM, Michal Wajdeczko wrote: > Since commit 6ca9a2beb5 ("drm/i915: Unwind i915_gem_init() failure") > we believed that we correctly handle all errors encountered during > GuC initialization, including special one that indicates request to > run driver with disabled GPU submission (-EIO). > > Unfortunately since commit 121981fafe ("drm/i915/guc: Combine > enable_guc_loading|submission modparams") we stopped using that > error code to avoid unwanted fallback to execlist submission mode. > > In result any GuC initialization failure was treated as non-recoverable > error leading to driver load abort, so we could not even read related > GuC error log to investigate cause of the problem. > > Fix that by always returning -EIO on uC hardware related failure. > > Signed-off-by: Michal Wajdeczko > Cc: Chris Wilson > Cc: Michal Winiarski > Cc: Daniele Ceraolo Spurio > Cc: Sagar Arun Kamble > --- > drivers/gpu/drm/i915/intel_uc.c | 5 + > 1 file changed, 1 insertion(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c > index 3a13cbb..1547e16 100644 > --- a/drivers/gpu/drm/i915/intel_uc.c > +++ b/drivers/gpu/drm/i915/intel_uc.c > @@ -432,11 +432,8 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv) >* Note that there is no fallback as either user explicitly asked for >* the GuC or driver default option was to run with the GuC enabled. >*/ > - if (GEM_WARN_ON(ret == -EIO)) > - ret = -EINVAL; > - > dev_err(dev_priv->drm.dev, "GuC initialization failed %d\n", ret); > - return ret; > + return -EIO; /* We want to disable GPU submission but keep KMS alive */ > } We'll need to avoid uc_fini_hw/ in unload path since GPU will be marked wedged after this? We should be avoiding uc_fini_hw in unload because we haven't completed uc_init_hw. Such failure should already be handled? It's even worst. In case of uc_init_hw() failure we always call uc_fini() and uc_fini_misc() regardless of EIO, and then in case of running in wedged more we will try to call them again in unload path ... While it is easy to postpone uc_fini[_misc] calls to unload stage, it is tricky to avoid calling uc_fini_hw there without adding extra check (earlier there was discussion that we should not have any conditions in _fini functions) Any ideas how to solve that ? Michal ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] ✓ Fi.CI.IGT: success for drm/i915/dp: refactoring + respect vbt max dp rate
== Series Details == Series: drm/i915/dp: refactoring + respect vbt max dp rate URL : https://patchwork.freedesktop.org/series/37475/ State : success == Summary == Test gem_exec_schedule: Subgroup preempt-other-vebox: pass -> FAIL (shard-apl) fdo#102848 Test kms_flip: Subgroup plain-flip-fb-recreate: pass -> FAIL (shard-hsw) fdo#100368 Test kms_cursor_crc: Subgroup cursor-128x128-suspend: pass -> SKIP (shard-snb) fdo#103880 Test perf: Subgroup buffer-fill: fail -> PASS (shard-apl) fdo#103755 Test pm_rps: Subgroup waitboost: pass -> FAIL (shard-apl) fdo#102250 fdo#102848 https://bugs.freedesktop.org/show_bug.cgi?id=102848 fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368 fdo#103880 https://bugs.freedesktop.org/show_bug.cgi?id=103880 fdo#103755 https://bugs.freedesktop.org/show_bug.cgi?id=103755 fdo#102250 https://bugs.freedesktop.org/show_bug.cgi?id=102250 shard-apltotal:2838 pass:1748 dwarn:1 dfail:0 fail:25 skip:1064 time:12638s shard-hswtotal:2838 pass:1734 dwarn:1 dfail:0 fail:12 skip:1090 time:11965s shard-snbtotal:2838 pass:1329 dwarn:1 dfail:0 fail:10 skip:1498 time:6527s Blacklisted hosts: shard-kbltotal:2817 pass:1845 dwarn:12 dfail:1 fail:23 skip:935 time:9496s == Logs == For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7850/shards.html ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v2] drm/i915/selftests: Use a sacrificial context for hang testing
Avoid injecting hangs in to the i915->kernel_context in case the GPU reset leaves corruption in the context image in its wake (leading to continual failures and system hangs after the selftests are ostensibly complete). Use a sacrificial kernel_context instead. v2: Closing a context is tricky; export a function (for selftests) from i915_gem_context.c to get it right. Signed-off-by: Chris WilsonCc: Mika Kuoppala Cc: Michel Thierry i915 = i915; + h->ctx = kernel_context(i915); + if (IS_ERR(h->ctx)) + return PTR_ERR(h->ctx); + h->hws = i915_gem_object_create_internal(i915, PAGE_SIZE); - if (IS_ERR(h->hws)) - return PTR_ERR(h->hws); + if (IS_ERR(h->hws)) { + err = PTR_ERR(h->hws); + goto err_ctx; + } h->obj = i915_gem_object_create_internal(i915, PAGE_SIZE); if (IS_ERR(h->obj)) { @@ -79,6 +86,8 @@ static int hang_init(struct hang *h, struct drm_i915_private *i915) i915_gem_object_put(h->obj); err_hws: i915_gem_object_put(h->hws); +err_ctx: + kernel_context_close(h->ctx); return err; } @@ -196,9 +205,7 @@ static int emit_recurse_batch(struct hang *h, } static struct drm_i915_gem_request * -hang_create_request(struct hang *h, - struct intel_engine_cs *engine, - struct i915_gem_context *ctx) +hang_create_request(struct hang *h, struct intel_engine_cs *engine) { struct drm_i915_gem_request *rq; int err; @@ -225,7 +232,7 @@ hang_create_request(struct hang *h, h->batch = vaddr; } - rq = i915_gem_request_alloc(engine, ctx); + rq = i915_gem_request_alloc(engine, h->ctx); if (IS_ERR(rq)) return rq; @@ -255,6 +262,8 @@ static void hang_fini(struct hang *h) i915_gem_object_unpin_map(h->hws); i915_gem_object_put(h->hws); + kernel_context_close(h->ctx); + i915_gem_wait_for_idle(h->i915, I915_WAIT_LOCKED); } @@ -290,7 +299,7 @@ static int igt_hang_sanitycheck(void *arg) if (!intel_engine_can_store_dword(engine)) continue; - rq = hang_create_request(, engine, i915->kernel_context); + rq = hang_create_request(, engine); if (IS_ERR(rq)) { err = PTR_ERR(rq); pr_err("Failed to create request for %s, err=%d\n", @@ -427,8 +436,7 @@ static int __igt_reset_engine(struct drm_i915_private *i915, bool active) struct drm_i915_gem_request *rq; mutex_lock(>drm.struct_mutex); - rq = hang_create_request(, engine, -i915->kernel_context); + rq = hang_create_request(, engine); if (IS_ERR(rq)) { err = PTR_ERR(rq); mutex_unlock(>drm.struct_mutex); @@ -633,8 +641,7 @@ static int __igt_reset_engine_others(struct drm_i915_private *i915, struct drm_i915_gem_request *rq; mutex_lock(>drm.struct_mutex); - rq = hang_create_request(, engine, -i915->kernel_context); + rq = hang_create_request(, engine); if (IS_ERR(rq)) { err = PTR_ERR(rq); mutex_unlock(>drm.struct_mutex); @@ -787,7 +794,7 @@ static int igt_wait_reset(void *arg) if (err) goto unlock; - rq = hang_create_request(, i915->engine[RCS], i915->kernel_context); + rq = hang_create_request(, i915->engine[RCS]); if (IS_ERR(rq)) { err = PTR_ERR(rq); goto
Re: [Intel-gfx] [RFC] drm/i915/pmu: Micro-optimize sampling loop
On 31/01/2018 16:07, Chris Wilson wrote: Quoting Tvrtko Ursulin (2018-01-31 16:02:38) From: Tvrtko UrsulinBy carefully chosing slightly unsynchronized values for timer frequency and period, we can remove the multiplications from the sampling loop and replace them with shifts only. Downside is that the counter read callback now has to do a divide with a non-power-of-two, but the rationale is that sampling loop which runs at ~200 Hz, multiplied by number of engines, is more important than less frequent counter read-out. Furthermore, the divide in counter read-out seems to be optimized by the compiler to some shifts and multiply. We can only do all this at the expense of introducing a systematic error to the sampling counters to the amount of 0.18%. At the same time we are increasing the sampling rate from 200 to 238 Hz which may cancel some of this out. Signed-off-by: Tvrtko Ursulin Cc: Chris Wilson --- Motivated by Chris' insisting to use power-of-two scale factor in the engine queue depth PMU series. I was actually reluctant to give in there so this is even more questionable. :) I have no idea if it is cheaper to run the sampling timer at 200Hz with some multiplies, or run it at 238Hz with only shifts. I can't say I have any insight into the timer wheel or hrtimer either. Let's watch. I got 0.35% vs 0.19% CPU usage for i915_sample, tip vs this patch. But in general it seems to fluctuate a lot so I don't really believe those numbers. FWIW 0.18% systematic error at least doesn't sound like a big deal when sampling counters are concerned. Yeah, since even pinning it down to a 5% systematic error is hard, but we'll see if this 0.18% is the straw that breaks CI back! It passed but I don't think we should be tempted, unless someone can prove it is really worth it. I'll just forget about it for time being. Regards, Tvrtko ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] ✗ Fi.CI.IGT: warning for series starting with [1/6] disable-gem-trace
== Series Details == Series: series starting with [1/6] disable-gem-trace URL : https://patchwork.freedesktop.org/series/37473/ State : warning == Summary == Test gem_eio: Subgroup in-flight-contexts: fail -> PASS (shard-hsw) fdo#104676 Subgroup in-flight: pass -> DMESG-WARN (shard-snb) fdo#104058 Test perf: Subgroup blocking: pass -> FAIL (shard-hsw) fdo#102252 Subgroup oa-exponents: fail -> PASS (shard-apl) fdo#102254 Subgroup buffer-fill: fail -> PASS (shard-apl) fdo#103755 Test drv_selftest: Subgroup live_hangcheck: pass -> INCOMPLETE (shard-apl) fdo#104262 Test kms_flip: Subgroup flip-vs-absolute-wf_vblank: pass -> FAIL (shard-snb) fdo#100368 Test kms_cursor_crc: Subgroup cursor-128x128-suspend: pass -> SKIP (shard-snb) fdo#103880 Test kms_frontbuffer_tracking: Subgroup fbc-1p-primscrn-cur-indfb-onoff: pass -> SKIP (shard-snb) fdo#103167 +1 Test kms_atomic: Subgroup plane_invalid_params_fence: pass -> SKIP (shard-snb) fdo#104676 https://bugs.freedesktop.org/show_bug.cgi?id=104676 fdo#104058 https://bugs.freedesktop.org/show_bug.cgi?id=104058 fdo#102252 https://bugs.freedesktop.org/show_bug.cgi?id=102252 fdo#102254 https://bugs.freedesktop.org/show_bug.cgi?id=102254 fdo#103755 https://bugs.freedesktop.org/show_bug.cgi?id=103755 fdo#104262 https://bugs.freedesktop.org/show_bug.cgi?id=104262 fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368 fdo#103880 https://bugs.freedesktop.org/show_bug.cgi?id=103880 fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167 shard-apltotal:2820 pass:1732 dwarn:1 dfail:0 fail:22 skip:1064 time:12269s shard-hswtotal:2838 pass:1735 dwarn:1 dfail:0 fail:11 skip:1090 time:12054s shard-snbtotal:2838 pass:1324 dwarn:2 dfail:0 fail:11 skip:1501 time:6460s == Logs == For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7849/shards.html ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/7] drm/i915: Don't set cursor pipe select bits on g4x+
On Tue, 2018-01-30 at 22:38 +0200, Ville Syrjala wrote: > From: Ville Syrjälä> > G4x cursor control registers still allow us to write to the pipe > select > bits even though cursors are supposed to be fixed to a specific pipe. > Bspec tells us that we should only ever write 0 to these bits. Let's > follow that recommendation. On ilk+ the bits become hardwired to 0. > > Also looks like ICL repurposes these bits for some other use, so > we had better stop setting them to bogus values there. > Reviewed-by: Mika Kahola > Signed-off-by: Ville Syrjälä > --- > drivers/gpu/drm/i915/intel_display.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c > index af659d25943b..1126f1d5 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -9532,7 +9532,8 @@ static u32 i9xx_cursor_ctl(const struct > intel_crtc_state *crtc_state, > if (HAS_DDI(dev_priv)) > cntl |= CURSOR_PIPE_CSC_ENABLE; > > - cntl |= MCURSOR_PIPE_SELECT(crtc->pipe); > + if (INTEL_GEN(dev_priv) < 5 && !IS_G4X(dev_priv)) > + cntl |= MCURSOR_PIPE_SELECT(crtc->pipe); > > switch (plane_state->base.crtc_w) { > case 64: -- Mika Kahola - Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/3] drm/i915/dp: limit DP link rate based on VBT on CNL+
On Thu, Feb 01, 2018 at 01:03:43PM +0200, Jani Nikula wrote: > We have the max DP link rate info available in VBT since BDB version > 216, included in child device config since commit c4fb60b9aba9 > ("drm/i915/bios: add DP max link rate to VBT child device > struct"). Parse it and use it. > > Cc: Ville Syrjälä> Cc: Rodrigo Vivi > Cc: Dhinakaran Pandiyan > Signed-off-by: Jani Nikula > --- > drivers/gpu/drm/i915/i915_drv.h | 1 + > drivers/gpu/drm/i915/intel_bios.c | 21 + > drivers/gpu/drm/i915/intel_dp.c | 9 - > drivers/gpu/drm/i915/intel_vbt_defs.h | 5 + > 4 files changed, 35 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index c676269ed843..26b91c25b8a0 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1282,6 +1282,7 @@ struct ddi_vbt_port_info { > > uint8_t dp_boost_level; > uint8_t hdmi_boost_level; > + int dp_max_link_rate; /* 0 for not limited by VBT */ > }; > > enum psr_lines_to_wait { > diff --git a/drivers/gpu/drm/i915/intel_bios.c > b/drivers/gpu/drm/i915/intel_bios.c > index cf3f8f1ba6f7..4e74aa2f16bc 100644 > --- a/drivers/gpu/drm/i915/intel_bios.c > +++ b/drivers/gpu/drm/i915/intel_bios.c > @@ -1274,6 +1274,27 @@ static void parse_ddi_port(struct drm_i915_private > *dev_priv, enum port port, > DRM_DEBUG_KMS("VBT HDMI boost level for port %c: %d\n", > port_name(port), info->hdmi_boost_level); > } > + > + /* DP max link rate for CNL+ */ > + if (bdb_version >= 216) { > + switch (child->dp_max_link_rate) { > + default: > + case VBT_DP_MAX_LINK_RATE_HBR3: > + info->dp_max_link_rate = 81; > + break; > + case VBT_DP_MAX_LINK_RATE_HBR2: > + info->dp_max_link_rate = 54; > + break; > + case VBT_DP_MAX_LINK_RATE_HBR: > + info->dp_max_link_rate = 27; > + break; > + case VBT_DP_MAX_LINK_RATE_LBR: > + info->dp_max_link_rate = 162000; > + break; > + } > + DRM_DEBUG_KMS("VBT DP max link rate for port %c: %d\n", > + port_name(port), info->dp_max_link_rate); > + } > } > > static void parse_ddi_ports(struct drm_i915_private *dev_priv, u8 > bdb_version) > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 8bef858919c8..9a610d4783d8 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -270,8 +270,10 @@ intel_dp_set_source_rates(struct intel_dp *intel_dp) > { > struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp); > struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev); > + const struct ddi_vbt_port_info *info = > + _priv->vbt.ddi_port_info[dig_port->base.port]; > const int *source_rates; > - int size, max_rate = 0; > + int size, max_rate = 0, vbt_max_rate = info->dp_max_link_rate; > > /* This should only be done once */ > WARN_ON(intel_dp->source_rates || intel_dp->num_source_rates); > @@ -295,6 +297,11 @@ intel_dp_set_source_rates(struct intel_dp *intel_dp) > size = ARRAY_SIZE(default_rates) - 1; > } > > + if (max_rate && vbt_max_rate) > + max_rate = min(max_rate, vbt_max_rate); > + else if (vbt_max_rate) > + max_rate = vbt_max_rate; I kinda wish this would look exactly like the hdmi version, but can't do that without populating max_rate for every platform. So I guess we'll go with this. Series looks good to me: Reviewed-by: Ville Syrjälä > + > if (max_rate) > size = intel_dp_rate_limit_len(source_rates, size, max_rate); > > diff --git a/drivers/gpu/drm/i915/intel_vbt_defs.h > b/drivers/gpu/drm/i915/intel_vbt_defs.h > index 3d3feee9b5dd..458468237b5f 100644 > --- a/drivers/gpu/drm/i915/intel_vbt_defs.h > +++ b/drivers/gpu/drm/i915/intel_vbt_defs.h > @@ -320,6 +320,11 @@ enum vbt_gmbus_ddi { > DDC_BUS_DDI_F, > }; > > +#define VBT_DP_MAX_LINK_RATE_HBR30 > +#define VBT_DP_MAX_LINK_RATE_HBR21 > +#define VBT_DP_MAX_LINK_RATE_HBR 2 > +#define VBT_DP_MAX_LINK_RATE_LBR 3 > + > /* > * The child device config, aka the display device data structure, provides a > * description of a port and its configuration on the platform. > -- > 2.11.0 -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/5] drm/i915/guc: Don't try to create log runtime if there is no log
Quoting Michal Wajdeczko (2018-01-31 17:32:39) > In case of GuC initialization failure we may continue with driver > load, but we wrongly assume that GuC is fully functional. This > leads to the BUG as we attempt to access non-existing log vma. > > [26386.121085] BUG: unable to handle kernel NULL pointer dereference at > 00a0 > [26386.121225] IP: guc_log_runtime_create+0x23/0xe0 [i915] > [26386.121763] Call Trace: > [26386.121870] guc_log_late_setup+0xfd/0x140 [i915] > [26386.121969] i915_driver_load+0x7ab/0x1730 [i915] > [26386.122069] i915_pci_probe+0x2d/0x90 [i915] > [26386.122089] pci_device_probe+0x9c/0x120 > [26386.122107] driver_probe_device+0x2a9/0x490 > [26386.122126] __driver_attach+0xd9/0xe0 > [26386.122143] ? driver_probe_device+0x490/0x490 > [26386.122158] bus_for_each_dev+0x57/0x90 > [26386.122175] bus_add_driver+0x1eb/0x260 > [26386.122190] ? 0xa069a000 > [26386.122206] driver_register+0x52/0xc0 > [26386.10] ? 0xa069a000 > [26386.122234] do_one_initcall+0x39/0x170 > [26386.122252] ? kmem_cache_alloc_trace+0x1fd/0x2e0 > [26386.122273] do_init_module+0x56/0x1ec > [26386.122289] load_module+0x219e/0x2550 > [26386.122309] ? vfs_read+0x121/0x140 > [26386.122331] ? SyS_finit_module+0xa5/0xe0 > [26386.122346] SyS_finit_module+0xa5/0xe0 > [26386.122371] entry_SYSCALL_64_fastpath+0x22/0x8f > > Signed-off-by: Michal Wajdeczko> Cc: Chris Wilson > Cc: Michal Winiarski > Cc: Daniele Ceraolo Spurio > Cc: Sagar Arun Kamble > --- > drivers/gpu/drm/i915/intel_guc_log.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_guc_log.c > b/drivers/gpu/drm/i915/intel_guc_log.c > index 3fbe93a..fdb1895 100644 > --- a/drivers/gpu/drm/i915/intel_guc_log.c > +++ b/drivers/gpu/drm/i915/intel_guc_log.c > @@ -395,6 +395,9 @@ static int guc_log_runtime_create(struct intel_guc *guc) > void *vaddr; > int ret; > > + if (!guc->log.vma) > + return -ENODEV; > + > lockdep_assert_held(_priv->drm.struct_mutex); We like to keep the caller precondition guards at the start of the function, so I reversed this pair of lines. I've pushed the two BUG fixes that Sagar reviewed. The fault injection is a good idea, but I've left it alone while there is a question on a couple of the patches. Thanks for the patches and review, looking forward to the next series :) -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/5] drm/i915/guc: Use correct error code for GuC initialization failure
Quoting Sagar Arun Kamble (2018-02-01 11:48:54) > > > On 1/31/2018 11:02 PM, Michal Wajdeczko wrote: > > Since commit 6ca9a2beb5 ("drm/i915: Unwind i915_gem_init() failure") > > we believed that we correctly handle all errors encountered during > > GuC initialization, including special one that indicates request to > > run driver with disabled GPU submission (-EIO). > > > > Unfortunately since commit 121981fafe ("drm/i915/guc: Combine > > enable_guc_loading|submission modparams") we stopped using that > > error code to avoid unwanted fallback to execlist submission mode. > > > > In result any GuC initialization failure was treated as non-recoverable > > error leading to driver load abort, so we could not even read related > > GuC error log to investigate cause of the problem. > > > > Fix that by always returning -EIO on uC hardware related failure. > > > > Signed-off-by: Michal Wajdeczko> > Cc: Chris Wilson > > Cc: Michal Winiarski > > Cc: Daniele Ceraolo Spurio > > Cc: Sagar Arun Kamble > > --- > > drivers/gpu/drm/i915/intel_uc.c | 5 + > > 1 file changed, 1 insertion(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_uc.c > > b/drivers/gpu/drm/i915/intel_uc.c > > index 3a13cbb..1547e16 100644 > > --- a/drivers/gpu/drm/i915/intel_uc.c > > +++ b/drivers/gpu/drm/i915/intel_uc.c > > @@ -432,11 +432,8 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv) > >* Note that there is no fallback as either user explicitly asked for > >* the GuC or driver default option was to run with the GuC enabled. > >*/ > > - if (GEM_WARN_ON(ret == -EIO)) > > - ret = -EINVAL; > > - > > dev_err(dev_priv->drm.dev, "GuC initialization failed %d\n", ret); > > - return ret; > > + return -EIO; /* We want to disable GPU submission but keep KMS alive > > */ > > } > We'll need to avoid uc_fini_hw/ in unload path since GPU will be marked > wedged after this? We should be avoiding uc_fini_hw in unload because we haven't completed uc_init_hw. Such failure should already be handled? -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] Enabling i915 Panel Self Refresh by default on some devices ?
Hi All, As you may have heard I've recently been working on improving Linux laptop battery life, specifically the OOTB experience without tweaking any options such as e.g. powertop --auto-tune would do, see: https://fedoraproject.org/wiki/Changes/ImprovedLaptopBatteryLife So far this is going quite nicely, it looks like Fedora 28 will have SATA ALPM (big win), autosuspend of USB Bluetooth HCIs and snd_intel_hda powersaving all enabled OOTB. Looking for more savings I've run some quick tests with i915.enable_psr=1, this seems to be another nice win (for an idle system) of aprox. 0.5W. So as with the other 3 items I just mentioned I'm now looking into somehow enabling this be default, at least one some models. Currently I'm thinking doing a whitelist or blacklist (*) for this, but first I think we need some more data about on how much models this just works and where it is causing issues, as such I've done a blog post to gather this data: https://hansdegoede.livejournal.com/18653.html So I will revisit this eventually, once people have had some time to respond to this blog-post. In the mean time I wonder if anyone can explain why this options is currently disabled by default. E.g. are there any known specific models laptops / panels which are causing issues, are the bugzillas for this? Etc. ? Also does anyone know if any problems are mainly panel or laptop model specific ? I would expect this to mostly be panel specific and not depend on the model laptop (given then certain models ship with different panels over their production lifetime). Regards, Hans p.s. If anyone on this list can make 10 minutes to run the tests described in my blogpost and gather the data mentioned there, then that would be great. *) I know that maintaining such a white/blacklist in the kernel is going to be a pain, so my current thinking on this is to make this runtime configurable through a sysfs attribute and then use a udev rule + udev hwdb entries for this. ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx