Re: [Intel-gfx] [PATCH 1/3] drm: Add new DRM_IOCTL_MODE_GETPLANE2

2023-11-24 Thread Andy Shevchenko 
On Thu, Jan 12, 2017 at 12:23:16PM +0200, Ville Syrjälä wrote:
> On Wed, Jan 11, 2017 at 04:51:16PM -0800, Ben Widawsky wrote:

...

> > +   memcpy(plane->modifiers, format_modifiers,
> > +  format_modifier_count * sizeof(format_modifiers[0]));
> 
> Looks all right since we do the same for formats anyway. But it did
> occur to me (twice at least) that a kmemdup_array() might a nice thing
> to have for things like this. But that's a separate topic.

JFYI:
https://lore.kernel.org/all/20231017052322.2636-2-kkar...@nvidia.com/

-- 
With Best Regards,
Andy Shevchenko

Re: [Intel-gfx] [PATCH 1/3] drm: Add new DRM_IOCTL_MODE_GETPLANE2

2017-01-13 Thread Daniel Stone 
Hey,

On 13 January 2017 at 09:37, Ville Syrjälä
 wrote:
> On Thu, Jan 12, 2017 at 07:27:03PM +, Daniel Stone wrote:
>> It would make sense, but then gbm_surface_create_with_modifiers takes
>> a fixed pixel format and a list of acceptable modifiers (which to me
>> seems like the right way around as an API), so whenever I was creating
>> a surface, I'd have to walk through and create a new list, flipped
>> back the other way.
>
> Yeah, for that your original order makes more sense, even if it
> potentially uses more memory.

Given the size of framebuffers, I'm really not concerned by the
in-memory size of a format list. :)

Cheers,
Daniel
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH 1/3] drm: Add new DRM_IOCTL_MODE_GETPLANE2

2017-01-13 Thread Ville Syrjälä 
On Thu, Jan 12, 2017 at 07:27:03PM +, Daniel Stone wrote:
> Hi,
> 
> On 12 January 2017 at 18:11, Ville Syrjälä
>  wrote:
> > On Thu, Jan 12, 2017 at 05:50:15PM +, Daniel Stone wrote:
> >> struct drm_plane {
> >> struct {
> >> uint32_t format;
> >> uint64_t modifiers[];
> >> } formats[];
> >> }
> >
> > Flipping formats[] vs. modifiers[] here would seem like it should make
> > this easier with the proposed kernel API. And if the kernel will also
> > uarantee that multiple instances of the same modifier must be returned
> > contiguously, then it should be even easier.
> >
> > Oh and flipping formats[] and modifiers[] should also save a quite a
> > bit of space since each format takes twice as much space as each
> > modifier. But I suppose that might come at a runtime cost if you have
> > to look for a specific format in each modifier's format list instead
> > of having to look at just the modifier list of a specific format. So
> > I suppose not flipping might be better after all, which I guess would
> > complicate populating the infromation somewhat.
> >
> > Anyways, that's all a bit unrelated to the matter at hand, so I'll stop
> > now and just state that I don't mind having an explicit offset if
> > people really want it.
> 
> It would make sense, but then gbm_surface_create_with_modifiers takes
> a fixed pixel format and a list of acceptable modifiers (which to me
> seems like the right way around as an API), so whenever I was creating
> a surface, I'd have to walk through and create a new list, flipped
> back the other way.

Yeah, for that your original order makes more sense, even if it
potentially uses more memory.

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

Re: [Intel-gfx] [PATCH 1/3] drm: Add new DRM_IOCTL_MODE_GETPLANE2

2017-01-12 Thread Daniel Stone 
Hi,

On 12 January 2017 at 18:11, Ville Syrjälä
 wrote:
> On Thu, Jan 12, 2017 at 05:50:15PM +, Daniel Stone wrote:
>> struct drm_plane {
>> struct {
>> uint32_t format;
>> uint64_t modifiers[];
>> } formats[];
>> }
>
> Flipping formats[] vs. modifiers[] here would seem like it should make
> this easier with the proposed kernel API. And if the kernel will also
> uarantee that multiple instances of the same modifier must be returned
> contiguously, then it should be even easier.
>
> Oh and flipping formats[] and modifiers[] should also save a quite a
> bit of space since each format takes twice as much space as each
> modifier. But I suppose that might come at a runtime cost if you have
> to look for a specific format in each modifier's format list instead
> of having to look at just the modifier list of a specific format. So
> I suppose not flipping might be better after all, which I guess would
> complicate populating the infromation somewhat.
>
> Anyways, that's all a bit unrelated to the matter at hand, so I'll stop
> now and just state that I don't mind having an explicit offset if
> people really want it.

It would make sense, but then gbm_surface_create_with_modifiers takes
a fixed pixel format and a list of acceptable modifiers (which to me
seems like the right way around as an API), so whenever I was creating
a surface, I'd have to walk through and create a new list, flipped
back the other way.

Cheers,
Daniel
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH 1/3] drm: Add new DRM_IOCTL_MODE_GETPLANE2

2017-01-12 Thread Ville Syrjälä 
On Thu, Jan 12, 2017 at 05:50:15PM +, Daniel Stone wrote:
> Hi,
> 
> On 12 January 2017 at 17:45, Ville Syrjälä
>  wrote:
> > On Thu, Jan 12, 2017 at 05:04:46PM +, Daniel Stone wrote:
> >> Implicit is clever but horrible. AFAICT, the only way to do it
> >> properly would be to have a nested forwards loop walk when you first
> >> hit a modifier, searching for further occurrences of that modifier to
> >> collect the complete set of formats that modifier applies to.
> >
> > Not sure for what that is the "only way". In fact I can't right now
> > think of any operation that would require an extra loop necessarily.
> > For some things you might just want to look for a specific
> > format+modifier combo, for that it doesn't matter how many blocks there
> > are.
> 
> Right, that just needs a local variable to act as a counter.
> 
> > And if you want to transform the reply into some less convoluted
> > form, well then you'd just need some modifier+dynamic format list thing,
> > or if you want to keep to bitmasks you'd either need a bitmask that can
> > grow when running out of bits or just make it big enough to handle a
> > sufficiently large worst case number of bits.
> >
> > Dunno, maybe I just lack imagination. Then again, I'm not even sure if
> > we're talking about userspace of kernel code here, which might explain
> > my general confusion :)
> 
> I'm talking about userspace, where I want to have:
> struct drm_plane {
> struct {
> uint32_t format;
> uint64_t modifiers[];
> } formats[];
> }

Flipping formats[] vs. modifiers[] here would seem like it should make
this easier with the proposed kernel API. And if the kernel will also
uarantee that multiple instances of the same modifier must be returned
contiguously, then it should be even easier.

Oh and flipping formats[] and modifiers[] should also save a quite a
bit of space since each format takes twice as much space as each
modifier. But I suppose that might come at a runtime cost if you have
to look for a specific format in each modifier's format list instead
of having to look at just the modifier list of a specific format. So
I suppose not flipping might be better after all, which I guess would
complicate populating the infromation somewhat.

Anyways, that's all a bit unrelated to the matter at hand, so I'll stop
now and just state that I don't mind having an explicit offset if
people really want it.

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

Re: [Intel-gfx] [PATCH 1/3] drm: Add new DRM_IOCTL_MODE_GETPLANE2

2017-01-12 Thread Daniel Stone 
Hi,

On 12 January 2017 at 17:45, Ville Syrjälä
 wrote:
> On Thu, Jan 12, 2017 at 05:04:46PM +, Daniel Stone wrote:
>> Implicit is clever but horrible. AFAICT, the only way to do it
>> properly would be to have a nested forwards loop walk when you first
>> hit a modifier, searching for further occurrences of that modifier to
>> collect the complete set of formats that modifier applies to.
>
> Not sure for what that is the "only way". In fact I can't right now
> think of any operation that would require an extra loop necessarily.
> For some things you might just want to look for a specific
> format+modifier combo, for that it doesn't matter how many blocks there
> are.

Right, that just needs a local variable to act as a counter.

> And if you want to transform the reply into some less convoluted
> form, well then you'd just need some modifier+dynamic format list thing,
> or if you want to keep to bitmasks you'd either need a bitmask that can
> grow when running out of bits or just make it big enough to handle a
> sufficiently large worst case number of bits.
>
> Dunno, maybe I just lack imagination. Then again, I'm not even sure if
> we're talking about userspace of kernel code here, which might explain
> my general confusion :)

I'm talking about userspace, where I want to have:
struct drm_plane {
struct {
uint32_t format;
uint64_t modifiers[];
} formats[];
}

