Re: [PATCH] drm/i915: Moving no of scalers initialization to runtime init

["Maiti, Nabendu Bikash" <nabendu.bikash.maiti@intel.com>]

On 11/22/2016 9:05 PM, Chris Wilson wrote:
> On Tue, Nov 22, 2016 at 05:31:05PM +0530, Nabendu Maiti wrote:
>
> Try to avoid confusing contractions (i.e. no no) and give us a verb in
> that sentence in that Subject line.
>
>> Number of scalers initialization is moved to runtime init from
>> intel_crtc_init for platform specific initialization.
>
> so that... Tell us why!
Next patch set.
>
>> Signed-off-by: Nabendu Maiti <nabendu.bikash.maiti@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_debugfs.c      |  2 +-
>>  drivers/gpu/drm/i915/i915_drv.h          |  1 +
>>  drivers/gpu/drm/i915/intel_device_info.c |  3 +++
>>  drivers/gpu/drm/i915/intel_display.c     | 10 ++--------
>>  drivers/gpu/drm/i915/intel_drv.h         |  4 ++--
>>  5 files changed, 9 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
>> index b7f42c4..8349abe 100644
>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>> @@ -3080,7 +3080,7 @@ static void intel_scaler_info(struct seq_file *m, struct intel_crtc *intel_crtc)
>>  			   pipe_config->scaler_state.scaler_users,
>>  			   pipe_config->scaler_state.scaler_id);
>>
>> -		for (i = 0; i < SKL_NUM_SCALERS; i++) {
>> +		for (i = 0; i < num_scalers; i++) {
>>  			struct intel_scaler *sc =
>>  					&pipe_config->scaler_state.scalers[i];
>
> Already applied.
>
Removed from this patch.

>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index be67aee..6eed9c2 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -731,6 +731,7 @@ struct intel_device_info {
>>  	u16 device_id;
>>  	u8 num_pipes;
>>  	u8 num_sprites[I915_MAX_PIPES];
>> +	u8 num_scalers[I915_MAX_PIPES];
>>  	u8 gen;
>>  	u16 gen_mask;
>>  	u8 ring_mask; /* Rings supported by the HW */
>> diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c
>> index 185e3bb..ef26fa8 100644
>> --- a/drivers/gpu/drm/i915/intel_device_info.c
>> +++ b/drivers/gpu/drm/i915/intel_device_info.c
>> @@ -282,6 +282,9 @@ void intel_device_info_runtime_init(struct drm_i915_private *dev_priv)
>>  		info->num_sprites[PIPE_A] = 2;
>>  		info->num_sprites[PIPE_B] = 2;
>>  		info->num_sprites[PIPE_C] = 1;
>> +		info->num_scalers[PIPE_A] = 2;
>> +		info->num_scalers[PIPE_B] = 2;
>> +		info->num_scalers[PIPE_C] = 1;
>>  	} else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
>>  		for_each_pipe(dev_priv, pipe)
>>  			info->num_sprites[pipe] = 2;
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index bd2c99e..d2023c4 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -15269,14 +15269,8 @@ static int intel_crtc_init(struct drm_i915_private *dev_priv, enum pipe pipe)
>>  	crtc_state->base.crtc = &intel_crtc->base;
>>
>>  	/* initialize shared scalers */
>> -	if (INTEL_GEN(dev_priv) >= 9) {
>> -		if (pipe == PIPE_C)
>> -			intel_crtc->num_scalers = 1;
>> -		else
>> -			intel_crtc->num_scalers = SKL_NUM_SCALERS;
>> -
>> -		skl_init_scalers(dev_priv, intel_crtc, crtc_state);
>> -	}
>> +	intel_crtc->num_scalers =  dev_priv->info.num_scalers[pipe];
>
> Double space after =.
>
Done.

>> +	skl_init_scalers(dev_priv, intel_crtc, crtc_state);
>
> Now called for everybody, not just gen9+. Why? Is it safe? If it was
> safe it would not be called skl_*(). intel_crtc->num_scalers is now a
> candidate for including in init_scalers().
>
Added check on numbers of scalers in each crtc in next version. If no 
scaler, return without initialization. Also can move skl_init_scalers to 
below plane initialization. Scaler is not utilized upto this point.
In next patches will change the function names more generic.

>>  	primary = intel_primary_plane_create(dev_priv, pipe);
>>  	if (IS_ERR(primary)) {
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index cd132c2..3f89607 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -430,8 +430,8 @@ struct intel_scaler {
>>  };
>>
>>  struct intel_crtc_scaler_state {
>> -#define SKL_NUM_SCALERS 2
>> -	struct intel_scaler scalers[SKL_NUM_SCALERS];
>> +#define MAX_NUM_SCALERS 2
>> +	struct intel_scaler scalers[MAX_NUM_SCALERS];
>
> Replaced the gen marker rather than the abreviation. So now we have
> what reads as NUM_NUM_SCALERS.
>
Reverting to original SKL_NUM_SCALERS, Will give more generic name later.
-Chris
>

-- 
Regards,
Nabendu
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx