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 <[email protected]>
---
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
--
2.17.1
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 <[email protected]>
> ---
> 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
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Ville Syrj?l?
Intel
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 <[email protected]>
> ---
> 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 <[email protected]>
---
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
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? <[email protected]>
>
> 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 <[email protected]>
> ---
> 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
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Ville Syrj?l?
Intel
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ä <[email protected]>
>
> >
> > 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 <[email protected]>
> > ---
> > 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
> > [email protected]
> > 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? <[email protected]>
> >
> > >
> > > 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 <[email protected]>
> > > ---
> > > 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
> > > [email protected]
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
--
Ville Syrj?l?
Intel
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 <[email protected]>
Reviewed-by: Ville Syrjälä <[email protected]>
---
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