Rather than keeping the original format around and doing the lookup every time.

Cheers,
Daniel
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH 1/3] drm: Add new DRM_IOCTL_MODE_GETPLANE2

2017-01-12 Thread Daniel Stone 
Hi,

On 12 January 2017 at 14:56, Rob Clark  wrote:
> On Thu, Jan 12, 2017 at 4:38 AM, Ville Syrjälä
>  wrote:
>> Isn't an implicit offset enough? As in first mask for a specific
>> modifier is for format indexes 0-63, second mask for the same modifier
>> is for 64-127, and so on.
>
> hmm, hadn't thought of that approach.  Definitely if we go w/ implicit
> then we want to have userspace support from the get-go.  For explicit,
> I guess userspace could complain and ignore if it saw a non-zero
> offset similar to what we do w/ pad and unknown flags in the other
> direction?

Implicit is clever but horrible. AFAICT, the only way to do it
properly would be to have a nested forwards loop walk when you first
hit a modifier, searching for further occurrences of that modifier to
collect the complete set of formats that modifier applies to.
Depending on what you did with the structures, you'd either have to
destroy the drm_format_modifiers in the GetPlane return so further
instances of your outer loop didn't hit them, or have a _second_
nested loop walk into wherever you copied the formats/modifiers,
searching for anything with that.

Too clever by half, and everyone will get it wrong. Just add an explicit offset.

Cheers,
Daniel
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH 1/3] drm: Add new DRM_IOCTL_MODE_GETPLANE2

2017-01-12 Thread Ville Syrjälä 
On Wed, Jan 11, 2017 at 04:51:16PM -0800, Ben Widawsky wrote:
> Originally based off of a patch by Kristian.
> 
> This new ioctl extends DRM_IOCTL_MODE_GETPLANE, by returning information
> about the modifiers that will work with each format.
> 
> It's modified from Kristian's patch in that the modifiers and formats
> are setup by the driver, and then a callback is used to create the
> format list. The LOC was enough difference that I don't think it made
> sense to leave his authorship, but the new UABI was primarily his idea.
> 
> Additionally, I hit a couple of drivers which Kristian missed updating.
> 
> It also contains a change requested by Daniel to make the modifiers
> array a sentinel based structure instead of a sized one. Upon discussion
> on IRC, it was determined that having an invalid modifier might make
> sense in general as well.
> 
> References: https://patchwork.kernel.org/patch/9482393/
> Signed-off-by: Ben Widawsky 
> ---

> diff --git a/drivers/gpu/drm/drm_modeset_helper.c 
> b/drivers/gpu/drm/drm_modeset_helper.c
> index 2b33825f2f93..9cb1eede0b4d 100644
> --- a/drivers/gpu/drm/drm_modeset_helper.c
> +++ b/drivers/gpu/drm/drm_modeset_helper.c
> @@ -124,6 +124,7 @@ static struct drm_plane *create_primary_plane(struct 
> drm_device *dev)
>  _primary_helper_funcs,
>  safe_modeset_formats,
>  ARRAY_SIZE(safe_modeset_formats),
> +NULL,
>  DRM_PLANE_TYPE_PRIMARY, NULL);
>   if (ret) {
>   kfree(primary);
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index 7b7275f0c2df..2d4fad5db8ed 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -70,6 +70,7 @@ static unsigned int drm_num_planes(struct drm_device *dev)
>   * @funcs: callbacks for the new plane
>   * @formats: array of supported formats (DRM_FORMAT\_\*)
>   * @format_count: number of elements in @formats
> + * @format_modifiers: array of struct drm_format modifiers terminated by 
> INVALID
>   * @type: type of plane (overlay, primary, cursor)
>   * @name: printf style format string for the plane name, or NULL for default 
> name
>   *
> @@ -82,10 +83,12 @@ int drm_universal_plane_init(struct drm_device *dev, 
> struct drm_plane *plane,
>uint32_t possible_crtcs,
>const struct drm_plane_funcs *funcs,
>const uint32_t *formats, unsigned int format_count,
> +  const uint64_t *format_modifiers,
>enum drm_plane_type type,
>const char *name, ...)
>  {
>   struct drm_mode_config *config = >mode_config;
> + unsigned int format_modifier_count = 0;
>   int ret;
>  
>   ret = drm_mode_object_get(dev, >base, DRM_MODE_OBJECT_PLANE);
> @@ -105,6 +108,28 @@ int drm_universal_plane_init(struct drm_device *dev, 
> struct drm_plane *plane,
>   return -ENOMEM;
>   }
>  
> + if (format_modifiers) {
> + const uint64_t *temp_modifiers = format_modifiers;
> + while (*temp_modifiers++ != DRM_FORMAT_MOD_INVALID)
> + format_modifier_count++;
> + }
> +
> + if (format_modifier_count)
> + DRM_DEBUG_KMS("%d format modifiers added to list\n",
> +   format_modifier_count);

nit: Not sure this is printk worthy.

> +
> + plane->modifier_count = format_modifier_count;
> + plane->modifiers = kmalloc_array(format_modifier_count,
> +  sizeof(format_modifiers[0]),
> +  GFP_KERNEL);
> +
> + if (format_modifier_count && !plane->modifiers) {
> + DRM_DEBUG_KMS("out of memory when allocating plane\n");
> + kfree(plane->format_types);
> + drm_mode_object_unregister(dev, >base);
> + return -ENOMEM;
> + }
> +
>   if (name) {
>   va_list ap;
>  
> @@ -117,12 +142,15 @@ int drm_universal_plane_init(struct drm_device *dev, 
> struct drm_plane *plane,
>   }
>   if (!plane->name) {
>   kfree(plane->format_types);
> + kfree(plane->modifiers);
>   drm_mode_object_unregister(dev, >base);
>   return -ENOMEM;
>   }
>  
>   memcpy(plane->format_types, formats, format_count * sizeof(uint32_t));
>   plane->format_count = format_count;
> + memcpy(plane->modifiers, format_modifiers,
> +format_modifier_count * sizeof(format_modifiers[0]));

Looks all right since we do the same for formats anyway. But it did
occur to me (twice at least) that a kmemdup_array() might a nice thing
to have for things like this. But that's a separate topic.

>   plane->possible_crtcs = possible_crtcs;
>   plane->type = type;
>  
> @@

Re: [Intel-gfx] [PATCH 1/3] drm: Add new DRM_IOCTL_MODE_GETPLANE2

2017-01-11 Thread Rob Clark 
On Wed, Jan 11, 2017 at 7:51 PM, Ben Widawsky  wrote:
>
> +struct drm_format_modifier {
> +   /* Bitmask of formats in get_plane format list this info
> +* applies to. */
> +   uint64_t formats;

re: the uabi, I'd suggest to at least make this 'u32 offset; u32
formats'.. we can keep the existing implementation in this patch and
always set 'offset' to zero, and let the first one to hit more than 32
formats deal with the implementation.  (Maybe a strategically placed
WARN_ON() if you go that route..)

Otherwise I guess it is just a couple years until getplane3 ;-)

BR,
-R

> +
> +   /* This modifier can be used with the format for this plane. */
> +   uint64_t modifier;
> +};
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/3] drm: Add new DRM_IOCTL_MODE_GETPLANE2

2017-01-11 Thread Ben Widawsky 
Originally based off of a patch by Kristian.

This new ioctl extends DRM_IOCTL_MODE_GETPLANE, by returning information
about the modifiers that will work with each format.

It's modified from Kristian's patch in that the modifiers and formats
are setup by the driver, and then a callback is used to create the
format list. The LOC was enough difference that I don't think it made
sense to leave his authorship, but the new UABI was primarily his idea.

Additionally, I hit a couple of drivers which Kristian missed updating.

It also contains a change requested by Daniel to make the modifiers
array a sentinel based structure instead of a sized one. Upon discussion
on IRC, it was determined that having an invalid modifier might make
sense in general as well.

References: https://patchwork.kernel.org/patch/9482393/
Signed-off-by: Ben Widawsky 
---
 drivers/gpu/drm/arc/arcpgu_crtc.c   |  1 +
 drivers/gpu/drm/arm/hdlcd_crtc.c|  1 +
 drivers/gpu/drm/arm/malidp_planes.c |  2 +-
 drivers/gpu/drm/armada/armada_crtc.c|  1 +
 drivers/gpu/drm/armada/armada_overlay.c |  1 +
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c |  4 +-
 drivers/gpu/drm/drm_ioctl.c |  2 +-
 drivers/gpu/drm/drm_modeset_helper.c|  1 +
 drivers/gpu/drm/drm_plane.c | 67 -
 drivers/gpu/drm/drm_simple_kms_helper.c |  3 ++
 drivers/gpu/drm/exynos/exynos_drm_plane.c   |  2 +-
 drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c |  2 +-
 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c  |  1 +
 drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c |  2 +-
 drivers/gpu/drm/i915/intel_display.c|  7 ++-
 drivers/gpu/drm/i915/intel_sprite.c |  4 +-
 drivers/gpu/drm/imx/ipuv3-plane.c   |  4 +-
 drivers/gpu/drm/mediatek/mtk_drm_plane.c|  2 +-
 drivers/gpu/drm/meson/meson_plane.c |  1 +
 drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c   |  2 +-
 drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c   |  2 +-
 drivers/gpu/drm/mxsfb/mxsfb_drv.c   |  2 +-
 drivers/gpu/drm/nouveau/nv50_display.c  |  5 +-
 drivers/gpu/drm/omapdrm/omap_plane.c|  3 +-
 drivers/gpu/drm/rcar-du/rcar_du_plane.c |  4 +-
 drivers/gpu/drm/rcar-du/rcar_du_vsp.c   |  5 +-
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c |  4 +-
 drivers/gpu/drm/sti/sti_cursor.c|  1 +
 drivers/gpu/drm/sti/sti_gdp.c   |  2 +-
 drivers/gpu/drm/sti/sti_hqvdp.c |  2 +-
 drivers/gpu/drm/sun4i/sun4i_layer.c |  1 +
 drivers/gpu/drm/tegra/dc.c  | 12 ++---
 drivers/gpu/drm/vc4/vc4_plane.c |  2 +-
 drivers/gpu/drm/virtio/virtgpu_plane.c  |  2 +-
 drivers/gpu/drm/zte/zx_plane.c  |  2 +-
 include/drm/drm_plane.h | 21 +++-
 include/drm/drm_simple_kms_helper.h |  1 +
 include/uapi/drm/drm.h  |  1 +
 include/uapi/drm/drm_fourcc.h   | 11 
 include/uapi/drm/drm_mode.h | 27 ++
 40 files changed, 182 insertions(+), 38 deletions(-)

diff --git a/drivers/gpu/drm/arc/arcpgu_crtc.c 
b/drivers/gpu/drm/arc/arcpgu_crtc.c
index ad9a95916f1f..cd8a24c7c67d 100644
--- a/drivers/gpu/drm/arc/arcpgu_crtc.c
+++ b/drivers/gpu/drm/arc/arcpgu_crtc.c
@@ -218,6 +218,7 @@ static struct drm_plane *arc_pgu_plane_init(struct 
drm_device *drm)
 
ret = drm_universal_plane_init(drm, plane, 0xff, _pgu_plane_funcs,
   formats, ARRAY_SIZE(formats),
+  NULL,
   DRM_PLANE_TYPE_PRIMARY, NULL);
if (ret)
return ERR_PTR(ret);
diff --git a/drivers/gpu/drm/arm/hdlcd_crtc.c b/drivers/gpu/drm/arm/hdlcd_crtc.c
index 20ebfb4fbdfa..89fded880807 100644
--- a/drivers/gpu/drm/arm/hdlcd_crtc.c
+++ b/drivers/gpu/drm/arm/hdlcd_crtc.c
@@ -283,6 +283,7 @@ static struct drm_plane *hdlcd_plane_init(struct drm_device 
*drm)
 
ret = drm_universal_plane_init(drm, plane, 0xff, _plane_funcs,
   formats, ARRAY_SIZE(formats),
+  NULL,
   DRM_PLANE_TYPE_PRIMARY, NULL);
if (ret) {
devm_kfree(drm->dev, plane);
diff --git a/drivers/gpu/drm/arm/malidp_planes.c 
b/drivers/gpu/drm/arm/malidp_planes.c
index eff2fe47e26a..94dbcbc9ad8f 100644
--- a/drivers/gpu/drm/arm/malidp_planes.c
+++ b/drivers/gpu/drm/arm/malidp_planes.c
@@ -283,7 +283,7 @@ int malidp_de_planes_init(struct drm_device *drm)
DRM_PLANE_TYPE_OVERLAY;
ret = drm_universal_plane_init(drm, >base, crtcs,
   _de_plane_funcs, formats,
-  n,