Re: [Intel-gfx] [v8 3/5] drm/edid: read HF-EEODB ext block
On Wed, Mar 23, 2022 at 12:11:50PM +0200, Jani Nikula wrote: > On Thu, 17 Mar 2022, Lee Shawn C wrote: > > According to HDMI 2.1 spec. > > > > "The HDMI Forum EDID Extension Override Data Block (HF-EEODB) > > is utilized by Sink Devices to provide an alternate method to > > indicate an EDID Extension Block count larger than 1, while > > avoiding the need to present a VESA Block Map in the first > > E-EDID Extension Block." > > > > It is a mandatory for HDMI 2.1 protocol compliance as well. > > This patch help to know how many HF_EEODB blocks report by sink > > and read allo HF_EEODB blocks back. > > It still just boggles my mind that they've implemented something like > this. They cite avoiding the EDID Block Map as the rationale... but it's > been optional since E-EDID structure v1.4, published in 2006. 15+ years > ago. > > Can anyone tell me a sane reason for this? What does it provide that > E-EDID 1.4 does not? Do they want to use E-EDID v1.3 with this? Why? Looks to be pretty much the same approach as the DPCD extended receiver cap mess we already have to deal with. So I presume this is a hack to avoid breaking some garbage source devices that explode when they see too many extension blocks, or something along those lines. -- Ville Syrjälä Intel
Re: [Intel-gfx] [v8 3/5] drm/edid: read HF-EEODB ext block
On Wed, 23 Mar 2022, Simon Ser wrote: > On Wednesday, March 23rd, 2022 at 13:02, Jani Nikula > wrote: > >> Simon and Daniel also tell me on IRC we can't just modify the base block >> extension count to match HF-EEODB to dodge the problem, because the EDID >> gets exposed to userspace. > > I'm not familiar how the EDID blob gets exposed to user-space. If the > EDID data gets copied when creating the blob, and the blob is created > before the kernel mutates the EDID to accomodate for HF-EEODB, then > this proposal might still be workable. You'd still end up with tracking separate copies of the EDID, which is not necessarily easier. There are almost 70 calls to update the connector EDID property that gets exposed to userspace, and the call sites would need to know which copy to pass. BR, Jani. -- Jani Nikula, Intel Open Source Graphics Center
Re: [Intel-gfx] [v8 3/5] drm/edid: read HF-EEODB ext block
On Wednesday, March 23rd, 2022 at 13:02, Jani Nikula wrote: > Simon and Daniel also tell me on IRC we can't just modify the base block > extension count to match HF-EEODB to dodge the problem, because the EDID > gets exposed to userspace. I'm not familiar how the EDID blob gets exposed to user-space. If the EDID data gets copied when creating the blob, and the blob is created before the kernel mutates the EDID to accomodate for HF-EEODB, then this proposal might still be workable.
Re: [Intel-gfx] [v8 3/5] drm/edid: read HF-EEODB ext block
On Wed, 23 Mar 2022, Jani Nikula wrote:
> On Thu, 17 Mar 2022, Lee Shawn C wrote:
>> According to HDMI 2.1 spec.
>>
>> "The HDMI Forum EDID Extension Override Data Block (HF-EEODB)
>> is utilized by Sink Devices to provide an alternate method to
>> indicate an EDID Extension Block count larger than 1, while
>> avoiding the need to present a VESA Block Map in the first
>> E-EDID Extension Block."
>>
>> It is a mandatory for HDMI 2.1 protocol compliance as well.
>> This patch help to know how many HF_EEODB blocks report by sink
>> and read allo HF_EEODB blocks back.
>
> It still just boggles my mind that they've implemented something like
> this. They cite avoiding the EDID Block Map as the rationale... but it's
> been optional since E-EDID structure v1.4, published in 2006. 15+ years
> ago.
>
> Can anyone tell me a sane reason for this? What does it provide that
> E-EDID 1.4 does not? Do they want to use E-EDID v1.3 with this? Why?
Oh, and the main problem with the whole thing, and this implementation?
If you have a struct edid *, there's no way way to tell if whoever
allocated the memory or copied the block was actually HF-EEODB aware.
Basically we'd have to audit every single piece of EDID handling code
across all drivers to see if they handle HF-EEODB properly. If we don't,
it's a question of time a struct edid * with HF-EEODB but no memory
allocated for the extensions gets passed to a function that understands
it, and overflows the buffer.
Simon and Daniel also tell me on IRC we can't just modify the base block
extension count to match HF-EEODB to dodge the problem, because the EDID
gets exposed to userspace.
BR,
Jani.
>
> BR,
> Jani.
>
>
>>
>> v2: support to find CEA block, check EEODB block format, and return
>> available block number in drm_edid_read_hf_eeodb_blk_count().
>>
>> Cc: Jani Nikula
>> Cc: Ville Syrjala
>> Cc: Ankit Nautiyal
>> Cc: intel-gfx
>> Signed-off-by: Lee Shawn C
>> ---
>> drivers/gpu/drm/drm_connector.c | 8 +++-
>> drivers/gpu/drm/drm_edid.c | 71 +++--
>> include/drm/drm_edid.h | 2 +-
>> 3 files changed, 74 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_connector.c
>> b/drivers/gpu/drm/drm_connector.c
>> index a50c82bc2b2f..16011023c12e 100644
>> --- a/drivers/gpu/drm/drm_connector.c
>> +++ b/drivers/gpu/drm/drm_connector.c
>> @@ -2129,7 +2129,7 @@ int drm_connector_update_edid_property(struct
>> drm_connector *connector,
>> const struct edid *edid)
>> {
>> struct drm_device *dev = connector->dev;
>> -size_t size = 0;
>> +size_t size = 0, hf_eeodb_blk_count;
>> int ret;
>> const struct edid *old_edid;
>>
>> @@ -2137,8 +2137,12 @@ int drm_connector_update_edid_property(struct
>> drm_connector *connector,
>> if (connector->override_edid)
>> return 0;
>>
>> -if (edid)
>> +if (edid) {
>> size = EDID_LENGTH * (1 + edid->extensions);
>> +hf_eeodb_blk_count = drm_edid_read_hf_eeodb_blk_count(edid);
>> +if (hf_eeodb_blk_count)
>> +size = EDID_LENGTH * (1 + hf_eeodb_blk_count);
>> +}
>>
>> /* Set the display info, using edid if available, otherwise
>> * resetting the values to defaults. This duplicates the work
>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>> index ef65dd97d700..890038758660 100644
>> --- a/drivers/gpu/drm/drm_edid.c
>> +++ b/drivers/gpu/drm/drm_edid.c
>> @@ -1992,6 +1992,7 @@ struct edid *drm_do_get_edid(struct drm_connector
>> *connector,
>> {
>> int i, j = 0, valid_extensions = 0;
>> u8 *edid, *new;
>> +size_t hf_eeodb_blk_count;
>> struct edid *override;
>>
>> override = drm_get_override_edid(connector);
>> @@ -2051,7 +2052,35 @@ struct edid *drm_do_get_edid(struct drm_connector
>> *connector,
>> }
>>
>> kfree(edid);
>> +return (struct edid *)new;
>> +}
>> +
>> +hf_eeodb_blk_count = drm_edid_read_hf_eeodb_blk_count((struct edid
>> *)edid);
>> +if (hf_eeodb_blk_count >= 2) {
>> +new = krealloc(edid, (hf_eeodb_blk_count + 1) * EDID_LENGTH,
>> GFP_KERNEL);
>> +if (!new)
>> +goto out;
>> edid = new;
>> +
>> +valid_extensions = hf_eeodb_blk_count - 1;
>> +for (j = 2; j <= hf_eeodb_blk_count; j++) {
>> +u8 *block = edid + j * EDID_LENGTH;
>> +
>> +for (i = 0; i < 4; i++) {
>> +if (get_edid_block(data, block, j, EDID_LENGTH))
>> +goto out;
>> +if (drm_edid_block_valid(block, j, false, NULL))
>> +break;
>> +}
>> +
>> +if (i == 4)
>> +valid_extensions--;
>> +}
>> +
>> +if
Re: [Intel-gfx] [v8 3/5] drm/edid: read HF-EEODB ext block
On Thu, 17 Mar 2022, Lee Shawn C wrote:
> According to HDMI 2.1 spec.
>
> "The HDMI Forum EDID Extension Override Data Block (HF-EEODB)
> is utilized by Sink Devices to provide an alternate method to
> indicate an EDID Extension Block count larger than 1, while
> avoiding the need to present a VESA Block Map in the first
> E-EDID Extension Block."
>
> It is a mandatory for HDMI 2.1 protocol compliance as well.
> This patch help to know how many HF_EEODB blocks report by sink
> and read allo HF_EEODB blocks back.
It still just boggles my mind that they've implemented something like
this. They cite avoiding the EDID Block Map as the rationale... but it's
been optional since E-EDID structure v1.4, published in 2006. 15+ years
ago.
Can anyone tell me a sane reason for this? What does it provide that
E-EDID 1.4 does not? Do they want to use E-EDID v1.3 with this? Why?
BR,
Jani.
>
> v2: support to find CEA block, check EEODB block format, and return
> available block number in drm_edid_read_hf_eeodb_blk_count().
>
> Cc: Jani Nikula
> Cc: Ville Syrjala
> Cc: Ankit Nautiyal
> Cc: intel-gfx
> Signed-off-by: Lee Shawn C
> ---
> drivers/gpu/drm/drm_connector.c | 8 +++-
> drivers/gpu/drm/drm_edid.c | 71 +++--
> include/drm/drm_edid.h | 2 +-
> 3 files changed, 74 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index a50c82bc2b2f..16011023c12e 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -2129,7 +2129,7 @@ int drm_connector_update_edid_property(struct
> drm_connector *connector,
> const struct edid *edid)
> {
> struct drm_device *dev = connector->dev;
> - size_t size = 0;
> + size_t size = 0, hf_eeodb_blk_count;
> int ret;
> const struct edid *old_edid;
>
> @@ -2137,8 +2137,12 @@ int drm_connector_update_edid_property(struct
> drm_connector *connector,
> if (connector->override_edid)
> return 0;
>
> - if (edid)
> + if (edid) {
> size = EDID_LENGTH * (1 + edid->extensions);
> + hf_eeodb_blk_count = drm_edid_read_hf_eeodb_blk_count(edid);
> + if (hf_eeodb_blk_count)
> + size = EDID_LENGTH * (1 + hf_eeodb_blk_count);
> + }
>
> /* Set the display info, using edid if available, otherwise
>* resetting the values to defaults. This duplicates the work
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index ef65dd97d700..890038758660 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -1992,6 +1992,7 @@ struct edid *drm_do_get_edid(struct drm_connector
> *connector,
> {
> int i, j = 0, valid_extensions = 0;
> u8 *edid, *new;
> + size_t hf_eeodb_blk_count;
> struct edid *override;
>
> override = drm_get_override_edid(connector);
> @@ -2051,7 +2052,35 @@ struct edid *drm_do_get_edid(struct drm_connector
> *connector,
> }
>
> kfree(edid);
> + return (struct edid *)new;
> + }
> +
> + hf_eeodb_blk_count = drm_edid_read_hf_eeodb_blk_count((struct edid
> *)edid);
> + if (hf_eeodb_blk_count >= 2) {
> + new = krealloc(edid, (hf_eeodb_blk_count + 1) * EDID_LENGTH,
> GFP_KERNEL);
> + if (!new)
> + goto out;
> edid = new;
> +
> + valid_extensions = hf_eeodb_blk_count - 1;
> + for (j = 2; j <= hf_eeodb_blk_count; j++) {
> + u8 *block = edid + j * EDID_LENGTH;
> +
> + for (i = 0; i < 4; i++) {
> + if (get_edid_block(data, block, j, EDID_LENGTH))
> + goto out;
> + if (drm_edid_block_valid(block, j, false, NULL))
> + break;
> + }
> +
> + if (i == 4)
> + valid_extensions--;
> + }
> +
> + if (valid_extensions != hf_eeodb_blk_count - 1) {
> + DRM_ERROR("Not able to retrieve proper EDID contain
> HF-EEODB data.\n");
> + goto out;
> + }
> }
>
> return (struct edid *)edid;
> @@ -3315,15 +3344,17 @@ add_detailed_modes(struct drm_connector *connector,
> struct edid *edid,
> #define VIDEO_BLOCK 0x02
> #define VENDOR_BLOCK0x03
> #define SPEAKER_BLOCK0x04
> -#define HDR_STATIC_METADATA_BLOCK0x6
> -#define USE_EXTENDED_TAG 0x07
> -#define EXT_VIDEO_CAPABILITY_BLOCK 0x00
> +#define EXT_VIDEO_CAPABILITY_BLOCK 0x00
> +#define HDR_STATIC_METADATA_BLOCK0x06
> +#define USE_EXTENDED_TAG 0x07
> #define EXT_VIDEO_DATA_BLOCK_420 0x0E
> -#define EXT_VIDEO_CAP_BLOCK_Y420CMDB 0x0F
> +#define EXT_VIDEO_CAP_BLOCK_Y420CMDB 0x0F
> +#define
