Re: [PATCH v7 00/19] DSC enabling remaining patches

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel