Message-ID: <20190821115355.GH5942@intel.com>
Date:   Wed, 21 Aug 2019 14:53:55 +0300
From:   Ville Syrjälä <ville.syrjala@...ux.intel.com>
To:     Lyude Paul <lyude@...hat.com>
Cc:     dri-devel@...ts.freedesktop.org, nouveau@...ts.freedesktop.org,
        Maxime Ripard <maxime.ripard@...tlin.com>,
        linux-kernel@...r.kernel.org, David Airlie <airlied@...ux.ie>,
        Sean Paul <sean@...rly.run>
Subject: Re: [PATCH v2] drm: Bump encoder limit from 32 to 64

On Tue, Aug 20, 2019 at 08:16:55PM -0400, Lyude Paul wrote:
> Assuming that GPUs would never have even close to 32 separate video
> encoders is quite honestly a pretty reasonable assumption. Unfortunately
> we do not live in a reasonable world, as it looks like it is actually
> possible to find devices that will create more drm_encoder objects then
> this. Case in point: the ThinkPad P71's discrete GPU, which exposes 1
> eDP port and 5 DP ports. On the P71, nouveau attempts to create one
> encoder for the eDP port, and two encoders for each DP++/USB-C port
> along with 4 MST encoders for each DP port. This comes out to 35
> different encoders. Unfortunately, this can't really be optimized to
> make less encoders either.
> 
> So, what if we bumped the limit to 64? Unfortunately this has one very
> awkward drawback: we already expose 32-bit bitmasks for encoders to
> userspace in drm_encoder->possible_clones. Yikes. While cloning is still
> (rarely) used in certain modern video hardware, it's mostly used in
> situations where memory bandwidth is so limited that it's not possible
> to scan out from 2 CRTCs at once.
> 
> So, let's try to compromise here: allow encoders with indexes <32 to
> have non-zero values in drm_encoder->possible_clones, and don't allow
> encoders with higher indexes to set drm_encoder->possible_clones to a
> non-zero value. This allows us to avoid breaking UAPI and keep things
> working sanely for hardware which still uses cloning, while still being
> able to bump up the encoder limit.
> 
> This also fixes driver probing for nouveau on the ThinkPad P71.
> 
> Changes since v1:
> * Move index+possible_clones check out of drm_encoder_init() and into
>   drm_encoder_register_all(), since encoder->possible_clones can get
>   changed any time before registration - Daniel Vetter
> * Update the commit message a bit to accurately reflect modern day usage
>   of hardware cloning, which as Daniel Stone pointed out is apparently a
>   thing
> 
> Signed-off-by: Lyude Paul <lyude@...hat.com>
> Cc: nouveau@...ts.freedesktop.org
> ---
>  drivers/gpu/drm/drm_atomic.c  |  2 +-
>  drivers/gpu/drm/drm_encoder.c | 12 ++++++++++--
>  include/drm/drm_crtc.h        |  2 +-
>  include/drm/drm_encoder.h     | 20 +++++++++++++++-----
>  4 files changed, 27 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 419381abbdd1..27ce988ef0cc 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -392,7 +392,7 @@ static void drm_atomic_crtc_print_state(struct drm_printer *p,
>  	drm_printf(p, "\tcolor_mgmt_changed=%d\n", state->color_mgmt_changed);
>  	drm_printf(p, "\tplane_mask=%x\n", state->plane_mask);
>  	drm_printf(p, "\tconnector_mask=%x\n", state->connector_mask);
> -	drm_printf(p, "\tencoder_mask=%x\n", state->encoder_mask);
> +	drm_printf(p, "\tencoder_mask=%llx\n", state->encoder_mask);
>  	drm_printf(p, "\tmode: " DRM_MODE_FMT "\n", DRM_MODE_ARG(&state->mode));
>  
>  	if (crtc->funcs->atomic_print_state)
> diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c
> index 7fb47b7b8b44..9d443b45ebba 100644
> --- a/drivers/gpu/drm/drm_encoder.c
> +++ b/drivers/gpu/drm/drm_encoder.c
> @@ -71,6 +71,14 @@ int drm_encoder_register_all(struct drm_device *dev)
>  	int ret = 0;
>  
>  	drm_for_each_encoder(encoder, dev) {
> +		/*
> +		 * Since possible_clones has been exposed to userspace as a
> +		 * 32bit bitmask, we don't allow creating encoders with an
> +		 * index >=32 which are capable of cloning
> +		 */
> +		if (WARN_ON(encoder->index >= 32 && encoder->possible_clones))
> +			return -EINVAL;

I believe possible_clones was supposed to include the encoder itself. Not
really sure why though. I guess we've now decided that it's OK not to do 
that?

git grep tells me drm_atomic_helper.c has some uses of drm_encoder_mask()
that need to be looked at.

> +
>  		if (encoder->funcs->late_register)
>  			ret = encoder->funcs->late_register(encoder);
>  		if (ret)
> @@ -112,8 +120,8 @@ int drm_encoder_init(struct drm_device *dev,
>  {
>  	int ret;
>  
> -	/* encoder index is used with 32bit bitmasks */
> -	if (WARN_ON(dev->mode_config.num_encoder >= 32))
> +	/* encoder index is used with 64bit bitmasks */
> +	if (WARN_ON(dev->mode_config.num_encoder >= 64))
>  		return -EINVAL;
>  
>  	ret = drm_mode_object_add(dev, &encoder->base, DRM_MODE_OBJECT_ENCODER);
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 7d14c11bdc0a..fd0b2438c3d5 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -210,7 +210,7 @@ struct drm_crtc_state {
>  	 * @encoder_mask: Bitmask of drm_encoder_mask(encoder) of encoders
>  	 * attached to this CRTC.
>  	 */
> -	u32 encoder_mask;
> +	u64 encoder_mask;
>  
>  	/**
>  	 * @adjusted_mode:
> diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h
> index 70cfca03d812..3f9cb65694e1 100644
> --- a/include/drm/drm_encoder.h
> +++ b/include/drm/drm_encoder.h
> @@ -159,7 +159,15 @@ struct drm_encoder {
>  	 * encoders can be used in a cloned configuration, they both should have
>  	 * each another bits set.
>  	 *
> -	 * In reality almost every driver gets this wrong.
> +	 * In reality almost every driver gets this wrong, and most modern
> +	 * display hardware does not have support for cloning. As well, while we
> +	 * expose this mask to userspace as 32bits long, we do sure purely to
> +	 * avoid breaking pre-existing UAPI since the limitation on the number
> +	 * of encoders has been increased from 32 bits to 64 bits. In order to
> +	 * maintain functionality for drivers which do actually support cloning,
> +	 * we only allow cloning with encoders that have an index <32. Encoders
> +	 * with indexes higher than 32 are not allowed to specify a non-zero
> +	 * value here.
>  	 *
>  	 * Note that since encoder objects can't be hotplugged the assigned indices
>  	 * are stable and hence known before registering all objects.
> @@ -198,13 +206,15 @@ static inline unsigned int drm_encoder_index(const struct drm_encoder *encoder)
>  }
>  
>  /**
> - * drm_encoder_mask - find the mask of a registered ENCODER
> + * drm_encoder_mask - find the mask of a registered encoder
>   * @encoder: encoder to find mask for
>   *
> - * Given a registered encoder, return the mask bit of that encoder for an
> - * encoder's possible_clones field.
> + * Returns:
> + * A bit mask with the nth bit set, where n is the index of the encoder. Take
> + * care when using this, as the DRM UAPI only allows for 32 bit encoder masks
> + * while internally encoder masks are 64 bits.
>   */
> -static inline u32 drm_encoder_mask(const struct drm_encoder *encoder)
> +static inline u64 drm_encoder_mask(const struct drm_encoder *encoder)
>  {
>  	return 1 << drm_encoder_index(encoder);
>  }
> -- 
> 2.21.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@...ts.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel

Powered by blists - more mailing lists