LLVM does check -Wcast-function-type-sctrict, which is triggered in a
couple of places in the media subsystem.
Signed-off-by: Ricardo Ribalda <[email protected]>
---
Ricardo Ribalda (3):
media: pci: sta2x11: Fix Wcast-function-type-strict warnings
media: usb: pvrusb2: Fix Wcast-function-type-strict warnings
media: mediatek: vcodedc: Fix Wcast-function-type-strict warnings
drivers/media/pci/sta2x11/sta2x11_vip.c | 5 +++--
drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_vpu.c | 6 ++++--
drivers/media/usb/pvrusb2/pvrusb2-context.c | 5 +++--
drivers/media/usb/pvrusb2/pvrusb2-dvb.c | 7 ++++---
drivers/media/usb/pvrusb2/pvrusb2-v4l2.c | 7 ++++---
5 files changed, 18 insertions(+), 12 deletions(-)
---
base-commit: 615d300648869c774bd1fe54b4627bb0c20faed4
change-id: 20240128-fix-clang-warnings-6c12920467cb
Best regards,
--
Ricardo Ribalda <[email protected]>
Building with LLVM=1 throws the following warning:
drivers/media/pci/sta2x11/sta2x11_vip.c:1057:6: warning: cast from 'irqreturn_t (*)(int, struct sta2x11_vip *)' (aka 'enum irqreturn (*)(int, struct sta2x11_vip *)') to 'irq_handler_t' (aka 'enum irqreturn (*)(int, void *)') converts to incompatible function type [-Wcast-function-type-strict]
Signed-off-by: Ricardo Ribalda <[email protected]>
---
drivers/media/pci/sta2x11/sta2x11_vip.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/media/pci/sta2x11/sta2x11_vip.c b/drivers/media/pci/sta2x11/sta2x11_vip.c
index e4cf9d63e926..0a3827575753 100644
--- a/drivers/media/pci/sta2x11/sta2x11_vip.c
+++ b/drivers/media/pci/sta2x11/sta2x11_vip.c
@@ -757,7 +757,7 @@ static const struct video_device video_dev_template = {
/**
* vip_irq - interrupt routine
* @irq: Number of interrupt ( not used, correct number is assumed )
- * @vip: local data structure containing all information
+ * @data: local data structure containing all information
*
* check for both frame interrupts set ( top and bottom ).
* check FIFO overflow, but limit number of log messages after open.
@@ -767,9 +767,10 @@ static const struct video_device video_dev_template = {
*
* IRQ_HANDLED, interrupt done.
*/
-static irqreturn_t vip_irq(int irq, struct sta2x11_vip *vip)
+static irqreturn_t vip_irq(int irq, void *data)
{
unsigned int status;
+ struct sta2x11_vip *vip = data;
status = reg_read(vip, DVP_ITS);
--
2.43.0.429.g432eaa2c6b-goog
Building with LLVM=1 throws the following warning:
drivers/media/usb/pvrusb2/pvrusb2-context.c:110:6: warning: cast from 'void (*)(struct pvr2_context *)' to 'void (*)(void *)' converts to incompatible function type [-Wcast-function-type-strict]
drivers/media/usb/pvrusb2/pvrusb2-v4l2.c:1070:30: warning: cast from 'void (*)(struct pvr2_v4l2_fh *)' to 'pvr2_stream_callback' (aka 'void (*)(void *)') converts to incompatible function type [-Wcast-function-type-strict] drivers/media/usb/pvrusb2/pvrusb2-dvb.c:152:6: warning: cast from 'void (*)(struct pvr2_dvb_adapter *)' to 'pvr2_stream_callback' (aka 'void (*)(void *)') converts to incompatible function type [-Wcast-function-type-strict]
Signed-off-by: Ricardo Ribalda <[email protected]>
---
drivers/media/usb/pvrusb2/pvrusb2-context.c | 5 +++--
drivers/media/usb/pvrusb2/pvrusb2-dvb.c | 7 ++++---
drivers/media/usb/pvrusb2/pvrusb2-v4l2.c | 7 ++++---
3 files changed, 11 insertions(+), 8 deletions(-)
diff --git a/drivers/media/usb/pvrusb2/pvrusb2-context.c b/drivers/media/usb/pvrusb2/pvrusb2-context.c
index 1764674de98b..16edabda191c 100644
--- a/drivers/media/usb/pvrusb2/pvrusb2-context.c
+++ b/drivers/media/usb/pvrusb2/pvrusb2-context.c
@@ -90,8 +90,9 @@ static void pvr2_context_destroy(struct pvr2_context *mp)
}
-static void pvr2_context_notify(struct pvr2_context *mp)
+static void pvr2_context_notify(void *data)
{
+ struct pvr2_context *mp = data;
pvr2_context_set_notify(mp,!0);
}
@@ -107,7 +108,7 @@ static void pvr2_context_check(struct pvr2_context *mp)
"pvr2_context %p (initialize)", mp);
/* Finish hardware initialization */
if (pvr2_hdw_initialize(mp->hdw,
- (void (*)(void *))pvr2_context_notify,
+ pvr2_context_notify,
mp)) {
mp->video_stream.stream =
pvr2_hdw_get_video_stream(mp->hdw);
diff --git a/drivers/media/usb/pvrusb2/pvrusb2-dvb.c b/drivers/media/usb/pvrusb2/pvrusb2-dvb.c
index 26811efe0fb5..8b9f1a09bd53 100644
--- a/drivers/media/usb/pvrusb2/pvrusb2-dvb.c
+++ b/drivers/media/usb/pvrusb2/pvrusb2-dvb.c
@@ -88,8 +88,9 @@ static int pvr2_dvb_feed_thread(void *data)
return stat;
}
-static void pvr2_dvb_notify(struct pvr2_dvb_adapter *adap)
+static void pvr2_dvb_notify(void *data)
{
+ struct pvr2_dvb_adapter *adap = data;
wake_up(&adap->buffer_wait_data);
}
@@ -148,8 +149,8 @@ static int pvr2_dvb_stream_do_start(struct pvr2_dvb_adapter *adap)
if (!(adap->buffer_storage[idx])) return -ENOMEM;
}
- pvr2_stream_set_callback(pvr->video_stream.stream,
- (pvr2_stream_callback) pvr2_dvb_notify, adap);
+ pvr2_stream_set_callback(pvr->video_stream.stream, pvr2_dvb_notify,
+ adap);
ret = pvr2_stream_set_buffer_count(stream, PVR2_DVB_BUFFER_COUNT);
if (ret < 0) return ret;
diff --git a/drivers/media/usb/pvrusb2/pvrusb2-v4l2.c b/drivers/media/usb/pvrusb2/pvrusb2-v4l2.c
index c04ab7258d64..590f80949bbf 100644
--- a/drivers/media/usb/pvrusb2/pvrusb2-v4l2.c
+++ b/drivers/media/usb/pvrusb2/pvrusb2-v4l2.c
@@ -1032,9 +1032,10 @@ static int pvr2_v4l2_open(struct file *file)
return 0;
}
-
-static void pvr2_v4l2_notify(struct pvr2_v4l2_fh *fhp)
+static void pvr2_v4l2_notify(void *data)
{
+ struct pvr2_v4l2_fh *fhp = data;
+
wake_up(&fhp->wait_data);
}
@@ -1067,7 +1068,7 @@ static int pvr2_v4l2_iosetup(struct pvr2_v4l2_fh *fh)
hdw = fh->channel.mc_head->hdw;
sp = fh->pdi->stream->stream;
- pvr2_stream_set_callback(sp,(pvr2_stream_callback)pvr2_v4l2_notify,fh);
+ pvr2_stream_set_callback(sp, pvr2_v4l2_notify, fh);
pvr2_hdw_set_stream_type(hdw,fh->pdi->config);
if ((ret = pvr2_hdw_set_streaming(hdw,!0)) < 0) return ret;
return pvr2_ioread_set_enabled(fh->rhp,!0);
--
2.43.0.429.g432eaa2c6b-goog
Building with LLVM=1 throws the following warning:
drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_vpu.c:38:32: warning: cast from 'mtk_vcodec_ipi_handler' (aka 'void (*)(void *, unsigned int, void *)') to 'ipi_handler_t' (aka 'void (*)(const void *, unsigned int, void *)') converts to incompatible function type [-Wcast-function-type-strict]
Signed-off-by: Ricardo Ribalda <[email protected]>
---
drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_vpu.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_vpu.c b/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_vpu.c
index 9f6e4b59455d..781a0359afdc 100644
--- a/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_vpu.c
+++ b/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_vpu.c
@@ -33,9 +33,11 @@ static int mtk_vcodec_vpu_set_ipi_register(struct mtk_vcodec_fw *fw, int id,
* The handler we receive takes a void * as its first argument. We
* cannot change this because it needs to be passed down to the rproc
* subsystem when SCP is used. VPU takes a const argument, which is
- * more constrained, so the conversion below is safe.
+ * more constrained, so the conversion below is safe. We use the void
+ * casting, to convince clang with -Wcast-function-type-sctrict that
+ * this is safe.
*/
- ipi_handler_t handler_const = (ipi_handler_t)handler;
+ ipi_handler_t handler_const = (ipi_handler_t)((void *)handler);
return vpu_ipi_register(fw->pdev, id, handler_const, name, priv);
}
--
2.43.0.429.g432eaa2c6b-goog
On Sun, Jan 28, 2024 at 02:12:21AM +0000, Ricardo Ribalda wrote:
> Building with LLVM=1 throws the following warning:
> drivers/media/usb/pvrusb2/pvrusb2-context.c:110:6: warning: cast from 'void (*)(struct pvr2_context *)' to 'void (*)(void *)' converts to incompatible function type [-Wcast-function-type-strict]
> drivers/media/usb/pvrusb2/pvrusb2-v4l2.c:1070:30: warning: cast from 'void (*)(struct pvr2_v4l2_fh *)' to 'pvr2_stream_callback' (aka 'void (*)(void *)') converts to incompatible function type [-Wcast-function-type-strict] drivers/media/usb/pvrusb2/pvrusb2-dvb.c:152:6: warning: cast from 'void (*)(struct pvr2_dvb_adapter *)' to 'pvr2_stream_callback' (aka 'void (*)(void *)') converts to incompatible function type [-Wcast-function-type-strict]
>
> Signed-off-by: Ricardo Ribalda <[email protected]>
Reviewed-by: Nathan Chancellor <[email protected]>
> ---
> drivers/media/usb/pvrusb2/pvrusb2-context.c | 5 +++--
> drivers/media/usb/pvrusb2/pvrusb2-dvb.c | 7 ++++---
> drivers/media/usb/pvrusb2/pvrusb2-v4l2.c | 7 ++++---
> 3 files changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/media/usb/pvrusb2/pvrusb2-context.c b/drivers/media/usb/pvrusb2/pvrusb2-context.c
> index 1764674de98b..16edabda191c 100644
> --- a/drivers/media/usb/pvrusb2/pvrusb2-context.c
> +++ b/drivers/media/usb/pvrusb2/pvrusb2-context.c
> @@ -90,8 +90,9 @@ static void pvr2_context_destroy(struct pvr2_context *mp)
> }
>
>
> -static void pvr2_context_notify(struct pvr2_context *mp)
> +static void pvr2_context_notify(void *data)
> {
> + struct pvr2_context *mp = data;
> pvr2_context_set_notify(mp,!0);
> }
>
> @@ -107,7 +108,7 @@ static void pvr2_context_check(struct pvr2_context *mp)
> "pvr2_context %p (initialize)", mp);
> /* Finish hardware initialization */
> if (pvr2_hdw_initialize(mp->hdw,
> - (void (*)(void *))pvr2_context_notify,
> + pvr2_context_notify,
> mp)) {
> mp->video_stream.stream =
> pvr2_hdw_get_video_stream(mp->hdw);
> diff --git a/drivers/media/usb/pvrusb2/pvrusb2-dvb.c b/drivers/media/usb/pvrusb2/pvrusb2-dvb.c
> index 26811efe0fb5..8b9f1a09bd53 100644
> --- a/drivers/media/usb/pvrusb2/pvrusb2-dvb.c
> +++ b/drivers/media/usb/pvrusb2/pvrusb2-dvb.c
> @@ -88,8 +88,9 @@ static int pvr2_dvb_feed_thread(void *data)
> return stat;
> }
>
> -static void pvr2_dvb_notify(struct pvr2_dvb_adapter *adap)
> +static void pvr2_dvb_notify(void *data)
> {
> + struct pvr2_dvb_adapter *adap = data;
> wake_up(&adap->buffer_wait_data);
> }
>
> @@ -148,8 +149,8 @@ static int pvr2_dvb_stream_do_start(struct pvr2_dvb_adapter *adap)
> if (!(adap->buffer_storage[idx])) return -ENOMEM;
> }
>
> - pvr2_stream_set_callback(pvr->video_stream.stream,
> - (pvr2_stream_callback) pvr2_dvb_notify, adap);
> + pvr2_stream_set_callback(pvr->video_stream.stream, pvr2_dvb_notify,
> + adap);
>
> ret = pvr2_stream_set_buffer_count(stream, PVR2_DVB_BUFFER_COUNT);
> if (ret < 0) return ret;
> diff --git a/drivers/media/usb/pvrusb2/pvrusb2-v4l2.c b/drivers/media/usb/pvrusb2/pvrusb2-v4l2.c
> index c04ab7258d64..590f80949bbf 100644
> --- a/drivers/media/usb/pvrusb2/pvrusb2-v4l2.c
> +++ b/drivers/media/usb/pvrusb2/pvrusb2-v4l2.c
> @@ -1032,9 +1032,10 @@ static int pvr2_v4l2_open(struct file *file)
> return 0;
> }
>
> -
> -static void pvr2_v4l2_notify(struct pvr2_v4l2_fh *fhp)
> +static void pvr2_v4l2_notify(void *data)
> {
> + struct pvr2_v4l2_fh *fhp = data;
> +
> wake_up(&fhp->wait_data);
> }
>
> @@ -1067,7 +1068,7 @@ static int pvr2_v4l2_iosetup(struct pvr2_v4l2_fh *fh)
>
> hdw = fh->channel.mc_head->hdw;
> sp = fh->pdi->stream->stream;
> - pvr2_stream_set_callback(sp,(pvr2_stream_callback)pvr2_v4l2_notify,fh);
> + pvr2_stream_set_callback(sp, pvr2_v4l2_notify, fh);
> pvr2_hdw_set_stream_type(hdw,fh->pdi->config);
> if ((ret = pvr2_hdw_set_streaming(hdw,!0)) < 0) return ret;
> return pvr2_ioread_set_enabled(fh->rhp,!0);
>
> --
> 2.43.0.429.g432eaa2c6b-goog
>
On Thu, Feb 1, 2024 at 10:17 PM Nathan Chancellor <[email protected]> wrote:
>
> On Sun, Jan 28, 2024 at 02:12:22AM +0000, Ricardo Ribalda wrote:
> > Building with LLVM=1 throws the following warning:
> > drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_vpu.c:38:32: warning: cast from 'mtk_vcodec_ipi_handler' (aka 'void (*)(void *, unsigned int, void *)') to 'ipi_handler_t' (aka 'void (*)(const void *, unsigned int, void *)') converts to incompatible function type [-Wcast-function-type-strict]
> >
> > Signed-off-by: Ricardo Ribalda <[email protected]>
>
> I am not positive because I don't have any hardware to test this driver
> but I suspect this patch is just hiding the warning without actually
> addressing the issue that it is pointing out.
Agreed, this won't fix the issue. The correct solution is to drop the
cast and change the handler type to match the pointer type (i.e. use
const void* for the first argument).
Sami
On Sun, Jan 28, 2024 at 02:12:20AM +0000, Ricardo Ribalda wrote:
> Building with LLVM=1 throws the following warning:
> drivers/media/pci/sta2x11/sta2x11_vip.c:1057:6: warning: cast from 'irqreturn_t (*)(int, struct sta2x11_vip *)' (aka 'enum irqreturn (*)(int, struct sta2x11_vip *)') to 'irq_handler_t' (aka 'enum irqreturn (*)(int, void *)') converts to incompatible function type [-Wcast-function-type-strict]
>
> Signed-off-by: Ricardo Ribalda <[email protected]>
Reviewed-by: Nathan Chancellor <[email protected]>
I wonder if the media tree cares about reverse Christmas tree order for
variables?
> ---
> drivers/media/pci/sta2x11/sta2x11_vip.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/pci/sta2x11/sta2x11_vip.c b/drivers/media/pci/sta2x11/sta2x11_vip.c
> index e4cf9d63e926..0a3827575753 100644
> --- a/drivers/media/pci/sta2x11/sta2x11_vip.c
> +++ b/drivers/media/pci/sta2x11/sta2x11_vip.c
> @@ -757,7 +757,7 @@ static const struct video_device video_dev_template = {
> /**
> * vip_irq - interrupt routine
> * @irq: Number of interrupt ( not used, correct number is assumed )
> - * @vip: local data structure containing all information
> + * @data: local data structure containing all information
> *
> * check for both frame interrupts set ( top and bottom ).
> * check FIFO overflow, but limit number of log messages after open.
> @@ -767,9 +767,10 @@ static const struct video_device video_dev_template = {
> *
> * IRQ_HANDLED, interrupt done.
> */
> -static irqreturn_t vip_irq(int irq, struct sta2x11_vip *vip)
> +static irqreturn_t vip_irq(int irq, void *data)
> {
> unsigned int status;
> + struct sta2x11_vip *vip = data;
>
> status = reg_read(vip, DVP_ITS);
>
>
> --
> 2.43.0.429.g432eaa2c6b-goog
>
On Sun, Jan 28, 2024 at 02:12:22AM +0000, Ricardo Ribalda wrote:
> Building with LLVM=1 throws the following warning:
> drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_vpu.c:38:32: warning: cast from 'mtk_vcodec_ipi_handler' (aka 'void (*)(void *, unsigned int, void *)') to 'ipi_handler_t' (aka 'void (*)(const void *, unsigned int, void *)') converts to incompatible function type [-Wcast-function-type-strict]
>
> Signed-off-by: Ricardo Ribalda <[email protected]>
I am not positive because I don't have any hardware to test this driver
but I suspect this patch is just hiding the warning without actually
addressing the issue that it is pointing out. If handler is called
through vpu_ipi_register() indirectly (which it obviously is if it is
being passed down), there will be a CFI failure because the type of
mtk_vcodec_ipi_handler is not the same as ipi_handler_t, as the comment
mentions.
Has this seen testing on real hardware with kCFI?
> ---
> drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_vpu.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_vpu.c b/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_vpu.c
> index 9f6e4b59455d..781a0359afdc 100644
> --- a/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_vpu.c
> +++ b/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_vpu.c
> @@ -33,9 +33,11 @@ static int mtk_vcodec_vpu_set_ipi_register(struct mtk_vcodec_fw *fw, int id,
> * The handler we receive takes a void * as its first argument. We
> * cannot change this because it needs to be passed down to the rproc
> * subsystem when SCP is used. VPU takes a const argument, which is
> - * more constrained, so the conversion below is safe.
> + * more constrained, so the conversion below is safe. We use the void
> + * casting, to convince clang with -Wcast-function-type-sctrict that
> + * this is safe.
> */
> - ipi_handler_t handler_const = (ipi_handler_t)handler;
> + ipi_handler_t handler_const = (ipi_handler_t)((void *)handler);
>
> return vpu_ipi_register(fw->pdev, id, handler_const, name, priv);
> }
>
> --
> 2.43.0.429.g432eaa2c6b-goog
>
Il 01/02/24 23:25, Sami Tolvanen ha scritto:
> On Thu, Feb 1, 2024 at 10:17 PM Nathan Chancellor <[email protected]> wrote:
>>
>> On Sun, Jan 28, 2024 at 02:12:22AM +0000, Ricardo Ribalda wrote:
>>> Building with LLVM=1 throws the following warning:
>>> drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_vpu.c:38:32: warning: cast from 'mtk_vcodec_ipi_handler' (aka 'void (*)(void *, unsigned int, void *)') to 'ipi_handler_t' (aka 'void (*)(const void *, unsigned int, void *)') converts to incompatible function type [-Wcast-function-type-strict]
>>>
>>> Signed-off-by: Ricardo Ribalda <[email protected]>
>>
>> I am not positive because I don't have any hardware to test this driver
>> but I suspect this patch is just hiding the warning without actually
>> addressing the issue that it is pointing out.
>
> Agreed, this won't fix the issue. The correct solution is to drop the
> cast and change the handler type to match the pointer type (i.e. use
> const void* for the first argument).
>
Even though I agree that the correct solution is to change the handler's type,
I think that having a test on the actual hardware done is still valuable.
We scheduled a job on KernelCI to test this commit on our integration kernel,
you'll get results for ChromeOS' tast decoders (MT8195 only) and Fluster tests
on MT8183/8186/8192/8195.
The results should be available in a couple of hours here, relative to commit
`49955a84129dbe1f94fedf729690efcf28513828` on our tree:
https://chromeos.kernelci.org/job/collabora-chromeos-kernel/branch/for-kernelci/
P.S.: If they don't, feel free to ping me or Nicolas (added to the loop) about it.
Cheers,
Angelo
On Fri, Feb 02, 2024 at 01:58:05PM +0100, AngeloGioacchino Del Regno wrote:
> Il 01/02/24 23:25, Sami Tolvanen ha scritto:
> > On Thu, Feb 1, 2024 at 10:17 PM Nathan Chancellor <[email protected]> wrote:
> > >
> > > On Sun, Jan 28, 2024 at 02:12:22AM +0000, Ricardo Ribalda wrote:
> > > > Building with LLVM=1 throws the following warning:
> > > > drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_vpu.c:38:32: warning: cast from 'mtk_vcodec_ipi_handler' (aka 'void (*)(void *, unsigned int, void *)') to 'ipi_handler_t' (aka 'void (*)(const void *, unsigned int, void *)') converts to incompatible function type [-Wcast-function-type-strict]
> > > >
> > > > Signed-off-by: Ricardo Ribalda <[email protected]>
> > >
> > > I am not positive because I don't have any hardware to test this driver
> > > but I suspect this patch is just hiding the warning without actually
> > > addressing the issue that it is pointing out.
> >
> > Agreed, this won't fix the issue. The correct solution is to drop the
> > cast and change the handler type to match the pointer type (i.e. use
> > const void* for the first argument).
> >
>
> Even though I agree that the correct solution is to change the handler's type,
> I think that having a test on the actual hardware done is still valuable.
>
> We scheduled a job on KernelCI to test this commit on our integration kernel,
> you'll get results for ChromeOS' tast decoders (MT8195 only) and Fluster tests
> on MT8183/8186/8192/8195.
>
>
> The results should be available in a couple of hours here, relative to
> commit `49955a84129dbe1f94fedf729690efcf28513828` on our tree:
> https://chromeos.kernelci.org/job/collabora-chromeos-kernel/branch/for-kernelci/
>
> P.S.: If they don't, feel free to ping me or Nicolas (added to the loop) about it.
Hi,
the results are available at
https://chromeos.kernelci.org/test/job/collabora-chromeos-kernel/branch/for-kernelci/kernel/v6.8-rc2-3109-g49955a84129d/
(You need to type "decoder" into the search bar to limit the results to only the
decoder tests)
The only regressions I see are due to infrastructure error or broken test
unrelated to this change (v4l2-decoder-conformance-h264-frext test on
MT8195-Tomato, and cros-tast-decoder-v4l2-sl-h264 test on MT8183-Juniper)
Otherwise, all platforms (MT8183/8186/8192/8195) and video codecs
(VP8/VP9/H264/H265/AV1) seem unaffected.
Note that these are GCC builds.
Thanks,
Nícolas
On Fri, Feb 02, 2024 at 03:15:46PM -0500, Nícolas F. R. A. Prado wrote:
> On Fri, Feb 02, 2024 at 01:58:05PM +0100, AngeloGioacchino Del Regno wrote:
> > Il 01/02/24 23:25, Sami Tolvanen ha scritto:
> > > On Thu, Feb 1, 2024 at 10:17 PM Nathan Chancellor <[email protected]> wrote:
> > > >
> > > > On Sun, Jan 28, 2024 at 02:12:22AM +0000, Ricardo Ribalda wrote:
> > > > > Building with LLVM=1 throws the following warning:
> > > > > drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_vpu.c:38:32: warning: cast from 'mtk_vcodec_ipi_handler' (aka 'void (*)(void *, unsigned int, void *)') to 'ipi_handler_t' (aka 'void (*)(const void *, unsigned int, void *)') converts to incompatible function type [-Wcast-function-type-strict]
> > > > >
> > > > > Signed-off-by: Ricardo Ribalda <[email protected]>
> > > >
> > > > I am not positive because I don't have any hardware to test this driver
> > > > but I suspect this patch is just hiding the warning without actually
> > > > addressing the issue that it is pointing out.
> > >
> > > Agreed, this won't fix the issue. The correct solution is to drop the
> > > cast and change the handler type to match the pointer type (i.e. use
> > > const void* for the first argument).
> > >
> >
> > Even though I agree that the correct solution is to change the handler's type,
> > I think that having a test on the actual hardware done is still valuable.
> >
> > We scheduled a job on KernelCI to test this commit on our integration kernel,
> > you'll get results for ChromeOS' tast decoders (MT8195 only) and Fluster tests
> > on MT8183/8186/8192/8195.
> >
> >
> > The results should be available in a couple of hours here, relative to
> > commit `49955a84129dbe1f94fedf729690efcf28513828` on our tree:
> > https://chromeos.kernelci.org/job/collabora-chromeos-kernel/branch/for-kernelci/
> >
> > P.S.: If they don't, feel free to ping me or Nicolas (added to the loop) about it.
>
> Hi,
>
> the results are available at
>
> https://chromeos.kernelci.org/test/job/collabora-chromeos-kernel/branch/for-kernelci/kernel/v6.8-rc2-3109-g49955a84129d/
>
> (You need to type "decoder" into the search bar to limit the results to only the
> decoder tests)
>
> The only regressions I see are due to infrastructure error or broken test
> unrelated to this change (v4l2-decoder-conformance-h264-frext test on
> MT8195-Tomato, and cros-tast-decoder-v4l2-sl-h264 test on MT8183-Juniper)
>
> Otherwise, all platforms (MT8183/8186/8192/8195) and video codecs
> (VP8/VP9/H264/H265/AV1) seem unaffected.
>
> Note that these are GCC builds.
Thank you for running the tests to make sure this series does not
regress anything. If possible, it would be good to try and build with
LLVM and enable kernel Control Flow Integrity (kCFI, CONFIG_CFI_CLANG)
to see if my theory that this is currently broken is correct. I have
prebuilt LLVM toolchains on kernel.org but if it is too much of a
hassle, I would not worry about it.
https://mirrors.edge.kernel.org/pub/tools/llvm/
Cheers,
Nathan
On Sun, Feb 04, 2024 at 01:55:49PM -0700, Nathan Chancellor wrote:
> On Fri, Feb 02, 2024 at 03:15:46PM -0500, Nícolas F. R. A. Prado wrote:
> > On Fri, Feb 02, 2024 at 01:58:05PM +0100, AngeloGioacchino Del Regno wrote:
> > > Il 01/02/24 23:25, Sami Tolvanen ha scritto:
> > > > On Thu, Feb 1, 2024 at 10:17 PM Nathan Chancellor <[email protected]> wrote:
> > > > >
> > > > > On Sun, Jan 28, 2024 at 02:12:22AM +0000, Ricardo Ribalda wrote:
> > > > > > Building with LLVM=1 throws the following warning:
> > > > > > drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_vpu.c:38:32: warning: cast from 'mtk_vcodec_ipi_handler' (aka 'void (*)(void *, unsigned int, void *)') to 'ipi_handler_t' (aka 'void (*)(const void *, unsigned int, void *)') converts to incompatible function type [-Wcast-function-type-strict]
> > > > > >
> > > > > > Signed-off-by: Ricardo Ribalda <[email protected]>
> > > > >
> > > > > I am not positive because I don't have any hardware to test this driver
> > > > > but I suspect this patch is just hiding the warning without actually
> > > > > addressing the issue that it is pointing out.
> > > >
> > > > Agreed, this won't fix the issue. The correct solution is to drop the
> > > > cast and change the handler type to match the pointer type (i.e. use
> > > > const void* for the first argument).
> > > >
> > >
> > > Even though I agree that the correct solution is to change the handler's type,
> > > I think that having a test on the actual hardware done is still valuable.
> > >
> > > We scheduled a job on KernelCI to test this commit on our integration kernel,
> > > you'll get results for ChromeOS' tast decoders (MT8195 only) and Fluster tests
> > > on MT8183/8186/8192/8195.
> > >
> > >
> > > The results should be available in a couple of hours here, relative to
> > > commit `49955a84129dbe1f94fedf729690efcf28513828` on our tree:
> > > https://chromeos.kernelci.org/job/collabora-chromeos-kernel/branch/for-kernelci/
> > >
> > > P.S.: If they don't, feel free to ping me or Nicolas (added to the loop) about it.
> >
> > Hi,
> >
> > the results are available at
> >
> > https://chromeos.kernelci.org/test/job/collabora-chromeos-kernel/branch/for-kernelci/kernel/v6.8-rc2-3109-g49955a84129d/
> >
> > (You need to type "decoder" into the search bar to limit the results to only the
> > decoder tests)
> >
> > The only regressions I see are due to infrastructure error or broken test
> > unrelated to this change (v4l2-decoder-conformance-h264-frext test on
> > MT8195-Tomato, and cros-tast-decoder-v4l2-sl-h264 test on MT8183-Juniper)
> >
> > Otherwise, all platforms (MT8183/8186/8192/8195) and video codecs
> > (VP8/VP9/H264/H265/AV1) seem unaffected.
> >
> > Note that these are GCC builds.
>
> Thank you for running the tests to make sure this series does not
> regress anything. If possible, it would be good to try and build with
> LLVM and enable kernel Control Flow Integrity (kCFI, CONFIG_CFI_CLANG)
> to see if my theory that this is currently broken is correct. I have
> prebuilt LLVM toolchains on kernel.org but if it is too much of a
> hassle, I would not worry about it.
We don't have a way in KernelCI to run a one-off test, instead we need to make a
PR adding a build definition that will be triggered for every update on a given
tree. The results I reported above were for configurations that we already had
in place.
That said, maybe we should be running with LLVM and CONFIG_CFI_CLANG enabled on
a regular basis, if you think it is useful as a general tool for identifying
issues. I'd also be interested in hearing more from you on what other kind of
clang configs you think might be good to enable in CI to provide value to the
community.
Thanks,
Nícolas