Re: [PATCH v4 1/6] drm/i915/gen8: Add infrastructure to initialize WA batch buffers

["Siluvery, Arun" <arun.siluvery@linux.intel.com>]

On 16/06/2015 21:33, Chris Wilson wrote:
> On Tue, Jun 16, 2015 at 08:25:20PM +0100, Arun Siluvery wrote:
>> +static int lrc_setup_wa_ctx_obj(struct intel_engine_cs *ring, u32 size)
>> +{
>> +	int ret;
>> +	struct drm_device *dev = ring->dev;
>
> You only use it once, keeping it as a local seems counter-intuitive.
>
>> +	WARN_ON(ring->id != RCS);
>> +
>> +	size = roundup(size, PAGE_SIZE);
>
> Out of curiousity is gcc smart enough to turn this into an ALIGN()?

replaced with PAGE_ALIGN(size)

>
>> +	ring->wa_ctx.obj = i915_gem_alloc_object(dev, size);
>> +	if (!ring->wa_ctx.obj) {
>> +		DRM_DEBUG_DRIVER("alloc LRC WA ctx backing obj failed.\n");
>> +		return -ENOMEM;
>> +	}
>> +
>> +	ret = i915_gem_obj_ggtt_pin(ring->wa_ctx.obj, GEN8_LR_CONTEXT_ALIGN, 0);
>
> Strange choice of alignment since we pass around cacheline offsets.
>
this is from the initial version where it was part of context, sorry 
missed this, replaced with PAGE_SIZE.

>> +	if (ret) {
>> +		DRM_DEBUG_DRIVER("pin LRC WA ctx backing obj failed: %d\n",
>> +				 ret);
>> +		drm_gem_object_unreference(&ring->wa_ctx.obj->base);
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static void lrc_destroy_wa_ctx_obj(struct intel_engine_cs *ring)
>> +{
>> +	WARN_ON(ring->id != RCS);
>> +
>> +	i915_gem_object_ggtt_unpin(ring->wa_ctx.obj);
>> +	drm_gem_object_unreference(&ring->wa_ctx.obj->base);
>> +	ring->wa_ctx.obj = NULL;
>> +}
>> +
>>   /**
>>    * intel_logical_ring_cleanup() - deallocate the Engine Command Streamer
>>    *
>> @@ -1474,7 +1612,29 @@ static int logical_render_ring_init(struct drm_device *dev)
>>   	if (ret)
>>   		return ret;
>>
>> -	return intel_init_pipe_control(ring);
>> +	if (INTEL_INFO(ring->dev)->gen >= 8) {
>> +		ret = lrc_setup_wa_ctx_obj(ring, PAGE_SIZE);
>> +		if (ret) {
>> +			DRM_DEBUG_DRIVER("Failed to setup context WA page: %d\n",
>> +					 ret);
>> +			return ret;
>> +		}
>> +
>> +		ret = intel_init_workaround_bb(ring);
>> +		if (ret) {
>> +			lrc_destroy_wa_ctx_obj(ring);
>> +			DRM_ERROR("WA batch buffers are not initialized: %d\n",
>> +				  ret);
>> +		}
>> +	}
>> +
>> +	ret = intel_init_pipe_control(ring);
>
> Did you consider stuffing it into the spare are of the pipe control
> scatch bo? :)
Not exactly but I think it is better to keep them separate. It is not 
that a single page is not sufficient even if we add more WA in future 
but for logical reasons. In case if there is any error while 
initializing these WA we are destroying the page and continuing further 
which cannot be done with scratch page.

regards
Arun


> -Chris
>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx