[RFC] drm: Introduce max width and height properties for planes

[RFC] drm: Introduce max width and height properties for planes

[Archit Taneja]

High resoultion modes (4K/5K) are supported by encoder/crtc hardware, but
the backing hardware planes to scanout these buffers generally don't
support such large widths. Two (or more) hardware planes are used in
conjunction to scan out the entire width.

One way to support this is to virtualize the drm_plane objects that we
expose to userspace, and let the kms driver internally figure out whether
it needs to club multiple hardware pipes to run a particular mode.

The other option is to represent a drm_plane as it is in hardware, and
leave it to userspace use to manage multiple drm_planes for scanning
out these modes.

The advantage of the latter option is that the kms driver doesn't get
too complicated. It doesn't need to do the decision making on how to
distibute the hardware resources among the drm_planes. It also allows
userspace to continue doing some of the heavy lifting (figure out
the most power efficient combination of pipes, estimate bandwidth
consumption etc) which would have to be moved to the kernel if we
virtualized planes.

In order to achieve the latter, a drm_plane should expose an immutable
property to userspace describing the maximum width and height it can
support. Userspace can then prepare the drm_planes accordingly and
commit the state. This patch introduces these properties.

The main disadvantage is that legacy userspace (i.e. calling SetCrtc
on a high resolution framebuffer) wouldn't work. This, on the other
hand, wouldn't be a problem if we virtualized the planes and manipulated
the hardware pipes in the kenrel. One way around this is to fail large
resolution modes when using legacy setcrtc, or introduce some minimal
helpers in the kernel that automatically use multiple drm_planes when
we try to set such a mode. A solution to this issue isn't a part of the
RFC yet.

We're looking for feedback here and wondering whether this is the right
way to go or not.

Signed-off-by: Archit Taneja <architt@codeaurora.org>
---
 drivers/gpu/drm/drm_atomic.c |  9 +++++++++
 drivers/gpu/drm/drm_crtc.c   | 38 ++++++++++++++++++++++++++++++++++++++
 include/drm/drm_crtc.h       |  6 ++++++
 3 files changed, 53 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 8ee1db8..fded22a 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -839,6 +839,15 @@ static int drm_atomic_plane_check(struct drm_plane *plane,
 		return -ERANGE;
 	}
 
+	if (plane->max_width && (state->src_w >> 16) > plane->max_width ||
+	    plane->max_height && (state->src_h >> 16) > plane->max_height) {
+		DRM_DEBUG_ATOMIC("Invalid source width/height "
+				 "%u.%06ux%u.%06u\n",
+				 state->src_w >> 16, ((state->src_w & 0xffff) * 15625) >> 10,
+				 state->src_h >> 16, ((state->src_h & 0xffff) * 15625) >> 10);
+		return -ERANGE;
+	}
+
 	fb_width = state->fb->width << 16;
 	fb_height = state->fb->height << 16;
 
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index e08f962..f2d3b92 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -1267,6 +1267,9 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
 	plane->possible_crtcs = possible_crtcs;
 	plane->type = type;
 
+	plane->max_width = 0;
+	plane->max_height = 0;
+
 	list_add_tail(&plane->head, &config->plane_list);
 	config->num_total_plane++;
 	if (plane->type == DRM_PLANE_TYPE_OVERLAY)
@@ -4957,6 +4960,41 @@ int drm_mode_plane_set_obj_prop(struct drm_plane *plane,
 }
 EXPORT_SYMBOL(drm_mode_plane_set_obj_prop);
 
+int drm_plane_create_max_width_prop(struct drm_plane *plane, uint32_t max_width)
+{
+	struct drm_property *prop;
+
+	prop = drm_property_create_range(plane->dev, DRM_MODE_PROP_IMMUTABLE,
+					 "MAX_W", max_width, max_width);
+	if (!prop)
+		return -ENOMEM;
+
+	drm_object_attach_property(&plane->base, prop, max_width);
+
+	plane->max_width = max_width;
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_plane_create_max_width_prop);
+
+int drm_plane_create_max_height_prop(struct drm_plane *plane,
+				     uint32_t max_height)
+{
+	struct drm_property *prop;
+
+	prop = drm_property_create_range(plane->dev, DRM_MODE_PROP_IMMUTABLE,
+					 "MAX_H", max_height, max_height);
+	if (!prop)
+		return -ENOMEM;
+
+	drm_object_attach_property(&plane->base, prop, max_height);
+
+	plane->max_height = max_height;
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_plane_create_max_height_prop);
+
 /**
  * drm_mode_obj_get_properties_ioctl - get the current value of a object's property
  * @dev: DRM device
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index e0170bf..6104527 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -1531,6 +1531,8 @@ struct drm_plane {
 	uint32_t *format_types;
 	unsigned int format_count;
 	bool format_default;
+	uint32_t max_width;
+	uint32_t max_height;
 
 	struct drm_crtc *crtc;
 	struct drm_framebuffer *fb;
@@ -2550,6 +2552,10 @@ extern struct drm_property *drm_mode_create_rotation_property(struct drm_device
 							      unsigned int supported_rotations);
 extern unsigned int drm_rotation_simplify(unsigned int rotation,
 					  unsigned int supported_rotations);
+extern int drm_plane_create_max_width_prop(struct drm_plane *plane,
+					   uint32_t max_width);
+extern int drm_plane_create_max_height_prop(struct drm_plane *plane,
+					    uint32_t max_height);
 
 /* Helpers */
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC] drm: Introduce max width and height properties for planes

[Daniel Vetter]

On Wed, May 25, 2016 at 12:03:39PM +0530, Archit Taneja wrote:
> High resoultion modes (4K/5K) are supported by encoder/crtc hardware, but
> the backing hardware planes to scanout these buffers generally don't
> support such large widths. Two (or more) hardware planes are used in
> conjunction to scan out the entire width.

"generally" seems to be a bit an overstatement. Most sensible hw has no
problem at all scanning that kind of stuff out ;-)

> One way to support this is to virtualize the drm_plane objects that we
> expose to userspace, and let the kms driver internally figure out whether
> it needs to club multiple hardware pipes to run a particular mode.
> 
> The other option is to represent a drm_plane as it is in hardware, and
> leave it to userspace use to manage multiple drm_planes for scanning
> out these modes.
> 
> The advantage of the latter option is that the kms driver doesn't get
> too complicated. It doesn't need to do the decision making on how to
> distibute the hardware resources among the drm_planes. It also allows
> userspace to continue doing some of the heavy lifting (figure out
> the most power efficient combination of pipes, estimate bandwidth
> consumption etc) which would have to be moved to the kernel if we
> virtualized planes.
> 
> In order to achieve the latter, a drm_plane should expose an immutable
> property to userspace describing the maximum width and height it can
> support. Userspace can then prepare the drm_planes accordingly and
> commit the state. This patch introduces these properties.
> 
> The main disadvantage is that legacy userspace (i.e. calling SetCrtc
> on a high resolution framebuffer) wouldn't work. This, on the other
> hand, wouldn't be a problem if we virtualized the planes and manipulated
> the hardware pipes in the kenrel. One way around this is to fail large
> resolution modes when using legacy setcrtc, or introduce some minimal
> helpers in the kernel that automatically use multiple drm_planes when
> we try to set such a mode. A solution to this issue isn't a part of the
> RFC yet.
> 
> We're looking for feedback here and wondering whether this is the right
> way to go or not.

So just this week there was a bit a discussion on irc (and I haven't
gotten around to typing the writeup and asking everyone for feedback on
it) on new properties. Super-short summary:

- new properties are new abi like anything else

- drm wants a userspace demonstration vehicle to prove new abi which is:
  open source, patches reviewed & tested by respective canonical upstream
  (so not a vendor fork or something like that). Testcases are explicitly
  not enough, it needs to be the real deal.

- for props specifically which are meant to be used/interpreted by
  compositor this means a patch for drm_hwcomposer, one of the many
  wayland compositors there are (weston, ozone, mutter), or a patch for
  xfree86-video-modesetting. Other userspace thingies are probably a bit
  too much fringe to count.

- this isn't really new, but thus far arm simply sailed in the shadow of
  intel doing all that hard work. Now some arm drivers start to pull
  ahead, adding new ABI that's not yet proven by i915 folks, and with
  success comes a bit more responsibility.

For the interface itself just a few questions:
- We should make the cursor size stuff obselete by this, and instead first
  look up the size limits of the cursor plane, before checking those
  legacy limits.
- Is the size/width really independent of e.g. rotation/pixel format/...
  Should it be the maximum that's possible under best circumstance (things
  could still fail), or the minimum that's guaranteed to work everwhere.
  This is the kind of stuff we need the userspace part for, too.

Cheers, Daniel

> Signed-off-by: Archit Taneja <architt@codeaurora.org>
> ---
>  drivers/gpu/drm/drm_atomic.c |  9 +++++++++
>  drivers/gpu/drm/drm_crtc.c   | 38 ++++++++++++++++++++++++++++++++++++++
>  include/drm/drm_crtc.h       |  6 ++++++
>  3 files changed, 53 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 8ee1db8..fded22a 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -839,6 +839,15 @@ static int drm_atomic_plane_check(struct drm_plane *plane,
>  		return -ERANGE;
>  	}
>  
> +	if (plane->max_width && (state->src_w >> 16) > plane->max_width ||
> +	    plane->max_height && (state->src_h >> 16) > plane->max_height) {
> +		DRM_DEBUG_ATOMIC("Invalid source width/height "
> +				 "%u.%06ux%u.%06u\n",
> +				 state->src_w >> 16, ((state->src_w & 0xffff) * 15625) >> 10,
> +				 state->src_h >> 16, ((state->src_h & 0xffff) * 15625) >> 10);
> +		return -ERANGE;
> +	}
> +
>  	fb_width = state->fb->width << 16;
>  	fb_height = state->fb->height << 16;
>  
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index e08f962..f2d3b92 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -1267,6 +1267,9 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
>  	plane->possible_crtcs = possible_crtcs;
>  	plane->type = type;
>  
> +	plane->max_width = 0;
> +	plane->max_height = 0;
> +
>  	list_add_tail(&plane->head, &config->plane_list);
>  	config->num_total_plane++;
>  	if (plane->type == DRM_PLANE_TYPE_OVERLAY)
> @@ -4957,6 +4960,41 @@ int drm_mode_plane_set_obj_prop(struct drm_plane *plane,
>  }
>  EXPORT_SYMBOL(drm_mode_plane_set_obj_prop);
>  
> +int drm_plane_create_max_width_prop(struct drm_plane *plane, uint32_t max_width)
> +{
> +	struct drm_property *prop;
> +
> +	prop = drm_property_create_range(plane->dev, DRM_MODE_PROP_IMMUTABLE,
> +					 "MAX_W", max_width, max_width);
> +	if (!prop)
> +		return -ENOMEM;
> +
> +	drm_object_attach_property(&plane->base, prop, max_width);
> +
> +	plane->max_width = max_width;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_plane_create_max_width_prop);
> +
> +int drm_plane_create_max_height_prop(struct drm_plane *plane,
> +				     uint32_t max_height)
> +{
> +	struct drm_property *prop;
> +
> +	prop = drm_property_create_range(plane->dev, DRM_MODE_PROP_IMMUTABLE,
> +					 "MAX_H", max_height, max_height);
> +	if (!prop)
> +		return -ENOMEM;
> +
> +	drm_object_attach_property(&plane->base, prop, max_height);
> +
> +	plane->max_height = max_height;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_plane_create_max_height_prop);
> +
>  /**
>   * drm_mode_obj_get_properties_ioctl - get the current value of a object's property
>   * @dev: DRM device
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index e0170bf..6104527 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -1531,6 +1531,8 @@ struct drm_plane {
>  	uint32_t *format_types;
>  	unsigned int format_count;
>  	bool format_default;
> +	uint32_t max_width;
> +	uint32_t max_height;
>  
>  	struct drm_crtc *crtc;
>  	struct drm_framebuffer *fb;
> @@ -2550,6 +2552,10 @@ extern struct drm_property *drm_mode_create_rotation_property(struct drm_device
>  							      unsigned int supported_rotations);
>  extern unsigned int drm_rotation_simplify(unsigned int rotation,
>  					  unsigned int supported_rotations);
> +extern int drm_plane_create_max_width_prop(struct drm_plane *plane,
> +					   uint32_t max_width);
> +extern int drm_plane_create_max_height_prop(struct drm_plane *plane,
> +					    uint32_t max_height);
>  
>  /* Helpers */
>  
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> hosted by The Linux Foundation
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC] drm: Introduce max width and height properties for planes

[Archit Taneja]

On 05/25/2016 12:40 PM, Daniel Vetter wrote:
> On Wed, May 25, 2016 at 12:03:39PM +0530, Archit Taneja wrote:
>> High resoultion modes (4K/5K) are supported by encoder/crtc hardware, but
>> the backing hardware planes to scanout these buffers generally don't
>> support such large widths. Two (or more) hardware planes are used in
>> conjunction to scan out the entire width.
>
> "generally" seems to be a bit an overstatement. Most sensible hw has no
> problem at all scanning that kind of stuff out ;-)
>
>> One way to support this is to virtualize the drm_plane objects that we
>> expose to userspace, and let the kms driver internally figure out whether
>> it needs to club multiple hardware pipes to run a particular mode.
>>
>> The other option is to represent a drm_plane as it is in hardware, and
>> leave it to userspace use to manage multiple drm_planes for scanning
>> out these modes.
>>
>> The advantage of the latter option is that the kms driver doesn't get
>> too complicated. It doesn't need to do the decision making on how to
>> distibute the hardware resources among the drm_planes. It also allows
>> userspace to continue doing some of the heavy lifting (figure out
>> the most power efficient combination of pipes, estimate bandwidth
>> consumption etc) which would have to be moved to the kernel if we
>> virtualized planes.
>>
>> In order to achieve the latter, a drm_plane should expose an immutable
>> property to userspace describing the maximum width and height it can
>> support. Userspace can then prepare the drm_planes accordingly and
>> commit the state. This patch introduces these properties.
>>
>> The main disadvantage is that legacy userspace (i.e. calling SetCrtc
>> on a high resolution framebuffer) wouldn't work. This, on the other
>> hand, wouldn't be a problem if we virtualized the planes and manipulated
>> the hardware pipes in the kenrel. One way around this is to fail large
>> resolution modes when using legacy setcrtc, or introduce some minimal
>> helpers in the kernel that automatically use multiple drm_planes when
>> we try to set such a mode. A solution to this issue isn't a part of the
>> RFC yet.
>>
>> We're looking for feedback here and wondering whether this is the right
>> way to go or not.
>
> So just this week there was a bit a discussion on irc (and I haven't
> gotten around to typing the writeup and asking everyone for feedback on
> it) on new properties. Super-short summary:
>
> - new properties are new abi like anything else
>
> - drm wants a userspace demonstration vehicle to prove new abi which is:
>    open source, patches reviewed & tested by respective canonical upstream
>    (so not a vendor fork or something like that). Testcases are explicitly
>    not enough, it needs to be the real deal.
>
> - for props specifically which are meant to be used/interpreted by
>    compositor this means a patch for drm_hwcomposer, one of the many
>    wayland compositors there are (weston, ozone, mutter), or a patch for
>    xfree86-video-modesetting. Other userspace thingies are probably a bit
>    too much fringe to count.
>
> - this isn't really new, but thus far arm simply sailed in the shadow of
>    intel doing all that hard work. Now some arm drivers start to pull
>    ahead, adding new ABI that's not yet proven by i915 folks, and with
>    success comes a bit more responsibility.

That sounds fair enough.

>
> For the interface itself just a few questions:
> - We should make the cursor size stuff obselete by this, and instead first
>    look up the size limits of the cursor plane, before checking those
>    legacy limits.

Yeah, I guess querying the DRM_CAP_CURSOR_WIDTH/HEIGHT caps would
become obsolete.

> - Is the size/width really independent of e.g. rotation/pixel format/...
>    Should it be the maximum that's possible under best circumstance (things
>    could still fail), or the minimum that's guaranteed to work everwhere.
>    This is the kind of stuff we need the userspace part for, too.

Yeah, it isn't independent of these parameters. I'm not entirely sure
about this either.

Does it make sense to impose a rule that the user first sets the
rotation/format plane properties, and only then read the maximum
width? I'm assuming user space hates such kind of stuff.

If we use the 'best circumstance' max_width, we can first start
with a minimum number of planes that need to be grouped to achieve
the target mode. If that fails the atomic test, then we can try to
add one plane at a time, and reduce the width for each plane.

If we use the minimum/'guaranteed to work' max_width, we'll get
a higher number of planes than needed for this mode. This would pass
the atomic test. We could then reduce a plane at a time and see when
we fail the atomic test.

I guess we need to chose the one that's more probable to get right
the first time. Considering only pixel formats for now, the
minimum/'guaranteed to work' would map to the RGB formats. The best
circumstance ones would probably be the planar video formats. Since
we use RGB more often, the minimum one might make more sense.

We could, of course, give the property a range of max widths to
confuse user space even more.

Archit

>
> Cheers, Daniel
>
>> Signed-off-by: Archit Taneja <architt@codeaurora.org>
>> ---
>>   drivers/gpu/drm/drm_atomic.c |  9 +++++++++
>>   drivers/gpu/drm/drm_crtc.c   | 38 ++++++++++++++++++++++++++++++++++++++
>>   include/drm/drm_crtc.h       |  6 ++++++
>>   3 files changed, 53 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>> index 8ee1db8..fded22a 100644
>> --- a/drivers/gpu/drm/drm_atomic.c
>> +++ b/drivers/gpu/drm/drm_atomic.c
>> @@ -839,6 +839,15 @@ static int drm_atomic_plane_check(struct drm_plane *plane,
>>   		return -ERANGE;
>>   	}
>>
>> +	if (plane->max_width && (state->src_w >> 16) > plane->max_width ||
>> +	    plane->max_height && (state->src_h >> 16) > plane->max_height) {
>> +		DRM_DEBUG_ATOMIC("Invalid source width/height "
>> +				 "%u.%06ux%u.%06u\n",
>> +				 state->src_w >> 16, ((state->src_w & 0xffff) * 15625) >> 10,
>> +				 state->src_h >> 16, ((state->src_h & 0xffff) * 15625) >> 10);
>> +		return -ERANGE;
>> +	}
>> +
>>   	fb_width = state->fb->width << 16;
>>   	fb_height = state->fb->height << 16;
>>
>> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
>> index e08f962..f2d3b92 100644
>> --- a/drivers/gpu/drm/drm_crtc.c
>> +++ b/drivers/gpu/drm/drm_crtc.c
>> @@ -1267,6 +1267,9 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
>>   	plane->possible_crtcs = possible_crtcs;
>>   	plane->type = type;
>>
>> +	plane->max_width = 0;
>> +	plane->max_height = 0;
>> +
>>   	list_add_tail(&plane->head, &config->plane_list);
>>   	config->num_total_plane++;
>>   	if (plane->type == DRM_PLANE_TYPE_OVERLAY)
>> @@ -4957,6 +4960,41 @@ int drm_mode_plane_set_obj_prop(struct drm_plane *plane,
>>   }
>>   EXPORT_SYMBOL(drm_mode_plane_set_obj_prop);
>>
>> +int drm_plane_create_max_width_prop(struct drm_plane *plane, uint32_t max_width)
>> +{
>> +	struct drm_property *prop;
>> +
>> +	prop = drm_property_create_range(plane->dev, DRM_MODE_PROP_IMMUTABLE,
>> +					 "MAX_W", max_width, max_width);
>> +	if (!prop)
>> +		return -ENOMEM;
>> +
>> +	drm_object_attach_property(&plane->base, prop, max_width);
>> +
>> +	plane->max_width = max_width;
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL(drm_plane_create_max_width_prop);
>> +
>> +int drm_plane_create_max_height_prop(struct drm_plane *plane,
>> +				     uint32_t max_height)
>> +{
>> +	struct drm_property *prop;
>> +
>> +	prop = drm_property_create_range(plane->dev, DRM_MODE_PROP_IMMUTABLE,
>> +					 "MAX_H", max_height, max_height);
>> +	if (!prop)
>> +		return -ENOMEM;
>> +
>> +	drm_object_attach_property(&plane->base, prop, max_height);
>> +
>> +	plane->max_height = max_height;
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL(drm_plane_create_max_height_prop);
>> +
>>   /**
>>    * drm_mode_obj_get_properties_ioctl - get the current value of a object's property
>>    * @dev: DRM device
>> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
>> index e0170bf..6104527 100644
>> --- a/include/drm/drm_crtc.h
>> +++ b/include/drm/drm_crtc.h
>> @@ -1531,6 +1531,8 @@ struct drm_plane {
>>   	uint32_t *format_types;
>>   	unsigned int format_count;
>>   	bool format_default;
>> +	uint32_t max_width;
>> +	uint32_t max_height;
>>
>>   	struct drm_crtc *crtc;
>>   	struct drm_framebuffer *fb;
>> @@ -2550,6 +2552,10 @@ extern struct drm_property *drm_mode_create_rotation_property(struct drm_device
>>   							      unsigned int supported_rotations);
>>   extern unsigned int drm_rotation_simplify(unsigned int rotation,
>>   					  unsigned int supported_rotations);
>> +extern int drm_plane_create_max_width_prop(struct drm_plane *plane,
>> +					   uint32_t max_width);
>> +extern int drm_plane_create_max_height_prop(struct drm_plane *plane,
>> +					    uint32_t max_height);
>>
>>   /* Helpers */
>>
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>> hosted by The Linux Foundation
>>
>

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum, hosted by The Linux Foundation
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC] drm: Introduce max width and height properties for planes

[Daniel Vetter]

On Wed, May 25, 2016 at 04:28:36PM +0530, Archit Taneja wrote:
> On 05/25/2016 12:40 PM, Daniel Vetter wrote:
> >- Is the size/width really independent of e.g. rotation/pixel format/...
> >   Should it be the maximum that's possible under best circumstance (things
> >   could still fail), or the minimum that's guaranteed to work everwhere.
> >   This is the kind of stuff we need the userspace part for, too.
> 
> Yeah, it isn't independent of these parameters. I'm not entirely sure
> about this either.
> 
> Does it make sense to impose a rule that the user first sets the
> rotation/format plane properties, and only then read the maximum
> width? I'm assuming user space hates such kind of stuff.
> 
> If we use the 'best circumstance' max_width, we can first start
> with a minimum number of planes that need to be grouped to achieve
> the target mode. If that fails the atomic test, then we can try to
> add one plane at a time, and reduce the width for each plane.
> 
> If we use the minimum/'guaranteed to work' max_width, we'll get
> a higher number of planes than needed for this mode. This would pass
> the atomic test. We could then reduce a plane at a time and see when
> we fail the atomic test.
> 
> I guess we need to chose the one that's more probable to get right
> the first time. Considering only pixel formats for now, the
> minimum/'guaranteed to work' would map to the RGB formats. The best
> circumstance ones would probably be the planar video formats. Since
> we use RGB more often, the minimum one might make more sense.
> 
> We could, of course, give the property a range of max widths to
> confuse user space even more.

An entirely different idea for cases where a simple hint property doesn't
work (other ideas floating around are can_scale, to give a hint whether a
plan can at least in theory up/downscale, or not at all), is that the
kernel gives more specific hints about what it would like to change.

So if userspace asks for a plane, but for the given pixel format it's too
wide, the kernel could return a new proposed value for width. That would
be super-flexible and could cover all kinds of use-case like rotation
needing a specific tiling (fb_modifier) or specific pixel format, or
specific stride.

For the case at hand there's even more worms: What about stride
requirements? Afaik on some hw you just need to split the buffers into 2
planes, but can keep the wide stride (since the limit is the size of the
linebuffers in the hw). On others you need to split the buffer itself into
2, because the plane hw can't cope with huge strides. Again might depend
upon the pixel format.

So in a way height/width is both too much information and not precise
enough. Entirely different approches:
- We just add a might_need_split_plane prop to crtcs where this might be
  needed. Userspace then gets to retry with split buffers if it doesn't
  work with a huge one.

- When I discussed this with qualcom folks for msm we concluded that the
  simplest approach would be to hide this in the kernel. So if you have a
  too wide plane, and need 2 hw planes to scan it out, then do that
  transparently in the kernel. Of course this means that there will be 1
  (or 3 if you need a 2x2 split) fewer planes available, but userspace
  needs to iteratively build up the plane config using ATOMIC_TEST anyway.

