Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Wed, 5 Sep 2018 20:19:19 +0200
From:   Daniel Vetter <daniel@ffwll.ch>
To:     Gerd Hoffmann <kraxel@redhat.com>
Cc:     dri-devel@lists.freedesktop.org, David Airlie <airlied@linux.ie>,
        michel@daenzer.net, open list <linux-kernel@vger.kernel.org>,
        Sean Paul <sean@poorly.run>
Subject: Re: [PATCH v2 4/6] drm: fix drm_mode_addfb() on big endian machines.
Message-ID: <20180905181919.GH4929@phenom.ffwll.local>
Mail-Followup-To: Gerd Hoffmann <kraxel@redhat.com>,
        dri-devel@lists.freedesktop.org, David Airlie <airlied@linux.ie>,
        michel@daenzer.net, open list <linux-kernel@vger.kernel.org>,
        Sean Paul <sean@poorly.run>
References: <20180905060445.15008-1-kraxel@redhat.com>
 <20180905060445.15008-5-kraxel@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20180905060445.15008-5-kraxel@redhat.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, Sep 05, 2018 at 08:04:43AM +0200, Gerd Hoffmann wrote:
> Userspace on big endian machhines typically expects the ADDFB ioctl
> returns a big endian framebuffer.  drm_mode_addfb() will call
> drm_mode_addfb2() unconditionally with little endian DRM_FORMAT_*
> values though, which is wrong.  This patch fixes that.
> 
> Drivers (both kernel and xorg) have quirks in place to deal with the
> broken drm_mode_addfb() behavior.  Because of this we can't just change
> drm_mode_addfb() behavior for everybody without breaking things.  Add
> the quirk_addfb_prefer_host_byte_order field to mode_config, so drivers
> can opt-in.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  include/drm/drm_mode_config.h     | 14 ++++++++++++++
>  drivers/gpu/drm/drm_framebuffer.c | 11 +++++++++++
>  2 files changed, 25 insertions(+)
> 
> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> index 5d29f4ba6f..65020086c9 100644
> --- a/include/drm/drm_mode_config.h
> +++ b/include/drm/drm_mode_config.h
> @@ -811,6 +811,20 @@ struct drm_mode_config {
>  	uint32_t preferred_depth, prefer_shadow;
>  	bool quirk_addfb_prefer_xbgr_30bpp;
>  
> +	/*

This doesn't parse, needs /**

$ make htmldocs

to check it.

> +	 * @quirk_addfb_prefer_host_byte_order:
> +	 *
> +	 * When set to true drm_mode_addfb() will pick host byte order
> +	 * pixel_format when calling drm_mode_addfb2().  This is how
> +	 * drm_mode_addfb() should have worked from day one.  It
> +	 * didn't though, so we ended up with quirks in both kernel
> +	 * and userspace drivers to deal with the broken behavior.
> +	 * Simply fixing drm_mode_addfb() unconditionally would break
> +	 * these drivers, so add a quirk bit here to allow drivers
> +	 * opt-in.
> +	 */
> +	bool quirk_addfb_prefer_host_byte_order;

I wonder whether we should reject addFB2 on big endian hosts if this isn't
set. Otherwise we need addfb3, aka will never be able to fix this. Anyway,
that might be good work for a follow-up ...

Up to this patch series (with the kerneldoc fixed):

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> +
>  	/**
>  	 * @async_page_flip: Does this device support async flips on the primary
>  	 * plane?
> diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
> index 888c4d53cf..f863f8a20f 100644
> --- a/drivers/gpu/drm/drm_framebuffer.c
> +++ b/drivers/gpu/drm/drm_framebuffer.c
> @@ -124,6 +124,17 @@ int drm_mode_addfb(struct drm_device *dev, struct drm_mode_fb_cmd *or,
>  	    r.pixel_format == DRM_FORMAT_XRGB2101010)
>  		r.pixel_format = DRM_FORMAT_XBGR2101010;
>  
> +	if (dev->mode_config.quirk_addfb_prefer_host_byte_order) {
> +		if (r.pixel_format == DRM_FORMAT_XRGB8888)
> +			r.pixel_format = DRM_FORMAT_HOST_XRGB8888;
> +		if (r.pixel_format == DRM_FORMAT_ARGB8888)
> +			r.pixel_format = DRM_FORMAT_HOST_ARGB8888;
> +		if (r.pixel_format == DRM_FORMAT_RGB565)
> +			r.pixel_format = DRM_FORMAT_HOST_RGB565;
> +		if (r.pixel_format == DRM_FORMAT_XRGB1555)
> +			r.pixel_format = DRM_FORMAT_HOST_XRGB1555;
> +	}
> +
>  	ret = drm_mode_addfb2(dev, &r, file_priv);
>  	if (ret)
>  		return ret;
> -- 
> 2.9.3
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch