[Intel-gfx] [PATCH] drm/i915: Move number of scalers initialization to runtime init

[Maiti, Nabendu Bikash]

On 11/25/2016 3:14 PM, Chris Wilson wrote:
> On Fri, Nov 25, 2016 at 03:16:58PM +0530, Nabendu Maiti wrote:
>> In future patches, we require greater flexibility in describing
>> the number of scalers available on each CRTC. To ease that transition
>> we move the current assignment to intel_device_info.
>>
>> Scaler structure initialisation is done if scaler is available on the CRTC.
>> Gen9 check is not required as on depending upon numbers of scalers we
>> initialize scalers or return without doing anything in skl_init_scalers.
>>
>> v2: Added Chris's commenents.
> comments :)
Accepted. :)
>
>> Signed-off-by: Nabendu Maiti <nabendu.bikash.maiti at intel.com>
> Reviewed-by: Chris Wilson <chris at chris-wilson.co.uk>
>
>> ---
>>  drivers/gpu/drm/i915/i915_drv.h          |  1 +
>>  drivers/gpu/drm/i915/intel_device_info.c |  3 +++
>>  drivers/gpu/drm/i915/intel_display.c     | 18 ++++++++----------
>>  3 files changed, 12 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 1ec9619..bb8c5f0 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -758,6 +758,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 5d11002..2062170 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -15266,6 +15266,11 @@ static void skl_init_scalers(struct drm_i915_private *dev_priv,
>>  		&crtc_state->scaler_state;
>>  	int i;
>>
>> +	crtc->num_scalers = dev_priv->info.num_scalers[crtc->pipe];
>> +
>
> Blank here is overkill
>
>> +	if (!crtc->num_scalers)
>> +		return;
>> +
>
> crtc->num_scalers = <info>;
> if (!ctrc->num_scalers)
> 	return;
>
> is quite clean.
Accepted.

>
>>  	for (i = 0; i < crtc->num_scalers; i++) {
>>  		struct intel_scaler *scaler = &scaler_state->scalers[i];
>>
>> @@ -15297,16 +15302,6 @@ static int intel_crtc_init(struct drm_i915_private *dev_priv, enum pipe pipe)
>>  	intel_crtc->base.state = &crtc_state->base;
>>  	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);
>> -	}
>> -
>>  	primary = intel_primary_plane_create(dev_priv, pipe);
>>  	if (IS_ERR(primary)) {
>>  		ret = PTR_ERR(primary);
>> @@ -15348,6 +15343,9 @@ static int intel_crtc_init(struct drm_i915_private *dev_priv, enum pipe pipe)
>>
>>  	intel_crtc->wm.cxsr_allowed = true;
>>
>> +	/* initialize shared scalers */
>> +	skl_init_scalers(dev_priv, intel_crtc, crtc_state);
>
> As this is now called by all, it really should be
> intel_crtc_init_scalers() and we only need to pass intel_crtc at this
> point.
okey , changing to intel_crtc_init_scalers()

I think we can also potspone it later on by substituting skl_* by 
intel_crtc_*  or i9xx_crtc_*scaler in next patch. Cleanup needed for 
skylake_pfit** and some other scaler functions.

Yes removing dev_priv as folllowing,
struct drm_i915_private *dev_priv =  to_i915(crtc->base.dev); ?

But crtc_state need to to be passed, Apart from crtc_init , 
haswell_get_pipe_config also call it. We pass the current active 
pipe_state. Not sure *config in intel_crtc will serve the same purpose 
for both cases.

> -Chris
>

-- 
Regards,
Nabendu