| Message ID | 20220920-resend-hwtimestamp-v9-1-55a89f46f6be@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | uvcvideo: Fixes for hw timestamping | expand |
On (23/03/15 14:30), Ricardo Ribalda wrote: > > If head is 0, last will be addressing the index 0 instead of clock->size > -1. Luckily clock->head is unsiged, otherwise it would be addressing > 0xffffffff. > > Nontheless, this is not the intented behaviour and should be fixed. > > Fixes: 66847ef013cc ("[media] uvcvideo: Add UVC timestamps support") > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>
Hi Ricardo, Thank you for the patch. On Wed, Mar 15, 2023 at 02:30:12PM +0100, Ricardo Ribalda wrote: > If head is 0, last will be addressing the index 0 instead of clock->size > -1. Luckily clock->head is unsiged, otherwise it would be addressing > 0xffffffff. I'm not following you. In the expression (clock->head - 1) % clock->size clock->head is an unsigned int, and 1 as a signed int, so the result of the subtraction is promoted to an unsigned int. When clock->head is 0, the expression evaluates to 0xffffffff % clock->size clock->size is a power of two (hardcoded to 32 at the moment), so the expression evaluates to 31, as intended. Am I missing something ? > Nontheless, this is not the intented behaviour and should be fixed. > > Fixes: 66847ef013cc ("[media] uvcvideo: Add UVC timestamps support") > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > --- > drivers/media/usb/uvc/uvc_video.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c > index d4b023d4de7c..4ff4ab4471fe 100644 > --- a/drivers/media/usb/uvc/uvc_video.c > +++ b/drivers/media/usb/uvc/uvc_video.c > @@ -732,7 +732,7 @@ void uvc_video_clock_update(struct uvc_streaming *stream, > goto done; > > first = &clock->samples[clock->head]; > - last = &clock->samples[(clock->head - 1) % clock->size]; > + last = &clock->samples[(clock->head - 1 + clock->size) % clock->size]; > > /* First step, PTS to SOF conversion. */ > delta_stc = buf->pts - (1UL << 31);
Hi Laurent On Mon, 12 Feb 2024 at 23:59, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Ricardo, > > Thank you for the patch. > > On Wed, Mar 15, 2023 at 02:30:12PM +0100, Ricardo Ribalda wrote: > > If head is 0, last will be addressing the index 0 instead of clock->size > > -1. Luckily clock->head is unsiged, otherwise it would be addressing > > 0xffffffff. > > I'm not following you. In the expression > > (clock->head - 1) % clock->size > > clock->head is an unsigned int, and 1 as a signed int, so the result of > the subtraction is promoted to an unsigned int. When clock->head is 0, the expression evaluates to > > 0xffffffff % clock->size > > clock->size is a power of two (hardcoded to 32 at the moment), so the > expression evaluates to 31, as intended. > > Am I missing something ? Take a look to: https://godbolt.org/z/xYeqTx6ba The expression only works because the size is a power of two. In this set I am allowing sizes that are not powers of two. Regards! > > > Nontheless, this is not the intented behaviour and should be fixed. > > > > Fixes: 66847ef013cc ("[media] uvcvideo: Add UVC timestamps support") > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > > --- > > drivers/media/usb/uvc/uvc_video.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c > > index d4b023d4de7c..4ff4ab4471fe 100644 > > --- a/drivers/media/usb/uvc/uvc_video.c > > +++ b/drivers/media/usb/uvc/uvc_video.c > > @@ -732,7 +732,7 @@ void uvc_video_clock_update(struct uvc_streaming *stream, > > goto done; > > > > first = &clock->samples[clock->head]; > > - last = &clock->samples[(clock->head - 1) % clock->size]; > > + last = &clock->samples[(clock->head - 1 + clock->size) % clock->size]; > > > > /* First step, PTS to SOF conversion. */ > > delta_stc = buf->pts - (1UL << 31); > > -- > Regards, > > Laurent Pinchart -- Ricardo Ribalda
Hi Ricardo, On Mon, Feb 19, 2024 at 11:28:03AM +0100, Ricardo Ribalda wrote: > On Mon, 12 Feb 2024 at 23:59, Laurent Pinchart wrote: > > On Wed, Mar 15, 2023 at 02:30:12PM +0100, Ricardo Ribalda wrote: > > > If head is 0, last will be addressing the index 0 instead of clock->size > > > -1. Luckily clock->head is unsiged, otherwise it would be addressing > > > 0xffffffff. > > > > I'm not following you. In the expression > > > > (clock->head - 1) % clock->size > > > > clock->head is an unsigned int, and 1 as a signed int, so the result of > > the subtraction is promoted to an unsigned int. When clock->head is 0, the expression evaluates to > > > > 0xffffffff % clock->size > > > > clock->size is a power of two (hardcoded to 32 at the moment), so the > > expression evaluates to 31, as intended. > > > > Am I missing something ? > > Take a look to: https://godbolt.org/z/xYeqTx6ba > > The expression only works because the size is a power of two. In this > set I am allowing sizes that are not powers of two. Could you then update the commit message to explain that ? I'll review the rest of the series this week. > > > Nontheless, this is not the intented behaviour and should be fixed. > > > > > > Fixes: 66847ef013cc ("[media] uvcvideo: Add UVC timestamps support") > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > > > --- > > > drivers/media/usb/uvc/uvc_video.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c > > > index d4b023d4de7c..4ff4ab4471fe 100644 > > > --- a/drivers/media/usb/uvc/uvc_video.c > > > +++ b/drivers/media/usb/uvc/uvc_video.c > > > @@ -732,7 +732,7 @@ void uvc_video_clock_update(struct uvc_streaming *stream, > > > goto done; > > > > > > first = &clock->samples[clock->head]; > > > - last = &clock->samples[(clock->head - 1) % clock->size]; > > > + last = &clock->samples[(clock->head - 1 + clock->size) % clock->size]; > > > > > > /* First step, PTS to SOF conversion. */ > > > delta_stc = buf->pts - (1UL << 31);
Hi Laurent On Mon, 19 Feb 2024 at 11:40, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Ricardo, > > On Mon, Feb 19, 2024 at 11:28:03AM +0100, Ricardo Ribalda wrote: > > On Mon, 12 Feb 2024 at 23:59, Laurent Pinchart wrote: > > > On Wed, Mar 15, 2023 at 02:30:12PM +0100, Ricardo Ribalda wrote: > > > > If head is 0, last will be addressing the index 0 instead of clock->size > > > > -1. Luckily clock->head is unsiged, otherwise it would be addressing > > > > 0xffffffff. > > > > > > I'm not following you. In the expression > > > > > > (clock->head - 1) % clock->size > > > > > > clock->head is an unsigned int, and 1 as a signed int, so the result of > > > the subtraction is promoted to an unsigned int. When clock->head is 0, the expression evaluates to > > > > > > 0xffffffff % clock->size > > > > > > clock->size is a power of two (hardcoded to 32 at the moment), so the > > > expression evaluates to 31, as intended. > > > > > > Am I missing something ? > > > > Take a look to: https://godbolt.org/z/xYeqTx6ba > > > > The expression only works because the size is a power of two. In this > > set I am allowing sizes that are not powers of two. > > Could you then update the commit message to explain that ? > > I'll review the rest of the series this week. Thanks Will update with the following text after the review: The tail of the list lives at the position before the head. This is mathematically noted as ``` (head-1) mod size. ``` Unfortunately C, does not have a modulus operator, but a remainder operator (%). The reminder operation has a different result than the modulus if (head -1) is a negative number and size is not a power of two. Adding size to (head-1) allows the code to run with any value of size. > > > > > Nontheless, this is not the intented behaviour and should be fixed. > > > > > > > > Fixes: 66847ef013cc ("[media] uvcvideo: Add UVC timestamps support") > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > > > > --- > > > > drivers/media/usb/uvc/uvc_video.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c > > > > index d4b023d4de7c..4ff4ab4471fe 100644 > > > > --- a/drivers/media/usb/uvc/uvc_video.c > > > > +++ b/drivers/media/usb/uvc/uvc_video.c > > > > @@ -732,7 +732,7 @@ void uvc_video_clock_update(struct uvc_streaming *stream, > > > > goto done; > > > > > > > > first = &clock->samples[clock->head]; > > > > - last = &clock->samples[(clock->head - 1) % clock->size]; > > > > + last = &clock->samples[(clock->head - 1 + clock->size) % clock->size]; > > > > > > > > /* First step, PTS to SOF conversion. */ > > > > delta_stc = buf->pts - (1UL << 31); > > -- > Regards, > > Laurent Pinchart
Hi Ricardo, On Mon, Feb 19, 2024 at 04:07:12PM +0100, Ricardo Ribalda wrote: > On Mon, 19 Feb 2024 at 11:40, Laurent Pinchart wrote: > > On Mon, Feb 19, 2024 at 11:28:03AM +0100, Ricardo Ribalda wrote: > > > On Mon, 12 Feb 2024 at 23:59, Laurent Pinchart wrote: > > > > On Wed, Mar 15, 2023 at 02:30:12PM +0100, Ricardo Ribalda wrote: > > > > > If head is 0, last will be addressing the index 0 instead of clock->size > > > > > -1. Luckily clock->head is unsiged, otherwise it would be addressing > > > > > 0xffffffff. > > > > > > > > I'm not following you. In the expression > > > > > > > > (clock->head - 1) % clock->size > > > > > > > > clock->head is an unsigned int, and 1 as a signed int, so the result of > > > > the subtraction is promoted to an unsigned int. When clock->head is 0, the expression evaluates to > > > > > > > > 0xffffffff % clock->size > > > > > > > > clock->size is a power of two (hardcoded to 32 at the moment), so the > > > > expression evaluates to 31, as intended. > > > > > > > > Am I missing something ? > > > > > > Take a look to: https://godbolt.org/z/xYeqTx6ba > > > > > > The expression only works because the size is a power of two. In this > > > set I am allowing sizes that are not powers of two. > > > > Could you then update the commit message to explain that ? > > > > I'll review the rest of the series this week. > Thanks > > Will update with the following text after the review: > > The tail of the list lives at the position before the head. This is > mathematically noted as > ``` > (head-1) mod size. > ``` > > Unfortunately C, does not have a modulus operator, but a remainder > operator (%). > The reminder operation has a different result than the modulus if > (head -1) is a negative number and size is not a power of two. > > Adding size to (head-1) allows the code to run with any value of size. Could you please add This does not change the current behaviour of the driver, as the size is always a power of two, but prepares for reworks that will change the size to a non power of two. or something similar ? > > > > > Nontheless, this is not the intented behaviour and should be fixed. > > > > > > > > > > Fixes: 66847ef013cc ("[media] uvcvideo: Add UVC timestamps support") I think this should be dropped, the patch doesn't fix an issue, but prepares for further changes that add new features. I'd also like to update the commit message to avoid stating "Fix", to avoid this being picked for stable kernels automatically. > > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > > > > > --- > > > > > drivers/media/usb/uvc/uvc_video.c | 2 +- > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c > > > > > index d4b023d4de7c..4ff4ab4471fe 100644 > > > > > --- a/drivers/media/usb/uvc/uvc_video.c > > > > > +++ b/drivers/media/usb/uvc/uvc_video.c > > > > > @@ -732,7 +732,7 @@ void uvc_video_clock_update(struct uvc_streaming *stream, > > > > > goto done; > > > > > > > > > > first = &clock->samples[clock->head]; > > > > > - last = &clock->samples[(clock->head - 1) % clock->size]; > > > > > + last = &clock->samples[(clock->head - 1 + clock->size) % clock->size]; > > > > > > > > > > /* First step, PTS to SOF conversion. */ > > > > > delta_stc = buf->pts - (1UL << 31);
On Thu, Mar 21, 2024 at 11:50:48PM +0200, Laurent Pinchart wrote: > On Mon, Feb 19, 2024 at 04:07:12PM +0100, Ricardo Ribalda wrote: > > On Mon, 19 Feb 2024 at 11:40, Laurent Pinchart wrote: > > > On Mon, Feb 19, 2024 at 11:28:03AM +0100, Ricardo Ribalda wrote: > > > > On Mon, 12 Feb 2024 at 23:59, Laurent Pinchart wrote: > > > > > On Wed, Mar 15, 2023 at 02:30:12PM +0100, Ricardo Ribalda wrote: > > > > > > If head is 0, last will be addressing the index 0 instead of clock->size > > > > > > -1. Luckily clock->head is unsiged, otherwise it would be addressing > > > > > > 0xffffffff. > > > > > > > > > > I'm not following you. In the expression > > > > > > > > > > (clock->head - 1) % clock->size > > > > > > > > > > clock->head is an unsigned int, and 1 as a signed int, so the result of > > > > > the subtraction is promoted to an unsigned int. When clock->head is 0, the expression evaluates to > > > > > > > > > > 0xffffffff % clock->size > > > > > > > > > > clock->size is a power of two (hardcoded to 32 at the moment), so the > > > > > expression evaluates to 31, as intended. > > > > > > > > > > Am I missing something ? > > > > > > > > Take a look to: https://godbolt.org/z/xYeqTx6ba > > > > > > > > The expression only works because the size is a power of two. In this > > > > set I am allowing sizes that are not powers of two. > > > > > > Could you then update the commit message to explain that ? > > > > > > I'll review the rest of the series this week. > > Thanks > > > > Will update with the following text after the review: > > > > The tail of the list lives at the position before the head. This is > > mathematically noted as > > ``` > > (head-1) mod size. > > ``` > > > > Unfortunately C, does not have a modulus operator, but a remainder > > operator (%). > > The reminder operation has a different result than the modulus if > > (head -1) is a negative number and size is not a power of two. > > > > Adding size to (head-1) allows the code to run with any value of size. > > Could you please add > > This does not change the current behaviour of the driver, as the size is > always a power of two, but prepares for reworks that will change the > size to a non power of two. > > or something similar ? > > > > > > > Nontheless, this is not the intented behaviour and should be fixed. > > > > > > > > > > > > Fixes: 66847ef013cc ("[media] uvcvideo: Add UVC timestamps support") > > I think this should be dropped, the patch doesn't fix an issue, but > prepares for further changes that add new features. I'd also like to > update the commit message to avoid stating "Fix", to avoid this being > picked for stable kernels automatically. After reviewing the whole series, it seems that clock->size stays at its current value of 32. Do we thus need this patch ? > > > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > > > > > > --- > > > > > > drivers/media/usb/uvc/uvc_video.c | 2 +- > > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > > > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c > > > > > > index d4b023d4de7c..4ff4ab4471fe 100644 > > > > > > --- a/drivers/media/usb/uvc/uvc_video.c > > > > > > +++ b/drivers/media/usb/uvc/uvc_video.c > > > > > > @@ -732,7 +732,7 @@ void uvc_video_clock_update(struct uvc_streaming *stream, > > > > > > goto done; > > > > > > > > > > > > first = &clock->samples[clock->head]; > > > > > > - last = &clock->samples[(clock->head - 1) % clock->size]; > > > > > > + last = &clock->samples[(clock->head - 1 + clock->size) % clock->size]; > > > > > > > > > > > > /* First step, PTS to SOF conversion. */ > > > > > > delta_stc = buf->pts - (1UL << 31);
Hi Laurent On Fri, 22 Mar 2024 at 10:19, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > On Thu, Mar 21, 2024 at 11:50:48PM +0200, Laurent Pinchart wrote: > > On Mon, Feb 19, 2024 at 04:07:12PM +0100, Ricardo Ribalda wrote: > > > On Mon, 19 Feb 2024 at 11:40, Laurent Pinchart wrote: > > > > On Mon, Feb 19, 2024 at 11:28:03AM +0100, Ricardo Ribalda wrote: > > > > > On Mon, 12 Feb 2024 at 23:59, Laurent Pinchart wrote: > > > > > > On Wed, Mar 15, 2023 at 02:30:12PM +0100, Ricardo Ribalda wrote: > > > > > > > If head is 0, last will be addressing the index 0 instead of clock->size > > > > > > > -1. Luckily clock->head is unsiged, otherwise it would be addressing > > > > > > > 0xffffffff. > > > > > > > > > > > > I'm not following you. In the expression > > > > > > > > > > > > (clock->head - 1) % clock->size > > > > > > > > > > > > clock->head is an unsigned int, and 1 as a signed int, so the result of > > > > > > the subtraction is promoted to an unsigned int. When clock->head is 0, the expression evaluates to > > > > > > > > > > > > 0xffffffff % clock->size > > > > > > > > > > > > clock->size is a power of two (hardcoded to 32 at the moment), so the > > > > > > expression evaluates to 31, as intended. > > > > > > > > > > > > Am I missing something ? > > > > > > > > > > Take a look to: https://godbolt.org/z/xYeqTx6ba > > > > > > > > > > The expression only works because the size is a power of two. In this > > > > > set I am allowing sizes that are not powers of two. > > > > > > > > Could you then update the commit message to explain that ? > > > > > > > > I'll review the rest of the series this week. > > > Thanks > > > > > > Will update with the following text after the review: > > > > > > The tail of the list lives at the position before the head. This is > > > mathematically noted as > > > ``` > > > (head-1) mod size. > > > ``` > > > > > > Unfortunately C, does not have a modulus operator, but a remainder > > > operator (%). > > > The reminder operation has a different result than the modulus if > > > (head -1) is a negative number and size is not a power of two. > > > > > > Adding size to (head-1) allows the code to run with any value of size. > > > > Could you please add > > > > This does not change the current behaviour of the driver, as the size is > > always a power of two, but prepares for reworks that will change the > > size to a non power of two. > > > > or something similar ? > > > > > > > > > Nontheless, this is not the intented behaviour and should be fixed. > > > > > > > > > > > > > > Fixes: 66847ef013cc ("[media] uvcvideo: Add UVC timestamps support") > > > > I think this should be dropped, the patch doesn't fix an issue, but > > prepares for further changes that add new features. I'd also like to > > update the commit message to avoid stating "Fix", to avoid this being > > picked for stable kernels automatically. > > After reviewing the whole series, it seems that clock->size stays at its > current value of 32. Do we thus need this patch ? I remember that at some point, when I changed the size, it had been really painful to figure out why the code was not working. I'd rather keep this patch with a different commit message. """ This does not change the current behaviour of the driver, as the size is always a power of two, but avoid tedious debugging if we ever change its size. """ WDYT? > > > > > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > > > > > > > --- > > > > > > > drivers/media/usb/uvc/uvc_video.c | 2 +- > > > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > > > > > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c > > > > > > > index d4b023d4de7c..4ff4ab4471fe 100644 > > > > > > > --- a/drivers/media/usb/uvc/uvc_video.c > > > > > > > +++ b/drivers/media/usb/uvc/uvc_video.c > > > > > > > @@ -732,7 +732,7 @@ void uvc_video_clock_update(struct uvc_streaming *stream, > > > > > > > goto done; > > > > > > > > > > > > > > first = &clock->samples[clock->head]; > > > > > > > - last = &clock->samples[(clock->head - 1) % clock->size]; > > > > > > > + last = &clock->samples[(clock->head - 1 + clock->size) % clock->size]; > > > > > > > > > > > > > > /* First step, PTS to SOF conversion. */ > > > > > > > delta_stc = buf->pts - (1UL << 31); > > -- > Regards, > > Laurent Pinchart
On Fri, Mar 22, 2024 at 10:29:03AM +0100, Ricardo Ribalda wrote: > On Fri, 22 Mar 2024 at 10:19, Laurent Pinchart wrote: > > On Thu, Mar 21, 2024 at 11:50:48PM +0200, Laurent Pinchart wrote: > > > On Mon, Feb 19, 2024 at 04:07:12PM +0100, Ricardo Ribalda wrote: > > > > On Mon, 19 Feb 2024 at 11:40, Laurent Pinchart wrote: > > > > > On Mon, Feb 19, 2024 at 11:28:03AM +0100, Ricardo Ribalda wrote: > > > > > > On Mon, 12 Feb 2024 at 23:59, Laurent Pinchart wrote: > > > > > > > On Wed, Mar 15, 2023 at 02:30:12PM +0100, Ricardo Ribalda wrote: > > > > > > > > If head is 0, last will be addressing the index 0 instead of clock->size > > > > > > > > -1. Luckily clock->head is unsiged, otherwise it would be addressing > > > > > > > > 0xffffffff. > > > > > > > > > > > > > > I'm not following you. In the expression > > > > > > > > > > > > > > (clock->head - 1) % clock->size > > > > > > > > > > > > > > clock->head is an unsigned int, and 1 as a signed int, so the result of > > > > > > > the subtraction is promoted to an unsigned int. When clock->head is 0, the expression evaluates to > > > > > > > > > > > > > > 0xffffffff % clock->size > > > > > > > > > > > > > > clock->size is a power of two (hardcoded to 32 at the moment), so the > > > > > > > expression evaluates to 31, as intended. > > > > > > > > > > > > > > Am I missing something ? > > > > > > > > > > > > Take a look to: https://godbolt.org/z/xYeqTx6ba > > > > > > > > > > > > The expression only works because the size is a power of two. In this > > > > > > set I am allowing sizes that are not powers of two. > > > > > > > > > > Could you then update the commit message to explain that ? > > > > > > > > > > I'll review the rest of the series this week. > > > > Thanks > > > > > > > > Will update with the following text after the review: > > > > > > > > The tail of the list lives at the position before the head. This is > > > > mathematically noted as > > > > ``` > > > > (head-1) mod size. > > > > ``` > > > > > > > > Unfortunately C, does not have a modulus operator, but a remainder > > > > operator (%). > > > > The reminder operation has a different result than the modulus if > > > > (head -1) is a negative number and size is not a power of two. > > > > > > > > Adding size to (head-1) allows the code to run with any value of size. > > > > > > Could you please add > > > > > > This does not change the current behaviour of the driver, as the size is > > > always a power of two, but prepares for reworks that will change the > > > size to a non power of two. > > > > > > or something similar ? > > > > > > > > > > > Nontheless, this is not the intented behaviour and should be fixed. > > > > > > > > > > > > > > > > Fixes: 66847ef013cc ("[media] uvcvideo: Add UVC timestamps support") > > > > > > I think this should be dropped, the patch doesn't fix an issue, but > > > prepares for further changes that add new features. I'd also like to > > > update the commit message to avoid stating "Fix", to avoid this being > > > picked for stable kernels automatically. > > > > After reviewing the whole series, it seems that clock->size stays at its > > current value of 32. Do we thus need this patch ? > > I remember that at some point, when I changed the size, it had been > really painful to figure out why the code was not working. > > I'd rather keep this patch with a different commit message. > > """ > This does not change the current behaviour of the driver, as the size is > always a power of two, but avoid tedious debugging if we ever change its size. > """ > > WDYT? I'm OK with that. > > > > > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > > > > > > > > --- > > > > > > > > drivers/media/usb/uvc/uvc_video.c | 2 +- > > > > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > > > > > > > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c > > > > > > > > index d4b023d4de7c..4ff4ab4471fe 100644 > > > > > > > > --- a/drivers/media/usb/uvc/uvc_video.c > > > > > > > > +++ b/drivers/media/usb/uvc/uvc_video.c > > > > > > > > @@ -732,7 +732,7 @@ void uvc_video_clock_update(struct uvc_streaming *stream, > > > > > > > > goto done; > > > > > > > > > > > > > > > > first = &clock->samples[clock->head]; > > > > > > > > - last = &clock->samples[(clock->head - 1) % clock->size]; > > > > > > > > + last = &clock->samples[(clock->head - 1 + clock->size) % clock->size]; > > > > > > > > > > > > > > > > /* First step, PTS to SOF conversion. */ > > > > > > > > delta_stc = buf->pts - (1UL << 31);
diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c index d4b023d4de7c..4ff4ab4471fe 100644 --- a/drivers/media/usb/uvc/uvc_video.c +++ b/drivers/media/usb/uvc/uvc_video.c @@ -732,7 +732,7 @@ void uvc_video_clock_update(struct uvc_streaming *stream, goto done; first = &clock->samples[clock->head]; - last = &clock->samples[(clock->head - 1) % clock->size]; + last = &clock->samples[(clock->head - 1 + clock->size) % clock->size]; /* First step, PTS to SOF conversion. */ delta_stc = buf->pts - (1UL << 31);
If head is 0, last will be addressing the index 0 instead of clock->size -1. Luckily clock->head is unsiged, otherwise it would be addressing 0xffffffff. Nontheless, this is not the intented behaviour and should be fixed. Fixes: 66847ef013cc ("[media] uvcvideo: Add UVC timestamps support") Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> --- drivers/media/usb/uvc/uvc_video.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)