If possible for your hw I'm heavily leaning towards this last approach. If
fits entirely into the current atomic design, and all the complexity is
restricted to your driver (you need to have some allocation map between
drm planes and real hw planes, but that's it).

Thoughts?
-Daniel

> 

> Archit
> 
> >
> >Cheers, Daniel
> >
> >>Signed-off-by: Archit Taneja <architt@codeaurora.org>
> >>---
> >>  drivers/gpu/drm/drm_atomic.c |  9 +++++++++
> >>  drivers/gpu/drm/drm_crtc.c   | 38 ++++++++++++++++++++++++++++++++++++++
> >>  include/drm/drm_crtc.h       |  6 ++++++
> >>  3 files changed, 53 insertions(+)
> >>
> >>diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> >>index 8ee1db8..fded22a 100644
> >>--- a/drivers/gpu/drm/drm_atomic.c
> >>+++ b/drivers/gpu/drm/drm_atomic.c
> >>@@ -839,6 +839,15 @@ static int drm_atomic_plane_check(struct drm_plane *plane,
> >>  		return -ERANGE;
> >>  	}
> >>
> >>+	if (plane->max_width && (state->src_w >> 16) > plane->max_width ||
> >>+	    plane->max_height && (state->src_h >> 16) > plane->max_height) {
> >>+		DRM_DEBUG_ATOMIC("Invalid source width/height "
> >>+				 "%u.%06ux%u.%06u\n",
> >>+				 state->src_w >> 16, ((state->src_w & 0xffff) * 15625) >> 10,
> >>+				 state->src_h >> 16, ((state->src_h & 0xffff) * 15625) >> 10);
> >>+		return -ERANGE;
> >>+	}
> >>+
> >>  	fb_width = state->fb->width << 16;
> >>  	fb_height = state->fb->height << 16;
> >>
> >>diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> >>index e08f962..f2d3b92 100644
> >>--- a/drivers/gpu/drm/drm_crtc.c
> >>+++ b/drivers/gpu/drm/drm_crtc.c
> >>@@ -1267,6 +1267,9 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
> >>  	plane->possible_crtcs = possible_crtcs;
> >>  	plane->type = type;
> >>
> >>+	plane->max_width = 0;
> >>+	plane->max_height = 0;
> >>+
> >>  	list_add_tail(&plane->head, &config->plane_list);
> >>  	config->num_total_plane++;
> >>  	if (plane->type == DRM_PLANE_TYPE_OVERLAY)
> >>@@ -4957,6 +4960,41 @@ int drm_mode_plane_set_obj_prop(struct drm_plane *plane,
> >>  }
> >>  EXPORT_SYMBOL(drm_mode_plane_set_obj_prop);
> >>
> >>+int drm_plane_create_max_width_prop(struct drm_plane *plane, uint32_t max_width)
> >>+{
> >>+	struct drm_property *prop;
> >>+
> >>+	prop = drm_property_create_range(plane->dev, DRM_MODE_PROP_IMMUTABLE,
> >>+					 "MAX_W", max_width, max_width);
> >>+	if (!prop)
> >>+		return -ENOMEM;
> >>+
> >>+	drm_object_attach_property(&plane->base, prop, max_width);
> >>+
> >>+	plane->max_width = max_width;
> >>+
> >>+	return 0;
> >>+}
> >>+EXPORT_SYMBOL(drm_plane_create_max_width_prop);
> >>+
> >>+int drm_plane_create_max_height_prop(struct drm_plane *plane,
> >>+				     uint32_t max_height)
> >>+{
> >>+	struct drm_property *prop;
> >>+
> >>+	prop = drm_property_create_range(plane->dev, DRM_MODE_PROP_IMMUTABLE,
> >>+					 "MAX_H", max_height, max_height);
> >>+	if (!prop)
> >>+		return -ENOMEM;
> >>+
> >>+	drm_object_attach_property(&plane->base, prop, max_height);
> >>+
> >>+	plane->max_height = max_height;
> >>+
> >>+	return 0;
> >>+}
> >>+EXPORT_SYMBOL(drm_plane_create_max_height_prop);
> >>+
> >>  /**
> >>   * drm_mode_obj_get_properties_ioctl - get the current value of a object's property
> >>   * @dev: DRM device
> >>diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> >>index e0170bf..6104527 100644
> >>--- a/include/drm/drm_crtc.h
> >>+++ b/include/drm/drm_crtc.h
> >>@@ -1531,6 +1531,8 @@ struct drm_plane {
> >>  	uint32_t *format_types;
> >>  	unsigned int format_count;
> >>  	bool format_default;
> >>+	uint32_t max_width;
> >>+	uint32_t max_height;
> >>
> >>  	struct drm_crtc *crtc;
> >>  	struct drm_framebuffer *fb;
> >>@@ -2550,6 +2552,10 @@ extern struct drm_property *drm_mode_create_rotation_property(struct drm_device
> >>  							      unsigned int supported_rotations);
> >>  extern unsigned int drm_rotation_simplify(unsigned int rotation,
> >>  					  unsigned int supported_rotations);
> >>+extern int drm_plane_create_max_width_prop(struct drm_plane *plane,
> >>+					   uint32_t max_width);
> >>+extern int drm_plane_create_max_height_prop(struct drm_plane *plane,
> >>+					    uint32_t max_height);
> >>
> >>  /* Helpers */
> >>
> >>--
> >>The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> >>hosted by The Linux Foundation
> >>
> >
> 
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> hosted by The Linux Foundation

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC] drm: Introduce max width and height properties for planes

[Rob Clark]

On Wed, May 25, 2016 at 9:12 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, May 25, 2016 at 04:28:36PM +0530, Archit Taneja wrote:
>> On 05/25/2016 12:40 PM, Daniel Vetter wrote:
>> >- Is the size/width really independent of e.g. rotation/pixel format/...
>> >   Should it be the maximum that's possible under best circumstance (things
>> >   could still fail), or the minimum that's guaranteed to work everwhere.
>> >   This is the kind of stuff we need the userspace part for, too.
>>
>> Yeah, it isn't independent of these parameters. I'm not entirely sure
>> about this either.
>>
>> Does it make sense to impose a rule that the user first sets the
>> rotation/format plane properties, and only then read the maximum
>> width? I'm assuming user space hates such kind of stuff.
>>
>> If we use the 'best circumstance' max_width, we can first start
>> with a minimum number of planes that need to be grouped to achieve
>> the target mode. If that fails the atomic test, then we can try to
>> add one plane at a time, and reduce the width for each plane.
>>
>> If we use the minimum/'guaranteed to work' max_width, we'll get
>> a higher number of planes than needed for this mode. This would pass
>> the atomic test. We could then reduce a plane at a time and see when
>> we fail the atomic test.
>>
>> I guess we need to chose the one that's more probable to get right
>> the first time. Considering only pixel formats for now, the
>> minimum/'guaranteed to work' would map to the RGB formats. The best
>> circumstance ones would probably be the planar video formats. Since
>> we use RGB more often, the minimum one might make more sense.
>>
>> We could, of course, give the property a range of max widths to
>> confuse user space even more.
>
> An entirely different idea for cases where a simple hint property doesn't
> work (other ideas floating around are can_scale, to give a hint whether a
> plan can at least in theory up/downscale, or not at all), is that the
> kernel gives more specific hints about what it would like to change.
>
> So if userspace asks for a plane, but for the given pixel format it's too
> wide, the kernel could return a new proposed value for width. That would
> be super-flexible and could cover all kinds of use-case like rotation
> needing a specific tiling (fb_modifier) or specific pixel format, or
> specific stride.
>
> For the case at hand there's even more worms: What about stride
> requirements? Afaik on some hw you just need to split the buffers into 2
> planes, but can keep the wide stride (since the limit is the size of the
> linebuffers in the hw). On others you need to split the buffer itself into
> 2, because the plane hw can't cope with huge strides. Again might depend
> upon the pixel format.
>
> So in a way height/width is both too much information and not precise
> enough. Entirely different approches:
> - We just add a might_need_split_plane prop to crtcs where this might be
>   needed. Userspace then gets to retry with split buffers if it doesn't
>   work with a huge one.
>
> - When I discussed this with qualcom folks for msm we concluded that the
>   simplest approach would be to hide this in the kernel. So if you have a
>   too wide plane, and need 2 hw planes to scan it out, then do that
>   transparently in the kernel. Of course this means that there will be 1
>   (or 3 if you need a 2x2 split) fewer planes available, but userspace
>   needs to iteratively build up the plane config using ATOMIC_TEST anyway.

