[Mesa-dev] [PATCH 4/6] [v2] i965: Handle Y-tiled modifier
Jason Ekstrand
jason at jlekstrand.net
Tue Mar 21 15:36:20 UTC 2017
On Tue, Mar 21, 2017 at 8:33 AM, Ben Widawsky <ben at bwidawsk.net> wrote:
> 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?
That's a problem because the user could provide a list of modifiers none of
which we support. If this happens, we'll return INVALID from
select_best_modifier anyway. There's no way that a higher layer can
possibly know whether or not a list of modifiers contains something that we
can handle unless we require LINEAR to be in the list (which seems a bit
harsh). I think we should just fail to create the image in that case and
set errno to -EINVAL or something like that.
>
> +
>>>
>>>> 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
>>>>
>>>>
>>>>
>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170321/4b896c45/attachment-0001.html>
More information about the mesa-dev
mailing list