Re: [PATCH] drm/i915/tgl: Enabling DSC on Pipe A for TGL

["Tolakanahalli Pradeep, Madhumitha" <madhumitha.tolakanahalli.pradeep@intel.com>]

On Thu, 2019-08-15 at 11:53 -0700, Manasi Navare wrote:
> On Thu, Aug 15, 2019 at 11:39:54AM -0700, Tolakanahalli Pradeep,
> Madhumitha wrote:
> > On Thu, 2019-08-15 at 11:24 -0700, Manasi Navare wrote:
> > > On Wed, Aug 14, 2019 at 04:51:17PM -0700, Madhumitha
> > > Tolakanahalli
> > > Pradeep wrote:
> > > > Removing restriction on Pipe A as TigerLake onwards, all the
> > > > pipes
> > > > support DSC.
> > > 
> > > May be elaborate this commit message a little bit something like:
> > > "On previous platforms, DSC was not supported on Pipe A while
> > > starting TGL, its is supported
> > > on all pipes. So remove the DSC and FEC restriction on Pipe A for
> > > TGL
> > > onwards.
> > > 
> > 
> > Alright, will update that for rev2.
> > 
> > > > 
> > > > Cc: Manasi Navare <manasi.d.navare@intel.com>
> > > > Signed-off-by: Madhumitha Tolakanahalli Pradeep <
> > > > madhumitha.tolakanahalli.pradeep@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/display/intel_dp.c | 16 ++++++++++++----
> > > >  1 file changed, 12 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > > > b/drivers/gpu/drm/i915/display/intel_dp.c
> > > > index 4884c87c8ed7..a5b50f93fac5 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > > > @@ -1739,8 +1739,12 @@ static bool
> > > > intel_dp_source_supports_fec(struct intel_dp *intel_dp,
> > > >  {
> > > >  	struct drm_i915_private *dev_priv =
> > > > dp_to_i915(intel_dp);
> > > >  
> > > > -	return INTEL_GEN(dev_priv) >= 11 &&
> > > > -		pipe_config->cpu_transcoder != TRANSCODER_A;
> > > > +	/* On TGL, DSC is supported on all Pipes */
> > > 
> > >                    ^^^^ FEC supported on all pipes
> > 
> > Oops, will change this.
> > 
> > > > +	if (INTEL_GEN(dev_priv) >= 12)
> > > > +		return true;
> > > > +	else
> > > > +		return INTEL_GEN(dev_priv) == 11 &&
> 
> Also here I think its better to use IS_GEN(dev_priv, 11)

Yes, that does make it clearer, I'll change it for v2.

> 
> > > > +			pipe_config->cpu_transcoder !=
> > > > TRANSCODER_A;
> > > >  }
> > > >  
> > > >  static bool intel_dp_supports_fec(struct intel_dp *intel_dp,
> > > > @@ -1755,8 +1759,12 @@ static bool
> > > > intel_dp_source_supports_dsc(struct intel_dp *intel_dp,
> > > >  {
> > > >  	struct drm_i915_private *dev_priv =
> > > > dp_to_i915(intel_dp);
> > > >  
> > > > -	return INTEL_GEN(dev_priv) >= 10 &&
> > > > -		pipe_config->cpu_transcoder != TRANSCODER_A;
> > > > +	/* On TGL, DSC is supported on all Pipes */
> > > > +	if (INTEL_GEN(dev_priv) >= 12)
> > > > +		return true;
> > > > +	else
> > > > +		return (INTEL_GEN(dev_priv) == 10 ||
> > > > INTEL_GEN(dev_priv) == 11) &&
> > > 
> > > Why cant you just use INTEL_GEN(dev_priv) >=10 ?
> > 
> > INTEL_GEN(dev_priv) >= 10 was the existing condition. With the new
> > condition added, there would be an overlapping set
> > ie INTEL_GEN(dev_priv) >= 10 would still be TRUE for GEN >= 12.
> > Though
> > this wouldn't affect the logic of the code, thought it would make
> > more
> > sense to sperate it out this way. 
> 
> But since we return for GEN >=12 , the only time it would fall to GEN
> >=10 is for 10 and 11
> so that should work, or you could use IS_GEN(dev_priv, 10) ||
> IS_GEN(dev_priv, 11)
> 
> But may be check with Lucas on what would be the preferred way

Yeah, it wouldn't affect the logic. I debated about it for a while too.

@Lucas, what do you think is the preferred way to implement this?

> 
> Manasi
> > 
> > > 
> > > Manasi
> > > 
> > > > +			pipe_config->cpu_transcoder !=
> > > > TRANSCODER_A;
> > > >  }
> > > >  
> > > >  static bool intel_dp_supports_dsc(struct intel_dp *intel_dp,
> > > > -- 
> > > > 2.17.1
> > > > 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx