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

[Manasi Navare <manasi.d.navare@intel.com>]

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