Just fwiw, there are a few things that we will still end up
abstracting in the kernel by virtualizing the mapping between kms
planes and hw pipes.  And the approach of weston atomic of
incrementally adding more planes w/ TESTONLY flag should work well for
that.  (Let's hope the weston bits get upstream some day..)

But exposing width limit avoids the one-plane to multiple-pipes case,
considerably simplifying things.  And seemed like a generic enough
limit (iirc, it applies to omapdss and probably others), that it would
be cleaner to expose the limit to userspace.  So there should be at
least a couple other drivers that could avoid virtualizing planes with
some help from userspace for this case.

Regarding rotation, I'm not 100% sure.. seems like we could just
document these as the un-rotated limits.  If we really had to, we
could do some sort of dance where userspace sets rotation property on
an un-used plane, and then reads-back the current values of the
read-only prop's.  But that seems awkward.

BR,
-R

> If possible for your hw I'm heavily leaning towards this last approach. If
> fits entirely into the current atomic design, and all the complexity is
> restricted to your driver (you need to have some allocation map between
> drm planes and real hw planes, but that's it).
>
> Thoughts?
> -Daniel
>
>>
>
>> Archit
>>
>> >
>> >Cheers, Daniel
>> >
>> >>Signed-off-by: Archit Taneja <architt@codeaurora.org>
>> >>---
>> >>  drivers/gpu/drm/drm_atomic.c |  9 +++++++++
>> >>  drivers/gpu/drm/drm_crtc.c   | 38 ++++++++++++++++++++++++++++++++++++++
>> >>  include/drm/drm_crtc.h       |  6 ++++++
>> >>  3 files changed, 53 insertions(+)
>> >>
>> >>diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>> >>index 8ee1db8..fded22a 100644
>> >>--- a/drivers/gpu/drm/drm_atomic.c
>> >>+++ b/drivers/gpu/drm/drm_atomic.c
>> >>@@ -839,6 +839,15 @@ static int drm_atomic_plane_check(struct drm_plane *plane,
>> >>            return -ERANGE;
>> >>    }
>> >>
>> >>+   if (plane->max_width && (state->src_w >> 16) > plane->max_width ||
>> >>+       plane->max_height && (state->src_h >> 16) > plane->max_height) {
>> >>+           DRM_DEBUG_ATOMIC("Invalid source width/height "
>> >>+                            "%u.%06ux%u.%06u\n",
>> >>+                            state->src_w >> 16, ((state->src_w & 0xffff) * 15625) >> 10,
>> >>+                            state->src_h >> 16, ((state->src_h & 0xffff) * 15625) >> 10);
>> >>+           return -ERANGE;
>> >>+   }
>> >>+
>> >>    fb_width = state->fb->width << 16;
>> >>    fb_height = state->fb->height << 16;
>> >>
>> >>diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
>> >>index e08f962..f2d3b92 100644
>> >>--- a/drivers/gpu/drm/drm_crtc.c
>> >>+++ b/drivers/gpu/drm/drm_crtc.c
>> >>@@ -1267,6 +1267,9 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
>> >>    plane->possible_crtcs = possible_crtcs;
>> >>    plane->type = type;
>> >>
>> >>+   plane->max_width = 0;
>> >>+   plane->max_height = 0;
>> >>+
>> >>    list_add_tail(&plane->head, &config->plane_list);
>> >>    config->num_total_plane++;
>> >>    if (plane->type == DRM_PLANE_TYPE_OVERLAY)
>> >>@@ -4957,6 +4960,41 @@ int drm_mode_plane_set_obj_prop(struct drm_plane *plane,
>> >>  }
>> >>  EXPORT_SYMBOL(drm_mode_plane_set_obj_prop);
>> >>
>> >>+int drm_plane_create_max_width_prop(struct drm_plane *plane, uint32_t max_width)
>> >>+{
>> >>+   struct drm_property *prop;
>> >>+
>> >>+   prop = drm_property_create_range(plane->dev, DRM_MODE_PROP_IMMUTABLE,
>> >>+                                    "MAX_W", max_width, max_width);
>> >>+   if (!prop)
>> >>+           return -ENOMEM;
>> >>+
>> >>+   drm_object_attach_property(&plane->base, prop, max_width);
>> >>+
>> >>+   plane->max_width = max_width;
>> >>+
>> >>+   return 0;
>> >>+}
>> >>+EXPORT_SYMBOL(drm_plane_create_max_width_prop);
>> >>+
>> >>+int drm_plane_create_max_height_prop(struct drm_plane *plane,
>> >>+                                uint32_t max_height)
>> >>+{
>> >>+   struct drm_property *prop;
>> >>+
>> >>+   prop = drm_property_create_range(plane->dev, DRM_MODE_PROP_IMMUTABLE,
>> >>+                                    "MAX_H", max_height, max_height);
>> >>+   if (!prop)
>> >>+           return -ENOMEM;
>> >>+
>> >>+   drm_object_attach_property(&plane->base, prop, max_height);
>> >>+
>> >>+   plane->max_height = max_height;
>> >>+
>> >>+   return 0;
>> >>+}
>> >>+EXPORT_SYMBOL(drm_plane_create_max_height_prop);
>> >>+
>> >>  /**
>> >>   * drm_mode_obj_get_properties_ioctl - get the current value of a object's property
>> >>   * @dev: DRM device
>> >>diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
>> >>index e0170bf..6104527 100644
>> >>--- a/include/drm/drm_crtc.h
>> >>+++ b/include/drm/drm_crtc.h
>> >>@@ -1531,6 +1531,8 @@ struct drm_plane {
>> >>    uint32_t *format_types;
>> >>    unsigned int format_count;
>> >>    bool format_default;
>> >>+   uint32_t max_width;
>> >>+   uint32_t max_height;
>> >>
>> >>    struct drm_crtc *crtc;
>> >>    struct drm_framebuffer *fb;
>> >>@@ -2550,6 +2552,10 @@ extern struct drm_property *drm_mode_create_rotation_property(struct drm_device
>> >>                                                          unsigned int supported_rotations);
>> >>  extern unsigned int drm_rotation_simplify(unsigned int rotation,
>> >>                                      unsigned int supported_rotations);
>> >>+extern int drm_plane_create_max_width_prop(struct drm_plane *plane,
>> >>+                                      uint32_t max_width);
>> >>+extern int drm_plane_create_max_height_prop(struct drm_plane *plane,
>> >>+                                       uint32_t max_height);
>> >>
>> >>  /* Helpers */
>> >>
>> >>--
>> >>The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>> >>hosted by The Linux Foundation
>> >>
>> >
>>
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>> hosted by The Linux Foundation
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC] drm: Introduce max width and height properties for planes

[Ville Syrjälä]

On Wed, May 25, 2016 at 12:36:53PM -0400, Rob Clark wrote:
> On Wed, May 25, 2016 at 9:12 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Wed, May 25, 2016 at 04:28:36PM +0530, Archit Taneja wrote:
> >> On 05/25/2016 12:40 PM, Daniel Vetter wrote:
> >> >- Is the size/width really independent of e.g. rotation/pixel format/...
> >> >   Should it be the maximum that's possible under best circumstance (things
> >> >   could still fail), or the minimum that's guaranteed to work everwhere.
> >> >   This is the kind of stuff we need the userspace part for, too.
> >>
> >> Yeah, it isn't independent of these parameters. I'm not entirely sure
> >> about this either.
> >>
> >> Does it make sense to impose a rule that the user first sets the
> >> rotation/format plane properties, and only then read the maximum
> >> width? I'm assuming user space hates such kind of stuff.
> >>
> >> If we use the 'best circumstance' max_width, we can first start
> >> with a minimum number of planes that need to be grouped to achieve
> >> the target mode. If that fails the atomic test, then we can try to
> >> add one plane at a time, and reduce the width for each plane.
> >>
> >> If we use the minimum/'guaranteed to work' max_width, we'll get
> >> a higher number of planes than needed for this mode. This would pass
> >> the atomic test. We could then reduce a plane at a time and see when
> >> we fail the atomic test.
> >>
> >> I guess we need to chose the one that's more probable to get right
> >> the first time. Considering only pixel formats for now, the
> >> minimum/'guaranteed to work' would map to the RGB formats. The best
> >> circumstance ones would probably be the planar video formats. Since
> >> we use RGB more often, the minimum one might make more sense.
> >>
> >> We could, of course, give the property a range of max widths to
> >> confuse user space even more.
> >
> > An entirely different idea for cases where a simple hint property doesn't
> > work (other ideas floating around are can_scale, to give a hint whether a
> > plan can at least in theory up/downscale, or not at all), is that the
> > kernel gives more specific hints about what it would like to change.
> >
> > So if userspace asks for a plane, but for the given pixel format it's too
> > wide, the kernel could return a new proposed value for width. That would
> > be super-flexible and could cover all kinds of use-case like rotation
> > needing a specific tiling (fb_modifier) or specific pixel format, or
> > specific stride.
> >
> > For the case at hand there's even more worms: What about stride
> > requirements? Afaik on some hw you just need to split the buffers into 2
> > planes, but can keep the wide stride (since the limit is the size of the
> > linebuffers in the hw). On others you need to split the buffer itself into
> > 2, because the plane hw can't cope with huge strides. Again might depend
> > upon the pixel format.
> >
> > So in a way height/width is both too much information and not precise
> > enough. Entirely different approches:
> > - We just add a might_need_split_plane prop to crtcs where this might be
> >   needed. Userspace then gets to retry with split buffers if it doesn't
> >   work with a huge one.
> >
> > - When I discussed this with qualcom folks for msm we concluded that the
> >   simplest approach would be to hide this in the kernel. So if you have a
> >   too wide plane, and need 2 hw planes to scan it out, then do that
> >   transparently in the kernel. Of course this means that there will be 1
> >   (or 3 if you need a 2x2 split) fewer planes available, but userspace
> >   needs to iteratively build up the plane config using ATOMIC_TEST anyway.
> 
> Just fwiw, there are a few things that we will still end up
> abstracting in the kernel by virtualizing the mapping between kms
> planes and hw pipes.  And the approach of weston atomic of
> incrementally adding more planes w/ TESTONLY flag should work well for
> that.  (Let's hope the weston bits get upstream some day..)
> 
> But exposing width limit avoids the one-plane to multiple-pipes case,
> considerably simplifying things.  And seemed like a generic enough
> limit (iirc, it applies to omapdss and probably others), that it would
> be cleaner to expose the limit to userspace.  So there should be at
> least a couple other drivers that could avoid virtualizing planes with
> some help from userspace for this case.
> 
> Regarding rotation, I'm not 100% sure.. seems like we could just
> document these as the un-rotated limits.  If we really had to, we
> could do some sort of dance where userspace sets rotation property on
> an un-used plane, and then reads-back the current values of the
> read-only prop's.  But that seems awkward.

I'm thinking all of this is doomed to fail. So right now people seem to
want some kind of maximum size of the source viewport. What about the
destination size? Is the max size for unscaled/scaled/smething else?
Rotation/pixel format were already mentioned. How does this interact
with scaling limits? What if the user is willing/not willing to do a
modeset to get a higher clock speed to get moar scaling? How could
the user request a higher clock speed upfront?

There are just so many variables that you can't expose them with a few
numbers. I think maybe the only number we might be expose easily is
the global maximum.

As far as virtualizing the resources goes. I think that would need a
whole new API. Or at least a separate set of objects perhaps. I'm too
lazy to dig up all the old arguments now, but I'm pretty sure there
were many. If this would be done, I suspect the only sane way to do it
would be to just have a hwc implementation in the kernel. As in user
space would pass in the desired configuration, and the kernel would
assign resources as best it can and return the result back to userspace.

> 
> BR,
> -R
> 
> > If possible for your hw I'm heavily leaning towards this last approach. If
> > fits entirely into the current atomic design, and all the complexity is
> > restricted to your driver (you need to have some allocation map between
> > drm planes and real hw planes, but that's it).
> >
> > Thoughts?
> > -Daniel
> >
> >>
> >
> >> Archit
> >>
> >> >
> >> >Cheers, Daniel
> >> >
> >> >>Signed-off-by: Archit Taneja <architt@codeaurora.org>
> >> >>---
> >> >>  drivers/gpu/drm/drm_atomic.c |  9 +++++++++
> >> >>  drivers/gpu/drm/drm_crtc.c   | 38 ++++++++++++++++++++++++++++++++++++++
> >> >>  include/drm/drm_crtc.h       |  6 ++++++
> >> >>  3 files changed, 53 insertions(+)
> >> >>
> >> >>diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> >> >>index 8ee1db8..fded22a 100644
> >> >>--- a/drivers/gpu/drm/drm_atomic.c
> >> >>+++ b/drivers/gpu/drm/drm_atomic.c
> >> >>@@ -839,6 +839,15 @@ static int drm_atomic_plane_check(struct drm_plane *plane,
> >> >>            return -ERANGE;
> >> >>    }
> >> >>
> >> >>+   if (plane->max_width && (state->src_w >> 16) > plane->max_width ||
> >> >>+       plane->max_height && (state->src_h >> 16) > plane->max_height) {
> >> >>+           DRM_DEBUG_ATOMIC("Invalid source width/height "
> >> >>+                            "%u.%06ux%u.%06u\n",
> >> >>+                            state->src_w >> 16, ((state->src_w & 0xffff) * 15625) >> 10,
> >> >>+                            state->src_h >> 16, ((state->src_h & 0xffff) * 15625) >> 10);
> >> >>+           return -ERANGE;
> >> >>+   }
> >> >>+
> >> >>    fb_width = state->fb->width << 16;
> >> >>    fb_height = state->fb->height << 16;
> >> >>
> >> >>diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> >> >>index e08f962..f2d3b92 100644
> >> >>--- a/drivers/gpu/drm/drm_crtc.c
> >> >>+++ b/drivers/gpu/drm/drm_crtc.c
> >> >>@@ -1267,6 +1267,9 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
> >> >>    plane->possible_crtcs = possible_crtcs;
> >> >>    plane->type = type;
> >> >>
> >> >>+   plane->max_width = 0;
> >> >>+   plane->max_height = 0;
> >> >>+
> >> >>    list_add_tail(&plane->head, &config->plane_list);
> >> >>    config->num_total_plane++;
> >> >>    if (plane->type == DRM_PLANE_TYPE_OVERLAY)
> >> >>@@ -4957,6 +4960,41 @@ int drm_mode_plane_set_obj_prop(struct drm_plane *plane,
> >> >>  }
> >> >>  EXPORT_SYMBOL(drm_mode_plane_set_obj_prop);
> >> >>
> >> >>+int drm_plane_create_max_width_prop(struct drm_plane *plane, uint32_t max_width)
> >> >>+{
> >> >>+   struct drm_property *prop;
> >> >>+
> >> >>+   prop = drm_property_create_range(plane->dev, DRM_MODE_PROP_IMMUTABLE,
> >> >>+                                    "MAX_W", max_width, max_width);
> >> >>+   if (!prop)
> >> >>+           return -ENOMEM;
> >> >>+
> >> >>+   drm_object_attach_property(&plane->base, prop, max_width);
> >> >>+
> >> >>+   plane->max_width = max_width;
> >> >>+
> >> >>+   return 0;
> >> >>+}
> >> >>+EXPORT_SYMBOL(drm_plane_create_max_width_prop);
> >> >>+
> >> >>+int drm_plane_create_max_height_prop(struct drm_plane *plane,
> >> >>+                                uint32_t max_height)
> >> >>+{
> >> >>+   struct drm_property *prop;
> >> >>+
> >> >>+   prop = drm_property_create_range(plane->dev, DRM_MODE_PROP_IMMUTABLE,
> >> >>+                                    "MAX_H", max_height, max_height);
> >> >>+   if (!prop)
> >> >>+           return -ENOMEM;
> >> >>+
> >> >>+   drm_object_attach_property(&plane->base, prop, max_height);
> >> >>+
> >> >>+   plane->max_height = max_height;
> >> >>+
> >> >>+   return 0;
> >> >>+}
> >> >>+EXPORT_SYMBOL(drm_plane_create_max_height_prop);
> >> >>+
> >> >>  /**
> >> >>   * drm_mode_obj_get_properties_ioctl - get the current value of a object's property
> >> >>   * @dev: DRM device
> >> >>diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> >> >>index e0170bf..6104527 100644
> >> >>--- a/include/drm/drm_crtc.h
> >> >>+++ b/include/drm/drm_crtc.h
> >> >>@@ -1531,6 +1531,8 @@ struct drm_plane {
> >> >>    uint32_t *format_types;
> >> >>    unsigned int format_count;
> >> >>    bool format_default;
> >> >>+   uint32_t max_width;
> >> >>+   uint32_t max_height;
> >> >>
> >> >>    struct drm_crtc *crtc;
> >> >>    struct drm_framebuffer *fb;
> >> >>@@ -2550,6 +2552,10 @@ extern struct drm_property *drm_mode_create_rotation_property(struct drm_device
> >> >>                                                          unsigned int supported_rotations);
> >> >>  extern unsigned int drm_rotation_simplify(unsigned int rotation,
> >> >>                                      unsigned int supported_rotations);
> >> >>+extern int drm_plane_create_max_width_prop(struct drm_plane *plane,
> >> >>+                                      uint32_t max_width);
> >> >>+extern int drm_plane_create_max_height_prop(struct drm_plane *plane,
> >> >>+                                       uint32_t max_height);
> >> >>
> >> >>  /* Helpers */
> >> >>
> >> >>--
> >> >>The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> >> >>hosted by The Linux Foundation
> >> >>
> >> >
> >>
> >> --
> >> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> >> hosted by The Linux Foundation
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC] drm: Introduce max width and height properties for planes

[Rob Clark]

On Wed, May 25, 2016 at 1:20 PM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> On Wed, May 25, 2016 at 12:36:53PM -0400, Rob Clark wrote:
>> On Wed, May 25, 2016 at 9:12 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
>> > On Wed, May 25, 2016 at 04:28:36PM +0530, Archit Taneja wrote:
>> >> On 05/25/2016 12:40 PM, Daniel Vetter wrote:
>> >> >- Is the size/width really independent of e.g. rotation/pixel format/...
>> >> >   Should it be the maximum that's possible under best circumstance (things
>> >> >   could still fail), or the minimum that's guaranteed to work everwhere.
>> >> >   This is the kind of stuff we need the userspace part for, too.
>> >>
>> >> Yeah, it isn't independent of these parameters. I'm not entirely sure
>> >> about this either.
>> >>
>> >> Does it make sense to impose a rule that the user first sets the
>> >> rotation/format plane properties, and only then read the maximum
>> >> width? I'm assuming user space hates such kind of stuff.
>> >>
>> >> If we use the 'best circumstance' max_width, we can first start
>> >> with a minimum number of planes that need to be grouped to achieve
>> >> the target mode. If that fails the atomic test, then we can try to
>> >> add one plane at a time, and reduce the width for each plane.
>> >>
>> >> If we use the minimum/'guaranteed to work' max_width, we'll get
>> >> a higher number of planes than needed for this mode. This would pass
>> >> the atomic test. We could then reduce a plane at a time and see when
>> >> we fail the atomic test.
>> >>
>> >> I guess we need to chose the one that's more probable to get right
>> >> the first time. Considering only pixel formats for now, the
>> >> minimum/'guaranteed to work' would map to the RGB formats. The best
>> >> circumstance ones would probably be the planar video formats. Since
>> >> we use RGB more often, the minimum one might make more sense.
>> >>
>> >> We could, of course, give the property a range of max widths to
>> >> confuse user space even more.
>> >
>> > An entirely different idea for cases where a simple hint property doesn't
>> > work (other ideas floating around are can_scale, to give a hint whether a
>> > plan can at least in theory up/downscale, or not at all), is that the
>> > kernel gives more specific hints about what it would like to change.
>> >
>> > So if userspace asks for a plane, but for the given pixel format it's too
>> > wide, the kernel could return a new proposed value for width. That would
>> > be super-flexible and could cover all kinds of use-case like rotation
>> > needing a specific tiling (fb_modifier) or specific pixel format, or
>> > specific stride.
>> >
>> > For the case at hand there's even more worms: What about stride
>> > requirements? Afaik on some hw you just need to split the buffers into 2
>> > planes, but can keep the wide stride (since the limit is the size of the
>> > linebuffers in the hw). On others you need to split the buffer itself into
>> > 2, because the plane hw can't cope with huge strides. Again might depend
>> > upon the pixel format.
>> >
>> > So in a way height/width is both too much information and not precise
>> > enough. Entirely different approches:
>> > - We just add a might_need_split_plane prop to crtcs where this might be
>> >   needed. Userspace then gets to retry with split buffers if it doesn't
>> >   work with a huge one.
>> >
>> > - When I discussed this with qualcom folks for msm we concluded that the
>> >   simplest approach would be to hide this in the kernel. So if you have a
>> >   too wide plane, and need 2 hw planes to scan it out, then do that
>> >   transparently in the kernel. Of course this means that there will be 1
>> >   (or 3 if you need a 2x2 split) fewer planes available, but userspace
>> >   needs to iteratively build up the plane config using ATOMIC_TEST anyway.
>>
>> Just fwiw, there are a few things that we will still end up
>> abstracting in the kernel by virtualizing the mapping between kms
>> planes and hw pipes.  And the approach of weston atomic of
>> incrementally adding more planes w/ TESTONLY flag should work well for
>> that.  (Let's hope the weston bits get upstream some day..)
>>
>> But exposing width limit avoids the one-plane to multiple-pipes case,
>> considerably simplifying things.  And seemed like a generic enough
>> limit (iirc, it applies to omapdss and probably others), that it would
>> be cleaner to expose the limit to userspace.  So there should be at
>> least a couple other drivers that could avoid virtualizing planes with
>> some help from userspace for this case.
>>
>> Regarding rotation, I'm not 100% sure.. seems like we could just
>> document these as the un-rotated limits.  If we really had to, we
>> could do some sort of dance where userspace sets rotation property on
>> an un-used plane, and then reads-back the current values of the
>> read-only prop's.  But that seems awkward.
>
> I'm thinking all of this is doomed to fail. So right now people seem to
> want some kind of maximum size of the source viewport. What about the
> destination size? Is the max size for unscaled/scaled/smething else?
> Rotation/pixel format were already mentioned. How does this interact
> with scaling limits? What if the user is willing/not willing to do a
> modeset to get a higher clock speed to get moar scaling? How could
> the user request a higher clock speed upfront?
>
> There are just so many variables that you can't expose them with a few
> numbers. I think maybe the only number we might be expose easily is
> the global maximum.

global maximum sounds reasonable.. perhaps exposing more fine grained
limits should be the domain of some more-fine-grained error reporting
mechanism (that is probably needed, but I think yet to be invented)

> As far as virtualizing the resources goes. I think that would need a
> whole new API. Or at least a separate set of objects perhaps. I'm too
> lazy to dig up all the old arguments now, but I'm pretty sure there
> were many. If this would be done, I suspect the only sane way to do it
> would be to just have a hwc implementation in the kernel. As in user
> space would pass in the desired configuration, and the kernel would
> assign resources as best it can and return the result back to userspace.

actually, without introducing a completely new/different uapi, it
should work pretty reasonably with the way weston atomic works,
incrementally adding planes in TESTONLY steps until it fails.  I
suspect on some more creative hw (msm is prime candidate) we'll still
see custom hwc implementations to squeeze out the last bit of
performance/power.  But with the approach that weston uses a generic
userspace should still work pretty well on that hw.

BR,
-R

>>
>> BR,
>> -R
>>
>> > If possible for your hw I'm heavily leaning towards this last approach. If
>> > fits entirely into the current atomic design, and all the complexity is
>> > restricted to your driver (you need to have some allocation map between
>> > drm planes and real hw planes, but that's it).
>> >
>> > Thoughts?
>> > -Daniel
>> >
>> >>
>> >
>> >> Archit
>> >>
>> >> >
>> >> >Cheers, Daniel
>> >> >
>> >> >>Signed-off-by: Archit Taneja <architt@codeaurora.org>
>> >> >>---
>> >> >>  drivers/gpu/drm/drm_atomic.c |  9 +++++++++
>> >> >>  drivers/gpu/drm/drm_crtc.c   | 38 ++++++++++++++++++++++++++++++++++++++
>> >> >>  include/drm/drm_crtc.h       |  6 ++++++
>> >> >>  3 files changed, 53 insertions(+)
>> >> >>
>> >> >>diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>> >> >>index 8ee1db8..fded22a 100644
>> >> >>--- a/drivers/gpu/drm/drm_atomic.c
>> >> >>+++ b/drivers/gpu/drm/drm_atomic.c
>> >> >>@@ -839,6 +839,15 @@ static int drm_atomic_plane_check(struct drm_plane *plane,
>> >> >>            return -ERANGE;
>> >> >>    }
>> >> >>
>> >> >>+   if (plane->max_width && (state->src_w >> 16) > plane->max_width ||
>> >> >>+       plane->max_height && (state->src_h >> 16) > plane->max_height) {
>> >> >>+           DRM_DEBUG_ATOMIC("Invalid source width/height "
>> >> >>+                            "%u.%06ux%u.%06u\n",
>> >> >>+                            state->src_w >> 16, ((state->src_w & 0xffff) * 15625) >> 10,
>> >> >>+                            state->src_h >> 16, ((state->src_h & 0xffff) * 15625) >> 10);
>> >> >>+           return -ERANGE;
>> >> >>+   }
>> >> >>+
>> >> >>    fb_width = state->fb->width << 16;
>> >> >>    fb_height = state->fb->height << 16;
>> >> >>
>> >> >>diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
>> >> >>index e08f962..f2d3b92 100644
>> >> >>--- a/drivers/gpu/drm/drm_crtc.c
>> >> >>+++ b/drivers/gpu/drm/drm_crtc.c
>> >> >>@@ -1267,6 +1267,9 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
>> >> >>    plane->possible_crtcs = possible_crtcs;
>> >> >>    plane->type = type;
>> >> >>
>> >> >>+   plane->max_width = 0;
>> >> >>+   plane->max_height = 0;
>> >> >>+
>> >> >>    list_add_tail(&plane->head, &config->plane_list);
>> >> >>    config->num_total_plane++;
>> >> >>    if (plane->type == DRM_PLANE_TYPE_OVERLAY)
>> >> >>@@ -4957,6 +4960,41 @@ int drm_mode_plane_set_obj_prop(struct drm_plane *plane,
>> >> >>  }
>> >> >>  EXPORT_SYMBOL(drm_mode_plane_set_obj_prop);
>> >> >>
>> >> >>+int drm_plane_create_max_width_prop(struct drm_plane *plane, uint32_t max_width)
>> >> >>+{
>> >> >>+   struct drm_property *prop;
>> >> >>+
>> >> >>+   prop = drm_property_create_range(plane->dev, DRM_MODE_PROP_IMMUTABLE,
>> >> >>+                                    "MAX_W", max_width, max_width);
>> >> >>+   if (!prop)
>> >> >>+           return -ENOMEM;
>> >> >>+
>> >> >>+   drm_object_attach_property(&plane->base, prop, max_width);
>> >> >>+
>> >> >>+   plane->max_width = max_width;
>> >> >>+
>> >> >>+   return 0;
>> >> >>+}
>> >> >>+EXPORT_SYMBOL(drm_plane_create_max_width_prop);
>> >> >>+
>> >> >>+int drm_plane_create_max_height_prop(struct drm_plane *plane,
>> >> >>+                                uint32_t max_height)
>> >> >>+{
>> >> >>+   struct drm_property *prop;
>> >> >>+
>> >> >>+   prop = drm_property_create_range(plane->dev, DRM_MODE_PROP_IMMUTABLE,
>> >> >>+                                    "MAX_H", max_height, max_height);
>> >> >>+   if (!prop)
>> >> >>+           return -ENOMEM;
>> >> >>+
>> >> >>+   drm_object_attach_property(&plane->base, prop, max_height);
>> >> >>+
>> >> >>+   plane->max_height = max_height;
>> >> >>+
>> >> >>+   return 0;
>> >> >>+}
>> >> >>+EXPORT_SYMBOL(drm_plane_create_max_height_prop);
>> >> >>+
>> >> >>  /**
>> >> >>   * drm_mode_obj_get_properties_ioctl - get the current value of a object's property
>> >> >>   * @dev: DRM device
>> >> >>diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
>> >> >>index e0170bf..6104527 100644
>> >> >>--- a/include/drm/drm_crtc.h
>> >> >>+++ b/include/drm/drm_crtc.h
>> >> >>@@ -1531,6 +1531,8 @@ struct drm_plane {
>> >> >>    uint32_t *format_types;
>> >> >>    unsigned int format_count;
>> >> >>    bool format_default;
>> >> >>+   uint32_t max_width;
>> >> >>+   uint32_t max_height;
>> >> >>
>> >> >>    struct drm_crtc *crtc;
>> >> >>    struct drm_framebuffer *fb;
>> >> >>@@ -2550,6 +2552,10 @@ extern struct drm_property *drm_mode_create_rotation_property(struct drm_device
>> >> >>                                                          unsigned int supported_rotations);
>> >> >>  extern unsigned int drm_rotation_simplify(unsigned int rotation,
>> >> >>                                      unsigned int supported_rotations);
>> >> >>+extern int drm_plane_create_max_width_prop(struct drm_plane *plane,
>> >> >>+                                      uint32_t max_width);
>> >> >>+extern int drm_plane_create_max_height_prop(struct drm_plane *plane,
>> >> >>+                                       uint32_t max_height);
>> >> >>
>> >> >>  /* Helpers */
>> >> >>
>> >> >>--
>> >> >>The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>> >> >>hosted by The Linux Foundation
>> >> >>
>> >> >
>> >>
>> >> --
>> >> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>> >> hosted by The Linux Foundation
>> >
>> > --
>> > Daniel Vetter
>> > Software Engineer, Intel Corporation
>> > http://blog.ffwll.ch
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Ville Syrjälä
> Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC] drm: Introduce max width and height properties for planes

[Daniel Vetter]

On Wed, May 25, 2016 at 7:57 PM, Rob Clark <robdclark@gmail.com> wrote:
>> As far as virtualizing the resources goes. I think that would need a
>> whole new API. Or at least a separate set of objects perhaps. I'm too
>> lazy to dig up all the old arguments now, but I'm pretty sure there
>> were many. If this would be done, I suspect the only sane way to do it
>> would be to just have a hwc implementation in the kernel. As in user
>> space would pass in the desired configuration, and the kernel would
>> assign resources as best it can and return the result back to userspace.
>
> actually, without introducing a completely new/different uapi, it
> should work pretty reasonably with the way weston atomic works,
> incrementally adding planes in TESTONLY steps until it fails.  I
> suspect on some more creative hw (msm is prime candidate) we'll still
> see custom hwc implementations to squeeze out the last bit of
> performance/power.  But with the approach that weston uses a generic
> userspace should still work pretty well on that hw.

So summary from my points on the m-l:
- Whether we do this in all drivers that need this for dual-pipe 4k,
or in all compositors is probably similar amounts of work. The upside
of doing it in drivers is that it's closer to the metal, avoid
midlayer-like mistakes in trying to funnel tons of stuff between the
top and bottom layers through drm core, and still failing in corner
cases. Drivers can simply fix things up if they're in control.

- For a bunch of reasons we (well Ville is really big on those) want
to add clipped_src/dst rectangles to drm_plane_state. That's a bit of
churn, but it's per-driver opt-in churn, so manageable. Once we have
clipped rectangles and so fully decoupled the hw state from the uabi
state it's easy to clip a plane to a sub-pipe instead of the entire
virtual area of the crtc.

- Once we can clip planes we can add a bit of helper magic to steal
additional&unused planes for use as the 2nd/3rd/4th plane scanning out
one logical surface (drm_buffer) attached to a logical plane
(drm_plane). Add a helper on top which splits a 4/5k screen at a
passed in limit, and this boils down to a few lines in drivers. While
still giving the drivers full control over all the details, if they
want to.

- Really assigning resources within the driver while exposing whether
there's enough to userspace using TEST_ONLY is the point of atomic.
Doesn't matter whether it's fifo space, scalers, wizzbang gizmo A, B
or C or this thing called "scanout engine". If your hw needs more than
one of those to get a basic job done (fullscreen primary plane) then
it's imo the drivers job to juggle all the internal resources around.
KMS is supposed to work from workstation gpus down to soc in a
somewhat uniform way, even on crappy hw.

Summary: Still firmly in the "virtualize it!" camp ;-)

Cheers, Daniel



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC] drm: Introduce max width and height properties for planes

[Archit Taneja]

On 05/25/2016 10:06 PM, Rob Clark wrote:
> On Wed, May 25, 2016 at 9:12 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
>> On Wed, May 25, 2016 at 04:28:36PM +0530, Archit Taneja wrote:
>>> On 05/25/2016 12:40 PM, Daniel Vetter wrote:
>>>> - Is the size/width really independent of e.g. rotation/pixel format/...
>>>>    Should it be the maximum that's possible under best circumstance (things
>>>>    could still fail), or the minimum that's guaranteed to work everwhere.
>>>>    This is the kind of stuff we need the userspace part for, too.
>>>
>>> Yeah, it isn't independent of these parameters. I'm not entirely sure
>>> about this either.
>>>
>>> Does it make sense to impose a rule that the user first sets the
>>> rotation/format plane properties, and only then read the maximum
>>> width? I'm assuming user space hates such kind of stuff.
>>>
>>> If we use the 'best circumstance' max_width, we can first start
>>> with a minimum number of planes that need to be grouped to achieve
>>> the target mode. If that fails the atomic test, then we can try to
>>> add one plane at a time, and reduce the width for each plane.
>>>
>>> If we use the minimum/'guaranteed to work' max_width, we'll get
>>> a higher number of planes than needed for this mode. This would pass
>>> the atomic test. We could then reduce a plane at a time and see when
>>> we fail the atomic test.
>>>
>>> I guess we need to chose the one that's more probable to get right
>>> the first time. Considering only pixel formats for now, the
>>> minimum/'guaranteed to work' would map to the RGB formats. The best
>>> circumstance ones would probably be the planar video formats. Since
>>> we use RGB more often, the minimum one might make more sense.
>>>
>>> We could, of course, give the property a range of max widths to
>>> confuse user space even more.
>>
>> An entirely different idea for cases where a simple hint property doesn't
>> work (other ideas floating around are can_scale, to give a hint whether a
>> plan can at least in theory up/downscale, or not at all), is that the
>> kernel gives more specific hints about what it would like to change.
>>
>> So if userspace asks for a plane, but for the given pixel format it's too
>> wide, the kernel could return a new proposed value for width. That would
>> be super-flexible and could cover all kinds of use-case like rotation
>> needing a specific tiling (fb_modifier) or specific pixel format, or
>> specific stride.

This would be great to have, but it sounds like a new class of ABI
altogether, where you set a configuration, the kernel rejects it, but
gives a hint about what it wants. Do we already have something in DRI
that follows such a mechanism?

>>
>> For the case at hand there's even more worms: What about stride
>> requirements? Afaik on some hw you just need to split the buffers into 2
>> planes, but can keep the wide stride (since the limit is the size of the
>> linebuffers in the hw). On others you need to split the buffer itself into
>> 2, because the plane hw can't cope with huge strides. Again might depend
>> upon the pixel format.

I assumed that hw always belonged to the first category. For the
latter, the userspace strategy of allocating buffers would have to
change itself.

If we do decide to hide pipe virtualization in the kernel as you
mentioned below, how would we manage plane hw that can't do huge
strides. Would we need to copy the userspace populated buffer into
two separate kernel buffers that fit the stride requirements?

>>
>> So in a way height/width is both too much information and not precise
>> enough. Entirely different approches:
>> - We just add a might_need_split_plane prop to crtcs where this might be
>>    needed. Userspace then gets to retry with split buffers if it doesn't
>>    work with a huge one.
>>
>> - When I discussed this with qualcom folks for msm we concluded that the
>>    simplest approach would be to hide this in the kernel. So if you have a
>>    too wide plane, and need 2 hw planes to scan it out, then do that
>>    transparently in the kernel. Of course this means that there will be 1
>>    (or 3 if you need a 2x2 split) fewer planes available, but userspace
>>    needs to iteratively build up the plane config using ATOMIC_TEST anyway.
>
> Just fwiw, there are a few things that we will still end up
> abstracting in the kernel by virtualizing the mapping between kms
> planes and hw pipes.  And the approach of weston atomic of
> incrementally adding more planes w/ TESTONLY flag should work well for
> that.  (Let's hope the weston bits get upstream some day..)
>
> But exposing width limit avoids the one-plane to multiple-pipes case,
> considerably simplifying things.  And seemed like a generic enough
> limit (iirc, it applies to omapdss and probably others), that it would
> be cleaner to expose the limit to userspace.  So there should be at
> least a couple other drivers that could avoid virtualizing planes with
> some help from userspace for this case.
>
> Regarding rotation, I'm not 100% sure.. seems like we could just
> document these as the un-rotated limits.  If we really had to, we
> could do some sort of dance where userspace sets rotation property on
> an un-used plane, and then reads-back the current values of the
> read-only prop's.  But that seems awkward.
>
> BR,
> -R
>
>> If possible for your hw I'm heavily leaning towards this last approach. If
>> fits entirely into the current atomic design, and all the complexity is
>> restricted to your driver (you need to have some allocation map between
>> drm planes and real hw planes, but that's it).

I guess the last approach seems more apt for compositors that run the
same code for all hw (weston, etc). In android userspace, each display
hw has the luxury of having it's own middleware where they have more 
freedom on how they manage planes and how they interpret the DRM
ioctls. For such middleware, preventing virtualization helps in moving
the bulk of the code in userspace. I don't know if android will
continue to have such middleware when all SoCs move to drm hwcomposer,
though.

This probably needs more feedback Qcom and other SoC people. Although,
as Rob said, if a lot of hw has a limitation over the line buffer width
they have on a plane, it could be handy to have such expose such a
property for drivers that don't want to virtualize their plane hw just
for this one usecase. But, as you said, max_width isn't precise enough
information either. So, yeah, we'll probably need to think over this
a bit more.

Archit

>>
>> Thoughts?
>> -Daniel
>>
>>>
>>
>>> Archit
>>>
>>>>
>>>> Cheers, Daniel
>>>>
>>>>> Signed-off-by: Archit Taneja <architt@codeaurora.org>
>>>>> ---
>>>>>   drivers/gpu/drm/drm_atomic.c |  9 +++++++++
>>>>>   drivers/gpu/drm/drm_crtc.c   | 38 ++++++++++++++++++++++++++++++++++++++
>>>>>   include/drm/drm_crtc.h       |  6 ++++++
>>>>>   3 files changed, 53 insertions(+)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>>>>> index 8ee1db8..fded22a 100644
>>>>> --- a/drivers/gpu/drm/drm_atomic.c
>>>>> +++ b/drivers/gpu/drm/drm_atomic.c
>>>>> @@ -839,6 +839,15 @@ static int drm_atomic_plane_check(struct drm_plane *plane,
>>>>>             return -ERANGE;
>>>>>     }
>>>>>
>>>>> +   if (plane->max_width && (state->src_w >> 16) > plane->max_width ||
>>>>> +       plane->max_height && (state->src_h >> 16) > plane->max_height) {
>>>>> +           DRM_DEBUG_ATOMIC("Invalid source width/height "
>>>>> +                            "%u.%06ux%u.%06u\n",
>>>>> +                            state->src_w >> 16, ((state->src_w & 0xffff) * 15625) >> 10,
>>>>> +                            state->src_h >> 16, ((state->src_h & 0xffff) * 15625) >> 10);
>>>>> +           return -ERANGE;
>>>>> +   }
>>>>> +
>>>>>     fb_width = state->fb->width << 16;
>>>>>     fb_height = state->fb->height << 16;
>>>>>
>>>>> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
>>>>> index e08f962..f2d3b92 100644
>>>>> --- a/drivers/gpu/drm/drm_crtc.c
>>>>> +++ b/drivers/gpu/drm/drm_crtc.c
>>>>> @@ -1267,6 +1267,9 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
>>>>>     plane->possible_crtcs = possible_crtcs;
>>>>>     plane->type = type;
>>>>>
>>>>> +   plane->max_width = 0;
>>>>> +   plane->max_height = 0;
>>>>> +
>>>>>     list_add_tail(&plane->head, &config->plane_list);
>>>>>     config->num_total_plane++;
>>>>>     if (plane->type == DRM_PLANE_TYPE_OVERLAY)
>>>>> @@ -4957,6 +4960,41 @@ int drm_mode_plane_set_obj_prop(struct drm_plane *plane,
>>>>>   }
>>>>>   EXPORT_SYMBOL(drm_mode_plane_set_obj_prop);
>>>>>
>>>>> +int drm_plane_create_max_width_prop(struct drm_plane *plane, uint32_t max_width)
>>>>> +{
>>>>> +   struct drm_property *prop;
>>>>> +
>>>>> +   prop = drm_property_create_range(plane->dev, DRM_MODE_PROP_IMMUTABLE,
>>>>> +                                    "MAX_W", max_width, max_width);
>>>>> +   if (!prop)
>>>>> +           return -ENOMEM;
>>>>> +
>>>>> +   drm_object_attach_property(&plane->base, prop, max_width);
>>>>> +
>>>>> +   plane->max_width = max_width;
>>>>> +
>>>>> +   return 0;
>>>>> +}
>>>>> +EXPORT_SYMBOL(drm_plane_create_max_width_prop);
>>>>> +
>>>>> +int drm_plane_create_max_height_prop(struct drm_plane *plane,
>>>>> +                                uint32_t max_height)
>>>>> +{
>>>>> +   struct drm_property *prop;
>>>>> +
>>>>> +   prop = drm_property_create_range(plane->dev, DRM_MODE_PROP_IMMUTABLE,
>>>>> +                                    "MAX_H", max_height, max_height);
>>>>> +   if (!prop)
>>>>> +           return -ENOMEM;
>>>>> +
>>>>> +   drm_object_attach_property(&plane->base, prop, max_height);
>>>>> +
>>>>> +   plane->max_height = max_height;
>>>>> +
>>>>> +   return 0;
>>>>> +}
>>>>> +EXPORT_SYMBOL(drm_plane_create_max_height_prop);
>>>>> +
>>>>>   /**
>>>>>    * drm_mode_obj_get_properties_ioctl - get the current value of a object's property
>>>>>    * @dev: DRM device
>>>>> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
>>>>> index e0170bf..6104527 100644
>>>>> --- a/include/drm/drm_crtc.h
>>>>> +++ b/include/drm/drm_crtc.h
>>>>> @@ -1531,6 +1531,8 @@ struct drm_plane {
>>>>>     uint32_t *format_types;
>>>>>     unsigned int format_count;
>>>>>     bool format_default;
>>>>> +   uint32_t max_width;
>>>>> +   uint32_t max_height;
>>>>>
>>>>>     struct drm_crtc *crtc;
>>>>>     struct drm_framebuffer *fb;
>>>>> @@ -2550,6 +2552,10 @@ extern struct drm_property *drm_mode_create_rotation_property(struct drm_device
>>>>>                                                           unsigned int supported_rotations);
>>>>>   extern unsigned int drm_rotation_simplify(unsigned int rotation,
>>>>>                                       unsigned int supported_rotations);
>>>>> +extern int drm_plane_create_max_width_prop(struct drm_plane *plane,
>>>>> +                                      uint32_t max_width);
>>>>> +extern int drm_plane_create_max_height_prop(struct drm_plane *plane,
>>>>> +                                       uint32_t max_height);
>>>>>
>>>>>   /* Helpers */
>>>>>
>>>>> --
>>>>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>>>>> hosted by The Linux Foundation
>>>>>
>>>>
>>>
>>> --
>>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>>> hosted by The Linux Foundation
>>
>> --
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> http://blog.ffwll.ch

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum, hosted by The Linux Foundation
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC] drm: Introduce max width and height properties for planes

[Daniel Vetter]

On Mon, May 30, 2016 at 02:09:17PM +0530, Archit Taneja wrote:
> 
> 
> On 05/25/2016 10:06 PM, Rob Clark wrote:
> >On Wed, May 25, 2016 at 9:12 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> >>On Wed, May 25, 2016 at 04:28:36PM +0530, Archit Taneja wrote:
> >>>On 05/25/2016 12:40 PM, Daniel Vetter wrote:
> >>>>- Is the size/width really independent of e.g. rotation/pixel format/...
> >>>>   Should it be the maximum that's possible under best circumstance (things
> >>>>   could still fail), or the minimum that's guaranteed to work everwhere.
> >>>>   This is the kind of stuff we need the userspace part for, too.
> >>>
> >>>Yeah, it isn't independent of these parameters. I'm not entirely sure
> >>>about this either.
> >>>
> >>>Does it make sense to impose a rule that the user first sets the
> >>>rotation/format plane properties, and only then read the maximum
> >>>width? I'm assuming user space hates such kind of stuff.
> >>>
> >>>If we use the 'best circumstance' max_width, we can first start
> >>>with a minimum number of planes that need to be grouped to achieve
> >>>the target mode. If that fails the atomic test, then we can try to
> >>>add one plane at a time, and reduce the width for each plane.
> >>>
> >>>If we use the minimum/'guaranteed to work' max_width, we'll get
> >>>a higher number of planes than needed for this mode. This would pass
> >>>the atomic test. We could then reduce a plane at a time and see when
> >>>we fail the atomic test.
> >>>
> >>>I guess we need to chose the one that's more probable to get right
> >>>the first time. Considering only pixel formats for now, the
> >>>minimum/'guaranteed to work' would map to the RGB formats. The best
> >>>circumstance ones would probably be the planar video formats. Since
> >>>we use RGB more often, the minimum one might make more sense.
> >>>
> >>>We could, of course, give the property a range of max widths to
> >>>confuse user space even more.
> >>
> >>An entirely different idea for cases where a simple hint property doesn't
> >>work (other ideas floating around are can_scale, to give a hint whether a
> >>plan can at least in theory up/downscale, or not at all), is that the
> >>kernel gives more specific hints about what it would like to change.
> >>
> >>So if userspace asks for a plane, but for the given pixel format it's too
> >>wide, the kernel could return a new proposed value for width. That would
> >>be super-flexible and could cover all kinds of use-case like rotation
> >>needing a specific tiling (fb_modifier) or specific pixel format, or
> >>specific stride.
> 
> This would be great to have, but it sounds like a new class of ABI
> altogether, where you set a configuration, the kernel rejects it, but
> gives a hint about what it wants. Do we already have something in DRI
> that follows such a mechanism?

