[Intel-gfx] [PATCH 1/2] drm: Introduce plane SIZE_HINTS property

[Pekka Paalanen]

On Wed, 8 Feb 2023 23:16:56 +0200
Ville Syrjälä <ville.syrjala at linux.intel.com> wrote:

> On Wed, Feb 08, 2023 at 03:03:49PM +0200, Ville Syrjälä wrote:
> > On Wed, Feb 08, 2023 at 02:13:12PM +0200, Pekka Paalanen wrote:  
> > > On Wed,  8 Feb 2023 06:09:10 +0200
> > > Ville Syrjala <ville.syrjala at linux.intel.com> wrote:
> > >   
> > > > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > > 
> > > > Add a new immutable plane property by which a plane can advertise
> > > > a handful of recommended plane sizes. This would be mostly exposed
> > > > by cursor planes as a slightly more capable replacement for
> > > > the DRM_CAP_CURSOR_WIDTH/HEIGHT caps, which can only declare
> > > > a one size fits all limit for the whole device.
> > > > 
> > > > Currently eg. amdgpu/i915/nouveau just advertize the max cursor
> > > > size via the cursor size caps. But always using the max sized
> > > > cursor can waste a surprising amount of power, so a better
> > > > stragey is desirable.
> > > > 
> > > > Most other drivers don't specify any cursor size at all, in
> > > > which case the ioctl code just claims that 64x64 is a great
> > > > choice. Whether that is actually true is debatable.
> > > > 
> > > > A poll of various compositor developers informs us that
> > > > blindly probing with setcursor/atomic ioctl to determine
> > > > suitable cursor sizes is not acceptable, thus the
> > > > introduction of the new property to supplant the cursor
> > > > size caps. The compositor will now be free to select a
> > > > more optimal cursor size from the short list of options.
> > > > 
> > > > Note that the reported sizes (either via the property or the
> > > > caps) make no claims about things such as plane scaling. So
> > > > these things should only really be consulted for simple
> > > > "cursor like" use cases.
> > > > 
> > > > Cc: Simon Ser <contact at emersion.fr>
> > > > Cc: Jonas Ådahl <jadahl at redhat.com>
> > > > Cc: Daniel Stone <daniel at fooishbar.org>
> > > > Cc: Pekka Paalanen <pekka.paalanen at collabora.com>
> > > > Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > > ---
> > > >  drivers/gpu/drm/drm_mode_config.c |  7 +++++++
> > > >  drivers/gpu/drm/drm_plane.c       | 33 +++++++++++++++++++++++++++++++
> > > >  include/drm/drm_mode_config.h     |  5 +++++
> > > >  include/drm/drm_plane.h           |  4 ++++
> > > >  include/uapi/drm/drm_mode.h       |  5 +++++
> > > >  5 files changed, 54 insertions(+)

...

> > > > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> > > > index 46becedf5b2f..4f0551d7f481 100644
> > > > --- a/include/uapi/drm/drm_mode.h
> > > > +++ b/include/uapi/drm/drm_mode.h
> > > > @@ -849,6 +849,11 @@ struct drm_color_lut {
> > > >  	__u16 reserved;
> > > >  };
> > > >  
> > > > +struct drm_plane_size_hint {
> > > > +	__u16 width;
> > > > +	__u16 height;
> > > > +};  
> > > 
> > > Hi Ville,
> > > 
> > > uAPI documentation is missing.
> > > 
> > > When there is just a single recommended size listed, userspace has it
> > > easy: allocate a pair of DRM dumb buffers for each active CRTC, do
> > > atomic test commits with those, and if the test succeeds, copy in the
> > > pixels and fill the padding.
> > > 
> > > What if we have multiple or a huge number of possible sizes? Probably
> > > for each KMS reconfiguration cycle (which could be up to every refresh
> > > cycle) userspace would need to allocate a new dumb buffer to have the
> > > right size, and then test. Is that something we can actually afford to
> > > do? I don't know.  
> > 
> > Why would you allocate multiple buffers? Just allocate one
> > with the max size and then you can reuse it at any smaller
> > size as needed.  

We also need to double-buffer.

> Unfortunately the use of gbm here means the stride would
> be wrong for the smaller sizes. So I guess a different
> buffer for each size is what you need to do. Unless you
> can just ignore the original stride when filling the buffer.

Why would the stride be wrong? AddFB2 carries explicit stride.

You mean hardware would likely not accept the row padding?

When filling the buffer, we can simply fill the real buffer size.
That's what we already do to pad smaller surfaces up to the cursor size.

So essentially we would just re-do AddFB2 with any smaller size while
keeping the real stride. That's a good idea.

Maybe we could refine this so that userspace uses the stride and height
implied by the caps for allocation, and then use the exact cursor image
size for AddFB2? And have drivers pick any size between those two they
can use. The kernel would need the userspace to promise that the
padding is always zero-initialized, so the driver can simply scan out
any area of the buffer it needs.

Then we don't need SIZE_HINTS.


Thanks,
pq