Re: [Mesa-dev] [PATCH 3/3] i965/screen: Allow modifiers on sRGB formats
On Wed, 29 Aug 2018 at 16:43, Eric Engestrom wrote: > On Tuesday, 2018-08-28 21:44:54 -0500, Jason Ekstrand wrote: > > On Tue, Aug 28, 2018 at 5:22 PM Jason Ekstrand wrote: > > > This effectively reverts a26693493570a9d0f0fba1be617e01ee7bfff4db which > > > was a misguided attempt at protecting intel_query_dma_buf_modifiers from > > > invalid formats. Unfortunately, in some internal EGL cases, we can get > > > an SRGB format validly in this function. Rejecting such formats caused > > > us to not allow CCS in some cases where we should have been allowing it. > > > > > > There's some question of whether or not we really should be using SRGB > > > "fourcc" formats that aren't actually in drm_foucc.h but there's not > > > much harm in allowing them through here. > > > > > > [...] > > > > > > @@ -1302,6 +1302,14 @@ intel_query_dma_buf_formats(__DRIscreen *_screen, > > > int max, > > > int num_formats = 0, i; > > > > > > for (i = 0; i < ARRAY_SIZE(intel_image_formats); i++) { > > > + /* These two formats are valid DRI formats but do not exist in > > > + * drm_fourcc.h in the Linux kernel. We don't want to accidentally > > > + * advertise them through the EGL layer. > > > + */ > > > + if (intel_image_formats[i].fourcc == __DRI_IMAGE_FOURCC_SARGB > > > || > > > + intel_image_formats[i].fourcc == __DRI_IMAGE_FOURCC_SABGR) > > > + return false; > > > > > > > This should be a continue. Fixed locally. > > With that, the series is > Reviewed-by: Eric Engestrom ... and also: Reviewed-by: Daniel Stone ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 3/3] i965/screen: Allow modifiers on sRGB formats
On Wed, Aug 29, 2018 at 10:42 AM Eric Engestrom wrote: > On Tuesday, 2018-08-28 21:44:54 -0500, Jason Ekstrand wrote: > > On Tue, Aug 28, 2018 at 5:22 PM Jason Ekstrand > wrote: > > > > > This effectively reverts a26693493570a9d0f0fba1be617e01ee7bfff4db which > > > was a misguided attempt at protecting intel_query_dma_buf_modifiers > from > > > invalid formats. Unfortunately, in some internal EGL cases, we can get > > > an SRGB format validly in this function. Rejecting such formats caused > > > us to not allow CCS in some cases where we should have been allowing > it. > > > > > > There's some question of whether or not we really should be using SRGB > > > "fourcc" formats that aren't actually in drm_foucc.h but there's not > > > much harm in allowing them through here. > > > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107223 > > > Fixes: a26693493570 "i965/screen: Return false for unsupported..." > > > --- > > > src/mesa/drivers/dri/i965/intel_screen.c | 14 +++--- > > > 1 file changed, 11 insertions(+), 3 deletions(-) > > > > > > diff --git a/src/mesa/drivers/dri/i965/intel_screen.c > > > b/src/mesa/drivers/dri/i965/intel_screen.c > > > index eaf5a3b9feb..ac1938f6204 100644 > > > --- a/src/mesa/drivers/dri/i965/intel_screen.c > > > +++ b/src/mesa/drivers/dri/i965/intel_screen.c > > > @@ -1274,9 +1274,9 @@ static bool > > > intel_image_format_is_supported(const struct gen_device_info *devinfo, > > > const struct intel_image_format *fmt) > > > { > > > - if (fmt->fourcc == __DRI_IMAGE_FOURCC_SARGB || > > > - fmt->fourcc == __DRI_IMAGE_FOURCC_SABGR) > > > - return false; > > > + /* Currently, all formats with an intel_image_format are available > on > > > all > > > +* platforms so there's really nothing to check there. > > > +*/ > > > > > > #ifndef NDEBUG > > > if (fmt->nplanes == 1) { > > > @@ -1302,6 +1302,14 @@ intel_query_dma_buf_formats(__DRIscreen > *_screen, > > > int max, > > > int num_formats = 0, i; > > > > > > for (i = 0; i < ARRAY_SIZE(intel_image_formats); i++) { > > > + /* These two formats are valid DRI formats but do not exist in > > > + * drm_fourcc.h in the Linux kernel. We don't want to > accidentally > > > + * advertise them through the EGL layer. > > > + */ > > > + if (intel_image_formats[i].fourcc == > __DRI_IMAGE_FOURCC_SARGB || > > > + intel_image_formats[i].fourcc == > __DRI_IMAGE_FOURCC_SABGR) > > > + return false; > > > > > > > This should be a continue. Fixed locally. > > With that, the series is > Reviewed-by: Eric Engestrom > Thanks! ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 3/3] i965/screen: Allow modifiers on sRGB formats
On Tuesday, 2018-08-28 21:44:54 -0500, Jason Ekstrand wrote: > On Tue, Aug 28, 2018 at 5:22 PM Jason Ekstrand wrote: > > > This effectively reverts a26693493570a9d0f0fba1be617e01ee7bfff4db which > > was a misguided attempt at protecting intel_query_dma_buf_modifiers from > > invalid formats. Unfortunately, in some internal EGL cases, we can get > > an SRGB format validly in this function. Rejecting such formats caused > > us to not allow CCS in some cases where we should have been allowing it. > > > > There's some question of whether or not we really should be using SRGB > > "fourcc" formats that aren't actually in drm_foucc.h but there's not > > much harm in allowing them through here. > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107223 > > Fixes: a26693493570 "i965/screen: Return false for unsupported..." > > --- > > src/mesa/drivers/dri/i965/intel_screen.c | 14 +++--- > > 1 file changed, 11 insertions(+), 3 deletions(-) > > > > diff --git a/src/mesa/drivers/dri/i965/intel_screen.c > > b/src/mesa/drivers/dri/i965/intel_screen.c > > index eaf5a3b9feb..ac1938f6204 100644 > > --- a/src/mesa/drivers/dri/i965/intel_screen.c > > +++ b/src/mesa/drivers/dri/i965/intel_screen.c > > @@ -1274,9 +1274,9 @@ static bool > > intel_image_format_is_supported(const struct gen_device_info *devinfo, > > const struct intel_image_format *fmt) > > { > > - if (fmt->fourcc == __DRI_IMAGE_FOURCC_SARGB || > > - fmt->fourcc == __DRI_IMAGE_FOURCC_SABGR) > > - return false; > > + /* Currently, all formats with an intel_image_format are available on > > all > > +* platforms so there's really nothing to check there. > > +*/ > > > > #ifndef NDEBUG > > if (fmt->nplanes == 1) { > > @@ -1302,6 +1302,14 @@ intel_query_dma_buf_formats(__DRIscreen *_screen, > > int max, > > int num_formats = 0, i; > > > > for (i = 0; i < ARRAY_SIZE(intel_image_formats); i++) { > > + /* These two formats are valid DRI formats but do not exist in > > + * drm_fourcc.h in the Linux kernel. We don't want to accidentally > > + * advertise them through the EGL layer. > > + */ > > + if (intel_image_formats[i].fourcc == __DRI_IMAGE_FOURCC_SARGB || > > + intel_image_formats[i].fourcc == __DRI_IMAGE_FOURCC_SABGR) > > + return false; > > > > This should be a continue. Fixed locally. With that, the series is Reviewed-by: Eric Engestrom > > > > + > >if (!intel_image_format_is_supported(&screen->devinfo, > > &intel_image_formats[i])) > > continue; > > -- > > 2.17.1 > > > > > ___ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 3/3] i965/screen: Allow modifiers on sRGB formats
Hi, On 29.08.2018 01:22, Jason Ekstrand wrote: This effectively reverts a26693493570a9d0f0fba1be617e01ee7bfff4db which was a misguided attempt at protecting intel_query_dma_buf_modifiers from invalid formats. Unfortunately, in some internal EGL cases, we can get an SRGB format validly in this function. While in SynMark EGL tests E2E RBC got completely lost, there was some perf impact (at most half of E2E RBC impact) also in GLX using onscreen versions of GfxBench ALU2, Tessellation and Manhattan 3.0 tests. This patch series (with the "continue" fix you mentioned in later comment) fixes both SynMark & GfxBench regressions. Tested-By: Eero Tamminen - Eero Rejecting such formats caused us to not allow CCS in some cases where we should have been allowing it. There's some question of whether or not we really should be using SRGB "fourcc" formats that aren't actually in drm_foucc.h but there's not much harm in allowing them through here. Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107223 Fixes: a26693493570 "i965/screen: Return false for unsupported..." --- src/mesa/drivers/dri/i965/intel_screen.c | 14 +++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/src/mesa/drivers/dri/i965/intel_screen.c b/src/mesa/drivers/dri/i965/intel_screen.c index eaf5a3b9feb..ac1938f6204 100644 --- a/src/mesa/drivers/dri/i965/intel_screen.c +++ b/src/mesa/drivers/dri/i965/intel_screen.c @@ -1274,9 +1274,9 @@ static bool intel_image_format_is_supported(const struct gen_device_info *devinfo, const struct intel_image_format *fmt) { - if (fmt->fourcc == __DRI_IMAGE_FOURCC_SARGB || - fmt->fourcc == __DRI_IMAGE_FOURCC_SABGR) - return false; + /* Currently, all formats with an intel_image_format are available on all +* platforms so there's really nothing to check there. +*/ #ifndef NDEBUG if (fmt->nplanes == 1) { @@ -1302,6 +1302,14 @@ intel_query_dma_buf_formats(__DRIscreen *_screen, int max, int num_formats = 0, i; for (i = 0; i < ARRAY_SIZE(intel_image_formats); i++) { + /* These two formats are valid DRI formats but do not exist in + * drm_fourcc.h in the Linux kernel. We don't want to accidentally + * advertise them through the EGL layer. + */ + if (intel_image_formats[i].fourcc == __DRI_IMAGE_FOURCC_SARGB || + intel_image_formats[i].fourcc == __DRI_IMAGE_FOURCC_SABGR) + return false; + if (!intel_image_format_is_supported(&screen->devinfo, &intel_image_formats[i])) continue; ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 3/3] i965/screen: Allow modifiers on sRGB formats
On Tue, Aug 28, 2018 at 5:22 PM Jason Ekstrand wrote: > This effectively reverts a26693493570a9d0f0fba1be617e01ee7bfff4db which > was a misguided attempt at protecting intel_query_dma_buf_modifiers from > invalid formats. Unfortunately, in some internal EGL cases, we can get > an SRGB format validly in this function. Rejecting such formats caused > us to not allow CCS in some cases where we should have been allowing it. > > There's some question of whether or not we really should be using SRGB > "fourcc" formats that aren't actually in drm_foucc.h but there's not > much harm in allowing them through here. > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107223 > Fixes: a26693493570 "i965/screen: Return false for unsupported..." > --- > src/mesa/drivers/dri/i965/intel_screen.c | 14 +++--- > 1 file changed, 11 insertions(+), 3 deletions(-) > > diff --git a/src/mesa/drivers/dri/i965/intel_screen.c > b/src/mesa/drivers/dri/i965/intel_screen.c > index eaf5a3b9feb..ac1938f6204 100644 > --- a/src/mesa/drivers/dri/i965/intel_screen.c > +++ b/src/mesa/drivers/dri/i965/intel_screen.c > @@ -1274,9 +1274,9 @@ static bool > intel_image_format_is_supported(const struct gen_device_info *devinfo, > const struct intel_image_format *fmt) > { > - if (fmt->fourcc == __DRI_IMAGE_FOURCC_SARGB || > - fmt->fourcc == __DRI_IMAGE_FOURCC_SABGR) > - return false; > + /* Currently, all formats with an intel_image_format are available on > all > +* platforms so there's really nothing to check there. > +*/ > > #ifndef NDEBUG > if (fmt->nplanes == 1) { > @@ -1302,6 +1302,14 @@ intel_query_dma_buf_formats(__DRIscreen *_screen, > int max, > int num_formats = 0, i; > > for (i = 0; i < ARRAY_SIZE(intel_image_formats); i++) { > + /* These two formats are valid DRI formats but do not exist in > + * drm_fourcc.h in the Linux kernel. We don't want to accidentally > + * advertise them through the EGL layer. > + */ > + if (intel_image_formats[i].fourcc == __DRI_IMAGE_FOURCC_SARGB || > + intel_image_formats[i].fourcc == __DRI_IMAGE_FOURCC_SABGR) > + return false; > This should be a continue. Fixed locally. > + >if (!intel_image_format_is_supported(&screen->devinfo, > &intel_image_formats[i])) > continue; > -- > 2.17.1 > > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 3/3] i965/screen: Allow modifiers on sRGB formats
This effectively reverts a26693493570a9d0f0fba1be617e01ee7bfff4db which was a misguided attempt at protecting intel_query_dma_buf_modifiers from invalid formats. Unfortunately, in some internal EGL cases, we can get an SRGB format validly in this function. Rejecting such formats caused us to not allow CCS in some cases where we should have been allowing it. There's some question of whether or not we really should be using SRGB "fourcc" formats that aren't actually in drm_foucc.h but there's not much harm in allowing them through here. Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107223 Fixes: a26693493570 "i965/screen: Return false for unsupported..." --- src/mesa/drivers/dri/i965/intel_screen.c | 14 +++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/src/mesa/drivers/dri/i965/intel_screen.c b/src/mesa/drivers/dri/i965/intel_screen.c index eaf5a3b9feb..ac1938f6204 100644 --- a/src/mesa/drivers/dri/i965/intel_screen.c +++ b/src/mesa/drivers/dri/i965/intel_screen.c @@ -1274,9 +1274,9 @@ static bool intel_image_format_is_supported(const struct gen_device_info *devinfo, const struct intel_image_format *fmt) { - if (fmt->fourcc == __DRI_IMAGE_FOURCC_SARGB || - fmt->fourcc == __DRI_IMAGE_FOURCC_SABGR) - return false; + /* Currently, all formats with an intel_image_format are available on all +* platforms so there's really nothing to check there. +*/ #ifndef NDEBUG if (fmt->nplanes == 1) { @@ -1302,6 +1302,14 @@ intel_query_dma_buf_formats(__DRIscreen *_screen, int max, int num_formats = 0, i; for (i = 0; i < ARRAY_SIZE(intel_image_formats); i++) { + /* These two formats are valid DRI formats but do not exist in + * drm_fourcc.h in the Linux kernel. We don't want to accidentally + * advertise them through the EGL layer. + */ + if (intel_image_formats[i].fourcc == __DRI_IMAGE_FOURCC_SARGB || + intel_image_formats[i].fourcc == __DRI_IMAGE_FOURCC_SABGR) + return false; + if (!intel_image_format_is_supported(&screen->devinfo, &intel_image_formats[i])) continue; -- 2.17.1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev