[Intel-gfx] [RFC] drm/i915: Render decompression support for Gen9 and above

[Smith, Gary K]

I don't understand why this is an issue. Surely the fb is to describe static state about the buffer, not dynamic state. The fb should be created with the compressed modifier. The compressed property is just a hint to the kernel that the buffer has been completely resolved, hence currently it can be treated as an uncompressed fb (and the aux buffer can be ignored). This is dynamic state that may well change very regularly over the lifetime of the buffer.

It's still a compressed fb, it contains a aux buffer and had to be created with the compressed fb modifier. However, once the userspace has fully resolved the buffer, the aux buffer can be ignored and the compressed fb can be used in any situation where an uncompressed fb would normally be required. This is dynamic state that may well change very regularly over the lifetime of the buffer.

I could allocate two fbs always, and use the appropriate one. We already do this in order to indicate whether a RGBA buffer currently needs to be considered as opaque (RGBX) or blended. Experience has shown that it makes it very complex to debug when the fb keeps on changing its value. However, because we now have 4 different states (Blended/Opaque and Compressed or Resolved), we will now end up with up to 4 fbs per buffer.

We aren't just talking about a few fbs here, we already see more than 100 fbs active during complex situations. Potentially doubling this number is surely a significant increase in memory usage, both from the management side in userspace and the kernel side.

Thanks
Gary



-----Original Message-----
From: Jesse Barnes [mailto:jbarnes at virtuousgeek.org] 
Sent: Wednesday, September 9, 2015 4:25 PM
To: Daniel Vetter
Cc: Kannan, Vandana; intel-gfx at lists.freedesktop.org; Smith, Gary K
Subject: Re: [Intel-gfx] [RFC] drm/i915: Render decompression support for Gen9 and above

On 09/09/2015 08:23 AM, Daniel Vetter wrote:
> On Tue, Sep 08, 2015 at 03:07:40PM -0700, Jesse Barnes wrote:
>> On 09/07/2015 09:35 AM, Daniel Vetter wrote:
>>> On Sat, Sep 05, 2015 at 01:12:50AM +0530, Vandana Kannan wrote:
>>>> This patch includes enabling render decompression after checking 
>>>> all the requirements (format, tiling, rotation etc.). Along with 
>>>> this, the WAs mentioned in BSpec Workaround page have been implemented.
>>>>
>>>> This patch has been implemented on top of Nabendu/Chandra's NV12 patches.
>>>>
>>>> TODO:
>>>> 1. Disable stereo 3D when render decomp is enabled (bit 7:6) 2. 
>>>> Render decompression must not be used in VTd pass-through mode 3. 
>>>> Program hashing select CHICKEN_MISC1 bit 15 4. For Gen10, add 
>>>> support for RGB 1010102
>>>>
>>>> Signed-off-by: Vandana Kannan <vandana.kannan at intel.com>
>>>> ---
>>>>  drivers/gpu/drm/drm_atomic.c         |   4 +
>>>>  drivers/gpu/drm/drm_crtc.c           |  16 ++++
>>>>  drivers/gpu/drm/i915/intel_display.c | 174 +++++++++++++++++++++++++++++++++++
>>>>  drivers/gpu/drm/i915/intel_drv.h     |   7 ++
>>>>  drivers/gpu/drm/i915/intel_sprite.c  |  35 +++++++
>>>>  include/drm/drm_crtc.h               |  11 +++
>>>>  6 files changed, 247 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_atomic.c 
>>>> b/drivers/gpu/drm/drm_atomic.c index 940f80b..d9004e8 100644
>>>> --- a/drivers/gpu/drm/drm_atomic.c
>>>> +++ b/drivers/gpu/drm/drm_atomic.c
>>>> @@ -607,6 +607,8 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
>>>>  		state->src_h = val;
>>>>  	} else if (property == config->rotation_property) {
>>>>  		state->rotation = val;
>>>> +	} else if (property == config->compression_property) {
>>>> +		state->compression = val;
>>>
>>> Please use a framebuffer modifier instead. Also this needs userspace.
>>
>> I thought we already agreed, based on feedback from the userspace 
>> guys, that a property was easier to use and therefore the way to go?
> 
> Blob hwc want a property because they're afraid of the overhead of 
> creating an additional drm fb object. Until I see data that that 
> overhead is real I see no reason at all to have something else than 
> what the community consensus for these features from 1 year ago at xdc bordeaux.
> 
> If someone disagrees please convince Rob Clark and Thierry Redding 
> (and whomever else took part in that discussion) that we need to 
> change this, I personally don't see the value in this particular bikeshed.

I don't think it was overhead, just convenience and reasoning about how the feature is used.  Cc'ing Gary for more background.

Jesse

---------------------------------------------------------------------
Intel Corporation (UK) Limited
Registered No. 1134945 (England)
Registered Office: Pipers Way, Swindon SN3 1RJ
VAT No: 860 2173 47

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.