Message-ID: <20221027152222.60e84e79@eldfell>
Date:   Thu, 27 Oct 2022 15:22:22 +0300
From:   Pekka Paalanen <ppaalanen@...il.com>
To:     Thomas Zimmermann <tzimmermann@...e.de>
Cc:     Hector Martin <marcan@...can.st>,
        Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Maxime Ripard <mripard@...nel.org>,
        David Airlie <airlied@...il.com>,
        Daniel Vetter <daniel@...ll.ch>,
        Javier Martinez Canillas <javierm@...hat.com>,
        dri-devel@...ts.freedesktop.org, asahi@...ts.linux.dev,
        linux-kernel@...r.kernel.org, stable@...r.kernel.org
Subject: Re: [PATCH] drm/simpledrm: Only advertise formats that are
 supported

On Thu, 27 Oct 2022 13:08:24 +0200
Thomas Zimmermann <tzimmermann@...e.de> wrote:

> Hi
> 
> Am 27.10.22 um 12:13 schrieb Hector Martin:
> > Until now, simpledrm unconditionally advertised all formats that can be
> > supported natively as conversions. However, we don't actually have a
> > full conversion matrix of helpers. Although the list is arguably
> > provided to userspace in precedence order, userspace can pick something
> > out-of-order (and thus break when it shouldn't), or simply only support
> > a format that is unsupported (and thus think it can work, which results
> > in the appearance of a hang as FB blits fail later on, instead of the
> > initialization error you'd expect in this case).
> > 
> > Split up the format table into separate ones for each required subset,
> > and then pick one based on the native format. Also remove the
> > native<->conversion overlap check from the helper (which doesn't make
> > sense any more, since the native format is advertised anyway and this
> > way RGB565/RGB888 can share a format table), and instead print the same
> > message in simpledrm when the native format is not one for which we have
> > conversions at all.
> > 
> > This fixes a real user regression where the ?RGB2101010 support commit
> > started advertising it unconditionally where not supported, and KWin
> > decided to start to use it over the native format, but also the fixes
> > the spurious RGB565/RGB888 formats which have been wrongly
> > unconditionally advertised since the dawn of simpledrm.
> > 
> > Note: this patch is merged because splitting it into two patches, one
> > for the helper and one for simpledrm, would regress at the midpoint
> > regardless of the order. If simpledrm is changed first, that would break
> > working conversions to RGB565/RGB888 (since those share a table that
> > does not include the native formats). If the helper is changed first, it
> > would start spuriously advertising all conversion formats when the
> > native format doesn't have any supported conversions at all.
> > 
> > Acked-by: Pekka Paalanen <pekka.paalanen@...labora.com>
> > Fixes: 6ea966fca084 ("drm/simpledrm: Add [AX]RGB2101010 formats")
> > Fixes: 11e8f5fd223b ("drm: Add simpledrm driver")
> > Cc: stable@...r.kernel.org
> > Signed-off-by: Hector Martin <marcan@...can.st>
> > ---
> >   drivers/gpu/drm/drm_format_helper.c | 15 -------
> >   drivers/gpu/drm/tiny/simpledrm.c    | 62 +++++++++++++++++++++++++----  
> 
> We currently have two DRM drivers that call drm_fb_build_fourcc_list(): 
> simpledrm and ofdrm. I've been very careful to keep the format selection 
> in sync between them. (That's the reason why the helper exists at all.) 
> If the drivers start to use different logic, it will only become more 
> chaotic.
> 
> The format array of ofdrm is at [1]. At a minimum, ofdrm should get the 
> same fix as simpledrm.
> 
> [1] 
> https://cgit.freedesktop.org/drm/drm-tip/tree/drivers/gpu/drm/tiny/ofdrm.c#n760

Hi Thomas,

yes, the principle applies to all drivers except VKMS: do not emulate
anything in software unless it must be done to prevent kernel
regressions on specific hardware.

ofdrm should indeed do the same.

> >   2 files changed, 55 insertions(+), 22 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c
> > index e2f76621453c..c60c13f3a872 100644
> > --- a/drivers/gpu/drm/drm_format_helper.c
> > +++ b/drivers/gpu/drm/drm_format_helper.c
> > @@ -864,20 +864,6 @@ size_t drm_fb_build_fourcc_list(struct drm_device *dev,
> >   		++fourccs;
> >   	}
> >   
> > -	/*
> > -	 * The plane's atomic_update helper converts the framebuffer's color format
> > -	 * to a native format when copying to device memory.
> > -	 *
> > -	 * If there is not a single format supported by both, device and
> > -	 * driver, the native formats are likely not supported by the conversion
> > -	 * helpers. Therefore *only* support the native formats and add a
> > -	 * conversion helper ASAP.
> > -	 */
> > -	if (!found_native) {
> > -		drm_warn(dev, "Format conversion helpers required to add extra formats.\n");
> > -		goto out;
> > -	}
> > -
> >   	/*
> >   	 * The extra formats, emulated by the driver, go second.
> >   	 */
> > @@ -898,7 +884,6 @@ size_t drm_fb_build_fourcc_list(struct drm_device *dev,
> >   		++fourccs;
> >   	}
> >   
> > -out:
> >   	return fourccs - fourccs_out;
> >   }
> >   EXPORT_SYMBOL(drm_fb_build_fourcc_list);
> > diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
> > index 18489779fb8a..1257411f3d44 100644
> > --- a/drivers/gpu/drm/tiny/simpledrm.c
> > +++ b/drivers/gpu/drm/tiny/simpledrm.c
> > @@ -446,22 +446,48 @@ static int simpledrm_device_init_regulators(struct simpledrm_device *sdev)
> >    */
> >   
> >   /*
> > - * Support all formats of simplefb and maybe more; in order
> > - * of preference. The display's update function will do any
> > + * Support the subset of formats that we have conversion helpers for,
> > + * in order of preference. The display's update function will do any
> >    * conversion necessary.
> >    *
> >    * TODO: Add blit helpers for remaining formats and uncomment
> >    *       constants.
> >    */
> > -static const uint32_t simpledrm_primary_plane_formats[] = {
> > +
> > +/*
> > + * Supported conversions to RGB565 and RGB888:
> > + *   from [AX]RGB8888
> > + */
> > +static const uint32_t simpledrm_primary_plane_formats_base[] = {
> > +	DRM_FORMAT_XRGB8888,
> > +	DRM_FORMAT_ARGB8888,
> > +};
> > +
> > +/*
> > + * Supported conversions to [AX]RGB8888:
> > + *   A/X variants (no-op)
> > + *   from RGB565
> > + *   from RGB888
> > + */
> > +static const uint32_t simpledrm_primary_plane_formats_xrgb8888[] = {
> >   	DRM_FORMAT_XRGB8888,
> >   	DRM_FORMAT_ARGB8888,
> > +	DRM_FORMAT_RGB888,
> >   	DRM_FORMAT_RGB565,
> >   	//DRM_FORMAT_XRGB1555,
> >   	//DRM_FORMAT_ARGB1555,
> > -	DRM_FORMAT_RGB888,
> > +};
> > +
> > +/*
> > + * Supported conversions to [AX]RGB2101010:
> > + *   A/X variants (no-op)
> > + *   from [AX]RGB8888
> > + */
> > +static const uint32_t simpledrm_primary_plane_formats_xrgb2101010[] = {
> >   	DRM_FORMAT_XRGB2101010,
> >   	DRM_FORMAT_ARGB2101010,
> > +	DRM_FORMAT_XRGB8888,
> > +	DRM_FORMAT_ARGB8888,
> >   };
> >   
> >   static const uint64_t simpledrm_primary_plane_format_modifiers[] = {
> > @@ -642,7 +668,8 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
> >   	struct drm_encoder *encoder;
> >   	struct drm_connector *connector;
> >   	unsigned long max_width, max_height;
> > -	size_t nformats;
> > +	const uint32_t *conv_formats;
> > +	size_t conv_nformats, nformats;
> >   	int ret;
> >   
> >   	sdev = devm_drm_dev_alloc(&pdev->dev, drv, struct simpledrm_device, dev);
> > @@ -755,10 +782,31 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
> >   	dev->mode_config.funcs = &simpledrm_mode_config_funcs;
> >   
> >   	/* Primary plane */
> > +	switch (format->format) {  
> 
> I trust you when you say that <native>->XRGB8888 is not enough. But 
> although I've read your replies, I still don't understand why this 
> switch is necessary.
> 
> Why don't we call drm_fb_build_fourcc_list() with the native 
> format/formats and let it append a number of formats, such as adding 
> XRGB888, adding ARGB8888 if necessary, adding ARGB2101010 if necessary. 
> Each with a elaborate comment why and which userspace needs the format. (?)

Something like

uint32_t conv_formats[] = {
	DRM_FORMAT_XRGB8888, /* expected by old userspace */
	DRM_FORMAT_ARGB8888, /* historically exposed and working */
	0,
	0,
	0,
};
size_t conv_nformats = 2;

if (native_format == DRM_FORMAT_XRGB2101010)
	conv_formats[conv_nformats++] = DRM_FORMAT_ARGB2101010; /* historically exposed and working */

if (native_format == DRM_FORMAT_XRGB8888) {
	conv_formats[conv_nformats++] = DRM_FORMAT_RGB565; /* historically exposed and working */
	conv_formats[conv_nformats++] = DRM_FORMAT_RGB888; /* historically exposed and working */
}

maybe?



Thanks,
pq


> 
> Best regards
> Thomas
> 
> > +	case DRM_FORMAT_RGB565:
> > +	case DRM_FORMAT_RGB888:
> > +		conv_formats = simpledrm_primary_plane_formats_base;
> > +		conv_nformats = ARRAY_SIZE(simpledrm_primary_plane_formats_base);
> > +		break;
> > +	case DRM_FORMAT_XRGB8888:
> > +	case DRM_FORMAT_ARGB8888:
> > +		conv_formats = simpledrm_primary_plane_formats_xrgb8888;
> > +		conv_nformats = ARRAY_SIZE(simpledrm_primary_plane_formats_xrgb8888);
> > +		break;
> > +	case DRM_FORMAT_XRGB2101010:
> > +	case DRM_FORMAT_ARGB2101010:
> > +		conv_formats = simpledrm_primary_plane_formats_xrgb2101010;
> > +		conv_nformats = ARRAY_SIZE(simpledrm_primary_plane_formats_xrgb2101010);
> > +		break;
> > +	default:
> > +		conv_formats = NULL;
> > +		conv_nformats = 0;
> > +		drm_warn(dev, "Format conversion helpers required to add extra formats.\n");
> > +		break;
> > +	}
> >   
> >   	nformats = drm_fb_build_fourcc_list(dev, &format->format, 1,
> > -					    simpledrm_primary_plane_formats,
> > -					    ARRAY_SIZE(simpledrm_primary_plane_formats),
> > +					    conv_formats, conv_nformats,
> >   					    sdev->formats, ARRAY_SIZE(sdev->formats));
> >   
> >   	primary_plane = &sdev->primary_plane;  
> 


Content of type "application/pgp-signature" skipped

Powered by blists - more mailing lists