Re: [PATCH RFC v5 02/10] drm: Introduce solid fill DRM plane property

2023-08-07 Thread Jessica Zhang




On 8/4/2023 6:40 AM, Sebastian Wick wrote:

On Mon, Jul 31, 2023 at 6:01 AM Dmitry Baryshkov
 wrote:


On 28/07/2023 20:02, Jessica Zhang wrote:

Document and add support for solid_fill property to drm_plane. In
addition, add support for setting and getting the values for solid_fill.

To enable solid fill planes, userspace must assign a property blob to
the "solid_fill" plane property containing the following information:

struct drm_mode_solid_fill {
   u32 version;
   u32 r, g, b;
};

Signed-off-by: Jessica Zhang 
---
   drivers/gpu/drm/drm_atomic_state_helper.c |  9 +
   drivers/gpu/drm/drm_atomic_uapi.c | 55 
+++
   drivers/gpu/drm/drm_blend.c   | 30 +
   include/drm/drm_blend.h   |  1 +
   include/drm/drm_plane.h   | 35 
   include/uapi/drm/drm_mode.h   | 24 ++
   6 files changed, 154 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c 
b/drivers/gpu/drm/drm_atomic_state_helper.c
index 01638c51ce0a..86fb876efbe6 100644
--- a/drivers/gpu/drm/drm_atomic_state_helper.c
+++ b/drivers/gpu/drm/drm_atomic_state_helper.c
@@ -254,6 +254,11 @@ void __drm_atomic_helper_plane_state_reset(struct 
drm_plane_state *plane_state,
   plane_state->pixel_blend_mode = DRM_MODE_BLEND_PREMULTI;
   plane_state->pixel_source = DRM_PLANE_PIXEL_SOURCE_FB;

+ if (plane_state->solid_fill_blob) {
+ drm_property_blob_put(plane_state->solid_fill_blob);
+ plane_state->solid_fill_blob = NULL;
+ }
+
   if (plane->color_encoding_property) {
   if (!drm_object_property_get_default_value(&plane->base,
  
plane->color_encoding_property,
@@ -336,6 +341,9 @@ void __drm_atomic_helper_plane_duplicate_state(struct 
drm_plane *plane,
   if (state->fb)
   drm_framebuffer_get(state->fb);

+ if (state->solid_fill_blob)
+ drm_property_blob_get(state->solid_fill_blob);
+
   state->fence = NULL;
   state->commit = NULL;
   state->fb_damage_clips = NULL;
@@ -385,6 +393,7 @@ void __drm_atomic_helper_plane_destroy_state(struct 
drm_plane_state *state)
   drm_crtc_commit_put(state->commit);

   drm_property_blob_put(state->fb_damage_clips);
+ drm_property_blob_put(state->solid_fill_blob);
   }
   EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state);

diff --git a/drivers/gpu/drm/drm_atomic_uapi.c 
b/drivers/gpu/drm/drm_atomic_uapi.c
index 454f980e16c9..039686c78c2a 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -316,6 +316,51 @@ drm_atomic_set_crtc_for_connector(struct 
drm_connector_state *conn_state,
   }
   EXPORT_SYMBOL(drm_atomic_set_crtc_for_connector);

+static int drm_atomic_set_solid_fill_prop(struct drm_plane_state *state,
+ struct drm_property_blob *blob)
+{
+ int blob_version;
+
+ if (blob == state->solid_fill_blob)
+ return 0;
+
+ if (blob) {
+ struct drm_mode_solid_fill *user_info = (struct drm_mode_solid_fill 
*)blob->data;
+
+ if (blob->length != sizeof(struct drm_mode_solid_fill)) {
+ drm_dbg_atomic(state->plane->dev,
+"[PLANE:%d:%s] bad solid fill blob length: 
%zu\n",
+state->plane->base.id, state->plane->name,
+blob->length);
+ return -EINVAL;
+ }
+
+ blob_version = user_info->version;


I remember that we had versioning for quite some time. However it stroke
me while reviewing that we do not a way to negotiate supported
SOLID_FILL versions. However as we now have support for different
pixel_source properties, maybe we can drop version completely. If at
some point a driver needs to support different kind of SOLID_FILL
property (consider v2), we can add new pixel source to the enum.


Agreed. Let's drop the versioning.


Acked.

Thanks,

Jessica Zhang






+
+ /* Add more versions if necessary */
+ if (blob_version == 1) {
+ state->solid_fill.r = user_info->r;
+ state->solid_fill.g = user_info->g;
+ state->solid_fill.b = user_info->b;
+ } else {
+ drm_dbg_atomic(state->plane->dev,
+"[PLANE:%d:%s] unsupported solid fill version 
(version=%d)\n",
+state->plane->base.id, state->plane->name,
+blob_version);
+ return -EINVAL;
+ }
+ }
+
+ drm_property_blob_put(state->solid_fill_blob);
+
+ if (blob)
+ state->solid_fill_blob = drm_property_blob_get(blob);
+ else
+ state->solid_fill_blob = NULL;
+
+ return 0;
+}
+
   static void set_ou

Re: [PATCH RFC v5 04/10] drm/atomic: Add pixel source to plane state dump

2023-08-07 Thread Jessica Zhang




On 7/28/2023 5:04 PM, Dmitry Baryshkov wrote:

On 28/07/2023 20:02, Jessica Zhang wrote:

Add pixel source to the atomic plane state dump

Signed-off-by: Jessica Zhang 
---
  drivers/gpu/drm/drm_atomic.c    |  1 +
  drivers/gpu/drm/drm_crtc_internal.h |  2 ++
  drivers/gpu/drm/drm_plane.c | 12 
  3 files changed, 15 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index b4c6ffc438da..c38014abc590 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -713,6 +713,7 @@ static void drm_atomic_plane_print_state(struct 
drm_printer *p,

  drm_printf(p, "plane[%u]: %s\n", plane->base.id, plane->name);
  drm_printf(p, "\tcrtc=%s\n", state->crtc ? state->crtc->name : 
"(null)");
+    drm_printf(p, "\tpixel-source=%s\n", 
drm_plane_get_pixel_source_name(state->pixel_source));

  drm_printf(p, "\tfb=%u\n", state->fb ? state->fb->base.id : 0);
  if (state->fb)
  drm_framebuffer_print_info(p, 2, state->fb);
diff --git a/drivers/gpu/drm/drm_crtc_internal.h 
b/drivers/gpu/drm/drm_crtc_internal.h

index 501a10edd0e1..75b59ec9f1be 100644
--- a/drivers/gpu/drm/drm_crtc_internal.h
+++ b/drivers/gpu/drm/drm_crtc_internal.h
@@ -38,6 +38,7 @@ enum drm_color_encoding;
  enum drm_color_range;
  enum drm_connector_force;
  enum drm_mode_status;
+enum drm_plane_pixel_source;
  struct drm_atomic_state;
  struct drm_bridge;
@@ -267,6 +268,7 @@ int drm_plane_check_pixel_format(struct drm_plane 
*plane,

   u32 format, u64 modifier);
  struct drm_mode_rect *
  __drm_plane_get_damage_clips(const struct drm_plane_state *state);
+const char *drm_plane_get_pixel_source_name(enum 
drm_plane_pixel_source pixel_source);

  /* drm_bridge.c */
  void drm_bridge_detach(struct drm_bridge *bridge);
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index f342cf15412b..4188b3491625 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -1487,6 +1487,18 @@ __drm_plane_get_damage_clips(const struct 
drm_plane_state *state)

  state->fb_damage_clips->data : NULL);
  }
+const char *drm_plane_get_pixel_source_name(enum 
drm_plane_pixel_source pixel_source)

+{
+    switch(pixel_source) {
+    case DRM_PLANE_PIXEL_SOURCE_NONE:
+    return "NONE";
+    case DRM_PLANE_PIXEL_SOURCE_FB:
+    return "fb";
+    default:
+    return "";
+    }
+}


Please use DRM_ENUM_NAME_FN instead.


Hi Dmitry,

Acked.

Thanks,

Jessica Zhang




+
  /**
   * drm_plane_get_damage_clips - Returns damage clips.
   * @state: Plane state.



--
With best wishes
Dmitry



Re: [PATCH RFC v5 05/10] drm/atomic: Add solid fill data to plane state dump

2023-08-07 Thread Jessica Zhang




On 7/28/2023 5:05 PM, Dmitry Baryshkov wrote:

On 28/07/2023 20:02, Jessica Zhang wrote:

Add solid_fill property data to the atomic plane state dump.

Signed-off-by: Jessica Zhang 
---
  drivers/gpu/drm/drm_atomic.c |  4 
  drivers/gpu/drm/drm_plane.c  | 10 ++
  include/drm/drm_plane.h  |  3 +++
  3 files changed, 17 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index c38014abc590..1ee7d08041bc 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -717,6 +717,10 @@ static void drm_atomic_plane_print_state(struct 
drm_printer *p,

  drm_printf(p, "\tfb=%u\n", state->fb ? state->fb->base.id : 0);
  if (state->fb)
  drm_framebuffer_print_info(p, 2, state->fb);
+    drm_printf(p, "\tsolid_fill=%u\n",
+    state->solid_fill_blob ? state->solid_fill_blob->base.id 
: 0);

+    if (state->solid_fill_blob)
+    drm_plane_solid_fill_print_info(p, 2, state);
  drm_printf(p, "\tcrtc-pos=" DRM_RECT_FMT "\n", 
DRM_RECT_ARG(&dest));
  drm_printf(p, "\tsrc-pos=" DRM_RECT_FP_FMT "\n", 
DRM_RECT_FP_ARG(&src));

  drm_printf(p, "\trotation=%x\n", state->rotation);
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index 4188b3491625..009d3ebd9b39 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -1494,11 +1494,21 @@ const char 
*drm_plane_get_pixel_source_name(enum drm_plane_pixel_source pixel_so

  return "NONE";
  case DRM_PLANE_PIXEL_SOURCE_FB:
  return "fb";
+    case DRM_PLANE_PIXEL_SOURCE_SOLID_FILL:
+    return "solid_fill";
  default:
  return "";
  }
  }


This chunk should be a part of the previous commit. Or dropped 
completely once DRM_ENUM_NAME_FN is used.


The rest LGTM.


Hi Dmitry,

Sounds good -- will drop this.

Thanks,

Jessica Zhang



+void drm_plane_solid_fill_print_info(struct drm_printer *p, unsigned 
int indent,

+ const struct drm_plane_state *state)
+{
+    drm_printf_indent(p, indent, "r=0x%x\n", state->solid_fill.r);
+    drm_printf_indent(p, indent, "g=0x%x\n", state->solid_fill.g);
+    drm_printf_indent(p, indent, "b=0x%x\n", state->solid_fill.b);
+}
+
  /**
   * drm_plane_get_damage_clips - Returns damage clips.
   * @state: Plane state.
diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
index 234fee3d5a95..303f01f0588c 100644
--- a/include/drm/drm_plane.h
+++ b/include/drm/drm_plane.h
@@ -1000,6 +1000,9 @@ drm_plane_get_damage_clips_count(const struct 
drm_plane_state *state);

  struct drm_mode_rect *
  drm_plane_get_damage_clips(const struct drm_plane_state *state);
+void drm_plane_solid_fill_print_info(struct drm_printer *p, unsigned 
int indent,

+ const struct drm_plane_state *state);
+
  int drm_plane_create_scaling_filter_property(struct drm_plane *plane,
   unsigned int supported_filters);



--
With best wishes
Dmitry



Re: [PATCH RFC v5 09/10] drm/msm/dpu: Use DRM solid_fill property

2023-08-07 Thread Jessica Zhang




On 7/31/2023 5:52 PM, Dmitry Baryshkov wrote:

On 01/08/2023 03:39, Jessica Zhang wrote:



On 7/30/2023 9:15 PM, Dmitry Baryshkov wrote:

On 28/07/2023 20:02, Jessica Zhang wrote:

Drop DPU_PLANE_COLOR_FILL_FLAG and check the DRM solid_fill property to
determine if the plane is solid fill. In addition drop the DPU plane
color_fill field as we can now use drm_plane_state.solid_fill instead,
and pass in drm_plane_state.alpha to _dpu_plane_color_fill_pipe() to
allow userspace to configure the alpha value for the solid fill color.

Reviewed-by: Dmitry Baryshkov 
Signed-off-by: Jessica Zhang 
---
  drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 24 
++--

  1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c

index 114c803ff99b..95fc0394d13e 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
@@ -42,7 +42,6 @@
  #define SHARP_SMOOTH_THR_DEFAULT    8
  #define SHARP_NOISE_THR_DEFAULT    2
-#define DPU_PLANE_COLOR_FILL_FLAG    BIT(31)
  #define DPU_ZPOS_MAX 255
  /*
@@ -82,7 +81,6 @@ struct dpu_plane {
  enum dpu_sspp pipe;
-    uint32_t color_fill;
  bool is_error;
  bool is_rt_pipe;
  const struct dpu_mdss_cfg *catalog;
@@ -606,6 +604,20 @@ static void _dpu_plane_color_fill_pipe(struct 
dpu_plane_state *pstate,
  _dpu_plane_setup_scaler(pipe, fmt, true, &pipe_cfg, 
pstate->rotation);

  }
+static uint32_t _dpu_plane_get_bgr_fill_color(struct drm_solid_fill 
solid_fill)


As I commented for v4 (please excuse me for not responding to your 
email at thattime), we can return abgr here, taking 
plane->state->alpha into account.


Hi Dmitry,

Since it seems that this comment wasn't resolved, I can drop your R-B 
tag in the next revision.


It's a minor issue, so no need to drop the tag.



 From my previous response, I pointed out that the color parameter 
expects an RGB value [1].


So is the intention here to refactor _dpu_plane_color_fill() to accept 
an ABGR color?


That's what I'm suggesting.


Hi Dmitry,

Got it, sounds good to me.

Thanks,

Jessica Zhang





Thanks,

Jessica Zhang

[1] 
https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c#L676





+{
+    uint32_t ret = 0;
+    uint8_t b = solid_fill.b >> 24;
+    uint8_t g = solid_fill.g >> 24;
+    uint8_t r = solid_fill.r >> 24;
+
+    ret |= b << 16;
+    ret |= g << 8;
+    ret |= r;
+
+    return ret;
+}
+
  /**
   * _dpu_plane_color_fill - enables color fill on plane
   * @pdpu:   Pointer to DPU plane object
@@ -977,9 +989,9 @@ void dpu_plane_flush(struct drm_plane *plane)
  if (pdpu->is_error)
  /* force white frame with 100% alpha pipe output on error */
  _dpu_plane_color_fill(pdpu, 0xFF, 0xFF);
-    else if (pdpu->color_fill & DPU_PLANE_COLOR_FILL_FLAG)
-    /* force 100% alpha */
-    _dpu_plane_color_fill(pdpu, pdpu->color_fill, 0xFF);
+    else if (drm_plane_solid_fill_enabled(plane->state))
+    _dpu_plane_color_fill(pdpu, 
_dpu_plane_get_bgr_fill_color(plane->state->solid_fill),

+    plane->state->alpha);
  else {
  dpu_plane_flush_csc(pdpu, &pstate->pipe);
  dpu_plane_flush_csc(pdpu, &pstate->r_pipe);
@@ -1024,7 +1036,7 @@ static void dpu_plane_sspp_update_pipe(struct 
drm_plane *plane,

  }
  /* override for color fill */
-    if (pdpu->color_fill & DPU_PLANE_COLOR_FILL_FLAG) {
+    if (drm_plane_solid_fill_enabled(plane->state)) {
  _dpu_plane_set_qos_ctrl(plane, pipe, false);
  /* skip remaining processing on color fill */



--
With best wishes
Dmitry



--
With best wishes
Dmitry



Re: [PATCH RFC v5 01/10] drm: Introduce pixel_source DRM plane property

2023-08-07 Thread Jessica Zhang




On 8/4/2023 6:15 AM, Sebastian Wick wrote:

On Fri, Jul 28, 2023 at 7:03 PM Jessica Zhang  wrote:


Add support for pixel_source property to drm_plane and related
documentation. In addition, force pixel_source to
DRM_PLANE_PIXEL_SOURCE_FB in DRM_IOCTL_MODE_SETPLANE as to not break
legacy userspace.

This enum property will allow user to specify a pixel source for the
plane. Possible pixel sources will be defined in the
drm_plane_pixel_source enum.

The current possible pixel sources are DRM_PLANE_PIXEL_SOURCE_NONE and
DRM_PLANE_PIXEL_SOURCE_FB with *_PIXEL_SOURCE_FB being the default value.

Signed-off-by: Jessica Zhang 
---
  drivers/gpu/drm/drm_atomic_state_helper.c |  1 +
  drivers/gpu/drm/drm_atomic_uapi.c |  4 ++
  drivers/gpu/drm/drm_blend.c   | 85 +++
  drivers/gpu/drm/drm_plane.c   |  3 ++
  include/drm/drm_blend.h   |  2 +
  include/drm/drm_plane.h   | 21 
  6 files changed, 116 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c 
b/drivers/gpu/drm/drm_atomic_state_helper.c
index 784e63d70a42..01638c51ce0a 100644
--- a/drivers/gpu/drm/drm_atomic_state_helper.c
+++ b/drivers/gpu/drm/drm_atomic_state_helper.c
@@ -252,6 +252,7 @@ void __drm_atomic_helper_plane_state_reset(struct 
drm_plane_state *plane_state,

 plane_state->alpha = DRM_BLEND_ALPHA_OPAQUE;
 plane_state->pixel_blend_mode = DRM_MODE_BLEND_PREMULTI;
+   plane_state->pixel_source = DRM_PLANE_PIXEL_SOURCE_FB;

 if (plane->color_encoding_property) {
 if (!drm_object_property_get_default_value(&plane->base,
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c 
b/drivers/gpu/drm/drm_atomic_uapi.c
index d867e7f9f2cd..454f980e16c9 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -544,6 +544,8 @@ static int drm_atomic_plane_set_property(struct drm_plane 
*plane,
 state->src_w = val;
 } else if (property == config->prop_src_h) {
 state->src_h = val;
+   } else if (property == plane->pixel_source_property) {
+   state->pixel_source = val;
 } else if (property == plane->alpha_property) {
 state->alpha = val;
 } else if (property == plane->blend_mode_property) {
@@ -616,6 +618,8 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
 *val = state->src_w;
 } else if (property == config->prop_src_h) {
 *val = state->src_h;
+   } else if (property == plane->pixel_source_property) {
+   *val = state->pixel_source;
 } else if (property == plane->alpha_property) {
 *val = state->alpha;
 } else if (property == plane->blend_mode_property) {
diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
index 6e74de833466..c500310a3d09 100644
--- a/drivers/gpu/drm/drm_blend.c
+++ b/drivers/gpu/drm/drm_blend.c
@@ -185,6 +185,21 @@
   *  plane does not expose the "alpha" property, then this is
   *  assumed to be 1.0
   *
+ * pixel_source:
+ * pixel_source is set up with drm_plane_create_pixel_source_property().
+ * It is used to toggle the active source of pixel data for the plane.
+ * The plane will only display data from the set pixel_source -- any
+ * data from other sources will be ignored.
+ *
+ * Possible values:
+ *
+ * "NONE":
+ * No active pixel source.
+ * Committing with a NONE pixel source will disable the plane.
+ *
+ * "FB":
+ * Framebuffer source set by the "FB_ID" property.
+ *
   * Note that all the property extensions described here apply either to the
   * plane or the CRTC (e.g. for the background color, which currently is not
   * exposed and assumed to be black).
@@ -615,3 +630,73 @@ int drm_plane_create_blend_mode_property(struct drm_plane 
*plane,
 return 0;
  }
  EXPORT_SYMBOL(drm_plane_create_blend_mode_property);
+
+/**
+ * drm_plane_create_pixel_source_property - create a new pixel source property
+ * @plane: DRM plane
+ * @extra_sources: Bitmask of additional supported pixel_sources for the 
driver.
+ *DRM_PLANE_PIXEL_SOURCE_FB always be enabled as a supported
+ *source.
+ *
+ * This creates a new property describing the current source of pixel data for 
the
+ * plane. The pixel_source will be initialized as DRM_PLANE_PIXEL_SOURCE_FB by 
default.
+ *
+ * Drivers can set a custom default source by overriding the pixel_source 
value in
+ * drm_plane_funcs.reset()
+ *
+ * The property is exposed to userspace as an enumeration property called
+ * "pixel_source" and has the following enumeration values:
+ *
+ * "NONE":
+ *  No active pixel source
+ *
+ * "FB":
+ * Framebuffer pixel source
+ *
+ * Returns:
+ * Zero on success, negative errno on failure.
+ */
+int drm_plane_create_pixel_source_property(struct drm_plane *pla

Re: [PATCH RFC v5 02/10] drm: Introduce solid fill DRM plane property

2023-08-07 Thread Jessica Zhang




On 8/4/2023 6:27 AM, Dmitry Baryshkov wrote:

On Fri, 28 Jul 2023 at 20:03, Jessica Zhang  wrote:


Document and add support for solid_fill property to drm_plane. In
addition, add support for setting and getting the values for solid_fill.

To enable solid fill planes, userspace must assign a property blob to
the "solid_fill" plane property containing the following information:

struct drm_mode_solid_fill {
 u32 version;
 u32 r, g, b;
};

Signed-off-by: Jessica Zhang 
---
  drivers/gpu/drm/drm_atomic_state_helper.c |  9 +
  drivers/gpu/drm/drm_atomic_uapi.c | 55 +++
  drivers/gpu/drm/drm_blend.c   | 30 +
  include/drm/drm_blend.h   |  1 +
  include/drm/drm_plane.h   | 35 
  include/uapi/drm/drm_mode.h   | 24 ++
  6 files changed, 154 insertions(+)



[skipped most of the patch]


diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index 43691058d28f..53c8efa5ad7f 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -259,6 +259,30 @@ struct drm_mode_modeinfo {
 char name[DRM_DISPLAY_MODE_LEN];
  };

+/**
+ * struct drm_mode_solid_fill - User info for solid fill planes
+ *
+ * This is the userspace API solid fill information structure.
+ *
+ * Userspace can enable solid fill planes by assigning the plane "solid_fill"
+ * property to a blob containing a single drm_mode_solid_fill struct populated 
with an RGB323232
+ * color and setting the pixel source to "SOLID_FILL".
+ *
+ * For information on the plane property, see 
drm_plane_create_solid_fill_property()
+ *
+ * @version: Version of the blob. Currently, there is only support for version 
== 1
+ * @r: Red color value of single pixel
+ * @g: Green color value of single pixel
+ * @b: Blue color value of single pixel
+ */
+struct drm_mode_solid_fill {
+   __u32 version;
+   __u32 r;
+   __u32 g;
+   __u32 b;


Another thought about the drm_mode_solid_fill uABI. I still think we
should add alpha here. The reason is the following:

It is true that we have  drm_plane_state::alpha and the plane's
"alpha" property. However it is documented as "the plane-wide opacity
[...] It can be combined with pixel alpha. The pixel values in the
framebuffers are expected to not be pre-multiplied by the global alpha
associated to the plane.".

I can imagine a use case, when a user might want to enable plane-wide
opacity, set "pixel blend mode" to "Coverage" and then switch between
partially opaque framebuffer and partially opaque solid-fill without
touching the plane's alpha value.


Hi Dmitry,

I don't really agree that adding a solid fill alpha would be a good 
idea. Since the intent behind solid fill is to have a single color for 
the entire plane, I think it makes more sense to have solid fill rely on 
the global plane alpha.


As stated in earlier discussions, I think having both a solid_fill.alpha 
and a plane_state.alpha would be redundant and serve to confuse the user 
as to which one to set.


Thanks,

Jessica Zhang



--
With best wishes
Dmitry


Re: [PATCH RFC v5 02/10] drm: Introduce solid fill DRM plane property

2023-08-07 Thread Dmitry Baryshkov



On 8 August 2023 00:41:07 GMT+03:00, Jessica Zhang  
wrote:
>
>
>On 8/4/2023 6:27 AM, Dmitry Baryshkov wrote:
>> On Fri, 28 Jul 2023 at 20:03, Jessica Zhang  
>> wrote:
>>> 
>>> Document and add support for solid_fill property to drm_plane. In
>>> addition, add support for setting and getting the values for solid_fill.
>>> 
>>> To enable solid fill planes, userspace must assign a property blob to
>>> the "solid_fill" plane property containing the following information:
>>> 
>>> struct drm_mode_solid_fill {
>>>  u32 version;
>>>  u32 r, g, b;
>>> };
>>> 
>>> Signed-off-by: Jessica Zhang 
>>> ---
>>>   drivers/gpu/drm/drm_atomic_state_helper.c |  9 +
>>>   drivers/gpu/drm/drm_atomic_uapi.c | 55 
>>> +++
>>>   drivers/gpu/drm/drm_blend.c   | 30 +
>>>   include/drm/drm_blend.h   |  1 +
>>>   include/drm/drm_plane.h   | 35 
>>>   include/uapi/drm/drm_mode.h   | 24 ++
>>>   6 files changed, 154 insertions(+)
>>> 
>> 
>> [skipped most of the patch]
>> 
>>> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
>>> index 43691058d28f..53c8efa5ad7f 100644
>>> --- a/include/uapi/drm/drm_mode.h
>>> +++ b/include/uapi/drm/drm_mode.h
>>> @@ -259,6 +259,30 @@ struct drm_mode_modeinfo {
>>>  char name[DRM_DISPLAY_MODE_LEN];
>>>   };
>>> 
>>> +/**
>>> + * struct drm_mode_solid_fill - User info for solid fill planes
>>> + *
>>> + * This is the userspace API solid fill information structure.
>>> + *
>>> + * Userspace can enable solid fill planes by assigning the plane 
>>> "solid_fill"
>>> + * property to a blob containing a single drm_mode_solid_fill struct 
>>> populated with an RGB323232
>>> + * color and setting the pixel source to "SOLID_FILL".
>>> + *
>>> + * For information on the plane property, see 
>>> drm_plane_create_solid_fill_property()
>>> + *
>>> + * @version: Version of the blob. Currently, there is only support for 
>>> version == 1
>>> + * @r: Red color value of single pixel
>>> + * @g: Green color value of single pixel
>>> + * @b: Blue color value of single pixel
>>> + */
>>> +struct drm_mode_solid_fill {
>>> +   __u32 version;
>>> +   __u32 r;
>>> +   __u32 g;
>>> +   __u32 b;
>> 
>> Another thought about the drm_mode_solid_fill uABI. I still think we
>> should add alpha here. The reason is the following:
>> 
>> It is true that we have  drm_plane_state::alpha and the plane's
>> "alpha" property. However it is documented as "the plane-wide opacity
>> [...] It can be combined with pixel alpha. The pixel values in the
>> framebuffers are expected to not be pre-multiplied by the global alpha
>> associated to the plane.".
>> 
>> I can imagine a use case, when a user might want to enable plane-wide
>> opacity, set "pixel blend mode" to "Coverage" and then switch between
>> partially opaque framebuffer and partially opaque solid-fill without
>> touching the plane's alpha value.
>
>Hi Dmitry,
>
>I don't really agree that adding a solid fill alpha would be a good idea. 
>Since the intent behind solid fill is to have a single color for the entire 
>plane, I think it makes more sense to have solid fill rely on the global plane 
>alpha.
>
>As stated in earlier discussions, I think having both a solid_fill.alpha and a 
>plane_state.alpha would be redundant and serve to confuse the user as to which 
>one to set.

That depends on the blending mode: in Coverage mode one has independent plane 
and contents alpha values. And I consider alpha value to be a part of the 
colour in the rgba/bgra modes.


>
>Thanks,
>
>Jessica Zhang
>
>> 
>> -- 
>> With best wishes
>> Dmitry

-- 
With best wishes
Dmitry