[PATCH] drm/amd/display: Pass app_tf by value rather than by reference

[PATCH] drm/amd/display: Pass app_tf by value rather than by reference

[Nathan Chancellor]

Clang warns when an expression that equals zero is used as a null
pointer constant (in lieu of NULL):

drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:4435:3:
warning: expression which evaluates to zero treated as a null pointer
constant of type 'const enum color_transfer_func *'
[-Wnon-literal-null-conversion]
                TRANSFER_FUNC_UNKNOWN,
                ^~~~~~~~~~~~~~~~~~~~~
1 warning generated.

This warning is caused by commit bb47de736661 ("drm/amdgpu: Set FreeSync
state using drm VRR properties") and it could be solved by using NULL
instead of TRANSFER_FUNC_UNKNOWN or casting TRANSFER_FUNC_UNKNOWN as a
pointer. However, after looking into it, there doesn't appear to be a
good reason to pass app_tf by reference as it is never mutated along the
way. This is the only code path in which app_tf is used:

mod_freesync_build_vrr_infopacket ->
    build_vrr_infopacket_v2 ->
        build_vrr_infopacket_fs2_data

Neither mod_freesync_build_vrr_infopacket or build_vrr_infopacket_v2
modify app_tf's value and build_vrr_infopacket_fs2_data expects just
the value so we can avoid dereferencing anything by just passing in
app_tf's value to mod_freesync_build_vrr_infopacket and
build_vrr_infopacket_v2.

There is no functional change because build_vrr_infopacket_fs2_data
doesn't do anything if TRANSFER_FUNC_UNKNOWN is passed to it, the same
as not calling build_vrr_infopacket_fs2_data at all like before this
change when NULL was used for app_tf.

Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
---
 drivers/gpu/drm/amd/display/modules/freesync/freesync.c | 7 +++----
 drivers/gpu/drm/amd/display/modules/inc/mod_freesync.h  | 2 +-
 2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/modules/freesync/freesync.c b/drivers/gpu/drm/amd/display/modules/freesync/freesync.c
index 620a171620ee..520665a9d81a 100644
--- a/drivers/gpu/drm/amd/display/modules/freesync/freesync.c
+++ b/drivers/gpu/drm/amd/display/modules/freesync/freesync.c
@@ -656,7 +656,7 @@ static void build_vrr_infopacket_v1(enum signal_type signal,
 
 static void build_vrr_infopacket_v2(enum signal_type signal,
 		const struct mod_vrr_params *vrr,
-		const enum color_transfer_func *app_tf,
+		enum color_transfer_func app_tf,
 		struct dc_info_packet *infopacket)
 {
 	unsigned int payload_size = 0;
@@ -664,8 +664,7 @@ static void build_vrr_infopacket_v2(enum signal_type signal,
 	build_vrr_infopacket_header_v2(signal, infopacket, &payload_size);
 	build_vrr_infopacket_data(vrr, infopacket);
 
-	if (app_tf != NULL)
-		build_vrr_infopacket_fs2_data(*app_tf, infopacket);
+	build_vrr_infopacket_fs2_data(app_tf, infopacket);
 
 	build_vrr_infopacket_checksum(&payload_size, infopacket);
 
@@ -676,7 +675,7 @@ void mod_freesync_build_vrr_infopacket(struct mod_freesync *mod_freesync,
 		const struct dc_stream_state *stream,
 		const struct mod_vrr_params *vrr,
 		enum vrr_packet_type packet_type,
-		const enum color_transfer_func *app_tf,
+		enum color_transfer_func app_tf,
 		struct dc_info_packet *infopacket)
 {
 	/* SPD info packet for FreeSync */
diff --git a/drivers/gpu/drm/amd/display/modules/inc/mod_freesync.h b/drivers/gpu/drm/amd/display/modules/inc/mod_freesync.h
index 949a8b62aa98..063af6258fd9 100644
--- a/drivers/gpu/drm/amd/display/modules/inc/mod_freesync.h
+++ b/drivers/gpu/drm/amd/display/modules/inc/mod_freesync.h
@@ -145,7 +145,7 @@ void mod_freesync_build_vrr_infopacket(struct mod_freesync *mod_freesync,
 		const struct dc_stream_state *stream,
 		const struct mod_vrr_params *vrr,
 		enum vrr_packet_type packet_type,
-		const enum color_transfer_func *app_tf,
+		enum color_transfer_func app_tf,
 		struct dc_info_packet *infopacket);
 
 void mod_freesync_build_vrr_params(struct mod_freesync *mod_freesync,
-- 
2.20.0

Re: [PATCH] drm/amd/display: Pass app_tf by value rather than by reference

[Nick Desaulniers]

On Mon, Dec 10, 2018 at 3:42 PM Nathan Chancellor
<natechancellor@gmail.com> wrote:
>
> Clang warns when an expression that equals zero is used as a null
> pointer constant (in lieu of NULL):
>
> drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:4435:3:
> warning: expression which evaluates to zero treated as a null pointer
> constant of type 'const enum color_transfer_func *'
> [-Wnon-literal-null-conversion]
>                 TRANSFER_FUNC_UNKNOWN,
>                 ^~~~~~~~~~~~~~~~~~~~~
> 1 warning generated.
>
> This warning is caused by commit bb47de736661 ("drm/amdgpu: Set FreeSync
> state using drm VRR properties") and it could be solved by using NULL
> instead of TRANSFER_FUNC_UNKNOWN or casting TRANSFER_FUNC_UNKNOWN as a
> pointer. However, after looking into it, there doesn't appear to be a
> good reason to pass app_tf by reference as it is never mutated along the
> way. This is the only code path in which app_tf is used:
>
> mod_freesync_build_vrr_infopacket ->
>     build_vrr_infopacket_v2 ->
>         build_vrr_infopacket_fs2_data
>
> Neither mod_freesync_build_vrr_infopacket or build_vrr_infopacket_v2
> modify app_tf's value and build_vrr_infopacket_fs2_data expects just
> the value so we can avoid dereferencing anything by just passing in
> app_tf's value to mod_freesync_build_vrr_infopacket and
> build_vrr_infopacket_v2.
>
> There is no functional change because build_vrr_infopacket_fs2_data
> doesn't do anything if TRANSFER_FUNC_UNKNOWN is passed to it, the same
> as not calling build_vrr_infopacket_fs2_data at all like before this
> change when NULL was used for app_tf.

Nathan,
Thanks for sending this patch.  I was hoping to provide review sooner,
but have been quite busy lately.

Yeah, especially for LP64 targets, the pointer to enum is larger than
just the enum, and if it's not being updated ("in/out paramter")
there's no need to pass by pointer.

>
> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> ---
>  drivers/gpu/drm/amd/display/modules/freesync/freesync.c | 7 +++----
>  drivers/gpu/drm/amd/display/modules/inc/mod_freesync.h  | 2 +-
>  2 files changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/modules/freesync/freesync.c b/drivers/gpu/drm/amd/display/modules/freesync/freesync.c
> index 620a171620ee..520665a9d81a 100644
> --- a/drivers/gpu/drm/amd/display/modules/freesync/freesync.c
> +++ b/drivers/gpu/drm/amd/display/modules/freesync/freesync.c
> @@ -656,7 +656,7 @@ static void build_vrr_infopacket_v1(enum signal_type signal,
>
>  static void build_vrr_infopacket_v2(enum signal_type signal,
>                 const struct mod_vrr_params *vrr,
> -               const enum color_transfer_func *app_tf,
> +               enum color_transfer_func app_tf,
>                 struct dc_info_packet *infopacket)
>  {
>         unsigned int payload_size = 0;
> @@ -664,8 +664,7 @@ static void build_vrr_infopacket_v2(enum signal_type signal,
>         build_vrr_infopacket_header_v2(signal, infopacket, &payload_size);
>         build_vrr_infopacket_data(vrr, infopacket);
>
> -       if (app_tf != NULL)
> -               build_vrr_infopacket_fs2_data(*app_tf, infopacket);
> +       build_vrr_infopacket_fs2_data(app_tf, infopacket);
>
>         build_vrr_infopacket_checksum(&payload_size, infopacket);
>
> @@ -676,7 +675,7 @@ void mod_freesync_build_vrr_infopacket(struct mod_freesync *mod_freesync,
>                 const struct dc_stream_state *stream,
>                 const struct mod_vrr_params *vrr,
>                 enum vrr_packet_type packet_type,
> -               const enum color_transfer_func *app_tf,
> +               enum color_transfer_func app_tf,
>                 struct dc_info_packet *infopacket)
>  {
>         /* SPD info packet for FreeSync */
> diff --git a/drivers/gpu/drm/amd/display/modules/inc/mod_freesync.h b/drivers/gpu/drm/amd/display/modules/inc/mod_freesync.h
> index 949a8b62aa98..063af6258fd9 100644
> --- a/drivers/gpu/drm/amd/display/modules/inc/mod_freesync.h
> +++ b/drivers/gpu/drm/amd/display/modules/inc/mod_freesync.h
> @@ -145,7 +145,7 @@ void mod_freesync_build_vrr_infopacket(struct mod_freesync *mod_freesync,
>                 const struct dc_stream_state *stream,
>                 const struct mod_vrr_params *vrr,
>                 enum vrr_packet_type packet_type,
> -               const enum color_transfer_func *app_tf,
> +               enum color_transfer_func app_tf,

Don't you need to update the callsite of `mod_freesync_build_vrr_infopacket` in

drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c#4949:

-                                           NULL,
+                                          transfer_func_unknown,

Maybe at that point the `if (app_tf != NULL)` could be replaced with
`if (app_tf != transfer_func_unknown)` hoisted from
`build_vrr_infopacket_fs2_data`? (There's only one caller of
`build_vrr_infopacket_fs2_data` today, maybe fine to leave the
unconditional call and check).

Either way, I suspect without the change to
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c, that this fails to
compile?
-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] drm/amd/display: Pass app_tf by value rather than by reference

[Nathan Chancellor]

On Tue, Dec 11, 2018 at 01:25:00PM -0800, Nick Desaulniers wrote:
> On Mon, Dec 10, 2018 at 3:42 PM Nathan Chancellor
> <natechancellor@gmail.com> wrote:
> >
> > Clang warns when an expression that equals zero is used as a null
> > pointer constant (in lieu of NULL):
> >
> > drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:4435:3:
> > warning: expression which evaluates to zero treated as a null pointer
> > constant of type 'const enum color_transfer_func *'
> > [-Wnon-literal-null-conversion]
> >                 TRANSFER_FUNC_UNKNOWN,
> >                 ^~~~~~~~~~~~~~~~~~~~~
> > 1 warning generated.
> >
> > This warning is caused by commit bb47de736661 ("drm/amdgpu: Set FreeSync
> > state using drm VRR properties") and it could be solved by using NULL
> > instead of TRANSFER_FUNC_UNKNOWN or casting TRANSFER_FUNC_UNKNOWN as a
> > pointer. However, after looking into it, there doesn't appear to be a
> > good reason to pass app_tf by reference as it is never mutated along the
> > way. This is the only code path in which app_tf is used:
> >
> > mod_freesync_build_vrr_infopacket ->
> >     build_vrr_infopacket_v2 ->
> >         build_vrr_infopacket_fs2_data
> >
> > Neither mod_freesync_build_vrr_infopacket or build_vrr_infopacket_v2
> > modify app_tf's value and build_vrr_infopacket_fs2_data expects just
> > the value so we can avoid dereferencing anything by just passing in
> > app_tf's value to mod_freesync_build_vrr_infopacket and
> > build_vrr_infopacket_v2.
> >
> > There is no functional change because build_vrr_infopacket_fs2_data
> > doesn't do anything if TRANSFER_FUNC_UNKNOWN is passed to it, the same
> > as not calling build_vrr_infopacket_fs2_data at all like before this
> > change when NULL was used for app_tf.
> 
> Nathan,
> Thanks for sending this patch.  I was hoping to provide review sooner,
> but have been quite busy lately.
> 

Late review is better than no review, I appeciate you taking the time to
do this!

> Yeah, especially for LP64 targets, the pointer to enum is larger than
> just the enum, and if it's not being updated ("in/out paramter")
> there's no need to pass by pointer.
> 

Thanks for confirming!

> >
> > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> > ---
> >  drivers/gpu/drm/amd/display/modules/freesync/freesync.c | 7 +++----
> >  drivers/gpu/drm/amd/display/modules/inc/mod_freesync.h  | 2 +-
> >  2 files changed, 4 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/display/modules/freesync/freesync.c b/drivers/gpu/drm/amd/display/modules/freesync/freesync.c
> > index 620a171620ee..520665a9d81a 100644
> > --- a/drivers/gpu/drm/amd/display/modules/freesync/freesync.c
> > +++ b/drivers/gpu/drm/amd/display/modules/freesync/freesync.c
> > @@ -656,7 +656,7 @@ static void build_vrr_infopacket_v1(enum signal_type signal,
> >
> >  static void build_vrr_infopacket_v2(enum signal_type signal,
> >                 const struct mod_vrr_params *vrr,
> > -               const enum color_transfer_func *app_tf,
> > +               enum color_transfer_func app_tf,
> >                 struct dc_info_packet *infopacket)
> >  {
> >         unsigned int payload_size = 0;
> > @@ -664,8 +664,7 @@ static void build_vrr_infopacket_v2(enum signal_type signal,
> >         build_vrr_infopacket_header_v2(signal, infopacket, &payload_size);
> >         build_vrr_infopacket_data(vrr, infopacket);
> >
> > -       if (app_tf != NULL)
> > -               build_vrr_infopacket_fs2_data(*app_tf, infopacket);
> > +       build_vrr_infopacket_fs2_data(app_tf, infopacket);
> >
> >         build_vrr_infopacket_checksum(&payload_size, infopacket);
> >
> > @@ -676,7 +675,7 @@ void mod_freesync_build_vrr_infopacket(struct mod_freesync *mod_freesync,
> >                 const struct dc_stream_state *stream,
> >                 const struct mod_vrr_params *vrr,
> >                 enum vrr_packet_type packet_type,
> > -               const enum color_transfer_func *app_tf,
> > +               enum color_transfer_func app_tf,
> >                 struct dc_info_packet *infopacket)
> >  {
> >         /* SPD info packet for FreeSync */
> > diff --git a/drivers/gpu/drm/amd/display/modules/inc/mod_freesync.h b/drivers/gpu/drm/amd/display/modules/inc/mod_freesync.h
> > index 949a8b62aa98..063af6258fd9 100644
> > --- a/drivers/gpu/drm/amd/display/modules/inc/mod_freesync.h
> > +++ b/drivers/gpu/drm/amd/display/modules/inc/mod_freesync.h
> > @@ -145,7 +145,7 @@ void mod_freesync_build_vrr_infopacket(struct mod_freesync *mod_freesync,
> >                 const struct dc_stream_state *stream,
> >                 const struct mod_vrr_params *vrr,
> >                 enum vrr_packet_type packet_type,
> > -               const enum color_transfer_func *app_tf,
> > +               enum color_transfer_func app_tf,
> 
> Don't you need to update the callsite of `mod_freesync_build_vrr_infopacket` in
> 
> drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c#4949:
> 
> -                                           NULL,
> +                                          transfer_func_unknown,
> 

That change in commit bb47de736661 ("drm/amdgpu: Set FreeSync state
using drm VRR properties") in -next is what prompted this patch (the
warning in the commit message is not present in mainline):

        mod_freesync_build_vrr_infopacket(
                dm->freesync_module,
                new_stream,
                &vrr,
                packet_type_vrr,
                transfer_func_unknown,
                &vrr_infopacket);

> Maybe at that point the `if (app_tf != NULL)` could be replaced with
> `if (app_tf != transfer_func_unknown)` hoisted from
> `build_vrr_infopacket_fs2_data`? (There's only one caller of
> `build_vrr_infopacket_fs2_data` today, maybe fine to leave the
> unconditional call and check).
> 

Hmmm that's not unreasonable I suppose. I guess it depends on if
build_vrr_infopacket_fs2_data could ever be called from outside of
build_vrr_infopacket_v2; if it can, it makes sense to leave the
conditional check for 'app_tf != transfer_func_unknown' in
build_vrr_infopacket_fs2_data and leaving the unconditional call
to it in build_vrr_infopacket_v2 (since app_tf is no longer a pointer,
no need to check against NULL).

I'm happy to do a v2 if the maintainers feel strongly about it, thank
you for bringing that up.

> Either way, I suspect without the change to
> drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c, that this fails to
> compile?

Correct so this is targeted at -next, rather than mainline, probably
should get in the habit of adding '-next' to my patch subjects...

> -- 
> Thanks,
> ~Nick Desaulniers

Thank you for the review!
Nathan

Re: [PATCH] drm/amd/display: Pass app_tf by value rather than by reference

[Nick Desaulniers]

On Tue, Dec 11, 2018 at 1:42 PM Nathan Chancellor
<natechancellor@gmail.com> wrote:
>
> On Tue, Dec 11, 2018 at 01:25:00PM -0800, Nick Desaulniers wrote:
> > On Mon, Dec 10, 2018 at 3:42 PM Nathan Chancellor
> > <natechancellor@gmail.com> wrote:
> > >
> > > Clang warns when an expression that equals zero is used as a null
> > > pointer constant (in lieu of NULL):
> > >
> > > drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:4435:3:
> > > warning: expression which evaluates to zero treated as a null pointer
> > > constant of type 'const enum color_transfer_func *'
> > > [-Wnon-literal-null-conversion]
> > >                 TRANSFER_FUNC_UNKNOWN,
> > >                 ^~~~~~~~~~~~~~~~~~~~~
> > > 1 warning generated.
> > >
> > > This warning is caused by commit bb47de736661 ("drm/amdgpu: Set FreeSync
> > > state using drm VRR properties") and it could be solved by using NULL
> > > instead of TRANSFER_FUNC_UNKNOWN or casting TRANSFER_FUNC_UNKNOWN as a
> > > pointer. However, after looking into it, there doesn't appear to be a
> > > good reason to pass app_tf by reference as it is never mutated along the
> > > way. This is the only code path in which app_tf is used:
> > >
> > > mod_freesync_build_vrr_infopacket ->
> > >     build_vrr_infopacket_v2 ->
> > >         build_vrr_infopacket_fs2_data
> > >
> > > Neither mod_freesync_build_vrr_infopacket or build_vrr_infopacket_v2
> > > modify app_tf's value and build_vrr_infopacket_fs2_data expects just
> > > the value so we can avoid dereferencing anything by just passing in
> > > app_tf's value to mod_freesync_build_vrr_infopacket and
> > > build_vrr_infopacket_v2.
> > >
> > > There is no functional change because build_vrr_infopacket_fs2_data
> > > doesn't do anything if TRANSFER_FUNC_UNKNOWN is passed to it, the same
> > > as not calling build_vrr_infopacket_fs2_data at all like before this
> > > change when NULL was used for app_tf.
> >
> > Nathan,
> > Thanks for sending this patch.  I was hoping to provide review sooner,
> > but have been quite busy lately.
> >
>
> Late review is better than no review, I appeciate you taking the time to
> do this!
>
> > Yeah, especially for LP64 targets, the pointer to enum is larger than
> > just the enum, and if it's not being updated ("in/out paramter")
> > there's no need to pass by pointer.
> >
>
> Thanks for confirming!
>
> > >
> > > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> > > ---
> > >  drivers/gpu/drm/amd/display/modules/freesync/freesync.c | 7 +++----
> > >  drivers/gpu/drm/amd/display/modules/inc/mod_freesync.h  | 2 +-
> > >  2 files changed, 4 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/amd/display/modules/freesync/freesync.c b/drivers/gpu/drm/amd/display/modules/freesync/freesync.c
> > > index 620a171620ee..520665a9d81a 100644
> > > --- a/drivers/gpu/drm/amd/display/modules/freesync/freesync.c
> > > +++ b/drivers/gpu/drm/amd/display/modules/freesync/freesync.c
> > > @@ -656,7 +656,7 @@ static void build_vrr_infopacket_v1(enum signal_type signal,
> > >
> > >  static void build_vrr_infopacket_v2(enum signal_type signal,
> > >                 const struct mod_vrr_params *vrr,
> > > -               const enum color_transfer_func *app_tf,
> > > +               enum color_transfer_func app_tf,
> > >                 struct dc_info_packet *infopacket)
> > >  {
> > >         unsigned int payload_size = 0;
> > > @@ -664,8 +664,7 @@ static void build_vrr_infopacket_v2(enum signal_type signal,
> > >         build_vrr_infopacket_header_v2(signal, infopacket, &payload_size);
> > >         build_vrr_infopacket_data(vrr, infopacket);
> > >
> > > -       if (app_tf != NULL)
> > > -               build_vrr_infopacket_fs2_data(*app_tf, infopacket);
> > > +       build_vrr_infopacket_fs2_data(app_tf, infopacket);
> > >
> > >         build_vrr_infopacket_checksum(&payload_size, infopacket);
> > >
> > > @@ -676,7 +675,7 @@ void mod_freesync_build_vrr_infopacket(struct mod_freesync *mod_freesync,
> > >                 const struct dc_stream_state *stream,
> > >                 const struct mod_vrr_params *vrr,
> > >                 enum vrr_packet_type packet_type,
> > > -               const enum color_transfer_func *app_tf,
> > > +               enum color_transfer_func app_tf,
> > >                 struct dc_info_packet *infopacket)
> > >  {
> > >         /* SPD info packet for FreeSync */
> > > diff --git a/drivers/gpu/drm/amd/display/modules/inc/mod_freesync.h b/drivers/gpu/drm/amd/display/modules/inc/mod_freesync.h
> > > index 949a8b62aa98..063af6258fd9 100644
> > > --- a/drivers/gpu/drm/amd/display/modules/inc/mod_freesync.h
> > > +++ b/drivers/gpu/drm/amd/display/modules/inc/mod_freesync.h
> > > @@ -145,7 +145,7 @@ void mod_freesync_build_vrr_infopacket(struct mod_freesync *mod_freesync,
> > >                 const struct dc_stream_state *stream,
> > >                 const struct mod_vrr_params *vrr,
> > >                 enum vrr_packet_type packet_type,
> > > -               const enum color_transfer_func *app_tf,
> > > +               enum color_transfer_func app_tf,
> >
> > Don't you need to update the callsite of `mod_freesync_build_vrr_infopacket` in
> >
> > drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c#4949:
> >
> > -                                           NULL,
> > +                                          transfer_func_unknown,
> >
>
> That change in commit bb47de736661 ("drm/amdgpu: Set FreeSync state
> using drm VRR properties") in -next is what prompted this patch (the
> warning in the commit message is not present in mainline):

Ah! Sorry, I was looking at mainline.  I should have noticed
bb47de736661 wasn't there.

Shouldn't that change fail to compile, as transfer_func_unknown is an
`enum color_transfer_func` value, but
mod_freesync_build_vrr_infopacket takes a *pointer* to an `enum
color_transfer_func` value?
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/gpu/drm/amd/display/modules/freesync/freesync.c#n675

>
>         mod_freesync_build_vrr_infopacket(
>                 dm->freesync_module,
>                 new_stream,
>                 &vrr,
>                 packet_type_vrr,
>                 transfer_func_unknown,
>                 &vrr_infopacket);
>
> > Maybe at that point the `if (app_tf != NULL)` could be replaced with
> > `if (app_tf != transfer_func_unknown)` hoisted from
> > `build_vrr_infopacket_fs2_data`? (There's only one caller of
> > `build_vrr_infopacket_fs2_data` today, maybe fine to leave the
> > unconditional call and check).
> >
>
> Hmmm that's not unreasonable I suppose. I guess it depends on if
> build_vrr_infopacket_fs2_data could ever be called from outside of
> build_vrr_infopacket_v2; if it can, it makes sense to leave the
> conditional check for 'app_tf != transfer_func_unknown' in
> build_vrr_infopacket_fs2_data and leaving the unconditional call
> to it in build_vrr_infopacket_v2 (since app_tf is no longer a pointer,
> no need to check against NULL).
>
> I'm happy to do a v2 if the maintainers feel strongly about it, thank
> you for bringing that up.

Don't worry about it, I think it's fine.
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] drm/amd/display: Pass app_tf by value rather than by reference

[Nathan Chancellor]

On Tue, Dec 11, 2018 at 02:07:31PM -0800, Nick Desaulniers wrote:
> On Tue, Dec 11, 2018 at 1:42 PM Nathan Chancellor
> <natechancellor@gmail.com> wrote:
> >
> > On Tue, Dec 11, 2018 at 01:25:00PM -0800, Nick Desaulniers wrote:
> > > On Mon, Dec 10, 2018 at 3:42 PM Nathan Chancellor
> > > <natechancellor@gmail.com> wrote:
> > > >
> > > > Clang warns when an expression that equals zero is used as a null
> > > > pointer constant (in lieu of NULL):
> > > >
> > > > drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:4435:3:
> > > > warning: expression which evaluates to zero treated as a null pointer
> > > > constant of type 'const enum color_transfer_func *'
> > > > [-Wnon-literal-null-conversion]
> > > >                 TRANSFER_FUNC_UNKNOWN,
> > > >                 ^~~~~~~~~~~~~~~~~~~~~
> > > > 1 warning generated.
> > > >
> > > > This warning is caused by commit bb47de736661 ("drm/amdgpu: Set FreeSync
> > > > state using drm VRR properties") and it could be solved by using NULL
> > > > instead of TRANSFER_FUNC_UNKNOWN or casting TRANSFER_FUNC_UNKNOWN as a
> > > > pointer. However, after looking into it, there doesn't appear to be a
> > > > good reason to pass app_tf by reference as it is never mutated along the
> > > > way. This is the only code path in which app_tf is used:
> > > >
> > > > mod_freesync_build_vrr_infopacket ->
> > > >     build_vrr_infopacket_v2 ->
> > > >         build_vrr_infopacket_fs2_data
> > > >
> > > > Neither mod_freesync_build_vrr_infopacket or build_vrr_infopacket_v2
> > > > modify app_tf's value and build_vrr_infopacket_fs2_data expects just
> > > > the value so we can avoid dereferencing anything by just passing in
> > > > app_tf's value to mod_freesync_build_vrr_infopacket and
> > > > build_vrr_infopacket_v2.
> > > >
> > > > There is no functional change because build_vrr_infopacket_fs2_data
> > > > doesn't do anything if TRANSFER_FUNC_UNKNOWN is passed to it, the same
> > > > as not calling build_vrr_infopacket_fs2_data at all like before this
> > > > change when NULL was used for app_tf.
> > >
> > > Nathan,
> > > Thanks for sending this patch.  I was hoping to provide review sooner,
> > > but have been quite busy lately.
> > >
> >
> > Late review is better than no review, I appeciate you taking the time to
> > do this!
> >
> > > Yeah, especially for LP64 targets, the pointer to enum is larger than
> > > just the enum, and if it's not being updated ("in/out paramter")
> > > there's no need to pass by pointer.
> > >
> >
> > Thanks for confirming!
> >
> > > >
> > > > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> > > > ---
> > > >  drivers/gpu/drm/amd/display/modules/freesync/freesync.c | 7 +++----
> > > >  drivers/gpu/drm/amd/display/modules/inc/mod_freesync.h  | 2 +-
> > > >  2 files changed, 4 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/amd/display/modules/freesync/freesync.c b/drivers/gpu/drm/amd/display/modules/freesync/freesync.c
> > > > index 620a171620ee..520665a9d81a 100644
> > > > --- a/drivers/gpu/drm/amd/display/modules/freesync/freesync.c
> > > > +++ b/drivers/gpu/drm/amd/display/modules/freesync/freesync.c
> > > > @@ -656,7 +656,7 @@ static void build_vrr_infopacket_v1(enum signal_type signal,
> > > >
> > > >  static void build_vrr_infopacket_v2(enum signal_type signal,
> > > >                 const struct mod_vrr_params *vrr,
> > > > -               const enum color_transfer_func *app_tf,
> > > > +               enum color_transfer_func app_tf,
> > > >                 struct dc_info_packet *infopacket)
> > > >  {
> > > >         unsigned int payload_size = 0;
> > > > @@ -664,8 +664,7 @@ static void build_vrr_infopacket_v2(enum signal_type signal,
> > > >         build_vrr_infopacket_header_v2(signal, infopacket, &payload_size);
> > > >         build_vrr_infopacket_data(vrr, infopacket);
> > > >
> > > > -       if (app_tf != NULL)
> > > > -               build_vrr_infopacket_fs2_data(*app_tf, infopacket);
> > > > +       build_vrr_infopacket_fs2_data(app_tf, infopacket);
> > > >
> > > >         build_vrr_infopacket_checksum(&payload_size, infopacket);
> > > >
> > > > @@ -676,7 +675,7 @@ void mod_freesync_build_vrr_infopacket(struct mod_freesync *mod_freesync,
> > > >                 const struct dc_stream_state *stream,
> > > >                 const struct mod_vrr_params *vrr,
> > > >                 enum vrr_packet_type packet_type,
> > > > -               const enum color_transfer_func *app_tf,
> > > > +               enum color_transfer_func app_tf,
> > > >                 struct dc_info_packet *infopacket)
> > > >  {
> > > >         /* SPD info packet for FreeSync */
> > > > diff --git a/drivers/gpu/drm/amd/display/modules/inc/mod_freesync.h b/drivers/gpu/drm/amd/display/modules/inc/mod_freesync.h
> > > > index 949a8b62aa98..063af6258fd9 100644
> > > > --- a/drivers/gpu/drm/amd/display/modules/inc/mod_freesync.h
> > > > +++ b/drivers/gpu/drm/amd/display/modules/inc/mod_freesync.h
> > > > @@ -145,7 +145,7 @@ void mod_freesync_build_vrr_infopacket(struct mod_freesync *mod_freesync,
> > > >                 const struct dc_stream_state *stream,
> > > >                 const struct mod_vrr_params *vrr,
> > > >                 enum vrr_packet_type packet_type,
> > > > -               const enum color_transfer_func *app_tf,
> > > > +               enum color_transfer_func app_tf,
> > >
> > > Don't you need to update the callsite of `mod_freesync_build_vrr_infopacket` in
> > >
> > > drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c#4949:
> > >
> > > -                                           NULL,
> > > +                                          transfer_func_unknown,
> > >
> >
> > That change in commit bb47de736661 ("drm/amdgpu: Set FreeSync state
> > using drm VRR properties") in -next is what prompted this patch (the
> > warning in the commit message is not present in mainline):
> 
> Ah! Sorry, I was looking at mainline.  I should have noticed
> bb47de736661 wasn't there.
> 
> Shouldn't that change fail to compile, as transfer_func_unknown is an
> `enum color_transfer_func` value, but
> mod_freesync_build_vrr_infopacket takes a *pointer* to an `enum
> color_transfer_func` value?
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/gpu/drm/amd/display/modules/freesync/freesync.c#n675
> 

Unfortunately, no. GCC does not warn like Clang does when an expression
that equals zero is passed to a function requiring a pointer (it just
silently treats it as NULL, whereas Clang audibly complains).

They both do when the expression is something other than zero.

https://godbolt.org/z/goo91G

Nathan

> >
> >         mod_freesync_build_vrr_infopacket(
> >                 dm->freesync_module,
> >                 new_stream,
> >                 &vrr,
> >                 packet_type_vrr,
> >                 transfer_func_unknown,
> >                 &vrr_infopacket);
> >
> > > Maybe at that point the `if (app_tf != NULL)` could be replaced with
> > > `if (app_tf != transfer_func_unknown)` hoisted from
> > > `build_vrr_infopacket_fs2_data`? (There's only one caller of
> > > `build_vrr_infopacket_fs2_data` today, maybe fine to leave the
> > > unconditional call and check).
> > >
> >
> > Hmmm that's not unreasonable I suppose. I guess it depends on if
> > build_vrr_infopacket_fs2_data could ever be called from outside of
> > build_vrr_infopacket_v2; if it can, it makes sense to leave the
> > conditional check for 'app_tf != transfer_func_unknown' in
> > build_vrr_infopacket_fs2_data and leaving the unconditional call
> > to it in build_vrr_infopacket_v2 (since app_tf is no longer a pointer,
> > no need to check against NULL).
> >
> > I'm happy to do a v2 if the maintainers feel strongly about it, thank
> > you for bringing that up.
> 
> Don't worry about it, I think it's fine.
> Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
> 
> -- 
> Thanks,
> ~Nick Desaulniers

Re: [PATCH] drm/amd/display: Pass app_tf by value rather than by reference

[Wentland, Harry]

On 2018-12-11 5:07 p.m., Nick Desaulniers wrote:
> On Tue, Dec 11, 2018 at 1:42 PM Nathan Chancellor
> <natechancellor@gmail.com> wrote:
>>
>> On Tue, Dec 11, 2018 at 01:25:00PM -0800, Nick Desaulniers wrote:
>>> On Mon, Dec 10, 2018 at 3:42 PM Nathan Chancellor
>>> <natechancellor@gmail.com> wrote:
>>>>
>>>> Clang warns when an expression that equals zero is used as a null
>>>> pointer constant (in lieu of NULL):
>>>>
>>>> drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:4435:3:
>>>> warning: expression which evaluates to zero treated as a null pointer
>>>> constant of type 'const enum color_transfer_func *'
>>>> [-Wnon-literal-null-conversion]
>>>>                 TRANSFER_FUNC_UNKNOWN,
>>>>                 ^~~~~~~~~~~~~~~~~~~~~
>>>> 1 warning generated.
>>>>
>>>> This warning is caused by commit bb47de736661 ("drm/amdgpu: Set FreeSync
>>>> state using drm VRR properties") and it could be solved by using NULL
>>>> instead of TRANSFER_FUNC_UNKNOWN or casting TRANSFER_FUNC_UNKNOWN as a
>>>> pointer. However, after looking into it, there doesn't appear to be a
>>>> good reason to pass app_tf by reference as it is never mutated along the
>>>> way. This is the only code path in which app_tf is used:
>>>>
>>>> mod_freesync_build_vrr_infopacket ->
>>>>     build_vrr_infopacket_v2 ->
>>>>         build_vrr_infopacket_fs2_data
>>>>
>>>> Neither mod_freesync_build_vrr_infopacket or build_vrr_infopacket_v2
>>>> modify app_tf's value and build_vrr_infopacket_fs2_data expects just
>>>> the value so we can avoid dereferencing anything by just passing in
>>>> app_tf's value to mod_freesync_build_vrr_infopacket and
>>>> build_vrr_infopacket_v2.
>>>>
>>>> There is no functional change because build_vrr_infopacket_fs2_data
>>>> doesn't do anything if TRANSFER_FUNC_UNKNOWN is passed to it, the same
>>>> as not calling build_vrr_infopacket_fs2_data at all like before this
>>>> change when NULL was used for app_tf.
>>>
>>> Nathan,
>>> Thanks for sending this patch.  I was hoping to provide review sooner,
>>> but have been quite busy lately.
>>>
>>
>> Late review is better than no review, I appeciate you taking the time to
>> do this!
>>
>>> Yeah, especially for LP64 targets, the pointer to enum is larger than
>>> just the enum, and if it's not being updated ("in/out paramter")
>>> there's no need to pass by pointer.
>>>
>>
>> Thanks for confirming!
>>
>>>>
>>>> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
>>>> ---
>>>>  drivers/gpu/drm/amd/display/modules/freesync/freesync.c | 7 +++----
>>>>  drivers/gpu/drm/amd/display/modules/inc/mod_freesync.h  | 2 +-
>>>>  2 files changed, 4 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/display/modules/freesync/freesync.c b/drivers/gpu/drm/amd/display/modules/freesync/freesync.c
>>>> index 620a171620ee..520665a9d81a 100644
>>>> --- a/drivers/gpu/drm/amd/display/modules/freesync/freesync.c
>>>> +++ b/drivers/gpu/drm/amd/display/modules/freesync/freesync.c
>>>> @@ -656,7 +656,7 @@ static void build_vrr_infopacket_v1(enum signal_type signal,
>>>>
>>>>  static void build_vrr_infopacket_v2(enum signal_type signal,
>>>>                 const struct mod_vrr_params *vrr,
>>>> -               const enum color_transfer_func *app_tf,
>>>> +               enum color_transfer_func app_tf,
>>>>                 struct dc_info_packet *infopacket)
>>>>  {
>>>>         unsigned int payload_size = 0;
>>>> @@ -664,8 +664,7 @@ static void build_vrr_infopacket_v2(enum signal_type signal,
>>>>         build_vrr_infopacket_header_v2(signal, infopacket, &payload_size);
>>>>         build_vrr_infopacket_data(vrr, infopacket);
>>>>
>>>> -       if (app_tf != NULL)
>>>> -               build_vrr_infopacket_fs2_data(*app_tf, infopacket);
>>>> +       build_vrr_infopacket_fs2_data(app_tf, infopacket);
>>>>
>>>>         build_vrr_infopacket_checksum(&payload_size, infopacket);
>>>>
>>>> @@ -676,7 +675,7 @@ void mod_freesync_build_vrr_infopacket(struct mod_freesync *mod_freesync,
>>>>                 const struct dc_stream_state *stream,
>>>>                 const struct mod_vrr_params *vrr,
>>>>                 enum vrr_packet_type packet_type,
>>>> -               const enum color_transfer_func *app_tf,
>>>> +               enum color_transfer_func app_tf,
>>>>                 struct dc_info_packet *infopacket)
>>>>  {
>>>>         /* SPD info packet for FreeSync */
>>>> diff --git a/drivers/gpu/drm/amd/display/modules/inc/mod_freesync.h b/drivers/gpu/drm/amd/display/modules/inc/mod_freesync.h
>>>> index 949a8b62aa98..063af6258fd9 100644
>>>> --- a/drivers/gpu/drm/amd/display/modules/inc/mod_freesync.h
>>>> +++ b/drivers/gpu/drm/amd/display/modules/inc/mod_freesync.h
>>>> @@ -145,7 +145,7 @@ void mod_freesync_build_vrr_infopacket(struct mod_freesync *mod_freesync,
>>>>                 const struct dc_stream_state *stream,
>>>>                 const struct mod_vrr_params *vrr,
>>>>                 enum vrr_packet_type packet_type,
>>>> -               const enum color_transfer_func *app_tf,
>>>> +               enum color_transfer_func app_tf,
>>>
>>> Don't you need to update the callsite of `mod_freesync_build_vrr_infopacket` in
>>>
>>> drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c#4949:
>>>
>>> -                                           NULL,
>>> +                                          transfer_func_unknown,
>>>
>>
>> That change in commit bb47de736661 ("drm/amdgpu: Set FreeSync state
>> using drm VRR properties") in -next is what prompted this patch (the
>> warning in the commit message is not present in mainline):
> 
> Ah! Sorry, I was looking at mainline.  I should have noticed
> bb47de736661 wasn't there.
> 
> Shouldn't that change fail to compile, as transfer_func_unknown is an
> `enum color_transfer_func` value, but
> mod_freesync_build_vrr_infopacket takes a *pointer* to an `enum
> color_transfer_func` value?
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/gpu/drm/amd/display/modules/freesync/freesync.c#n675
> 
>>
>>         mod_freesync_build_vrr_infopacket(
>>                 dm->freesync_module,
>>                 new_stream,
>>                 &vrr,
>>                 packet_type_vrr,
>>                 transfer_func_unknown,
>>                 &vrr_infopacket);
>>
>>> Maybe at that point the `if (app_tf != NULL)` could be replaced with
>>> `if (app_tf != transfer_func_unknown)` hoisted from
>>> `build_vrr_infopacket_fs2_data`? (There's only one caller of
>>> `build_vrr_infopacket_fs2_data` today, maybe fine to leave the
>>> unconditional call and check).
>>>
>>
>> Hmmm that's not unreasonable I suppose. I guess it depends on if
>> build_vrr_infopacket_fs2_data could ever be called from outside of
>> build_vrr_infopacket_v2; if it can, it makes sense to leave the
>> conditional check for 'app_tf != transfer_func_unknown' in
>> build_vrr_infopacket_fs2_data and leaving the unconditional call
>> to it in build_vrr_infopacket_v2 (since app_tf is no longer a pointer,
>> no need to check against NULL).
>>
>> I'm happy to do a v2 if the maintainers feel strongly about it, thank
>> you for bringing that up.
> 
> Don't worry about it, I think it's fine.
> Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
> 

Sorry for being late to the game. FWIW, code is also
Reviewed-by: Harry Wentland <harry.wentland@amd.com>

We can change the comparison separately later.

Not sure why this was ever passed as a pointer.

Harry

Re: [PATCH] drm/amd/display: Pass app_tf by value rather than by reference

[Nathan Chancellor]

On Fri, Dec 14, 2018 at 08:09:00PM +0000, Wentland, Harry wrote:
> On 2018-12-11 5:07 p.m., Nick Desaulniers wrote:
> > On Tue, Dec 11, 2018 at 1:42 PM Nathan Chancellor
> > <natechancellor@gmail.com> wrote:
> >>
> >> On Tue, Dec 11, 2018 at 01:25:00PM -0800, Nick Desaulniers wrote:
> >>> On Mon, Dec 10, 2018 at 3:42 PM Nathan Chancellor
> >>> <natechancellor@gmail.com> wrote:
> >>>>
> >>>> Clang warns when an expression that equals zero is used as a null
> >>>> pointer constant (in lieu of NULL):
> >>>>
> >>>> drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:4435:3:
> >>>> warning: expression which evaluates to zero treated as a null pointer
> >>>> constant of type 'const enum color_transfer_func *'
> >>>> [-Wnon-literal-null-conversion]
> >>>>                 TRANSFER_FUNC_UNKNOWN,
> >>>>                 ^~~~~~~~~~~~~~~~~~~~~
> >>>> 1 warning generated.
> >>>>
> >>>> This warning is caused by commit bb47de736661 ("drm/amdgpu: Set FreeSync
> >>>> state using drm VRR properties") and it could be solved by using NULL
> >>>> instead of TRANSFER_FUNC_UNKNOWN or casting TRANSFER_FUNC_UNKNOWN as a
> >>>> pointer. However, after looking into it, there doesn't appear to be a
> >>>> good reason to pass app_tf by reference as it is never mutated along the
> >>>> way. This is the only code path in which app_tf is used:
> >>>>
> >>>> mod_freesync_build_vrr_infopacket ->
> >>>>     build_vrr_infopacket_v2 ->
> >>>>         build_vrr_infopacket_fs2_data
> >>>>
> >>>> Neither mod_freesync_build_vrr_infopacket or build_vrr_infopacket_v2
> >>>> modify app_tf's value and build_vrr_infopacket_fs2_data expects just
> >>>> the value so we can avoid dereferencing anything by just passing in
> >>>> app_tf's value to mod_freesync_build_vrr_infopacket and
> >>>> build_vrr_infopacket_v2.
> >>>>
> >>>> There is no functional change because build_vrr_infopacket_fs2_data
> >>>> doesn't do anything if TRANSFER_FUNC_UNKNOWN is passed to it, the same
> >>>> as not calling build_vrr_infopacket_fs2_data at all like before this
> >>>> change when NULL was used for app_tf.
> >>>
> >>> Nathan,
> >>> Thanks for sending this patch.  I was hoping to provide review sooner,
> >>> but have been quite busy lately.
> >>>
> >>
> >> Late review is better than no review, I appeciate you taking the time to
> >> do this!
> >>
> >>> Yeah, especially for LP64 targets, the pointer to enum is larger than
> >>> just the enum, and if it's not being updated ("in/out paramter")
> >>> there's no need to pass by pointer.
> >>>
> >>
> >> Thanks for confirming!
> >>
> >>>>
> >>>> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> >>>> ---
> >>>>  drivers/gpu/drm/amd/display/modules/freesync/freesync.c | 7 +++----
> >>>>  drivers/gpu/drm/amd/display/modules/inc/mod_freesync.h  | 2 +-
> >>>>  2 files changed, 4 insertions(+), 5 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/amd/display/modules/freesync/freesync.c b/drivers/gpu/drm/amd/display/modules/freesync/freesync.c
> >>>> index 620a171620ee..520665a9d81a 100644
> >>>> --- a/drivers/gpu/drm/amd/display/modules/freesync/freesync.c
> >>>> +++ b/drivers/gpu/drm/amd/display/modules/freesync/freesync.c
> >>>> @@ -656,7 +656,7 @@ static void build_vrr_infopacket_v1(enum signal_type signal,
> >>>>
> >>>>  static void build_vrr_infopacket_v2(enum signal_type signal,
> >>>>                 const struct mod_vrr_params *vrr,
> >>>> -               const enum color_transfer_func *app_tf,
> >>>> +               enum color_transfer_func app_tf,
> >>>>                 struct dc_info_packet *infopacket)
> >>>>  {
> >>>>         unsigned int payload_size = 0;
> >>>> @@ -664,8 +664,7 @@ static void build_vrr_infopacket_v2(enum signal_type signal,
> >>>>         build_vrr_infopacket_header_v2(signal, infopacket, &payload_size);
> >>>>         build_vrr_infopacket_data(vrr, infopacket);
> >>>>
> >>>> -       if (app_tf != NULL)
> >>>> -               build_vrr_infopacket_fs2_data(*app_tf, infopacket);
> >>>> +       build_vrr_infopacket_fs2_data(app_tf, infopacket);
> >>>>
> >>>>         build_vrr_infopacket_checksum(&payload_size, infopacket);
> >>>>
> >>>> @@ -676,7 +675,7 @@ void mod_freesync_build_vrr_infopacket(struct mod_freesync *mod_freesync,
> >>>>                 const struct dc_stream_state *stream,
> >>>>                 const struct mod_vrr_params *vrr,
> >>>>                 enum vrr_packet_type packet_type,
> >>>> -               const enum color_transfer_func *app_tf,
> >>>> +               enum color_transfer_func app_tf,
> >>>>                 struct dc_info_packet *infopacket)
> >>>>  {
> >>>>         /* SPD info packet for FreeSync */
> >>>> diff --git a/drivers/gpu/drm/amd/display/modules/inc/mod_freesync.h b/drivers/gpu/drm/amd/display/modules/inc/mod_freesync.h
> >>>> index 949a8b62aa98..063af6258fd9 100644
> >>>> --- a/drivers/gpu/drm/amd/display/modules/inc/mod_freesync.h
> >>>> +++ b/drivers/gpu/drm/amd/display/modules/inc/mod_freesync.h
> >>>> @@ -145,7 +145,7 @@ void mod_freesync_build_vrr_infopacket(struct mod_freesync *mod_freesync,
> >>>>                 const struct dc_stream_state *stream,
> >>>>                 const struct mod_vrr_params *vrr,
> >>>>                 enum vrr_packet_type packet_type,
> >>>> -               const enum color_transfer_func *app_tf,
> >>>> +               enum color_transfer_func app_tf,
> >>>
> >>> Don't you need to update the callsite of `mod_freesync_build_vrr_infopacket` in
> >>>
> >>> drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c#4949:
> >>>
> >>> -                                           NULL,
> >>> +                                          transfer_func_unknown,
> >>>
> >>
> >> That change in commit bb47de736661 ("drm/amdgpu: Set FreeSync state
> >> using drm VRR properties") in -next is what prompted this patch (the
> >> warning in the commit message is not present in mainline):
> > 
> > Ah! Sorry, I was looking at mainline.  I should have noticed
> > bb47de736661 wasn't there.
> > 
> > Shouldn't that change fail to compile, as transfer_func_unknown is an
> > `enum color_transfer_func` value, but
> > mod_freesync_build_vrr_infopacket takes a *pointer* to an `enum
> > color_transfer_func` value?
> > https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/gpu/drm/amd/display/modules/freesync/freesync.c#n675
> > 
> >>
> >>         mod_freesync_build_vrr_infopacket(
> >>                 dm->freesync_module,
> >>                 new_stream,
> >>                 &vrr,
> >>                 packet_type_vrr,
> >>                 transfer_func_unknown,
> >>                 &vrr_infopacket);
> >>
> >>> Maybe at that point the `if (app_tf != NULL)` could be replaced with
> >>> `if (app_tf != transfer_func_unknown)` hoisted from
> >>> `build_vrr_infopacket_fs2_data`? (There's only one caller of
> >>> `build_vrr_infopacket_fs2_data` today, maybe fine to leave the
> >>> unconditional call and check).
> >>>
> >>
> >> Hmmm that's not unreasonable I suppose. I guess it depends on if
> >> build_vrr_infopacket_fs2_data could ever be called from outside of
> >> build_vrr_infopacket_v2; if it can, it makes sense to leave the
> >> conditional check for 'app_tf != transfer_func_unknown' in
> >> build_vrr_infopacket_fs2_data and leaving the unconditional call
> >> to it in build_vrr_infopacket_v2 (since app_tf is no longer a pointer,
> >> no need to check against NULL).
> >>
> >> I'm happy to do a v2 if the maintainers feel strongly about it, thank
> >> you for bringing that up.
> > 
> > Don't worry about it, I think it's fine.
> > Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
> > 
> 
> Sorry for being late to the game. FWIW, code is also
> Reviewed-by: Harry Wentland <harry.wentland@amd.com>
> 
> We can change the comparison separately later.
> 
> Not sure why this was ever passed as a pointer.
> 
> Harry

Thanks Harry, could you (or someone) pick this up? This warning is now in mainline.

Nathan

Re: [PATCH] drm/amd/display: Pass app_tf by value rather than by reference

[Nathan Chancellor]

On Mon, Dec 10, 2018 at 04:42:01PM -0700, Nathan Chancellor wrote:
> Clang warns when an expression that equals zero is used as a null
> pointer constant (in lieu of NULL):
> 
> drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:4435:3:
> warning: expression which evaluates to zero treated as a null pointer
> constant of type 'const enum color_transfer_func *'
> [-Wnon-literal-null-conversion]
>                 TRANSFER_FUNC_UNKNOWN,
>                 ^~~~~~~~~~~~~~~~~~~~~
> 1 warning generated.
> 
> This warning is caused by commit bb47de736661 ("drm/amdgpu: Set FreeSync
> state using drm VRR properties") and it could be solved by using NULL
> instead of TRANSFER_FUNC_UNKNOWN or casting TRANSFER_FUNC_UNKNOWN as a
> pointer. However, after looking into it, there doesn't appear to be a
> good reason to pass app_tf by reference as it is never mutated along the
> way. This is the only code path in which app_tf is used:
> 
> mod_freesync_build_vrr_infopacket ->
>     build_vrr_infopacket_v2 ->
>         build_vrr_infopacket_fs2_data
> 
> Neither mod_freesync_build_vrr_infopacket or build_vrr_infopacket_v2
> modify app_tf's value and build_vrr_infopacket_fs2_data expects just
> the value so we can avoid dereferencing anything by just passing in
> app_tf's value to mod_freesync_build_vrr_infopacket and
> build_vrr_infopacket_v2.
> 
> There is no functional change because build_vrr_infopacket_fs2_data
> doesn't do anything if TRANSFER_FUNC_UNKNOWN is passed to it, the same
> as not calling build_vrr_infopacket_fs2_data at all like before this
> change when NULL was used for app_tf.
> 
> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> ---
>  drivers/gpu/drm/amd/display/modules/freesync/freesync.c | 7 +++----
>  drivers/gpu/drm/amd/display/modules/inc/mod_freesync.h  | 2 +-
>  2 files changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/display/modules/freesync/freesync.c b/drivers/gpu/drm/amd/display/modules/freesync/freesync.c
> index 620a171620ee..520665a9d81a 100644
> --- a/drivers/gpu/drm/amd/display/modules/freesync/freesync.c
> +++ b/drivers/gpu/drm/amd/display/modules/freesync/freesync.c
> @@ -656,7 +656,7 @@ static void build_vrr_infopacket_v1(enum signal_type signal,
>  
>  static void build_vrr_infopacket_v2(enum signal_type signal,
>  		const struct mod_vrr_params *vrr,
> -		const enum color_transfer_func *app_tf,
> +		enum color_transfer_func app_tf,
>  		struct dc_info_packet *infopacket)
>  {
>  	unsigned int payload_size = 0;
> @@ -664,8 +664,7 @@ static void build_vrr_infopacket_v2(enum signal_type signal,
>  	build_vrr_infopacket_header_v2(signal, infopacket, &payload_size);
>  	build_vrr_infopacket_data(vrr, infopacket);
>  
> -	if (app_tf != NULL)
> -		build_vrr_infopacket_fs2_data(*app_tf, infopacket);
> +	build_vrr_infopacket_fs2_data(app_tf, infopacket);
>  
>  	build_vrr_infopacket_checksum(&payload_size, infopacket);
>  
> @@ -676,7 +675,7 @@ void mod_freesync_build_vrr_infopacket(struct mod_freesync *mod_freesync,
>  		const struct dc_stream_state *stream,
>  		const struct mod_vrr_params *vrr,
>  		enum vrr_packet_type packet_type,
> -		const enum color_transfer_func *app_tf,
> +		enum color_transfer_func app_tf,
>  		struct dc_info_packet *infopacket)
>  {
>  	/* SPD info packet for FreeSync */
> diff --git a/drivers/gpu/drm/amd/display/modules/inc/mod_freesync.h b/drivers/gpu/drm/amd/display/modules/inc/mod_freesync.h
> index 949a8b62aa98..063af6258fd9 100644
> --- a/drivers/gpu/drm/amd/display/modules/inc/mod_freesync.h
> +++ b/drivers/gpu/drm/amd/display/modules/inc/mod_freesync.h
> @@ -145,7 +145,7 @@ void mod_freesync_build_vrr_infopacket(struct mod_freesync *mod_freesync,
>  		const struct dc_stream_state *stream,
>  		const struct mod_vrr_params *vrr,
>  		enum vrr_packet_type packet_type,
> -		const enum color_transfer_func *app_tf,
> +		enum color_transfer_func app_tf,
>  		struct dc_info_packet *infopacket);
>  
>  void mod_freesync_build_vrr_params(struct mod_freesync *mod_freesync,
> -- 
> 2.20.0
> 

Gentle ping on this patch, it would be nice to get this fixed and into
5.1!

Thanks,
Nathan

Re: [PATCH] drm/amd/display: Pass app_tf by value rather than by reference

[Alex Deucher]

Applied.  Sorry for the delay.

Alex

On Fri, Feb 22, 2019 at 10:36 AM Nathan Chancellor
<natechancellor@gmail.com> wrote:
>
> On Mon, Dec 10, 2018 at 04:42:01PM -0700, Nathan Chancellor wrote:
> > Clang warns when an expression that equals zero is used as a null
> > pointer constant (in lieu of NULL):
> >
> > drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:4435:3:
> > warning: expression which evaluates to zero treated as a null pointer
> > constant of type 'const enum color_transfer_func *'
> > [-Wnon-literal-null-conversion]
> >                 TRANSFER_FUNC_UNKNOWN,
> >                 ^~~~~~~~~~~~~~~~~~~~~
> > 1 warning generated.
> >
> > This warning is caused by commit bb47de736661 ("drm/amdgpu: Set FreeSync
> > state using drm VRR properties") and it could be solved by using NULL
> > instead of TRANSFER_FUNC_UNKNOWN or casting TRANSFER_FUNC_UNKNOWN as a
> > pointer. However, after looking into it, there doesn't appear to be a
> > good reason to pass app_tf by reference as it is never mutated along the
> > way. This is the only code path in which app_tf is used:
> >
> > mod_freesync_build_vrr_infopacket ->
> >     build_vrr_infopacket_v2 ->
> >         build_vrr_infopacket_fs2_data
> >
> > Neither mod_freesync_build_vrr_infopacket or build_vrr_infopacket_v2
> > modify app_tf's value and build_vrr_infopacket_fs2_data expects just
> > the value so we can avoid dereferencing anything by just passing in
> > app_tf's value to mod_freesync_build_vrr_infopacket and
> > build_vrr_infopacket_v2.
> >
> > There is no functional change because build_vrr_infopacket_fs2_data
> > doesn't do anything if TRANSFER_FUNC_UNKNOWN is passed to it, the same
> > as not calling build_vrr_infopacket_fs2_data at all like before this
> > change when NULL was used for app_tf.
> >
> > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> > ---
> >  drivers/gpu/drm/amd/display/modules/freesync/freesync.c | 7 +++----
> >  drivers/gpu/drm/amd/display/modules/inc/mod_freesync.h  | 2 +-
> >  2 files changed, 4 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/display/modules/freesync/freesync.c b/drivers/gpu/drm/amd/display/modules/freesync/freesync.c
> > index 620a171620ee..520665a9d81a 100644
> > --- a/drivers/gpu/drm/amd/display/modules/freesync/freesync.c
> > +++ b/drivers/gpu/drm/amd/display/modules/freesync/freesync.c
> > @@ -656,7 +656,7 @@ static void build_vrr_infopacket_v1(enum signal_type signal,
> >
> >  static void build_vrr_infopacket_v2(enum signal_type signal,
> >               const struct mod_vrr_params *vrr,
> > -             const enum color_transfer_func *app_tf,
> > +             enum color_transfer_func app_tf,
> >               struct dc_info_packet *infopacket)
> >  {
> >       unsigned int payload_size = 0;
> > @@ -664,8 +664,7 @@ static void build_vrr_infopacket_v2(enum signal_type signal,
> >       build_vrr_infopacket_header_v2(signal, infopacket, &payload_size);
> >       build_vrr_infopacket_data(vrr, infopacket);
> >
> > -     if (app_tf != NULL)
> > -             build_vrr_infopacket_fs2_data(*app_tf, infopacket);
> > +     build_vrr_infopacket_fs2_data(app_tf, infopacket);
> >
> >       build_vrr_infopacket_checksum(&payload_size, infopacket);
> >
> > @@ -676,7 +675,7 @@ void mod_freesync_build_vrr_infopacket(struct mod_freesync *mod_freesync,
> >               const struct dc_stream_state *stream,
> >               const struct mod_vrr_params *vrr,
> >               enum vrr_packet_type packet_type,
> > -             const enum color_transfer_func *app_tf,
> > +             enum color_transfer_func app_tf,
> >               struct dc_info_packet *infopacket)
> >  {
> >       /* SPD info packet for FreeSync */
> > diff --git a/drivers/gpu/drm/amd/display/modules/inc/mod_freesync.h b/drivers/gpu/drm/amd/display/modules/inc/mod_freesync.h
> > index 949a8b62aa98..063af6258fd9 100644
> > --- a/drivers/gpu/drm/amd/display/modules/inc/mod_freesync.h
> > +++ b/drivers/gpu/drm/amd/display/modules/inc/mod_freesync.h
> > @@ -145,7 +145,7 @@ void mod_freesync_build_vrr_infopacket(struct mod_freesync *mod_freesync,
> >               const struct dc_stream_state *stream,
> >               const struct mod_vrr_params *vrr,
> >               enum vrr_packet_type packet_type,
> > -             const enum color_transfer_func *app_tf,
> > +             enum color_transfer_func app_tf,
> >               struct dc_info_packet *infopacket);
> >
> >  void mod_freesync_build_vrr_params(struct mod_freesync *mod_freesync,
> > --
> > 2.20.0
> >
>
> Gentle ping on this patch, it would be nice to get this fixed and into
> 5.1!
>
> Thanks,
> Nathan
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx