| Message ID | 20220718165649.16407-1-jernej.skrabec@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: cedrus: hevc: Add check for invalid timestamp | expand |
Le lundi 18 juillet 2022 à 18:56 +0200, Jernej Skrabec a écrit : > Not all DPB entries will be used most of the time. Unused entries will > thus have invalid timestamps. They will produce negative buffer index > which is not specifically handled. This works just by chance in current > code. It will even produce bogus pointer, but since it's not used, it > won't do any harm. > > Let's fix that brittle design by skipping writing DPB entry altogether > if timestamp is invalid. > > Fixes: 86caab29da78 ("media: cedrus: Add HEVC/H.265 decoding support") > Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com> > --- > drivers/staging/media/sunxi/cedrus/cedrus_h265.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c > index 1afc6797d806..687f87598f78 100644 > --- a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c > @@ -147,6 +147,9 @@ static void cedrus_h265_frame_info_write_dpb(struct cedrus_ctx *ctx, > dpb[i].pic_order_cnt_val > }; > > + if (buffer_index < 0) > + continue; When I compare to other codecs, when the buffer_index does not exist, the addr 0 is being programmed into the HW. With this implementation is is left to whatever it was set for the previous decode operation. I think its is nicer done the other way. > + > cedrus_h265_frame_info_write_single(ctx, i, dpb[i].field_pic, > pic_order_cnt, > buffer_index);
Dne ponedeljek, 18. julij 2022 ob 19:41:48 CEST je Nicolas Dufresne napisal(a): > Le lundi 18 juillet 2022 à 18:56 +0200, Jernej Skrabec a écrit : > > Not all DPB entries will be used most of the time. Unused entries will > > thus have invalid timestamps. They will produce negative buffer index > > which is not specifically handled. This works just by chance in current > > code. It will even produce bogus pointer, but since it's not used, it > > won't do any harm. > > > > Let's fix that brittle design by skipping writing DPB entry altogether > > if timestamp is invalid. > > > > Fixes: 86caab29da78 ("media: cedrus: Add HEVC/H.265 decoding support") > > Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com> > > --- > > > > drivers/staging/media/sunxi/cedrus/cedrus_h265.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c > > b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c index > > 1afc6797d806..687f87598f78 100644 > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c > > @@ -147,6 +147,9 @@ static void cedrus_h265_frame_info_write_dpb(struct > > cedrus_ctx *ctx,> > > dpb[i].pic_order_cnt_val > > > > }; > > > > + if (buffer_index < 0) > > + continue; > > When I compare to other codecs, when the buffer_index does not exist, the > addr 0 is being programmed into the HW. With this implementation is is left > to whatever it was set for the previous decode operation. I think its is > nicer done the other way. It's done the same way as it's done in vendor lib. As I stated in commit message, actual values don't matter for unused entries. If it is used by accident, HW reaction on all zero pointers can only be worse than using old, but valid entry. Due to no real documentation and Allwinner unwillingness to share details, we'll probably never know what's best response for each error. Some things can be deduced from vendor code, but not all. I would rather not complicate this fix, especially since it's candidate for backporting. Best regards, Jernej > > > + > > > > cedrus_h265_frame_info_write_single(ctx, i, dpb[i].field_pic, > > > > pic_order_cnt, > > buffer_index);
Le lundi 18 juillet 2022 à 19:57 +0200, Jernej Škrabec a écrit : > Dne ponedeljek, 18. julij 2022 ob 19:41:48 CEST je Nicolas Dufresne > napisal(a): > > Le lundi 18 juillet 2022 à 18:56 +0200, Jernej Skrabec a écrit : > > > Not all DPB entries will be used most of the time. Unused entries will > > > thus have invalid timestamps. They will produce negative buffer index > > > which is not specifically handled. This works just by chance in current > > > code. It will even produce bogus pointer, but since it's not used, it > > > won't do any harm. > > > > > > Let's fix that brittle design by skipping writing DPB entry altogether > > > if timestamp is invalid. > > > > > > Fixes: 86caab29da78 ("media: cedrus: Add HEVC/H.265 decoding support") > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com> > > > --- > > > > > > drivers/staging/media/sunxi/cedrus/cedrus_h265.c | 3 +++ > > > 1 file changed, 3 insertions(+) > > > > > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c > > > b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c index > > > 1afc6797d806..687f87598f78 100644 > > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c > > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c > > > @@ -147,6 +147,9 @@ static void cedrus_h265_frame_info_write_dpb(struct > > > cedrus_ctx *ctx,> > > > dpb[i].pic_order_cnt_val > > > > > > }; > > > > > > + if (buffer_index < 0) > > > + continue; > > > > When I compare to other codecs, when the buffer_index does not exist, the > > addr 0 is being programmed into the HW. With this implementation is is left > > to whatever it was set for the previous decode operation. I think its is > > nicer done the other way. > > It's done the same way as it's done in vendor lib. As I stated in commit > message, actual values don't matter for unused entries. If it is used by > accident, HW reaction on all zero pointers can only be worse than using old, > but valid entry. > > Due to no real documentation and Allwinner unwillingness to share details, > we'll probably never know what's best response for each error. Some things can > be deduced from vendor code, but not all. > > I would rather not complicate this fix, especially since it's candidate for > backporting. I am simply trying to highlight that this is not consistant with how the H264 part is done. Why do we reset the register for one codec and not the other ? Perhaps you should sync to your preference the driver as a whole. It also seems that before your patch, some bits would be 0 and some other would be very large values. Between this and leaving random value, I don't really see any gain or reason for a backport. It neither break or fix anything as far as I understand. My general opinion, is that we fixe the unused address (like to 0) then when something goes wrong, as least it will go wrong consistently. > > Best regards, > Jernej > > > > > > + > > > > > > cedrus_h265_frame_info_write_single(ctx, i, > dpb[i].field_pic, > > > > > > > pic_order_cnt, > > > > buffer_index); > >
Dne ponedeljek, 18. julij 2022 ob 21:37:31 CEST je Nicolas Dufresne napisal(a): > Le lundi 18 juillet 2022 à 19:57 +0200, Jernej Škrabec a écrit : > > Dne ponedeljek, 18. julij 2022 ob 19:41:48 CEST je Nicolas Dufresne > > > > napisal(a): > > > Le lundi 18 juillet 2022 à 18:56 +0200, Jernej Skrabec a écrit : > > > > Not all DPB entries will be used most of the time. Unused entries will > > > > thus have invalid timestamps. They will produce negative buffer index > > > > which is not specifically handled. This works just by chance in > > > > current > > > > code. It will even produce bogus pointer, but since it's not used, it > > > > won't do any harm. > > > > > > > > Let's fix that brittle design by skipping writing DPB entry altogether > > > > if timestamp is invalid. > > > > > > > > Fixes: 86caab29da78 ("media: cedrus: Add HEVC/H.265 decoding support") > > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com> > > > > --- > > > > > > > > drivers/staging/media/sunxi/cedrus/cedrus_h265.c | 3 +++ > > > > 1 file changed, 3 insertions(+) > > > > > > > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c > > > > b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c index > > > > 1afc6797d806..687f87598f78 100644 > > > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c > > > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c > > > > @@ -147,6 +147,9 @@ static void > > > > cedrus_h265_frame_info_write_dpb(struct > > > > cedrus_ctx *ctx,> > > > > > > > > dpb[i].pic_order_cnt_val > > > > > > > > }; > > > > > > > > + if (buffer_index < 0) > > > > + continue; > > > > > > When I compare to other codecs, when the buffer_index does not exist, > > > the > > > addr 0 is being programmed into the HW. With this implementation is is > > > left > > > to whatever it was set for the previous decode operation. I think its is > > > nicer done the other way. > > > > It's done the same way as it's done in vendor lib. As I stated in commit > > message, actual values don't matter for unused entries. If it is used by > > accident, HW reaction on all zero pointers can only be worse than using > > old, but valid entry. > > > > Due to no real documentation and Allwinner unwillingness to share details, > > we'll probably never know what's best response for each error. Some things > > can be deduced from vendor code, but not all. > > > > I would rather not complicate this fix, especially since it's candidate > > for > > backporting. > > I am simply trying to highlight that this is not consistant with how the > H264 part is done. Why do we reset the register for one codec and not the > other ? While H264 and HEVC are similar in many ways, Cedrus uses two different cores or in Cedrus slang, engines, for them. They have their own quirks. One of the most apparent is handling of DPB array. H264 requires that same entry is always at the same position in HW DPB. That's not required by HEVC. Additional reasons for differences come from the fact that it's from two different authors (Maxime and Paul). Those differences were created at the beginning and it is what it is. > > Perhaps you should sync to your preference the driver as a whole. It also > seems that before your patch, some bits would be 0 and some other would be > very large values. Between this and leaving random value, I don't really > see any gain or reason for a backport. It neither break or fix anything as > far as I understand. Maybe there is no need to backport, but the change is nevertheless useful. As I explained, current code works only by chance, as we noticed with Ezequiel's rework. It's certainly worthwhile to make code less brittle. As far as I'm concerned, fixes tag can be dropped or even Ezequiel can squash this change into his commit, with appropriate adjustments, of course. I'm not completely sure what do you mean by syncing driver preference. Code changes always need a good reason to be accepted. Moving code around and renaming things just to be similar with something else is not. Best regards, Jernej > > My general opinion, is that we fixe the unused address (like to 0) then when > something goes wrong, as least it will go wrong consistently. > > > Best regards, > > Jernej > > > > > > + > > > > > > > > cedrus_h265_frame_info_write_single(ctx, i, > > > > dpb[i].field_pic, > > > > > > pic_order_cnt, > > > > buffer_index);
Hi Jernej, On Mon, Jul 18, 2022 at 10:34:37PM +0200, Jernej Škrabec wrote: > Dne ponedeljek, 18. julij 2022 ob 21:37:31 CEST je Nicolas Dufresne > napisal(a): > > Le lundi 18 juillet 2022 à 19:57 +0200, Jernej Škrabec a écrit : > > > Dne ponedeljek, 18. julij 2022 ob 19:41:48 CEST je Nicolas Dufresne > > > > > > napisal(a): > > > > Le lundi 18 juillet 2022 à 18:56 +0200, Jernej Skrabec a écrit : > > > > > Not all DPB entries will be used most of the time. Unused entries will > > > > > thus have invalid timestamps. They will produce negative buffer index > > > > > which is not specifically handled. This works just by chance in > > > > > current > > > > > code. It will even produce bogus pointer, but since it's not used, it > > > > > won't do any harm. > > > > > > > > > > Let's fix that brittle design by skipping writing DPB entry altogether > > > > > if timestamp is invalid. > > > > > > > > > > Fixes: 86caab29da78 ("media: cedrus: Add HEVC/H.265 decoding support") > > > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com> > > > > > --- > > > > > > > > > > drivers/staging/media/sunxi/cedrus/cedrus_h265.c | 3 +++ > > > > > 1 file changed, 3 insertions(+) > > > > > > > > > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c > > > > > b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c index > > > > > 1afc6797d806..687f87598f78 100644 > > > > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c > > > > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c > > > > > @@ -147,6 +147,9 @@ static void > > > > > cedrus_h265_frame_info_write_dpb(struct > > > > > cedrus_ctx *ctx,> > > > > > > > > > > dpb[i].pic_order_cnt_val > > > > > > > > > > }; > > > > > > > > > > + if (buffer_index < 0) > > > > > + continue; > > > > > > > > When I compare to other codecs, when the buffer_index does not exist, > > > > the > > > > addr 0 is being programmed into the HW. With this implementation is is > > > > left > > > > to whatever it was set for the previous decode operation. I think its is > > > > nicer done the other way. > > > > > > It's done the same way as it's done in vendor lib. As I stated in commit > > > message, actual values don't matter for unused entries. If it is used by > > > accident, HW reaction on all zero pointers can only be worse than using > > > old, but valid entry. > > > > > > Due to no real documentation and Allwinner unwillingness to share details, > > > we'll probably never know what's best response for each error. Some things > > > can be deduced from vendor code, but not all. > > > > > > I would rather not complicate this fix, especially since it's candidate > > > for > > > backporting. I think this makes sense, since it allows to fix the bug for the time being. Reviewed-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> Thanks! Ezequiel > > > > I am simply trying to highlight that this is not consistant with how the > > H264 part is done. Why do we reset the register for one codec and not the > > other ? > > While H264 and HEVC are similar in many ways, Cedrus uses two different cores > or in Cedrus slang, engines, for them. They have their own quirks. One of the > most apparent is handling of DPB array. H264 requires that same entry is > always at the same position in HW DPB. That's not required by HEVC. > > Additional reasons for differences come from the fact that it's from two > different authors (Maxime and Paul). Those differences were created at the > beginning and it is what it is. > > > > > Perhaps you should sync to your preference the driver as a whole. It also > > seems that before your patch, some bits would be 0 and some other would be > > very large values. Between this and leaving random value, I don't really > > see any gain or reason for a backport. It neither break or fix anything as > > far as I understand. > > Maybe there is no need to backport, but the change is nevertheless useful. As > I explained, current code works only by chance, as we noticed with Ezequiel's > rework. It's certainly worthwhile to make code less brittle. As far as I'm > concerned, fixes tag can be dropped or even Ezequiel can squash this change > into his commit, with appropriate adjustments, of course. > > I'm not completely sure what do you mean by syncing driver preference. Code > changes always need a good reason to be accepted. Moving code around and > renaming things just to be similar with something else is not. > > Best regards, > Jernej > > > > > My general opinion, is that we fixe the unused address (like to 0) then when > > something goes wrong, as least it will go wrong consistently. > > > > > Best regards, > > > Jernej > > > > > > > > + > > > > > > > > > > cedrus_h265_frame_info_write_single(ctx, i, > > > > > > dpb[i].field_pic, > > > > > > > > > pic_order_cnt, > > > > > > buffer_index); > > > >
Le lundi 18 juillet 2022 à 18:49 -0300, Ezequiel Garcia a écrit : > Hi Jernej, > > On Mon, Jul 18, 2022 at 10:34:37PM +0200, Jernej Škrabec wrote: > > Dne ponedeljek, 18. julij 2022 ob 21:37:31 CEST je Nicolas Dufresne > > napisal(a): > > > Le lundi 18 juillet 2022 à 19:57 +0200, Jernej Škrabec a écrit : > > > > Dne ponedeljek, 18. julij 2022 ob 19:41:48 CEST je Nicolas Dufresne > > > > > > > > napisal(a): > > > > > Le lundi 18 juillet 2022 à 18:56 +0200, Jernej Skrabec a écrit : > > > > > > Not all DPB entries will be used most of the time. Unused entries will > > > > > > thus have invalid timestamps. They will produce negative buffer index > > > > > > which is not specifically handled. This works just by chance in > > > > > > current > > > > > > code. It will even produce bogus pointer, but since it's not used, it > > > > > > won't do any harm. > > > > > > > > > > > > Let's fix that brittle design by skipping writing DPB entry altogether > > > > > > if timestamp is invalid. > > > > > > > > > > > > Fixes: 86caab29da78 ("media: cedrus: Add HEVC/H.265 decoding support") > > > > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com> > > > > > > --- > > > > > > > > > > > > drivers/staging/media/sunxi/cedrus/cedrus_h265.c | 3 +++ > > > > > > 1 file changed, 3 insertions(+) > > > > > > > > > > > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c > > > > > > b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c index > > > > > > 1afc6797d806..687f87598f78 100644 > > > > > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c > > > > > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c > > > > > > @@ -147,6 +147,9 @@ static void > > > > > > cedrus_h265_frame_info_write_dpb(struct > > > > > > cedrus_ctx *ctx,> > > > > > > > > > > > > dpb[i].pic_order_cnt_val > > > > > > > > > > > > }; > > > > > > > > > > > > + if (buffer_index < 0) > > > > > > + continue; > > > > > > > > > > When I compare to other codecs, when the buffer_index does not exist, > > > > > the > > > > > addr 0 is being programmed into the HW. With this implementation is is > > > > > left > > > > > to whatever it was set for the previous decode operation. I think its is > > > > > nicer done the other way. > > > > > > > > It's done the same way as it's done in vendor lib. As I stated in commit > > > > message, actual values don't matter for unused entries. If it is used by > > > > accident, HW reaction on all zero pointers can only be worse than using > > > > old, but valid entry. > > > > > > > > Due to no real documentation and Allwinner unwillingness to share details, > > > > we'll probably never know what's best response for each error. Some things > > > > can be deduced from vendor code, but not all. > > > > > > > > I would rather not complicate this fix, especially since it's candidate > > > > for > > > > backporting. > > I think this makes sense, since it allows to fix the bug for the time > being. > > Reviewed-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> Ack. > > Thanks! > Ezequiel > > > > > > > I am simply trying to highlight that this is not consistant with how the > > > H264 part is done. Why do we reset the register for one codec and not the > > > other ? > > > > While H264 and HEVC are similar in many ways, Cedrus uses two different cores > > or in Cedrus slang, engines, for them. They have their own quirks. One of the > > most apparent is handling of DPB array. H264 requires that same entry is > > always at the same position in HW DPB. That's not required by HEVC. > > > > Additional reasons for differences come from the fact that it's from two > > different authors (Maxime and Paul). Those differences were created at the > > beginning and it is what it is. > > > > > > > > Perhaps you should sync to your preference the driver as a whole. It also > > > seems that before your patch, some bits would be 0 and some other would be > > > very large values. Between this and leaving random value, I don't really > > > see any gain or reason for a backport. It neither break or fix anything as > > > far as I understand. > > > > Maybe there is no need to backport, but the change is nevertheless useful. As > > I explained, current code works only by chance, as we noticed with Ezequiel's > > rework. It's certainly worthwhile to make code less brittle. As far as I'm > > concerned, fixes tag can be dropped or even Ezequiel can squash this change > > into his commit, with appropriate adjustments, of course. > > > > I'm not completely sure what do you mean by syncing driver preference. Code > > changes always need a good reason to be accepted. Moving code around and > > renaming things just to be similar with something else is not. > > > > Best regards, > > Jernej > > > > > > > > My general opinion, is that we fixe the unused address (like to 0) then when > > > something goes wrong, as least it will go wrong consistently. > > > > > > > Best regards, > > > > Jernej > > > > > > > > > > + > > > > > > > > > > > > cedrus_h265_frame_info_write_single(ctx, i, > > > > > > > > dpb[i].field_pic, > > > > > > > > > > > > pic_order_cnt, > > > > > > > > buffer_index); > > > > > > > >
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c index 1afc6797d806..687f87598f78 100644 --- a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c @@ -147,6 +147,9 @@ static void cedrus_h265_frame_info_write_dpb(struct cedrus_ctx *ctx, dpb[i].pic_order_cnt_val }; + if (buffer_index < 0) + continue; + cedrus_h265_frame_info_write_single(ctx, i, dpb[i].field_pic, pic_order_cnt, buffer_index);
Not all DPB entries will be used most of the time. Unused entries will thus have invalid timestamps. They will produce negative buffer index which is not specifically handled. This works just by chance in current code. It will even produce bogus pointer, but since it's not used, it won't do any harm. Let's fix that brittle design by skipping writing DPB entry altogether if timestamp is invalid. Fixes: 86caab29da78 ("media: cedrus: Add HEVC/H.265 decoding support") Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com> --- drivers/staging/media/sunxi/cedrus/cedrus_h265.c | 3 +++ 1 file changed, 3 insertions(+)