|
Regards Shashank On 7/15/2017 12:32 AM, Ville Syrjälä
wrote:
The whole patch series was checked with checkpatch every time it was sent for review. It hadOn Thu, Jul 13, 2017 at 09:03:06PM +0530, Shashank Sharma wrote: 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.cApart 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 --------------------------------------------------------------- total: 0 errors, 0 warnings, 33 lines checked v8-0004-drm-edid-cleanup-patch-for-CEA-extended-tag-macro.patch has no obvious style problems and is ready for submission. ------------------------------------------------------- v8-0005-drm-add-helper-to-validate-YCBCR420-modes.patch ------------------------------------------------------- total: 0 errors, 0 warnings, 110 lines checked v8-0005-drm-add-helper-to-validate-YCBCR420-modes.patch has no obvious style problems and is ready for submission. ---------------------------------------------------------- v8-0006-drm-edid-parse-YCBCR420-videomodes-from-EDID.patch ---------------------------------------------------------- total: 0 errors, 0 warnings, 228 lines checked v8-0006-drm-edid-parse-YCBCR420-videomodes-from-EDID.patch has no obvious style problems and is ready for submission. ------------------------------------------------------------- v8-0007-drm-edid-parse-ycbcr-420-deep-color-information.patch ------------------------------------------------------------- total: 0 errors, 0 warnings, 47 lines checked v8-0007-drm-edid-parse-ycbcr-420-deep-color-information.patch has no obvious style problems and is ready for submission. ------------------------------------------------------------ v8-0008-drm-add-helper-functions-for-YCBCR420-handling.patch ------------------------------------------------------------ total: 0 errors, 0 warnings, 73 lines checked v8-0008-drm-add-helper-functions-for-YCBCR420-handling.patch has no obvious style problems and is ready for submission. --------------------------------------------------------------- v8-0009-drm-i915-add-config-function-for-YCBCR420-outputs.patch --------------------------------------------------------------- total: 0 errors, 0 warnings, 89 lines checked v8-0009-drm-i915-add-config-function-for-YCBCR420-outputs.patch has no obvious style problems and is ready for submission. ---------------------------------------------------------- v8-0010-drm-i915-prepare-scaler-for-YCBCR420-modeset.patch ---------------------------------------------------------- total: 0 errors, 0 warnings, 58 lines checked v8-0010-drm-i915-prepare-scaler-for-YCBCR420-modeset.patch has no obvious style problems and is ready for submission. ------------------------------------------------------- v8-0011-drm-i915-prepare-pipe-for-YCBCR420-output.patch ------------------------------------------------------- total: 0 errors, 0 warnings, 28 lines checked v8-0011-drm-i915-prepare-pipe-for-YCBCR420-output.patch has no obvious style problems and is ready for submission. ----------------------------------------------------------- v8-0012-drm-i915-prepare-csc-unit-for-YCBCR420-output.patch ----------------------------------------------------------- total: 0 errors, 0 warnings, 85 lines checked v8-0012-drm-i915-prepare-csc-unit-for-YCBCR420-output.patch has no obvious style problems and is ready for submission. ---------------------------------------------------------- v8-0013-drm-i915-set-colorspace-for-YCBCR420-outputs.patch ---------------------------------------------------------- total: 0 errors, 0 warnings, 18 lines checked v8-0013-drm-i915-set-colorspace-for-YCBCR420-outputs.patch has no obvious style problems and is ready for submission. -------------------------------------------------- v8-0014-drm-i915-glk-set-HDMI-2.0-identifier.patch -------------------------------------------------- total: 0 errors, 0 warnings, 9 lines checked v8-0014-drm-i915-glk-set-HDMI-2.0-identifier.patch has no obvious style problems and is ready for submission. Yes, I had seen that, and I knew why is that happening, but I needed a discussion on what is the right way to fix it.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. It was about readouts of the state and pixel clock mismatch (as YCBCR420 clock = half). But at the same time, I am at fault, I should have mentioned this somewhere in the cover letter or I should have mentioned it to you on IRC. Sorry about that, My bad :( ! Thanks, for the detailed review, and the time you invested on this.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/ As such I had tested this whole series with this combination: - GLK system & KBL NUC system - 2 different HDMI 2.0 monitors ACER/Samsung - 1 HDMI 1.4 monitor (Dell) - 1 HDMI analyzer (Astro VA1844) - Mint desktop GUI - IGT (testdisplay) and I dint see any greying issue even once, but May be I will try to test with the way you are testing. Thanks !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 identifierThese 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ä <ville.syrjala@xxxxxxxxxxxxxxx> onto the patches. - Shashank drivers/gpu/drm/amd/amdgpu/dce_v10_0.c | 2 +- drivers/gpu/drm/amd/amdgpu/dce_v11_0.c | 2 +- drivers/gpu/drm/amd/amdgpu/dce_v6_0.c | 2 +- drivers/gpu/drm/amd/amdgpu/dce_v8_0.c | 2 +- drivers/gpu/drm/bridge/analogix-anx78xx.c | 3 +- drivers/gpu/drm/bridge/sii902x.c | 2 +- drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 2 +- drivers/gpu/drm/drm_edid.c | 438 +++++++++++++++++++++++++++++- drivers/gpu/drm/drm_modes.c | 87 ++++++ drivers/gpu/drm/drm_probe_helper.c | 4 + drivers/gpu/drm/exynos/exynos_hdmi.c | 2 +- drivers/gpu/drm/i2c/tda998x_drv.c | 2 +- drivers/gpu/drm/i915/i915_reg.h | 3 + drivers/gpu/drm/i915/intel_color.c | 46 +++- drivers/gpu/drm/i915/intel_display.c | 26 ++ drivers/gpu/drm/i915/intel_drv.h | 7 +- drivers/gpu/drm/i915/intel_hdmi.c | 69 ++++- drivers/gpu/drm/i915/intel_panel.c | 3 +- drivers/gpu/drm/i915/intel_sdvo.c | 3 +- drivers/gpu/drm/mediatek/mtk_hdmi.c | 2 +- drivers/gpu/drm/msm/hdmi/hdmi_bridge.c | 2 +- drivers/gpu/drm/nouveau/nv50_display.c | 3 +- drivers/gpu/drm/omapdrm/omap_encoder.c | 3 +- drivers/gpu/drm/radeon/radeon_audio.c | 2 +- drivers/gpu/drm/rockchip/inno_hdmi.c | 2 +- drivers/gpu/drm/sti/sti_hdmi.c | 2 +- drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 2 +- drivers/gpu/drm/tegra/hdmi.c | 2 +- drivers/gpu/drm/tegra/sor.c | 2 +- drivers/gpu/drm/vc4/vc4_hdmi.c | 2 +- drivers/gpu/drm/zte/zx_hdmi.c | 2 +- include/drm/drm_connector.h | 32 +++ include/drm/drm_edid.h | 11 +- include/drm/drm_modes.h | 11 + 34 files changed, 746 insertions(+), 39 deletions(-) -- 2.7.4 _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel |
_______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