Yeah, we definitely don't want to start out with that.

> >>For the case at hand there's even more worms: What about stride
> >>requirements? Afaik on some hw you just need to split the buffers into 2
> >>planes, but can keep the wide stride (since the limit is the size of the
> >>linebuffers in the hw). On others you need to split the buffer itself into
> >>2, because the plane hw can't cope with huge strides. Again might depend
> >>upon the pixel format.
> 
> I assumed that hw always belonged to the first category. For the
> latter, the userspace strategy of allocating buffers would have to
> change itself.
> 
> If we do decide to hide pipe virtualization in the kernel as you
> mentioned below, how would we manage plane hw that can't do huge
> strides. Would we need to copy the userspace populated buffer into
> two separate kernel buffers that fit the stride requirements?

I think at that point the kernel needs to make sure that the preferred
mode is _not_ a mode that requires such a stride. And then userspace needs
to piece stuff together on it's own. This is hw-mis-design on a level that
I don't think makes much sense to abstract away ;-)

But afaik the discussion here is just for hw where we might need 2 or 4
planes to scan out one logical buffer.

> >>So in a way height/width is both too much information and not precise
> >>enough. Entirely different approches:
> >>- We just add a might_need_split_plane prop to crtcs where this might be
> >>   needed. Userspace then gets to retry with split buffers if it doesn't
> >>   work with a huge one.
> >>
> >>- When I discussed this with qualcom folks for msm we concluded that the
> >>   simplest approach would be to hide this in the kernel. So if you have a
> >>   too wide plane, and need 2 hw planes to scan it out, then do that
> >>   transparently in the kernel. Of course this means that there will be 1
> >>   (or 3 if you need a 2x2 split) fewer planes available, but userspace
> >>   needs to iteratively build up the plane config using ATOMIC_TEST anyway.
> >
> >Just fwiw, there are a few things that we will still end up
> >abstracting in the kernel by virtualizing the mapping between kms
> >planes and hw pipes.  And the approach of weston atomic of
> >incrementally adding more planes w/ TESTONLY flag should work well for
> >that.  (Let's hope the weston bits get upstream some day..)
> >
> >But exposing width limit avoids the one-plane to multiple-pipes case,
> >considerably simplifying things.  And seemed like a generic enough
> >limit (iirc, it applies to omapdss and probably others), that it would
> >be cleaner to expose the limit to userspace.  So there should be at
> >least a couple other drivers that could avoid virtualizing planes with
> >some help from userspace for this case.
> >
> >Regarding rotation, I'm not 100% sure.. seems like we could just
> >document these as the un-rotated limits.  If we really had to, we
> >could do some sort of dance where userspace sets rotation property on
> >an un-used plane, and then reads-back the current values of the
> >read-only prop's.  But that seems awkward.
> >
> >BR,
> >-R
> >
> >>If possible for your hw I'm heavily leaning towards this last approach. If
> >>fits entirely into the current atomic design, and all the complexity is
> >>restricted to your driver (you need to have some allocation map between
> >>drm planes and real hw planes, but that's it).
> 
> I guess the last approach seems more apt for compositors that run the
> same code for all hw (weston, etc). In android userspace, each display
> hw has the luxury of having it's own middleware where they have more freedom
> on how they manage planes and how they interpret the DRM
> ioctls. For such middleware, preventing virtualization helps in moving
> the bulk of the code in userspace. I don't know if android will
> continue to have such middleware when all SoCs move to drm hwcomposer,
> though.

drm_hwcomposer (developed by Google) is a generic hwc implementation along
the lines of weston and any other generic kms compositor. I think it makes
a lot of sense to aim for a reasonable baseline API which works the same
way everywhere. And that reasonable baseline abi imo includes that at
least 1 plane can be used full-screen, without jumping through hooks.

Because if you don't have that, then it's not just weston and android you
need to patch up. But also
- fbdev
- any boot splash
- any igt testcases (I'm starting to push hard to make those a requirement
  for kms drivers)
- X
- any other compositor, like mutter, kwin, ozone, ...

At that point we're not doing a real generic platform any more, but fully
leaking all the details into userspace (like hwc does on Android). That's
not terribly useful.

Imo the rule should be that special code can only be required by userspace
for optimizations (more power efficient or whatever), not for basic
functionality. Displaying a boot splash is _very_ basic ;-)

> This probably needs more feedback Qcom and other SoC people. Although,
> as Rob said, if a lot of hw has a limitation over the line buffer width
> they have on a plane, it could be handy to have such expose such a
> property for drivers that don't want to virtualize their plane hw just
> for this one usecase. But, as you said, max_width isn't precise enough
> information either. So, yeah, we'll probably need to think over this
> a bit more.

I massively disagree with "this one usecase". If you can't do all the
things above, there's no reason at all to have a kms driver imo. At least
not in upstream. If it only works on that one vendor tree with the one
vendor hwc and no where else, I don't think it makes sense to have it in
upstream.

But like discussed with Rob, I think it should be pretty easy to have a
set of generic helpers in-kernel to make this virtualization really easy.
In the end you want to share the code between different compositors
anyway, and the best place to put such shared code is within the kernel
itself imo. Of course for such a helper we'd need to have 2-3 drivers
using it, to make sure it's good enough.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC] drm: Introduce max width and height properties for planes

[Rob Clark]

On Mon, May 30, 2016 at 5:32 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Mon, May 30, 2016 at 02:09:17PM +0530, Archit Taneja wrote:
>>
>>
>> On 05/25/2016 10:06 PM, Rob Clark wrote:
>> >On Wed, May 25, 2016 at 9:12 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
>> >>On Wed, May 25, 2016 at 04:28:36PM +0530, Archit Taneja wrote:
>> >>>On 05/25/2016 12:40 PM, Daniel Vetter wrote:
>> >>>>- Is the size/width really independent of e.g. rotation/pixel format/...
>> >>>>   Should it be the maximum that's possible under best circumstance (things
>> >>>>   could still fail), or the minimum that's guaranteed to work everwhere.
>> >>>>   This is the kind of stuff we need the userspace part for, too.
>> >>>
>> >>>Yeah, it isn't independent of these parameters. I'm not entirely sure
>> >>>about this either.
>> >>>
>> >>>Does it make sense to impose a rule that the user first sets the
>> >>>rotation/format plane properties, and only then read the maximum
>> >>>width? I'm assuming user space hates such kind of stuff.
>> >>>
>> >>>If we use the 'best circumstance' max_width, we can first start
>> >>>with a minimum number of planes that need to be grouped to achieve
>> >>>the target mode. If that fails the atomic test, then we can try to
>> >>>add one plane at a time, and reduce the width for each plane.
>> >>>
>> >>>If we use the minimum/'guaranteed to work' max_width, we'll get
>> >>>a higher number of planes than needed for this mode. This would pass
>> >>>the atomic test. We could then reduce a plane at a time and see when
>> >>>we fail the atomic test.
>> >>>
>> >>>I guess we need to chose the one that's more probable to get right
>> >>>the first time. Considering only pixel formats for now, the
>> >>>minimum/'guaranteed to work' would map to the RGB formats. The best
>> >>>circumstance ones would probably be the planar video formats. Since
>> >>>we use RGB more often, the minimum one might make more sense.
>> >>>
>> >>>We could, of course, give the property a range of max widths to
>> >>>confuse user space even more.
>> >>
>> >>An entirely different idea for cases where a simple hint property doesn't
>> >>work (other ideas floating around are can_scale, to give a hint whether a
>> >>plan can at least in theory up/downscale, or not at all), is that the
>> >>kernel gives more specific hints about what it would like to change.
>> >>
>> >>So if userspace asks for a plane, but for the given pixel format it's too
>> >>wide, the kernel could return a new proposed value for width. That would
>> >>be super-flexible and could cover all kinds of use-case like rotation
>> >>needing a specific tiling (fb_modifier) or specific pixel format, or
>> >>specific stride.
>>
>> This would be great to have, but it sounds like a new class of ABI
>> altogether, where you set a configuration, the kernel rejects it, but
>> gives a hint about what it wants. Do we already have something in DRI
>> that follows such a mechanism?
>
> Yeah, we definitely don't want to start out with that.
>
>> >>For the case at hand there's even more worms: What about stride
>> >>requirements? Afaik on some hw you just need to split the buffers into 2
>> >>planes, but can keep the wide stride (since the limit is the size of the
>> >>linebuffers in the hw). On others you need to split the buffer itself into
>> >>2, because the plane hw can't cope with huge strides. Again might depend
>> >>upon the pixel format.
>>
>> I assumed that hw always belonged to the first category. For the
>> latter, the userspace strategy of allocating buffers would have to
>> change itself.
>>
>> If we do decide to hide pipe virtualization in the kernel as you
>> mentioned below, how would we manage plane hw that can't do huge
>> strides. Would we need to copy the userspace populated buffer into
>> two separate kernel buffers that fit the stride requirements?
>
> I think at that point the kernel needs to make sure that the preferred
> mode is _not_ a mode that requires such a stride. And then userspace needs
> to piece stuff together on it's own. This is hw-mis-design on a level that
> I don't think makes much sense to abstract away ;-)
>
> But afaik the discussion here is just for hw where we might need 2 or 4
> planes to scan out one logical buffer.

