| Message ID | 20191123164604.268-6-joevt@shaw.ca (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | edid-decode: bug fixes, additions, changes | expand |
On 11/23/19 5:45 PM, joevt wrote: > - "YCbCr 4:2:0 capability map data block" now outputs the modes that support YCbCr 4:2:0 instead of just indexes of the modes. Indexes refer to timings in "Video Data Block". > - Warnings are included in the output if "Video Data Block" doesn't appear before "YCbCr 4:2:0 Capability Map Data Block" or if the index is outside the range defined in the "Video Data Block". > - Moved inner loop of cta_svd into a new function cta_svd_one so that it can be reused by cta_y420cmdb. This isn't sufficient. There may be multiple SVD blocks in the EDID, and that's not taken into account. Also, I can't find any requirement in the CTA-861 spec that the YCbCr 4:2:0 Capability Map Data Block has to appear after all SVD blocks. So the final check if the Y420CMDB block references invalid SVDs should be postponed to the end of the CTA block. I also found a pre-existing bug: if length == 0 in cta_y420cmdb() then that means that all SVDs support 4:2:0. That should be added to cta_y420cmdb(). Regards, Hans > > Signed-off-by: Joe van Tunen <joevt@shaw.ca> > --- > edid-decode.c | 44 +++++++++++++++++++++++++++++++++++--------- > 1 file changed, 35 insertions(+), 9 deletions(-) > > diff --git a/edid-decode.c b/edid-decode.c > index b833178..4d6fe29 100644 > --- a/edid-decode.c > +++ b/edid-decode.c > @@ -1454,13 +1454,10 @@ static const struct edid_cta_mode *vic_to_mode(unsigned char vic) > return NULL; > } > > -static void cta_svd(const unsigned char *x, unsigned n, int for_ycbcr420) > +static void cta_svd_one(const unsigned char *x, int for_ycbcr420) > { > - unsigned i; > - > - for (i = 0; i < n; i++) { > const struct edid_cta_mode *vicmode = NULL; > - unsigned char svd = x[i]; > + unsigned char svd = x[0]; > unsigned char native; > unsigned char vic; > const char *mode; > @@ -1468,7 +1465,7 @@ static void cta_svd(const unsigned char *x, unsigned n, int for_ycbcr420) > unsigned clock_khz = 0; > > if ((svd & 0x7f) == 0) > - continue; > + return; > > if ((svd - 1) & 0x40) { > vic = svd; > @@ -1511,10 +1508,23 @@ static void cta_svd(const unsigned char *x, unsigned n, int for_ycbcr420) > if (vic == 1) > has_cta861_vic_1 = 1; > } > + > +static void cta_svd(const unsigned char *x, int n, int for_ycbcr420) > +{ > + for (unsigned i = 0; i < n; i++) { > + printf(" "); > + cta_svd_one(x+i, for_ycbcr420); > + printf("\n"); > + } > } > > +unsigned const char *last_cta_video_block_start = NULL; > +unsigned last_cta_video_block_length = 0; > + > static void cta_video_block(const unsigned char *x, unsigned length) > { > + last_cta_video_block_start = x; > + last_cta_video_block_length = length; > cta_svd(x, length, 0); > } > > @@ -1531,9 +1541,25 @@ static void cta_y420cmdb(const unsigned char *x, unsigned length) > uint8_t v = x[0 + i]; > unsigned j; > > - for (j = 0; j < 8; j++) > - if (v & (1 << j)) > - printf(" VSD Index %u\n", i * 8 + j); > + for (j = 0; j < 8; j++) { > + if (v & (1 << j)) { > + unsigned k = i * 8 + j; > + printf(" VSD Index %u", k + 1); > + if (k >= last_cta_video_block_length) { > + if (last_cta_video_block_start) { > + printf(" (out of range)"); > + } else { > + printf(" (missing CTA video block)"); > + } > + } > + else > + { > + printf(" "); > + cta_svd_one(last_cta_video_block_start+k, 0); > + } > + printf("\n"); > + } > + } > } > } > >
Right. The code will have to iterate EDID extension blocks, find CTA extension blocks, then find VDB blocks. In the case where Y420CMDB length = 0, I would maybe output "All VDB SVDs"? Also, shouldn't "VSD Index" be "SVD Index" to match the abbreviations in the spec (or "VDB SVD Index" since it doesn't include SVDs from other sources such as Y420VDB)? Can we assume that an EDID can't use more than one Y420CMDB to get beyond VDB SVD Index 240? Each Y420CMDB will use the same set of SVDs (starting at VDB SVD index 1). The spec explicitly says that bit 0 is the first SVD of the VDBs. On 2019-11-24, 2:26 AM, "Hans Verkuil" <hverkuil@xs4all.nl> wrote: On 11/23/19 5:45 PM, joevt wrote: > - "YCbCr 4:2:0 capability map data block" now outputs the modes that support YCbCr 4:2:0 instead of just indexes of the modes. Indexes refer to timings in "Video Data Block". > - Warnings are included in the output if "Video Data Block" doesn't appear before "YCbCr 4:2:0 Capability Map Data Block" or if the index is outside the range defined in the "Video Data Block". > - Moved inner loop of cta_svd into a new function cta_svd_one so that it can be reused by cta_y420cmdb. This isn't sufficient. There may be multiple SVD blocks in the EDID, and that's not taken into account. Also, I can't find any requirement in the CTA-861 spec that the YCbCr 4:2:0 Capability Map Data Block has to appear after all SVD blocks. So the final check if the Y420CMDB block references invalid SVDs should be postponed to the end of the CTA block. I also found a pre-existing bug: if length == 0 in cta_y420cmdb() then that means that all SVDs support 4:2:0. That should be added to cta_y420cmdb(). Regards, Hans > > Signed-off-by: Joe van Tunen <joevt@shaw.ca> > --- > edid-decode.c | 44 +++++++++++++++++++++++++++++++++++--------- > 1 file changed, 35 insertions(+), 9 deletions(-) > > diff --git a/edid-decode.c b/edid-decode.c > index b833178..4d6fe29 100644 > --- a/edid-decode.c > +++ b/edid-decode.c > @@ -1454,13 +1454,10 @@ static const struct edid_cta_mode *vic_to_mode(unsigned char vic) > return NULL; > } > > -static void cta_svd(const unsigned char *x, unsigned n, int for_ycbcr420) > +static void cta_svd_one(const unsigned char *x, int for_ycbcr420) > { > - unsigned i; > - > - for (i = 0; i < n; i++) { > const struct edid_cta_mode *vicmode = NULL; > - unsigned char svd = x[i]; > + unsigned char svd = x[0]; > unsigned char native; > unsigned char vic; > const char *mode; > @@ -1468,7 +1465,7 @@ static void cta_svd(const unsigned char *x, unsigned n, int for_ycbcr420) > unsigned clock_khz = 0; > > if ((svd & 0x7f) == 0) > - continue; > + return; > > if ((svd - 1) & 0x40) { > vic = svd; > @@ -1511,10 +1508,23 @@ static void cta_svd(const unsigned char *x, unsigned n, int for_ycbcr420) > if (vic == 1) > has_cta861_vic_1 = 1; > } > + > +static void cta_svd(const unsigned char *x, int n, int for_ycbcr420) > +{ > + for (unsigned i = 0; i < n; i++) { > + printf(" "); > + cta_svd_one(x+i, for_ycbcr420); > + printf("\n"); > + } > } > > +unsigned const char *last_cta_video_block_start = NULL; > +unsigned last_cta_video_block_length = 0; > + > static void cta_video_block(const unsigned char *x, unsigned length) > { > + last_cta_video_block_start = x; > + last_cta_video_block_length = length; > cta_svd(x, length, 0); > } > > @@ -1531,9 +1541,25 @@ static void cta_y420cmdb(const unsigned char *x, unsigned length) > uint8_t v = x[0 + i]; > unsigned j; > > - for (j = 0; j < 8; j++) > - if (v & (1 << j)) > - printf(" VSD Index %u\n", i * 8 + j); > + for (j = 0; j < 8; j++) { > + if (v & (1 << j)) { > + unsigned k = i * 8 + j; > + printf(" VSD Index %u", k + 1); > + if (k >= last_cta_video_block_length) { > + if (last_cta_video_block_start) { > + printf(" (out of range)"); > + } else { > + printf(" (missing CTA video block)"); > + } > + } > + else > + { > + printf(" "); > + cta_svd_one(last_cta_video_block_start+k, 0); > + } > + printf("\n"); > + } > + } > } > } > >
On 11/25/19 7:32 AM, Joe van Tunen wrote: > Right. The code will have to iterate EDID extension blocks, find CTA extension blocks, then find VDB blocks. > In the case where Y420CMDB length = 0, I would maybe output "All VDB SVDs"? Yes, > Also, shouldn't "VSD Index" be "SVD Index" to match the abbreviations in the spec (or "VDB SVD Index" since it doesn't include SVDs from other sources such as Y420VDB)? I think 'VBD SVDs' is correct. > > Can we assume that an EDID can't use more than one Y420CMDB to get beyond VDB SVD Index 240? Each Y420CMDB will use the same set of SVDs (starting at VDB SVD index 1). The spec explicitly says that bit 0 is the first SVD of the VDBs. I agree with that assumption. Regards, Hans > > On 2019-11-24, 2:26 AM, "Hans Verkuil" <hverkuil@xs4all.nl> wrote: > > On 11/23/19 5:45 PM, joevt wrote: > > - "YCbCr 4:2:0 capability map data block" now outputs the modes that support YCbCr 4:2:0 instead of just indexes of the modes. Indexes refer to timings in "Video Data Block". > > - Warnings are included in the output if "Video Data Block" doesn't appear before "YCbCr 4:2:0 Capability Map Data Block" or if the index is outside the range defined in the "Video Data Block". > > - Moved inner loop of cta_svd into a new function cta_svd_one so that it can be reused by cta_y420cmdb. > > This isn't sufficient. There may be multiple SVD blocks in the EDID, and that's not taken > into account. > > Also, I can't find any requirement in the CTA-861 spec that the YCbCr 4:2:0 Capability > Map Data Block has to appear after all SVD blocks. So the final check if the Y420CMDB > block references invalid SVDs should be postponed to the end of the CTA block. > > I also found a pre-existing bug: if length == 0 in cta_y420cmdb() then that means that > all SVDs support 4:2:0. That should be added to cta_y420cmdb(). > > Regards, > > Hans > > > > > Signed-off-by: Joe van Tunen <joevt@shaw.ca> > > --- > > edid-decode.c | 44 +++++++++++++++++++++++++++++++++++--------- > > 1 file changed, 35 insertions(+), 9 deletions(-) > > > > diff --git a/edid-decode.c b/edid-decode.c > > index b833178..4d6fe29 100644 > > --- a/edid-decode.c > > +++ b/edid-decode.c > > @@ -1454,13 +1454,10 @@ static const struct edid_cta_mode *vic_to_mode(unsigned char vic) > > return NULL; > > } > > > > -static void cta_svd(const unsigned char *x, unsigned n, int for_ycbcr420) > > +static void cta_svd_one(const unsigned char *x, int for_ycbcr420) > > { > > - unsigned i; > > - > > - for (i = 0; i < n; i++) { > > const struct edid_cta_mode *vicmode = NULL; > > - unsigned char svd = x[i]; > > + unsigned char svd = x[0]; > > unsigned char native; > > unsigned char vic; > > const char *mode; > > @@ -1468,7 +1465,7 @@ static void cta_svd(const unsigned char *x, unsigned n, int for_ycbcr420) > > unsigned clock_khz = 0; > > > > if ((svd & 0x7f) == 0) > > - continue; > > + return; > > > > if ((svd - 1) & 0x40) { > > vic = svd; > > @@ -1511,10 +1508,23 @@ static void cta_svd(const unsigned char *x, unsigned n, int for_ycbcr420) > > if (vic == 1) > > has_cta861_vic_1 = 1; > > } > > + > > +static void cta_svd(const unsigned char *x, int n, int for_ycbcr420) > > +{ > > + for (unsigned i = 0; i < n; i++) { > > + printf(" "); > > + cta_svd_one(x+i, for_ycbcr420); > > + printf("\n"); > > + } > > } > > > > +unsigned const char *last_cta_video_block_start = NULL; > > +unsigned last_cta_video_block_length = 0; > > + > > static void cta_video_block(const unsigned char *x, unsigned length) > > { > > + last_cta_video_block_start = x; > > + last_cta_video_block_length = length; > > cta_svd(x, length, 0); > > } > > > > @@ -1531,9 +1541,25 @@ static void cta_y420cmdb(const unsigned char *x, unsigned length) > > uint8_t v = x[0 + i]; > > unsigned j; > > > > - for (j = 0; j < 8; j++) > > - if (v & (1 << j)) > > - printf(" VSD Index %u\n", i * 8 + j); > > + for (j = 0; j < 8; j++) { > > + if (v & (1 << j)) { > > + unsigned k = i * 8 + j; > > + printf(" VSD Index %u", k + 1); > > + if (k >= last_cta_video_block_length) { > > + if (last_cta_video_block_start) { > > + printf(" (out of range)"); > > + } else { > > + printf(" (missing CTA video block)"); > > + } > > + } > > + else > > + { > > + printf(" "); > > + cta_svd_one(last_cta_video_block_start+k, 0); > > + } > > + printf("\n"); > > + } > > + } > > } > > } > > > > > > > >
diff --git a/edid-decode.c b/edid-decode.c index b833178..4d6fe29 100644 --- a/edid-decode.c +++ b/edid-decode.c @@ -1454,13 +1454,10 @@ static const struct edid_cta_mode *vic_to_mode(unsigned char vic) return NULL; } -static void cta_svd(const unsigned char *x, unsigned n, int for_ycbcr420) +static void cta_svd_one(const unsigned char *x, int for_ycbcr420) { - unsigned i; - - for (i = 0; i < n; i++) { const struct edid_cta_mode *vicmode = NULL; - unsigned char svd = x[i]; + unsigned char svd = x[0]; unsigned char native; unsigned char vic; const char *mode; @@ -1468,7 +1465,7 @@ static void cta_svd(const unsigned char *x, unsigned n, int for_ycbcr420) unsigned clock_khz = 0; if ((svd & 0x7f) == 0) - continue; + return; if ((svd - 1) & 0x40) { vic = svd; @@ -1511,10 +1508,23 @@ static void cta_svd(const unsigned char *x, unsigned n, int for_ycbcr420) if (vic == 1) has_cta861_vic_1 = 1; } + +static void cta_svd(const unsigned char *x, int n, int for_ycbcr420) +{ + for (unsigned i = 0; i < n; i++) { + printf(" "); + cta_svd_one(x+i, for_ycbcr420); + printf("\n"); + } } +unsigned const char *last_cta_video_block_start = NULL; +unsigned last_cta_video_block_length = 0; + static void cta_video_block(const unsigned char *x, unsigned length) { + last_cta_video_block_start = x; + last_cta_video_block_length = length; cta_svd(x, length, 0); } @@ -1531,9 +1541,25 @@ static void cta_y420cmdb(const unsigned char *x, unsigned length) uint8_t v = x[0 + i]; unsigned j; - for (j = 0; j < 8; j++) - if (v & (1 << j)) - printf(" VSD Index %u\n", i * 8 + j); + for (j = 0; j < 8; j++) { + if (v & (1 << j)) { + unsigned k = i * 8 + j; + printf(" VSD Index %u", k + 1); + if (k >= last_cta_video_block_length) { + if (last_cta_video_block_start) { + printf(" (out of range)"); + } else { + printf(" (missing CTA video block)"); + } + } + else + { + printf(" "); + cta_svd_one(last_cta_video_block_start+k, 0); + } + printf("\n"); + } + } } }
- "YCbCr 4:2:0 capability map data block" now outputs the modes that support YCbCr 4:2:0 instead of just indexes of the modes. Indexes refer to timings in "Video Data Block". - Warnings are included in the output if "Video Data Block" doesn't appear before "YCbCr 4:2:0 Capability Map Data Block" or if the index is outside the range defined in the "Video Data Block". - Moved inner loop of cta_svd into a new function cta_svd_one so that it can be reused by cta_y420cmdb. Signed-off-by: Joe van Tunen <joevt@shaw.ca> --- edid-decode.c | 44 +++++++++++++++++++++++++++++++++++--------- 1 file changed, 35 insertions(+), 9 deletions(-)