[Mesa-dev] [PATCH 4/6] [v2] i965: Handle Y-tiled modifier
Ben Widawsky
ben at bwidawsk.net
Tue Mar 21 15:33:23 UTC 2017
On 17-03-21 08:07:22, Jason Ekstrand wrote:
>On Tue, Mar 21, 2017 at 8:04 AM, Jason Ekstrand <jason at jlekstrand.net>
>wrote:
>
>> On Mon, Mar 20, 2017 at 8:35 PM, Ben Widawsky <ben at bwidawsk.net> wrote:
>>
>>> This patch begins introducing how we'll actually handle the potentially
>>> many modifiers coming in from the API, how we'll store them, and the
>>> structure in the code to support it.
>>>
>>> Prior to this patch, the Y-tiled modifier would be entirely ignored. It
>>> shouldn't actually be used until this point because we've not bumped the
>>> DRIimage extension version (which is a requirement to use modifiers).
>>>
>>> Measuring later in the series with kmscbe:
>>> Linear:
>>> Read bandwidth: 1048.44 MiB/s
>>> Write bandwidth: 1483.17 MiB/s
>>>
>>> Y-tiled:
>>> Read bandwidth: 471.13 MiB/s
>>> Write bandwidth: 589.10 MiB/s
>>>
>>> Similar functionality was introduced and then reverted here:
>>>
>>> commit 6a0d036483caf87d43ebe2edd1905873446c9589
>>> Author: Ben Widawsky <ben at bwidawsk.net>
>>> Date: Thu Apr 21 20:14:58 2016 -0700
>>>
>>> i965: Always use Y-tiled buffers on SKL+
>>>
>>> v2: Use last set bit instead of first set bit in modifiers to address
>>> bug found by Daniel Stone.
>>>
>>> v3: Use the new priority modifier selection thing. This nullifies the
>>> bug fixed by v2 also.
>>>
>>> Signed-off-by: Ben Widawsky <benjamin.widawsky at intel.com>
>>> Reviewed-by: Eric Engestrom <eric.engestrom at imgtec.com>
>>> Acked-by: Daniel Stone <daniels at collabora.com>
>>> ---
>>> src/gbm/backends/dri/gbm_dri.c | 18 ++++++++++++++++
>>> src/mesa/drivers/dri/i965/intel_screen.c | 36
>>> ++++++++++++++++++++++++++++----
>>> 2 files changed, 50 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/src/gbm/backends/dri/gbm_dri.c
>>> b/src/gbm/backends/dri/gbm_dri.c
>>> index a7ac149365..a78ea89fca 100644
>>> --- a/src/gbm/backends/dri/gbm_dri.c
>>> +++ b/src/gbm/backends/dri/gbm_dri.c
>>> @@ -1143,6 +1143,15 @@ gbm_dri_bo_create(struct gbm_device *gbm,
>>> goto failed;
>>> }
>>>
>>> + if (modifiers) {
>>> + for (int i = 0; i < count; i++)
>>> + if (modifiers[i] == DRM_FORMAT_MOD_INVALID) {
>>> + fprintf(stderr, "Invalid modifier passed in
>>> DRM_FORMAT_MOD_INVALID");
>>> + errno = EINVAL;
>>> + goto failed;
>>> + }
>>> + }
>>> +
>>> bo->image =
>>> dri->image->createImageWithModifiers(dri->screen,
>>> width, height,
>>> @@ -1240,6 +1249,15 @@ gbm_dri_surface_create(struct gbm_device *gbm,
>>> return NULL;
>>> }
>>>
>>> + if (modifiers) {
>>> + for (int i = 0; i < count; i++)
>>> + if (modifiers[i] == DRM_FORMAT_MOD_INVALID) {
>>> + fprintf(stderr, "Invalid modifier passed in
>>> DRM_FORMAT_MOD_INVALID");
>>> + errno = EINVAL;
>>> + return NULL;
>>> + }
>>> + }
>>>
>>
>> This could be its own patch but I don't care too much.
>>
>
>Also, you don't need this anymore now that MOD_INVALID just falls through
>and doesn't assert below. Feel free to drop it, pull it into its own
>patch, or leave it here. (I have a mild preference for dropping it and
>just ignoring MOD_INVALID but don't care too much). With that, series is
>
>Reviewed-by: Jason Ekstrand <jason at jlekstrand.net>
>
>
intel_create_image_with_modifiers() (which will be called by this) will assert
if INVALID modifier slips through - which is what I wanted. I don't want any dri
implementation to have to deal with INVALID modifiers. So I think it needs to be
here. Did I misunderstand you?
>> +
>>> surf = calloc(1, sizeof *surf);
>>> if (surf == NULL) {
>>> errno = ENOMEM;
>>> diff --git a/src/mesa/drivers/dri/i965/intel_screen.c
>>> b/src/mesa/drivers/dri/i965/intel_screen.c
>>> index 14e60ef1a1..e4f858ed33 100644
>>> --- a/src/mesa/drivers/dri/i965/intel_screen.c
>>> +++ b/src/mesa/drivers/dri/i965/intel_screen.c
>>> @@ -23,6 +23,7 @@
>>> * SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
>>> */
>>>
>>> +#include <drm_fourcc.h>
>>> #include <errno.h>
>>> #include <time.h>
>>> #include <unistd.h>
>>> @@ -518,11 +519,13 @@ intel_destroy_image(__DRIimage *image)
>>> enum modifier_priority {
>>> MODIFIER_PRIORITY_INVALID = 0,
>>> MODIFIER_PRIORITY_LINEAR,
>>> + MODIFIER_PRIORITY_Y,
>>> };
>>>
>>> const uint64_t priority_to_modifier[] = {
>>> [MODIFIER_PRIORITY_INVALID] = DRM_FORMAT_MOD_INVALID,
>>> [MODIFIER_PRIORITY_LINEAR] = DRM_FORMAT_MOD_LINEAR,
>>> + [MODIFIER_PRIORITY_Y] = I915_FORMAT_MOD_Y_TILED,
>>> };
>>>
>>> static uint64_t
>>> @@ -530,11 +533,13 @@ select_best_modifier(struct gen_device_info
>>> *devinfo,
>>> const uint64_t *modifiers,
>>> const unsigned count)
>>> {
>>> -
>>> enum modifier_priority prio = MODIFIER_PRIORITY_INVALID;
>>>
>>> for (int i = 0; i < count; i++) {
>>> switch (modifiers[i]) {
>>> + case I915_FORMAT_MOD_Y_TILED:
>>> + prio = MAX2(prio, MODIFIER_PRIORITY_Y);
>>> + break;
>>> case DRM_FORMAT_MOD_LINEAR:
>>> prio = MAX2(prio, MODIFIER_PRIORITY_LINEAR);
>>> break;
>>> @@ -575,6 +580,9 @@ intel_create_image_common(__DRIscreen *dri_screen,
>>> case DRM_FORMAT_MOD_LINEAR:
>>> tiling = I915_TILING_NONE;
>>> break;
>>> + case I915_FORMAT_MOD_Y_TILED:
>>> + tiling = I915_TILING_Y;
>>> + break;
>>> case DRM_FORMAT_MOD_INVALID:
>>> default:
>>> break;
>>> @@ -626,8 +634,26 @@ intel_create_image_with_modifiers(__DRIscreen
>>> *dri_screen,
>>> const unsigned count,
>>> void *loaderPrivate)
>>> {
>>> - return intel_create_image_common(dri_screen, width, height, format,
>>> 0, NULL,
>>> - 0, loaderPrivate);
>>> + uint64_t local_mods[count];
>>> + int local_count = 0;
>>> +
>>> + /* This compacts the actual modifiers to the ones we know how to
>>> handle */
>>> + for (int i = 0; i < count; i++) {
>>> + switch (modifiers[i]) {
>>> + case I915_FORMAT_MOD_Y_TILED:
>>> + case DRM_FORMAT_MOD_LINEAR:
>>> + local_mods[local_count++] = modifiers[i];
>>> + break;
>>> + case DRM_FORMAT_MOD_INVALID:
>>> + unreachable("Invalid modifiers specified\n");
>>> + default:
>>> + /* Modifiers from other vendors would land here. */
>>> + break;
>>> + }
>>> + }
>>> +
>>> + return intel_create_image_common(dri_screen, width, height, format,
>>> 0,
>>> + local_mods, local_count,
>>> loaderPrivate);
>>> }
>>>
>>> static GLboolean
>>> @@ -1997,7 +2023,9 @@ intelAllocateBuffer(__DRIscreen *dri_screen,
>>> if (intelBuffer == NULL)
>>> return NULL;
>>>
>>> - /* The front and back buffers are color buffers, which are X tiled. */
>>> + /* The front and back buffers are color buffers, which are X tiled.
>>> GEN9+
>>> + * supports Y tiled and compressed buffers, but there is no way to
>>> plumb that
>>> + * through to here. */
>>> uint32_t tiling = I915_TILING_X;
>>> unsigned long pitch;
>>> int cpp = format / 8;
>>> --
>>> 2.12.0
>>>
>>>
>>
More information about the mesa-dev
mailing list