| Message ID | 20240617103018.515f0bf1@endymion.delvare (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RESEND] drm/display: Drop obsolete dependency on COMPILE_TEST | expand |
On Mon, Jun 17, 2024 at 10:30:30AM GMT, Jean Delvare wrote: > Since commit 0166dc11be91 ("of: make CONFIG_OF user selectable"), it > is possible to test-build any driver which depends on OF on any > architecture by explicitly selecting OF. Therefore depending on > COMPILE_TEST as an alternative is no longer needed. The goal of this clause is to allow build-testing the driver with OF being disabled (meaning that some of OF functions are stubbed and some might disappear). I don't see how user-selectable OF provides the same result. > > Signed-off-by: Jean Delvare <jdelvare@suse.de> > Reviewed-by: Javier Martinez Canillas <javierm@redhat.com> > Cc: David Airlie <airlied@gmail.com> > Cc: Daniel Vetter <daniel@ffwll.ch> > --- > Already sent on: 2022-11-21, 2023-01-27, 2023-12-02 > > This is one of the only 3 remaining occurrences of this deprecated > construct. > > Who can pick this patch and get it upstream? > > drivers/gpu/drm/display/Kconfig | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > --- linux-6.6.orig/drivers/gpu/drm/display/Kconfig > +++ linux-6.6/drivers/gpu/drm/display/Kconfig > @@ -3,7 +3,7 @@ > config DRM_DP_AUX_BUS > tristate > depends on DRM > - depends on OF || COMPILE_TEST > + depends on OF > > config DRM_DISPLAY_HELPER > tristate > > > -- > Jean Delvare > SUSE L3 Support
Hi Dmitry, Thanks for your feedback. On Mon, 17 Jun 2024 12:57:19 +0300, Dmitry Baryshkov wrote: > On Mon, Jun 17, 2024 at 10:30:30AM GMT, Jean Delvare wrote: > > Since commit 0166dc11be91 ("of: make CONFIG_OF user selectable"), it > > is possible to test-build any driver which depends on OF on any > > architecture by explicitly selecting OF. Therefore depending on > > COMPILE_TEST as an alternative is no longer needed. > > The goal of this clause is to allow build-testing the driver with OF > being disabled (meaning that some of OF functions are stubbed and some > might disappear). I don't see how user-selectable OF provides the same > result. Historically, the goal of this clause *was* to allow build-testing the driver on architectures which did not support OF, and that did make sense back then. As I understand it, building the driver without OF support was never a goal per se (if it was, then the driver wouldn't be set to depend on OF in the first place). Some of my other submissions include the following explanation which you might find useful (I ended up stripping it on resubmission after being told I was being too verbose, but maybe it was needed after all): It is actually better to always build such drivers with OF enabled, so that the test builds are closer to how each driver will actually be built on its intended target. Building them without OF may not test much as the compiler will optimize out potentially large parts of the code. In the worst case, this could even pop false positive warnings. Dropping COMPILE_TEST here improves the quality of our testing and avoids wasting time on non-existent issues.
On Mon, Jun 17, 2024 at 01:23:48PM GMT, Jean Delvare wrote: > Hi Dmitry, > > Thanks for your feedback. > > On Mon, 17 Jun 2024 12:57:19 +0300, Dmitry Baryshkov wrote: > > On Mon, Jun 17, 2024 at 10:30:30AM GMT, Jean Delvare wrote: > > > Since commit 0166dc11be91 ("of: make CONFIG_OF user selectable"), it > > > is possible to test-build any driver which depends on OF on any > > > architecture by explicitly selecting OF. Therefore depending on > > > COMPILE_TEST as an alternative is no longer needed. > > > > The goal of this clause is to allow build-testing the driver with OF > > being disabled (meaning that some of OF functions are stubbed and some > > might disappear). I don't see how user-selectable OF provides the same > > result. > > Historically, the goal of this clause *was* to allow build-testing the > driver on architectures which did not support OF, and that did make > sense back then. As I understand it, building the driver without OF > support was never a goal per se (if it was, then the driver wouldn't be > set to depend on OF in the first place). > > Some of my other submissions include the following explanation which > you might find useful (I ended up stripping it on resubmission after > being told I was being too verbose, but maybe it was needed after all): > > It is actually better to always build such drivers with OF enabled, > so that the test builds are closer to how each driver will actually be > built on its intended target. Building them without OF may not test > much as the compiler will optimize out potentially large parts of the > code. In the worst case, this could even pop false positive warnings. > Dropping COMPILE_TEST here improves the quality of our testing and > avoids wasting time on non-existent issues. This doesn't seem to match the COMPILE_TEST usage that I observe in other places. For example, we frequently use 'depends on ARCH_QCOM || COMPILE_TEST'. Which means that the driver itself doesn't make sense without ARCH_QCOM, but we want for it to be tested on non-ARCH_QCOM cases. I think the same logic applies to 'depends on OF || COMPILE_TEST' clauses. The driver (DP AUX bus) depends on OF to be fully functional, but it should be compilable even without OF case. I'll let Douglas (in cc) comment if my understanding is incorrect.
On Mon, 17 Jun 2024 14:55:22 +0300, Dmitry Baryshkov wrote: > On Mon, Jun 17, 2024 at 01:23:48PM GMT, Jean Delvare wrote: > > Hi Dmitry, > > > > Thanks for your feedback. > > > > On Mon, 17 Jun 2024 12:57:19 +0300, Dmitry Baryshkov wrote: > > > On Mon, Jun 17, 2024 at 10:30:30AM GMT, Jean Delvare wrote: > > > > Since commit 0166dc11be91 ("of: make CONFIG_OF user selectable"), it > > > > is possible to test-build any driver which depends on OF on any > > > > architecture by explicitly selecting OF. Therefore depending on > > > > COMPILE_TEST as an alternative is no longer needed. > > > > > > The goal of this clause is to allow build-testing the driver with OF > > > being disabled (meaning that some of OF functions are stubbed and some > > > might disappear). I don't see how user-selectable OF provides the same > > > result. > > > > Historically, the goal of this clause *was* to allow build-testing the > > driver on architectures which did not support OF, and that did make > > sense back then. As I understand it, building the driver without OF > > support was never a goal per se (if it was, then the driver wouldn't be > > set to depend on OF in the first place). > > > > Some of my other submissions include the following explanation which > > you might find useful (I ended up stripping it on resubmission after > > being told I was being too verbose, but maybe it was needed after all): > > > > It is actually better to always build such drivers with OF enabled, > > so that the test builds are closer to how each driver will actually be > > built on its intended target. Building them without OF may not test > > much as the compiler will optimize out potentially large parts of the > > code. In the worst case, this could even pop false positive warnings. > > Dropping COMPILE_TEST here improves the quality of our testing and > > avoids wasting time on non-existent issues. > > This doesn't seem to match the COMPILE_TEST usage that I observe in > other places. For example, we frequently use 'depends on ARCH_QCOM || > COMPILE_TEST'. Which means that the driver itself doesn't make sense > without ARCH_QCOM, but we want for it to be tested on non-ARCH_QCOM > cases. I think the same logic applies to 'depends on OF || > COMPILE_TEST' clauses. The driver (DP AUX bus) depends on OF to be fully > functional, but it should be compilable even without OF case. The major difference is that one can't possibly enable ARCH_QCOM if building on X86 for example. Therefore COMPILE_TEST is the only way to let everyone (including randconfig/allmodconfig build farms) test-build your code. On the other hand, if you want to test-build drm_dp_aux_bus, you can simply enable OF, because it is available on all architectures and doesn't depend on anything. No need for COMPILE_TEST. For clarity, I'm not advocating against the use of COMPILE_TEST, actually if you check the history of my kernel contributions 10 years back, you'll find commits from me adding COMPILE_TEST in addition to arch-specific dependencies to many drivers. All I'm saying is that it should only be used when it is the only way to enable the build.
On Mon, Jun 17, 2024 at 08:18:14PM GMT, Jean Delvare wrote: > On Mon, 17 Jun 2024 14:55:22 +0300, Dmitry Baryshkov wrote: > > On Mon, Jun 17, 2024 at 01:23:48PM GMT, Jean Delvare wrote: > > > Hi Dmitry, > > > > > > Thanks for your feedback. > > > > > > On Mon, 17 Jun 2024 12:57:19 +0300, Dmitry Baryshkov wrote: > > > > On Mon, Jun 17, 2024 at 10:30:30AM GMT, Jean Delvare wrote: > > > > > Since commit 0166dc11be91 ("of: make CONFIG_OF user selectable"), it > > > > > is possible to test-build any driver which depends on OF on any > > > > > architecture by explicitly selecting OF. Therefore depending on > > > > > COMPILE_TEST as an alternative is no longer needed. > > > > > > > > The goal of this clause is to allow build-testing the driver with OF > > > > being disabled (meaning that some of OF functions are stubbed and some > > > > might disappear). I don't see how user-selectable OF provides the same > > > > result. > > > > > > Historically, the goal of this clause *was* to allow build-testing the > > > driver on architectures which did not support OF, and that did make > > > sense back then. As I understand it, building the driver without OF > > > support was never a goal per se (if it was, then the driver wouldn't be > > > set to depend on OF in the first place). > > > > > > Some of my other submissions include the following explanation which > > > you might find useful (I ended up stripping it on resubmission after > > > being told I was being too verbose, but maybe it was needed after all): > > > > > > It is actually better to always build such drivers with OF enabled, > > > so that the test builds are closer to how each driver will actually be > > > built on its intended target. Building them without OF may not test > > > much as the compiler will optimize out potentially large parts of the > > > code. In the worst case, this could even pop false positive warnings. > > > Dropping COMPILE_TEST here improves the quality of our testing and > > > avoids wasting time on non-existent issues. > > > > This doesn't seem to match the COMPILE_TEST usage that I observe in > > other places. For example, we frequently use 'depends on ARCH_QCOM || > > COMPILE_TEST'. Which means that the driver itself doesn't make sense > > without ARCH_QCOM, but we want for it to be tested on non-ARCH_QCOM > > cases. I think the same logic applies to 'depends on OF || > > COMPILE_TEST' clauses. The driver (DP AUX bus) depends on OF to be fully > > functional, but it should be compilable even without OF case. > > The major difference is that one can't possibly enable ARCH_QCOM if > building on X86 for example. Therefore COMPILE_TEST is the only way to > let everyone (including randconfig/allmodconfig build farms) test-build > your code. > > On the other hand, if you want to test-build drm_dp_aux_bus, you can > simply enable OF, because it is available on all architectures and > doesn't depend on anything. No need for COMPILE_TEST. I'd probably let Doug respond, what was his intention. > > For clarity, I'm not advocating against the use of COMPILE_TEST, > actually if you check the history of my kernel contributions 10 years > back, you'll find commits from me adding COMPILE_TEST in addition to > arch-specific dependencies to many drivers. All I'm saying is that it > should only be used when it is the only way to enable the build.
Hi, On Mon, Jun 17, 2024 at 3:26 PM Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > On Mon, Jun 17, 2024 at 08:18:14PM GMT, Jean Delvare wrote: > > On Mon, 17 Jun 2024 14:55:22 +0300, Dmitry Baryshkov wrote: > > > On Mon, Jun 17, 2024 at 01:23:48PM GMT, Jean Delvare wrote: > > > > Hi Dmitry, > > > > > > > > Thanks for your feedback. > > > > > > > > On Mon, 17 Jun 2024 12:57:19 +0300, Dmitry Baryshkov wrote: > > > > > On Mon, Jun 17, 2024 at 10:30:30AM GMT, Jean Delvare wrote: > > > > > > Since commit 0166dc11be91 ("of: make CONFIG_OF user selectable"), it > > > > > > is possible to test-build any driver which depends on OF on any > > > > > > architecture by explicitly selecting OF. Therefore depending on > > > > > > COMPILE_TEST as an alternative is no longer needed. > > > > > > > > > > The goal of this clause is to allow build-testing the driver with OF > > > > > being disabled (meaning that some of OF functions are stubbed and some > > > > > might disappear). I don't see how user-selectable OF provides the same > > > > > result. > > > > > > > > Historically, the goal of this clause *was* to allow build-testing the > > > > driver on architectures which did not support OF, and that did make > > > > sense back then. As I understand it, building the driver without OF > > > > support was never a goal per se (if it was, then the driver wouldn't be > > > > set to depend on OF in the first place). > > > > > > > > Some of my other submissions include the following explanation which > > > > you might find useful (I ended up stripping it on resubmission after > > > > being told I was being too verbose, but maybe it was needed after all): > > > > > > > > It is actually better to always build such drivers with OF enabled, > > > > so that the test builds are closer to how each driver will actually be > > > > built on its intended target. Building them without OF may not test > > > > much as the compiler will optimize out potentially large parts of the > > > > code. In the worst case, this could even pop false positive warnings. > > > > Dropping COMPILE_TEST here improves the quality of our testing and > > > > avoids wasting time on non-existent issues. > > > > > > This doesn't seem to match the COMPILE_TEST usage that I observe in > > > other places. For example, we frequently use 'depends on ARCH_QCOM || > > > COMPILE_TEST'. Which means that the driver itself doesn't make sense > > > without ARCH_QCOM, but we want for it to be tested on non-ARCH_QCOM > > > cases. I think the same logic applies to 'depends on OF || > > > COMPILE_TEST' clauses. The driver (DP AUX bus) depends on OF to be fully > > > functional, but it should be compilable even without OF case. > > > > The major difference is that one can't possibly enable ARCH_QCOM if > > building on X86 for example. Therefore COMPILE_TEST is the only way to > > let everyone (including randconfig/allmodconfig build farms) test-build > > your code. > > > > On the other hand, if you want to test-build drm_dp_aux_bus, you can > > simply enable OF, because it is available on all architectures and > > doesn't depend on anything. No need for COMPILE_TEST. > > I'd probably let Doug respond, what was his intention. Is this me? This looks like a straight revert of commit 876271118aa4 ("drm/display: Fix build error without CONFIG_OF") I don't personally have anything against removing COMPILE_TEST for this given that I wasn't the one who added it, but make sure it's not going to cause randconfig issues. -Doug
Hi Doug, On Mon, 17 Jun 2024 19:12:05 -0700, Doug Anderson wrote: > On Mon, Jun 17, 2024 at 3:26 PM Dmitry Baryshkov > <dmitry.baryshkov@linaro.org> wrote: > > > > On Mon, Jun 17, 2024 at 08:18:14PM GMT, Jean Delvare wrote: > > > The major difference is that one can't possibly enable ARCH_QCOM if > > > building on X86 for example. Therefore COMPILE_TEST is the only way to > > > let everyone (including randconfig/allmodconfig build farms) test-build > > > your code. > > > > > > On the other hand, if you want to test-build drm_dp_aux_bus, you can > > > simply enable OF, because it is available on all architectures and > > > doesn't depend on anything. No need for COMPILE_TEST. > > > > I'd probably let Doug respond, what was his intention. > > Is this me? This looks like a straight revert of commit 876271118aa4 > ("drm/display: Fix build error without CONFIG_OF") > > I don't personally have anything against removing COMPILE_TEST for > this given that I wasn't the one who added it, but make sure it's not > going to cause randconfig issues. Thanks for pointing this commit to my attention, I wasn't aware. This means that my proposed patch is incomplete and would indeed break randconfig. I'll send a v2.
On Mon, Jun 17, 2024 at 07:12:05PM GMT, Doug Anderson wrote: > Hi, > > On Mon, Jun 17, 2024 at 3:26 PM Dmitry Baryshkov > <dmitry.baryshkov@linaro.org> wrote: > > > > On Mon, Jun 17, 2024 at 08:18:14PM GMT, Jean Delvare wrote: > > > On Mon, 17 Jun 2024 14:55:22 +0300, Dmitry Baryshkov wrote: > > > > On Mon, Jun 17, 2024 at 01:23:48PM GMT, Jean Delvare wrote: > > > > > Hi Dmitry, > > > > > > > > > > Thanks for your feedback. > > > > > > > > > > On Mon, 17 Jun 2024 12:57:19 +0300, Dmitry Baryshkov wrote: > > > > > > On Mon, Jun 17, 2024 at 10:30:30AM GMT, Jean Delvare wrote: > > > > > > > Since commit 0166dc11be91 ("of: make CONFIG_OF user selectable"), it > > > > > > > is possible to test-build any driver which depends on OF on any > > > > > > > architecture by explicitly selecting OF. Therefore depending on > > > > > > > COMPILE_TEST as an alternative is no longer needed. > > > > > > > > > > > > The goal of this clause is to allow build-testing the driver with OF > > > > > > being disabled (meaning that some of OF functions are stubbed and some > > > > > > might disappear). I don't see how user-selectable OF provides the same > > > > > > result. > > > > > > > > > > Historically, the goal of this clause *was* to allow build-testing the > > > > > driver on architectures which did not support OF, and that did make > > > > > sense back then. As I understand it, building the driver without OF > > > > > support was never a goal per se (if it was, then the driver wouldn't be > > > > > set to depend on OF in the first place). > > > > > > > > > > Some of my other submissions include the following explanation which > > > > > you might find useful (I ended up stripping it on resubmission after > > > > > being told I was being too verbose, but maybe it was needed after all): > > > > > > > > > > It is actually better to always build such drivers with OF enabled, > > > > > so that the test builds are closer to how each driver will actually be > > > > > built on its intended target. Building them without OF may not test > > > > > much as the compiler will optimize out potentially large parts of the > > > > > code. In the worst case, this could even pop false positive warnings. > > > > > Dropping COMPILE_TEST here improves the quality of our testing and > > > > > avoids wasting time on non-existent issues. > > > > > > > > This doesn't seem to match the COMPILE_TEST usage that I observe in > > > > other places. For example, we frequently use 'depends on ARCH_QCOM || > > > > COMPILE_TEST'. Which means that the driver itself doesn't make sense > > > > without ARCH_QCOM, but we want for it to be tested on non-ARCH_QCOM > > > > cases. I think the same logic applies to 'depends on OF || > > > > COMPILE_TEST' clauses. The driver (DP AUX bus) depends on OF to be fully > > > > functional, but it should be compilable even without OF case. > > > > > > The major difference is that one can't possibly enable ARCH_QCOM if > > > building on X86 for example. Therefore COMPILE_TEST is the only way to > > > let everyone (including randconfig/allmodconfig build farms) test-build > > > your code. > > > > > > On the other hand, if you want to test-build drm_dp_aux_bus, you can > > > simply enable OF, because it is available on all architectures and > > > doesn't depend on anything. No need for COMPILE_TEST. > > > > I'd probably let Doug respond, what was his intention. > > Is this me? This looks like a straight revert of commit 876271118aa4 > ("drm/display: Fix build error without CONFIG_OF") Thanks! > I don't personally have anything against removing COMPILE_TEST for > this given that I wasn't the one who added it, but make sure it's not > going to cause randconfig issues.
--- linux-6.6.orig/drivers/gpu/drm/display/Kconfig +++ linux-6.6/drivers/gpu/drm/display/Kconfig @@ -3,7 +3,7 @@ config DRM_DP_AUX_BUS tristate depends on DRM - depends on OF || COMPILE_TEST + depends on OF config DRM_DISPLAY_HELPER tristate
Since commit 0166dc11be91 ("of: make CONFIG_OF user selectable"), it is possible to test-build any driver which depends on OF on any architecture by explicitly selecting OF. Therefore depending on COMPILE_TEST as an alternative is no longer needed. Signed-off-by: Jean Delvare <jdelvare@suse.de> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com> Cc: David Airlie <airlied@gmail.com> Cc: Daniel Vetter <daniel@ffwll.ch> --- Already sent on: 2022-11-21, 2023-01-27, 2023-12-02 This is one of the only 3 remaining occurrences of this deprecated construct. Who can pick this patch and get it upstream? drivers/gpu/drm/display/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)