To human readers
"array of struct drm_format modifiers" is almost indistinguishable from "array of struct drm_format_modifiers", especially given that struct drm_format_modifier does exist.
And indeed the parameter passes an array of uint64_t rather than an array of structs, but the first words of the comment suggest that it passes an array of structs.
Signed-off-by: Andrzej Pietrasiewicz andrzej.p@collabora.com --- drivers/gpu/drm/drm_plane.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c index d6ad60ab0d38..df05d8a0dd63 100644 --- a/drivers/gpu/drm/drm_plane.c +++ b/drivers/gpu/drm/drm_plane.c @@ -160,7 +160,7 @@ static int create_in_format_blob(struct drm_device *dev, struct drm_plane *plane * @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 + * @format_modifiers: array of modifiers of struct drm_format terminated by * DRM_FORMAT_MOD_INVALID * @type: type of plane (overlay, primary, cursor) * @name: printf style format string for the plane name, or NULL for default name
On Wed, Oct 02, 2019 at 05:43:49PM +0200, Andrzej Pietrasiewicz wrote:
To human readers
"array of struct drm_format modifiers" is almost indistinguishable from "array of struct drm_format_modifiers",
Unless I'm blind those two *are* indistinguishable :P
especially given that struct drm_format_modifier does exist.
And indeed the parameter passes an array of uint64_t rather than an array of structs, but the first words of the comment suggest that it passes an array of structs.
Signed-off-by: Andrzej Pietrasiewicz andrzej.p@collabora.com
drivers/gpu/drm/drm_plane.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c index d6ad60ab0d38..df05d8a0dd63 100644 --- a/drivers/gpu/drm/drm_plane.c +++ b/drivers/gpu/drm/drm_plane.c @@ -160,7 +160,7 @@ static int create_in_format_blob(struct drm_device *dev, struct drm_plane *plane
Looks like you have a broken version of git.
- @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
- @format_modifiers: array of modifiers of struct drm_format terminated by
Now it seems to be saying it's passing in struct drm_format foo[]. That doesn't seem right either.
DRM_FORMAT_MOD_INVALID- @type: type of plane (overlay, primary, cursor)
- @name: printf style format string for the plane name, or NULL for default name
-- 2.17.1
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi Ville,
W dniu 02.10.2019 o 18:18, Ville Syrjälä pisze:
On Wed, Oct 02, 2019 at 05:43:49PM +0200, Andrzej Pietrasiewicz wrote:
To human readers
"array of struct drm_format modifiers" is almost indistinguishable from "array of struct drm_format_modifiers",
Unless I'm blind those two *are* indistinguishable :P
especially given that struct drm_format_modifier does exist.
And indeed the parameter passes an array of uint64_t rather than an array of structs, but the first words of the comment suggest that it passes an array of structs.
Signed-off-by: Andrzej Pietrasiewicz andrzej.p@collabora.com
drivers/gpu/drm/drm_plane.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c index d6ad60ab0d38..df05d8a0dd63 100644 --- a/drivers/gpu/drm/drm_plane.c +++ b/drivers/gpu/drm/drm_plane.c @@ -160,7 +160,7 @@ static int create_in_format_blob(struct drm_device *dev, struct drm_plane *plane
Looks like you have a broken version of git.
- @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
- @format_modifiers: array of modifiers of struct drm_format terminated by
Now it seems to be saying it's passing in struct drm_format foo[]. That doesn't seem right either.
Good point!
array of modifiers applied to struct drm_format?
Andrzej
On Wed, Oct 02, 2019 at 06:22:23PM +0200, Andrzej Pietrasiewicz wrote:
Hi Ville,
W dniu 02.10.2019 o 18:18, Ville Syrjälä pisze:
On Wed, Oct 02, 2019 at 05:43:49PM +0200, Andrzej Pietrasiewicz wrote:
To human readers
"array of struct drm_format modifiers" is almost indistinguishable from "array of struct drm_format_modifiers",
Unless I'm blind those two *are* indistinguishable :P
especially given that struct drm_format_modifier does exist.
And indeed the parameter passes an array of uint64_t rather than an array of structs, but the first words of the comment suggest that it passes an array of structs.
Signed-off-by: Andrzej Pietrasiewicz andrzej.p@collabora.com
drivers/gpu/drm/drm_plane.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c index d6ad60ab0d38..df05d8a0dd63 100644 --- a/drivers/gpu/drm/drm_plane.c +++ b/drivers/gpu/drm/drm_plane.c @@ -160,7 +160,7 @@ static int create_in_format_blob(struct drm_device *dev, struct drm_plane *plane
Looks like you have a broken version of git.
- @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
- @format_modifiers: array of modifiers of struct drm_format terminated by
Now it seems to be saying it's passing in struct drm_format foo[]. That doesn't seem right either.
Good point!
array of modifiers applied to struct drm_format?
Not sure what this has to do with that struct?
I think I'd just make it "array of modifiers terminated by ..."
Hi Andrzej
Always good to have clear documentation.
On Wed, Oct 02, 2019 at 05:43:49PM +0200, Andrzej Pietrasiewicz wrote:
To human readers
"array of struct drm_format modifiers" is almost indistinguishable from "array of struct drm_format_modifiers", especially given that struct drm_format_modifier does exist.
And indeed the parameter passes an array of uint64_t rather than an array of structs, but the first words of the comment suggest that it passes an array of structs.
Signed-off-by: Andrzej Pietrasiewicz andrzej.p@collabora.com
drivers/gpu/drm/drm_plane.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c index d6ad60ab0d38..df05d8a0dd63 100644 --- a/drivers/gpu/drm/drm_plane.c +++ b/drivers/gpu/drm/drm_plane.c @@ -160,7 +160,7 @@ static int create_in_format_blob(struct drm_device *dev, struct drm_plane *plane
- @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
- @format_modifiers: array of modifiers of struct drm_format terminated by
DRM_FORMAT_MOD_INVALID
@format_modifiers is an array of DRM_FORMAT_* which are defined as:
#define fourcc_code(a, b, c, d) ((__u32)(a) | ((__u32)(b) << 8) | \ ((__u32)(c) << 16) | ((__u32)(d) << 24))
But the array is a u64[] like this: static const u64 tegra20_modifiers[] = { DRM_FORMAT_MOD_LINEAR, DRM_FORMAT_MOD_NVIDIA_TEGRA_TILED, DRM_FORMAT_MOD_INVALID };
So this is not struct drm_format.
Can you try to give it a shot more?
Sam
To human readers
"array of struct drm_format modifiers" is almost indistinguishable from "array of struct drm_format_modifiers", especially given that struct drm_format_modifier does exist.
And indeed the parameter passes an array of uint64_t rather than an array of structs, but the first words of the comment suggest that it passes an array of structs.
Signed-off-by: Andrzej Pietrasiewicz andrzej.p@collabora.com --- drivers/gpu/drm/drm_plane.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c index d6ad60ab0d38..0d4f9172c0dd 100644 --- a/drivers/gpu/drm/drm_plane.c +++ b/drivers/gpu/drm/drm_plane.c @@ -160,7 +160,7 @@ static int create_in_format_blob(struct drm_device *dev, struct drm_plane *plane * @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 + * @format_modifiers: array of format modifiers terminated by * DRM_FORMAT_MOD_INVALID * @type: type of plane (overlay, primary, cursor) * @name: printf style format string for the plane name, or NULL for default name
On Thu, Oct 03, 2019 at 09:51:18AM +0200, Andrzej Pietrasiewicz wrote:
To human readers
The commit message is always for human readers, no need to point that out...
"array of struct drm_format modifiers" is almost indistinguishable from "array of struct drm_format_modifiers", especially given that struct drm_format_modifier does exist.
..but this paragraph still manages to 100% confuse this particular human.
The actual code changes lgtm, so with the commit message reworded this patch is Reviewed-by: Ville Syrjälä ville.syrjala@linux.intel.com
And indeed the parameter passes an array of uint64_t rather than an array of structs, but the first words of the comment suggest that it passes
Signed-off-by: Andrzej Pietrasiewicz andrzej.p@collabora.com
drivers/gpu/drm/drm_plane.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c index d6ad60ab0d38..0d4f9172c0dd 100644 --- a/drivers/gpu/drm/drm_plane.c +++ b/drivers/gpu/drm/drm_plane.c @@ -160,7 +160,7 @@ static int create_in_format_blob(struct drm_device *dev, struct drm_plane *plane
- @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
- @format_modifiers: array of format modifiers terminated by
DRM_FORMAT_MOD_INVALID- @type: type of plane (overlay, primary, cursor)
- @name: printf style format string for the plane name, or NULL for default name
-- 2.17.1
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Thursday, 2019-10-03 16:53:18 +0300, Ville Syrjälä wrote:
On Thu, Oct 03, 2019 at 09:51:18AM +0200, Andrzej Pietrasiewicz wrote:
To human readers
The commit message is always for human readers, no need to point that out...
"array of struct drm_format modifiers" is almost indistinguishable from "array of struct drm_format_modifiers", especially given that struct drm_format_modifier does exist.
..but this paragraph still manages to 100% confuse this particular human.
There's an underscore instead of a space on the second line (s/drm_format modifiers/drm_format_modifiers/).
It should definitely be reworded to be much clearer.
The actual code changes lgtm, so with the commit message reworded this patch is Reviewed-by: Ville Syrjälä ville.syrjala@linux.intel.com
And indeed the parameter passes an array of uint64_t rather than an array of structs, but the first words of the comment suggest that it passes
Signed-off-by: Andrzej Pietrasiewicz andrzej.p@collabora.com
drivers/gpu/drm/drm_plane.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c index d6ad60ab0d38..0d4f9172c0dd 100644 --- a/drivers/gpu/drm/drm_plane.c +++ b/drivers/gpu/drm/drm_plane.c @@ -160,7 +160,7 @@ static int create_in_format_blob(struct drm_device *dev, struct drm_plane *plane
- @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
- @format_modifiers: array of format modifiers terminated by
DRM_FORMAT_MOD_INVALID- @type: type of plane (overlay, primary, cursor)
- @name: printf style format string for the plane name, or NULL for default name
-- 2.17.1
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Fri, Oct 04, 2019 at 08:08:26PM +0100, Eric Engestrom wrote:
On Thursday, 2019-10-03 16:53:18 +0300, Ville Syrjälä wrote:
On Thu, Oct 03, 2019 at 09:51:18AM +0200, Andrzej Pietrasiewicz wrote:
To human readers
The commit message is always for human readers, no need to point that out...
"array of struct drm_format modifiers" is almost indistinguishable from "array of struct drm_format_modifiers", especially given that struct drm_format_modifier does exist.
..but this paragraph still manages to 100% confuse this particular human.
There's an underscore instead of a space on the second line (s/drm_format modifiers/drm_format_modifiers/).
OK, so I guess I really am blind.
It should definitely be reworded to be much clearer.
The actual code changes lgtm, so with the commit message reworded this patch is Reviewed-by: Ville Syrjälä ville.syrjala@linux.intel.com
And indeed the parameter passes an array of uint64_t rather than an array of structs, but the first words of the comment suggest that it passes
Signed-off-by: Andrzej Pietrasiewicz andrzej.p@collabora.com
drivers/gpu/drm/drm_plane.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c index d6ad60ab0d38..0d4f9172c0dd 100644 --- a/drivers/gpu/drm/drm_plane.c +++ b/drivers/gpu/drm/drm_plane.c @@ -160,7 +160,7 @@ static int create_in_format_blob(struct drm_device *dev, struct drm_plane *plane
- @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
- @format_modifiers: array of format modifiers terminated by
DRM_FORMAT_MOD_INVALID- @type: type of plane (overlay, primary, cursor)
- @name: printf style format string for the plane name, or NULL for default name
-- 2.17.1
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
The parameter passes an array of uint64_t rather than an array of structs, but the first words of the comment suggest that it passes an array of structs - if the reader stops reading at the word "struct".
If the commit is read beyond that point the reader will likely confuse "drm_format<SPACE>modifiers" with "drm_format<UNDERSCORE>modifiers" and understand the meaning as "passing an array of elements which are of type struct drm_format_modifier". That is not correct.
Only if the reader is able to read the comment as "array of struct drm_format<SPACE>modifiers" will they be close to the correct meaning. But still not quite there, because the modifiers do not influence struct drm_format in any way - it is not clear what "a modifier of a struct" would be.
The comment is changed to simply say that the parameter passes an array of format modifiers, which is the correct meaning.
Signed-off-by: Andrzej Pietrasiewicz andrzej.p@collabora.com Reviewed-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/drm_plane.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c index d6ad60ab0d38..0d4f9172c0dd 100644 --- a/drivers/gpu/drm/drm_plane.c +++ b/drivers/gpu/drm/drm_plane.c @@ -160,7 +160,7 @@ static int create_in_format_blob(struct drm_device *dev, struct drm_plane *plane * @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 + * @format_modifiers: array of format modifiers terminated by * DRM_FORMAT_MOD_INVALID * @type: type of plane (overlay, primary, cursor) * @name: printf style format string for the plane name, or NULL for default name
dri-devel@lists.freedesktop.org
-
Andrzej Pietrasiewicz -
Eric Engestrom -
Sam Ravnborg -
Ville Syrjälä