Re: [v2 1/6] i915/dp/fec: Cache the FEC_CAPABLE DPCD register

From: Jani Nikula <jani.nikula@linux.intel.com>
To: Manasi Navare <manasi.d.navare@intel.com>,
	Anusha Srivatsa <anusha.srivatsa@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [v2 1/6] i915/dp/fec: Cache the FEC_CAPABLE DPCD register
Date: Tue, 16 Oct 2018 10:43:51 +0300	[thread overview]
Message-ID: <87y3ayl4js.fsf@intel.com> (raw)
In-Reply-To: <20181015220522.GD2371@intel.com>

On Mon, 15 Oct 2018, Manasi Navare <manasi.d.navare@intel.com> wrote:
> On Mon, Oct 15, 2018 at 02:50:32PM -0700, Anusha Srivatsa wrote:
>> Similar to DSC DPCD registers, let us cache
>> FEC_CAPABLE register to avoid using stale
>> values. With this we can avoid aux reads
>> everytime and instead read the cached values.
>> 
>> Suggested-by: Jani Nikula <jani.nikula@linux.intel.com>
>> Cc: Jani Nikula <jani.nikula@linux.intel.com>
>> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
>> Cc: Manasi Navare <manasi.d.navare@intel.com>
>> Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_dp.c  | 8 ++++++++
>>  drivers/gpu/drm/i915/intel_drv.h | 1 +
>>  2 files changed, 9 insertions(+)
>> 
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index dcc5a207fcbd..8a0e0a0b26f6 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -4163,8 +4163,10 @@ static void intel_dp_get_dsc_sink_cap(struct intel_dp *intel_dp)
>>  	/*
>>  	 *Clear the cached register set to avoid using stale values
>>  	 * for the sinks that do not support DSC.
>> +	 * Similarly, clear the cached FEC register.
>>  	 */
>>  	memset(intel_dp->dsc_dpcd, 0, sizeof(intel_dp->dsc_dpcd));
>> +	memset(intel_dp->fec_dpcd, 0, sizeof(intel_dp->fec_dpcd));
>
> Memset is an expensive opertaion for just a single value,
> just set that to 0

Agreed on making it a single byte and setting it to 0, but the memset
impact on performance is noise compared to e.g. the native aux
transactions on this path.

BR,
Jani.

>
>>  
>>  	/* Cache the DSC DPCD if eDP or DP rev >= 1.4 */
>>  	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x14 ||
>> @@ -4179,6 +4181,12 @@ static void intel_dp_get_dsc_sink_cap(struct intel_dp *intel_dp)
>>  			      (int)sizeof(intel_dp->dsc_dpcd),
>>  			      intel_dp->dsc_dpcd);
>>  	}
>> +	/* FEC is supported only on DP 1.4 */
>> +	if (!intel_dp_is_edp(intel_dp) && intel_dp->dpcd[DP_DPCD_REV] >= 0x14) {
>> +		if (drm_dp_dpcd_readb(&intel_dp->aux, DP_FEC_CAPABILITY,
>> +				      intel_dp->fec_dpcd) < 0)
>> +			DRM_ERROR("Failed to read FEC DPCD register\n");
>> +	}
>>  }
>>  
>>  static bool
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 7b4af8cba279..b87ea052c9ca 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -1086,6 +1086,7 @@ struct intel_dp {
>>  	uint8_t downstream_ports[DP_MAX_DOWNSTREAM_PORTS];
>>  	uint8_t edp_dpcd[EDP_DISPLAY_CTL_CAP_SIZE];
>>  	u8 dsc_dpcd[DP_DSC_RECEIVER_CAP_SIZE];
>> +	u8 fec_dpcd[1];
>
> Why create an array for just 1 value, I think just a u8 fec_capable field
> will suffice
>
> Manasi
>
>>  	/* source rates */
>>  	int num_source_rates;
>>  	const int *source_rates;
>> -- 
>> 2.17.1
>> 

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx