| Message ID | 20181102064659.8991-1-manasi.d.navare@intel.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | DSC enabling remaining patches | expand |
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@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
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@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > > -- > Ville Syrjälä > Intel
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 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