Re: [PATCH v2 00/14] YCBCR 4:2:0 handling in DRM layer
Regards Shashank On 7/15/2017 12:32 AM, Ville Syrjälä wrote: On Thu, Jul 13, 2017 at 09:03:06PM +0530, Shashank Sharma wrote: Following YCBCR 4:4:4 and 4:2:2, YCBCR 4:2:0 is a new output format, which is currently supported on HDMI 2.0 sources/sinks. Due to lower chroma sub-sampling rate, YCBCR 4:2:0 can drive the video modes at half the pixel clock than YCBCR 4:4:4 or RGB 8:8:8 outputs. For example, a CEA 4K@60, RGB 8:8:8 mode needs a clock of appx 594Mhz, but it can be driven at 297Mhz using YCBCR 4:2:0 output. Of course, the lower rate of chroma subsampling, causes the quality of YCBCR 4:2:0 to be lower than YCBCR 4:4:4 or RGB 8:8:8. This patch series adds support for YCBCR 4:2:0 output in DRM layer. - First 2 patches, complete the CEA mode-db in drm driver, by adding new 4k modes. Current CEA mode-db contains 64 modes only (VIC 1-64), whereas CEA-861-F defined VICs up to 107, including 4k modes, from VIC range 93-107. First patch makes sure that inclusion of these modes doesn't break existing HDMI 1.4 monitors, across various drivers. - Next 5 patches focus on parsing new YCBCR 4:2:0 EDID blocks, and adding YCBCR 4:2:0 modes in connector. They also contain a prune function, which will cleanup the YCBCR 4:2:0 modes from list, if the connector doesn't declare them supported. - Next 2 patches add helper functions for identifing YCBCR 4:2:0 modes and setup the color space in AVI infoframes. - Next 6 patches add code for I915 layer handling of YCBCR 4:2:0 output. This patch series was initially published as a complete framework to handle all YCBCR outputs (4:4:4, 4:2:2, 4:2:0), but based on the code reviews, now its been published as YCBCR 4:2:0 handling series only. The previous discussion and reviews can be found here: V5: https://patchwork.freedesktop.org/series/26815/ V1-V4: https://patchwork.freedesktop.org/series/22683/ Now re-publishing this patch series as YCBCR 4:2:0 handling series here. V2: Addressed review comments from Ville This series dropped one patch in V2(patch 8 from V1), so 14 patches in V2 This series has been tested with drm-tip code with following setup: Source: Intel Geminilake device. Sink: ACER S277HK HDMI 2.0 monitor. Protocol testing: Astro VA-1844A HDMI analyzer. Shashank Sharma (14): drm: handle HDMI 2.0 VICs in AVI info-frames drm/edid: complete CEA modedb(VIC 1-107) drm/edid: parse sink information before CEA blocks drm/edid: cleanup patch for CEA extended-tag macro drm: add helper to validate YCBCR420 modes drm/edid: parse YCBCR420 videomodes from EDID drm/edid: parse ycbcr 420 deep color information drm: add helper functions for YCBCR420 handling I just finished pushing the core bits into drm-misc-next. But I'm not super happy how that went because there were still quite a few trivial checkpatch warnings and indentation issues I had to fix up. All of that should have been dealt with before even submitting the patches to the list. Review bandwidth is a scarce resource so we don't want to squander it on this sort of stuff. The whole patch series was checked with checkpatch every time it was sent for review. It had only 1 checkpatch warning, for which I had added an explanation in patch's header like: For patch 1: PS: This patch touches a few lines in few files, which were already above 80 char, so checkpatch gives 80 char warning again. - gpu/drm/omapdrm/omap_encoder.c - gpu/drm/i915/intel_sdvo.c Apart from this, here is the checkpatch output: ../scripts/checkpatch.pl *.patch - v8-0001-drm-handle-HDMI-2.0-VICs-in-AVI-info-frames.patch - WARNING: line over 80 characters #278: FILE: drivers/gpu/drm/i915/intel_sdvo.c:999: + &pipe_config->base.adjusted_mode, WARNING: line over 80 characters #332: FILE: drivers/gpu/drm/omapdrm/omap_encoder.c:88: + r = drm_hdmi_avi_infoframe_from_display_mode(&avi, adjusted_mode, total: 0 errors, 2 warnings, 247 lines checked v8-0001-drm-handle-HDMI-2.0-VICs-in-AVI-info-frames.patch has style problems, please review. v8-0002-drm-edid-complete-CEA-modedb-VIC-1-107.patch total: 0 errors, 0 warnings, 245 lines checked v8-0002-drm-edid-complete-CEA-modedb-VIC-1-107.patch has no obvious style problems and is ready for submission. --- v8-0003-drm-edid-parse-sink-information-before-CEA-blocks.patch --- total: 0 errors, 0 warnings, 21 lines checked v8-0003-drm-edid-parse-sink-information-before-CEA-blocks.patch has no obvious style problems and is ready for submission. --- v8-0004-drm-edid-cleanup-patch-for-CEA-extended-tag-macro.patch ---
Re: [PATCH v2 12/14] drm/i915: prepare csc unit for YCBCR420 output
Regards Shashank On 7/15/2017 12:06 AM, Ville Syrjälä wrote: On Thu, Jul 13, 2017 at 09:03:18PM +0530, Shashank Sharma wrote: To support ycbcr output, we need a pipe CSC block to do RGB->YCBCR conversion. Current Intel platforms have only one pipe CSC unit, so we can either do color correction using it, or we can perform RGB->YCBCR conversion. This function adds a csc handler, which uses recommended bspec values to perform RGB->YCBCR conversion (target color space BT709) V2: Rebase V3: Rebase V4: Rebase V5: Addressed review comments from Ander - Remove extra line added in the patch - Add the spec details in the commit message - Combine two if(cond) while calling intel_crtc_compute_config V6: Handle YCBCR420 outputs only (Ville) V7: Addressed review comments from Ville: - Add description about target colorspace - Remove the comments from CSC function - DRM_DEBUG->DEBUG_KMS for atomic failure due to CSC unit busy - Remove unnecessary debug message about YCBCR420 possibe Cc: Ville Syrjala Cc: Daniel Vetter Cc: Ander Conselvan de Oliveira Signed-off-by: Shashank Sharma --- drivers/gpu/drm/i915/intel_color.c | 46 +++- drivers/gpu/drm/i915/intel_display.c | 15 2 files changed, 60 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c index f85d575..8698691 100644 --- a/drivers/gpu/drm/i915/intel_color.c +++ b/drivers/gpu/drm/i915/intel_color.c @@ -41,6 +41,22 @@ #define LEGACY_LUT_LENGTH (sizeof(struct drm_color_lut) * 256) +/* Post offset values for RGB->YCBCR conversion */ +#define POSTOFF_RGB_TO_YUV_HI 0x800 +#define POSTOFF_RGB_TO_YUV_ME 0x100 +#define POSTOFF_RGB_TO_YUV_LO 0x800 + +/* + * These values are direct register values specified in the Bspec, + * for RGB->YUV conversion matrix (colorspace BT709) + */ +#define CSC_RGB_TO_YUV_RU_GU 0x2ba809d8 +#define CSC_RGB_TO_YUV_BU 0x37e8 +#define CSC_RGB_TO_YUV_RY_GY 0x1e089cc0 +#define CSC_RGB_TO_YUV_BY 0xb528 +#define CSC_RGB_TO_YUV_RV_GV 0xbce89ad8 +#define CSC_RGB_TO_YUV_BV 0x1e08 + /* * Extract the CSC coefficient from a CTM coefficient (in U32.32 fixed point * format). This macro takes the coefficient we want transformed and the @@ -91,6 +107,31 @@ static void ctm_mult_by_limited(uint64_t *result, int64_t *input) } } +void i9xx_load_ycbcr_conversion_matrix(struct intel_crtc *intel_crtc) +{ + int pipe = intel_crtc->pipe; + struct drm_i915_private *dev_priv = to_i915(intel_crtc->base.dev); + + + I915_WRITE(PIPE_CSC_PREOFF_HI(pipe), 0); + I915_WRITE(PIPE_CSC_PREOFF_ME(pipe), 0); + I915_WRITE(PIPE_CSC_PREOFF_LO(pipe), 0); + + I915_WRITE(PIPE_CSC_COEFF_RU_GU(pipe), CSC_RGB_TO_YUV_RU_GU); + I915_WRITE(PIPE_CSC_COEFF_BU(pipe), CSC_RGB_TO_YUV_BU); + + I915_WRITE(PIPE_CSC_COEFF_RY_GY(pipe), CSC_RGB_TO_YUV_RY_GY); + I915_WRITE(PIPE_CSC_COEFF_BY(pipe), CSC_RGB_TO_YUV_BY); + + I915_WRITE(PIPE_CSC_COEFF_RV_GV(pipe), CSC_RGB_TO_YUV_RV_GV); + I915_WRITE(PIPE_CSC_COEFF_BV(pipe), CSC_RGB_TO_YUV_BV); + + I915_WRITE(PIPE_CSC_POSTOFF_HI(pipe), POSTOFF_RGB_TO_YUV_HI); + I915_WRITE(PIPE_CSC_POSTOFF_ME(pipe), POSTOFF_RGB_TO_YUV_ME); + I915_WRITE(PIPE_CSC_POSTOFF_LO(pipe), POSTOFF_RGB_TO_YUV_LO); + I915_WRITE(PIPE_CSC_MODE(pipe), 0); +} + /* Set up the pipe CSC unit. */ static void i9xx_load_csc_matrix(struct drm_crtc_state *crtc_state) { @@ -101,7 +142,10 @@ 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 (crtc_state->ctm) { + if (intel_crtc_state->ycbcr420) { + i9xx_load_ycbcr_conversion_matrix(intel_crtc); + return; + } else if (crtc_state->ctm) { struct drm_color_ctm *ctm = (struct drm_color_ctm *)crtc_state->ctm->data; uint64_t input[9] = { 0, }; diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 1a23ec0..9c6a1ed 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -6283,6 +6283,21 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc, return -EINVAL; } + /* YCBCR420 feasibility check */ Not a useful comment. Got it. + if (pipe_config->ycbcr420) { + struct drm_crtc_state *drm_state = &pipe_config->base; Having aliasing pointer for the same thing is just annoying. We do have quite a lot of that going around, but IMO we need to clean that all up at some point. So pls just do this instead: if (pipe_config->ycbcr420 && pipe_config->base.ctm) { ... } Most of the time its done to cover for 80 char limit, and to make code look good. but let me see if I can accommodate this one.
Re: [PATCH v2 11/14] drm/i915: prepare pipe for YCBCR420 output
Regards Shashank On 7/15/2017 12:03 AM, Ville Syrjälä wrote: On Thu, Jul 13, 2017 at 09:03:17PM +0530, Shashank Sharma wrote: To get HDMI YCBCR420 output, the PIPEMISC register should be programmed to: - Generate YCBCR output (bit 11) - In case of YCBCR420 outputs, it should be programmed in full blend mode to use the scaler in 5x3 ratio (bits 26 and 27) This patch: - Adds definition of these bits. - Programs PIPEMISC for YCBCR420 outputs. V2: rebase V3: rebase V4: rebase V5: added r-b from Ander V6: Handle only YCBCR420 outputs (ville) V7: rebase Cc: Ville Syrjala Cc: Ander Conselvan de Oliveira Cc: Daniel Vetter Reviewed-by: Ander Conselvan de Oliveira Signed-off-by: Shashank Sharma --- drivers/gpu/drm/i915/i915_reg.h | 3 +++ drivers/gpu/drm/i915/intel_display.c | 7 +++ 2 files changed, 10 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index c712d01..e5020d6 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -5227,6 +5227,9 @@ enum { #define _PIPE_MISC_A 0x70030 #define _PIPE_MISC_B 0x71030 +#define PIPEMISC_YCBCR420_ENABLE (1<<27) +#define PIPEMISC_YCBCR420_MODE_BLEND (1<<26) +#define PIPEMISC_OUTPUT_YCBCR(1<<11) Please rename to match spec. So something like: PIPEMISC_YUV420_ENABLE (1<<27) PIPEMISC_YUV420_MODE_FULL_BLEND (1<<26) PIPEMISC_OUTPUT_COLORSPACE_YUV (1<<11) got it. #define PIPEMISC_DITHER_BPC_MASK(7<<5) #define PIPEMISC_DITHER_8_BPC (0<<5) #define PIPEMISC_DITHER_10_BPC (1<<5) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index d78f1c2..1a23ec0 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -8076,6 +8076,7 @@ static void haswell_set_pipemisc(struct drm_crtc *crtc) { struct drm_i915_private *dev_priv = to_i915(crtc->dev); struct intel_crtc *intel_crtc = to_intel_crtc(crtc); + struct intel_crtc_state *config = intel_crtc->config; if (IS_BROADWELL(dev_priv) || INTEL_INFO(dev_priv)->gen >= 9) { u32 val = 0; @@ -8101,6 +8102,12 @@ 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_YCBCR | + PIPEMISC_YCBCR420_ENABLE | + PIPEMISC_YCBCR420_MODE_BLEND; + } I think we'll want two flags. One to specify whether we're outputting YCbCr and the other to indicate whether we need the 4:2:0 subsamling. So maybe something like bool ycbcr; bool ycbcr420; This would have been true if we were support YCBCR444/422/420 all, but the recent patch series only supports 420, so if its ycbcr its ycbcr420. We might be able to add preference of a 420_also mode in case of RGB Vs YCBCR420, by adding another property. We also need state readout for this stuff. With those two flags I think we can do something like: if (IS_BDW || GEN >= 9) { tmp = READ(PIPEMISC); crtc_state->ycbcr = tmp & OUTPUT_YUV; We dont support YCBCR (apart from 420) anymore, as there is no HDMI output property. So this doesn't look required. if (IS_GLK || GEN >= 10) crtc_state->ycbcr420 = tmp & YUV420_ENABLE; Yes, this is a great suggestion, and I believe I was missing this. Thanks ! } The other missing readout thing is adjustment of the clock. ddi_dotclock_get() will need to double the dotclock when we're outputting ycbcr420. Pls also add something along the lines of DRM_DEBUG_KMS("ycbcr: %i, ycbcr420: %i\n", ...); to intel_dump_pipe_config() so that we can actually tell when we're outputting YCbCr and 4:2:0. Thanks, this is also what I was looking for. - Shashank + I915_WRITE(PIPEMISC(intel_crtc->pipe), val); } } -- 2.7.4 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 10/14] drm/i915: prepare scaler for YCBCR420 modeset
Regards Shahank On 7/15/2017 12:00 AM, Ville Syrjälä wrote: On Thu, Jul 13, 2017 at 09:03:16PM +0530, Shashank Sharma wrote: To get a YCBCR420 output from intel platforms, we need one scaler to scale down YCBCR444 samples to YCBCR420 samples. This patch: - Does scaler allocation for HDMI ycbcr420 outputs. - Programs PIPE_MISC register for ycbcr420 output. - Adds a new scaler user "HDMI output" to plug-into existing scaler framework. This output type is identified using bit 30 of the scaler users bitmap. V2: rebase V3: rebase V4: rebase V5: addressed review comments from Ander: - No need to check both scaler_user && hdmi_output. Check for scaler_user is enough. V6: rebase V7: Do not create a new scaler user, use existing pipe scaler user. Cc: Ville Syrjala Cc: Ander Conselvan De Oliveira Signed-off-by: Shashank Sharma --- drivers/gpu/drm/i915/intel_display.c | 3 +++ drivers/gpu/drm/i915/intel_drv.h | 4 +++- drivers/gpu/drm/i915/intel_hdmi.c| 12 drivers/gpu/drm/i915/intel_panel.c | 3 ++- 4 files changed, 20 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index a5140e4..d78f1c2 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -4624,6 +4624,9 @@ 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) + need_scaling = true; + /* * Scaling/fitting not supported in IF-ID mode in GEN9+ * TODO: Interlace fetch mode doesn't support YUV420 planar formats. diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 592243b..94ea6ed 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -471,7 +471,8 @@ struct intel_crtc_scaler_state { * * If a bit is set, a user is using a scaler. * Here user can be a plane or crtc as defined below: -* bits 0-30 - plane (bit position is index from drm_plane_index) +* bits 0-29 - plane (bit position is index from drm_plane_index) +* bit 30- hdmi output That's no longer true. Agree ! * bit 31- crtc * * Instead of creating a new index to cover planes and crtc, using @@ -484,6 +485,7 @@ struct intel_crtc_scaler_state { * avilability. */ #define SKL_CRTC_INDEX 31 + Bogus whitespace change. Got it. unsigned scaler_users; /* scaler used by crtc for panel fitting purpose */ diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c index eb6243c..49f4fb8 100644 --- a/drivers/gpu/drm/i915/intel_hdmi.c +++ b/drivers/gpu/drm/i915/intel_hdmi.c @@ -1356,6 +1356,7 @@ intel_hdmi_ycbcr420_config(struct drm_connector *connector, struct intel_crtc_state *config, int *clock_12bpc, int *clock_8bpc) { + struct intel_crtc *intel_crtc = to_intel_crtc(config->base.crtc); if (!connector->ycbcr_420_allowed) { DRM_ERROR("Platform doesn't support YCBCR420 output\n"); @@ -1367,6 +1368,17 @@ intel_hdmi_ycbcr420_config(struct drm_connector *connector, *clock_12bpc /= 2; *clock_8bpc /= 2; config->ycbcr420 = true; + + /* YCBCR 420 output conversion needs a scaler */ + if (skl_update_scaler_crtc(config)) { + DRM_ERROR("Scaler allocation for output failed\n"); DRM_DEBUG_KMS Ok, + return false; + } + + /* Bind this scaler to pipe */ Unnecesary comment. Ok, Shashank + intel_pch_panel_fitting(intel_crtc, config, + DRM_MODE_SCALE_FULLSCREEN); + return true; } diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c index 96c2cbd..fd2e081 100644 --- a/drivers/gpu/drm/i915/intel_panel.c +++ b/drivers/gpu/drm/i915/intel_panel.c @@ -110,7 +110,8 @@ intel_pch_panel_fitting(struct intel_crtc *intel_crtc, /* Native modes don't need fitting */ if (adjusted_mode->crtc_hdisplay == pipe_config->pipe_src_w && - adjusted_mode->crtc_vdisplay == pipe_config->pipe_src_h) + adjusted_mode->crtc_vdisplay == pipe_config->pipe_src_h && + !pipe_config->ycbcr420) goto done; switch (fitting_mode) { -- 2.7.4 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 09/14] drm/i915: add config function for YCBCR420 outputs
Regards Shashank On 7/15/2017 12:00 AM, Ville Syrjälä wrote: On Thu, Jul 13, 2017 at 09:03:15PM +0530, Shashank Sharma wrote: This patch checks encoder level support for YCBCR420 outputs. The logic goes as simple as this: If the input mode is YCBCR420-only mode: prepare HDMI for YCBCR420 output, else continue with RGB output mode. It checks if the mode is YCBCR420 and source can support this output then it marks the ycbcr_420 output indicator into crtc state, for further staging in driver. V2: Split the patch into two, kept helper functions in DRM layer. V3: Changed the compute_config function based on new DRM API. V4: Rebase V5: Rebase V6: Check and handle YCBCR420-only modes, discard the property based approach (Ville) V7: Addressed review comments from Ville - add else case in 12BPC check. - extract ycbcr420 state inside hdmi_12bpc_possible function. Cc: Ville Syrjala Cc: Daniel Vetter Cc: Ander Conselvan de Oliveira Signed-off-by: Shashank Sharma --- drivers/gpu/drm/i915/intel_display.c | 1 + drivers/gpu/drm/i915/intel_drv.h | 3 +++ drivers/gpu/drm/i915/intel_hdmi.c| 43 +--- 3 files changed, 44 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 2144adc..a5140e4 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -11945,6 +11945,7 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv, PIPE_CONF_CHECK_I(hdmi_scrambling); PIPE_CONF_CHECK_I(hdmi_high_tmds_clock_ratio); PIPE_CONF_CHECK_I(has_infoframe); + PIPE_CONF_CHECK_I(ycbcr420); PIPE_CONF_CHECK_I(has_audio); diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index d17a324..592243b 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -780,6 +780,9 @@ struct intel_crtc_state { /* HDMI High TMDS char rate ratio */ bool hdmi_high_tmds_clock_ratio; + + /* HDMI output type */ We'll want DP too at some point. So that's going to get stale pretty soon. Agree, will make it output format ycbcr420 + bool ycbcr420; }; struct intel_crtc { diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c index cc0d100..eb6243c 100644 --- a/drivers/gpu/drm/i915/intel_hdmi.c +++ b/drivers/gpu/drm/i915/intel_hdmi.c @@ -1312,6 +1312,7 @@ static bool hdmi_12bpc_possible(struct intel_crtc_state *crtc_state) struct drm_atomic_state *state = crtc_state->base.state; struct drm_connector_state *connector_state; struct drm_connector *connector; + bool output_ycbcr420 = crtc_state->ycbcr420; Pointless variable. Just use 'crtc_state->ycbcr420' in the code directly. Ah, yes it was used at more places before we removed some of the code, during review. Sure, will use that directly. int i; if (HAS_GMCH_DISPLAY(dev_priv)) @@ -1330,8 +1331,16 @@ static bool hdmi_12bpc_possible(struct intel_crtc_state *crtc_state) if (connector_state->crtc != crtc_state->base.crtc) continue; - if ((info->edid_hdmi_dc_modes & DRM_EDID_HDMI_DC_36) == 0) - return false; + if (output_ycbcr420) { + const struct drm_hdmi_info *hdmi = &info->hdmi; + + if (!(hdmi->y420_dc_modes & DRM_EDID_YCBCR420_DC_36)) + return false; + } else { + Bogus whitespace. Got it. + if (!(info->edid_hdmi_dc_modes & DRM_EDID_HDMI_DC_36)) + return false; + } } /* Display Wa #1139 */ @@ -1342,6 +1351,25 @@ static bool hdmi_12bpc_possible(struct intel_crtc_state *crtc_state) return true; } +static bool +intel_hdmi_ycbcr420_config(struct drm_connector *connector, + struct intel_crtc_state *config, + int *clock_12bpc, int *clock_8bpc) +{ + Bogus whitespace. Got it. - Shashank + if (!connector->ycbcr_420_allowed) { + DRM_ERROR("Platform doesn't support YCBCR420 output\n"); + return false; + } + + /* YCBCR420 TMDS rate requirement is half the pixel clock */ + config->port_clock /= 2; + *clock_12bpc /= 2; + *clock_8bpc /= 2; + config->ycbcr420 = true; + return true; +} + bool intel_hdmi_compute_config(struct intel_encoder *encoder, struct intel_crtc_state *pipe_config, struct drm_connector_state *conn_state) @@ -1349,7 +1377,8 @@ bool intel_hdmi_compute_config(struct intel_encoder *encoder, struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&encoder->base); struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); struct drm_display_mode
Re: [PATCH 1/2] drm: Implement vm_operations_struct.access
On 15/07/17 04:47 AM, Felix Kuehling wrote: > On 17-07-13 11:26 PM, Michel Dänzer wrote: >> On 14/07/17 06:08 AM, Felix Kuehling wrote: >>> Allows gdb to access contents of user mode mapped BOs. VRAM access >>> requires the driver to implement a new callback in ttm_bo_driver. >> Thanks for doing this. Looks mostly good, but I still have some comments. >> >> The shortlog prefix should be "drm/ttm:". >> >> >>> diff --git a/drivers/gpu/drm/ttm/ttm_bo_vm.c >>> b/drivers/gpu/drm/ttm/ttm_bo_vm.c >>> index 9f53df9..0ef2eb9 100644 >>> --- a/drivers/gpu/drm/ttm/ttm_bo_vm.c >>> +++ b/drivers/gpu/drm/ttm/ttm_bo_vm.c >>> @@ -294,10 +294,74 @@ static void ttm_bo_vm_close(struct vm_area_struct >>> *vma) >>> vma->vm_private_data = NULL; >>> } >>> >>> +static int ttm_bo_vm_access_kmap(struct ttm_buffer_object *bo, >>> +unsigned long offset, >>> +void *buf, int len, int write) >>> +{ >>> + unsigned long first_page = offset >> PAGE_SHIFT; >>> + unsigned long last_page = (offset + len - 1) >> PAGE_SHIFT; >>> + unsigned long num_pages = last_page - first_page + 1; >>> + struct ttm_bo_kmap_obj map; >>> + void *ptr; >>> + bool is_iomem; >>> + int ret; >>> + >>> + ret = ttm_bo_kmap(bo, first_page, num_pages, &map); >>> + if (ret) >>> + return ret; >> Could there be cases (e.g. on 32-bit) where mapping all pages at once >> fails, but mapping one page at a time would work? > > Maybe. I'm not really familiar with ttm_bo_kmap limitations on 32-bit. I > guess the the access would have to be really big. I think in practice > GDB doesn't do very big accesses. So I'm not sure it's worth the trouble. Okay, I guess it can always be changed later if necessary. >>> + int (*access_vram)(struct ttm_buffer_object *bo, unsigned long offset, >>> + void *buf, int len, int write); >>> }; >> I suggest making the write parameter a bool. > > I made this as similar as possible to the vm_ops->access API, where > write is also an integer. I see, makes sense. -- Earthling Michel Dänzer | http://www.amd.com Libre software enthusiast | Mesa and X developer ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 6/8] drm: Allow DSI devices to be registered before the host registers.
Archit Taneja writes: > On 06/29/2017 04:09 PM, Andrzej Hajda wrote: >> On 29.06.2017 07:03, Archit Taneja wrote: >>> >>> On 06/28/2017 01:28 AM, Eric Anholt wrote: When a mipi_dsi_host is registered, the DT is walked to find any child nodes with compatible strings. Those get registered as DSI devices, and most DSI panel drivers are mipi_dsi_drivers that attach to those nodes. There is one special case currently, the adv7533 bridge, where the bridge probes on I2C, and during the bridge attach step it looks up the mipi_dsi_host and registers the mipi_dsi_device (for its own stub mipi_dsi_driver). For the Raspberry Pi panel, though, we also need to attach on I2C (our control bus), but don't have a bridge driver. The lack of a bridge's attach() step like adv7533 uses means that we aren't able to delay the mipi_dsi_device creation until the mipi_dsi_host is present. To fix this, we extend mipi_dsi_device_register_full() to allow being called with a NULL host, which puts the device on a queue waiting for a host to appear. When a new host is registered, we fill in the host value and finish the device creation process. >>> This is quite a nice idea. The only bothering thing is the info.of_node >>> usage >>> varies between child nodes (mipi_dsi_devs) and non-child nodes (i2c control >>> bus). >>> >>> For DSI children expressed in DT, the of_node in info holds the DT node >>> corresponding to the DSI child itself. For non-DT ones, this patch assumes >>> that info.of_node stores the DSI host DT node. I think it should be okay as >>> long as we mention the usage in a comment somewhere. The other option is to >>> have a new info.host_node field to keep a track of the host DT node. >> >> Field abuse is not a biggest issue. >> >> This patch changes totally semantic of mipi_dsi_device_register_full. >> Currently semantic of *_device_register* function is to create and add >> device to existing bus, ie after return we have device attached to bus, >> so it can be instantly used. With this change function can return some >> unattached device, without warranty it will be ever attached - kind of >> hidden deferring. Such change looks for me quite dangerous, even if it >> looks convenient in this case. >> >> As discussed in other thread more appealing solution for me would be: >> 1. host creates dsi bus, but doesn't call component_add as it does not >> have all required resources. >> 2. host waits for all required dsi devs attached, gets registered panels >> or bridges and calls component_add after that. >> 3. in bind phase it has all it needs, hasn't it? > > This seems like it would work, but would require KMS drivers to restructure > themselves around this approach. For KMS drivers that don't even use the > component stuff, it might be asking too much. > > We could maybe consider Eric's patch as an intermediate solution, we should > definitely put WARN_ON(!dsi->host) like checks for all the existing > mipi_dsi_*() API. Could you clarify which entrypoints you'd like a warning on? Is it just "everything that gets the host ops"? signature.asc Description: PGP signature ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 6/8] drm: Allow DSI devices to be registered before the host registers.
Archit Taneja writes: > On 06/28/2017 01:28 AM, Eric Anholt wrote: >> When a mipi_dsi_host is registered, the DT is walked to find any child >> nodes with compatible strings. Those get registered as DSI devices, >> and most DSI panel drivers are mipi_dsi_drivers that attach to those nodes. >> >> There is one special case currently, the adv7533 bridge, where the >> bridge probes on I2C, and during the bridge attach step it looks up >> the mipi_dsi_host and registers the mipi_dsi_device (for its own stub >> mipi_dsi_driver). >> >> For the Raspberry Pi panel, though, we also need to attach on I2C (our >> control bus), but don't have a bridge driver. The lack of a bridge's >> attach() step like adv7533 uses means that we aren't able to delay the >> mipi_dsi_device creation until the mipi_dsi_host is present. >> >> To fix this, we extend mipi_dsi_device_register_full() to allow being >> called with a NULL host, which puts the device on a queue waiting for >> a host to appear. When a new host is registered, we fill in the host >> value and finish the device creation process. > > This is quite a nice idea. The only bothering thing is the info.of_node usage > varies between child nodes (mipi_dsi_devs) and non-child nodes (i2c control > bus). > > For DSI children expressed in DT, the of_node in info holds the DT node > corresponding to the DSI child itself. For non-DT ones, this patch assumes > that info.of_node stores the DSI host DT node. I think it should be okay as > long as we mention the usage in a comment somewhere. The other option is to > have a new info.host_node field to keep a track of the host DT node. I think maybe you misread the patch? We're using of_get_parent(dsi->dev.node), which came from info->node, to compare to host->dev->of_node(). signature.asc Description: PGP signature ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 3/3] drm/i915: unregister interfaces first in unload
We first need to make sure no one else can get at us anymore, before we can proceed to tear down all the datastructures. Just a small step towards eventually the perfect unload code ... Reviewed-by: Chris Wilson Signed-off-by: Daniel Vetter --- drivers/gpu/drm/i915/i915_drv.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 2c2afb19975d..38990b264b97 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -1372,6 +1372,8 @@ void i915_driver_unload(struct drm_device *dev) struct drm_i915_private *dev_priv = to_i915(dev); struct pci_dev *pdev = dev_priv->drm.pdev; + i915_driver_unregister(dev_priv); + if (i915_gem_suspend(dev_priv)) DRM_ERROR("failed to idle hardware; continuing to unload!\n"); @@ -1381,8 +1383,6 @@ void i915_driver_unload(struct drm_device *dev) intel_gvt_cleanup(dev_priv); - i915_driver_unregister(dev_priv); - intel_modeset_cleanup(dev); /* -- 2.13.2 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 1/3] drm/atomic-helper: Fix leak in disable_all
The legacy plane->fb pointer is refcounted by calling drm_atomic_clean_old_fb(). In practice this isn't a real problem because: - The caller in the i915 gpu reset code restores the original state again, which means the plane->fb pointer won't change, hence can't leak. - Drivers using drm_atomic_helper_shutdown call the fbdev cleanup first, and that usually cleans up the fb through drm_remove_framebuffer, which does this correctly. - Without fbdev the only framebuffers are from userspace, and those get cleaned up (again using drm_remove_framebuffer) befor the driver can even be unloaded. But in i915 I've switched the cleanup sequence around so that the _shutdown() calls happens after the drm_remove_framebuffer(), which is how I discovered this issue. v2: My analysis why the current code was ok for gpu reset and suspend/resume was correct, but then I totally failed to realize that we better keep this symmetric. Thanksfully CI noticed that for balance, a refcounting bug must exist at 2 places if previously there was no issue ... Cc: martin.pe...@free.fr Cc: ch...@chris-wilson.co.uk Signed-off-by: Daniel Vetter --- drivers/gpu/drm/drm_atomic_helper.c | 12 +++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index b07fc30372d3..3a04619d3bec 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -2726,6 +2726,7 @@ int drm_atomic_helper_disable_all(struct drm_device *dev, struct drm_plane *plane; struct drm_crtc_state *crtc_state; struct drm_crtc *crtc; + unsigned plane_mask = 0; int ret, i; state = drm_atomic_state_alloc(dev); @@ -2768,10 +2769,14 @@ int drm_atomic_helper_disable_all(struct drm_device *dev, goto free; drm_atomic_set_fb_for_plane(plane_state, NULL); + plane_mask |= BIT(drm_plane_index(plane)); + plane->old_fb = plane->fb; } ret = drm_atomic_commit(state); free: + if (plane_mask) + drm_atomic_clean_old_fb(dev, plane_mask, ret); drm_atomic_state_put(state); return ret; } @@ -2902,6 +2907,8 @@ int drm_atomic_helper_commit_duplicated_state(struct drm_atomic_state *state, struct drm_connector_state *new_conn_state; struct drm_crtc *crtc; struct drm_crtc_state *new_crtc_state; + struct drm_device *dev = state->dev; + int ret; state->acquire_ctx = ctx; @@ -2914,7 +2921,10 @@ int drm_atomic_helper_commit_duplicated_state(struct drm_atomic_state *state, for_each_new_connector_in_state(state, connector, new_conn_state, i) state->connectors[i].old_state = connector->state; - return drm_atomic_commit(state); + ret = drm_atomic_commit(state); + drm_atomic_clean_old_fb(dev, ~0U, ret); + + return ret; } EXPORT_SYMBOL(drm_atomic_helper_commit_duplicated_state); -- 2.13.2 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 2/3] drm/i915: Fix fbdev unload sequence
First thing we need to do is unregister the fbdev instance, but we can't just go ahead and kfree it. That must wait until the hotplug and polling work are stopped, since they can race with the with the teardown. That means we need to split up the fbdev teardown into the unregister part and the cleanup part. I originally suspected that this was broken in one of the unload shuffles, but on closer inspection the oldest sequence I've dug out also gets this wrong. Just not quite so badly. I've run drv_module_reload a few hundred times and it's rock solid compared to insta-death beforehand. This bug seems to have been uncovered by commit 88be58be886f1215cc73dc8c273c985eecd7385c Author: Daniel Vetter Date: Thu Jul 6 15:00:19 2017 +0200 drm/i915/fbdev: Always forward hotplug events But the effect of that seems to only be to increase the race window enough to make it blow up easier. I'm not exactly clear on what's going on there ... v2: Fix whitespace and use fetch_and_zero (Chris). Testcase: igt/drv_module_reload Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=101791 Cc: martin.pe...@free.fr Cc: ch...@chris-wilson.co.uk Reviewed-by: Chris Wilson Signed-off-by: Daniel Vetter --- drivers/gpu/drm/i915/i915_drv.c | 3 +-- drivers/gpu/drm/i915/intel_display.c | 3 +++ drivers/gpu/drm/i915/intel_drv.h | 9 +++-- drivers/gpu/drm/i915/intel_fbdev.c | 22 +++--- 4 files changed, 26 insertions(+), 11 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index d310d8245dca..2c2afb19975d 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -1240,6 +1240,7 @@ static void i915_driver_register(struct drm_i915_private *dev_priv) */ static void i915_driver_unregister(struct drm_i915_private *dev_priv) { + intel_fbdev_unregister(dev_priv); intel_audio_deinit(dev_priv); intel_gpu_ips_teardown(); @@ -1371,8 +1372,6 @@ void i915_driver_unload(struct drm_device *dev) struct drm_i915_private *dev_priv = to_i915(dev); struct pci_dev *pdev = dev_priv->drm.pdev; - intel_fbdev_fini(dev); - if (i915_gem_suspend(dev_priv)) DRM_ERROR("failed to idle hardware; continuing to unload!\n"); diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index ecb4e30673fc..e10fae2faa88 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -15111,6 +15111,9 @@ void intel_modeset_cleanup(struct drm_device *dev) */ drm_kms_helper_poll_fini(dev); + /* poll work can call into fbdev, hence clean that up afterwards */ + intel_fbdev_fini(dev_priv); + intel_unregister_dsm_handler(); intel_fbc_global_disable(dev_priv); diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 3132260f18ce..7361e983d953 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1597,7 +1597,8 @@ void intel_hpd_poll_init(struct drm_i915_private *dev_priv); #ifdef CONFIG_DRM_FBDEV_EMULATION extern int intel_fbdev_init(struct drm_device *dev); extern void intel_fbdev_initial_config_async(struct drm_device *dev); -extern void intel_fbdev_fini(struct drm_device *dev); +extern void intel_fbdev_unregister(struct drm_i915_private *dev_priv); +extern void intel_fbdev_fini(struct drm_i915_private *dev_priv); extern void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous); extern void intel_fbdev_output_poll_changed(struct drm_device *dev); extern void intel_fbdev_restore_mode(struct drm_device *dev); @@ -1611,7 +1612,11 @@ static inline void intel_fbdev_initial_config_async(struct drm_device *dev) { } -static inline void intel_fbdev_fini(struct drm_device *dev) +static inline void intel_fbdev_unregister(struct drm_i915_private *dev_priv) +{ +} + +static inline void intel_fbdev_fini(struct drm_i915_private *dev_priv) { } diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c index b953365a3eec..391992373d27 100644 --- a/drivers/gpu/drm/i915/intel_fbdev.c +++ b/drivers/gpu/drm/i915/intel_fbdev.c @@ -538,8 +538,6 @@ static void intel_fbdev_destroy(struct intel_fbdev *ifbdev) * trying to rectify all the possible error paths leading here. */ - drm_fb_helper_unregister_fbi(&ifbdev->helper); - drm_fb_helper_fini(&ifbdev->helper); if (ifbdev->vma) { @@ -727,8 +725,10 @@ static void intel_fbdev_initial_config(void *data, async_cookie_t cookie) /* Due to peculiar init order wrt to hpd handling this is separate. */ if (drm_fb_helper_initial_config(&ifbdev->helper, -ifbdev->preferred_bpp)) - intel_fbdev_fini(ifbdev->helper.dev); +ifbdev->preferred_bpp)) { + intel_fbdev_unregister(
Re: [Intel-gfx] [PATCH RESEND] drm/i915: Fix pipe/transcoder enum mismatches
On Fri, Jul 14, 2017 at 7:32 PM, Grant Grundler wrote: > On Fri, Jul 14, 2017 at 2:11 AM, Jani Nikula > wrote: >> On Thu, 13 Jul 2017, Stéphane Marchesin wrote: >>> So, if you think this is wrong, can you fix this warning in a way that >>> you'd like? >> >> As I replied previously [1], with more background, fixing the warnings >> properly, in a way that actually improves the code instead of making it >> worse, would mean a bunch of churn that's not just purely mechanical >> conversion. > > That's fair. > >> Unless you can point out a bug which is actually caused by mixing the >> types (which is mostly intentional, see the background) I have a hard >> time telling people this should be a priority. > > This feels like "can't see the forest because of the trees". > > The original patch was submitted in order to compile cleanly using > clang and the above suggests using clang is not important. Using > clang is important to Matthias and the Chrome OS organization for many > good reasons - including better warnings. > > The original patch message was clear that clang was generating the > warning. This isn't the only patch mka has sent to kernel devs. What > one can infer is Chrome OS is trying to move to clang (like other > Google products _already_ have.) My impression is all these products > are a priority to Intel - but it would be good to know otherwise. > >> Definitely something we'd >> like to do in the long run and pedantically correct (and I tend to >> prefer code that way) but we certainly have more important things to do. > > The long run is now. Everyone agrees the code should change and you > don't have to do it. Matthias submitted an unacceptable patch and > giving him some concrete guidance on what would be acceptable would > enable him to implement/test it (or anyone else could for that > matter). Can you do that? > > Just give an example of what the "right" API looks like and see where it goes. We've replied and discussed on May 5th what that roughly should be, right when Matthias pinged us. The original submission unfortunately fell through the cracks (it happens, not much we can do with this flood). Matthias didn't seem to have any questions about the proposed solutions (we laid out both the minimal short-term fix to unconfuse things, and what might be done on top), I think a reasonable assumption was that it's all clear. Otherwise he should have asked. Now, over 2 months later (and complete silence from your side) there's suddenly mass panic and multiple escalations on all available channels, which feels like a rather decent overreaction and not a terrible constructive way to collaborate on the upstream codebase. Anyway, I've done the quick draft for the function declaration changes that would clear up the confusion, just needs a clang run to update all the parameters to match, and passed that on to Stéphane Marchesin. I expect you to follow up with the corresponding patch right away. Thanks a lot. Yours, Daniel For reference the diff, but probably whitespace mangled because the real machine is down already: diff --git a/drivers/gpu/drm/i915/intel_fifo_underrun.c b/drivers/gpu/drm/i915/intel_fifo_underrun.c index d484862cc7df..21c221b4ae57 100644 --- a/drivers/gpu/drm/i915/intel_fifo_underrun.c +++ b/drivers/gpu/drm/i915/intel_fifo_underrun.c @@ -313,7 +313,7 @@ bool intel_set_cpu_fifo_underrun_reporting(struct drm_i915_private *dev_priv, * Returns the previous state of underrun reporting. */ bool intel_set_pch_fifo_underrun_reporting(struct drm_i915_private *dev_priv, - enum transcoder pch_transcoder, + enum pipe pch_transcoder, bool enable) { struct intel_crtc *crtc = @@ -390,7 +390,7 @@ void intel_cpu_fifo_underrun_irq_handler(struct drm_i915_private *dev_priv, * interrupt to avoid an irq storm. */ void intel_pch_fifo_underrun_irq_handler(struct drm_i915_private *dev_priv, - enum transcoder pch_transcoder) + enum pipe pch_transcoder) { if (intel_set_pch_fifo_underrun_reporting(dev_priv, pch_transcoder, false)) { -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/vc4: Fix VBLANK handling in crtc->enable() path
Boris Brezillon writes: > When we are enabling a CRTC, drm_crtc_vblank_get() is called before > drm_crtc_vblank_on(), which is not supposed to happen (hence the > WARN_ON() in the code). To solve the problem, we delay the 'update > display list' operation after the CRTC is actually enabled. Reviewed and pushed to drm-misc-next-fixes (with a Fixes tag for the refactor you did that made this a more prominent warning). Sorry for the delay! signature.asc Description: PGP signature ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH, RESEND 03/14] drm/vmwgfx: avoid gcc-7 parentheses warning
On Fri, Jul 14, 2017 at 9:23 PM, Linus Torvalds wrote: > On Fri, Jul 14, 2017 at 12:21 PM, Linus Torvalds > wrote: >> >> NAK. This takes unintentionally insane code and turns it intentionally >> insane. Any non-zero return is considered an error. >> >> The right fix is almost certainly to just return -EINVAL unconditionally. > > Btw, this is why I hate compiler warning fix patch series. Even when > they don't actually break the code (and sometimes they do that too), > they can actually end up making the code worse. I generally agree, and this is also why I held up sending patches for the -Wformat warnings until you brought those up. I also frequently send patches for recently introduced warnings, which tend to have a better chance of getting reviewed by the person that just introduced the code, to catch this kind of mistake in my patches. I also regularly run into cases where I send a correct patch and find that another broken patch has been applied the following day ;-) > The *intent* of that code was to return zero for the CAP_SYS_ADMIN. > But the code has never done that in its lifetime and nobody ever > noticed, so clearly the code shouldn't even have tried. Makes sense, yes. In this case, the review process has failed as well, as one of the maintainers even gave an Ack on the wrong patch, and then the patch got dropped without any feedback. Arnd ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/2] drm: Implement vm_operations_struct.access
On 17-07-13 11:26 PM, Michel Dänzer wrote: > On 14/07/17 06:08 AM, Felix Kuehling wrote: >> Allows gdb to access contents of user mode mapped BOs. VRAM access >> requires the driver to implement a new callback in ttm_bo_driver. > Thanks for doing this. Looks mostly good, but I still have some comments. > > The shortlog prefix should be "drm/ttm:". > > >> diff --git a/drivers/gpu/drm/ttm/ttm_bo_vm.c >> b/drivers/gpu/drm/ttm/ttm_bo_vm.c >> index 9f53df9..0ef2eb9 100644 >> --- a/drivers/gpu/drm/ttm/ttm_bo_vm.c >> +++ b/drivers/gpu/drm/ttm/ttm_bo_vm.c >> @@ -294,10 +294,74 @@ static void ttm_bo_vm_close(struct vm_area_struct *vma) >> vma->vm_private_data = NULL; >> } >> >> +static int ttm_bo_vm_access_kmap(struct ttm_buffer_object *bo, >> + unsigned long offset, >> + void *buf, int len, int write) >> +{ >> +unsigned long first_page = offset >> PAGE_SHIFT; >> +unsigned long last_page = (offset + len - 1) >> PAGE_SHIFT; >> +unsigned long num_pages = last_page - first_page + 1; >> +struct ttm_bo_kmap_obj map; >> +void *ptr; >> +bool is_iomem; >> +int ret; >> + >> +ret = ttm_bo_kmap(bo, first_page, num_pages, &map); >> +if (ret) >> +return ret; > Could there be cases (e.g. on 32-bit) where mapping all pages at once > fails, but mapping one page at a time would work? Maybe. I'm not really familiar with ttm_bo_kmap limitations on 32-bit. I guess the the access would have to be really big. I think in practice GDB doesn't do very big accesses. So I'm not sure it's worth the trouble. > > >> +offset -= first_page << PAGE_SHIFT; >> +ptr = (uint8_t *)ttm_kmap_obj_virtual(&map, &is_iomem) + offset; >> +WARN_ON(is_iomem); > I suggest making this WARN_ON_ONCE, to avoid flooding dmesg if it ever > triggers in practice. > > >> static struct ttm_buffer_object *ttm_bo_vm_lookup(struct ttm_bo_device >> *bdev, >> diff --git a/include/drm/ttm/ttm_bo_driver.h >> b/include/drm/ttm/ttm_bo_driver.h >> index 6bbd34d..6ce5094 100644 >> --- a/include/drm/ttm/ttm_bo_driver.h >> +++ b/include/drm/ttm/ttm_bo_driver.h >> @@ -471,6 +471,18 @@ struct ttm_bo_driver { >> */ >> unsigned long (*io_mem_pfn)(struct ttm_buffer_object *bo, >> unsigned long page_offset); >> + >> +/** >> + * Read/write VRAM buffers for ptrace access > Is there any reason for making this specific to VRAM? ttm_bo_vm_access > could just call this for anything except TTM_PL_SYSTEM / TTM_PL_TT, and > let the driver return an error if it can't handle the memory type. Good point. I'll change that. > > >> + * @bo: the BO to access >> + * @offset: the offset from the start of the BO >> + * @buf: pointer to source/destination buffer >> + * @len: number of bytes to copy >> + * @write: whether to read (0) from or write (non-0) to BO >> + */ > The meaning of the return value should also be documented here. > > >> +int (*access_vram)(struct ttm_buffer_object *bo, unsigned long offset, >> + void *buf, int len, int write); >> }; > I suggest making the write parameter a bool. I made this as similar as possible to the vm_ops->access API, where write is also an integer. Regards, Felix > > ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 08/14] Input: adxl34x - fix gcc-7 -Wint-in-bool-context warning
On Fri, Jul 14, 2017 at 9:24 PM, Linus Torvalds wrote: > On Fri, Jul 14, 2017 at 2:25 AM, Arnd Bergmann wrote: >> FIFO_MODE is an macro expression with a '<<' operator, which >> gcc points out could be misread as a '<': > > Yeah, no, NAK again. > > We don't make the code look worse just because gcc is being a f*cking > moron about things. > > This warning is clearly pure garbage. > I looked at this one again and found a better approach, matching the check that is done a few lines later. Unless you find something wrong with that one, I'd resubmit it with the fixup below. Arnd --- a/drivers/input/misc/adxl34x.c +++ b/drivers/input/misc/adxl34x.c @@ -789,21 +789,21 @@ struct adxl34x *adxl34x_probe(struct device *dev, int irq, __set_bit(pdata->ev_code_ff, input_dev->keybit); } if (pdata->ev_code_act_inactivity) __set_bit(pdata->ev_code_act_inactivity, input_dev->keybit); ac->int_mask |= ACTIVITY | INACTIVITY; if (pdata->watermark) { ac->int_mask |= WATERMARK; - if (FIFO_MODE(pdata->fifo_mode) == 0) + if (FIFO_MODE(pdata->fifo_mode) == FIFO_BYPASS) ac->pdata.fifo_mode |= FIFO_STREAM; } else { ac->int_mask |= DATA_READY; } if (pdata->tap_axis_control & (TAP_X_EN | TAP_Y_EN | TAP_Z_EN)) ac->int_mask |= SINGLE_TAP | DOUBLE_TAP; if (FIFO_MODE(pdata->fifo_mode) == FIFO_BYPASS) ac->fifo_delay = false; ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/2] drm: Implement vm_operations_struct.access
On 17-07-14 06:06 AM, Christian König wrote: > Am 13.07.2017 um 23:08 schrieb Felix Kuehling: >> Allows gdb to access contents of user mode mapped BOs. VRAM access >> requires the driver to implement a new callback in ttm_bo_driver. > > One more comment additionally to what Michel already wrote below, > apart from that it looks good to me. > >> >> +switch(bo->mem.mem_type) { >> +case TTM_PL_SYSTEM: > > When the BO is in the system domain you need to add this as well I think: > > if (unlikely(bo->ttm->page_flags & TTM_PAGE_FLAG_SWAPPED)) { > ret = ttm_tt_swapin(bo->ttm); > if (unlikely(ret != 0)) > return ret; > } OK, thanks for pointing that out. Regards, Felix > > Regards, > Christian. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/2] drm/amdgpu: Implement ttm_bo_driver.access_vram callback
On 17-07-14 06:08 AM, Christian König wrote: > Am 13.07.2017 um 23:08 schrieb Felix Kuehling: >> Allows gdb to access contents of user mode mapped VRAM BOs. >> >> Signed-off-by: Felix Kuehling >> --- >> drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 59 >> + >> drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h | 2 ++ >> 2 files changed, 61 insertions(+) >> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c >> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c >> index ff5614b..d65551d 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c >> @@ -1115,6 +1115,64 @@ static bool >> amdgpu_ttm_bo_eviction_valuable(struct ttm_buffer_object *bo, >> return ttm_bo_eviction_valuable(bo, place); >> } >> +static int amdgpu_ttm_access_vram(struct ttm_buffer_object *bo, >> + unsigned long offset, >> + void *buf, int len, int write) >> +{ >> +struct amdgpu_bo *abo = container_of(bo, struct amdgpu_bo, tbo); >> +struct amdgpu_device *adev = amdgpu_ttm_adev(abo->tbo.bdev); >> +struct drm_mm_node *nodes = abo->tbo.mem.mm_node; >> +uint32_t value = 0; >> +int result = 0; >> +uint64_t pos; >> +unsigned long flags; >> + >> +while (offset >= (nodes->size << PAGE_SHIFT)) { >> +offset -= nodes->size << PAGE_SHIFT; >> +++nodes; >> +} >> +pos = (nodes->start << PAGE_SHIFT) + offset; > > This silently assumes that a read would never cross a node boundary, > doesn't it? It doesn't. See below ... > > Christian. > >> + >> +while (len && pos < adev->mc.mc_vram_size) { >> +uint64_t aligned_pos = pos & ~(uint64_t)3; >> +uint32_t bytes = 4 - (pos & 3); >> +uint32_t shift = (pos & 3) * 8; >> +uint32_t mask = 0x << shift; >> + >> +if (len < bytes) { >> +mask &= 0x >> (bytes - len) * 8; >> +bytes = len; >> +} >> + >> +spin_lock_irqsave(&adev->mmio_idx_lock, flags); >> +WREG32(mmMM_INDEX, ((uint32_t)aligned_pos) | 0x8000); >> +WREG32(mmMM_INDEX_HI, aligned_pos >> 31); >> +if (!write || mask != 0x) >> +value = RREG32(mmMM_DATA); >> +if (write) { >> +value &= ~mask; >> +value |= (*(uint32_t *)buf << shift) & mask; >> +WREG32(mmMM_DATA, value); >> +} >> +spin_unlock_irqrestore(&adev->mmio_idx_lock, flags); >> +if (!write) { >> +value = (value & mask) >> shift; >> +memcpy(buf, &value, bytes); >> +} >> + >> +result += bytes; >> +buf = (uint8_t *)buf + bytes; >> +pos += bytes; >> +len -= bytes; >> +if (pos >= (nodes->start + nodes->size) << PAGE_SHIFT) { >> +++nodes; >> +pos = (nodes->start << PAGE_SHIFT); ... Here I handle crossing a node boundary. Yes, I actually added this case to my kfdtest unit test and made sure it works, along with all odd alignments that the code above handles. Regards, Felix >> +} >> +} >> + >> +return result; >> +} >> + >> static struct ttm_bo_driver amdgpu_bo_driver = { >> .ttm_tt_create = &amdgpu_ttm_tt_create, >> .ttm_tt_populate = &amdgpu_ttm_tt_populate, >> @@ -1130,6 +1188,7 @@ static bool >> amdgpu_ttm_bo_eviction_valuable(struct ttm_buffer_object *bo, >> .io_mem_reserve = &amdgpu_ttm_io_mem_reserve, >> .io_mem_free = &amdgpu_ttm_io_mem_free, >> .io_mem_pfn = amdgpu_ttm_io_mem_pfn, >> +.access_vram = &amdgpu_ttm_access_vram >> }; >> int amdgpu_ttm_init(struct amdgpu_device *adev) >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h >> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h >> index f137c24..a22e430 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h >> @@ -78,6 +78,8 @@ int amdgpu_fill_buffer(struct amdgpu_bo *bo, >> struct dma_fence **fence); >> int amdgpu_mmap(struct file *filp, struct vm_area_struct *vma); >> +int amdgpu_bo_mmap(struct file *filp, struct vm_area_struct *vma, >> + struct ttm_bo_device *bdev); >> bool amdgpu_ttm_is_bound(struct ttm_tt *ttm); >> int amdgpu_ttm_bind(struct ttm_buffer_object *bo, struct >> ttm_mem_reg *bo_mem); >> int amdgpu_ttm_recover_gart(struct amdgpu_device *adev); > > ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [regression drm/noveau] suspend to ram -> BOOM: exception RIP: drm_calc_vbltimestamp_from_scanoutpos+335
On Fri, Jul 14, 2017 at 06:33:01PM +0200, Mike Galbraith wrote: > On Fri, 2017-07-14 at 18:10 +0200, Peter Zijlstra wrote: > > On Fri, Jul 14, 2017 at 05:58:18PM +0200, Mike Galbraith wrote: > > > On Fri, 2017-07-14 at 17:50 +0200, Peter Zijlstra wrote: > > > > > > Urgh, is for some mysterious reason the __bug_table section of modules > > > > ending up in RO memory? > > > > > > > > I forever get lost in that link magic :/ > > > > > > +1 > > > > > > drm.ko > > > 20 __bug_table 0630 0004bff3 > > > 2**0 > > > CONTENTS, ALLOC, LOAD, RELOC, READONLY, DATA > > > vmlinux > > > 15 __bug_table ba84 81af26c0 01af26c0 00cf26c0 > > > 2**0 > > > CONTENTS, ALLOC, LOAD, READONLY, DATA > > > > > > Danged if I know... um um RELOC business mucks things up? > > > > Argh, it shouldn't be READONLY for vmlinux either, but apparently that > > is working for mysterious reasons. > > > > Some architectures were in fact complaining that I broke that, and hence > > patch: > > > > b5effd3815cc ("debug: Fix __bug_table[] in arch linker scripts") > > > > I think we need professional help with this linking stuff, but who to > > ask? > > Andy Lutomirski? Does this fix it? diff --git a/arch/x86/include/asm/bug.h b/arch/x86/include/asm/bug.h index 39e702d..aa6b202 100644 --- a/arch/x86/include/asm/bug.h +++ b/arch/x86/include/asm/bug.h @@ -35,7 +35,7 @@ #define _BUG_FLAGS(ins, flags) \ do { \ asm volatile("1:\t" ins "\n"\ -".pushsection __bug_table,\"a\"\n" \ +".pushsection __bug_table,\"aw\"\n"\ "2:\t" __BUG_REL(1b) "\t# bug_entry::bug_addr\n" \ "\t" __BUG_REL(%c0) "\t# bug_entry::file\n" \ "\t.word %c1""\t# bug_entry::line\n" \ @@ -52,7 +52,7 @@ do { \ #define _BUG_FLAGS(ins, flags) \ do { \ asm volatile("1:\t" ins "\n"\ -".pushsection __bug_table,\"a\"\n" \ +".pushsection __bug_table,\"aw\"\n"\ "2:\t" __BUG_REL(1b) "\t# bug_entry::bug_addr\n" \ "\t.word %c0""\t# bug_entry::flags\n" \ "\t.org 2b+%c1\n" \ ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] [PATCH 3/3] drm/i915: unregister interfaces first in unload
Quoting Daniel Vetter (2017-07-14 20:14:39) > We first need to make sure no one else can get at us anymore, > before we can proceed to tear down all the datastructures. > > Just a small step towards eventually the perfect unload code ... > > Signed-off-by: Daniel Vetter Just some concern from the inertia of having the old order for years, but this looks completely correct to me. That the driver may still be active as we disable the userspace interfaces should not affect those. Reviewed-by: Chris Wilson -Chris ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/3] drm/i915: Fix fbdev unload sequence
Quoting Daniel Vetter (2017-07-14 20:14:38) Second nit: > +void intel_fbdev_fini(struct drm_i915_private *dev_priv) > +{ > + struct intel_fbdev *ifbdev = dev_priv->fbdev; > + > + if (!ifbdev) > + return; > + > intel_fbdev_destroy(ifbdev); > dev_priv->fbdev = NULL; Maybe struct intel_fbdev *ifbdev; ifbdev = fetch_and_zero(&dev_priv->fbdev); if (!fbdev) return; intel_fbdev_destroy(ifbdev); -Chris ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 14/14] [media] fix warning on v4l2_subdev_call() result interpreted as bool
On Fri, Jul 14, 2017 at 3:09 PM, Dan Carpenter wrote: > On Fri, Jul 14, 2017 at 03:55:26PM +0300, Dan Carpenter wrote: >> I don't agree with it as a static analysis dev... > > What I mean is if it's a macro that returns -ENODEV or a function that > returns -ENODEV, they should both be treated the same. The other > warnings this check prints are quite clever. I think this is what gcc tries to do, and it should work normally, but it fails when using ccache. I know I had cases like that, not entirely sure if this is one of them. Maybe it just means I should give up on using ccache in preprocessor mode. Arnd ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/3] drm/i915: Fix fbdev unload sequence
Quoting Daniel Vetter (2017-07-14 20:14:38) > First thing we need to do is unregister the fbdev instance, but we > can't just go ahead and kfree it. That must wait until the hotplug and > polling work are stopped, since they can race with the with the > teardown. That means we need to split up the fbdev teardown into the > unregister part and the cleanup part. > > I originally suspected that this was broken in one of the unload > shuffles, but on closer inspection the oldest sequence I've dug out > also gets this wrong. Just not quite so badly. > > I've run drv_module_reload a few hundred times and it's rock solid > compared to insta-death beforehand. This bug seems to have been > uncovered by > > commit 88be58be886f1215cc73dc8c273c985eecd7385c > Author: Daniel Vetter > Date: Thu Jul 6 15:00:19 2017 +0200 > > drm/i915/fbdev: Always forward hotplug events > > But the effect of that seems to only be to increase the race window > enough to make it blow up easier. I'm not exactly clear on what's > going on there ... > > Testcase: igt/drv_module_reload > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=101791 > Cc: martin.pe...@free.fr > Cc: ch...@chris-wilson.co.uk > Signed-off-by: Daniel Vetter Splitting out the unregister and running it early makes sense and is how we expect to unload to proceed. Reviewed-by: Chris Wilson Nit: > @@ -1383,7 +1383,6 @@ void i915_driver_unload(struct drm_device *dev) > intel_gvt_cleanup(dev_priv); > > i915_driver_unregister(dev_priv); > - > intel_modeset_cleanup(dev); I think it will useful to maintain the visual break here. (I know next patch moves it again...) -Chris ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/3] drm/atomic-helper: Fix leak in disable_all
Quoting Daniel Vetter (2017-07-14 20:14:37) > The legacy plane->fb pointer is refcounted by calling > drm_atomic_clean_old_fb(). > > In practice this isn't a real problem because: > - The caller in the i915 gpu reset code restores the original state > again, which means the plane->fb pointer won't change, hence can't > leak. > - Drivers using drm_atomic_helper_shutdown call the fbdev cleanup > first, and that usually cleans up the fb through > drm_remove_framebuffer, which does this correctly. > - Without fbdev the only framebuffers are from userspace, and those > get cleaned up (again using drm_remove_framebuffer) befor the driver > can even be unloaded. > > But in i915 I've switched the cleanup sequence around so that the > _shutdown() calls happens after the drm_remove_framebuffer(), which is > how I discovered this issue. If only we have refcounted fb now and didn't need every caller manually tracking the old pointers. Given the requirement for the caller to cleanup old_fb around drm_atomic_commit(), this looks correct to me. > Cc: martin.pe...@free.fr > Cc: ch...@chris-wilson.co.uk > Signed-off-by: Daniel Vetter Reviewed-by: Chris Wilson -Chris ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 08/14] Input: adxl34x - fix gcc-7 -Wint-in-bool-context warning
On Fri, Jul 14, 2017 at 2:25 AM, Arnd Bergmann wrote: > FIFO_MODE is an macro expression with a '<<' operator, which > gcc points out could be misread as a '<': Yeah, no, NAK again. We don't make the code look worse just because gcc is being a f*cking moron about things. This warning is clearly pure garbage. Linus ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH, RESEND 03/14] drm/vmwgfx: avoid gcc-7 parentheses warning
On Fri, Jul 14, 2017 at 12:21 PM, Linus Torvalds wrote: > > NAK. This takes unintentionally insane code and turns it intentionally > insane. Any non-zero return is considered an error. > > The right fix is almost certainly to just return -EINVAL unconditionally. Btw, this is why I hate compiler warning fix patch series. Even when they don't actually break the code (and sometimes they do that too), they can actually end up making the code worse. The *intent* of that code was to return zero for the CAP_SYS_ADMIN. But the code has never done that in its lifetime and nobody ever noticed, so clearly the code shouldn't even have tried. Linus ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH, RESEND 03/14] drm/vmwgfx: avoid gcc-7 parentheses warning
On Fri, Jul 14, 2017 at 2:25 AM, Arnd Bergmann wrote: > - return capable(CAP_SYS_ADMIN) ? : -EINVAL; > + return capable(CAP_SYS_ADMIN) ? 1 : -EINVAL; NAK. This takes unintentionally insane code and turns it intentionally insane. Any non-zero return is considered an error. The right fix is almost certainly to just return -EINVAL unconditionally. Linus ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] dma-buf/fence: Avoid use of uninitialised timestamp
2017-07-11 Chris Wilson : > Quoting Chris Wilson (2017-02-14 12:40:01) > > [ 236.821534] WARNING: kmemcheck: Caught 64-bit read from uninitialized > > memory (8802538683d0) > > [ 236.828642] > > 42001e7f0008 > > [ 236.839543] i i i i u u u u i i i i i i i i u u u u u u u u u u u u u u > > u u > > [ 236.850420] ^ > > [ 236.854123] RIP: 0010:[] [] > > fence_signal+0x17/0xd0 > > [ 236.861313] RSP: 0018:88024acd7ba0 EFLAGS: 00010282 > > [ 236.865027] RAX: 812f6a90 RBX: 8802527ca800 RCX: > > 880252cb30e0 > > [ 236.868801] RDX: 88024ac5d918 RSI: 880252f780e0 RDI: > > 880253868380 > > [ 236.872579] RBP: 88024acd7bc0 R08: 88024acd7be0 R09: > > > > [ 236.876407] R10: R11: R12: > > 880253868380 > > [ 236.880185] R13: 8802538684d0 R14: 880253868380 R15: > > 88024cd48e00 > > [ 236.883983] FS: 7f1646d1a740() GS:88025d00() > > knlGS: > > [ 236.890959] CS: 0010 DS: ES: CR0: 80050033 > > [ 236.894702] CR2: 880251360318 CR3: 00024ad21000 CR4: > > 001406f0 > > [ 236.898481] [] i915_gem_request_retire+0x1cd/0x230 > > [ 236.902439] [] i915_gem_request_alloc+0xa3/0x2f0 > > [ 236.906435] [] > > i915_gem_do_execbuffer.isra.41+0xb6d/0x18b0 > > [ 236.910434] [] i915_gem_execbuffer2+0x95/0x1e0 > > [ 236.914390] [] drm_ioctl+0x1e5/0x460 > > [ 236.918275] [] do_vfs_ioctl+0x8f/0x5c0 > > [ 236.922168] [] SyS_ioctl+0x3c/0x70 > > [ 236.926090] [] entry_SYSCALL_64_fastpath+0x17/0x93 > > [ 236.930045] [] 0x > > Ah something that I didn't take into account, and indeed gives this a bit > more urgency than I realised, is that the timestamp is exposed to > userspace. As such we are feeding it garbage, at best. > > The trivial option is just to clear it in dma_fence_init(). I still have > the slight preference for the extra complication here (for the reader) as > it should be quicker for the more common path of signaling the fence. > > > We only set the timestamp before we mark the fence as signaled. It is > > done before to avoid observers having a window in which they may see the > > fence as complete but no timestamp. Having it does incur a potential for > > the timestamp to be written twice, and even for it to be corrupted if > > the u64 write is not atomic. Instead use a new bit to record the > > presence of the timestamp, and teach the readers to wait until it is set > > if the fence is complete. There still remains a race where the timestamp > > for the signaled fence may be shown before the fence is reported as > > signaled, but that's a pre-existing error. > > Now deserves a > Reported-by: Rafael Antognolli > > > Signed-off-by: Chris Wilson > > Cc: Sumit Semwal > > Cc: Gustavo Padovan > > Cc: Daniel Vetter > Cc: Rafael Antognolli > > > --- > > drivers/dma-buf/dma-fence.c | 17 ++--- > > drivers/dma-buf/sync_debug.c | 2 +- > > drivers/dma-buf/sync_file.c | 8 +++- > > include/linux/dma-fence.h| 2 ++ > > 4 files changed, 16 insertions(+), 13 deletions(-) Finally pushed this one to drm-misc-fixes. Thanks. Gustavo ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 3/3] drm/i915: unregister interfaces first in unload
We first need to make sure no one else can get at us anymore, before we can proceed to tear down all the datastructures. Just a small step towards eventually the perfect unload code ... Signed-off-by: Daniel Vetter --- drivers/gpu/drm/i915/i915_drv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 3a6dc04bd51e..38990b264b97 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -1372,6 +1372,7 @@ void i915_driver_unload(struct drm_device *dev) struct drm_i915_private *dev_priv = to_i915(dev); struct pci_dev *pdev = dev_priv->drm.pdev; + i915_driver_unregister(dev_priv); if (i915_gem_suspend(dev_priv)) DRM_ERROR("failed to idle hardware; continuing to unload!\n"); @@ -1382,7 +1383,6 @@ void i915_driver_unload(struct drm_device *dev) intel_gvt_cleanup(dev_priv); - i915_driver_unregister(dev_priv); intel_modeset_cleanup(dev); /* -- 2.13.2 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 2/3] drm/i915: Fix fbdev unload sequence
First thing we need to do is unregister the fbdev instance, but we can't just go ahead and kfree it. That must wait until the hotplug and polling work are stopped, since they can race with the with the teardown. That means we need to split up the fbdev teardown into the unregister part and the cleanup part. I originally suspected that this was broken in one of the unload shuffles, but on closer inspection the oldest sequence I've dug out also gets this wrong. Just not quite so badly. I've run drv_module_reload a few hundred times and it's rock solid compared to insta-death beforehand. This bug seems to have been uncovered by commit 88be58be886f1215cc73dc8c273c985eecd7385c Author: Daniel Vetter Date: Thu Jul 6 15:00:19 2017 +0200 drm/i915/fbdev: Always forward hotplug events But the effect of that seems to only be to increase the race window enough to make it blow up easier. I'm not exactly clear on what's going on there ... Testcase: igt/drv_module_reload Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=101791 Cc: martin.pe...@free.fr Cc: ch...@chris-wilson.co.uk Signed-off-by: Daniel Vetter --- drivers/gpu/drm/i915/i915_drv.c | 3 +-- drivers/gpu/drm/i915/intel_display.c | 3 +++ drivers/gpu/drm/i915/intel_drv.h | 9 +++-- drivers/gpu/drm/i915/intel_fbdev.c | 21 +++-- 4 files changed, 26 insertions(+), 10 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index d310d8245dca..3a6dc04bd51e 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -1240,6 +1240,7 @@ static void i915_driver_register(struct drm_i915_private *dev_priv) */ static void i915_driver_unregister(struct drm_i915_private *dev_priv) { + intel_fbdev_unregister(dev_priv); intel_audio_deinit(dev_priv); intel_gpu_ips_teardown(); @@ -1371,7 +1372,6 @@ void i915_driver_unload(struct drm_device *dev) struct drm_i915_private *dev_priv = to_i915(dev); struct pci_dev *pdev = dev_priv->drm.pdev; - intel_fbdev_fini(dev); if (i915_gem_suspend(dev_priv)) DRM_ERROR("failed to idle hardware; continuing to unload!\n"); @@ -1383,7 +1383,6 @@ void i915_driver_unload(struct drm_device *dev) intel_gvt_cleanup(dev_priv); i915_driver_unregister(dev_priv); - intel_modeset_cleanup(dev); /* diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index ecb4e30673fc..e10fae2faa88 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -15111,6 +15111,9 @@ void intel_modeset_cleanup(struct drm_device *dev) */ drm_kms_helper_poll_fini(dev); + /* poll work can call into fbdev, hence clean that up afterwards */ + intel_fbdev_fini(dev_priv); + intel_unregister_dsm_handler(); intel_fbc_global_disable(dev_priv); diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 3132260f18ce..7361e983d953 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1597,7 +1597,8 @@ void intel_hpd_poll_init(struct drm_i915_private *dev_priv); #ifdef CONFIG_DRM_FBDEV_EMULATION extern int intel_fbdev_init(struct drm_device *dev); extern void intel_fbdev_initial_config_async(struct drm_device *dev); -extern void intel_fbdev_fini(struct drm_device *dev); +extern void intel_fbdev_unregister(struct drm_i915_private *dev_priv); +extern void intel_fbdev_fini(struct drm_i915_private *dev_priv); extern void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous); extern void intel_fbdev_output_poll_changed(struct drm_device *dev); extern void intel_fbdev_restore_mode(struct drm_device *dev); @@ -1611,7 +1612,11 @@ static inline void intel_fbdev_initial_config_async(struct drm_device *dev) { } -static inline void intel_fbdev_fini(struct drm_device *dev) +static inline void intel_fbdev_unregister(struct drm_i915_private *dev_priv) +{ +} + +static inline void intel_fbdev_fini(struct drm_i915_private *dev_priv) { } diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c index b953365a3eec..82280e104692 100644 --- a/drivers/gpu/drm/i915/intel_fbdev.c +++ b/drivers/gpu/drm/i915/intel_fbdev.c @@ -538,8 +538,6 @@ static void intel_fbdev_destroy(struct intel_fbdev *ifbdev) * trying to rectify all the possible error paths leading here. */ - drm_fb_helper_unregister_fbi(&ifbdev->helper); - drm_fb_helper_fini(&ifbdev->helper); if (ifbdev->vma) { @@ -727,8 +725,10 @@ static void intel_fbdev_initial_config(void *data, async_cookie_t cookie) /* Due to peculiar init order wrt to hpd handling this is separate. */ if (drm_fb_helper_initial_config(&ifbdev->helper, -ifbdev->preferred_bpp)) - intel_fbdev_fini(ifbdev-
[PATCH 1/3] drm/atomic-helper: Fix leak in disable_all
The legacy plane->fb pointer is refcounted by calling drm_atomic_clean_old_fb(). In practice this isn't a real problem because: - The caller in the i915 gpu reset code restores the original state again, which means the plane->fb pointer won't change, hence can't leak. - Drivers using drm_atomic_helper_shutdown call the fbdev cleanup first, and that usually cleans up the fb through drm_remove_framebuffer, which does this correctly. - Without fbdev the only framebuffers are from userspace, and those get cleaned up (again using drm_remove_framebuffer) befor the driver can even be unloaded. But in i915 I've switched the cleanup sequence around so that the _shutdown() calls happens after the drm_remove_framebuffer(), which is how I discovered this issue. Cc: martin.pe...@free.fr Cc: ch...@chris-wilson.co.uk Signed-off-by: Daniel Vetter --- drivers/gpu/drm/drm_atomic_helper.c | 5 + 1 file changed, 5 insertions(+) diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index b07fc30372d3..71b5f71e61e0 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -2726,6 +2726,7 @@ int drm_atomic_helper_disable_all(struct drm_device *dev, struct drm_plane *plane; struct drm_crtc_state *crtc_state; struct drm_crtc *crtc; + unsigned plane_mask = 0; int ret, i; state = drm_atomic_state_alloc(dev); @@ -2768,10 +2769,14 @@ int drm_atomic_helper_disable_all(struct drm_device *dev, goto free; drm_atomic_set_fb_for_plane(plane_state, NULL); + plane_mask |= BIT(drm_plane_index(plane)); + plane->old_fb = plane->fb; } ret = drm_atomic_commit(state); free: + if (plane_mask) + drm_atomic_clean_old_fb(dev, plane_mask, ret); drm_atomic_state_put(state); return ret; } -- 2.13.2 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] [PATCH 2/4] drm: Create a format/modifier blob
On Fri, Jul 14, 2017 at 11:41:49AM -0700, Ben Widawsky wrote: > On 17-06-29 22:49:44, Ville Syrjälä wrote: > > [snip] > > > > >... but here it's ALIGN(formats_offset+formats_size). I think we should > >be aligning the same thing in both cases, or we add a BUILD_BUG_ON to > >make sure the header size always stays a multiple of 8 bytes. > > > >That's mainly because the design of the structure seems geared towards > >expanding the header in the future (as in why else would you have the > >offsets?). > > > > I guess I don't quite understand what you're asking for. The first thing is > determining a size, the second is finding an offset into the blob. If I remember my thinking correctly, then what I was trying to say was ALIGN(multiple_of_4, 8) + ALIGN(multiple_of_4, 8) != ALIGN(multiple_of_4 + multiple_of_4, 8) but the code was assuming that it's true, or that at least one of those things is a multiple of 8 already. > I don't mind changing this, but tell me what you want. > > BUILD_BUG_ON sounds good to me regardless. > > [snip] > > -- > Ben Widawsky, Intel Open Source Technology Center -- Ville Syrjälä Intel OTC ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 00/14] YCBCR 4:2:0 handling in DRM layer
On Thu, Jul 13, 2017 at 09:03:06PM +0530, Shashank Sharma wrote: > Following YCBCR 4:4:4 and 4:2:2, YCBCR 4:2:0 is a new output format, > which is currently supported on HDMI 2.0 sources/sinks. Due to lower > chroma sub-sampling rate, YCBCR 4:2:0 can drive the video modes at half > the pixel clock than YCBCR 4:4:4 or RGB 8:8:8 outputs. For example, a CEA > 4K@60, RGB 8:8:8 mode needs a clock of appx 594Mhz, but it can be driven at > 297Mhz using YCBCR 4:2:0 output. > > Of course, the lower rate of chroma subsampling, causes the quality of YCBCR > 4:2:0 to be lower than YCBCR 4:4:4 or RGB 8:8:8. > > This patch series adds support for YCBCR 4:2:0 output in DRM layer. > > - First 2 patches, complete the CEA mode-db in drm driver, by adding > new 4k modes. Current CEA mode-db contains 64 modes only (VIC 1-64), > whereas CEA-861-F defined VICs up to 107, including 4k modes, from VIC > range 93-107. First patch makes sure that inclusion of these modes doesn't > break existing HDMI 1.4 monitors, across various drivers. > > - Next 5 patches focus on parsing new YCBCR 4:2:0 EDID blocks, and adding > YCBCR 4:2:0 modes in connector. They also contain a prune function, which > will cleanup the YCBCR 4:2:0 modes from list, if the connector doesn't > declare them supported. > > - Next 2 patches add helper functions for identifing YCBCR 4:2:0 modes and > setup the color space in AVI infoframes. > > - Next 6 patches add code for I915 layer handling of YCBCR 4:2:0 output. > > This patch series was initially published as a complete framework to handle > all YCBCR outputs (4:4:4, 4:2:2, 4:2:0), but based on the code reviews, now > its been published as YCBCR 4:2:0 handling series only. > > The previous discussion and reviews can be found here: > V5: https://patchwork.freedesktop.org/series/26815/ > V1-V4: https://patchwork.freedesktop.org/series/22683/ > > Now re-publishing this patch series as YCBCR 4:2:0 handling series here. > V2: Addressed review comments from Ville > This series dropped one patch in V2(patch 8 from V1), so 14 patches in V2 > > This series has been tested with drm-tip code with following setup: > Source: Intel Geminilake device. > Sink: ACER S277HK HDMI 2.0 monitor. > Protocol testing: Astro VA-1844A HDMI analyzer. > > Shashank Sharma (14): > drm: handle HDMI 2.0 VICs in AVI info-frames > drm/edid: complete CEA modedb(VIC 1-107) > drm/edid: parse sink information before CEA blocks > drm/edid: cleanup patch for CEA extended-tag macro > drm: add helper to validate YCBCR420 modes > drm/edid: parse YCBCR420 videomodes from EDID > drm/edid: parse ycbcr 420 deep color information > drm: add helper functions for YCBCR420 handling I just finished pushing the core bits into drm-misc-next. But I'm not super happy how that went because there were still quite a few trivial checkpatch warnings and indentation issues I had to fix up. All of that should have been dealt with before even submitting the patches to the list. Review bandwidth is a scarce resource so we don't want to squander it on this sort of stuff. Another issue I ran into when I tried to actually run the code. As soon as I loaded the driver a warning and the accompanying backtrace from the state checker flew past my terminal. That too should have been caught before even sending the patches to the list. So I think the next time someone asks me to review something I will start by asking these basic questions: - Did you configure your editor so that it automagically formats code correctly? - Did you check for new compiler/sparse/checkpatch warnings? - Did you check for new runtime errors/warnings from the driver? If the answer to any of those is "no", then I won't even bother reviewing anything. OK, that's enough ranting. Now for the more positive feedback. The code does work for me, for the mose part. There is some kind of initial problem though. When I first load the driver I get some kind of weird shifted grey screen. So I guess either we're misconfiguring something in the source, or we're not properly telling the sink what we're sending it. After another modeset the problem goes away and everything appears to work correctly. So congrats on making that happen \o/ > drm/i915: add config function for YCBCR420 outputs > drm/i915: prepare scaler for YCBCR420 modeset > drm/i915: prepare pipe for YCBCR420 output > drm/i915: prepare csc unit for YCBCR420 output > drm/i915: set colorspace for YCBCR420 outputs > drm/i915/glk: set HDMI 2.0 identifier These remaining i915 patches still have a few issues, the lack of state readout and the grey screen issue are the major ones. I've left more detailed feedback on the individual patches, but right now I don't have a clear idea what's causing the grey screen. Assuming the remaining issues I've highlighted get fixed, and the remaining patches won't get massively altered in the process, you can slap Reviewed-by: Ville Syrjälä onto the patches. > >
Re: [Intel-gfx] [PATCH 2/4] drm: Create a format/modifier blob
On 17-06-29 22:49:44, Ville Syrjälä wrote: [snip] ... but here it's ALIGN(formats_offset+formats_size). I think we should be aligning the same thing in both cases, or we add a BUILD_BUG_ON to make sure the header size always stays a multiple of 8 bytes. That's mainly because the design of the structure seems geared towards expanding the header in the future (as in why else would you have the offsets?). I guess I don't quite understand what you're asking for. The first thing is determining a size, the second is finding an offset into the blob. I don't mind changing this, but tell me what you want. BUILD_BUG_ON sounds good to me regardless. [snip] -- Ben Widawsky, Intel Open Source Technology Center ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 12/14] drm/i915: prepare csc unit for YCBCR420 output
On Thu, Jul 13, 2017 at 09:03:18PM +0530, Shashank Sharma wrote: > To support ycbcr output, we need a pipe CSC block to do > RGB->YCBCR conversion. > > Current Intel platforms have only one pipe CSC unit, so > we can either do color correction using it, or we can perform > RGB->YCBCR conversion. > > This function adds a csc handler, which uses recommended bspec > values to perform RGB->YCBCR conversion (target color space BT709) > > V2: Rebase > V3: Rebase > V4: Rebase > V5: Addressed review comments from Ander > - Remove extra line added in the patch > - Add the spec details in the commit message > - Combine two if(cond) while calling intel_crtc_compute_config > V6: Handle YCBCR420 outputs only (Ville) > V7: Addressed review comments from Ville: > - Add description about target colorspace > - Remove the comments from CSC function > - DRM_DEBUG->DEBUG_KMS for atomic failure due to CSC unit busy > - Remove unnecessary debug message about YCBCR420 possibe > > Cc: Ville Syrjala > Cc: Daniel Vetter > Cc: Ander Conselvan de Oliveira > > Signed-off-by: Shashank Sharma > --- > drivers/gpu/drm/i915/intel_color.c | 46 > +++- > drivers/gpu/drm/i915/intel_display.c | 15 > 2 files changed, 60 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_color.c > b/drivers/gpu/drm/i915/intel_color.c > index f85d575..8698691 100644 > --- a/drivers/gpu/drm/i915/intel_color.c > +++ b/drivers/gpu/drm/i915/intel_color.c > @@ -41,6 +41,22 @@ > > #define LEGACY_LUT_LENGTH(sizeof(struct drm_color_lut) * 256) > > +/* Post offset values for RGB->YCBCR conversion */ > +#define POSTOFF_RGB_TO_YUV_HI 0x800 > +#define POSTOFF_RGB_TO_YUV_ME 0x100 > +#define POSTOFF_RGB_TO_YUV_LO 0x800 > + > +/* > + * These values are direct register values specified in the Bspec, > + * for RGB->YUV conversion matrix (colorspace BT709) > + */ > +#define CSC_RGB_TO_YUV_RU_GU 0x2ba809d8 > +#define CSC_RGB_TO_YUV_BU 0x37e8 > +#define CSC_RGB_TO_YUV_RY_GY 0x1e089cc0 > +#define CSC_RGB_TO_YUV_BY 0xb528 > +#define CSC_RGB_TO_YUV_RV_GV 0xbce89ad8 > +#define CSC_RGB_TO_YUV_BV 0x1e08 > + > /* > * Extract the CSC coefficient from a CTM coefficient (in U32.32 fixed point > * format). This macro takes the coefficient we want transformed and the > @@ -91,6 +107,31 @@ static void ctm_mult_by_limited(uint64_t *result, int64_t > *input) > } > } > > +void i9xx_load_ycbcr_conversion_matrix(struct intel_crtc *intel_crtc) > +{ > + int pipe = intel_crtc->pipe; > + struct drm_i915_private *dev_priv = to_i915(intel_crtc->base.dev); > + > + > + I915_WRITE(PIPE_CSC_PREOFF_HI(pipe), 0); > + I915_WRITE(PIPE_CSC_PREOFF_ME(pipe), 0); > + I915_WRITE(PIPE_CSC_PREOFF_LO(pipe), 0); > + > + I915_WRITE(PIPE_CSC_COEFF_RU_GU(pipe), CSC_RGB_TO_YUV_RU_GU); > + I915_WRITE(PIPE_CSC_COEFF_BU(pipe), CSC_RGB_TO_YUV_BU); > + > + I915_WRITE(PIPE_CSC_COEFF_RY_GY(pipe), CSC_RGB_TO_YUV_RY_GY); > + I915_WRITE(PIPE_CSC_COEFF_BY(pipe), CSC_RGB_TO_YUV_BY); > + > + I915_WRITE(PIPE_CSC_COEFF_RV_GV(pipe), CSC_RGB_TO_YUV_RV_GV); > + I915_WRITE(PIPE_CSC_COEFF_BV(pipe), CSC_RGB_TO_YUV_BV); > + > + I915_WRITE(PIPE_CSC_POSTOFF_HI(pipe), POSTOFF_RGB_TO_YUV_HI); > + I915_WRITE(PIPE_CSC_POSTOFF_ME(pipe), POSTOFF_RGB_TO_YUV_ME); > + I915_WRITE(PIPE_CSC_POSTOFF_LO(pipe), POSTOFF_RGB_TO_YUV_LO); > + I915_WRITE(PIPE_CSC_MODE(pipe), 0); > +} > + > /* Set up the pipe CSC unit. */ > static void i9xx_load_csc_matrix(struct drm_crtc_state *crtc_state) > { > @@ -101,7 +142,10 @@ 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 (crtc_state->ctm) { > + if (intel_crtc_state->ycbcr420) { > + i9xx_load_ycbcr_conversion_matrix(intel_crtc); > + return; > + } else if (crtc_state->ctm) { > struct drm_color_ctm *ctm = > (struct drm_color_ctm *)crtc_state->ctm->data; > uint64_t input[9] = { 0, }; > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c > index 1a23ec0..9c6a1ed 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -6283,6 +6283,21 @@ static int intel_crtc_compute_config(struct intel_crtc > *crtc, > return -EINVAL; > } > > + /* YCBCR420 feasibility check */ Not a useful comment. > + if (pipe_config->ycbcr420) { > + struct drm_crtc_state *drm_state = &pipe_config->base; Having aliasing pointer for the same thing is just annoying. We do have quite a lot of that going around, but IMO we need to clean that all up at some point. So pls just do this instead: if (pipe_config->ycbcr420 && pipe_config->base.ctm) { ... } > + > + /* >
Re: [PATCH v2 11/14] drm/i915: prepare pipe for YCBCR420 output
On Thu, Jul 13, 2017 at 09:03:17PM +0530, Shashank Sharma wrote: > To get HDMI YCBCR420 output, the PIPEMISC register should be > programmed to: > - Generate YCBCR output (bit 11) > - In case of YCBCR420 outputs, it should be programmed in full > blend mode to use the scaler in 5x3 ratio (bits 26 and 27) > > This patch: > - Adds definition of these bits. > - Programs PIPEMISC for YCBCR420 outputs. > > V2: rebase > V3: rebase > V4: rebase > V5: added r-b from Ander > V6: Handle only YCBCR420 outputs (ville) > V7: rebase > > Cc: Ville Syrjala > Cc: Ander Conselvan de Oliveira > Cc: Daniel Vetter > > Reviewed-by: Ander Conselvan de Oliveira > Signed-off-by: Shashank Sharma > --- > drivers/gpu/drm/i915/i915_reg.h | 3 +++ > drivers/gpu/drm/i915/intel_display.c | 7 +++ > 2 files changed, 10 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index c712d01..e5020d6 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -5227,6 +5227,9 @@ enum { > > #define _PIPE_MISC_A 0x70030 > #define _PIPE_MISC_B 0x71030 > +#define PIPEMISC_YCBCR420_ENABLE (1<<27) > +#define PIPEMISC_YCBCR420_MODE_BLEND (1<<26) > +#define PIPEMISC_OUTPUT_YCBCR (1<<11) Please rename to match spec. So something like: PIPEMISC_YUV420_ENABLE (1<<27) PIPEMISC_YUV420_MODE_FULL_BLEND (1<<26) PIPEMISC_OUTPUT_COLORSPACE_YUV (1<<11) > #define PIPEMISC_DITHER_BPC_MASK (7<<5) > #define PIPEMISC_DITHER_8_BPC (0<<5) > #define PIPEMISC_DITHER_10_BPC (1<<5) > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c > index d78f1c2..1a23ec0 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -8076,6 +8076,7 @@ static void haswell_set_pipemisc(struct drm_crtc *crtc) > { > struct drm_i915_private *dev_priv = to_i915(crtc->dev); > struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > + struct intel_crtc_state *config = intel_crtc->config; > > if (IS_BROADWELL(dev_priv) || INTEL_INFO(dev_priv)->gen >= 9) { > u32 val = 0; > @@ -8101,6 +8102,12 @@ 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_YCBCR | > + PIPEMISC_YCBCR420_ENABLE | > + PIPEMISC_YCBCR420_MODE_BLEND; > + } I think we'll want two flags. One to specify whether we're outputting YCbCr and the other to indicate whether we need the 4:2:0 subsamling. So maybe something like bool ycbcr; bool ycbcr420; We also need state readout for this stuff. With those two flags I think we can do something like: if (IS_BDW || GEN >= 9) { tmp = READ(PIPEMISC); crtc_state->ycbcr = tmp & OUTPUT_YUV; if (IS_GLK || GEN >= 10) crtc_state->ycbcr420 = tmp & YUV420_ENABLE; } The other missing readout thing is adjustment of the clock. ddi_dotclock_get() will need to double the dotclock when we're outputting ycbcr420. Pls also add something along the lines of DRM_DEBUG_KMS("ycbcr: %i, ycbcr420: %i\n", ...); to intel_dump_pipe_config() so that we can actually tell when we're outputting YCbCr and 4:2:0. > + > I915_WRITE(PIPEMISC(intel_crtc->pipe), val); > } > } > -- > 2.7.4 -- Ville Syrjälä Intel OTC ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 09/14] drm/i915: add config function for YCBCR420 outputs
On Thu, Jul 13, 2017 at 09:03:15PM +0530, Shashank Sharma wrote: > This patch checks encoder level support for YCBCR420 outputs. > The logic goes as simple as this: > If the input mode is YCBCR420-only mode: prepare HDMI for > YCBCR420 output, else continue with RGB output mode. > > It checks if the mode is YCBCR420 and source can support this > output then it marks the ycbcr_420 output indicator into crtc > state, for further staging in driver. > > V2: Split the patch into two, kept helper functions in DRM layer. > V3: Changed the compute_config function based on new DRM API. > V4: Rebase > V5: Rebase > V6: Check and handle YCBCR420-only modes, discard the property > based approach (Ville) > V7: Addressed review comments from Ville > - add else case in 12BPC check. > - extract ycbcr420 state inside hdmi_12bpc_possible function. > > Cc: Ville Syrjala > Cc: Daniel Vetter > Cc: Ander Conselvan de Oliveira > > Signed-off-by: Shashank Sharma > --- > drivers/gpu/drm/i915/intel_display.c | 1 + > drivers/gpu/drm/i915/intel_drv.h | 3 +++ > drivers/gpu/drm/i915/intel_hdmi.c| 43 > +--- > 3 files changed, 44 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c > index 2144adc..a5140e4 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -11945,6 +11945,7 @@ intel_pipe_config_compare(struct drm_i915_private > *dev_priv, > PIPE_CONF_CHECK_I(hdmi_scrambling); > PIPE_CONF_CHECK_I(hdmi_high_tmds_clock_ratio); > PIPE_CONF_CHECK_I(has_infoframe); > + PIPE_CONF_CHECK_I(ycbcr420); > > PIPE_CONF_CHECK_I(has_audio); > > diff --git a/drivers/gpu/drm/i915/intel_drv.h > b/drivers/gpu/drm/i915/intel_drv.h > index d17a324..592243b 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -780,6 +780,9 @@ struct intel_crtc_state { > > /* HDMI High TMDS char rate ratio */ > bool hdmi_high_tmds_clock_ratio; > + > + /* HDMI output type */ We'll want DP too at some point. So that's going to get stale pretty soon. > + bool ycbcr420; > }; > > struct intel_crtc { > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c > b/drivers/gpu/drm/i915/intel_hdmi.c > index cc0d100..eb6243c 100644 > --- a/drivers/gpu/drm/i915/intel_hdmi.c > +++ b/drivers/gpu/drm/i915/intel_hdmi.c > @@ -1312,6 +1312,7 @@ static bool hdmi_12bpc_possible(struct intel_crtc_state > *crtc_state) > struct drm_atomic_state *state = crtc_state->base.state; > struct drm_connector_state *connector_state; > struct drm_connector *connector; > + bool output_ycbcr420 = crtc_state->ycbcr420; Pointless variable. Just use 'crtc_state->ycbcr420' in the code directly. > int i; > > if (HAS_GMCH_DISPLAY(dev_priv)) > @@ -1330,8 +1331,16 @@ static bool hdmi_12bpc_possible(struct > intel_crtc_state *crtc_state) > if (connector_state->crtc != crtc_state->base.crtc) > continue; > > - if ((info->edid_hdmi_dc_modes & DRM_EDID_HDMI_DC_36) == 0) > - return false; > + if (output_ycbcr420) { > + const struct drm_hdmi_info *hdmi = &info->hdmi; > + > + if (!(hdmi->y420_dc_modes & DRM_EDID_YCBCR420_DC_36)) > + return false; > + } else { > + Bogus whitespace. > + if (!(info->edid_hdmi_dc_modes & DRM_EDID_HDMI_DC_36)) > + return false; > + } > } > > /* Display Wa #1139 */ > @@ -1342,6 +1351,25 @@ static bool hdmi_12bpc_possible(struct > intel_crtc_state *crtc_state) > return true; > } > > +static bool > +intel_hdmi_ycbcr420_config(struct drm_connector *connector, > +struct intel_crtc_state *config, > +int *clock_12bpc, int *clock_8bpc) > +{ > + Bogus whitespace. > + if (!connector->ycbcr_420_allowed) { > + DRM_ERROR("Platform doesn't support YCBCR420 output\n"); > + return false; > + } > + > + /* YCBCR420 TMDS rate requirement is half the pixel clock */ > + config->port_clock /= 2; > + *clock_12bpc /= 2; > + *clock_8bpc /= 2; > + config->ycbcr420 = true; > + return true; > +} > + > bool intel_hdmi_compute_config(struct intel_encoder *encoder, > struct intel_crtc_state *pipe_config, > struct drm_connector_state *conn_state) > @@ -1349,7 +1377,8 @@ bool intel_hdmi_compute_config(struct intel_encoder > *encoder, > struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&encoder->base); > struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); > struct drm_display_mode *adjusted_mode = > &pipe_config->base.adjusted_mode; > - struct drm_scdc *scdc = &conn_
Re: [PATCH v2 10/14] drm/i915: prepare scaler for YCBCR420 modeset
On Thu, Jul 13, 2017 at 09:03:16PM +0530, Shashank Sharma wrote: > To get a YCBCR420 output from intel platforms, we need one > scaler to scale down YCBCR444 samples to YCBCR420 samples. > > This patch: > - Does scaler allocation for HDMI ycbcr420 outputs. > - Programs PIPE_MISC register for ycbcr420 output. > - Adds a new scaler user "HDMI output" to plug-into existing > scaler framework. This output type is identified using bit > 30 of the scaler users bitmap. > > V2: rebase > V3: rebase > V4: rebase > V5: addressed review comments from Ander: > - No need to check both scaler_user && hdmi_output. > Check for scaler_user is enough. > V6: rebase > V7: Do not create a new scaler user, use existing pipe scaler user. > > Cc: Ville Syrjala > Cc: Ander Conselvan De Oliveira > > Signed-off-by: Shashank Sharma > --- > drivers/gpu/drm/i915/intel_display.c | 3 +++ > drivers/gpu/drm/i915/intel_drv.h | 4 +++- > drivers/gpu/drm/i915/intel_hdmi.c| 12 > drivers/gpu/drm/i915/intel_panel.c | 3 ++- > 4 files changed, 20 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c > index a5140e4..d78f1c2 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -4624,6 +4624,9 @@ 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) > + need_scaling = true; > + > /* >* Scaling/fitting not supported in IF-ID mode in GEN9+ >* TODO: Interlace fetch mode doesn't support YUV420 planar formats. > diff --git a/drivers/gpu/drm/i915/intel_drv.h > b/drivers/gpu/drm/i915/intel_drv.h > index 592243b..94ea6ed 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -471,7 +471,8 @@ struct intel_crtc_scaler_state { >* >* If a bit is set, a user is using a scaler. >* Here user can be a plane or crtc as defined below: > - * bits 0-30 - plane (bit position is index from drm_plane_index) > + * bits 0-29 - plane (bit position is index from drm_plane_index) > + * bit 30- hdmi output That's no longer true. >* bit 31- crtc >* >* Instead of creating a new index to cover planes and crtc, using > @@ -484,6 +485,7 @@ struct intel_crtc_scaler_state { >* avilability. >*/ > #define SKL_CRTC_INDEX 31 > + Bogus whitespace change. > unsigned scaler_users; > > /* scaler used by crtc for panel fitting purpose */ > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c > b/drivers/gpu/drm/i915/intel_hdmi.c > index eb6243c..49f4fb8 100644 > --- a/drivers/gpu/drm/i915/intel_hdmi.c > +++ b/drivers/gpu/drm/i915/intel_hdmi.c > @@ -1356,6 +1356,7 @@ intel_hdmi_ycbcr420_config(struct drm_connector > *connector, > struct intel_crtc_state *config, > int *clock_12bpc, int *clock_8bpc) > { > + struct intel_crtc *intel_crtc = to_intel_crtc(config->base.crtc); > > if (!connector->ycbcr_420_allowed) { > DRM_ERROR("Platform doesn't support YCBCR420 output\n"); > @@ -1367,6 +1368,17 @@ intel_hdmi_ycbcr420_config(struct drm_connector > *connector, > *clock_12bpc /= 2; > *clock_8bpc /= 2; > config->ycbcr420 = true; > + > + /* YCBCR 420 output conversion needs a scaler */ > + if (skl_update_scaler_crtc(config)) { > + DRM_ERROR("Scaler allocation for output failed\n"); DRM_DEBUG_KMS > + return false; > + } > + > + /* Bind this scaler to pipe */ Unnecesary comment. > + intel_pch_panel_fitting(intel_crtc, config, > + DRM_MODE_SCALE_FULLSCREEN); > + > return true; > } > > diff --git a/drivers/gpu/drm/i915/intel_panel.c > b/drivers/gpu/drm/i915/intel_panel.c > index 96c2cbd..fd2e081 100644 > --- a/drivers/gpu/drm/i915/intel_panel.c > +++ b/drivers/gpu/drm/i915/intel_panel.c > @@ -110,7 +110,8 @@ intel_pch_panel_fitting(struct intel_crtc *intel_crtc, > > /* Native modes don't need fitting */ > if (adjusted_mode->crtc_hdisplay == pipe_config->pipe_src_w && > - adjusted_mode->crtc_vdisplay == pipe_config->pipe_src_h) > + adjusted_mode->crtc_vdisplay == pipe_config->pipe_src_h && > + !pipe_config->ycbcr420) > goto done; > > switch (fitting_mode) { > -- > 2.7.4 -- Ville Syrjälä Intel OTC ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Jetson TK1 & HDMI output in mainline kernel
Hi Mikko, Thierry, While setting up my Jetson TK1 using the mainline kernel I discovered that the HDMI output didn't work. After some more debugging I discovered that this commit 404bfb78daf3bedafb0bfab24947059575cbea3d (gpu: host1x: Add IOMMU support) was the culprit. As far as I understand it host1x_probe() calls iommu_attach_device(), which in turn tries to find the 'iommus' property in the DT. But the hdmi@5428 device has no such property and so iommu_attach_device() returns -ENODEV and the host1x_probe fails. After making this extremely ugly patch it all works again: diff --git a/drivers/gpu/host1x/dev.c b/drivers/gpu/host1x/dev.c index 2c58a390123a..683f3a5f382a 100644 --- a/drivers/gpu/host1x/dev.c +++ b/drivers/gpu/host1x/dev.c @@ -186,6 +186,11 @@ static int host1x_probe(struct platform_device *pdev) return -ENOMEM; err = iommu_attach_device(host->domain, &pdev->dev); + if (err == -ENODEV) { + iommu_domain_free(host->domain); + host->domain = NULL; + goto no_mmu; + } if (err) goto fail_free_domain; @@ -197,7 +202,7 @@ static int host1x_probe(struct platform_device *pdev) geometry->aperture_end >> order); host->iova_end = geometry->aperture_end; } - +no_mmu: err = host1x_channel_list_init(&host->channel_list, host->info->nb_channels); if (err) { My plan is to use my Jetson TK1 to upstream the Tegra CEC driver that I wrote, but a working HDMI output would help a lot :-) I'm not sure if my analysis of the cause is correct, but if someone can take a look then I would appreciate that! Regards, Hans ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 3/3] drm/ttm: add transparent huge page support for DMA allocations
On Thu, Jul 13, 2017 at 9:56 AM, Christian König wrote: > From: Christian König > > Try to allocate huge pages when it makes sense. > > Signed-off-by: Christian König > --- > drivers/gpu/drm/ttm/ttm_page_alloc_dma.c | 217 > --- > 1 file changed, 169 insertions(+), 48 deletions(-) > > diff --git a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c > b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c > index 2081e20..e51d3fd 100644 > --- a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c > +++ b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c > @@ -57,21 +57,25 @@ > #define NUM_PAGES_TO_ALLOC (PAGE_SIZE/sizeof(struct page *)) > #define SMALL_ALLOCATION 4 > #define FREE_ALL_PAGES (~0U) > +#define VADDR_FLAG_HUGE_POOL 1UL > > enum pool_type { > IS_UNDEFINED= 0, > IS_WC = 1 << 1, > IS_UC = 1 << 2, > IS_CACHED = 1 << 3, > - IS_DMA32= 1 << 4 > + IS_DMA32= 1 << 4, > + IS_HUGE = 1 << 5 > }; > > /* > - * The pool structure. There are usually six pools: > + * The pool structure. There are up to seven pools: > * - generic (not restricted to DMA32): > * - write combined, uncached, cached. > * - dma32 (up to 2^32 - so up 4GB): > * - write combined, uncached, cached. > + * - huge (not restricted to DMA32): > + * - cached. Any need for uncached or write combined huge pages? > * for each 'struct device'. The 'cached' is for pages that are actively > used. > * The other ones can be shrunk by the shrinker API if neccessary. > * @pools: The 'struct device->dma_pools' link. > @@ -111,13 +115,14 @@ struct dma_pool { > * The accounting page keeping track of the allocated page along with > * the DMA address. > * @page_list: The link to the 'page_list' in 'struct dma_pool'. > - * @vaddr: The virtual address of the page > + * @vaddr: The virtual address of the page and a flag if the page belongs to > a > + * huge pool > * @dma: The bus address of the page. If the page is not allocated > * via the DMA API, it will be -1. > */ > struct dma_page { > struct list_head page_list; > - void *vaddr; > + unsigned long vaddr; > struct page *p; > dma_addr_t dma; > }; > @@ -316,7 +321,8 @@ static int ttm_set_pages_caching(struct dma_pool *pool, > static void __ttm_dma_free_page(struct dma_pool *pool, struct dma_page > *d_page) > { > dma_addr_t dma = d_page->dma; > - dma_free_coherent(pool->dev, pool->size, d_page->vaddr, dma); > + d_page->vaddr &= ~VADDR_FLAG_HUGE_POOL; > + dma_free_coherent(pool->dev, pool->size, (void *)d_page->vaddr, dma); > > kfree(d_page); > d_page = NULL; > @@ -324,19 +330,22 @@ static void __ttm_dma_free_page(struct dma_pool *pool, > struct dma_page *d_page) > static struct dma_page *__ttm_dma_alloc_page(struct dma_pool *pool) > { > struct dma_page *d_page; > + void *vaddr; > > d_page = kmalloc(sizeof(struct dma_page), GFP_KERNEL); > if (!d_page) > return NULL; > > - d_page->vaddr = dma_alloc_coherent(pool->dev, pool->size, > - &d_page->dma, > - pool->gfp_flags); > - if (d_page->vaddr) { > - if (is_vmalloc_addr(d_page->vaddr)) > - d_page->p = vmalloc_to_page(d_page->vaddr); > + vaddr = dma_alloc_coherent(pool->dev, pool->size, &d_page->dma, > + pool->gfp_flags); > + if (vaddr) { > + if (is_vmalloc_addr(vaddr)) > + d_page->p = vmalloc_to_page(vaddr); > else > - d_page->p = virt_to_page(d_page->vaddr); > + d_page->p = virt_to_page(vaddr); > + d_page->vaddr = (unsigned long)vaddr; > + if (pool->type & IS_HUGE) > + d_page->vaddr |= VADDR_FLAG_HUGE_POOL; > } else { > kfree(d_page); > d_page = NULL; > @@ -368,11 +377,40 @@ static void ttm_pool_update_free_locked(struct dma_pool > *pool, > } > > /* set memory back to wb and free the pages. */ > +static void ttm_dma_page_put(struct dma_pool *pool, struct dma_page *d_page) > +{ > + struct page *page = d_page->p; > + unsigned i, num_pages; > + int ret; > + > + /* Don't set WB on WB page pool. */ > + if (!(pool->type & IS_CACHED)) { > + num_pages = pool->size / PAGE_SIZE; > + for (i = 0; i < num_pages; ++i, ++page) { > + ret = set_pages_array_wb(&page, 1); > + if (ret) { > + pr_err("%s: Failed to set %d pages to wb!\n", > + pool->dev_name, 1); > + } > + } > + } > + > + lis
Re: [regression drm/noveau] suspend to ram -> BOOM: exception RIP: drm_calc_vbltimestamp_from_scanoutpos+335
On Fri, Jul 14, 2017 at 05:58:18PM +0200, Mike Galbraith wrote: > On Fri, 2017-07-14 at 17:50 +0200, Peter Zijlstra wrote: > > Urgh, is for some mysterious reason the __bug_table section of modules > > ending up in RO memory? > > > > I forever get lost in that link magic :/ > > +1 > > drm.ko > 20 __bug_table 0630 0004bff3 2**0 > CONTENTS, ALLOC, LOAD, RELOC, READONLY, DATA > vmlinux > 15 __bug_table ba84 81af26c0 01af26c0 00cf26c0 2**0 > CONTENTS, ALLOC, LOAD, READONLY, DATA > > Danged if I know... um um RELOC business mucks things up? Argh, it shouldn't be READONLY for vmlinux either, but apparently that is working for mysterious reasons. Some architectures were in fact complaining that I broke that, and hence patch: b5effd3815cc ("debug: Fix __bug_table[] in arch linker scripts") I think we need professional help with this linking stuff, but who to ask? ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: Linux: Smooth splashscreen with system having weston with drm-backend
On Thu, Jul 13, 2017 at 03:50:56PM +0200, Daniel Vetter wrote: > On Thu, Jul 13, 2017 at 3:33 PM, Vikas Patil wrote: > > Dear All, > > > > I am looking for an solution to have early smooth splashscreen on the > > Linux system with Weston and drm-backend. > > > > I tried showing splashscreen using Linux logo and fbcon Linux features > > but it is not smooth as when system boots logo gets displayed via > > kernel and as the Weston starts > > I see flicker and blackscreen. > > > > Another approach I tried is having standalone drm api based > > application [1] which uses the one of the available hardware > > plane/overlay for displaying splash image and Weston is > > starting and uses default plane which is different from plane utilized > > by above application. but still I see flicker and black screen when > > Weston starts. > > > > I want splash to be displayed on one of the h/w plane and Weston > > should start in background without any flicker and black screen on > > some other plane. > > > > Anyone here before achieved such a use case with such system? Looking > > for inputs/suggestions/ideas to achieve this. It is ok if something > > needs to be hacked/changd at any level (kernel driver, weston). > > > > This is required on platform such as TI (Jacinto) and Intel (Broxton) > > or devices based on drm graphics stack. > > > > [1] > > http://git.ti.com/glsdk/example-applications/blobs/39080525baca7bf136f2412d62436366a736af6b/drm-tests/drm_z_alpha.c > > > > Thanking you all for your time and inputs in advance. > > If you make sure you have matching modes between the 2 users of drm > this should work. kms drivers (especially if they are already > converted to atomic) will automatically optimize transitions to not > flicker (if possible). > > If you still flicker then it's either a kernel driver issue, a hw > limitations (some hw needs a full modeset for e.g. changing the bit > depth) or you program two different things. > > Also, you must ensure that the boot-splash does not quit until weston > has fully taken over, otherwise kms will first shut down the screen > (when the splash quits), then re-enable it when weston starts. > This is what we do in Chrome OS with frecon [1]. It presents the splash screen until Chrome is ready to take over. [1]- https://chromium.googlesource.com/chromiumos/platform/frecon Sean > Cheers, Daniel > > > > > Thanks & Regards, > > Vikash > > ___ > > wayland-devel mailing list > > wayland-de...@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/wayland-devel > > > > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch > ___ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Sean Paul, Software Engineer, Google / Chromium OS ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Nouveau] [regression drm/noveau] suspend to ram -> BOOM: exception RIP: drm_calc_vbltimestamp_from_scanoutpos+335
On Fri, Jul 14, 2017 at 11:20:01AM -0400, Ilia Mirkin wrote: > On Fri, Jul 14, 2017 at 11:19 AM, Tobias Klausmann > wrote: > > The conversion is a nice catch, but i'd like to have a bit more context, see > > below! > > > > With a better description: > > > > Tobias Klausmann > > I don't think it was meant as a serious patch. WARN_ON_ONCE should > work. The fix isn't to remove all instances of WARN_ON_ONCE. The fix > is to fix WARN_ON_ONCE. Quite so. Clearly I buggered it for modules; that really wasn't the plan. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [regression drm/noveau] suspend to ram -> BOOM: exception RIP: drm_calc_vbltimestamp_from_scanoutpos+335
On Fri, Jul 14, 2017 at 03:36:08PM +0200, Mike Galbraith wrote: > Ok, a network outage gave me time to go hunting. Indeed it is a bad > interaction with the tree DRM merged into. All DRM did was to slip a > WARN_ON_ONCE() that nouveau triggers into a kernel module where such > things no longer warn, they blow the box out of the water. I made a > dinky testcase module (attached), and bisected to the real root > > 19d436268dde95389c616bb3819da73f0a8b28a8 is the first bad commit > commit 19d436268dde95389c616bb3819da73f0a8b28a8 > Author: Peter Zijlstra > Date: Sat Feb 25 08:56:53 2017 +0100 > > debug: Add _ONCE() logic to report_bug() Urgh, is for some mysterious reason the __bug_table section of modules ending up in RO memory? I forever get lost in that link magic :/ ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/3] drm/ttm: cleanup ttm_page_alloc_dma.c
On Thu, Jul 13, 2017 at 9:56 AM, Christian König wrote: > From: Christian König > > Remove unused defines and variables. Also stop computing the > gfp_flags when they aren't used. > > No intended functional change. > > Signed-off-by: Christian König Reviewed-by: Alex Deucher > --- > drivers/gpu/drm/ttm/ttm_page_alloc_dma.c | 42 > > 1 file changed, 16 insertions(+), 26 deletions(-) > > diff --git a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c > b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c > index 6c38046..2081e20 100644 > --- a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c > +++ b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c > @@ -57,22 +57,15 @@ > #define NUM_PAGES_TO_ALLOC (PAGE_SIZE/sizeof(struct page *)) > #define SMALL_ALLOCATION 4 > #define FREE_ALL_PAGES (~0U) > -/* times are in msecs */ > -#define IS_UNDEFINED (0) > -#define IS_WC (1<<1) > -#define IS_UC (1<<2) > -#define IS_CACHED (1<<3) > -#define IS_DMA32 (1<<4) > > enum pool_type { > - POOL_IS_UNDEFINED, > - POOL_IS_WC = IS_WC, > - POOL_IS_UC = IS_UC, > - POOL_IS_CACHED = IS_CACHED, > - POOL_IS_WC_DMA32 = IS_WC | IS_DMA32, > - POOL_IS_UC_DMA32 = IS_UC | IS_DMA32, > - POOL_IS_CACHED_DMA32 = IS_CACHED | IS_DMA32, > + IS_UNDEFINED= 0, > + IS_WC = 1 << 1, > + IS_UC = 1 << 2, > + IS_CACHED = 1 << 3, > + IS_DMA32= 1 << 4 > }; > + > /* > * The pool structure. There are usually six pools: > * - generic (not restricted to DMA32): > @@ -83,11 +76,9 @@ enum pool_type { > * The other ones can be shrunk by the shrinker API if neccessary. > * @pools: The 'struct device->dma_pools' link. > * @type: Type of the pool > - * @lock: Protects the inuse_list and free_list from concurrnet access. Must > be > + * @lock: Protects the free_list from concurrnet access. Must be > * used with irqsave/irqrestore variants because pool allocator maybe called > * from delayed work. > - * @inuse_list: Pool of pages that are in use. The order is very important > and > - * it is in the order that the TTM pages that are put back are in. > * @free_list: Pool of pages that are free to be used. No order requirements. > * @dev: The device that is associated with these pools. > * @size: Size used during DMA allocation. > @@ -104,7 +95,6 @@ struct dma_pool { > struct list_head pools; /* The 'struct device->dma_pools link */ > enum pool_type type; > spinlock_t lock; > - struct list_head inuse_list; > struct list_head free_list; > struct device *dev; > unsigned size; > @@ -606,7 +596,6 @@ static struct dma_pool *ttm_dma_pool_init(struct device > *dev, gfp_t flags, > sec_pool->pool = pool; > > INIT_LIST_HEAD(&pool->free_list); > - INIT_LIST_HEAD(&pool->inuse_list); > INIT_LIST_HEAD(&pool->pools); > spin_lock_init(&pool->lock); > pool->dev = dev; > @@ -879,22 +868,23 @@ int ttm_dma_populate(struct ttm_dma_tt *ttm_dma, struct > device *dev) > struct dma_pool *pool; > enum pool_type type; > unsigned i; > - gfp_t gfp_flags; > int ret; > > if (ttm->state != tt_unpopulated) > return 0; > > type = ttm_to_type(ttm->page_flags, ttm->caching_state); > - if (ttm->page_flags & TTM_PAGE_FLAG_DMA32) > - gfp_flags = GFP_USER | GFP_DMA32; > - else > - gfp_flags = GFP_HIGHUSER; > - if (ttm->page_flags & TTM_PAGE_FLAG_ZERO_ALLOC) > - gfp_flags |= __GFP_ZERO; > - > pool = ttm_dma_find_pool(dev, type); > if (!pool) { > + gfp_t gfp_flags; > + > + if (ttm->page_flags & TTM_PAGE_FLAG_DMA32) > + gfp_flags = GFP_USER | GFP_DMA32; > + else > + gfp_flags = GFP_HIGHUSER; > + if (ttm->page_flags & TTM_PAGE_FLAG_ZERO_ALLOC) > + gfp_flags |= __GFP_ZERO; > + > pool = ttm_dma_pool_init(dev, gfp_flags, type); > if (IS_ERR_OR_NULL(pool)) { > return -ENOMEM; > -- > 2.7.4 > > ___ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/3] drm/ttm: add support for different pool sizes
On Thu, Jul 13, 2017 at 9:56 AM, Christian König wrote: > From: Christian König > > Correctly handle different page sizes in the memory accounting. > > Signed-off-by: Christian König I'd split this patch in two, one to remove the no_wait and interruptible flags and one to add the size parameter. Alex > --- > drivers/gpu/drm/ttm/ttm_memory.c | 12 +--- > drivers/gpu/drm/ttm/ttm_page_alloc.c | 4 ++-- > drivers/gpu/drm/ttm/ttm_page_alloc_dma.c | 6 +++--- > include/drm/ttm/ttm_memory.h | 5 ++--- > 4 files changed, 12 insertions(+), 15 deletions(-) > > diff --git a/drivers/gpu/drm/ttm/ttm_memory.c > b/drivers/gpu/drm/ttm/ttm_memory.c > index 29855be..e963749 100644 > --- a/drivers/gpu/drm/ttm/ttm_memory.c > +++ b/drivers/gpu/drm/ttm/ttm_memory.c > @@ -546,8 +546,7 @@ int ttm_mem_global_alloc(struct ttm_mem_global *glob, > uint64_t memory, > EXPORT_SYMBOL(ttm_mem_global_alloc); > > int ttm_mem_global_alloc_page(struct ttm_mem_global *glob, > - struct page *page, > - bool no_wait, bool interruptible) > + struct page *page, uint64_t size) > { > > struct ttm_mem_zone *zone = NULL; > @@ -564,11 +563,11 @@ int ttm_mem_global_alloc_page(struct ttm_mem_global > *glob, > if (glob->zone_dma32 && page_to_pfn(page) > 0x0010UL) > zone = glob->zone_kernel; > #endif > - return ttm_mem_global_alloc_zone(glob, zone, PAGE_SIZE, no_wait, > -interruptible); > + return ttm_mem_global_alloc_zone(glob, zone, size, false, false); > } > > -void ttm_mem_global_free_page(struct ttm_mem_global *glob, struct page *page) > +void ttm_mem_global_free_page(struct ttm_mem_global *glob, struct page *page, > + uint64_t size) > { > struct ttm_mem_zone *zone = NULL; > > @@ -579,10 +578,9 @@ void ttm_mem_global_free_page(struct ttm_mem_global > *glob, struct page *page) > if (glob->zone_dma32 && page_to_pfn(page) > 0x0010UL) > zone = glob->zone_kernel; > #endif > - ttm_mem_global_free_zone(glob, zone, PAGE_SIZE); > + ttm_mem_global_free_zone(glob, zone, size); > } > > - > size_t ttm_round_pot(size_t size) > { > if ((size & (size - 1)) == 0) > diff --git a/drivers/gpu/drm/ttm/ttm_page_alloc.c > b/drivers/gpu/drm/ttm/ttm_page_alloc.c > index a37de5d..6add044 100644 > --- a/drivers/gpu/drm/ttm/ttm_page_alloc.c > +++ b/drivers/gpu/drm/ttm/ttm_page_alloc.c > @@ -880,7 +880,7 @@ int ttm_pool_populate(struct ttm_tt *ttm) > } > > ret = ttm_mem_global_alloc_page(mem_glob, ttm->pages[i], > - false, false); > + PAGE_SIZE); > if (unlikely(ret != 0)) { > ttm_pool_unpopulate(ttm); > return -ENOMEM; > @@ -907,7 +907,7 @@ void ttm_pool_unpopulate(struct ttm_tt *ttm) > for (i = 0; i < ttm->num_pages; ++i) { > if (ttm->pages[i]) { > ttm_mem_global_free_page(ttm->glob->mem_glob, > -ttm->pages[i]); > +ttm->pages[i], PAGE_SIZE); > ttm_put_pages(&ttm->pages[i], 1, > ttm->page_flags, > ttm->caching_state); > diff --git a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c > b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c > index cec4b4b..6c38046 100644 > --- a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c > +++ b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c > @@ -910,7 +910,7 @@ int ttm_dma_populate(struct ttm_dma_tt *ttm_dma, struct > device *dev) > } > > ret = ttm_mem_global_alloc_page(mem_glob, ttm->pages[i], > - false, false); > + pool->size); > if (unlikely(ret != 0)) { > ttm_dma_unpopulate(ttm_dma, dev); > return -ENOMEM; > @@ -975,13 +975,13 @@ void ttm_dma_unpopulate(struct ttm_dma_tt *ttm_dma, > struct device *dev) > if (is_cached) { > list_for_each_entry_safe(d_page, next, &ttm_dma->pages_list, > page_list) { > ttm_mem_global_free_page(ttm->glob->mem_glob, > -d_page->p); > +d_page->p, pool->size); > ttm_dma_page_put(pool, d_page); > } > } else { > for (i = 0; i < count; i++) { > ttm_mem_global_free_page(ttm->glob->mem_glob, > -ttm->pages[i]); > +
Re: [Nouveau] [regression drm/noveau] suspend to ram -> BOOM: exception RIP: drm_calc_vbltimestamp_from_scanoutpos+335
On Fri, Jul 14, 2017 at 11:19 AM, Tobias Klausmann wrote: > The conversion is a nice catch, but i'd like to have a bit more context, see > below! > > With a better description: > > Tobias Klausmann I don't think it was meant as a serious patch. WARN_ON_ONCE should work. The fix isn't to remove all instances of WARN_ON_ONCE. The fix is to fix WARN_ON_ONCE. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH i-g-t v2 7/7] kms_writeback: Add tests using a cloned output
From: Brian Starkey Update the connector search to also optionally attempt to find a non-writeback connector to clone to. Add a subtest which is the same as writeback-check-output, but also clones to the second connector. Signed-off-by: Brian Starkey --- tests/kms_writeback.c | 63 --- 1 file changed, 55 insertions(+), 8 deletions(-) diff --git a/tests/kms_writeback.c b/tests/kms_writeback.c index 8201a81c..9a34bca0 100644 --- a/tests/kms_writeback.c +++ b/tests/kms_writeback.c @@ -81,7 +81,8 @@ static uint32_t pick_writeback_format(igt_output_t *output) return format; } -static bool check_writeback_config(igt_display_t *display, igt_output_t *output) +static bool check_writeback_config(igt_display_t *display, igt_output_t *output, + int pipe, igt_output_t **clone) { igt_fb_t input_fb, output_fb; igt_plane_t *plane; @@ -123,6 +124,27 @@ static bool check_writeback_config(igt_display_t *display, igt_output_t *output) ret = igt_display_try_commit_atomic(display, DRM_MODE_ATOMIC_TEST_ONLY | DRM_MODE_ATOMIC_ALLOW_MODESET, NULL); + if (!ret && clone) { + /* Try and find a clone */ + int i, newret; + *clone = NULL; + + for (i = 0; i < display->n_outputs; i++) { + igt_output_t *second_output = &display->outputs[i]; + if (output != second_output && + igt_pipe_connector_valid(pipe, second_output)) { + + igt_output_clone_pipe(second_output, pipe); + newret = igt_display_try_commit_atomic(display, DRM_MODE_ATOMIC_TEST_ONLY | + DRM_MODE_ATOMIC_ALLOW_MODESET, NULL); + igt_output_set_pipe(second_output, PIPE_NONE); + if (!newret) { + *clone = second_output; + break; + } + } + } + } igt_plane_set_fb(plane, NULL); igt_remove_fb(display->drm_fd, &input_fb); igt_remove_fb(display->drm_fd, &output_fb); @@ -130,7 +152,8 @@ static bool check_writeback_config(igt_display_t *display, igt_output_t *output) return !ret; } -static igt_output_t *kms_writeback_get_output(igt_display_t *display) +static igt_output_t *kms_writeback_get_output(igt_display_t *display, enum pipe *pipe, + igt_output_t **clone) { int i; @@ -146,10 +169,16 @@ static igt_output_t *kms_writeback_get_output(igt_display_t *display) for (j = 0; j < igt_display_get_n_pipes(display); j++) { igt_output_set_pipe(output, j); - if (check_writeback_config(display, output)) { + if (check_writeback_config(display, output, j, clone)) { igt_debug("Using connector %u:%s on pipe %d\n", output->config.connector->connector_id, output->name, j); + if (clone && *clone) + igt_debug("Cloning to connector %u:%s\n", + (*clone)->config.connector->connector_id, + (*clone)->name); + if (pipe) + *pipe = j; return output; } } @@ -190,9 +219,6 @@ static int do_writeback_test(igt_output_t *output, uint32_t flags, igt_pipe_t *pipe_obj = &display->pipes[pipe]; igt_plane_t *plane; - /* -* Add CRTC Properties to the property set -*/ igt_atomic_prepare_crtc_commit(pipe_obj, req); for_each_plane_on_pipe(display, pipe, plane) { @@ -391,10 +417,11 @@ static void writeback_check_output(igt_output_t *output, igt_plane_t *plane, igt_main { igt_display_t display; - igt_output_t *output; + igt_output_t *output, *clone; igt_plane_t *plane; igt_fb_t input_fb; drmModeModeInfo mode; + enum pipe pipe; int ret; memset(&display, 0, sizeof(display)); @@ -409,7 +436,7 @@ igt_main igt_require(display.is_atomic); - output = kms_writeback_get_output(&display); + output = kms_writeback_get_output(&display, &pipe, &clone); igt_require(output); if (output->use_override_mode) @@ -487,6 +514,26 @@ igt_main igt_remove_fb(d
[PATCH i-g-t v2 2/7] lib/igt_kms: Add writeback support in lib/
From: Brian Starkey Add support in igt_kms for Writeback connectors, with the ability to attach framebuffers and retrieve fences. Signed-off-by: Brian Starkey --- lib/igt_aux.c | 1 + lib/igt_kms.c | 76 ++- lib/igt_kms.h | 16 + 3 files changed, 92 insertions(+), 1 deletion(-) diff --git a/lib/igt_aux.c b/lib/igt_aux.c index 86a213c2..ad80452a 100644 --- a/lib/igt_aux.c +++ b/lib/igt_aux.c @@ -1106,6 +1106,7 @@ static const struct type_name connector_type_names[] = { { DRM_MODE_CONNECTOR_eDP, "eDP" }, { DRM_MODE_CONNECTOR_VIRTUAL, "Virtual" }, { DRM_MODE_CONNECTOR_DSI, "DSI" }, + { DRM_MODE_CONNECTOR_WRITEBACK, "Writeback" }, {} }; diff --git a/lib/igt_kms.c b/lib/igt_kms.c index 6390229f..02e2b274 100644 --- a/lib/igt_kms.c +++ b/lib/igt_kms.c @@ -186,7 +186,10 @@ const char *igt_crtc_prop_names[IGT_NUM_CRTC_PROPS] = { const char *igt_connector_prop_names[IGT_NUM_CONNECTOR_PROPS] = { "scaling mode", - "CRTC_ID" + "CRTC_ID", + "WRITEBACK_PIXEL_FORMATS", + "WRITEBACK_FB_ID", + "WRITEBACK_OUT_FENCE_PTR" }; /* @@ -1832,6 +1835,7 @@ void igt_display_init(igt_display_t *display, int drm_fd) output->pending_crtc_idx_mask = 0; output->id = resources->connectors[i]; output->display = display; + output->writeback_out_fence_fd = -1; igt_output_refresh(output); @@ -1899,6 +1903,42 @@ igt_output_t *igt_output_from_connector(igt_display_t *display, return found; } +void igt_output_set_writeback_fb(igt_output_t *output, struct igt_fb *fb) +{ + igt_display_t *display = output->display; + struct kmstest_connector_config *config = &output->config; + + if (config->connector->connector_type != DRM_MODE_CONNECTOR_WRITEBACK) + return; + + LOG(display, "%s: output_set_writeback_fb(%d)\n", output->name, + fb ? fb->fb_id : 0); + + output->writeback_fb = fb; +} + +static void igt_output_reset_writeback_out_fence(igt_output_t *output) +{ + if (output->writeback_out_fence_fd >= 0) { + close(output->writeback_out_fence_fd); + output->writeback_out_fence_fd = -1; + } +} + +void igt_output_request_writeback_out_fence(igt_output_t *output) +{ + igt_output_reset_writeback_out_fence(output); + output->writeback_out_fence_requested = true; +} + +int igt_output_get_last_writeback_out_fence(igt_output_t *output) +{ + int fd = output->writeback_out_fence_fd; + output->writeback_out_fence_fd = -1; + + return fd; +} + static void igt_pipe_fini(igt_pipe_t *pipe) { int i; @@ -1922,6 +1962,8 @@ static void igt_pipe_fini(igt_pipe_t *pipe) static void igt_output_fini(igt_output_t *output) { kmstest_free_connector_config(&output->config); + if (output->writeback_out_fence_fd >= 0) + close(output->writeback_out_fence_fd); free(output->name); output->name = NULL; } @@ -2541,10 +2583,41 @@ static void igt_atomic_prepare_connector_commit(igt_output_t *output, drmModeAto igt_atomic_populate_connector_req(req, output, IGT_CONNECTOR_CRTC_ID, crtc_id); } + + if (output->writeback_fb) { + igt_atomic_populate_connector_req(req, output, IGT_CONNECTOR_WRITEBACK_FB_ID, output->writeback_fb->fb_id); + output->writeback_fb = NULL; + } + + igt_output_reset_writeback_out_fence(output); + if (output->writeback_out_fence_requested) { + igt_atomic_populate_connector_req(req, output, IGT_CONNECTOR_WRITEBACK_OUT_FENCE_PTR, + (uint64_t)(uintptr_t)&output->writeback_out_fence_fd); + } + /* * TODO: Add all other connector level properties here */ +} + +static void handle_writeback_out_fences(igt_display_t *display, uint32_t flags, int ret) +{ + int i; + for (i = 0; i < display->n_outputs; i++) { + igt_output_t *output = &display->outputs[i]; + + if (!output->config.connector) + continue; + + if (!output->writeback_out_fence_requested) + continue; + + output->writeback_out_fence_requested = false; + + if (ret || (flags & DRM_MODE_ATOMIC_TEST_ONLY)) + igt_assert(output->writeback_out_fence_fd == -1); + } } /* @@ -2593,6 +2666,7 @@ static int igt_atomic_commit(igt_display_t *display, uint32_t flags, void *user_ } ret = drmModeAtomicCommit(display->drm_fd, req, flags, user_data); + handle_writeback_out_fences(display, flags, ret); drmModeAtomicFree(req); return ret; diff --git a/lib/igt_kms.h b/lib/igt_kms.h index 35428f3e..ce9a35ef 100644 --- a/lib/igt_kms.h +++ b/lib/ig
[PATCH i-g-t v2 3/7] kms_writeback: Add initial writeback tests
From: Brian Starkey Add tests for the WRITEBACK_PIXEL_FORMATS, WRITEBACK_OUT_FENCE_PTR and WRITEBACK_FB_ID properties on writeback connectors, ensuring their behaviour is correct. Signed-off-by: Brian Starkey --- lib/igt_kms.c | 6 +- lib/igt_kms.h | 7 + tests/Makefile.sources | 1 + tests/kms_writeback.c | 371 + 4 files changed, 382 insertions(+), 3 deletions(-) create mode 100644 tests/kms_writeback.c diff --git a/lib/igt_kms.c b/lib/igt_kms.c index 02e2b274..aae32202 100644 --- a/lib/igt_kms.c +++ b/lib/igt_kms.c @@ -2137,7 +2137,7 @@ static uint32_t igt_plane_get_fb_gem_handle(igt_plane_t *plane) /* * Add position and fb changes of a plane to the atomic property set */ -static void +void igt_atomic_prepare_plane_commit(igt_plane_t *plane, igt_pipe_t *pipe, drmModeAtomicReq *req) { @@ -2515,7 +2515,7 @@ igt_pipe_replace_blob(igt_pipe_t *pipe, uint64_t *blob, void *ptr, size_t length /* * Add crtc property changes to the atomic property set */ -static void igt_atomic_prepare_crtc_commit(igt_pipe_t *pipe_obj, drmModeAtomicReq *req) +void igt_atomic_prepare_crtc_commit(igt_pipe_t *pipe_obj, drmModeAtomicReq *req) { if (pipe_obj->background_changed) igt_atomic_populate_crtc_req(req, pipe_obj, IGT_CRTC_BACKGROUND, pipe_obj->background); @@ -2567,7 +2567,7 @@ static void igt_atomic_prepare_crtc_commit(igt_pipe_t *pipe_obj, drmModeAtomicRe /* * Add connector property changes to the atomic property set */ -static void igt_atomic_prepare_connector_commit(igt_output_t *output, drmModeAtomicReq *req) +void igt_atomic_prepare_connector_commit(igt_output_t *output, drmModeAtomicReq *req) { struct kmstest_connector_config *config = &output->config; diff --git a/lib/igt_kms.h b/lib/igt_kms.h index ce9a35ef..ab8ec764 100644 --- a/lib/igt_kms.h +++ b/lib/igt_kms.h @@ -533,6 +533,8 @@ static inline bool igt_output_is_connected(igt_output_t *output) #define igt_atomic_populate_plane_req(req, plane, prop, value) \ igt_assert_lt(0, drmModeAtomicAddProperty(req, plane->drm_plane->plane_id,\ plane->atomic_props_plane[prop], value)) +void igt_atomic_prepare_plane_commit(igt_plane_t *plane, igt_pipe_t *pipe, + drmModeAtomicReq *req); /** * igt_atomic_populate_crtc_req: @@ -544,6 +546,9 @@ static inline bool igt_output_is_connected(igt_output_t *output) #define igt_atomic_populate_crtc_req(req, pipe, prop, value) \ igt_assert_lt(0, drmModeAtomicAddProperty(req, pipe->crtc_id,\ pipe->atomic_props_crtc[prop], value)) + +void igt_atomic_prepare_crtc_commit(igt_pipe_t *pipe_obj, drmModeAtomicReq *req); + /** * igt_atomic_populate_connector_req: * @req: A pointer to drmModeAtomicReq @@ -555,6 +560,8 @@ static inline bool igt_output_is_connected(igt_output_t *output) igt_assert_lt(0, drmModeAtomicAddProperty(req, output->config.connector->connector_id,\ output->config.atomic_props_connector[prop], value)) +void igt_atomic_prepare_connector_commit(igt_output_t *output, drmModeAtomicReq *req); + void igt_enable_connectors(void); void igt_reset_connectors(void); diff --git a/tests/Makefile.sources b/tests/Makefile.sources index 5b98a5a3..7318855d 100644 --- a/tests/Makefile.sources +++ b/tests/Makefile.sources @@ -213,6 +213,7 @@ TESTS_progs = \ kms_tv_load_detect \ kms_universal_plane \ kms_vblank \ + kms_writeback \ meta_test \ perf \ pm_backlight \ diff --git a/tests/kms_writeback.c b/tests/kms_writeback.c new file mode 100644 index ..d2066482 --- /dev/null +++ b/tests/kms_writeback.c @@ -0,0 +1,371 @@ +/* + * (C) COPYRIGHT 2017 ARM Limited. All rights reserved. + * + * Permission is hereby granted, free of charge, to any person obtaining a + * copy of this software and associated documentation files (the "Software"), + * to deal in the Software without restriction, including without limitation + * the rights to use, copy, modify, merge, publish, distribute, sublicense, + * and/or sell copies of the Software, and to permit persons to whom the + * Software is furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice (including the next + * paragraph) shall be included in all copies or substantial portions of the + * Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE O
[PATCH i-g-t v2 4/7] lib: Add function to hash a framebuffer
From: Brian Starkey To use writeback buffers as a CRC source, we need to be able to hash them. Implement a simple FVA-1a hashing routine for this purpose. Doing a bytewise hash on the framebuffer directly can be very slow if the memory is noncached. By making a copy of each line in the FB first (which can take advantage of word-access speedup), we can do the hash on a cached copy, which is much faster (10x speedup on my platform). Signed-off-by: Brian Starkey --- lib/igt_fb.c | 65 lib/igt_fb.h | 5 + 2 files changed, 70 insertions(+) diff --git a/lib/igt_fb.c b/lib/igt_fb.c index d2b7e9e3..f613bb2e 100644 --- a/lib/igt_fb.c +++ b/lib/igt_fb.c @@ -1302,3 +1302,68 @@ void igt_get_all_cairo_formats(const uint32_t **formats, int *format_count) *formats = drm_formats; *format_count = n_formats; } + +/* + * This implements the FNV-1a hashing algorithm instead of CRC, for + * simplicity + * http://www.isthe.com/chongo/tech/comp/fnv/index.html + * + * hash = offset_basis + * for each octet_of_data to be hashed + * hash = hash xor octet_of_data + * hash = hash * FNV_prime + * return hash + * + * 32 bit offset_basis = 2166136261 + * 32 bit FNV_prime = 224 + 28 + 0x93 = 16777619 + */ +int igt_fb_get_crc(struct igt_fb *fb, igt_crc_t *crc) +{ +#define FNV1a_OFFSET_BIAS 2166136261 +#define FNV1a_PRIME 16777619 + uint32_t hash; + void *map; + char *ptr, *line = NULL; + int x, y, cpp = igt_drm_format_to_bpp(fb->drm_format) / 8; + + if (fb->is_dumb) + map = kmstest_dumb_map_buffer(fb->fd, fb->gem_handle, fb->size, + PROT_READ); + else + map = gem_mmap__gtt(fb->fd, fb->gem_handle, fb->size, + PROT_READ); + ptr = map; + + /* +* Framebuffers are often uncached, which can make byte-wise accesses +* very slow. We copy each line of the FB into a local buffer to speed +* up the hashing. +*/ + line = malloc(fb->stride); + if (!line) { + munmap(map, fb->size); + return -ENOMEM; + } + + hash = FNV1a_OFFSET_BIAS; + + for (y = 0; y < fb->height; y++, ptr += fb->stride) { + + memcpy(line, ptr, fb->stride); + + for (x = 0; x < fb->width * cpp; x++) { + hash ^= line[x]; + hash *= FNV1a_PRIME; + } + } + + crc->n_words = 1; + crc->crc[0] = hash; + + free(line); + munmap(map, fb->size); + + return 0; +#undef FNV1a_OFFSET_BIAS +#undef FNV1a_PRIME +} diff --git a/lib/igt_fb.h b/lib/igt_fb.h index 4a680cef..04430e9d 100644 --- a/lib/igt_fb.h +++ b/lib/igt_fb.h @@ -43,6 +43,8 @@ typedef struct _cairo cairo_t; #include +#include "igt_crc.h" + /** * igt_fb_t: * @fb_id: KMS ID of the framebuffer @@ -156,5 +158,8 @@ uint32_t igt_drm_format_to_bpp(uint32_t drm_format); const char *igt_format_str(uint32_t drm_format); void igt_get_all_cairo_formats(const uint32_t **formats, int *format_count); +/* Get a hash for a framebuffer */ +int igt_fb_get_crc(struct igt_fb *fb, igt_crc_t *crc); + #endif /* __IGT_FB_H__ */ -- 2.13.2 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH i-g-t v2 1/7] igt: lib/igt_crc: Split out CRC functionality
From: Brian Starkey Separate out the CRC code for better compartmentalisation. Should ease the addition of more/different CRC sources in the future. Signed-off-by: Brian Starkey --- lib/Makefile.sources | 2 + lib/igt_chamelium.h | 1 + lib/igt_crc.c | 563 ++ lib/igt_crc.h | 125 + lib/igt_debugfs.c | 547 lib/igt_debugfs.h | 81 -- tests/chamelium.c | 1 + tests/kms_atomic_transition.c | 1 + tests/kms_ccs.c | 1 + tests/kms_chv_cursor_fail.c | 1 + tests/kms_crtc_background_color.c | 1 + tests/kms_cursor_crc.c| 1 + tests/kms_cursor_legacy.c | 1 + tests/kms_draw_crc.c | 1 + tests/kms_fbc_crc.c | 1 + tests/kms_flip_tiling.c | 1 + tests/kms_frontbuffer_tracking.c | 1 + tests/kms_mmap_write_crc.c| 1 + tests/kms_mmio_vs_cs_flip.c | 1 + tests/kms_pipe_color.c| 1 + tests/kms_pipe_crc_basic.c| 1 + tests/kms_plane.c | 1 + tests/kms_plane_lowres.c | 1 + tests/kms_plane_multiple.c| 1 + tests/kms_plane_scaling.c | 1 + tests/kms_pwrite_crc.c| 1 + tests/kms_rotation_crc.c | 1 + tests/kms_universal_plane.c | 1 + tools/intel_display_crc.c | 1 + 29 files changed, 714 insertions(+), 628 deletions(-) create mode 100644 lib/igt_crc.c create mode 100644 lib/igt_crc.h diff --git a/lib/Makefile.sources b/lib/Makefile.sources index 53fdb54c..cfba15c9 100644 --- a/lib/Makefile.sources +++ b/lib/Makefile.sources @@ -11,6 +11,8 @@ lib_source_list = \ igt_debugfs.h \ igt_aux.c \ igt_aux.h \ + igt_crc.c \ + igt_crc.h \ igt_edid_template.h \ igt_gt.c\ igt_gt.h\ diff --git a/lib/igt_chamelium.h b/lib/igt_chamelium.h index 81322ad2..ea5abc2e 100644 --- a/lib/igt_chamelium.h +++ b/lib/igt_chamelium.h @@ -31,6 +31,7 @@ #endif #include "igt.h" +#include "igt_crc.h" #include struct chamelium; diff --git a/lib/igt_crc.c b/lib/igt_crc.c new file mode 100644 index ..91a0b5a8 --- /dev/null +++ b/lib/igt_crc.c @@ -0,0 +1,563 @@ +/* + * Copyright © 2013 Intel Corporation + * + * Permission is hereby granted, free of charge, to any person obtaining a + * copy of this software and associated documentation files (the "Software"), + * to deal in the Software without restriction, including without limitation + * the rights to use, copy, modify, merge, publish, distribute, sublicense, + * and/or sell copies of the Software, and to permit persons to whom the + * Software is furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice (including the next + * paragraph) shall be included in all copies or substantial portions of the + * Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS + * IN THE SOFTWARE. + * + */ +#include +#include +#include +#include +#include +#include +#include +#include + +#include "igt_aux.h" +#include "igt_crc.h" +#include "igt_core.h" +#include "igt_debugfs.h" +#include "igt_kms.h" + +/** + * igt_assert_crc_equal: + * @a: first pipe CRC value + * @b: second pipe CRC value + * + * Compares two CRC values and fails the testcase if they don't match with + * igt_fail(). Note that due to CRC collisions CRC based testcase can only + * assert that CRCs match, never that they are different. Otherwise there might + * be random testcase failures when different screen contents end up with the + * same CRC by chance. + */ +void igt_assert_crc_equal(const igt_crc_t *a, const igt_crc_t *b) +{ + int i; + + for (i = 0; i < a->n_words; i++) + igt_assert_eq_u32(a->crc[i], b->crc[i]); +} + +/** + * igt_crc_to_string: + * @crc: pipe CRC value to print + * + * This formats @crc into a string buffer which is owned by igt_crc_to_string(). + * The next call will override the buffer again, which makes this multithreading + * unsafe. + * + * This should only ever be used for diagnostic debug output. + */ +char *igt_crc_to_string(igt_crc_t *crc) +{ + int i; + char buf[128] = { 0 }; + + for (i = 0; i < crc->n_words; i++) + sprintf(buf + strlen(buf), "%08x
[PATCH i-g-t v2 6/7] lib/igt_kms: Add igt_output_clone_pipe for cloning
From: Brian Starkey An output can be added as a clone of any other output(s) attached to a pipe using igt_output_clone_pipe() Signed-off-by: Brian Starkey --- lib/igt_kms.c | 90 +-- lib/igt_kms.h | 3 ++ 2 files changed, 59 insertions(+), 34 deletions(-) diff --git a/lib/igt_kms.c b/lib/igt_kms.c index aae32202..85dc0aa8 100644 --- a/lib/igt_kms.c +++ b/lib/igt_kms.c @@ -1560,6 +1560,17 @@ static void igt_display_log_shift(igt_display_t *display, int shift) igt_assert(display->log_shift >= 0); } +static int igt_output_idx(igt_output_t *output) +{ + int i; + + for (i = 0; i < output->display->n_outputs; i++) + if (&output->display->outputs[i] == output) + return i; + + return -1; +} + static void igt_output_refresh(igt_output_t *output) { igt_display_t *display = output->display; @@ -1990,40 +2001,6 @@ void igt_display_fini(igt_display_t *display) display->pipes = NULL; } -static void igt_display_refresh(igt_display_t *display) -{ - igt_output_t *output; - int i; - - unsigned long pipes_in_use = 0; - - /* Check that two outputs aren't trying to use the same pipe */ - for (i = 0; i < display->n_outputs; i++) { - output = &display->outputs[i]; - - if (pipes_in_use & output->pending_crtc_idx_mask) - goto report_dup; - - pipes_in_use |= output->pending_crtc_idx_mask; - - if (output->force_reprobe) - igt_output_refresh(output); - } - - return; - -report_dup: - for (; i > 0; i--) { - igt_output_t *b = &display->outputs[i - 1]; - - igt_assert_f(output->pending_crtc_idx_mask != -b->pending_crtc_idx_mask, -"%s and %s are both trying to use pipe %s\n", -igt_output_name(output), igt_output_name(b), -kmstest_pipe_name(ffs(b->pending_crtc_idx_mask) - 1)); - } -} - static igt_pipe_t *igt_output_get_driving_pipe(igt_output_t *output) { igt_display_t *display = output->display; @@ -2047,6 +2024,38 @@ static igt_pipe_t *igt_output_get_driving_pipe(igt_output_t *output) return &display->pipes[pipe]; } +static void igt_display_refresh(igt_display_t *display) +{ + igt_output_t *output; + igt_pipe_t *pipe; + int i; + + unsigned long pipes_in_use = 0; + + /* Check that outputs and pipes agree wrt. cloning */ + for (i = 0; i < display->n_outputs; i++) { + output = &display->outputs[i]; + + pipe = igt_output_get_driving_pipe(output); + if (pipe) { + igt_assert_f(pipe->outputs & (1 << igt_output_idx(output)), +"Output %s not expected to be using pipe %s\n", +igt_output_name(output), +kmstest_pipe_name(pipe->pipe)); + + if (pipes_in_use & output->pending_crtc_idx_mask) + LOG(display, "Output %s clones pipe %s\n", + igt_output_name(output), + kmstest_pipe_name(pipe->pipe)); + } + + pipes_in_use |= output->pending_crtc_idx_mask; + + if (output->force_reprobe) + igt_output_refresh(output); + } +} + static igt_plane_t *igt_pipe_get_plane(igt_pipe_t *pipe, int plane_idx) { igt_assert_f(plane_idx >= 0 && plane_idx < pipe->n_planes, @@ -2941,6 +2950,16 @@ void igt_output_override_mode(igt_output_t *output, drmModeModeInfo *mode) pipe->mode_changed = true; } +void igt_output_clone_pipe(igt_output_t *output, enum pipe pipe) +{ + igt_display_t *display = output->display; + uint32_t current_clones = display->pipes[pipe].outputs; + + igt_output_set_pipe(output, pipe); + + display->pipes[pipe].outputs |= current_clones; +} + void igt_output_set_pipe(igt_output_t *output, enum pipe pipe) { igt_display_t *display = output->display; @@ -2952,6 +2971,7 @@ void igt_output_set_pipe(igt_output_t *output, enum pipe pipe) old_pipe = igt_output_get_driving_pipe(output); old_pipe->mode_changed = true; + old_pipe->outputs &= ~(1 << igt_output_idx(output)); } if (pipe == PIPE_NONE) { @@ -2963,6 +2983,8 @@ void igt_output_set_pipe(igt_output_t *output, enum pipe pipe) output->pending_crtc_idx_mask = 1 << pipe; display->pipes[pipe].mode_changed = true; + + display->pipes[pipe].outputs = (1 << igt_output_idx(output)); } output->config.pipe_changed = true; diff --git a/lib/igt_kms.h b/lib/igt_kms.h index ab8ec764..9d
[PATCH i-g-t v2 5/7] kms_writeback: Add writeback-check-output
From: Brian Starkey Add a test which makes commits using the writeback connector, and checks the output buffer hash to make sure it is/isn't written as appropriate. Signed-off-by: Brian Starkey --- tests/kms_writeback.c | 123 ++ 1 file changed, 123 insertions(+) diff --git a/tests/kms_writeback.c b/tests/kms_writeback.c index d2066482..8201a81c 100644 --- a/tests/kms_writeback.c +++ b/tests/kms_writeback.c @@ -29,6 +29,7 @@ #include "igt.h" #include "igt_fb.h" +#include "sw_sync.h" /* We need to define these ourselves until we get an updated libdrm */ #ifndef DRM_MODE_CONNECTOR_WRITEBACK @@ -278,6 +279,115 @@ static void writeback_fb_id(igt_output_t *output, igt_fb_t *valid_fb, igt_fb_t * igt_assert(ret == 0); } +static void fill_fb(igt_fb_t *fb, double color[3]) +{ + cairo_t *cr = igt_get_cairo_ctx(fb->fd, fb); + igt_assert(cr); + + igt_paint_color(cr, 0, 0, fb->width, fb->height, + color[0], color[1], color[2]); +} + +static void get_and_wait_out_fence(igt_output_t *output) +{ + int ret, out_fence = out_fence = igt_output_get_last_writeback_out_fence(output); + igt_assert(out_fence >= 0); + + ret = sync_fence_wait(out_fence, 1000); + igt_assert(ret == 0); + close(out_fence); +} + +static void writeback_seqence(igt_output_t *output, igt_plane_t *plane, + igt_fb_t *in_fb, igt_fb_t *out_fbs[], int n_commits) +{ + int i, color_idx = 0; + double in_fb_colors[2][3] = { + { 1.0, 0.0, 0.0 }, + { 0.0, 1.0, 0.0 }, + }; + double clear_color[3] = { 1.0, 1.0, 1.0 }; + igt_crc_t cleared_crc, out_expected; + + for (i = 0; i < n_commits; i++, color_idx++) { + /* Change the input color each time */ + fill_fb(in_fb, in_fb_colors[color_idx % 2]); + + if (out_fbs[i]) { + igt_crc_t out_before; + + /* Get the expected CRC */ + fill_fb(out_fbs[i], in_fb_colors[color_idx % 2]); + igt_fb_get_crc(out_fbs[i], &out_expected); + + fill_fb(out_fbs[i], clear_color); + if (i == 0) + igt_fb_get_crc(out_fbs[i], &cleared_crc); + igt_fb_get_crc(out_fbs[i], &out_before); + igt_assert_crc_equal(&cleared_crc, &out_before); + } + + /* Commit */ + igt_plane_set_fb(plane, in_fb); + igt_output_set_writeback_fb(output, out_fbs[i]); + if (out_fbs[i]) + igt_output_request_writeback_out_fence(output); + igt_display_commit_atomic(output->display, + DRM_MODE_ATOMIC_ALLOW_MODESET, + NULL); + if (out_fbs[i]) + get_and_wait_out_fence(output); + + /* Make sure the old output buffer is untouched */ + if (i > 0 && out_fbs[i - 1] && (out_fbs[i] != out_fbs[i - 1])) { + igt_crc_t out_prev; + igt_fb_get_crc(out_fbs[i - 1], &out_prev); + igt_assert_crc_equal(&cleared_crc, &out_prev); + } + + /* Make sure this output buffer is written */ + if (out_fbs[i]) { + igt_crc_t out_after; + igt_fb_get_crc(out_fbs[i], &out_after); + igt_assert_crc_equal(&out_expected, &out_after); + + /* And clear it, for the next time */ + fill_fb(out_fbs[i], clear_color); + } + } +} + +static void writeback_check_output(igt_output_t *output, igt_plane_t *plane, + igt_fb_t *input_fb, igt_fb_t *output_fb) +{ + igt_fb_t *out_fbs[2] = { 0 }; + igt_fb_t second_out_fb; + int ret; + + /* One commit, with a writeback. */ + writeback_seqence(output, plane, input_fb, &output_fb, 1); + + /* Two commits, the second with no writeback */ + out_fbs[0] = output_fb; + writeback_seqence(output, plane, input_fb, out_fbs, 2); + + /* Two commits, both with writeback */ + out_fbs[1] = output_fb; + writeback_seqence(output, plane, input_fb, out_fbs, 2); + + ret = igt_create_fb(output_fb->fd, output_fb->width, output_fb->height, + DRM_FORMAT_XRGB, + igt_fb_mod_to_tiling(0), + &second_out_fb); + igt_require(ret > 0); + + /* Two commits, with different writeback buffers */ + out_fbs[1] = &second_out_fb; + writeback_seqence(output, plane, input_fb, out_fbs, 2); + + igt_remove_fb(output_fb->fd, &second_out_fb); +} + igt_main { igt_displa
[PATCH i-g-t v2 0/7] igt: Add support for testing writeback connectors
We're trying to introduce support for writeback connectors, a way to expose in DRM the hardware functionality from display engines that allows to write back into memory the result of the DE's composition of supported planes. Generic DRM support is available here [1] and will be merged once this patchset gets approved for inclusion into igt. VC4 support for writeback is added here [2] and for mali-dp is added here [3]. Changelog: - v2: Rebased on top of a844ccbdbab9fd16c37de81281c6281bc800e97a - v1: Original (re)submission [4] Many thanks, Liviu [1] https://lists.freedesktop.org/archives/dri-devel/2017-May/141796.html [2] https://lists.freedesktop.org/archives/dri-devel/2017-June/143337.html [3] https://lists.freedesktop.org/archives/dri-devel/2017-May/141799.html [4] https://lists.freedesktop.org/archives/intel-gfx/2017-July/132344.html Brian Starkey (7): igt: lib/igt_crc: Split out CRC functionality lib/igt_kms: Add writeback support in lib/ kms_writeback: Add initial writeback tests lib: Add function to hash a framebuffer kms_writeback: Add writeback-check-output lib/igt_kms: Add igt_output_clone_pipe for cloning kms_writeback: Add tests using a cloned output lib/Makefile.sources | 2 + lib/igt_aux.c | 1 + lib/igt_chamelium.h | 1 + lib/igt_crc.c | 563 ++ lib/igt_crc.h | 125 + lib/igt_debugfs.c | 547 lib/igt_debugfs.h | 81 -- lib/igt_fb.c | 65 + lib/igt_fb.h | 5 + lib/igt_kms.c | 172 +--- lib/igt_kms.h | 26 ++ tests/Makefile.sources| 1 + tests/chamelium.c | 1 + tests/kms_atomic_transition.c | 1 + tests/kms_ccs.c | 1 + tests/kms_chv_cursor_fail.c | 1 + tests/kms_crtc_background_color.c | 1 + tests/kms_cursor_crc.c| 1 + tests/kms_cursor_legacy.c | 1 + tests/kms_draw_crc.c | 1 + tests/kms_fbc_crc.c | 1 + tests/kms_flip_tiling.c | 1 + tests/kms_frontbuffer_tracking.c | 1 + tests/kms_mmap_write_crc.c| 1 + tests/kms_mmio_vs_cs_flip.c | 1 + tests/kms_pipe_color.c| 1 + tests/kms_pipe_crc_basic.c| 1 + tests/kms_plane.c | 1 + tests/kms_plane_lowres.c | 1 + tests/kms_plane_multiple.c| 1 + tests/kms_plane_scaling.c | 1 + tests/kms_pwrite_crc.c| 1 + tests/kms_rotation_crc.c | 1 + tests/kms_universal_plane.c | 1 + tests/kms_writeback.c | 541 tools/intel_display_crc.c | 1 + 36 files changed, 1487 insertions(+), 666 deletions(-) create mode 100644 lib/igt_crc.c create mode 100644 lib/igt_crc.h create mode 100644 tests/kms_writeback.c -- 2.13.2 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Nouveau] [regression drm/noveau] suspend to ram -> BOOM: exception RIP: drm_calc_vbltimestamp_from_scanoutpos+335
On Fri, Jul 14, 2017 at 11:15 AM, Mike Galbraith wrote: > On Fri, 2017-07-14 at 17:10 +0200, Karol Herbst wrote: >> Yeah, we shouldn't let the machine die. Are there more WARN_ON_ONCE >> usage we could convert to WARN_ONCE? > > Shooting the messenger is generally considered uncool :) That's never stopped it from being a popular practice... ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] [PATCH v2 12/12] drm/atomic: Allow drm_atomic_helper_swap_state to fail
On Tue, Jul 11, 2017 at 04:33:14PM +0200, Maarten Lankhorst wrote: > Signed-off-by: Maarten Lankhorst > --- > drivers/gpu/drm/drm_atomic_helper.c | 15 +++ > 1 file changed, 7 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c > b/drivers/gpu/drm/drm_atomic_helper.c > index bfb98fbd0e0e..05a22aeffbb0 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -2261,13 +2261,13 @@ EXPORT_SYMBOL(drm_atomic_helper_cleanup_planes); > * > * Returns: > * > - * Always returns 0, cannot fail yet. > + * Returns 0 on success. Can return -ERESTARTSYS when @stall is true and the > + * waiting for the previous commits has been interrupted. > */ Reviewed-by: Daniel Vetter > int drm_atomic_helper_swap_state(struct drm_atomic_state *state, > bool stall) > { > - int i; > - long ret; > + int i, ret; > struct drm_connector *connector; > struct drm_connector_state *old_conn_state, *new_conn_state; > struct drm_crtc *crtc; > @@ -2290,12 +2290,11 @@ int drm_atomic_helper_swap_state(struct > drm_atomic_state *state, > if (!commit) > continue; > > - ret = wait_for_completion_timeout(&commit->hw_done, > - 10*HZ); > - if (ret == 0) > - DRM_ERROR("[CRTC:%d:%s] hw_done timed out\n", > - crtc->base.id, crtc->name); > + ret = > wait_for_completion_interruptible(&commit->hw_done); > drm_crtc_commit_put(commit); > + > + if (ret) > + return ret; > } > } > > -- > 2.11.0 > > ___ > Intel-gfx mailing list > intel-...@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] [PATCH v2 11/12] drm/atomic: Add __must_check to drm_atomic_helper_swap_state.
On Tue, Jul 11, 2017 at 04:33:13PM +0200, Maarten Lankhorst wrote: > Now that all drivers check the return value, convert swap_state to > __must_check. This is done separately to force build warnings if we > missed a driver. > > Signed-off-by: Maarten Lankhorst Maybe squash in with the next commit, seems a bit silly to split this out. Either way: Reviewed-by: Daniel Vetter > --- > include/drm/drm_atomic_helper.h | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h > index 37c9534ff691..8871741fa723 100644 > --- a/include/drm/drm_atomic_helper.h > +++ b/include/drm/drm_atomic_helper.h > @@ -84,7 +84,8 @@ void > drm_atomic_helper_disable_planes_on_crtc(struct drm_crtc_state > *old_crtc_state, >bool atomic); > > -int drm_atomic_helper_swap_state(struct drm_atomic_state *state, bool stall); > +int __must_check drm_atomic_helper_swap_state(struct drm_atomic_state *state, > + bool stall); > > /* nonblocking commit helpers */ > int drm_atomic_helper_setup_commit(struct drm_atomic_state *state, > -- > 2.11.0 > > ___ > Intel-gfx mailing list > intel-...@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] [PATCH v2 10/12] drm/vc4: Handle drm_atomic_helper_swap_state failure
On Tue, Jul 11, 2017 at 04:33:12PM +0200, Maarten Lankhorst wrote: > drm_atomic_helper_swap_state() will be changed to interruptible waiting > in the next few commits, so all drivers have to be changed to handling > failure. > > VC4 has its own nonblocking modeset tracking through the vc4->async_modeset > semaphore, so it doesn't need to stall in swap_state. Pass stall = false > and BUG_ON when it returns an error. This should never happen for !stall. > > Signed-off-by: Maarten Lankhorst > Cc: Eric Anholt Reviewed-by: Daniel Vetter > --- > drivers/gpu/drm/vc4/vc4_kms.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c > index 27edae427025..aeec6e8703d2 100644 > --- a/drivers/gpu/drm/vc4/vc4_kms.c > +++ b/drivers/gpu/drm/vc4/vc4_kms.c > @@ -126,7 +126,7 @@ static int vc4_atomic_commit(struct drm_device *dev, >* the software side now. >*/ > > - drm_atomic_helper_swap_state(state, true); > + BUG_ON(drm_atomic_helper_swap_state(state, false) < 0); > > /* >* Everything below can be run asynchronously without the need to grab > -- > 2.11.0 > > ___ > Intel-gfx mailing list > intel-...@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] [PATCH v2 08/12] drm/tegra: Handle drm_atomic_helper_swap_state failure
On Tue, Jul 11, 2017 at 04:33:10PM +0200, Maarten Lankhorst wrote: > drm_atomic_helper_swap_state() will be changed to interruptible waiting > in the next few commits, so all drivers have to be changed to handling > failure. > > Signed-off-by: Maarten Lankhorst > Cc: Thierry Reding > Cc: Jonathan Hunter > Cc: linux-te...@vger.kernel.org Reviewed-by: Daniel Vetter > --- > drivers/gpu/drm/tegra/drm.c | 7 ++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c > index ad3d124a972d..3ba659a5940d 100644 > --- a/drivers/gpu/drm/tegra/drm.c > +++ b/drivers/gpu/drm/tegra/drm.c > @@ -100,7 +100,12 @@ static int tegra_atomic_commit(struct drm_device *drm, >* the software side now. >*/ > > - drm_atomic_helper_swap_state(state, true); > + err = drm_atomic_helper_swap_state(state, true); > + if (err) { > + mutex_unlock(&tegra->commit.lock); > + drm_atomic_helper_cleanup_planes(drm, state); > + return err; > + } > > drm_atomic_state_get(state); > if (nonblock) > -- > 2.11.0 > > ___ > Intel-gfx mailing list > intel-...@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] [PATCH v2 07/12] drm/msm: Handle drm_atomic_helper_swap_state failure
On Tue, Jul 11, 2017 at 04:33:09PM +0200, Maarten Lankhorst wrote: > drm_atomic_helper_swap_state() will be changed to interruptible waiting > in the next few commits, so all drivers have to be changed to handling > failure. > > MSM has its own busy tracking, which means the swap_state call can be > done with stall = false, in which case it should never return an error. > Handle failure with BUG_ON for this reason. > > Signed-off-by: Maarten Lankhorst > Cc: Rob Clark > Cc: linux-arm-...@vger.kernel.org > Cc: freedr...@lists.freedesktop.org > --- > drivers/gpu/drm/msm/msm_atomic.c | 16 > 1 file changed, 8 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/msm/msm_atomic.c > b/drivers/gpu/drm/msm/msm_atomic.c > index 9633a68b14d7..badfa8717317 100644 > --- a/drivers/gpu/drm/msm/msm_atomic.c > +++ b/drivers/gpu/drm/msm/msm_atomic.c > @@ -232,20 +232,18 @@ int msm_atomic_commit(struct drm_device *dev, >* mark our set of crtc's as busy: >*/ > ret = start_atomic(dev->dev_private, c->crtc_mask); > - if (ret) { > - kfree(c); > - goto error; > - } > + if (ret) > + goto err_free; > + > + BUG_ON(drm_atomic_helper_swap_state(state, false) < 0); Hm, not sure we want to do this, makes switching msm to the nonblocking helpers a bit more tricky. And the got err_free thing looks like leftovers from an old version. But it's all correct. Reviewed-by: Daniel Vetter > > /* >* This is the point of no return - everything below never fails except >* when the hw goes bonghits. Which means we can commit the new state on >* the software side now. > + * > + * swap driver private state while still holding state_lock >*/ > - > - drm_atomic_helper_swap_state(state, true); > - > - /* swap driver private state while still holding state_lock */ > if (to_kms_state(state)->state) > priv->kms->funcs->swap_state(priv->kms, state); > > @@ -275,6 +273,8 @@ int msm_atomic_commit(struct drm_device *dev, > > return 0; > > +err_free: > + kfree(c); > error: > drm_atomic_helper_cleanup_planes(dev, state); > return ret; > -- > 2.11.0 > > ___ > Intel-gfx mailing list > intel-...@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] [PATCH v2 00/12] drm/atomic: Make drm_atomic_helper_swap_state waiting interruptible.
On Tue, Jul 11, 2017 at 04:33:02PM +0200, Maarten Lankhorst wrote: > drm_atomic_helper_swap_state could previously never fail, in order to still > continue it would set a limit of 10s on waiting for previous updates to > complete, > then it moved forward. This can be very bad when ignoring previous updates, > because the hw state and sw state may get out of sync when for example a > modeset > is ignored. > > By converting the swap_state to interruptible and handling the error in each > driver, > we fix this issue because if a update takes forever it can be aborted through > signals. > > Changes since first version: > - Split out driver conversions. > - Fix nouveau error handling first. > > Maarten Lankhorst (12): > drm/nouveau: Fix error handling in nv50_disp_atomic_commit > drm/atomic: Change drm_atomic_helper_swap_state to return an error. > drm/nouveau: Handle drm_atomic_helper_swap_state failure > drm/atmel-hlcdc: Handle drm_atomic_helper_swap_state failure > drm/i915: Handle drm_atomic_helper_swap_state failure > drm/mediatek: Handle drm_atomic_helper_swap_state failure > drm/msm: Handle drm_atomic_helper_swap_state failure > drm/tegra: Handle drm_atomic_helper_swap_state failure > drm/tilcdc: Handle drm_atomic_helper_swap_state failure > drm/vc4: Handle drm_atomic_helper_swap_state failure > drm/atomic: Add __must_check to drm_atomic_helper_swap_state. > drm/atomic: Allow drm_atomic_helper_swap_state to fail Hi Maarten, The set looks good to me. The core changes make sense and seem like a good step forward, and the driver changes seem like they do the right thing. Normally, I'd suggest waiting a week or so for maintainer acks to trickle in, but since this is fixing behavior in i915, it makes sense to expedite it. Perhaps ping individuals on IRC? For the whole set, Reviewed-by: Sean Paul Sean > > drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 13 ++- > drivers/gpu/drm/drm_atomic_helper.c | 35 > +--- > drivers/gpu/drm/i915/intel_display.c | 10 +++- > drivers/gpu/drm/mediatek/mtk_drm_drv.c | 7 +- > drivers/gpu/drm/msm/msm_atomic.c | 16 ++--- > drivers/gpu/drm/nouveau/nv50_display.c | 12 +++--- > drivers/gpu/drm/tegra/drm.c | 7 +- > drivers/gpu/drm/tilcdc/tilcdc_drv.c | 6 - > drivers/gpu/drm/vc4/vc4_kms.c| 2 +- > include/drm/drm_atomic_helper.h | 4 ++-- > 10 files changed, 75 insertions(+), 37 deletions(-) > > -- > 2.11.0 > > ___ > Intel-gfx mailing list > intel-...@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Sean Paul, Software Engineer, Google / Chromium OS ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] [PATCH v2 02/12] drm/atomic: Change drm_atomic_helper_swap_state to return an error.
On Fri, Jul 14, 2017 at 04:30:30PM +0200, Daniel Vetter wrote: > On Tue, Jul 11, 2017 at 04:33:04PM +0200, Maarten Lankhorst wrote: > > We want to change swap_state to wait indefinitely, but to do this > > swap_state should wait interruptibly. This requires propagating > > the error to each driver. > > > > Cc: dri-devel@lists.freedesktop.org > > Cc: linux-ker...@vger.kernel.org > > Cc: intel-...@lists.freedesktop.org > > Signed-off-by: Maarten Lankhorst > > Reviewed-by: Daniel Vetter > > > --- > > drivers/gpu/drm/drm_atomic_helper.c | 22 -- > > include/drm/drm_atomic_helper.h | 3 +-- > > 2 files changed, 17 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c > > b/drivers/gpu/drm/drm_atomic_helper.c > > index 667ec97d4efb..bfb98fbd0e0e 100644 > > --- a/drivers/gpu/drm/drm_atomic_helper.c > > +++ b/drivers/gpu/drm/drm_atomic_helper.c > > @@ -1510,10 +1510,8 @@ int drm_atomic_helper_commit(struct drm_device *dev, > > > > if (!nonblock) { > > ret = drm_atomic_helper_wait_for_fences(dev, state, true); > > - if (ret) { > > - drm_atomic_helper_cleanup_planes(dev, state); > > - return ret; > > - } > > + if (ret) > > + goto err; > > } > > > > /* > > @@ -1522,7 +1520,9 @@ int drm_atomic_helper_commit(struct drm_device *dev, > > * the software side now. > > */ > > > > - drm_atomic_helper_swap_state(state, true); > > + ret = drm_atomic_helper_swap_state(state, true); > > + if (ret) > > + goto err; > > > > /* > > * Everything below can be run asynchronously without the need to grab > > @@ -1551,6 +1551,10 @@ int drm_atomic_helper_commit(struct drm_device *dev, > > commit_tail(state); > > > > return 0; > > + > > +err: > > + drm_atomic_helper_cleanup_planes(dev, state); > > + return ret; > > } > > EXPORT_SYMBOL(drm_atomic_helper_commit); > > > > @@ -2254,8 +2258,12 @@ EXPORT_SYMBOL(drm_atomic_helper_cleanup_planes); > > * the &drm_plane.state, &drm_crtc.state or &drm_connector.state pointer. > > With > > * the current atomic helpers this is almost always the case, since the > > helpers > > * don't pass the right state structures to the callbacks. While you're changing this comment... would you mind s/proceeding/preceding/ and s/swaped/swapped/ ? Otherwise, Reviewed-by: Sean Paul > > + * > > + * Returns: > > + * > > + * Always returns 0, cannot fail yet. > > */ > > -void drm_atomic_helper_swap_state(struct drm_atomic_state *state, > > +int drm_atomic_helper_swap_state(struct drm_atomic_state *state, > > bool stall) > > { > > int i; > > @@ -2332,6 +2340,8 @@ void drm_atomic_helper_swap_state(struct > > drm_atomic_state *state, > > > > __for_each_private_obj(state, obj, obj_state, i, funcs) > > funcs->swap_state(obj, &state->private_objs[i].obj_state); > > + > > + return 0; > > } > > EXPORT_SYMBOL(drm_atomic_helper_swap_state); > > > > diff --git a/include/drm/drm_atomic_helper.h > > b/include/drm/drm_atomic_helper.h > > index dd196cc0afd7..37c9534ff691 100644 > > --- a/include/drm/drm_atomic_helper.h > > +++ b/include/drm/drm_atomic_helper.h > > @@ -84,8 +84,7 @@ void > > drm_atomic_helper_disable_planes_on_crtc(struct drm_crtc_state > > *old_crtc_state, > > bool atomic); > > > > -void drm_atomic_helper_swap_state(struct drm_atomic_state *state, > > - bool stall); > > +int drm_atomic_helper_swap_state(struct drm_atomic_state *state, bool > > stall); > > > > /* nonblocking commit helpers */ > > int drm_atomic_helper_setup_commit(struct drm_atomic_state *state, > > -- > > 2.11.0 > > > > ___ > > Intel-gfx mailing list > > intel-...@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch > ___ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Sean Paul, Software Engineer, Google / Chromium OS ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/etnaviv: select CMA and DMA_CMA if available
While this is no build dependency, etnaviv will only work correctly on most systems if CMA and DMA_CMA are enabled. Select both options if available to avoid users ending up with a non-working GPU due to a lacking kernel config. Signed-off-by: Lucas Stach --- drivers/gpu/drm/etnaviv/Kconfig | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/gpu/drm/etnaviv/Kconfig b/drivers/gpu/drm/etnaviv/Kconfig index 71cee4e9fefb..38b477b5fbf9 100644 --- a/drivers/gpu/drm/etnaviv/Kconfig +++ b/drivers/gpu/drm/etnaviv/Kconfig @@ -10,6 +10,8 @@ config DRM_ETNAVIV select IOMMU_API select IOMMU_SUPPORT select WANT_DEV_COREDUMP + select CMA if HAVE_DMA_CONTIGUOUS + select DMA_CMA if HAVE_DMA_CONTIGUOUS help DRM driver for Vivante GPUs. -- 2.11.0 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] [PATCH v2 02/12] drm/atomic: Change drm_atomic_helper_swap_state to return an error.
On Tue, Jul 11, 2017 at 04:33:04PM +0200, Maarten Lankhorst wrote: > We want to change swap_state to wait indefinitely, but to do this > swap_state should wait interruptibly. This requires propagating > the error to each driver. > > Cc: dri-devel@lists.freedesktop.org > Cc: linux-ker...@vger.kernel.org > Cc: intel-...@lists.freedesktop.org > Signed-off-by: Maarten Lankhorst Reviewed-by: Daniel Vetter > --- > drivers/gpu/drm/drm_atomic_helper.c | 22 -- > include/drm/drm_atomic_helper.h | 3 +-- > 2 files changed, 17 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c > b/drivers/gpu/drm/drm_atomic_helper.c > index 667ec97d4efb..bfb98fbd0e0e 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -1510,10 +1510,8 @@ int drm_atomic_helper_commit(struct drm_device *dev, > > if (!nonblock) { > ret = drm_atomic_helper_wait_for_fences(dev, state, true); > - if (ret) { > - drm_atomic_helper_cleanup_planes(dev, state); > - return ret; > - } > + if (ret) > + goto err; > } > > /* > @@ -1522,7 +1520,9 @@ int drm_atomic_helper_commit(struct drm_device *dev, >* the software side now. >*/ > > - drm_atomic_helper_swap_state(state, true); > + ret = drm_atomic_helper_swap_state(state, true); > + if (ret) > + goto err; > > /* >* Everything below can be run asynchronously without the need to grab > @@ -1551,6 +1551,10 @@ int drm_atomic_helper_commit(struct drm_device *dev, > commit_tail(state); > > return 0; > + > +err: > + drm_atomic_helper_cleanup_planes(dev, state); > + return ret; > } > EXPORT_SYMBOL(drm_atomic_helper_commit); > > @@ -2254,8 +2258,12 @@ EXPORT_SYMBOL(drm_atomic_helper_cleanup_planes); > * the &drm_plane.state, &drm_crtc.state or &drm_connector.state pointer. > With > * the current atomic helpers this is almost always the case, since the > helpers > * don't pass the right state structures to the callbacks. > + * > + * Returns: > + * > + * Always returns 0, cannot fail yet. > */ > -void drm_atomic_helper_swap_state(struct drm_atomic_state *state, > +int drm_atomic_helper_swap_state(struct drm_atomic_state *state, > bool stall) > { > int i; > @@ -2332,6 +2340,8 @@ void drm_atomic_helper_swap_state(struct > drm_atomic_state *state, > > __for_each_private_obj(state, obj, obj_state, i, funcs) > funcs->swap_state(obj, &state->private_objs[i].obj_state); > + > + return 0; > } > EXPORT_SYMBOL(drm_atomic_helper_swap_state); > > diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h > index dd196cc0afd7..37c9534ff691 100644 > --- a/include/drm/drm_atomic_helper.h > +++ b/include/drm/drm_atomic_helper.h > @@ -84,8 +84,7 @@ void > drm_atomic_helper_disable_planes_on_crtc(struct drm_crtc_state > *old_crtc_state, >bool atomic); > > -void drm_atomic_helper_swap_state(struct drm_atomic_state *state, > - bool stall); > +int drm_atomic_helper_swap_state(struct drm_atomic_state *state, bool stall); > > /* nonblocking commit helpers */ > int drm_atomic_helper_setup_commit(struct drm_atomic_state *state, > -- > 2.11.0 > > ___ > Intel-gfx mailing list > intel-...@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] [PATCH v2 01/12] drm/nouveau: Fix error handling in nv50_disp_atomic_commit
On Tue, Jul 11, 2017 at 04:33:03PM +0200, Maarten Lankhorst wrote: > Make it more clear that post commit return ret is really return 0, > > and add a missing drm_atomic_helper_cleanup_planes when > drm_atomic_helper_wait_for_fences fails. > > Fixes: 839ca903f12e ("drm/nouveau/kms/nv50: transition to atomic interfaces > internally") > Cc: Ben Skeggs > Cc: dri-devel@lists.freedesktop.org > Cc: nouv...@lists.freedesktop.org > Cc: # v4.10+ > Signed-off-by: Maarten Lankhorst > --- > drivers/gpu/drm/nouveau/nv50_display.c | 7 +-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/nouveau/nv50_display.c > b/drivers/gpu/drm/nouveau/nv50_display.c > index 42a85c14aea0..8fb55b96c40f 100644 > --- a/drivers/gpu/drm/nouveau/nv50_display.c > +++ b/drivers/gpu/drm/nouveau/nv50_display.c > @@ -4119,7 +4119,7 @@ nv50_disp_atomic_commit(struct drm_device *dev, > if (!nonblock) { > ret = drm_atomic_helper_wait_for_fences(dev, state, true); > if (ret) > - goto done; > + goto err_cleanup; > } > > for_each_plane_in_state(state, plane, plane_state, i) { > @@ -4147,7 +4147,7 @@ nv50_disp_atomic_commit(struct drm_device *dev, > if (crtc->state->enable) { > if (!drm->have_disp_power_ref) { > drm->have_disp_power_ref = true; > - return ret; > + return 0; > } > active = true; > break; > @@ -4158,7 +4158,10 @@ nv50_disp_atomic_commit(struct drm_device *dev, > pm_runtime_put_autosuspend(dev->dev); > drm->have_disp_power_ref = false; > } > + goto done; > A bit convoluted, I prefer to wrap these in if (ret) to make them only run for the failure case. Either way: Reviewed-by: Daniel Vetter > +err_cleanup: > + drm_atomic_helper_cleanup_planes(dev, state); > done: > pm_runtime_put_autosuspend(dev->dev); > return ret; > -- > 2.11.0 > > ___ > Intel-gfx mailing list > intel-...@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 2/2] drm/panel: simple: Add support for ddc-only panel
On Thu, Jul 13, 2017 at 6:04 AM, wrote: > From: Patrick Bruenn > > This is a fix for the CX9020 Embedded PC. On that device the 24-bit > parallel-display signal of the imx53 is combined with an I2C channel > and converted to DVI-D port. Devicetree magic always requires a panel > connected to the parallel-display port. Then fix this. It's not the DT that requires this, but limitations in the i.MX driver. > We add an empty panel_desc, to get recognized by the panel-simple > driver, which can then use the ddc bus to read the panels configuration. > > Signed-off-by: Patrick Bruenn > --- > drivers/gpu/drm/panel/panel-simple.c | 6 ++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/gpu/drm/panel/panel-simple.c > b/drivers/gpu/drm/panel/panel-simple.c > index c4566ce8fda7..52b2a7fb59ed 100644 > --- a/drivers/gpu/drm/panel/panel-simple.c > +++ b/drivers/gpu/drm/panel/panel-simple.c > @@ -1856,6 +1856,9 @@ static const struct panel_desc winstar_wf35ltiacd = { > .bus_format = MEDIA_BUS_FMT_RGB888_1X24, > }; > > +static const struct panel_desc simple_ddc_only = { > +}; > + > static const struct of_device_id platform_of_match[] = { > { > .compatible = "ampire,am-480272h3tmqw-t01h", > @@ -2050,6 +2053,9 @@ static const struct of_device_id platform_of_match[] = { > .compatible = "winstar,wf35ltiacd", > .data = &winstar_wf35ltiacd, > }, { > + .compatible = "simple,ddc-only", If you notice the rest of the file, we require compatibles be specific for the panels. What you want here is a "dvi-connector" node. Rob ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[Bug 26345] [845G] CPU/GPU incoherency
https://bugs.freedesktop.org/show_bug.cgi?id=26345 Ленар changed: What|Removed |Added i915 platform||ALL Hardware|x86 (IA32) |All -- You are receiving this mail because: You are the assignee for the bug.___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v5 12/14] drm: radeon: remove dead code and pointless local lut storage
On Thu, Jul 13, 2017 at 12:25 PM, Peter Rosin wrote: > The redundant fb helpers .load_lut, .gamma_set and .gamma_get are > no longer used. Remove the dead code and hook up the crtc .gamma_set > to use the crtc gamma_store directly instead of duplicating that > info locally. > > Acked-by: Daniel Vetter > Signed-off-by: Peter Rosin Acked-by: Alex Deucher > --- > drivers/gpu/drm/radeon/atombios_crtc.c | 1 - > drivers/gpu/drm/radeon/radeon_connectors.c | 7 ++- > drivers/gpu/drm/radeon/radeon_display.c | 71 > - > drivers/gpu/drm/radeon/radeon_fb.c | 2 - > drivers/gpu/drm/radeon/radeon_legacy_crtc.c | 1 - > drivers/gpu/drm/radeon/radeon_mode.h| 4 -- > 6 files changed, 33 insertions(+), 53 deletions(-) > > diff --git a/drivers/gpu/drm/radeon/atombios_crtc.c > b/drivers/gpu/drm/radeon/atombios_crtc.c > index 3c492a0aa6bd..02baaaf20e9d 100644 > --- a/drivers/gpu/drm/radeon/atombios_crtc.c > +++ b/drivers/gpu/drm/radeon/atombios_crtc.c > @@ -2217,7 +2217,6 @@ static const struct drm_crtc_helper_funcs > atombios_helper_funcs = { > .mode_set_base_atomic = atombios_crtc_set_base_atomic, > .prepare = atombios_crtc_prepare, > .commit = atombios_crtc_commit, > - .load_lut = radeon_crtc_load_lut, > .disable = atombios_crtc_disable, > }; > > diff --git a/drivers/gpu/drm/radeon/radeon_connectors.c > b/drivers/gpu/drm/radeon/radeon_connectors.c > index 27affbde058c..2f642cbefd8e 100644 > --- a/drivers/gpu/drm/radeon/radeon_connectors.c > +++ b/drivers/gpu/drm/radeon/radeon_connectors.c > @@ -773,12 +773,15 @@ static int radeon_connector_set_property(struct > drm_connector *connector, struct > > if (connector->encoder->crtc) { > struct drm_crtc *crtc = connector->encoder->crtc; > - const struct drm_crtc_helper_funcs *crtc_funcs = > crtc->helper_private; > struct radeon_crtc *radeon_crtc = > to_radeon_crtc(crtc); > > radeon_crtc->output_csc = radeon_encoder->output_csc; > > - (*crtc_funcs->load_lut)(crtc); > + /* > +* Our .gamma_set assumes the .gamma_store has been > +* prefilled and don't care about its arguments. > +*/ > + crtc->funcs->gamma_set(crtc, NULL, NULL, NULL, 0, > NULL); > } > } > > diff --git a/drivers/gpu/drm/radeon/radeon_display.c > b/drivers/gpu/drm/radeon/radeon_display.c > index 17d3dafc8319..8b7d7a0d3ca8 100644 > --- a/drivers/gpu/drm/radeon/radeon_display.c > +++ b/drivers/gpu/drm/radeon/radeon_display.c > @@ -42,6 +42,7 @@ static void avivo_crtc_load_lut(struct drm_crtc *crtc) > struct radeon_crtc *radeon_crtc = to_radeon_crtc(crtc); > struct drm_device *dev = crtc->dev; > struct radeon_device *rdev = dev->dev_private; > + u16 *r, *g, *b; > int i; > > DRM_DEBUG_KMS("%d\n", radeon_crtc->crtc_id); > @@ -60,11 +61,14 @@ static void avivo_crtc_load_lut(struct drm_crtc *crtc) > WREG32(AVIVO_DC_LUT_WRITE_EN_MASK, 0x003f); > > WREG8(AVIVO_DC_LUT_RW_INDEX, 0); > + r = crtc->gamma_store; > + g = r + crtc->gamma_size; > + b = g + crtc->gamma_size; > for (i = 0; i < 256; i++) { > WREG32(AVIVO_DC_LUT_30_COLOR, > -(radeon_crtc->lut_r[i] << 20) | > -(radeon_crtc->lut_g[i] << 10) | > -(radeon_crtc->lut_b[i] << 0)); > + ((*r++ & 0xffc0) << 14) | > + ((*g++ & 0xffc0) << 4) | > + (*b++ >> 6)); > } > > /* Only change bit 0 of LUT_SEL, other bits are set elsewhere */ > @@ -76,6 +80,7 @@ static void dce4_crtc_load_lut(struct drm_crtc *crtc) > struct radeon_crtc *radeon_crtc = to_radeon_crtc(crtc); > struct drm_device *dev = crtc->dev; > struct radeon_device *rdev = dev->dev_private; > + u16 *r, *g, *b; > int i; > > DRM_DEBUG_KMS("%d\n", radeon_crtc->crtc_id); > @@ -93,11 +98,14 @@ static void dce4_crtc_load_lut(struct drm_crtc *crtc) > WREG32(EVERGREEN_DC_LUT_WRITE_EN_MASK + radeon_crtc->crtc_offset, > 0x0007); > > WREG32(EVERGREEN_DC_LUT_RW_INDEX + radeon_crtc->crtc_offset, 0); > + r = crtc->gamma_store; > + g = r + crtc->gamma_size; > + b = g + crtc->gamma_size; > for (i = 0; i < 256; i++) { > WREG32(EVERGREEN_DC_LUT_30_COLOR + radeon_crtc->crtc_offset, > - (radeon_crtc->lut_r[i] << 20) | > - (radeon_crtc->lut_g[i] << 10) | > - (radeon_crtc->lut_b[i] << 0)); > + ((*r++ & 0xffc0) << 14) | > + ((*g++ & 0xffc0) << 4) | > + (*b++ >> 6)); >
Re: [PATCH v5 04/14] drm: amd: remove dead code and pointless local lut storage
On Thu, Jul 13, 2017 at 12:25 PM, Peter Rosin wrote: > The redundant fb helpers .load_lut, .gamma_set and .gamma_get are > no longer used. Remove the dead code and hook up the crtc .gamma_set > to use the crtc gamma_store directly instead of duplicating that > info locally. > > Acked-by: Daniel Vetter > Signed-off-by: Peter Rosin Acked-by: Alex Deucher > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c | 24 > drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h | 1 - > drivers/gpu/drm/amd/amdgpu/dce_v10_0.c | 27 +++ > drivers/gpu/drm/amd/amdgpu/dce_v11_0.c | 27 +++ > drivers/gpu/drm/amd/amdgpu/dce_v6_0.c| 27 +++ > drivers/gpu/drm/amd/amdgpu/dce_v8_0.c| 27 +++ > drivers/gpu/drm/amd/amdgpu/dce_virtual.c | 23 --- > 7 files changed, 28 insertions(+), 128 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c > index c0d8c6ff6380..7dc378013a42 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c > @@ -312,31 +312,7 @@ static int amdgpu_fbdev_destroy(struct drm_device *dev, > struct amdgpu_fbdev *rfb > return 0; > } > > -/** Sets the color ramps on behalf of fbcon */ > -static void amdgpu_crtc_fb_gamma_set(struct drm_crtc *crtc, u16 red, u16 > green, > - u16 blue, int regno) > -{ > - struct amdgpu_crtc *amdgpu_crtc = to_amdgpu_crtc(crtc); > - > - amdgpu_crtc->lut_r[regno] = red >> 6; > - amdgpu_crtc->lut_g[regno] = green >> 6; > - amdgpu_crtc->lut_b[regno] = blue >> 6; > -} > - > -/** Gets the color ramps on behalf of fbcon */ > -static void amdgpu_crtc_fb_gamma_get(struct drm_crtc *crtc, u16 *red, u16 > *green, > - u16 *blue, int regno) > -{ > - struct amdgpu_crtc *amdgpu_crtc = to_amdgpu_crtc(crtc); > - > - *red = amdgpu_crtc->lut_r[regno] << 6; > - *green = amdgpu_crtc->lut_g[regno] << 6; > - *blue = amdgpu_crtc->lut_b[regno] << 6; > -} > - > static const struct drm_fb_helper_funcs amdgpu_fb_helper_funcs = { > - .gamma_set = amdgpu_crtc_fb_gamma_set, > - .gamma_get = amdgpu_crtc_fb_gamma_get, > .fb_probe = amdgpufb_create, > }; > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h > b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h > index 43a9d3aec6c4..39f7eda6091e 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h > @@ -369,7 +369,6 @@ struct amdgpu_atom_ss { > struct amdgpu_crtc { > struct drm_crtc base; > int crtc_id; > - u16 lut_r[256], lut_g[256], lut_b[256]; > bool enabled; > bool can_tile; > uint32_t crtc_offset; > diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c > b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c > index 9f78c03a2e31..c9580235e35b 100644 > --- a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c > +++ b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c > @@ -2267,6 +2267,7 @@ static void dce_v10_0_crtc_load_lut(struct drm_crtc > *crtc) > struct amdgpu_crtc *amdgpu_crtc = to_amdgpu_crtc(crtc); > struct drm_device *dev = crtc->dev; > struct amdgpu_device *adev = dev->dev_private; > + u16 *r, *g, *b; > int i; > u32 tmp; > > @@ -2304,11 +2305,14 @@ static void dce_v10_0_crtc_load_lut(struct drm_crtc > *crtc) > WREG32(mmDC_LUT_WRITE_EN_MASK + amdgpu_crtc->crtc_offset, 0x0007); > > WREG32(mmDC_LUT_RW_INDEX + amdgpu_crtc->crtc_offset, 0); > + r = crtc->gamma_store; > + g = r + crtc->gamma_size; > + b = g + crtc->gamma_size; > for (i = 0; i < 256; i++) { > WREG32(mmDC_LUT_30_COLOR + amdgpu_crtc->crtc_offset, > - (amdgpu_crtc->lut_r[i] << 20) | > - (amdgpu_crtc->lut_g[i] << 10) | > - (amdgpu_crtc->lut_b[i] << 0)); > + ((*r++ & 0xffc0) << 14) | > + ((*g++ & 0xffc0) << 4) | > + (*b++ >> 6)); > } > > tmp = RREG32(mmDEGAMMA_CONTROL + amdgpu_crtc->crtc_offset); > @@ -2624,15 +2628,6 @@ static int dce_v10_0_crtc_gamma_set(struct drm_crtc > *crtc, u16 *red, u16 *green, > u16 *blue, uint32_t size, > struct drm_modeset_acquire_ctx *ctx) > { > - struct amdgpu_crtc *amdgpu_crtc = to_amdgpu_crtc(crtc); > - int i; > - > - /* userspace palettes are always correct as is */ > - for (i = 0; i < size; i++) { > - amdgpu_crtc->lut_r[i] = red[i] >> 6; > - amdgpu_crtc->lut_g[i] = green[i] >> 6; > - amdgpu_crtc->lut_b[i] = blue[i] >> 6; > - } > dce_v10_0_crtc_load_lut(crtc); > > return 0; > @@ -2844,14 +2839,12 @@ static const struct
Re: [RFC v2] drm/hdcp: drm enum property for HDCP State
On Fri, Jul 14, 2017 at 04:51:43PM +0530, Ramalingam C wrote: > DRM connector property is created to represent the content protection > state of the connector and to configure the same. > > CP States defined: > DRM_CP_UNSUPPORTED - CP is not supported > DRM_CP_DISABLE - CP is disabled > DRM_CP_ENABLE - CP is enabled Why are we changing the names from the original version (that's used in CrOS)? It's not obvious what "CP" stands for when looking at the name. > > V2: Redesigned the property to match with CP needs of CrOS. > > Signed-off-by: Ramalingam C > --- > drivers/gpu/drm/drm_connector.c | 14 + > include/drm/drm_hdcp.h | 44 > + > include/drm/drm_mode_config.h | 5 + > 3 files changed, 63 insertions(+) > create mode 100644 include/drm/drm_hdcp.h > > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c > index 5cd61af..64895fb 100644 > --- a/drivers/gpu/drm/drm_connector.c > +++ b/drivers/gpu/drm/drm_connector.c > @@ -24,6 +24,7 @@ > #include > #include > #include > +#include > > #include "drm_crtc_internal.h" > #include "drm_internal.h" > @@ -617,6 +618,13 @@ static const struct drm_prop_enum_list > drm_link_status_enum_list[] = { > }; > DRM_ENUM_NAME_FN(drm_get_link_status_name, drm_link_status_enum_list) > > +static const struct drm_prop_enum_list drm_cp_enum_list[] = { > + { DRM_CP_UNSUPPORTED, "CP Not Supported" }, > + { DRM_CP_DISABLE, "Disable CP" }, > + { DRM_CP_ENABLE,"Enable CP for Type0 content" }, Type0 has no meaning in this case. The whole point of using "Content Protection" is to abstract the means of protection from userspace. The names are also much more verbose than they need be. "Unsupported", "Disabled", "Enabled" are fine. > +}; > +DRM_ENUM_NAME_FN(drm_get_cp_status_name, drm_cp_enum_list) > + > /** > * drm_display_info_set_bus_formats - set the supported bus formats > * @info: display info to store bus formats in > @@ -789,6 +797,12 @@ int drm_connector_create_standard_properties(struct > drm_device *dev) > return -ENOMEM; > dev->mode_config.link_status_property = prop; > > + prop = drm_property_create_enum(dev, 0, "cp", drm_cp_enum_list, > +ARRAY_SIZE(drm_cp_enum_list)); Your property name here, on the other hand, is not descriptive enough. Please expand to something meaningful. > + if (!prop) > + return -ENOMEM; > + dev->mode_config.cp_property = prop; > + > return 0; > } > > diff --git a/include/drm/drm_hdcp.h b/include/drm/drm_hdcp.h Why create a new header for this? That seems a little excessive. Sean > new file mode 100644 > index 000..f6d0160 > --- /dev/null > +++ b/include/drm/drm_hdcp.h > @@ -0,0 +1,44 @@ > +/* > + * Copyright (c) 2017 Intel Corporation > + * > + * Permission to use, copy, modify, distribute, and sell this software and > its > + * documentation for any purpose is hereby granted without fee, provided that > + * the above copyright notice appear in all copies and that both that > copyright > + * notice and this permission notice appear in supporting documentation, and > + * that the name of the copyright holders not be used in advertising or > + * publicity pertaining to distribution of the software without specific, > + * written prior permission. The copyright holders make no representations > + * about the suitability of this software for any purpose. It is provided > "as > + * is" without express or implied warranty. > + * > + * THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS > SOFTWARE, > + * INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS, IN NO > + * EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY SPECIAL, INDIRECT OR > + * CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM LOSS OF > USE, > + * DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR OTHER > + * TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR > PERFORMANCE > + * OF THIS SOFTWARE. > + */ > + > +/* > + * Header for HDCP specific data > + */ > + > +#ifndef __DRM_HDCP_H__ > +#define __DRM_HDCP_H__ > + > +/** > + * CP property related information > + */ > +enum drm_cp_state { > + > + /* HDCP sink and Src dont have any common HDCP ver supported */ > + DRM_CP_UNSUPPORTED, > + > + /* Disable Content Protection */ > + DRM_CP_DISABLE, > + > + /* Enable Content Protection */ > + DRM_CP_ENABLE, > +}; > +#endif /* __DRM_HDCP_H__ */ > diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h > index 4298171..7acb8b2 100644 > --- a/include/drm/drm_mode_config.h > +++ b/include/drm/drm_mode_config.h > @@ -538,6 +538,11 @@ struct drm_mode_config { >*/ > struct drm_property *link_status_property; > /** > + * @cp_property: Default connector property for
Re: Linux: Smooth splashscreen with system having weston with drm-backend
On Fri, Jul 14, 2017 at 03:07:11PM +0530, Vikas Patil wrote: > On Thu, Jul 13, 2017 at 7:20 PM, Daniel Vetter wrote: > > On Thu, Jul 13, 2017 at 3:33 PM, Vikas Patil wrote: > >> Dear All, > >> > >> I am looking for an solution to have early smooth splashscreen on the > >> Linux system with Weston and drm-backend. > >> > >> I tried showing splashscreen using Linux logo and fbcon Linux features > >> but it is not smooth as when system boots logo gets displayed via > >> kernel and as the Weston starts > >> I see flicker and blackscreen. > >> > >> Another approach I tried is having standalone drm api based > >> application [1] which uses the one of the available hardware > >> plane/overlay for displaying splash image and Weston is > >> starting and uses default plane which is different from plane utilized > >> by above application. but still I see flicker and black screen when > >> Weston starts. > >> > >> I want splash to be displayed on one of the h/w plane and Weston > >> should start in background without any flicker and black screen on > >> some other plane. > >> > >> Anyone here before achieved such a use case with such system? Looking > >> for inputs/suggestions/ideas to achieve this. It is ok if something > >> needs to be hacked/changd at any level (kernel driver, weston). > >> > >> This is required on platform such as TI (Jacinto) and Intel (Broxton) > >> or devices based on drm graphics stack. > >> > >> [1] > >> http://git.ti.com/glsdk/example-applications/blobs/39080525baca7bf136f2412d62436366a736af6b/drm-tests/drm_z_alpha.c > >> > >> Thanking you all for your time and inputs in advance. > > > > If you make sure you have matching modes between the 2 users of drm > > this should work. kms drivers (especially if they are already > > converted to atomic) will automatically optimize transitions to not > > flicker (if possible). > > > > I am not sure how this will work. As per what I understood is when > drmModeSetCrtc() is called it uses the primary/default plane and if > application wants to use any other plane then it need to setup the > planes using drmModeSetPlane() in application before doing > drmModeSetCrtc() so when primary plane scan-outs at crtc phase it also > knows it has to compose from using other planes too. This is what > splash application is doing. But when I run weston after splash app, > weston I think calls drmModeSetCrtc() without the knowledge of planes > and previous configuration overrides so it doesn't work. Oh that doesn't work, you can't have multiple userspace things using the display at the same time. You need to composite the different planes in userspace first. KMS only allows one compositor at the same time (the so called drm master). -Daniel > > If you still flicker then it's either a kernel driver issue, a hw > > limitations (some hw needs a full modeset for e.g. changing the bit > > depth) or you program two different things. > > > > Also, you must ensure that the boot-splash does not quit until weston > > has fully taken over, otherwise kms will first shut down the screen > > (when the splash quits), then re-enable it when weston starts. > > > I have tried adding while with sleep in splash app but still the > behavior is same. FYI, I am doing this on TI's Jacinto6 platform but > similar required on intel platform too. > > I was also thinking if there is something can be done in drm-backend > of weston. e.g disabling mode setting or some code but not sure if > weston will work with it.. > > Thanks & regards, > Vikash -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v5 03/14] drm/fb-helper: separate the fb_setcmap helper into atomic and legacy paths
On Thu, Jul 13, 2017 at 06:25:27PM +0200, Peter Rosin wrote: > The legacy path implements setcmap in terms of crtc .gamma_set. > > The atomic path implements setcmap by directly updating the crtc gamma_lut > property. > > This has a couple of benefits: > - it makes the redundant fb helpers .load_lut, .gamma_set and .gamma_get > completely obsolete. They are now unused and subject for removal. > - atomic drivers that support clut modes get fbdev support for those from > the drm core. This includes atmel-hlcdc, but perhaps others as well? > > Signed-off-by: Peter Rosin Ok, I merged the core parts. I'll wait with the driver stuff for a bit more (maybe 1-2 weeks) for more acks. Pls remind me in case I forget to pull them in. Thanks a lot for doing this, great work! -Daniel > --- > drivers/gpu/drm/drm_fb_helper.c | 231 > > 1 file changed, 160 insertions(+), 71 deletions(-) > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c > index 721511da4de6..42090fe00ef9 100644 > --- a/drivers/gpu/drm/drm_fb_helper.c > +++ b/drivers/gpu/drm/drm_fb_helper.c > @@ -1195,27 +1195,6 @@ void drm_fb_helper_set_suspend_unlocked(struct > drm_fb_helper *fb_helper, > } > EXPORT_SYMBOL(drm_fb_helper_set_suspend_unlocked); > > -static int setcolreg(struct drm_crtc *crtc, u16 red, u16 green, > - u16 blue, u16 regno, struct fb_info *info) > -{ > - struct drm_fb_helper *fb_helper = info->par; > - struct drm_framebuffer *fb = fb_helper->fb; > - > - /* > - * The driver really shouldn't advertise pseudo/directcolor > - * visuals if it can't deal with the palette. > - */ > - if (WARN_ON(!fb_helper->funcs->gamma_set || > - !fb_helper->funcs->gamma_get)) > - return -EINVAL; > - > - WARN_ON(fb->format->cpp[0] != 1); > - > - fb_helper->funcs->gamma_set(crtc, red, green, blue, regno); > - > - return 0; > -} > - > static int setcmap_pseudo_palette(struct fb_cmap *cmap, struct fb_info *info) > { > u32 *palette = (u32 *)info->pseudo_palette; > @@ -1248,57 +1227,138 @@ static int setcmap_pseudo_palette(struct fb_cmap > *cmap, struct fb_info *info) > return 0; > } > > -/** > - * drm_fb_helper_setcmap - implementation for &fb_ops.fb_setcmap > - * @cmap: cmap to set > - * @info: fbdev registered by the helper > - */ > -int drm_fb_helper_setcmap(struct fb_cmap *cmap, struct fb_info *info) > +static int setcmap_legacy(struct fb_cmap *cmap, struct fb_info *info) > { > struct drm_fb_helper *fb_helper = info->par; > - struct drm_device *dev = fb_helper->dev; > - const struct drm_crtc_helper_funcs *crtc_funcs; > - u16 *red, *green, *blue, *transp; > struct drm_crtc *crtc; > u16 *r, *g, *b; > - int i, j, rc = 0; > - int start; > + int i, ret = 0; > > - if (oops_in_progress) > - return -EBUSY; > + drm_modeset_lock_all(fb_helper->dev); > + for (i = 0; i < fb_helper->crtc_count; i++) { > + crtc = fb_helper->crtc_info[i].mode_set.crtc; > + if (!crtc->funcs->gamma_set || !crtc->gamma_size) > + return -EINVAL; > > - mutex_lock(&fb_helper->lock); > - if (!drm_fb_helper_is_bound(fb_helper)) { > - mutex_unlock(&fb_helper->lock); > - return -EBUSY; > + if (cmap->start + cmap->len > crtc->gamma_size) > + return -EINVAL; > + > + r = crtc->gamma_store; > + g = r + crtc->gamma_size; > + b = g + crtc->gamma_size; > + > + memcpy(r + cmap->start, cmap->red, cmap->len * sizeof(*r)); > + memcpy(g + cmap->start, cmap->green, cmap->len * sizeof(*g)); > + memcpy(b + cmap->start, cmap->blue, cmap->len * sizeof(*b)); > + > + ret = crtc->funcs->gamma_set(crtc, r, g, b, > + crtc->gamma_size, NULL); > + if (ret) > + return ret; > } > + drm_modeset_unlock_all(fb_helper->dev); > > - drm_modeset_lock_all(dev); > - if (info->fix.visual == FB_VISUAL_TRUECOLOR) { > - rc = setcmap_pseudo_palette(cmap, info); > - goto out; > + return ret; > +} > + > +static struct drm_property_blob *setcmap_new_gamma_lut(struct drm_crtc *crtc, > +struct fb_cmap *cmap) > +{ > + struct drm_device *dev = crtc->dev; > + struct drm_property_blob *gamma_lut; > + struct drm_color_lut *lut; > + int size = crtc->gamma_size; > + int i; > + > + if (!size || cmap->start + cmap->len > size) > + return ERR_PTR(-EINVAL); > + > + gamma_lut = drm_property_create_blob(dev, sizeof(*lut) * size, NULL); > + if (IS_ERR(gamma_lut)) > + return gamma_lut; > + > + lut = (struct drm_color_lut *)gamma_lut->data; > + if (cmap->start || cmap->len != size) { >
Re: [PATCH 2/2] dim: Try to gc the rr-cache
On Fri, Jul 14, 2017 at 12:57:23PM +0300, Jani Nikula wrote: > On Thu, 13 Jul 2017, Sean Paul wrote: > > On Wed, Jul 12, 2017 at 02:12:24PM +0200, Daniel Vetter wrote: > >> The problem is that we have a distributed cache - every committer has > >> a copy. Which means even just a slight clock skew will make sure that > >> a naive gc algorithm results in lots of thrashing around. > >> > >> To fix this add a huge hysteresis: Only add files newer than 1 day, > >> and only remove them when older than 60 days. As long as people have > >> reasonable accurate clocks on their machines this should work. > >> > >> A different problem is that we can't use filesystem timestamps (and > >> hence can't use git rerere gc): When someone comes back from vacations > >> and updates git rerere, all the files will have current timestamps, > >> even when they've been pushed out weeks ago. To fix that, use the git > >> log to judge old files to remove. Also, remove old files before adding > >> new ones, to avoid confusion. > >> > >> Also, we need to teach the cp -r to preserve timestamps, otherwise > >> this won't work. > >> > >> v2: Use git log to remove old files. > >> > >> v3: Remove the debug uncommenting (Sean). > >> > >> v4: Split out code movement and explain better what's going on (Jani). > > > > Yeah, much easier to digest with the split. > > > > Reviewed-by: Sean Paul > > I'll trust that and hope for the best. ;) Pushed and asked everyone I managed to ping over irc to upgrade. Let's see how often we have to push out a revert until the old stuff is dead for good (I already screwed up twice locally ...). -Daniel > > BR, > Jani. > > > > > >> > >> Signed-off-by: Daniel Vetter > >> --- > >> dim | 10 -- > >> 1 file changed, 8 insertions(+), 2 deletions(-) > >> > >> diff --git a/dim b/dim > >> index b788edd29653..79d616cbf354 100755 > >> --- a/dim > >> +++ b/dim > >> @@ -513,9 +513,15 @@ function commit_rerere_cache > >> > >>git pull >& /dev/null > >>rm $(rr_cache_dir)/rr-cache -Rf &> /dev/null || true > >> - cp $(rr_cache_dir)/* rr-cache -r > >> + cp $(rr_cache_dir)/* rr-cache -r --preserve=timestamps > >>git add ./*.patch >& /dev/null || true > >> - git add rr-cache/* > /dev/null > >> + for file in $(git ls-files); do > >> + if ! git log --since="60 days ago" --name-only -- $file > >> | grep $file &> /dev/null; then > >> + git rm $file &> /dev/null > >> + echo deleting $file > >> + fi > >> + done > >> + find rr-cache/ -ctime -1 -type f -print0 | xargs -0 git add > > >> /dev/null > >>git rm rr-cache/rr-cache &> /dev/null || true > >>if git commit -m "$time: $integration_branch rerere cache > >> update" >& /dev/null; then > >>echo -n "New commit. " > >> -- > >> 2.13.2 > >> > >> ___ > >> dri-devel mailing list > >> dri-devel@lists.freedesktop.org > >> https://lists.freedesktop.org/mailman/listinfo/dri-devel > > -- > Jani Nikula, Intel Open Source Technology Center -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] [PATCH 1/5] drm/i915/fbdev: Always forward hotplug events
On Thu, Jul 06, 2017 at 03:05:19PM +0200, Maarten Lankhorst wrote: > Op 06-07-17 om 15:00 schreef Daniel Vetter: > > With deferred fbdev setup we always need to forward hotplug events, > > even if fbdev isn't fully set up yet. Otherwise the deferred setup > > will neer happen. > > > > Originally this check was added in > > > > commit c45eb4fed12d278d3619f1904885bd0d7bcbf036 (tag: > > drm-intel-next-fixes-2016-08-05) > > Author: Chris Wilson > > Date: Wed Jul 13 18:34:45 2016 +0100 > > > > drm/i915/fbdev: Check for the framebuffer before use > > > > But the specific case of the hotplug function blowing up was fixed in > > > > commit 50c3dc970a09b3b60422a58934cc27a413288bab > > Author: Daniel Vetter > > Date: Fri Jun 27 17:19:22 2014 +0200 > > > > drm/fb-helper: Fix hpd vs. initial config races > > > > Cc: Maarten Lankhorst > > Cc: Mika Kuoppala > > Cc: Chris Wilson > > Signed-off-by: Daniel Vetter > > --- > > drivers/gpu/drm/i915/intel_fbdev.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_fbdev.c > > b/drivers/gpu/drm/i915/intel_fbdev.c > > index f11ee039ff45..5536591d3da0 100644 > > --- a/drivers/gpu/drm/i915/intel_fbdev.c > > +++ b/drivers/gpu/drm/i915/intel_fbdev.c > > @@ -821,7 +821,7 @@ void intel_fbdev_output_poll_changed(struct drm_device > > *dev) > > { > > struct intel_fbdev *ifbdev = to_i915(dev)->fbdev; > > > > - if (ifbdev && ifbdev->vma) > > + if (ifbdev) > > drm_fb_helper_hotplug_event(&ifbdev->helper); > > } > > > > Reviewed-by: Maarten Lankhorst > > Could you also change the @intel.com to @linux.intel.com in second patch? :-) Done, thx for your review, first 2 patches merged to drm-intel. I'll apply the others once drm-misc is resynced. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 14/14] [media] fix warning on v4l2_subdev_call() result interpreted as bool
On Fri, Jul 14, 2017 at 03:55:26PM +0300, Dan Carpenter wrote: > I don't agree with it as a static analysis dev... What I mean is if it's a macro that returns -ENODEV or a function that returns -ENODEV, they should both be treated the same. The other warnings this check prints are quite clever. regards, dan carpenter ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 14/14] [media] fix warning on v4l2_subdev_call() result interpreted as bool
Ah... I see why it's complaining about these ones in particular... I don't agree with it as a static analysis dev, and I don't like the changes too much. But since it's only generating a hand full of warnings then I don't care. regards, dan carpenter ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 14/14] [media] fix warning on v4l2_subdev_call() result interpreted as bool
On Fri, Jul 14, 2017 at 11:36:56AM +0200, Arnd Bergmann wrote: > @@ -1158,7 +1158,8 @@ static void ccdc_configure(struct isp_ccdc_device *ccdc) >*/ > fmt_src.pad = pad->index; > fmt_src.which = V4L2_SUBDEV_FORMAT_ACTIVE; > - if (!v4l2_subdev_call(sensor, pad, get_fmt, NULL, &fmt_src)) { > + ret = v4l2_subdev_call(sensor, pad, get_fmt, NULL, &fmt_src); > + if (!ret) { > fmt_info = omap3isp_video_format_info(fmt_src.format.code); > depth_in = fmt_info->width; > } Is the original code buggy? media/platform/omap3isp/ispccdc.c 1156 /* Compute the lane shifter shift value and enable the bridge when the 1157 * input format is a non-BT.656 YUV variant. 1158 */ 1159 fmt_src.pad = pad->index; 1160 fmt_src.which = V4L2_SUBDEV_FORMAT_ACTIVE; 1161 if (!v4l2_subdev_call(sensor, pad, get_fmt, NULL, &fmt_src)) { 1162 fmt_info = omap3isp_video_format_info(fmt_src.format.code); 1163 depth_in = fmt_info->width; 1164 } If v4l2_subdev_call() then depth_in is zero. 1165 1166 fmt_info = omap3isp_video_format_info(format->code); 1167 depth_out = fmt_info->width; 1168 shift = depth_in - depth_out; How do we know that this subtraction doesn't set "shift" to a very high positive? 1169 1170 if (ccdc->bt656) 1171 bridge = ISPCTRL_PAR_BRIDGE_DISABLE; 1172 else if (fmt_info->code == MEDIA_BUS_FMT_YUYV8_2X8) 1173 bridge = ISPCTRL_PAR_BRIDGE_LENDIAN; 1174 else if (fmt_info->code == MEDIA_BUS_FMT_UYVY8_2X8) 1175 bridge = ISPCTRL_PAR_BRIDGE_BENDIAN; 1176 else 1177 bridge = ISPCTRL_PAR_BRIDGE_DISABLE; 1178 1179 omap3isp_configure_bridge(isp, ccdc->input, parcfg, shift, bridge); regards, dan carpenter ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 14/14] [media] fix warning on v4l2_subdev_call() result interpreted as bool
On Fri, Jul 14, 2017 at 2:05 PM, Dan Carpenter wrote: > Changing: > > - if (!frob()) { > + if (frob() == 0) { > > is a totally pointless change. They're both bad, because they're doing > success testing instead of failure testing, but probably the second one > is slightly worse. > > This warning seems dumb. I can't imagine it has even a 10% success rate > at finding real bugs. Just disable it. > > Changing the code to propagate error codes, is the right thing of course > so long as it doesn't introduce bugs. It found a two of bugs that I fixed earlier: f0e8faa7a5e8 ("ARM: ux500: fix prcmu_is_cpu_in_wfi() calculation") af15769ffab1 ("scsi: mvsas: fix command_active typo") plus three patches from this series: 1. staging:iio:resolver:ad2s1210 fix negative IIO_ANGL_VEL read 2. isdn: isdnloop: suppress a gcc-7 warning (my patch is wrong, as Joe pointed out there is a real bug) 3. drm/vmwgfx: avoid gcc-7 parentheses (here, Linus had a better analysis of the problem, so we should consider that a bug as well) I would estimate around 25% success rate here, which isn't that bad for a new warning. I agree that most of the false positives are really dumb though. Arnd ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 14/14] [media] fix warning on v4l2_subdev_call() result interpreted as bool
Changing: - if (!frob()) { + if (frob() == 0) { is a totally pointless change. They're both bad, because they're doing success testing instead of failure testing, but probably the second one is slightly worse. This warning seems dumb. I can't imagine it has even a 10% success rate at finding real bugs. Just disable it. Changing the code to propagate error codes, is the right thing of course so long as it doesn't introduce bugs. regards, dan carpenter ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[Bug 101528] RX460 Memory clock stays high until card / display is "used"
https://bugs.freedesktop.org/show_bug.cgi?id=101528 Sverd Johnsen changed: What|Removed |Added Resolution|--- |FIXED Summary|RX460 Memory clock stays|RX460 Memory clock stays |high until card is "used" |high until card / display ||is "used" Status|NEW |RESOLVED -- You are receiving this mail because: You are the assignee for the bug.___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[Bug 101528] RX460 Memory clock stays high until card is "used"
https://bugs.freedesktop.org/show_bug.cgi?id=101528 --- Comment #5 from Sverd Johnsen --- Works for me on 4.11.10. Display off, MCLK is low and card temperature is 27°C as expected. Thanks. -- You are receiving this mail because: You are the assignee for the bug.___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[RFC v2] drm/hdcp: drm enum property for HDCP State
DRM connector property is created to represent the content protection state of the connector and to configure the same. CP States defined: DRM_CP_UNSUPPORTED - CP is not supported DRM_CP_DISABLE - CP is disabled DRM_CP_ENABLE - CP is enabled V2: Redesigned the property to match with CP needs of CrOS. Signed-off-by: Ramalingam C --- drivers/gpu/drm/drm_connector.c | 14 + include/drm/drm_hdcp.h | 44 + include/drm/drm_mode_config.h | 5 + 3 files changed, 63 insertions(+) create mode 100644 include/drm/drm_hdcp.h diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c index 5cd61af..64895fb 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c @@ -24,6 +24,7 @@ #include #include #include +#include #include "drm_crtc_internal.h" #include "drm_internal.h" @@ -617,6 +618,13 @@ static const struct drm_prop_enum_list drm_link_status_enum_list[] = { }; DRM_ENUM_NAME_FN(drm_get_link_status_name, drm_link_status_enum_list) +static const struct drm_prop_enum_list drm_cp_enum_list[] = { + { DRM_CP_UNSUPPORTED, "CP Not Supported" }, + { DRM_CP_DISABLE, "Disable CP" }, + { DRM_CP_ENABLE,"Enable CP for Type0 content" }, +}; +DRM_ENUM_NAME_FN(drm_get_cp_status_name, drm_cp_enum_list) + /** * drm_display_info_set_bus_formats - set the supported bus formats * @info: display info to store bus formats in @@ -789,6 +797,12 @@ int drm_connector_create_standard_properties(struct drm_device *dev) return -ENOMEM; dev->mode_config.link_status_property = prop; + prop = drm_property_create_enum(dev, 0, "cp", drm_cp_enum_list, + ARRAY_SIZE(drm_cp_enum_list)); + if (!prop) + return -ENOMEM; + dev->mode_config.cp_property = prop; + return 0; } diff --git a/include/drm/drm_hdcp.h b/include/drm/drm_hdcp.h new file mode 100644 index 000..f6d0160 --- /dev/null +++ b/include/drm/drm_hdcp.h @@ -0,0 +1,44 @@ +/* + * Copyright (c) 2017 Intel Corporation + * + * Permission to use, copy, modify, distribute, and sell this software and its + * documentation for any purpose is hereby granted without fee, provided that + * the above copyright notice appear in all copies and that both that copyright + * notice and this permission notice appear in supporting documentation, and + * that the name of the copyright holders not be used in advertising or + * publicity pertaining to distribution of the software without specific, + * written prior permission. The copyright holders make no representations + * about the suitability of this software for any purpose. It is provided "as + * is" without express or implied warranty. + * + * THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS SOFTWARE, + * INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS, IN NO + * EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY SPECIAL, INDIRECT OR + * CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM LOSS OF USE, + * DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR OTHER + * TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR PERFORMANCE + * OF THIS SOFTWARE. + */ + +/* + * Header for HDCP specific data + */ + +#ifndef __DRM_HDCP_H__ +#define __DRM_HDCP_H__ + +/** + * CP property related information + */ +enum drm_cp_state { + + /* HDCP sink and Src dont have any common HDCP ver supported */ + DRM_CP_UNSUPPORTED, + + /* Disable Content Protection */ + DRM_CP_DISABLE, + + /* Enable Content Protection */ + DRM_CP_ENABLE, +}; +#endif /* __DRM_HDCP_H__ */ diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h index 4298171..7acb8b2 100644 --- a/include/drm/drm_mode_config.h +++ b/include/drm/drm_mode_config.h @@ -538,6 +538,11 @@ struct drm_mode_config { */ struct drm_property *link_status_property; /** +* @cp_property: Default connector property for CP +* of a connector +*/ + struct drm_property *cp_property; + /** * @plane_type_property: Default plane property to differentiate * CURSOR, PRIMARY and OVERLAY legacy uses of planes. */ -- 2.7.4 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 13/18] dt-bindings: vendor: Add Huarui Lighting
On Thu, Jul 13, 2017 at 10:13 PM, Maxime Ripard wrote: > Huarui Lighting makes display panel, add it to the list of panels. I could not find any information on "Huarui Lighting" within the context of LCD panels. The company I found makes LED lighting fixtures, floodlights, and high power LED drivers. This might not be a legitimate branding? ChenYu > > Signed-off-by: Maxime Ripard > --- > Documentation/devicetree/bindings/vendor-prefixes.txt | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt > b/Documentation/devicetree/bindings/vendor-prefixes.txt > index afbb47ce50dd..cc5850a325f8 100644 > --- a/Documentation/devicetree/bindings/vendor-prefixes.txt > +++ b/Documentation/devicetree/bindings/vendor-prefixes.txt > @@ -142,6 +142,7 @@ holtHolt Integrated Circuits, Inc. > honeywell Honeywell > hp Hewlett Packard > holtek Holtek Semiconductor, Inc. > +huarui Huarui Lighting Co. Ltd > hwacom HwaCom Systems Inc. > i2se I2SE GmbH > ibmInternational Business Machines (IBM) > -- > git-series 0.9.1 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 4/4] drm/sun4i: make sure we don't have a commit pending
Hi, On Thu, Jul 13, 2017 at 10:41 PM, Maxime Ripard wrote: > In the earlier display engine designs, any register access while a commit > is pending is forbidden. > > One of the symptoms is that reading a register will return another, random, > register value which can lead to register corruptions if we ever do a > read/modify/write cycle. Alternatively, if changes to the backend (layers) are guaranteed to happen while the CRTC is disabled (which seems to be the case after looking at drm_atomic_helper_commit_planes and drm_atomic_helper_commit_tail), we could just turn on register auto-commit all the time and not deal with this. ChenYu > > Signed-off-by: Maxime Ripard > --- > drivers/gpu/drm/sun4i/sun4i_backend.c | 14 ++ > drivers/gpu/drm/sun4i/sun4i_crtc.c| 1 + > 2 files changed, 15 insertions(+) > > diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.c > b/drivers/gpu/drm/sun4i/sun4i_backend.c > index cf480218daa5..ce1f40f7511e 100644 > --- a/drivers/gpu/drm/sun4i/sun4i_backend.c > +++ b/drivers/gpu/drm/sun4i/sun4i_backend.c > @@ -67,6 +67,19 @@ static void sun4i_backend_commit(struct sunxi_engine > *engine) > SUN4I_BACKEND_REGBUFFCTL_LOADCTL); > } > > +static int sun4i_backend_commit_poll(struct sunxi_engine *engine) > +{ > + u32 val; > + > + DRM_DEBUG_DRIVER("Polling for the commit to end\n"); > + > + return regmap_read_poll_timeout(engine->regs, > + SUN4I_BACKEND_REGBUFFCTL_REG, > + val, > + !(val & > SUN4I_BACKEND_REGBUFFCTL_LOADCTL), > + 100, 5); > +} > + > void sun4i_backend_layer_enable(struct sun4i_backend *backend, > int layer, bool enable) > { > @@ -330,6 +343,7 @@ static int sun4i_backend_of_get_id(struct device_node > *node) > > static const struct sunxi_engine_ops sun4i_backend_engine_ops = { > .commit = sun4i_backend_commit, > + .commit_poll= sun4i_backend_commit_poll, > .layers_init= sun4i_layers_init, > .apply_color_correction = > sun4i_backend_apply_color_correction, > .disable_color_correction = > sun4i_backend_disable_color_correction, > diff --git a/drivers/gpu/drm/sun4i/sun4i_crtc.c > b/drivers/gpu/drm/sun4i/sun4i_crtc.c > index 2eba1d8639d8..31550493fa1d 100644 > --- a/drivers/gpu/drm/sun4i/sun4i_crtc.c > +++ b/drivers/gpu/drm/sun4i/sun4i_crtc.c > @@ -34,6 +34,7 @@ static void sun4i_crtc_atomic_begin(struct drm_crtc *crtc, > struct drm_crtc_state *old_state) > { > struct sun4i_crtc *scrtc = drm_crtc_to_sun4i_crtc(crtc); > + struct sunxi_engine *engine = scrtc->engine; > struct drm_device *dev = crtc->dev; > unsigned long flags; > > -- > git-series 0.9.1 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
drm/edid: api to register cea modes if no edid supported
Adding drm_add_modes_noedid_cea API for supporting cea modes for drm devices which does not have panel framework or edid support. Protocols like SDI whic have minimal support in linux kernel can benifit from this. There is already a API drm_add_modes_noedid, but that supports only dmt modes. Signed-off-by: Saurabh Sengar --- drivers/gpu/drm/drm_edid.c | 48 ++ include/drm/drm_edid.h | 2 ++ 2 files changed, 50 insertions(+) diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index ec77bd3..08cfd9d 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -4110,6 +4110,54 @@ int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid) EXPORT_SYMBOL(drm_add_edid_modes); /** + * drm_add_modes_noedid_cea - add cea modes for the connectors without EDID + * @connector: connector we're probing + * @hdisplay: the horizontal display limit + * @vdisplay: the vertical display limit + * + * Add the cea modes to the connector's mode list. Only when the + * hdisplay/vdisplay is not beyond the given limit, it will be added. + * + * Return: The number of modes added or 0 if we couldn't find any. + */ +int drm_add_modes_noedid_cea(struct drm_connector *connector, + int hdisplay, int vdisplay) +{ + int i, count, num_modes = 0; + struct drm_display_mode *mode; + struct drm_device *dev = connector->dev; + + count = ARRAY_SIZE(edid_cea_modes); + if (hdisplay < 0) + hdisplay = 0; + if (vdisplay < 0) + vdisplay = 0; + + for (i = 0; i < count; i++) { + const struct drm_display_mode *ptr = &edid_cea_modes[i]; + if (hdisplay && vdisplay) { + /* +* Only when two are valid, they will be used to check +* whether the mode should be added to the mode list of +* the connector. +*/ + if (ptr->hdisplay > hdisplay || + ptr->vdisplay > vdisplay) + continue; + } + if (drm_mode_vrefresh(ptr) > 61) + continue; + mode = drm_mode_duplicate(dev, ptr); + if (mode) { + drm_mode_probed_add(connector, mode); + num_modes++; + } + } + return num_modes; +} +EXPORT_SYMBOL(drm_add_modes_noedid_cea); + +/** * drm_add_modes_noedid - add modes for the connectors without EDID * @connector: connector we're probing * @hdisplay: the horizontal display limit diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h index c3a7d44..67f4c26 100644 --- a/include/drm/drm_edid.h +++ b/include/drm/drm_edid.h @@ -445,6 +445,8 @@ bool drm_detect_monitor_audio(struct edid *edid); bool drm_rgb_quant_range_selectable(struct edid *edid); int drm_add_modes_noedid(struct drm_connector *connector, int hdisplay, int vdisplay); +int drm_add_modes_noedid_cea(struct drm_connector *connector, +int hdisplay, int vdisplay); void drm_set_preferred_mode(struct drm_connector *connector, int hpref, int vpref); -- 2.1.1 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 08/14] v4l: vsp1: Add support for new VSP2-BS, VSP2-DL and VSP2-D instances
On 14/07/17 00:31, Laurent Pinchart wrote: > Hi Kieran, > > On Thursday 13 Jul 2017 18:49:19 Kieran Bingham wrote: >> On 26/06/17 19:12, Laurent Pinchart wrote: >>> New Gen3 SoCs come with two new VSP2 variants names VSP2-BS and VSP2-DL, >>> as well as a new VSP2-D variant on V3M and V3H SoCs. Add new entries for >>> them in the VSP device info table. >>> >>> Signed-off-by: Laurent Pinchart >>> >> >> Code in the patch looks OK - but I can't see where the difference between >> the horizontal widths are supported between VSPD H3/VC >> >> I see this in the datasheet: (32.1.1.6 in this particular part) >> >> Direct connection to display module >> — Supporting 4096 pixels in horizontal direction [R-Car H3/R-Car M3-W/ R-Car >> M3-N] >> — Supporting 2048 pixels in horizontal direction [R-Car V3M/R-Car V3H/R-Car >> D3/R-Car E3] >> >> Do we have this information encoded anywhere? or are they just talking about >> maximum performance capability there? > > No, we don't. It's a limit that we should have. I think we should fix that in > a separate patch, as the 4096 pixels limit isn't implemented either. I'm not so worried about these limits (unless they cause the hardware to hang), but I think they should be encoded somewhere, and I would certainly count that as a separate patch. Of course (excluding pipelines using BRU/BRS) the partition algorithm could provide the capability to support image processing beyond limitations of the pipeline maximum size... But this can't cater for throughput limitations of bandwidth so there will be a limit to how big an image we really want to support ... Non realtime processing of large megapixel images might be relevant though - but still not a use case to worry about here. >> Also some features that are implied as supported aren't mentioned - but >> that's not a blocker to adding in the initial devices at all. >> >> Therefore: >> >> Reviewed-by: Kieran Bingham >> >>> --- >>> >>> drivers/media/platform/vsp1/vsp1_drv.c | 24 >>> drivers/media/platform/vsp1/vsp1_regs.h | 15 +-- >>> 2 files changed, 37 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/media/platform/vsp1/vsp1_drv.c >>> b/drivers/media/platform/vsp1/vsp1_drv.c index 6a9aeb71aedf..c4f2ac61f7d2 >>> 100644 >>> --- a/drivers/media/platform/vsp1/vsp1_drv.c >>> +++ b/drivers/media/platform/vsp1/vsp1_drv.c >>> @@ -710,6 +710,14 @@ static const struct vsp1_device_info >>> vsp1_device_infos[] = {> >>> .num_bru_inputs = 5, >>> .uapi = true, >>> }, { >>> + .version = VI6_IP_VERSION_MODEL_VSPBS_GEN3, >>> + .model = "VSP2-BS", >>> + .gen = 3, >>> + .features = VSP1_HAS_BRS, >> >> 32.1.1.5 implies: >> | VSP1_HAS_WPF_VFLIP >> >> But Figure 32.5 implies that it doesn't ... > > The figures only tell whether the full combination of rotation and H/V flip > is > available. I think you're right, I'll add VSP1_HAS_WPF_VFLIP. > >> Figure 32.5 also implies that | VSP1_HAS_CLU is there too on both RPF0, and >> RPF1 > > Note that CLUT != CLU. I know it's confusing :-) Oh ! :-S ... /me goes back to the datasheet ... > >>> + .rpf_count = 2, >>> + .wpf_count = 1, >>> + .uapi = true, >>> + }, { >>> .version = VI6_IP_VERSION_MODEL_VSPD_GEN3, >>> .model = "VSP2-D", >>> .gen = 3, >>> @@ -717,6 +725,22 @@ static const struct vsp1_device_info >>> vsp1_device_infos[] = {> >>> .rpf_count = 5, >>> .wpf_count = 2, >>> .num_bru_inputs = 5, >>> + }, { >>> + .version = VI6_IP_VERSION_MODEL_VSPD_V3, >>> + .model = "VSP2-D", >>> + .gen = 3, >>> + .features = VSP1_HAS_BRS | VSP1_HAS_BRU | VSP1_HAS_LIF, >>> + .rpf_count = 5, >>> + .wpf_count = 1, >>> + .num_bru_inputs = 5, >>> + }, { >>> + .version = VI6_IP_VERSION_MODEL_VSPDL_GEN3, >>> + .model = "VSP2-DL", >>> + .gen = 3, >>> + .features = VSP1_HAS_BRS | VSP1_HAS_BRU | VSP1_HAS_LIF, >> >> Hrm. 32.1.1.7 says: >> — Vertical flipping in case of output to memory. >> So thats some sort of a conditional : | VSP1_HAS_WPF_VFLIP >> >> So looking at this and the settings of the existing models, I guess it looks >> like we don't support flip if we have an LIF output (as that would then be >> unsupported) > > On Gen3 vertical flipping seems to always be supported, unlike on Gen2 where > VSPD is specifically documented as not supporting vertical flipping. We could > add the WFLIP on all VSP2-D* instances. This would create a corresponding > control, which wouldn't do much harm as the VSPD instances on Gen3 are not > exposed to userspace, but that would waste a bit of memory for no good > purpose > (beside correctness I suppose). I wonder if that's worth it, what do you > think > ? If so, VSP2-D should be fixed too, so I'd prefer doing that in a separate > patch. I thi
Re: Linux: Smooth splashscreen with system having weston with drm-backend
On Thu, Jul 13, 2017 at 7:20 PM, Daniel Vetter wrote: > On Thu, Jul 13, 2017 at 3:33 PM, Vikas Patil wrote: >> Dear All, >> >> I am looking for an solution to have early smooth splashscreen on the >> Linux system with Weston and drm-backend. >> >> I tried showing splashscreen using Linux logo and fbcon Linux features >> but it is not smooth as when system boots logo gets displayed via >> kernel and as the Weston starts >> I see flicker and blackscreen. >> >> Another approach I tried is having standalone drm api based >> application [1] which uses the one of the available hardware >> plane/overlay for displaying splash image and Weston is >> starting and uses default plane which is different from plane utilized >> by above application. but still I see flicker and black screen when >> Weston starts. >> >> I want splash to be displayed on one of the h/w plane and Weston >> should start in background without any flicker and black screen on >> some other plane. >> >> Anyone here before achieved such a use case with such system? Looking >> for inputs/suggestions/ideas to achieve this. It is ok if something >> needs to be hacked/changd at any level (kernel driver, weston). >> >> This is required on platform such as TI (Jacinto) and Intel (Broxton) >> or devices based on drm graphics stack. >> >> [1] >> http://git.ti.com/glsdk/example-applications/blobs/39080525baca7bf136f2412d62436366a736af6b/drm-tests/drm_z_alpha.c >> >> Thanking you all for your time and inputs in advance. > > If you make sure you have matching modes between the 2 users of drm > this should work. kms drivers (especially if they are already > converted to atomic) will automatically optimize transitions to not > flicker (if possible). > I am not sure how this will work. As per what I understood is when drmModeSetCrtc() is called it uses the primary/default plane and if application wants to use any other plane then it need to setup the planes using drmModeSetPlane() in application before doing drmModeSetCrtc() so when primary plane scan-outs at crtc phase it also knows it has to compose from using other planes too. This is what splash application is doing. But when I run weston after splash app, weston I think calls drmModeSetCrtc() without the knowledge of planes and previous configuration overrides so it doesn't work. > If you still flicker then it's either a kernel driver issue, a hw > limitations (some hw needs a full modeset for e.g. changing the bit > depth) or you program two different things. > > Also, you must ensure that the boot-splash does not quit until weston > has fully taken over, otherwise kms will first shut down the screen > (when the splash quits), then re-enable it when weston starts. > I have tried adding while with sleep in splash app but still the behavior is same. FYI, I am doing this on TI's Jacinto6 platform but similar required on intel platform too. I was also thinking if there is something can be done in drm-backend of weston. e.g disabling mode setting or some code but not sure if weston will work with it.. Thanks & regards, Vikash ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC 1/3] clk: inherit display clocks enabled by bootloader
On Fri, Jul 14, 2017 at 12:52 AM, Rajendra Nayak wrote: > Hi Rob, > > On 07/11/2017 11:50 PM, Rob Clark wrote: >> The goal here is to support inheriting a display setup by bootloader, >> although there may also be some non-display related use-cases. >> >> Rough idea is to add a flag for clks and power domains that might >> already be enabled when kernel starts, and make corresponding fixups >> to clk enable/prepare_count and power-domain state so that these are >> not automatically disabled late in boot. >> >> If bootloader is enabling display, and kernel is using efifb before >> real display driver is loaded (potentially from kernel module after >> userspace starts, in a typical distro kernel), we don't want to kill >> the clocks and power domains that are used by the display before >> userspace starts. >> >> Second part is for drm/msm to check if display related clocks are >> enabled when it is loaded, and if so read back hw state to sync >> existing display state w/ software state, and skip the initial >> clk_enable's and otherwise fixing up clk/regulator/etc ref counts >> (skipping the normal display-enable codepaths), therefore inheriting >> the enable done by bootloader. >> >> Obviously this should be split up into multiple patches and many >> TODOs addressed. But I guess this is enough for now to start >> discussing the approach, and in particular how drm and clock/pd >> drivers work together to handle handover from bootloader. >> >> The CLK_INHERIT_BOOTLOADER and related gsdc flag should only be set >> on leaf nodes. >> --- > > [].. > >> diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c >> index d523991c945f..90b698c910d0 100644 >> --- a/drivers/clk/qcom/common.c >> +++ b/drivers/clk/qcom/common.c >> @@ -11,6 +11,7 @@ >> * GNU General Public License for more details. >> */ >> >> +#include >> #include >> #include >> #include >> @@ -258,6 +259,33 @@ int qcom_cc_really_probe(struct platform_device *pdev, >> if (ret) >> return ret; >> >> + /* Check which of clocks that we inherit state from bootloader >> + * are enabled, and fixup enable/prepare state (as well as that >> + * of it's parents). >> + * >> + * TODO can we assume that parents coming from another clk >> + * driver are already registered? >> + */ >> + for (i = 0; i < num_clks; i++) { >> + struct clk_hw *hw; >> + >> + if (!rclks[i]) >> + continue; >> + >> + hw = &rclks[i]->hw; >> + >> + if (!(hw->init->flags & CLK_INHERIT_BOOTLOADER)) >> + continue; >> + >> + if (!clk_is_enabled_regmap(hw)) >> + continue; >> + >> + dev_dbg(dev, "%s is enabled from bootloader!\n", >> + hw->init->name); >> + >> + clk_inherit_enabled(hw->clk); > > how about just calling a clk_prepare_enable(hw->clk) instead of adding a new > API? > The flag could also be something qcom specific and then we avoid having to add > anything in generic CCF code and its all handled in the qcom clock drivers. Hmm, I could try that.. I *guess* it doesn't hurt to enable an already enabled clk.. beyond that, I wonder if this is something that other platforms would want, so maybe I should be doing the check for enabled in CCF core? If not, making this a qcom specific flag makes sense. >> + } >> + >> reset = &cc->reset; >> reset->rcdev.of_node = dev->of_node; >> reset->rcdev.ops = &qcom_reset_ops; > > [] .. > >> diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c >> index a4f3580587b7..440d819b2d9d 100644 >> --- a/drivers/clk/qcom/gdsc.c >> +++ b/drivers/clk/qcom/gdsc.c >> @@ -291,6 +291,12 @@ static int gdsc_init(struct gdsc *sc) >> if ((sc->flags & VOTABLE) && on) >> gdsc_enable(&sc->pd); >> >> + if ((sc->flags & INHERIT_BL) && on) { >> + pr_debug("gdsc: %s is enabled from bootloader!\n", >> sc->pd.name); >> + gdsc_enable(&sc->pd); >> + sc->pd.flags |= GENPD_FLAG_ALWAYS_ON; > > Would this not prevent the powerdomain from ever getting disabled? Yeah, this is a hack, because I couldn't figure out something better. The problem is that gdsc/powerdomain doesn't have it's own enable_count but this is tracked at the device level (afaict.. maybe I'm miss-understanding something). I guess we could add our own enable_count within gdsc? Suggestions welcome, since I don't know these parts of the code so well. BR, -R > regards, > Rajendra > >> + } >> + >> if (on || (sc->pwrsts & PWRSTS_RET)) >> gdsc_force_mem_on(sc); >> else >> diff --git a/drivers/clk/qcom/gdsc.h b/drivers/clk/qcom/gdsc.h >> index 39648348e5ec..3b5e64b060c2 100644 >> --- a/drivers/clk/qcom/gdsc.h >> +++ b/drivers/clk/qcom/gdsc.h >> @@ -53,6 +53,7 @@ struct gdsc { >> #define VOTABLE BIT(0) >> #define CLAMP_IO BIT(1) >> #define HW_CTRL
Re: [Intel-gfx] [RFC v1 01/20] drm/hdcp: HDCP bitmask property for connectors
On Friday 14 July 2017 12:32 AM, Daniel Vetter wrote: On Thu, Jul 13, 2017 at 06:29:33PM +0530, Ramalingam C wrote: On Thursday 13 July 2017 04:06 PM, Daniel Vetter wrote: On Thu, Jul 13, 2017 at 12:15 PM, Ramalingam C wrote: On Thursday 13 July 2017 02:15 PM, Daniel Vetter wrote: On Thu, Jul 13, 2017 at 8:54 AM, Ramalingam C wrote: On Thursday 13 July 2017 11:39 AM, Daniel Vetter wrote: On Wed, Jul 12, 2017 at 9:10 PM, Sean Paul wrote: Why all these intermediate steps and different failure modes? Either hdcp works, or it doesnt (and we can split up with the type 0 or type 1 if needed), but I don't know what userspace would do with all the other stuff? enum values HDCP_ENABLE, HDCP_ENABLE_TYPE1 and HDCP_DISABLE along with kobj-uevent for HDCP state change, could do the bare minimal HDCP1.4 and HDCP2.2 configuration. And without Type info it is not possible for HDCP2.2. I've had requests from chrome team to expose HDCP version, so I don't think this is too contentious. I think it'd still be easier if we start out with the current content protection props that CrOS is using, and then figure out how to layer the exact version/standard on top? One thing at a time and all that. -Daniel I understand the approach. But Only problem is current upstreaming effort is for HDCP2.2 support at DRM with a design which can easily accommodate other versions too. So we need to stretch current CrOS property a bit with ENABLE_TYPE1 and UNSUPPORTED etc. Hope that should be fine for all. Yeah, but if we just go with enable (without specifying the type) we could still enable the highest hdcp level (so 2.2 for our case). At least I don't see a reason why we need to already have the enable_type1 thing. Can you pls explain why you think this is necessary? There seems to be a need to force type1, but I think it's easier to do that as an extension. Of course we need to keep it in mind meanwhile. Background for this need of Type info in HDCP2.2 implementation is as follows: Aside: You're quoting is broken for inline quoting. Either fix the quoting or top-quote (there's no difference between your text and mine, mine should be indented with > or | or similar). Sorry for the inconvenience. Hope now it is fine. Just reset the settings on thunderbird Yes, looks good now. HDCP2.2 Spec classify the protected content as Type 0 and Type 1. For Example lets say - A HDCP2.2 Src is connected to HDCP repeater - that repeater is connected to a HDCP2.2 panel - that same repeater is also connected to a HDCP1.4 panel. In this topology, as part of Repeater authentication: - HDCP2.2 Source will mention the Content Type to HDCP2.2 Repeater - Repeater can transmit this Type 1 content to HDCP2.2 compliant sink only (which is HDCP 2.2 panel here). - Repeater can transmit any type0 content to any other devices (like HDCP1.4 panel here). - Device with no HDCP support will get Neither of Type 0 or Type 1. So if we implement HDCP2.2 with HDCP_ENABLE state alone there is no way for Userspace to request for HDCP2.2 protection only. In this case we wont know the content type classification. Yes, that is the case, but also the point of gradual enabling. Atm (with the current CrOS usersapce) userspace can ask for "pls give me content protection, I don't care what level/type". That itself is already useful, and a good step forward. Allowing to ask for a specific type is something on top. Ok. When i think over it, that sounds as a good idea to go gradually for enabling HDCP2.2. Even if we force Content type to Type1, in above topology Type 0 content that could be rendered to HDCP1.4 compliant panel wont be rendered as that has been forcibly classified as Type 1 by KMD. Why? HDCP_ENABLE would just give you the "best" HDCP, so we'd fall back to type 0 (if that's available). I think i misinterpreted. We could enable the HDCP2.2(if supported on panel) for the Type 0 content. No issue on that Forcing type 1 content to Type 0 will break the association of type1 content to HDCP2.2 devices only. I didn't propose to force type1 everywhere. Why do you think this is needed. More than that Devices with our indented DRM HDCP2.2 support wont pass the HDCP2.2 compliance. Considering we could extend the CrOS Userspace for HDCP2.2, I would prefer to go ahead with HDCP_ENABLE_TYPE1 along with HDCP_ENABLE. Yes, it's only hdcp1.4, and getting to full hdcp2.2 will take more work. You can do all of that in one go, but my experience with upstreaming new uabi is that usually that's not the most effective way to go about things. But in the end, that's your choice. Agreed. We will go gradually about enabling HDCP2.2. 1. Enable HDCP2.2 for HDCP_ENABLE with Content type as 0. 2. Enable HDCP2.2 for Type 1 content with Enum value HDCP_ENABLE_TYPE1 3. Making HDCP2.2 support as HDCP2.2 spec compliant. But I think i will just add another enum value HDCP_UNSUPPORTED to mark the no HDCP supported on the setup. I hope that is fine. Makes sense.
Re: [PATCH 05/14] isdn: isdnloop: suppress a gcc-7 warning
On Fri, Jul 14, 2017 at 12:08 PM, Joe Perches wrote: > On Fri, 2017-07-14 at 11:25 +0200, Arnd Bergmann wrote: >> We test whether a bit is set in a mask here, which is correct >> but gcc warns about it as it thinks it might be confusing: >> >> drivers/isdn/isdnloop/isdnloop.c:412:37: error: ?: using integer constants >> in boolean context, the expression will always evaluate to 'true' >> [-Werror=int-in-bool-context] >> >> This replaces the negation of an integer with an equivalent >> comparison to zero, which gets rid of the warning. > [] >> diff --git a/drivers/isdn/isdnloop/isdnloop.c >> b/drivers/isdn/isdnloop/isdnloop.c > [] >> @@ -409,7 +409,7 @@ isdnloop_sendbuf(int channel, struct sk_buff *skb, >> isdnloop_card *card) >> return -EINVAL; >> } >> if (len) { >> - if (!(card->flags & (channel) ? ISDNLOOP_FLAGS_B2ACTIVE : >> ISDNLOOP_FLAGS_B1ACTIVE)) >> + if ((card->flags & (channel) ? ISDNLOOP_FLAGS_B2ACTIVE : >> ISDNLOOP_FLAGS_B1ACTIVE) == 0) >> return 0; >> if (card->sndcount[channel] > ISDNLOOP_MAX_SQUEUE) >> return 0; > > The if as written can not be zero. > > drivers/isdn/isdnloop/isdnloop.h:#define ISDNLOOP_FLAGS_B1ACTIVE 1 /* > B-Channel-1 is open */ > drivers/isdn/isdnloop/isdnloop.h:#define ISDNLOOP_FLAGS_B2ACTIVE 2 /* > B-Channel-2 is open */ > > Perhaps this is a logic defect and should be: > > if (!(card->flags & ((channel) ? ISDNLOOP_FLAGS_B2ACTIVE : > ISDNLOOP_FLAGS_B1ACTIVE))) Yes, good catch. I had thought about it for a bit whether that would be the answer, but come to the wrong conclusion on my own. Note that the version you suggested will still have the warning, so I think it needs to be if (card->flags & ((channel) ? ISDNLOOP_FLAGS_B2ACTIVE : ISDNLOOP_FLAGS_B1ACTIVE) == 0) or something like that, probably having a temporary flag variable would be best: int flag = channel ? ISDNLOOP_FLAGS_B2ACTIVE : ISDNLOOP_FLAGS_B1ACTIVE; if ((card->flags & flag) == 0) return 0; Arnd ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v8 06/14] drm/edid: parse YCBCR420 videomodes from EDID
HDMI 2.0 spec adds support for YCBCR420 sub-sampled output. CEA-861-F adds two new blocks in EDID's CEA extension blocks, to provide information about sink's YCBCR420 output capabilities. These blocks are: - YCBCR420vdb(YCBCR 420 video data block): This block contains VICs of video modes, which can be sopported only in YCBCR420 output mode (Not in RGB/YCBCR444/422. Its like a normal SVD block, valid for YCBCR420 modes only. - YCBCR420cmdb(YCBCR 420 capability map data block): This block gives information about video modes which can support YCBCR420 output mode also (along with RGB,YCBCR444/422 etc) This block contains a bitmap index of normal svd videomodes, which can support YCBCR420 output too. So if bit 0 from first vcb byte is set, first video mode in the svd list can support YCBCR420 output too. Bit 1 means second video mode from svd list can support YCBCR420 output too, and so on. This patch adds two bitmaps in display's hdmi_info structure, one each for VCB and VDB modes. If the source is HDMI 2.0 capable, this patch adds: - VDB modes (YCBCR 420 only modes) in connector's mode list, also makes an entry in the vdb_bitmap per vic. - VCB modes (YCBCR 420 also modes) only entry in the vcb_bitmap. Cc: Ville Syrjala Cc: Jose Abreu Cc: Emil Velikov V2: Addressed Review comments from Emil: - Use 1ULL< 64 modes in capability map block. - Use y420cmdb in function names and macros while dealing with vcb to be aligned with spec. - Move the display information parsing block ahead of mode parsing blocks. V3: Addressed design/review comments from Ville - Do not add flags in video modes, else we have to expose them to user - There should not be a UABI change, and kernel should detect the choice of the output based on type of mode, and the bitmaps. - Use standard bitops from kernel bitmap header, instead of calculating bit positions manually. V4: Addressed review comments from Ville: - s/ycbcr_420_vdb/y420vdb - s/ycbcr_420_vcb/y420cmdb - Be less verbose on description of do_y420vdb_modes - Move newmode variable in the loop scope. - Use svd_to_vic() to get a VIC, instead of 0x7f - Remove bitmap description for CMDB modes & VDB modes - Dont add connector->ycbcr_420_allowed check for cmdb modes - Remove 'len' variable, in is_y420cmdb function, which is used only once - Add length check in is_y420vdb function - Remove unnecessary if (!db) check in function parse_y420cmdb_bitmap - Do not add print about YCBCR 420 modes - Fix indentation in few places - Move ycbcr420_dc_modes in next patch, where its used - Add a separate patch for movement of drm_add_display_info() V5: Addressed review comments from Ville: - Add the patch which cleans up the current EXTENDED_TAG usage - Make y420_cmdb_map u64 - Do not block ycbcr420 modes while parsing the EDID, rather add a separate helper function to prune ycbcr420-only modes from connector's probed modes. V6: Rebase V7: Move this patch after the 420_only validation patch (Ville) V8: Addressed review comments from Ville - use cea_vic_valid check before adding cmdb/vdb modes - add check for i < 64 while adding cmdb modes - use 1ULL while checking bitmap Signed-off-by: Shashank Sharma --- drivers/gpu/drm/drm_edid.c | 154 +++- include/drm/drm_connector.h | 12 2 files changed, 164 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 96eee5a..9fa0d7f 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -2783,6 +2783,8 @@ add_detailed_modes(struct drm_connector *connector, struct edid *edid, #define SPEAKER_BLOCK 0x04 #define USE_EXTENDED_TAG 0x07 #define EXT_VIDEO_CAPABILITY_BLOCK 0x00 +#define EXT_VIDEO_DATA_BLOCK_420 0x0E +#define EXT_VIDEO_CAP_BLOCK_Y420CMDB 0x0F #define EDID_BASIC_AUDIO (1 << 6) #define EDID_CEA_YCRCB444 (1 << 5) #define EDID_CEA_YCRCB422 (1 << 4) @@ -3155,15 +3157,85 @@ drm_display_mode_from_vic_index(struct drm_connector *connector, return newmode; } +/* + * do_y420vdb_modes - Parse YCBCR 420 only modes + * @connector: connector corresponding to the HDMI sink + * @svds: start of the data block of CEA YCBCR 420 VDB + * @len: length of the CEA YCBCR 420 VDB + * + * Parse the CEA-861-F YCBCR 420 Video Data Block (Y420VDB) + * which contains modes which can be supported in YCBCR 420 + * output format only. + */ +static int do_y420vdb_modes(struct drm_connector *connector, + const u8 *svds, u8 svds_len) +{ + int modes = 0, i; + struct drm_device *dev = connector->dev; + struct drm_display_info *info = &connector->display_info; + struct drm_hdmi_info *hdmi = &info->hdmi; + + for (i = 0; i < svds_len; i++) { + u8 vic = svd_to_vic(svds[i]); + struct drm_disp
Re: [PATCH 00/14] gcc-7 warnings
On Fri, Jul 14, 2017 at 11:25:12AM +0200, Arnd Bergmann wrote: > This series should shut up all warnings introduced by gcc-6 or gcc-7 on > today's linux-next, as observed in "allmodconfig" builds on x86, > arm and arm64. > > I have sent some of these before, but some others are new, as I had > at some point disabled the -Wint-in-bool-context warning in my > randconfig testing and did not notice the other warnings. > > I have another series to address all -Wformat-overflow warnings, > and one more patch to turn off the -Wformat-truncation warnings > unless we build with "make W=1". I'll send that separately. > > Most of these are consist of trivial refactoring of the code to > shut up false-positive warnings, the one exception being > "staging:iio:resolver:ad2s1210 fix negative IIO_ANGL_VEL read", > which fixes a regression against linux-3.1 that has gone > unnoticed since then. Still, review from subsystem maintainers > would be appreciated. > > I would suggest that Andrew Morton can pick these up into linux-mm > so we can make sure they all make it into the release. Alternatively > Linus might feel like picking them all up himself. > > While I did not mark the harmless ones for stable backports, > Greg may also want to pick them up once they go upstream, to > help build-test the stable kernels with gcc-7. Thanks for these, I'll keep an eye out for them to get into the stable trees, so I can eventually update my test-build box to gcc-7. thanks, greg k-h ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 13/14] iopoll: avoid -Wint-in-bool-context warning
On Fri, Jul 14, 2017 at 11:55 AM, Joe Perches wrote: > On Fri, 2017-07-14 at 11:31 +0200, Arnd Bergmann wrote: >> When we pass the result of a multiplication as the timeout, we >> can get a warning: >> >> drivers/mmc/host/bcm2835.c:596:149: error: '*' in boolean context, suggest >> '&&' instead [-Werror=int-in-bool-context] >> drivers/mfd/arizona-core.c:247:195: error: '*' in boolean context, suggest >> '&&' instead [-Werror=int-in-bool-context] >> >> This is easy to avoid by comparing the timeout to zero instead, >> making it a boolean expression. > > Perhaps this is better as != 0 if the multiply is signed. I thought about that, but decided that as a negative timeout_us already gives us rather random behavior (ktime_add_us() takes an unsigned argument), the '>' comparison gives a more well-defined result by ignoring the timeout. Arnd ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 12/18] drm/sun4i: Add Allwinner A31 MIPI-DSI controller support
Hi Maxime, [auto build test ERROR on next-20170710] [cannot apply to mripard/sunxi/for-next robh/for-next regmap/for-next v4.12 v4.12-rc7 v4.12-rc6 v4.12] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Maxime-Ripard/drm-sun4i-Allwinner-MIPI-DSI-support/20170714-123103 config: arm-sunxi_defconfig (attached as .config) compiler: arm-linux-gnueabi-gcc (Debian 6.1.1-9) 6.1.1 20160705 reproduce: wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree make.cross ARCH=arm All errors (new ones prefixed by >>): drivers/gpu/drm/sun4i/sun6i_mipi_dphy.c: In function 'sun6i_dphy_init': >> drivers/gpu/drm/sun4i/sun6i_mipi_dphy.c:92:2: error: implicit declaration of >> function 'clk_set_rate_protect' [-Werror=implicit-function-declaration] clk_set_rate_protect(dphy->mod_clk, 15000); ^~~~ drivers/gpu/drm/sun4i/sun6i_mipi_dphy.c: In function 'sun6i_dphy_exit': >> drivers/gpu/drm/sun4i/sun6i_mipi_dphy.c:199:2: error: implicit declaration >> of function 'clk_rate_unprotect' [-Werror=implicit-function-declaration] clk_rate_unprotect(dphy->mod_clk); ^~ cc1: some warnings being treated as errors -- drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c: In function 'sun6i_dsi_probe': >> drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c:984:2: error: implicit declaration of >> function 'clk_set_rate_protect' [-Werror=implicit-function-declaration] clk_set_rate_protect(dsi->mod_clk, 29700); ^~~~ >> drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c:1016:2: error: implicit declaration >> of function 'clk_rate_unprotect' [-Werror=implicit-function-declaration] clk_rate_unprotect(dsi->mod_clk); ^~ cc1: some warnings being treated as errors vim +/clk_set_rate_protect +92 drivers/gpu/drm/sun4i/sun6i_mipi_dphy.c 87 88 int sun6i_dphy_init(struct sun6i_dphy *dphy, unsigned int lanes) 89 { 90 reset_control_deassert(dphy->reset); 91 clk_prepare_enable(dphy->mod_clk); > 92 clk_set_rate_protect(dphy->mod_clk, 15000); 93 94 regmap_write(dphy->regs, SUN6I_DPHY_TX_CTL_REG, 95 SUN6I_DPHY_TX_CTL_HS_TX_CLK_CONT); 96 97 regmap_write(dphy->regs, SUN6I_DPHY_TX_TIME0_REG, 98 SUN6I_DPHY_TX_TIME0_LP_CLK_DIV(14) | 99 SUN6I_DPHY_TX_TIME0_HS_PREPARE(6) | 100 SUN6I_DPHY_TX_TIME0_HS_TRAIL(10)); 101 102 regmap_write(dphy->regs, SUN6I_DPHY_TX_TIME1_REG, 103 SUN6I_DPHY_TX_TIME1_CLK_PREPARE(7) | 104 SUN6I_DPHY_TX_TIME1_CLK_ZERO(50) | 105 SUN6I_DPHY_TX_TIME1_CLK_PRE(3) | 106 SUN6I_DPHY_TX_TIME1_CLK_POST(10)); 107 108 regmap_write(dphy->regs, SUN6I_DPHY_TX_TIME2_REG, 109 SUN6I_DPHY_TX_TIME2_CLK_TRAIL(30)); 110 111 regmap_write(dphy->regs, SUN6I_DPHY_TX_TIME3_REG, 0); 112 113 regmap_write(dphy->regs, SUN6I_DPHY_TX_TIME4_REG, 114 SUN6I_DPHY_TX_TIME4_HS_TX_ANA0(3) | 115 SUN6I_DPHY_TX_TIME4_HS_TX_ANA1(3)); 116 117 /* FIXME: Number of lanes? */ 118 regmap_write(dphy->regs, SUN6I_DPHY_GCTL_REG, 119 SUN6I_DPHY_GCTL_LANE_NUM(lanes) | 120 SUN6I_DPHY_GCTL_EN); 121 122 return 0; 123 } 124 125 int sun6i_dphy_power_on(struct sun6i_dphy *dphy, unsigned int lanes) 126 { 127 u8 lanes_mask = GENMASK(lanes - 1, 0); 128 129 regmap_write(dphy->regs, SUN6I_DPHY_ANA0_REG, 130 SUN6I_DPHY_ANA0_REG_PWS | 131 SUN6I_DPHY_ANA0_REG_DMPC | 132 SUN6I_DPHY_ANA0_REG_SLV(7) | 133 SUN6I_DPHY_ANA0_REG_DMPD(lanes_mask) | 134 SUN6I_DPHY_ANA0_REG_DEN(lanes_mask)); 135 136 regmap_write(dphy->regs, SUN6I_DPHY_ANA1_REG, 137 SUN6I_DPHY_ANA1_REG_CSMPS(1) | 138 SUN6I_DPHY_ANA1_REG_SVTT(7)); 139 140 regmap_write(dphy->regs, SUN6I_DPHY_ANA4_REG, 141 SUN6I_DPHY_ANA4_REG_CKDV(1) | 142 SUN6I_DPHY_ANA4_REG_TMSC(1) | 143 SUN6I_DPHY_ANA4_REG_TMSD(1) | 144 SUN6I_DPHY_ANA4_REG_TXDNSC(1) | 145
Re: [PATCH 05/14] isdn: isdnloop: suppress a gcc-7 warning
On Fri, 2017-07-14 at 11:25 +0200, Arnd Bergmann wrote: > We test whether a bit is set in a mask here, which is correct > but gcc warns about it as it thinks it might be confusing: > > drivers/isdn/isdnloop/isdnloop.c:412:37: error: ?: using integer constants in > boolean context, the expression will always evaluate to 'true' > [-Werror=int-in-bool-context] > > This replaces the negation of an integer with an equivalent > comparison to zero, which gets rid of the warning. [] > diff --git a/drivers/isdn/isdnloop/isdnloop.c > b/drivers/isdn/isdnloop/isdnloop.c [] > @@ -409,7 +409,7 @@ isdnloop_sendbuf(int channel, struct sk_buff *skb, > isdnloop_card *card) > return -EINVAL; > } > if (len) { > - if (!(card->flags & (channel) ? ISDNLOOP_FLAGS_B2ACTIVE : > ISDNLOOP_FLAGS_B1ACTIVE)) > + if ((card->flags & (channel) ? ISDNLOOP_FLAGS_B2ACTIVE : > ISDNLOOP_FLAGS_B1ACTIVE) == 0) > return 0; > if (card->sndcount[channel] > ISDNLOOP_MAX_SQUEUE) > return 0; The if as written can not be zero. drivers/isdn/isdnloop/isdnloop.h:#define ISDNLOOP_FLAGS_B1ACTIVE 1 /* B-Channel-1 is open */ drivers/isdn/isdnloop/isdnloop.h:#define ISDNLOOP_FLAGS_B2ACTIVE 2 /* B-Channel-2 is open */ Perhaps this is a logic defect and should be: if (!(card->flags & ((channel) ? ISDNLOOP_FLAGS_B2ACTIVE : ISDNLOOP_FLAGS_B1ACTIVE))) ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/2] drm/amdgpu: Implement ttm_bo_driver.access_vram callback
Am 13.07.2017 um 23:08 schrieb Felix Kuehling: Allows gdb to access contents of user mode mapped VRAM BOs. Signed-off-by: Felix Kuehling --- drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 59 + drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h | 2 ++ 2 files changed, 61 insertions(+) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c index ff5614b..d65551d 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c @@ -1115,6 +1115,64 @@ static bool amdgpu_ttm_bo_eviction_valuable(struct ttm_buffer_object *bo, return ttm_bo_eviction_valuable(bo, place); } +static int amdgpu_ttm_access_vram(struct ttm_buffer_object *bo, + unsigned long offset, + void *buf, int len, int write) +{ + struct amdgpu_bo *abo = container_of(bo, struct amdgpu_bo, tbo); + struct amdgpu_device *adev = amdgpu_ttm_adev(abo->tbo.bdev); + struct drm_mm_node *nodes = abo->tbo.mem.mm_node; + uint32_t value = 0; + int result = 0; + uint64_t pos; + unsigned long flags; + + while (offset >= (nodes->size << PAGE_SHIFT)) { + offset -= nodes->size << PAGE_SHIFT; + ++nodes; + } + pos = (nodes->start << PAGE_SHIFT) + offset; This silently assumes that a read would never cross a node boundary, doesn't it? Christian. + + while (len && pos < adev->mc.mc_vram_size) { + uint64_t aligned_pos = pos & ~(uint64_t)3; + uint32_t bytes = 4 - (pos & 3); + uint32_t shift = (pos & 3) * 8; + uint32_t mask = 0x << shift; + + if (len < bytes) { + mask &= 0x >> (bytes - len) * 8; + bytes = len; + } + + spin_lock_irqsave(&adev->mmio_idx_lock, flags); + WREG32(mmMM_INDEX, ((uint32_t)aligned_pos) | 0x8000); + WREG32(mmMM_INDEX_HI, aligned_pos >> 31); + if (!write || mask != 0x) + value = RREG32(mmMM_DATA); + if (write) { + value &= ~mask; + value |= (*(uint32_t *)buf << shift) & mask; + WREG32(mmMM_DATA, value); + } + spin_unlock_irqrestore(&adev->mmio_idx_lock, flags); + if (!write) { + value = (value & mask) >> shift; + memcpy(buf, &value, bytes); + } + + result += bytes; + buf = (uint8_t *)buf + bytes; + pos += bytes; + len -= bytes; + if (pos >= (nodes->start + nodes->size) << PAGE_SHIFT) { + ++nodes; + pos = (nodes->start << PAGE_SHIFT); + } + } + + return result; +} + static struct ttm_bo_driver amdgpu_bo_driver = { .ttm_tt_create = &amdgpu_ttm_tt_create, .ttm_tt_populate = &amdgpu_ttm_tt_populate, @@ -1130,6 +1188,7 @@ static bool amdgpu_ttm_bo_eviction_valuable(struct ttm_buffer_object *bo, .io_mem_reserve = &amdgpu_ttm_io_mem_reserve, .io_mem_free = &amdgpu_ttm_io_mem_free, .io_mem_pfn = amdgpu_ttm_io_mem_pfn, + .access_vram = &amdgpu_ttm_access_vram }; int amdgpu_ttm_init(struct amdgpu_device *adev) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h index f137c24..a22e430 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h @@ -78,6 +78,8 @@ int amdgpu_fill_buffer(struct amdgpu_bo *bo, struct dma_fence **fence); int amdgpu_mmap(struct file *filp, struct vm_area_struct *vma); +int amdgpu_bo_mmap(struct file *filp, struct vm_area_struct *vma, + struct ttm_bo_device *bdev); bool amdgpu_ttm_is_bound(struct ttm_tt *ttm); int amdgpu_ttm_bind(struct ttm_buffer_object *bo, struct ttm_mem_reg *bo_mem); int amdgpu_ttm_recover_gart(struct amdgpu_device *adev); ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/2] drm: Implement vm_operations_struct.access
Am 13.07.2017 um 23:08 schrieb Felix Kuehling: Allows gdb to access contents of user mode mapped BOs. VRAM access requires the driver to implement a new callback in ttm_bo_driver. One more comment additionally to what Michel already wrote below, apart from that it looks good to me. Signed-off-by: Felix Kuehling --- drivers/gpu/drm/ttm/ttm_bo_vm.c | 66 - include/drm/ttm/ttm_bo_driver.h | 12 2 files changed, 77 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/ttm/ttm_bo_vm.c b/drivers/gpu/drm/ttm/ttm_bo_vm.c index 9f53df9..0ef2eb9 100644 --- a/drivers/gpu/drm/ttm/ttm_bo_vm.c +++ b/drivers/gpu/drm/ttm/ttm_bo_vm.c @@ -294,10 +294,74 @@ static void ttm_bo_vm_close(struct vm_area_struct *vma) vma->vm_private_data = NULL; } +static int ttm_bo_vm_access_kmap(struct ttm_buffer_object *bo, +unsigned long offset, +void *buf, int len, int write) +{ + unsigned long first_page = offset >> PAGE_SHIFT; + unsigned long last_page = (offset + len - 1) >> PAGE_SHIFT; + unsigned long num_pages = last_page - first_page + 1; + struct ttm_bo_kmap_obj map; + void *ptr; + bool is_iomem; + int ret; + + ret = ttm_bo_kmap(bo, first_page, num_pages, &map); + if (ret) + return ret; + + offset -= first_page << PAGE_SHIFT; + ptr = (uint8_t *)ttm_kmap_obj_virtual(&map, &is_iomem) + offset; + WARN_ON(is_iomem); + if (write) + memcpy(ptr, buf, len); + else + memcpy(buf, ptr, len); + ttm_bo_kunmap(&map); + + return len; +} + +static int ttm_bo_vm_access(struct vm_area_struct *vma, unsigned long addr, + void *buf, int len, int write) +{ + unsigned long offset = (addr) - vma->vm_start; + struct ttm_buffer_object *bo = vma->vm_private_data; + int ret; + + if (len < 1 || (offset + len) >> PAGE_SHIFT > bo->num_pages) + return -EIO; + + ret = ttm_bo_reserve(bo, true, false, NULL); + if (ret) + return ret; + + switch(bo->mem.mem_type) { + case TTM_PL_SYSTEM: When the BO is in the system domain you need to add this as well I think: if (unlikely(bo->ttm->page_flags & TTM_PAGE_FLAG_SWAPPED)) { ret = ttm_tt_swapin(bo->ttm); if (unlikely(ret != 0)) return ret; } Regards, Christian. + case TTM_PL_TT: + ret = ttm_bo_vm_access_kmap(bo, offset, buf, len, write); + break; + case TTM_PL_VRAM: + if (bo->bdev->driver->access_vram) + ret = bo->bdev->driver->access_vram( + bo, offset, buf, len, write); + else + ret = -EIO; + break; + default: + ret = -EIO; + } + + ttm_bo_unreserve(bo); + + return ret; +} + static const struct vm_operations_struct ttm_bo_vm_ops = { .fault = ttm_bo_vm_fault, .open = ttm_bo_vm_open, - .close = ttm_bo_vm_close + .close = ttm_bo_vm_close, + .access = ttm_bo_vm_access }; static struct ttm_buffer_object *ttm_bo_vm_lookup(struct ttm_bo_device *bdev, diff --git a/include/drm/ttm/ttm_bo_driver.h b/include/drm/ttm/ttm_bo_driver.h index 6bbd34d..6ce5094 100644 --- a/include/drm/ttm/ttm_bo_driver.h +++ b/include/drm/ttm/ttm_bo_driver.h @@ -471,6 +471,18 @@ struct ttm_bo_driver { */ unsigned long (*io_mem_pfn)(struct ttm_buffer_object *bo, unsigned long page_offset); + + /** +* Read/write VRAM buffers for ptrace access +* +* @bo: the BO to access +* @offset: the offset from the start of the BO +* @buf: pointer to source/destination buffer +* @len: number of bytes to copy +* @write: whether to read (0) from or write (non-0) to BO +*/ + int (*access_vram)(struct ttm_buffer_object *bo, unsigned long offset, + void *buf, int len, int write); }; /** ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH, RESEND 03/14] drm/vmwgfx: avoid gcc-7 parentheses warning
On Fri, 14 Jul 2017, Arnd Bergmann wrote: > gcc-7 warns about slightly suspicious code in vmw_cmd_invalid: > > drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c: In function 'vmw_cmd_invalid': > drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c:522:23: error: the omitted middle > operand in ?: will always be 'true', suggest explicit middle operand > [-Werror=parentheses] > > The problem is that it is mixing boolean and integer values here. > I assume that the code actually works correctly, so making it use > a literal '1' instead of the implied 'true' makes it more readable > and avoids the warning. > > The code has been in this file since the start, but it could > make sense to backport this patch to stable to make it build cleanly > with gcc-7. > > Fixes: fb1d9738ca05 ("drm/vmwgfx: Add DRM driver for VMware Virtual GPU") > Reviewed-by: Sinclair Yeh > Signed-off-by: Arnd Bergmann > --- > Originally submitted on Nov 16, but for some reason it never appeared > upstream. The patch is still needed as of v4.11-rc2 See also the thread starting at http://lkml.kernel.org/r/CA+55aFwZV-QirBTY8y4y+h796V2CzpWdL=twb27rj1u3roj...@mail.gmail.com BR, Jani. > --- > drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c > b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c > index c7b53d987f06..3f343e55972a 100644 > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c > @@ -519,7 +519,7 @@ static int vmw_cmd_invalid(struct vmw_private *dev_priv, > struct vmw_sw_context *sw_context, > SVGA3dCmdHeader *header) > { > - return capable(CAP_SYS_ADMIN) ? : -EINVAL; > + return capable(CAP_SYS_ADMIN) ? 1 : -EINVAL; > } > > static int vmw_cmd_ok(struct vmw_private *dev_priv, -- Jani Nikula, Intel Open Source Technology Center ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 13/14] iopoll: avoid -Wint-in-bool-context warning
On Fri, 2017-07-14 at 11:31 +0200, Arnd Bergmann wrote: > When we pass the result of a multiplication as the timeout, we > can get a warning: > > drivers/mmc/host/bcm2835.c:596:149: error: '*' in boolean context, suggest > '&&' instead [-Werror=int-in-bool-context] > drivers/mfd/arizona-core.c:247:195: error: '*' in boolean context, suggest > '&&' instead [-Werror=int-in-bool-context] > > This is easy to avoid by comparing the timeout to zero instead, > making it a boolean expression. Perhaps this is better as != 0 if the multiply is signed. > diff --git a/include/linux/iopoll.h b/include/linux/iopoll.h [] > @@ -48,7 +48,8 @@ > (val) = op(addr); \ > if (cond) \ > break; \ > - if (timeout_us && ktime_compare(ktime_get(), timeout) > 0) { \ > + if ((timeout_us) > 0 && \ > + ktime_compare(ktime_get(), timeout) > 0) { \ > (val) = op(addr); \ > break; \ > } \ etc... ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel