| Message ID | 20181210234201.4850-1-natechancellor@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/amd/display: Pass app_tf by value rather than by reference | expand |
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?
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
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>
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
On Tue, Dec 11, 2018 at 2:24 PM Nathan Chancellor <natechancellor@gmail.com> wrote: > 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). > That C allows this is troubling to me. I may need to go seek therapy to deal with my anguish this has caused me... > > 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 >
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
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
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
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
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,
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(-)