Thanks Ville for this detailed review. I will spin this now with the required fixes and send it over. Manasi On Fri, Nov 02, 2018 at 08:06:04PM +0200, Ville Syrjälä wrote: > On Thu, Nov 01, 2018 at 11:46:40PM -0700, Manasi Navare wrote: > > This patch series addresses review comments on previous DSC series: > > > > https://patchwork.freedesktop.org/series/47514/ > > > > Gaurav K Singh (3): > > drm/i915/dsc: Define & Compute VESA DSC params > > drm/i915/dsc: Compute Rate Control parameters for DSC > > drm/i915/dp: Enable/Disable DSC in DP Sink > > > > Manasi Navare (15): > > drm/dsc: Define Display Stream Compression PPS infoframe > > drm/dsc: Define VESA Display Stream Compression Capabilities > > drm/dsc: Add helpers for DSC picture parameter set infoframes > > drm/i915/dp: Add DSC params and DSC config to intel_crtc_state > > drm/i915/dp: Compute DSC pipe config in atomic check > > drm/i915/dp: Do not enable PSR2 if DSC is enabled > > drm/dsc: Define the DSC 1.1 and 1.2 Line Buffer depth constants > > drm/i915/dsc: Add a power domain for VDSC on eDP/MIPI DSI > > drm/i915/dp: Configure i915 Picture parameter Set registers during DSC > > enabling > > drm/i915/dp: Use the existing write_infoframe() for DSC PPS SDPs > > drm/i915/dp: Populate DSC PPS SDP and send PPS infoframes > > drm/i915/dp: Configure Display stream splitter registers during DSC > > enable > > drm/i915/dp: Disable DSC in source by disabling DSS CTL bits > > drm/i915/dsc: Enable and disable appropriate power wells for VDSC > > drm/i915/dsc: Add Per connector debugfs node for DSC support/enable > > > > Srivatsa, Anusha (1): > > drm/dsc: Define Rate Control values that do not change over > > configurations > > I think we have these real functional issues left: > - no FEC so should reject DSC on external DP > - get_power_domains() thing wasn't right > > The potentially user triggerable DRM_ERROR()s have to be > removed or explained why they can't happen (in which case > a WARN() would probably be a more clear hint to the reader). > > The intel_dsc_enable() call I definitely would like see > moved into the encoder->per_enable(). No one will think to > look for it in the current location. > > The i915_modparams.enable_psr change seemed unrelated, but > no idea if it's intentional or not. > > And finally there were various style nits that are optional. > But I would recomment doing them since it's trivial stuff and > avoids further churn in the code later. > > I think that's about it really. > > > > > Documentation/gpu/drm-kms-helpers.rst | 12 + > > drivers/gpu/drm/Makefile | 2 +- > > drivers/gpu/drm/drm_dsc.c | 228 +++++ > > drivers/gpu/drm/i915/Makefile | 3 +- > > drivers/gpu/drm/i915/i915_debugfs.c | 71 +- > > drivers/gpu/drm/i915/i915_drv.h | 4 + > > drivers/gpu/drm/i915/i915_reg.h | 1 + > > drivers/gpu/drm/i915/intel_ddi.c | 8 +- > > drivers/gpu/drm/i915/intel_display.c | 28 +- > > drivers/gpu/drm/i915/intel_display.h | 4 +- > > drivers/gpu/drm/i915/intel_dp.c | 196 +++- > > drivers/gpu/drm/i915/intel_dp_mst.c | 2 +- > > drivers/gpu/drm/i915/intel_drv.h | 21 + > > drivers/gpu/drm/i915/intel_hdmi.c | 21 +- > > drivers/gpu/drm/i915/intel_psr.c | 16 +- > > drivers/gpu/drm/i915/intel_runtime_pm.c | 4 +- > > drivers/gpu/drm/i915/intel_vdsc.c | 1100 +++++++++++++++++++++++ > > include/drm/drm_dp_helper.h | 3 + > > include/drm/drm_dsc.h | 485 ++++++++++ > > 19 files changed, 2169 insertions(+), 40 deletions(-) > > create mode 100644 drivers/gpu/drm/drm_dsc.c > > create mode 100644 drivers/gpu/drm/i915/intel_vdsc.c > > create mode 100644 include/drm/drm_dsc.h > > > > -- > > 2.18.0 > > > > _______________________________________________ > > dri-devel mailing list > > dri-devel@xxxxxxxxxxxxxxxxxxxxx > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > > -- > Ville Syrjälä > Intel _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
