On 17-05-17 01:06:16, Emil Velikov wrote:
Hi Ben,
On 16 May 2017 at 22:31, Ben Widawsky <ben@xxxxxxxxxxxx> wrote:
Updated blob layout (Rob, Daniel, Kristian, xerpi)
v2:
* Removed __packed, and alignment (.+)
* Fix indent in drm_format_modifier fields (Liviu)
* Remove duplicated modifier > 64 check (Liviu)
* Change comment about modifier (Liviu)
* Remove arguments to blob creation, use plane instead (Liviu)
* Fix data types (Ben)
* Make the blob part of uapi (Daniel)
Cc: Rob Clark <robdclark@xxxxxxxxx>
Cc: Daniel Stone <daniels@xxxxxxxxxxxxx>
Cc: Kristian H. Kristensen <hoegsberg@xxxxxxxxx>
Cc: Liviu Dudau <liviu@xxxxxxxxxxx>
Signed-off-by: Ben Widawsky <ben@xxxxxxxxxxxx>
Reviewed-by: Daniel Stone <daniels@xxxxxxxxxxxxx>
I think this is almost perfect, barring the UAPI nitpick.
The rest is somewhat of a bikeshedding.
With the UAPI resolved, regardless of the rest
Reviewed-by: Emil Velikov <emil.l.velikov@xxxxxxxxx>
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
+static int create_in_format_blob(struct drm_device *dev, struct drm_plane *plane)
+{
+ const struct drm_mode_config *config = &dev->mode_config;
+ const uint64_t *temp_modifiers = plane->modifiers;
+ unsigned int format_modifier_count = 0;
+ struct drm_property_blob *blob = NULL;
+ struct drm_format_modifier *mod;
+ size_t blob_size = 0, formats_size, modifiers_size;
There's no need to initialize blob and blob_size here.
+ struct drm_format_modifier_blob *blob_data;
+ int i, j, ret = 0;
Make i and j unsigned to match format_modifier_count and
plane->format_count respectively.
Then expand ret in the only place where it's used?
Oh. ret has lost it's utility over the iterations of this patch. Make i and j
unsigned and dropped ret.
+
+ if (plane->modifiers)
+ while (*temp_modifiers++ != DRM_FORMAT_MOD_INVALID)
+ format_modifier_count++;
+
+ formats_size = sizeof(*plane->format_types) * plane->format_count;
+ if (WARN_ON(!formats_size)) {
+ /* 0 formats are never expected */
+ return 0;
+ }
+
+ modifiers_size =
+ sizeof(struct drm_format_modifier) * format_modifier_count;
+
+ blob_size = sizeof(struct drm_format_modifier_blob);
+ blob_size += ALIGN(formats_size, 8);
Worth having the "Modifiers offset is a pointer..." comment moved/copied here?
Yes it is.
+ blob_size += modifiers_size;
+
+ blob = drm_property_create_blob(dev, blob_size, NULL);
+ if (IS_ERR(blob))
+ return -1;
+
Maybe propagate the exact error... Hmm we don't seem to check if
create_in_format_blob fails either so perhaps it's not worth it.
In this case it's almost definitely ENOMEM. All values should be verified - I
think the other errors are there for when userspace is utilizing blob creation.
So I'll just leave it.
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -665,6 +665,56 @@ struct drm_mode_atomic {
__u64 user_data;
};
+struct drm_format_modifier_blob {
+#define FORMAT_BLOB_CURRENT 1
+ /* Version of this blob format */
+ u32 version;
+
+ /* Flags */
+ u32 flags;
+
+ /* Number of fourcc formats supported */
+ u32 count_formats;
+
+ /* Where in this blob the formats exist (in bytes) */
+ u32 formats_offset;
+
+ /* Number of drm_format_modifiers */
+ u32 count_modifiers;
+
+ /* Where in this blob the modifiers exist (in bytes) */
+ u32 modifiers_offset;
+
+ /* u32 formats[] */
+ /* struct drm_format_modifier modifiers[] */
+};
+
+struct drm_format_modifier {
+ /* Bitmask of formats in get_plane format list this info applies to. The
+ * offset allows a sliding window of which 64 formats (bits).
+ *
+ * Some examples:
+ * In today's world with < 65 formats, and formats 0, and 2 are
+ * supported
+ * 0x0000000000000005
+ * ^-offset = 0, formats = 5
+ *
+ * If the number formats grew to 128, and formats 98-102 are
+ * supported with the modifier:
G>> + *
+ * 0x0000003c00000000 0000000000000000
+ * ^
+ * |__offset = 64, formats = 0x3c00000000
+ *
+ */
+ __u64 formats;
+ __u32 offset;
+ __u32 pad;
+
+ /* The modifier that applies to the >get_plane format list bitmask. */
+ __u64 modifier;
Please drop the leading __ from the type names in UAPI headers.
Many other structures have the __, can you explain why please (this has probably
been beaten to death already; but I don't know)?
Regards,
Emil
--
Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
