Re: [Intel-gfx] [PATCH 02/10] drm: i915: Get DSC capability from DP sink

[Dan Carpenter]

Hi Gaurav,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on next-20180223]
[also build test WARNING on v4.16-rc3]
[cannot apply to drm-intel/for-linux-next drm/drm-next v4.16-rc3 v4.16-rc2 
v4.16-rc1]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Gaurav-K-Singh/Enabling-VDSC-in-i915-driver-for-GLK/20180226-114246

smatch warnings:
drivers/gpu/drm/i915/intel_dp.c:6379 intel_edp_init_connector() error: we 
previously assumed 'fixed_mode' could be null (see line 6367)

# 
https://github.com/0day-ci/linux/commit/0be77ee7aeeb70345533485fec0f376c0180bafa
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout 0be77ee7aeeb70345533485fec0f376c0180bafa
vim +/fixed_mode +6379 drivers/gpu/drm/i915/intel_dp.c

0be77ee7ae Gaurav K Singh 2018-02-23  6285  
ed92f0b239 Paulo Zanoni   2013-06-12  6286  static bool 
intel_edp_init_connector(struct intel_dp *intel_dp,
36b5f425dd Ville Syrjälä  2014-10-16  6287   
struct intel_connector *intel_connector)
ed92f0b239 Paulo Zanoni   2013-06-12  6288  {
2f7734770c Ville Syrjälä  2017-11-09  6289  struct drm_device *dev = 
intel_dp_to_dev(intel_dp);
fac5e23e3c Chris Wilson   2016-07-04  6290  struct drm_i915_private 
*dev_priv = to_i915(dev);
2f7734770c Ville Syrjälä  2017-11-09  6291  struct drm_connector *connector 
= _connector->base;
ed92f0b239 Paulo Zanoni   2013-06-12  6292  struct drm_display_mode 
*fixed_mode = NULL;
dc911f5bd8 Jim Bride  2017-08-09  6293  struct drm_display_mode 
*alt_fixed_mode = NULL;
4f9db5b51c Pradeep Bhat   2014-04-05  6294  struct drm_display_mode 
*downclock_mode = NULL;
0be77ee7ae Gaurav K Singh 2018-02-23  6295  struct dp_sink_dsc_caps 
sink_dp_dsc_caps = {0};
ed92f0b239 Paulo Zanoni   2013-06-12  6296  bool has_dpcd;
ed92f0b239 Paulo Zanoni   2013-06-12  6297  struct drm_display_mode *scan;
ed92f0b239 Paulo Zanoni   2013-06-12  6298  struct edid *edid;
6517d2734d Ville Syrjälä  2014-11-07  6299  enum pipe pipe = INVALID_PIPE;
ed92f0b239 Paulo Zanoni   2013-06-12  6300  
1853a9daa1 Jani Nikula2017-08-18  6301  if (!intel_dp_is_edp(intel_dp))
ed92f0b239 Paulo Zanoni   2013-06-12  6302  return true;
ed92f0b239 Paulo Zanoni   2013-06-12  6303  
97a824e156 Imre Deak  2016-06-21  6304  /*
97a824e156 Imre Deak  2016-06-21  6305   * On IBX/CPT we may get here 
with LVDS already registered. Since the
97a824e156 Imre Deak  2016-06-21  6306   * driver uses the only 
internal power sequencer available for both
97a824e156 Imre Deak  2016-06-21  6307   * eDP and LVDS bail out early 
in this case to prevent interfering
97a824e156 Imre Deak  2016-06-21  6308   * with an already powered-on 
LVDS power sequencer.
97a824e156 Imre Deak  2016-06-21  6309   */
2f7734770c Ville Syrjälä  2017-11-09  6310  if 
(intel_get_lvds_encoder(_priv->drm)) {
97a824e156 Imre Deak  2016-06-21  6311  
WARN_ON(!(HAS_PCH_IBX(dev_priv) || HAS_PCH_CPT(dev_priv)));
97a824e156 Imre Deak  2016-06-21  6312  DRM_INFO("LVDS was 
detected, not registering eDP\n");
97a824e156 Imre Deak  2016-06-21  6313  
97a824e156 Imre Deak  2016-06-21  6314  return false;
97a824e156 Imre Deak  2016-06-21  6315  }
97a824e156 Imre Deak  2016-06-21  6316  
49e6bc51bc Ville Syrjälä  2014-10-28  6317  pps_lock(intel_dp);
b4d06ede4e Imre Deak  2016-06-21  6318  
b4d06ede4e Imre Deak  2016-06-21  6319  
intel_dp_init_panel_power_timestamps(intel_dp);
46bd8383d8 Ville Syrjälä  2017-10-31  6320  intel_dp_pps_init(intel_dp);
b4d06ede4e Imre Deak  2016-06-21  6321  
intel_edp_panel_vdd_sanitize(intel_dp);
b4d06ede4e Imre Deak  2016-06-21  6322  
49e6bc51bc Ville Syrjälä  2014-10-28  6323  pps_unlock(intel_dp);
636352173a Paulo Zanoni   2014-04-22  6324  
ed92f0b239 Paulo Zanoni   2013-06-12  6325  /* Cache DPCD and EDID for edp. 
*/
fe5a66f91c Ville Syrjälä  2016-07-29  6326  has_dpcd = 
intel_edp_init_dpcd(intel_dp);
ed92f0b239 Paulo Zanoni   2013-06-12  6327  
fe5a66f91c Ville Syrjälä  2016-07-29  6328  if (!has_dpcd) {
ed92f0b239 Paulo Zanoni   2013-06-12  6329  /* if this fails, 
presume the device is a ghost */
ed92f0b239 Paulo Zanoni   2013-06-12  6330  DRM_INFO("failed to 
retrieve link info, disabling eDP\n");
b4d06ede4e Imre Deak  2016-06-21  6331  goto out_vdd_off;
ed92f0b239 Paulo Zanoni   2013-06-12  6332  }
ed92f0b239 Paulo Zanoni   2013-06-12  6333  
0be77ee7ae Gaurav K Singh 2018-02-23  6334  /* Get DSC capability of DP 
sink */
0be77ee7ae Gaurav K Singh 2018-02-23  6335  if (INTEL_GEN(dev_priv) >= 9) {
0be77ee7ae Gaurav K Singh 2018-02-23  6336  
intel_dp_sink_get_dsc_capability(intel_dp, 

Re: [Intel-gfx] [PATCH 02/10] drm: i915: Get DSC capability from DP sink

[Manasi Navare]

On Fri, Feb 23, 2018 at 09:25:45PM +0530, Gaurav K Singh wrote:
> Get decompression capabilities from DP sink by doing
> DPCD reads of different offsets as per eDP/DP specs.
> 
> Signed-off-by: Gaurav K Singh 
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 167 
> 
>  1 file changed, 167 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 1868f73f730c..f494a851ff89 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -5883,6 +5883,149 @@ void intel_edp_drrs_flush(struct drm_i915_private 
> *dev_priv,
>   return downclock_mode;
>  }
>  
> +static void intel_dp_sink_get_dsc_capability(struct intel_dp *intel_dp,
> + struct dp_sink_dsc_caps *dp_dsc_caps)
> +{
> + u8 rcbuffer_blocksize;
> + u8 fec_dpcd;
> + unsigned long line_buffer_bit_depth, sink_support_max_bpp_msb;
> +

Clear the previously cached dsc_dpcd before caching them again since it might 
still
have those from previous edp > 1.4 but we might not need them if current edp < 
1.4

> + /* VDSC is supported only for eDp v1.4 or higher, DPCD 0x00700 offset */
> + if (intel_dp->edp_dpcd[0] < 0x03)

Use the #define DP_EDP_14 for the eDP version check of 1.4

> + return;
> +
> + /* Read DPCD 0x060 to 0x06a */
> + if (drm_dp_dpcd_read(_dp->aux, DP_DSC_SUPPORT, intel_dp->dsc_dpcd,
> +  sizeof(intel_dp->dsc_dpcd)) < 0)
> + return;
> +
> + dp_dsc_caps->is_dsc_supported = intel_dp->dsc_dpcd[0] &
> + DP_DSC_DECOMPRESSION_IS_SUPPORTED;

Like I mentioned on patch 1/10 from the reviews I had got on my DSC patches,
no need to have is_dsc_supported field or dp_dsc_caps since the entire set of
DPCD registers is cached, this can be computed on the fly when needed.

> +
> + if (!dp_dsc_caps->is_dsc_supported)
> + return;
> +
> + drm_dp_dpcd_readb(_dp->aux, 0x090, _dpcd);
> + intel_dp->fec_dpcd = fec_dpcd;
> +
> + /* For DP DSC, FEC support is must */

FEC support is only needed for DP DSC not eDP, but looks like currently this
function is only getting called on eDP init connector and not on all DP
hotplug processing. If its for DP then, even DP 1.4 version check needed.
Since here its only eDP, FEC is not mandatory infact no FEC support on eDP 1.4.

> + if (!(intel_dp->fec_dpcd & 0x1))
> + return;
> +
> + /* No VDSC support for less than 8 BPC */
> + if (intel_dp->dsc_dpcd[0xa] < DP_DSC_8_BPC)

Use some #define instead of 0xa ( I had used intel_dp->dsc_dpcd[DP_DSC_BPC - 
DP_DSC_SUPPORT])

> + return;

What happens to the cached values when you return from this function in case of
no VDSC support. Shouldnt they be reset now that it cannot be implemented.

> +
> + if (intel_dp->dsc_dpcd[0xa] & DP_DSC_8_BPC)
> + DRM_INFO("8 Bits per color support\n");
> + if (intel_dp->dsc_dpcd[0xa] & DP_DSC_10_BPC)
> + DRM_INFO("10 Bits per color support\n");
> + if (intel_dp->dsc_dpcd[0xa] & DP_DSC_12_BPC)
> + DRM_INFO("12 Bits per color support\n");
> +
> + dp_dsc_caps->dsc_major_ver = intel_dp->dsc_dpcd[1] & DP_DSC_MAJOR_MASK;
> + dp_dsc_caps->dsc_minor_ver = (intel_dp->dsc_dpcd[1] &
> + DP_DSC_MINOR_MASK) >> DP_DSC_MINOR_SHIFT;
> +
> + rcbuffer_blocksize = intel_dp->dsc_dpcd[2] & 0x3;
> +
> + switch (rcbuffer_blocksize) {
> + case 0:
> + dp_dsc_caps->rcbuffer_blocksize = 1;
> + break;
> + case 1:
> + dp_dsc_caps->rcbuffer_blocksize = 4;
> + break;
> + case 2:
> + dp_dsc_caps->rcbuffer_blocksize = 16;
> + break;
> + case 3:
> + dp_dsc_caps->rcbuffer_blocksize = 64;
> + break;
> + default:
> + break;
> +
> + }
> + dp_dsc_caps->rcbuffer_size_in_blocks = intel_dp->dsc_dpcd[3] + 1;
> +
> + dp_dsc_caps->rcbuffer_size =
> + dp_dsc_caps->rcbuffer_size_in_blocks *
> + dp_dsc_caps->rcbuffer_blocksize * 1024 * 8;
> +

Move this rc buffer computation to a helper. (refer to my patch)

> + dp_dsc_caps->slice_caps = intel_dp->dsc_dpcd[4];

get this again in a  helper. Refer to my patch.

> + line_buffer_bit_depth = intel_dp->dsc_dpcd[5];
> +
> + if (line_buffer_bit_depth == 8)
> + dp_dsc_caps->line_buffer_bit_depth = intel_dp->dsc_dpcd[5];
> + else
> + dp_dsc_caps->line_buffer_bit_depth = intel_dp->dsc_dpcd[5] + 9;

Move these above to helpers and use #defines for these numerical values making 
it
more readable.

> +
> + dp_dsc_caps->is_block_pred_supported = intel_dp->dsc_dpcd[6] &
> + DP_DSC_BLK_PREDICTION_IS_SUPPORTED;
> +
> + dp_dsc_caps->sink_support_max_bpp =