On Wed, Sep 05, 2018 at 11:22:05AM +0100, Chris Wilson wrote:
> Since this is handling user provided bpp and depth, we need to sanity
> check and propagate the EINVAL back rather than assume what the insane
> client intended and fill the logs with DRM_ERROR.
>
> v2: Check both bpp and depth match the builtin pixel format, and
> introduce a canonical DRM_FORMAT_INVALID to reserve 0 against any future
> fourcc.
>
> Testcase: igt/kms_addfb_basic/legacy-format
> Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
I checked a bunch of randomly selected userspace pieces, and if there's
hairy stuff going on then it's mis-selected bpp.
> ---
> drivers/gpu/drm/drm_fourcc.c | 33 +++++++++++++++++++++----------
> drivers/gpu/drm/drm_framebuffer.c | 7 ++++++-
> include/uapi/drm/drm_fourcc.h | 3 +++
> 3 files changed, 32 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
> index 35c1e2742c27..d9dadbc43327 100644
> --- a/drivers/gpu/drm/drm_fourcc.c
> +++ b/drivers/gpu/drm/drm_fourcc.c
> @@ -45,32 +45,45 @@ static char printable_char(int c)
> */
> uint32_t drm_mode_legacy_fb_format(uint32_t bpp, uint32_t depth)
> {
> - uint32_t fmt;
> + uint32_t fmt = DRM_FORMAT_INVALID;
>
> switch (bpp) {
> case 8:
> - fmt = DRM_FORMAT_C8;
> + if (depth == 0)
> + fmt = DRM_FORMAT_C8;
Michel Dänzer thinks this should be depth == 8 here. I grepped around in
-modesetting, seems outright not supported. Apparently amd drivers do
support it, and set 8/8. With that addressed:
Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
> break;
> case 16:
> - if (depth == 15)
> + switch (depth) {
> + case 15:
> fmt = DRM_FORMAT_XRGB1555;
> - else
> + break;
> + case 16:
> fmt = DRM_FORMAT_RGB565;
> + break;
> + default:
> + break;
> + }
> break;
> case 24:
> - fmt = DRM_FORMAT_RGB888;
> + if (depth == 24)
> + fmt = DRM_FORMAT_RGB888;
> break;
> case 32:
> - if (depth == 24)
> + switch (depth) {
> + case 24:
> fmt = DRM_FORMAT_XRGB8888;
> - else if (depth == 30)
> + break;
> + case 30:
> fmt = DRM_FORMAT_XRGB2101010;
> - else
> + break;
> + case 32:
> fmt = DRM_FORMAT_ARGB8888;
> + break;
> + default:
> + break;
> + }
> break;
> default:
> - DRM_ERROR("bad bpp, assuming x8r8g8b8 pixel format\n");
> - fmt = DRM_FORMAT_XRGB8888;
> break;
> }
>
> diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
> index 781af1d42d76..636f626c5828 100644
> --- a/drivers/gpu/drm/drm_framebuffer.c
> +++ b/drivers/gpu/drm/drm_framebuffer.c
> @@ -112,12 +112,17 @@ int drm_mode_addfb(struct drm_device *dev, struct drm_mode_fb_cmd *or,
> struct drm_mode_fb_cmd2 r = {};
> int ret;
>
> + r.pixel_format = drm_mode_legacy_fb_format(or->bpp, or->depth);
> + if (r.pixel_format == DRM_FORMAT_INVALID) {
> + DRM_DEBUG("bad (bpp:%d, depth:%d)\n", or->bpp, or->depth);
> + return -EINVAL;
> + }
> +
> /* convert to new format and call new ioctl */
> r.fb_id = or->fb_id;
> r.width = or->width;
> r.height = or->height;
> r.pitches[0] = or->pitch;
> - r.pixel_format = drm_mode_legacy_fb_format(or->bpp, or->depth);
> r.handles[0] = or->handle;
>
> if (r.pixel_format == DRM_FORMAT_XRGB2101010 &&
> diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> index 2ed46e9ae16a..139632b87181 100644
> --- a/include/uapi/drm/drm_fourcc.h
> +++ b/include/uapi/drm/drm_fourcc.h
> @@ -71,6 +71,9 @@ extern "C" {
>
> #define DRM_FORMAT_BIG_ENDIAN (1<<31) /* format is big endian instead of little endian */
>
> +/* Reserve 0 for the invalid format specifier */
> +#define DRM_FORMAT_INVALID 0
> +
> /* color index */
> #define DRM_FORMAT_C8 fourcc_code('C', '8', ' ', ' ') /* [7:0] C */
>
> --
> 2.19.0.rc1
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