right, if you need userspace to composite split buffers, you've already lost..

>> >>So in a way height/width is both too much information and not precise
>> >>enough. Entirely different approches:
>> >>- We just add a might_need_split_plane prop to crtcs where this might be
>> >>   needed. Userspace then gets to retry with split buffers if it doesn't
>> >>   work with a huge one.
>> >>
>> >>- When I discussed this with qualcom folks for msm we concluded that the
>> >>   simplest approach would be to hide this in the kernel. So if you have a
>> >>   too wide plane, and need 2 hw planes to scan it out, then do that
>> >>   transparently in the kernel. Of course this means that there will be 1
>> >>   (or 3 if you need a 2x2 split) fewer planes available, but userspace
>> >>   needs to iteratively build up the plane config using ATOMIC_TEST anyway.
>> >
>> >Just fwiw, there are a few things that we will still end up
>> >abstracting in the kernel by virtualizing the mapping between kms
>> >planes and hw pipes.  And the approach of weston atomic of
>> >incrementally adding more planes w/ TESTONLY flag should work well for
>> >that.  (Let's hope the weston bits get upstream some day..)
>> >
>> >But exposing width limit avoids the one-plane to multiple-pipes case,
>> >considerably simplifying things.  And seemed like a generic enough
>> >limit (iirc, it applies to omapdss and probably others), that it would
>> >be cleaner to expose the limit to userspace.  So there should be at
>> >least a couple other drivers that could avoid virtualizing planes with
>> >some help from userspace for this case.
>> >
>> >Regarding rotation, I'm not 100% sure.. seems like we could just
>> >document these as the un-rotated limits.  If we really had to, we
>> >could do some sort of dance where userspace sets rotation property on
>> >an un-used plane, and then reads-back the current values of the
>> >read-only prop's.  But that seems awkward.
>> >
>> >BR,
>> >-R
>> >
>> >>If possible for your hw I'm heavily leaning towards this last approach. If
>> >>fits entirely into the current atomic design, and all the complexity is
>> >>restricted to your driver (you need to have some allocation map between
>> >>drm planes and real hw planes, but that's it).
>>
>> I guess the last approach seems more apt for compositors that run the
>> same code for all hw (weston, etc). In android userspace, each display
>> hw has the luxury of having it's own middleware where they have more freedom
>> on how they manage planes and how they interpret the DRM
>> ioctls. For such middleware, preventing virtualization helps in moving
>> the bulk of the code in userspace. I don't know if android will
>> continue to have such middleware when all SoCs move to drm hwcomposer,
>> though.
>
> drm_hwcomposer (developed by Google) is a generic hwc implementation along
> the lines of weston and any other generic kms compositor. I think it makes
> a lot of sense to aim for a reasonable baseline API which works the same
> way everywhere. And that reasonable baseline abi imo includes that at
> least 1 plane can be used full-screen, without jumping through hooks.
>
> Because if you don't have that, then it's not just weston and android you
> need to patch up. But also
> - fbdev
> - any boot splash
> - any igt testcases (I'm starting to push hard to make those a requirement
>   for kms drivers)
> - X
> - any other compositor, like mutter, kwin, ozone, ...

I think it's not so bad.  We fix the setcrtc helpers to do this for
legacy crtc path (which should pretty much cover most existing
compositors, X, splash, fbdev, etc).  After all they already pick one
plane, it shouldn't be too hard to make them pick two.

But for atomic, we make userspace deal with it, since things get more
complicated than what setcrtc/pageflip has to deal with.  Afaiu there
are basically two atomic compositors (and the weston one isn't even
merged), so I think dealing with this in userspace is not so bad.

BR,
-R

> At that point we're not doing a real generic platform any more, but fully
> leaking all the details into userspace (like hwc does on Android). That's
> not terribly useful.
>
> Imo the rule should be that special code can only be required by userspace
> for optimizations (more power efficient or whatever), not for basic
> functionality. Displaying a boot splash is _very_ basic ;-)
>
>> This probably needs more feedback Qcom and other SoC people. Although,
>> as Rob said, if a lot of hw has a limitation over the line buffer width
>> they have on a plane, it could be handy to have such expose such a
>> property for drivers that don't want to virtualize their plane hw just
>> for this one usecase. But, as you said, max_width isn't precise enough
>> information either. So, yeah, we'll probably need to think over this
>> a bit more.
>
> I massively disagree with "this one usecase". If you can't do all the
> things above, there's no reason at all to have a kms driver imo. At least
> not in upstream. If it only works on that one vendor tree with the one
> vendor hwc and no where else, I don't think it makes sense to have it in
> upstream.
>
> But like discussed with Rob, I think it should be pretty easy to have a
> set of generic helpers in-kernel to make this virtualization really easy.
> In the end you want to share the code between different compositors
> anyway, and the best place to put such shared code is within the kernel
> itself imo. Of course for such a helper we'd need to have 2-3 drivers
> using it, to make sure it's good enough.
>
> Cheers, Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC] drm: Introduce max width and height properties for planes

[Daniel Vetter]

On Mon, May 30, 2016 at 09:47:53AM -0400, Rob Clark wrote:
> On Mon, May 30, 2016 at 5:32 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > drm_hwcomposer (developed by Google) is a generic hwc implementation along
> > the lines of weston and any other generic kms compositor. I think it makes
> > a lot of sense to aim for a reasonable baseline API which works the same
> > way everywhere. And that reasonable baseline abi imo includes that at
> > least 1 plane can be used full-screen, without jumping through hooks.
> >
> > Because if you don't have that, then it's not just weston and android you
> > need to patch up. But also
> > - fbdev
> > - any boot splash
> > - any igt testcases (I'm starting to push hard to make those a requirement
> >   for kms drivers)
> > - X
> > - any other compositor, like mutter, kwin, ozone, ...
> 
> I think it's not so bad.  We fix the setcrtc helpers to do this for
> legacy crtc path (which should pretty much cover most existing
> compositors, X, splash, fbdev, etc).  After all they already pick one
> plane, it shouldn't be too hard to make them pick two.
> 
> But for atomic, we make userspace deal with it, since things get more
> complicated than what setcrtc/pageflip has to deal with.  Afaiu there
> are basically two atomic compositors (and the weston one isn't even
> merged), so I think dealing with this in userspace is not so bad.

What exactly is more complicated with doing this for full atomic? If you
do it in userspace for setcrtc, plus fbdev, plus
weston/ozone/drm_hwcomposer/ and maybe more, then that's already an
impressive pile of copies of that logic. And I really think it should be
pretty simple to type this up into a helper that works on any plane.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC] drm: Introduce max width and height properties for planes

[Rob Clark]

On Mon, May 30, 2016 at 10:38 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Mon, May 30, 2016 at 09:47:53AM -0400, Rob Clark wrote:
>> On Mon, May 30, 2016 at 5:32 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
>> > drm_hwcomposer (developed by Google) is a generic hwc implementation along
>> > the lines of weston and any other generic kms compositor. I think it makes
>> > a lot of sense to aim for a reasonable baseline API which works the same
>> > way everywhere. And that reasonable baseline abi imo includes that at
>> > least 1 plane can be used full-screen, without jumping through hooks.
>> >
>> > Because if you don't have that, then it's not just weston and android you
>> > need to patch up. But also
>> > - fbdev
>> > - any boot splash
>> > - any igt testcases (I'm starting to push hard to make those a requirement
>> >   for kms drivers)
>> > - X
>> > - any other compositor, like mutter, kwin, ozone, ...
>>
>> I think it's not so bad.  We fix the setcrtc helpers to do this for
>> legacy crtc path (which should pretty much cover most existing
>> compositors, X, splash, fbdev, etc).  After all they already pick one
>> plane, it shouldn't be too hard to make them pick two.
>>
>> But for atomic, we make userspace deal with it, since things get more
>> complicated than what setcrtc/pageflip has to deal with.  Afaiu there
>> are basically two atomic compositors (and the weston one isn't even
>> merged), so I think dealing with this in userspace is not so bad.
>
> What exactly is more complicated with doing this for full atomic? If you
> do it in userspace for setcrtc, plus fbdev, plus

s/userspace/kernel/ (for setcrtc/pageflip)

> weston/ozone/drm_hwcomposer/ and maybe more, then that's already an
> impressive pile of copies of that logic. And I really think it should be
> pretty simple to type this up into a helper that works on any plane.

For legacy setcrtc/pageflip, I think we just change crtc->primary into
an array of N (where N is worst case # of planes).  It is a scenario
where you don't have to care about scaling, or really other planes in
use, and a static association of planes to crtc's should be fine.  And
we wouldn't expose the additional primaries to userspace so no
conflicts to worry about..

Seems like a fairly small change in the atomic helpers, vs having to
invent virtual planes mapping to physical planes, etc.

BR,
-R

> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel