[Intel-gfx] [PATCH 3/3] drm/i915: Only grab correct forcewake for the engine with execlists

[Tvrtko Ursulin]

On 07/04/16 15:24, Chris Wilson wrote:
> On Thu, Apr 07, 2016 at 03:05:40PM +0100, Tvrtko Ursulin wrote:
>> @@ -2099,6 +2101,12 @@ logical_ring_init(struct drm_device *dev, struct intel_engine_cs *engine)
>>
>>   	logical_ring_init_platform_invariants(engine);
>>
>> +	engine->fw_domains_elsp =
>> +		intel_reg_write_fw_domains(dev_priv, RING_ELSP(engine));
>> +	engine->fw_domains_csb =
>> +		intel_reg_write_fw_domains(dev_priv,
>> +					   RING_CONTEXT_STATUS_PTR(engine));
>
> So is write a superset of fw? Tends to be reads that require fw more
> than writes (gen6/7 fifo, gen8 write shadowing).
>
> I think we need a READ | WRITE direction field.

Hm, yes embedding too much knowledge in the caller, how about just:

	engine->fw_domains_csb = intel_reg_read_fw_domains(dev_priv,
				 RING_CONTEXT_STATUS_PTR(engine));

	engine->fw_domains_csb |= intel_reg_write_fw_domains(dev_priv,
				  RING_CONTEXT_STATUS_PTR(engine));

?

>> +/**
>> + * intel_reg_write_fw_domains - which forcewake domains are needed to write a register
>> + * @dev_priv: pointer to struct drm_i915_private
>> + * @reg: register in question
>> + *
>> + * Returns a set of forcewake domains required to be taken with for example
>> + * intel_uncore_forcewake_get for the specified register to be writable with the
>> + * raw mmio accessors.
>> + */
>> +enum forcewake_domains
>> +intel_reg_write_fw_domains(struct drm_i915_private *dev_priv, i915_reg_t reg)
>> +{
>> +	enum forcewake_domains fw_domains;
>> +
>> +	if (intel_vgpu_active(dev_priv->dev))
>> +		return 0;
>> +
>> +	switch (INTEL_INFO(dev_priv)->gen) {
>> +	case 9:
>> +		fw_domains = __gen9_reg_write_fw_domains(i915_mmio_reg_offset(reg));
>> +		break;
>> +	case 8:
>> +		if (IS_CHERRYVIEW(dev_priv))
>> +			fw_domains = __chv_reg_write_fw_domains(i915_mmio_reg_offset(reg));
>> +		else
>> +			fw_domains = __gen8_reg_write_fw_domains(i915_mmio_reg_offset(reg));
>> +		break;
>> +	default:
>> +		MISSING_CASE(INTEL_INFO(dev_priv)->gen);
>> +	case 7:
>> +	case 6:
>
> This is actually a tricky one. gen6/7 maintain a FIFO to store mmio
> writes whilst it is powered down. If we fill that fifo we drop writes
> (and that fifo is shared with functions on the device, i.e. it is not
> ours to fill exclusively). So should we be saving that if you want to
> make lots of writes you should take this forcewake domain. Yes. We should
> report what domains they would require, it is still up to the caller as
> to whether they risk the FIFO overflowing, but they should have the right
> information to hand.

Missed that. But it isn't part of forcewake domains. So what would you 
return? Fake out a new domain just complicates things and adds cost for 
everyone. Maybe better just to limit the whole thing to gen8+ and leave 
olders platforms untouched?

Regards,

Tvrtko