[PATCH i-g-t v2 0/2] tool/intel_hdcp: Fix flickering and show actual HDCP status

[PATCH i-g-t v2 0/2] tool/intel_hdcp: Fix flickering and show actual HDCP status

[John Harrison]

Make it clear what the current HDCP status is rather than relying on
the user noticing a single frame flicker of the background colour.

CC: Santhosh Reddy Guddati <santhosh.reddy.guddati@intel.com>
CC: Suraj Kandpal <suraj.kandpal@intel.com>
Signed-off-by: John Harrison <John.Harrison@Igalia.com>
Acked-by: Kamil Konieczny <kamil.konieczny@linux.intel.com>

John Harrison (2):
  tool/intel_hdcp: Show actual HDCP status
  tool/intel_hdcp: Fix flickering when changing HDCP state

 tools/intel_hdcp.c | 49 +++++++++++++++++-----------------------------
 1 file changed, 18 insertions(+), 31 deletions(-)

-- 
2.43.0

[PATCH i-g-t v2 1/2] tool/intel_hdcp: Show actual HDCP status

[John Harrison]

The 'video player' showed what the requested HDCP mode was but not the
actual status. The menu system does redraw the screen with a special
colour to denote the status, but that is immediately overwritten by
the player thread. So all you get is a very brief full screen flicker
(to be fixed in next patch).

Instead, add the current HDCP status as an extra text line of the
video player's output so it is permanently visible and clear.

Also, move the enable of HDCP inside the mutex lock with the
internal state change. That way, the newly added 'enabled/desired'
line does not update in advance of the 'requested' line (due to the
video player thread being asynchronous and managing to turn HDCP on
while the menu thread is waiting on the mutex lock).

v2: Split into two patches (review feedback by Kamil)

CC: Santhosh Reddy Guddati <santhosh.reddy.guddati@intel.com>
CC: Suraj Kandpal <suraj.kandpal@intel.com>
Signed-off-by: John Harrison <John.Harrison@Igalia.com>
Acked-by: Kamil Konieczny <kamil.konieczny@linux.intel.com>
---
 tools/intel_hdcp.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/tools/intel_hdcp.c b/tools/intel_hdcp.c
index cd5d2b6be..8e29c5e32 100644
--- a/tools/intel_hdcp.c
+++ b/tools/intel_hdcp.c
@@ -482,6 +482,7 @@ static void *video_player_thread(void *arg)
 	char timer_str[32];
 	time_t current_time, elapsed;
 	int minutes, seconds;
+	const char *status = NULL;
 
 	data = (data_t *)arg;
 	cur_type = data->hdcp_type;
@@ -502,6 +503,8 @@ static void *video_player_thread(void *arg)
 			prev_type = cur_type;
 		}
 
+		status = get_hdcp_status(data, data->selected_connector);
+
 		switch (cur_type) {
 		case HDCP_TYPE_1_4:
 			bg_r = 0.0; bg_g = 0.7; bg_b = 0.0;
@@ -544,10 +547,18 @@ static void *video_player_thread(void *arg)
 		cairo_show_text(cr, timer_str);
 
 		/* Draw HDCP status above timer */
+		cairo_set_font_size(cr, font_size);
+		cairo_text_extents(cr, status, &ext);
+		x = data->width - ext.width - 20;
+		y = data->height - ext.height - 20;
+
+		cairo_move_to(cr, x, y);
+		cairo_show_text(cr, status);
+
 		cairo_set_font_size(cr, font_size);
 		cairo_text_extents(cr, hdcp_str, &ext);
 		x = data->width - ext.width - 20;
-		y = data->height - ext.height - 60;
+		y = data->height - ext.height - 120;
 
 		cairo_move_to(cr, x, y);
 		cairo_show_text(cr, hdcp_str);
@@ -605,8 +616,8 @@ static void stop_video_player(data_t *data)
 
 static void enable_hdcp_type(data_t *data, enum hdcp_type type)
 {
-		set_hdcp_prop(data, CP_DESIRED, type, data->selected_connector);
 		pthread_mutex_lock(&data->lock);
+		set_hdcp_prop(data, CP_DESIRED, type, data->selected_connector);
 		data->hdcp_type = type;
 		data->video_start_time = time(NULL);
 		pthread_mutex_unlock(&data->lock);
-- 
2.43.0

[PATCH i-g-t v2 2/2] tool/intel_hdcp: Fix flickering when changing HDCP state

[John Harrison]

The menu system would redraw the screen with a special colour to
denote the current HDCP status after a change was requested. However,
the screen was immediately redrawn by the player thread with a
background colour denoting the requested HDCP level. So all you
actually got was a very brief full screen flicker of the background
colour.

As the current HDCP status is now being shown as a separate line of
text in the video player's output, there is no need to use the screen
background to show the same. So, drop the menu updates once the video
player is running to remove the flickering.

Also, don't stop the video player thread when HDCP is disabled so that
the status update continues (given that the menu thread is no longer
redrawing the screen now).

v2: Split into two patches (review feedback by Kamil)

CC: Santhosh Reddy Guddati <santhosh.reddy.guddati@intel.com>
CC: Suraj Kandpal <suraj.kandpal@intel.com>
Signed-off-by: John Harrison <John.Harrison@Igalia.com>
Acked-by: Kamil Konieczny <kamil.konieczny@linux.intel.com>
---
 tools/intel_hdcp.c | 34 +++++-----------------------------
 1 file changed, 5 insertions(+), 29 deletions(-)

diff --git a/tools/intel_hdcp.c b/tools/intel_hdcp.c
index 8e29c5e32..56ed720cf 100644
--- a/tools/intel_hdcp.c
+++ b/tools/intel_hdcp.c
@@ -102,24 +102,13 @@ static void draw_menu_text(cairo_t *cr, data_t *data)
 	}
 }
 
-static void draw_menu(data_t *data, const char *status)
+static void draw_menu(data_t *data)
 {
 	cairo_t *cr = igt_get_cairo_ctx(data->fd, &data->fb);
 
-	/* Background color and text color based on HDCP status */
-	if (strcmp(status, "INIT") == 0) {
-		cairo_set_source_rgb(cr, 0.051, 0.278, 0.631);
-		cairo_paint(cr);
-		cairo_set_source_rgb(cr, 1.0, 1.0, 1.0);
-	} else if (strcmp(status, "Enabled") == 0) {
-		cairo_set_source_rgb(cr, 0.0, 1.0, 0.0);
-		cairo_paint(cr);
-		cairo_set_source_rgb(cr, 0.0, 0.0, 0.0);
-	} else {
-		cairo_set_source_rgb(cr, 1.0, 0.0, 0.0);
-		cairo_paint(cr);
-		cairo_set_source_rgb(cr, 1.0, 1.0, 1.0);
-	}
+	cairo_set_source_rgb(cr, 0.051, 0.278, 0.631);
+	cairo_paint(cr);
+	cairo_set_source_rgb(cr, 1.0, 1.0, 1.0);
 
 	draw_menu_text(cr, data);
 
@@ -678,13 +667,12 @@ int main(int argc, char **argv)
 {
 	data_t data;
 	pthread_t tid;
-	const char *status = NULL;
 	igt_plane_t *primary;
 
 	data.selected_connector = -1;
 	test_init(&data);
 
-	draw_menu(&data, "INIT");
+	draw_menu(&data);
 	primary = igt_output_get_plane_type(data.output, DRM_PLANE_TYPE_PRIMARY);
 	igt_plane_set_fb(primary, &data.fb);
 	igt_display_commit2(&data.display, COMMIT_ATOMIC);
@@ -718,7 +706,6 @@ int main(int argc, char **argv)
 			enable_hdcp_type(&data, HDCP_TYPE_2_2_TYPE_1);
 			break;
 		case 5:
-			stop_video_player(&data);
 			set_hdcp_prop(&data, CP_UNDESIRED, HDCP_TYPE_2_2_TYPE_1,
 				      data.selected_connector);
 			data.hdcp_type = HDCP_TYPE_NONE;
@@ -727,17 +714,6 @@ int main(int argc, char **argv)
 			break;
 		}
 
-		if (cmd == 1) {
-			draw_menu(&data, "INIT");
-		} else {
-			status = get_hdcp_status(&data, data.selected_connector);
-			draw_menu(&data, status);
-		}
-
-		primary = igt_output_get_plane_type(data.output, DRM_PLANE_TYPE_PRIMARY);
-		igt_plane_set_fb(primary, &data.fb);
-		igt_display_commit2(&data.display, COMMIT_ATOMIC);
-
 		usleep(200 * 1000);
 	}
 
-- 
2.43.0

RE: [PATCH i-g-t v2 1/2] tool/intel_hdcp: Show actual HDCP status

[Kandpal, Suraj]

> Subject: [PATCH i-g-t v2 1/2] tool/intel_hdcp: Show actual HDCP status
> 
> The 'video player' showed what the requested HDCP mode was but not the
> actual status. The menu system does redraw the screen with a special colour to
> denote the status, but that is immediately overwritten by the player thread. So
> all you get is a very brief full screen flicker (to be fixed in next patch).

So we do not want to refer to the next "patch" just mention what this patch is doing.
Also do not call it a "patch" since once it gets merged it becomes a commit.

> 
> Instead, add the current HDCP status as an extra text line of the video player's
> output so it is permanently visible and clear.
> 
> Also, move the enable of HDCP inside the mutex lock with the internal state
> change. That way, the newly added 'enabled/desired'
> line does not update in advance of the 'requested' line (due to the video player
> thread being asynchronous and managing to turn HDCP on while the menu
> thread is waiting on the mutex lock).

> 
> v2: Split into two patches (review feedback by Kamil)

You can just write (Kamil) here

> 
> CC: Santhosh Reddy Guddati <santhosh.reddy.guddati@intel.com>
> CC: Suraj Kandpal <suraj.kandpal@intel.com>
> Signed-off-by: John Harrison <John.Harrison@Igalia.com>
> Acked-by: Kamil Konieczny <kamil.konieczny@linux.intel.com>
> ---
>  tools/intel_hdcp.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/intel_hdcp.c b/tools/intel_hdcp.c index cd5d2b6be..8e29c5e32
> 100644
> --- a/tools/intel_hdcp.c
> +++ b/tools/intel_hdcp.c
> @@ -482,6 +482,7 @@ static void *video_player_thread(void *arg)
>  	char timer_str[32];
>  	time_t current_time, elapsed;
>  	int minutes, seconds;
> +	const char *status = NULL;
> 
>  	data = (data_t *)arg;
>  	cur_type = data->hdcp_type;
> @@ -502,6 +503,8 @@ static void *video_player_thread(void *arg)
>  			prev_type = cur_type;
>  		}
> 
> +		status = get_hdcp_status(data, data->selected_connector);
> +
>  		switch (cur_type) {
>  		case HDCP_TYPE_1_4:
>  			bg_r = 0.0; bg_g = 0.7; bg_b = 0.0;
> @@ -544,10 +547,18 @@ static void *video_player_thread(void *arg)
>  		cairo_show_text(cr, timer_str);
> 
>  		/* Draw HDCP status above timer */
> +		cairo_set_font_size(cr, font_size);
> +		cairo_text_extents(cr, status, &ext);
> +		x = data->width - ext.width - 20;
> +		y = data->height - ext.height - 20;
> +
> +		cairo_move_to(cr, x, y);
> +		cairo_show_text(cr, status);
> +
>  		cairo_set_font_size(cr, font_size);
>  		cairo_text_extents(cr, hdcp_str, &ext);
>  		x = data->width - ext.width - 20;
> -		y = data->height - ext.height - 60;
> +		y = data->height - ext.height - 120;
> 
>  		cairo_move_to(cr, x, y);
>  		cairo_show_text(cr, hdcp_str);
> @@ -605,8 +616,8 @@ static void stop_video_player(data_t *data)
> 
>  static void enable_hdcp_type(data_t *data, enum hdcp_type type)  {
> -		set_hdcp_prop(data, CP_DESIRED, type, data-
> >selected_connector);
>  		pthread_mutex_lock(&data->lock);
> +		set_hdcp_prop(data, CP_DESIRED, type, data-
> >selected_connector);

Move this change into its own patch logically a different change 

Regards,
Suraj Kandpal

>  		data->hdcp_type = type;
>  		data->video_start_time = time(NULL);
>  		pthread_mutex_unlock(&data->lock);
> --
> 2.43.0

RE: [PATCH i-g-t v2 2/2] tool/intel_hdcp: Fix flickering when changing HDCP state

[Kandpal, Suraj]

> Subject: [PATCH i-g-t v2 2/2] tool/intel_hdcp: Fix flickering when changing
> HDCP state
> 
> The menu system would redraw the screen with a special colour to denote the

* color

> current HDCP status after a change was requested. However, the screen was
> immediately redrawn by the player thread with a background colour denoting

*color

> the requested HDCP level. So all you actually got was a very brief full screen
> flicker of the background colour.

*color

You can keep this description here and remove it from the previous patch

> 
> As the current HDCP status is now being shown as a separate line of text in
> the video player's output, there is no need to use the screen background to
> show the same. So, drop the menu updates once the video player is running to
> remove the flickering.
> 
> Also, don't stop the video player thread when HDCP is disabled so that the
> status update continues (given that the menu thread is no longer redrawing
> the screen now).
> 
> v2: Split into two patches (review feedback by Kamil)
> 
> CC: Santhosh Reddy Guddati <santhosh.reddy.guddati@intel.com>
> CC: Suraj Kandpal <suraj.kandpal@intel.com>
> Signed-off-by: John Harrison <John.Harrison@Igalia.com>
> Acked-by: Kamil Konieczny <kamil.konieczny@linux.intel.com>
> ---
>  tools/intel_hdcp.c | 34 +++++-----------------------------
>  1 file changed, 5 insertions(+), 29 deletions(-)
> 
> diff --git a/tools/intel_hdcp.c b/tools/intel_hdcp.c index
> 8e29c5e32..56ed720cf 100644
> --- a/tools/intel_hdcp.c
> +++ b/tools/intel_hdcp.c
> @@ -102,24 +102,13 @@ static void draw_menu_text(cairo_t *cr, data_t
> *data)
>  	}
>  }
> 
> -static void draw_menu(data_t *data, const char *status)
> +static void draw_menu(data_t *data)
>  {
>  	cairo_t *cr = igt_get_cairo_ctx(data->fd, &data->fb);
> 
> -	/* Background color and text color based on HDCP status */
> -	if (strcmp(status, "INIT") == 0) {
> -		cairo_set_source_rgb(cr, 0.051, 0.278, 0.631);
> -		cairo_paint(cr);
> -		cairo_set_source_rgb(cr, 1.0, 1.0, 1.0);
> -	} else if (strcmp(status, "Enabled") == 0) {
> -		cairo_set_source_rgb(cr, 0.0, 1.0, 0.0);
> -		cairo_paint(cr);
> -		cairo_set_source_rgb(cr, 0.0, 0.0, 0.0);
> -	} else {
> -		cairo_set_source_rgb(cr, 1.0, 0.0, 0.0);
> -		cairo_paint(cr);
> -		cairo_set_source_rgb(cr, 1.0, 1.0, 1.0);
> -	}
> +	cairo_set_source_rgb(cr, 0.051, 0.278, 0.631);
> +	cairo_paint(cr);
> +	cairo_set_source_rgb(cr, 1.0, 1.0, 1.0);
> 
>  	draw_menu_text(cr, data);
> 
> @@ -678,13 +667,12 @@ int main(int argc, char **argv)  {
>  	data_t data;
>  	pthread_t tid;
> -	const char *status = NULL;
>  	igt_plane_t *primary;
> 
>  	data.selected_connector = -1;
>  	test_init(&data);
> 
> -	draw_menu(&data, "INIT");
> +	draw_menu(&data);
>  	primary = igt_output_get_plane_type(data.output,
> DRM_PLANE_TYPE_PRIMARY);
>  	igt_plane_set_fb(primary, &data.fb);
>  	igt_display_commit2(&data.display, COMMIT_ATOMIC); @@ -718,7
> +706,6 @@ int main(int argc, char **argv)
>  			enable_hdcp_type(&data, HDCP_TYPE_2_2_TYPE_1);
>  			break;
>  		case 5:
> -			stop_video_player(&data);

This removal to stop video player should be in it's own patch

As a rule of thumb if you have to add Also in the commit message keep it in a separate patch 

Regards,
Suraj Kandpal

>  			set_hdcp_prop(&data, CP_UNDESIRED,
> HDCP_TYPE_2_2_TYPE_1,
>  				      data.selected_connector);
>  			data.hdcp_type = HDCP_TYPE_NONE;
> @@ -727,17 +714,6 @@ int main(int argc, char **argv)
>  			break;
>  		}
> 
> -		if (cmd == 1) {
> -			draw_menu(&data, "INIT");
> -		} else {
> -			status = get_hdcp_status(&data,
> data.selected_connector);
> -			draw_menu(&data, status);
> -		}
> -
> -		primary = igt_output_get_plane_type(data.output,
> DRM_PLANE_TYPE_PRIMARY);
> -		igt_plane_set_fb(primary, &data.fb);
> -		igt_display_commit2(&data.display, COMMIT_ATOMIC);
> -
>  		usleep(200 * 1000);
>  	}
> 
> --
> 2.43.0

Re: [PATCH i-g-t v2 1/2] tool/intel_hdcp: Show actual HDCP status

[John Harrison]

On 2/16/26 19:00, Kandpal, Suraj wrote:
>> Subject: [PATCH i-g-t v2 1/2] tool/intel_hdcp: Show actual HDCP status
>>
>> The 'video player' showed what the requested HDCP mode was but not the
>> actual status. The menu system does redraw the screen with a special colour to
>> denote the status, but that is immediately overwritten by the player thread. So
>> all you get is a very brief full screen flicker (to be fixed in next patch).
> So we do not want to refer to the next "patch" just mention what this patch is doing.
> Also do not call it a "patch" since once it gets merged it becomes a commit.
But this patch is part of a series and a series of tightly related 
patches has always explained what is happening as a whole. Otherwise, 
you effectively have this patch/commit saying "this is totally broken 
and I'm not doing anything about it".

>
>> Instead, add the current HDCP status as an extra text line of the video player's
>> output so it is permanently visible and clear.
>>
>> Also, move the enable of HDCP inside the mutex lock with the internal state
>> change. That way, the newly added 'enabled/desired'
>> line does not update in advance of the 'requested' line (due to the video player
>> thread being asynchronous and managing to turn HDCP on while the menu
>> thread is waiting on the mutex lock).
>> v2: Split into two patches (review feedback by Kamil)
> You can just write (Kamil) here
>
>> CC: Santhosh Reddy Guddati <santhosh.reddy.guddati@intel.com>
>> CC: Suraj Kandpal <suraj.kandpal@intel.com>
>> Signed-off-by: John Harrison <John.Harrison@Igalia.com>
>> Acked-by: Kamil Konieczny <kamil.konieczny@linux.intel.com>
>> ---
>>   tools/intel_hdcp.c | 15 +++++++++++++--
>>   1 file changed, 13 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/intel_hdcp.c b/tools/intel_hdcp.c index cd5d2b6be..8e29c5e32
>> 100644
>> --- a/tools/intel_hdcp.c
>> +++ b/tools/intel_hdcp.c
>> @@ -482,6 +482,7 @@ static void *video_player_thread(void *arg)
>>   	char timer_str[32];
>>   	time_t current_time, elapsed;
>>   	int minutes, seconds;
>> +	const char *status = NULL;
>>
>>   	data = (data_t *)arg;
>>   	cur_type = data->hdcp_type;
>> @@ -502,6 +503,8 @@ static void *video_player_thread(void *arg)
>>   			prev_type = cur_type;
>>   		}
>>
>> +		status = get_hdcp_status(data, data->selected_connector);
>> +
>>   		switch (cur_type) {
>>   		case HDCP_TYPE_1_4:
>>   			bg_r = 0.0; bg_g = 0.7; bg_b = 0.0;
>> @@ -544,10 +547,18 @@ static void *video_player_thread(void *arg)
>>   		cairo_show_text(cr, timer_str);
>>
>>   		/* Draw HDCP status above timer */
>> +		cairo_set_font_size(cr, font_size);
>> +		cairo_text_extents(cr, status, &ext);
>> +		x = data->width - ext.width - 20;
>> +		y = data->height - ext.height - 20;
>> +
>> +		cairo_move_to(cr, x, y);
>> +		cairo_show_text(cr, status);
>> +
>>   		cairo_set_font_size(cr, font_size);
>>   		cairo_text_extents(cr, hdcp_str, &ext);
>>   		x = data->width - ext.width - 20;
>> -		y = data->height - ext.height - 60;
>> +		y = data->height - ext.height - 120;
>>
>>   		cairo_move_to(cr, x, y);
>>   		cairo_show_text(cr, hdcp_str);
>> @@ -605,8 +616,8 @@ static void stop_video_player(data_t *data)
>>
>>   static void enable_hdcp_type(data_t *data, enum hdcp_type type)  {
>> -		set_hdcp_prop(data, CP_DESIRED, type, data-
>>> selected_connector);
>>   		pthread_mutex_lock(&data->lock);
>> +		set_hdcp_prop(data, CP_DESIRED, type, data-
>>> selected_connector);
> Move this change into its own patch logically a different change
But it is not a logically different change. As per the commit 
description, the only reason for making this change is because the video 
thread is now displaying the HDCP status. Without the display of the 
status, it does not matter that the change occurs asynchronously to that 
display (because there is nothing for it to be asynchronous to). So 
splitting this out into a later patch means this patch would be 
introducing a bug and the follow up patch would need a 'Fixes' tag. 
Which is just pointless for the sake of a one line delta. Or it could be 
a prior patch but then it would need to say 'doing this thing that is 
pointless now but is required by a future patch' and you have already 
said you don't like descriptions referring to future changes.

John.

>
> Regards,
> Suraj Kandpal
>
>>   		data->hdcp_type = type;
>>   		data->video_start_time = time(NULL);
>>   		pthread_mutex_unlock(&data->lock);
>> --
>> 2.43.0

Re: [PATCH i-g-t v2 2/2] tool/intel_hdcp: Fix flickering when changing HDCP state

[John Harrison]

On 2/16/26 19:07, Kandpal, Suraj wrote:
>> Subject: [PATCH i-g-t v2 2/2] tool/intel_hdcp: Fix flickering when changing
>> HDCP state
>>
>> The menu system would redraw the screen with a special colour to denote the
> * color
>
>> current HDCP status after a change was requested. However, the screen was
>> immediately redrawn by the player thread with a background colour denoting
> *color
>
>> the requested HDCP level. So all you actually got was a very brief full screen
>> flicker of the background colour.
> *color
>
> You can keep this description here and remove it from the previous patch
But this issue is the entire point of the previous patch. If you remove 
the description as to why a change is being made then the review 
feedback becomes 'why is this change being made?' and 'there is already 
a display of the status - it's in the background colour'.

>
>> As the current HDCP status is now being shown as a separate line of text in
>> the video player's output, there is no need to use the screen background to
>> show the same. So, drop the menu updates once the video player is running to
>> remove the flickering.
>>
>> Also, don't stop the video player thread when HDCP is disabled so that the
>> status update continues (given that the menu thread is no longer redrawing
>> the screen now).
>>
>> v2: Split into two patches (review feedback by Kamil)
>>
>> CC: Santhosh Reddy Guddati <santhosh.reddy.guddati@intel.com>
>> CC: Suraj Kandpal <suraj.kandpal@intel.com>
>> Signed-off-by: John Harrison <John.Harrison@Igalia.com>
>> Acked-by: Kamil Konieczny <kamil.konieczny@linux.intel.com>
>> ---
>>   tools/intel_hdcp.c | 34 +++++-----------------------------
>>   1 file changed, 5 insertions(+), 29 deletions(-)
>>
>> diff --git a/tools/intel_hdcp.c b/tools/intel_hdcp.c index
>> 8e29c5e32..56ed720cf 100644
>> --- a/tools/intel_hdcp.c
>> +++ b/tools/intel_hdcp.c
>> @@ -102,24 +102,13 @@ static void draw_menu_text(cairo_t *cr, data_t
>> *data)
>>   	}
>>   }
>>
>> -static void draw_menu(data_t *data, const char *status)
>> +static void draw_menu(data_t *data)
>>   {
>>   	cairo_t *cr = igt_get_cairo_ctx(data->fd, &data->fb);
>>
>> -	/* Background color and text color based on HDCP status */
>> -	if (strcmp(status, "INIT") == 0) {
>> -		cairo_set_source_rgb(cr, 0.051, 0.278, 0.631);
>> -		cairo_paint(cr);
>> -		cairo_set_source_rgb(cr, 1.0, 1.0, 1.0);
>> -	} else if (strcmp(status, "Enabled") == 0) {
>> -		cairo_set_source_rgb(cr, 0.0, 1.0, 0.0);
>> -		cairo_paint(cr);
>> -		cairo_set_source_rgb(cr, 0.0, 0.0, 0.0);
>> -	} else {
>> -		cairo_set_source_rgb(cr, 1.0, 0.0, 0.0);
>> -		cairo_paint(cr);
>> -		cairo_set_source_rgb(cr, 1.0, 1.0, 1.0);
>> -	}
>> +	cairo_set_source_rgb(cr, 0.051, 0.278, 0.631);
>> +	cairo_paint(cr);
>> +	cairo_set_source_rgb(cr, 1.0, 1.0, 1.0);
>>
>>   	draw_menu_text(cr, data);
>>
>> @@ -678,13 +667,12 @@ int main(int argc, char **argv)  {
>>   	data_t data;
>>   	pthread_t tid;
>> -	const char *status = NULL;
>>   	igt_plane_t *primary;
>>
>>   	data.selected_connector = -1;
>>   	test_init(&data);
>>
>> -	draw_menu(&data, "INIT");
>> +	draw_menu(&data);
>>   	primary = igt_output_get_plane_type(data.output,
>> DRM_PLANE_TYPE_PRIMARY);
>>   	igt_plane_set_fb(primary, &data.fb);
>>   	igt_display_commit2(&data.display, COMMIT_ATOMIC); @@ -718,7
>> +706,6 @@ int main(int argc, char **argv)
>>   			enable_hdcp_type(&data, HDCP_TYPE_2_2_TYPE_1);
>>   			break;
>>   		case 5:
>> -			stop_video_player(&data);
> This removal to stop video player should be in it's own patch
>
> As a rule of thumb if you have to add Also in the commit message keep it in a separate patch
Again, the removal of the stop is because of the remove of the menu 
redraw. If you don't do both together then you are introducing a bug 
where disabling HDCP does not result in a display update so the user is 
not aware that anything actually happened. On the kernel side, there is 
an absolute rule that an intermediate patch in a series must not break 
things and require a later patch to fix it again. You can split patches 
up to help with review but they need to be squashed back into an atomic 
change for merge. I would assume the same applies to the IGT tree. And a 
one line change does not seem like it needs the extra work and 
complication of splitting out and squashing back just to make the review 
simpler.

John.

>
> Regards,
> Suraj Kandpal
>
>>   			set_hdcp_prop(&data, CP_UNDESIRED,
>> HDCP_TYPE_2_2_TYPE_1,
>>   				      data.selected_connector);
>>   			data.hdcp_type = HDCP_TYPE_NONE;
>> @@ -727,17 +714,6 @@ int main(int argc, char **argv)
>>   			break;
>>   		}
>>
>> -		if (cmd == 1) {
>> -			draw_menu(&data, "INIT");
>> -		} else {
>> -			status = get_hdcp_status(&data,
>> data.selected_connector);
>> -			draw_menu(&data, status);
>> -		}
>> -
>> -		primary = igt_output_get_plane_type(data.output,
>> DRM_PLANE_TYPE_PRIMARY);
>> -		igt_plane_set_fb(primary, &data.fb);
>> -		igt_display_commit2(&data.display, COMMIT_ATOMIC);
>> -
>>   		usleep(200 * 1000);
>>   	}
>>
>> --
>> 2.43.0

Re: [PATCH i-g-t v2 1/2] tool/intel_hdcp: Show actual HDCP status

[Kamil Konieczny]

Hi John,
On 2026-02-17 at 08:58:01 -0800, John Harrison wrote:
> On 2/16/26 19:00, Kandpal, Suraj wrote:
> > > Subject: [PATCH i-g-t v2 1/2] tool/intel_hdcp: Show actual HDCP status
> > > 
> > > The 'video player' showed what the requested HDCP mode was but not the
> > > actual status. The menu system does redraw the screen with a special colour to
> > > denote the status, but that is immediately overwritten by the player thread. So
> > > all you get is a very brief full screen flicker (to be fixed in next patch).
> > So we do not want to refer to the next "patch" just mention what this patch is doing.
> > Also do not call it a "patch" since once it gets merged it becomes a commit.
> But this patch is part of a series and a series of tightly related patches
> has always explained what is happening as a whole. Otherwise, you
> effectively have this patch/commit saying "this is totally broken and I'm
> not doing anything about it".
> 
> > 
> > > Instead, add the current HDCP status as an extra text line of the video player's
> > > output so it is permanently visible and clear.
> > > 
> > > Also, move the enable of HDCP inside the mutex lock with the internal state
> > > change. That way, the newly added 'enabled/desired'
> > > line does not update in advance of the 'requested' line (due to the video player
> > > thread being asynchronous and managing to turn HDCP on while the menu
> > > thread is waiting on the mutex lock).
> > > v2: Split into two patches (review feedback by Kamil)
> > You can just write (Kamil) here
> > 
> > > CC: Santhosh Reddy Guddati <santhosh.reddy.guddati@intel.com>
> > > CC: Suraj Kandpal <suraj.kandpal@intel.com>
> > > Signed-off-by: John Harrison <John.Harrison@Igalia.com>
> > > Acked-by: Kamil Konieczny <kamil.konieczny@linux.intel.com>
> > > ---
> > >   tools/intel_hdcp.c | 15 +++++++++++++--
> > >   1 file changed, 13 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/tools/intel_hdcp.c b/tools/intel_hdcp.c index cd5d2b6be..8e29c5e32
> > > 100644
> > > --- a/tools/intel_hdcp.c
> > > +++ b/tools/intel_hdcp.c
> > > @@ -482,6 +482,7 @@ static void *video_player_thread(void *arg)
> > >   	char timer_str[32];
> > >   	time_t current_time, elapsed;
> > >   	int minutes, seconds;
> > > +	const char *status = NULL;
> > > 
> > >   	data = (data_t *)arg;
> > >   	cur_type = data->hdcp_type;
> > > @@ -502,6 +503,8 @@ static void *video_player_thread(void *arg)
> > >   			prev_type = cur_type;
> > >   		}
> > > 
> > > +		status = get_hdcp_status(data, data->selected_connector);
> > > +
> > >   		switch (cur_type) {
> > >   		case HDCP_TYPE_1_4:
> > >   			bg_r = 0.0; bg_g = 0.7; bg_b = 0.0;
> > > @@ -544,10 +547,18 @@ static void *video_player_thread(void *arg)
> > >   		cairo_show_text(cr, timer_str);
> > > 
> > >   		/* Draw HDCP status above timer */
> > > +		cairo_set_font_size(cr, font_size);
> > > +		cairo_text_extents(cr, status, &ext);
> > > +		x = data->width - ext.width - 20;
> > > +		y = data->height - ext.height - 20;
> > > +
> > > +		cairo_move_to(cr, x, y);
> > > +		cairo_show_text(cr, status);
> > > +
> > >   		cairo_set_font_size(cr, font_size);
> > >   		cairo_text_extents(cr, hdcp_str, &ext);
> > >   		x = data->width - ext.width - 20;
> > > -		y = data->height - ext.height - 60;
> > > +		y = data->height - ext.height - 120;
> > > 
> > >   		cairo_move_to(cr, x, y);
> > >   		cairo_show_text(cr, hdcp_str);
> > > @@ -605,8 +616,8 @@ static void stop_video_player(data_t *data)
> > > 
> > >   static void enable_hdcp_type(data_t *data, enum hdcp_type type)  {
> > > -		set_hdcp_prop(data, CP_DESIRED, type, data-
> > > > selected_connector);
> > >   		pthread_mutex_lock(&data->lock);
> > > +		set_hdcp_prop(data, CP_DESIRED, type, data-
> > > > selected_connector);
> > Move this change into its own patch logically a different change
> But it is not a logically different change. As per the commit description,
> the only reason for making this change is because the video thread is now
> displaying the HDCP status. Without the display of the status, it does not
> matter that the change occurs asynchronously to that display (because there
> is nothing for it to be asynchronous to). So splitting this out into a later
> patch means this patch would be introducing a bug and the follow up patch

Would it help if this change will be the first one in a series?

If yes, then it should go first as a fix. If not, if this one
change do not help in anything then imho it should stay in this
one bigger change.

Regards,
Kamil

> would need a 'Fixes' tag. Which is just pointless for the sake of a one line
> delta. Or it could be a prior patch but then it would need to say 'doing
> this thing that is pointless now but is required by a future patch' and you
> have already said you don't like descriptions referring to future changes.
> 
> John.
> 
> > 
> > Regards,
> > Suraj Kandpal
> > 
> > >   		data->hdcp_type = type;
> > >   		data->video_start_time = time(NULL);
> > >   		pthread_mutex_unlock(&data->lock);
> > > --
> > > 2.43.0
> 

Re: [PATCH i-g-t v2 1/2] tool/intel_hdcp: Show actual HDCP status

[John Harrison]

On 2/24/26 11:30, Kamil Konieczny wrote:
> Hi John,
> On 2026-02-17 at 08:58:01 -0800, John Harrison wrote:
>> On 2/16/26 19:00, Kandpal, Suraj wrote:
>>>> Subject: [PATCH i-g-t v2 1/2] tool/intel_hdcp: Show actual HDCP status
>>>>
>>>> The 'video player' showed what the requested HDCP mode was but not the
>>>> actual status. The menu system does redraw the screen with a special colour to
>>>> denote the status, but that is immediately overwritten by the player thread. So
>>>> all you get is a very brief full screen flicker (to be fixed in next patch).
>>> So we do not want to refer to the next "patch" just mention what this patch is doing.
>>> Also do not call it a "patch" since once it gets merged it becomes a commit.
>> But this patch is part of a series and a series of tightly related patches
>> has always explained what is happening as a whole. Otherwise, you
>> effectively have this patch/commit saying "this is totally broken and I'm
>> not doing anything about it".
>>
>>>> Instead, add the current HDCP status as an extra text line of the video player's
>>>> output so it is permanently visible and clear.
>>>>
>>>> Also, move the enable of HDCP inside the mutex lock with the internal state
>>>> change. That way, the newly added 'enabled/desired'
>>>> line does not update in advance of the 'requested' line (due to the video player
>>>> thread being asynchronous and managing to turn HDCP on while the menu
>>>> thread is waiting on the mutex lock).
>>>> v2: Split into two patches (review feedback by Kamil)
>>> You can just write (Kamil) here
>>>
>>>> CC: Santhosh Reddy Guddati <santhosh.reddy.guddati@intel.com>
>>>> CC: Suraj Kandpal <suraj.kandpal@intel.com>
>>>> Signed-off-by: John Harrison <John.Harrison@Igalia.com>
>>>> Acked-by: Kamil Konieczny <kamil.konieczny@linux.intel.com>
>>>> ---
>>>>    tools/intel_hdcp.c | 15 +++++++++++++--
>>>>    1 file changed, 13 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/tools/intel_hdcp.c b/tools/intel_hdcp.c index cd5d2b6be..8e29c5e32
>>>> 100644
>>>> --- a/tools/intel_hdcp.c
>>>> +++ b/tools/intel_hdcp.c
>>>> @@ -482,6 +482,7 @@ static void *video_player_thread(void *arg)
>>>>    	char timer_str[32];
>>>>    	time_t current_time, elapsed;
>>>>    	int minutes, seconds;
>>>> +	const char *status = NULL;
>>>>
>>>>    	data = (data_t *)arg;
>>>>    	cur_type = data->hdcp_type;
>>>> @@ -502,6 +503,8 @@ static void *video_player_thread(void *arg)
>>>>    			prev_type = cur_type;
>>>>    		}
>>>>
>>>> +		status = get_hdcp_status(data, data->selected_connector);
>>>> +
>>>>    		switch (cur_type) {
>>>>    		case HDCP_TYPE_1_4:
>>>>    			bg_r = 0.0; bg_g = 0.7; bg_b = 0.0;
>>>> @@ -544,10 +547,18 @@ static void *video_player_thread(void *arg)
>>>>    		cairo_show_text(cr, timer_str);
>>>>
>>>>    		/* Draw HDCP status above timer */
>>>> +		cairo_set_font_size(cr, font_size);
>>>> +		cairo_text_extents(cr, status, &ext);
>>>> +		x = data->width - ext.width - 20;
>>>> +		y = data->height - ext.height - 20;
>>>> +
>>>> +		cairo_move_to(cr, x, y);
>>>> +		cairo_show_text(cr, status);
>>>> +
>>>>    		cairo_set_font_size(cr, font_size);
>>>>    		cairo_text_extents(cr, hdcp_str, &ext);
>>>>    		x = data->width - ext.width - 20;
>>>> -		y = data->height - ext.height - 60;
>>>> +		y = data->height - ext.height - 120;
>>>>
>>>>    		cairo_move_to(cr, x, y);
>>>>    		cairo_show_text(cr, hdcp_str);
>>>> @@ -605,8 +616,8 @@ static void stop_video_player(data_t *data)
>>>>
>>>>    static void enable_hdcp_type(data_t *data, enum hdcp_type type)  {
>>>> -		set_hdcp_prop(data, CP_DESIRED, type, data-
>>>>> selected_connector);
>>>>    		pthread_mutex_lock(&data->lock);
>>>> +		set_hdcp_prop(data, CP_DESIRED, type, data-
>>>>> selected_connector);
>>> Move this change into its own patch logically a different change
>> But it is not a logically different change. As per the commit description,
>> the only reason for making this change is because the video thread is now
>> displaying the HDCP status. Without the display of the status, it does not
>> matter that the change occurs asynchronously to that display (because there
>> is nothing for it to be asynchronous to). So splitting this out into a later
>> patch means this patch would be introducing a bug and the follow up patch
> Would it help if this change will be the first one in a series?
>
> If yes, then it should go first as a fix. If not, if this one
> change do not help in anything then imho it should stay in this
> one bigger change.
That's the thing, there is no reason for the set_hdcp_prop call to be 
inside the mutex lock without adding the display of the HDCP status to 
the video thread. Hence the reason it was previously not inside the lock.

The lock is protecting the display thread. If the display thread does 
not show HDCP status then changing the status asynchronously to that 
thread is totally fine. But with the changes in this patch to add the 
status to the display thread, having an asynchronous update does cause 
'flickering' of the display - the 'active' line changes before the 
'requested' line does.

You could move the re-order into it's own private patch. But given that 
it is always best to do the least amount of work inside a lock, that 
patch would have to have a description of 'moving this because of 
changes coming in the next commit'. Which is not ideal. Hence I think it 
is better kept together with the rest of this patch.

Thanks,
John.

>
> Regards,
> Kamil
>
>> would need a 'Fixes' tag. Which is just pointless for the sake of a one line
>> delta. Or it could be a prior patch but then it would need to say 'doing
>> this thing that is pointless now but is required by a future patch' and you
>> have already said you don't like descriptions referring to future changes.
>>
>> John.
>>
>>> Regards,
>>> Suraj Kandpal
>>>
>>>>    		data->hdcp_type = type;
>>>>    		data->video_start_time = time(NULL);
>>>>    		pthread_mutex_unlock(&data->lock);
>>>> --
>>>> 2.43.0

Re: [PATCH i-g-t v2 1/2] tool/intel_hdcp: Show actual HDCP status

[Kamil Konieczny]

Hi John,
On 2026-02-24 at 14:31:51 -0800, John Harrison wrote:
> On 2/24/26 11:30, Kamil Konieczny wrote:
> > Hi John,
> > On 2026-02-17 at 08:58:01 -0800, John Harrison wrote:
> > > On 2/16/26 19:00, Kandpal, Suraj wrote:
> > > > > Subject: [PATCH i-g-t v2 1/2] tool/intel_hdcp: Show actual HDCP status
> > > > > 
> > > > > The 'video player' showed what the requested HDCP mode was but not the
> > > > > actual status. The menu system does redraw the screen with a special colour to
> > > > > denote the status, but that is immediately overwritten by the player thread. So
> > > > > all you get is a very brief full screen flicker (to be fixed in next patch).
> > > > So we do not want to refer to the next "patch" just mention what this patch is doing.
> > > > Also do not call it a "patch" since once it gets merged it becomes a commit.
> > > But this patch is part of a series and a series of tightly related patches
> > > has always explained what is happening as a whole. Otherwise, you
> > > effectively have this patch/commit saying "this is totally broken and I'm
> > > not doing anything about it".
> > > 
> > > > > Instead, add the current HDCP status as an extra text line of the video player's
> > > > > output so it is permanently visible and clear.
> > > > > 
> > > > > Also, move the enable of HDCP inside the mutex lock with the internal state
> > > > > change. That way, the newly added 'enabled/desired'
> > > > > line does not update in advance of the 'requested' line (due to the video player
> > > > > thread being asynchronous and managing to turn HDCP on while the menu
> > > > > thread is waiting on the mutex lock).
> > > > > v2: Split into two patches (review feedback by Kamil)
> > > > You can just write (Kamil) here
> > > > 
> > > > > CC: Santhosh Reddy Guddati <santhosh.reddy.guddati@intel.com>
> > > > > CC: Suraj Kandpal <suraj.kandpal@intel.com>
> > > > > Signed-off-by: John Harrison <John.Harrison@Igalia.com>
> > > > > Acked-by: Kamil Konieczny <kamil.konieczny@linux.intel.com>
> > > > > ---
> > > > >    tools/intel_hdcp.c | 15 +++++++++++++--
> > > > >    1 file changed, 13 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/tools/intel_hdcp.c b/tools/intel_hdcp.c index cd5d2b6be..8e29c5e32
> > > > > 100644
> > > > > --- a/tools/intel_hdcp.c
> > > > > +++ b/tools/intel_hdcp.c
> > > > > @@ -482,6 +482,7 @@ static void *video_player_thread(void *arg)
> > > > >    	char timer_str[32];
> > > > >    	time_t current_time, elapsed;
> > > > >    	int minutes, seconds;
> > > > > +	const char *status = NULL;
> > > > > 
> > > > >    	data = (data_t *)arg;
> > > > >    	cur_type = data->hdcp_type;
> > > > > @@ -502,6 +503,8 @@ static void *video_player_thread(void *arg)
> > > > >    			prev_type = cur_type;
> > > > >    		}
> > > > > 
> > > > > +		status = get_hdcp_status(data, data->selected_connector);
> > > > > +
> > > > >    		switch (cur_type) {
> > > > >    		case HDCP_TYPE_1_4:
> > > > >    			bg_r = 0.0; bg_g = 0.7; bg_b = 0.0;
> > > > > @@ -544,10 +547,18 @@ static void *video_player_thread(void *arg)
> > > > >    		cairo_show_text(cr, timer_str);
> > > > > 
> > > > >    		/* Draw HDCP status above timer */
> > > > > +		cairo_set_font_size(cr, font_size);
> > > > > +		cairo_text_extents(cr, status, &ext);
> > > > > +		x = data->width - ext.width - 20;
> > > > > +		y = data->height - ext.height - 20;
> > > > > +
> > > > > +		cairo_move_to(cr, x, y);
> > > > > +		cairo_show_text(cr, status);
> > > > > +
> > > > >    		cairo_set_font_size(cr, font_size);
> > > > >    		cairo_text_extents(cr, hdcp_str, &ext);
> > > > >    		x = data->width - ext.width - 20;
> > > > > -		y = data->height - ext.height - 60;
> > > > > +		y = data->height - ext.height - 120;
> > > > > 
> > > > >    		cairo_move_to(cr, x, y);
> > > > >    		cairo_show_text(cr, hdcp_str);
> > > > > @@ -605,8 +616,8 @@ static void stop_video_player(data_t *data)
> > > > > 
> > > > >    static void enable_hdcp_type(data_t *data, enum hdcp_type type)  {
> > > > > -		set_hdcp_prop(data, CP_DESIRED, type, data-
> > > > > > selected_connector);
> > > > >    		pthread_mutex_lock(&data->lock);
> > > > > +		set_hdcp_prop(data, CP_DESIRED, type, data-
> > > > > > selected_connector);
> > > > Move this change into its own patch logically a different change
> > > But it is not a logically different change. As per the commit description,
> > > the only reason for making this change is because the video thread is now
> > > displaying the HDCP status. Without the display of the status, it does not
> > > matter that the change occurs asynchronously to that display (because there
> > > is nothing for it to be asynchronous to). So splitting this out into a later
> > > patch means this patch would be introducing a bug and the follow up patch
> > Would it help if this change will be the first one in a series?
> > 
> > If yes, then it should go first as a fix. If not, if this one
> > change do not help in anything then imho it should stay in this
> > one bigger change.
> That's the thing, there is no reason for the set_hdcp_prop call to be inside
> the mutex lock without adding the display of the HDCP status to the video
> thread. Hence the reason it was previously not inside the lock.
> 
> The lock is protecting the display thread. If the display thread does not
> show HDCP status then changing the status asynchronously to that thread is
> totally fine. But with the changes in this patch to add the status to the
> display thread, having an asynchronous update does cause 'flickering' of the
> display - the 'active' line changes before the 'requested' line does.
> 
> You could move the re-order into it's own private patch. But given that it
> is always best to do the least amount of work inside a lock, that patch
> would have to have a description of 'moving this because of changes coming
> in the next commit'. Which is not ideal. Hence I think it is better kept
> together with the rest of this patch.
> 
> Thanks,
> John.

Sound reasonable. Suraj could you please ack or review this series?
Does it improve this tool and works as intended?

Thanks in advance!

Regards,
Kamil

> 
> > 
> > Regards,
> > Kamil
> > 
> > > would need a 'Fixes' tag. Which is just pointless for the sake of a one line
> > > delta. Or it could be a prior patch but then it would need to say 'doing
> > > this thing that is pointless now but is required by a future patch' and you
> > > have already said you don't like descriptions referring to future changes.
> > > 
> > > John.
> > > 
> > > > Regards,
> > > > Suraj Kandpal
> > > > 
> > > > >    		data->hdcp_type = type;
> > > > >    		data->video_start_time = time(NULL);
> > > > >    		pthread_mutex_unlock(&data->lock);
> > > > > --
> > > > > 2.43.0
> 

Re: [PATCH i-g-t v2 1/2] tool/intel_hdcp: Show actual HDCP status

[John Harrison]

@Suraj, ping?

On 3/16/26 12:06, Kamil Konieczny wrote:
> Hi John,
> On 2026-02-24 at 14:31:51 -0800, John Harrison wrote:
>> On 2/24/26 11:30, Kamil Konieczny wrote:
>>> Hi John,
>>> On 2026-02-17 at 08:58:01 -0800, John Harrison wrote:
>>>> On 2/16/26 19:00, Kandpal, Suraj wrote:
>>>>>> Subject: [PATCH i-g-t v2 1/2] tool/intel_hdcp: Show actual HDCP status
>>>>>>
>>>>>> The 'video player' showed what the requested HDCP mode was but not the
>>>>>> actual status. The menu system does redraw the screen with a special colour to
>>>>>> denote the status, but that is immediately overwritten by the player thread. So
>>>>>> all you get is a very brief full screen flicker (to be fixed in next patch).
>>>>> So we do not want to refer to the next "patch" just mention what this patch is doing.
>>>>> Also do not call it a "patch" since once it gets merged it becomes a commit.
>>>> But this patch is part of a series and a series of tightly related patches
>>>> has always explained what is happening as a whole. Otherwise, you
>>>> effectively have this patch/commit saying "this is totally broken and I'm
>>>> not doing anything about it".
>>>>
>>>>>> Instead, add the current HDCP status as an extra text line of the video player's
>>>>>> output so it is permanently visible and clear.
>>>>>>
>>>>>> Also, move the enable of HDCP inside the mutex lock with the internal state
>>>>>> change. That way, the newly added 'enabled/desired'
>>>>>> line does not update in advance of the 'requested' line (due to the video player
>>>>>> thread being asynchronous and managing to turn HDCP on while the menu
>>>>>> thread is waiting on the mutex lock).
>>>>>> v2: Split into two patches (review feedback by Kamil)
>>>>> You can just write (Kamil) here
>>>>>
>>>>>> CC: Santhosh Reddy Guddati <santhosh.reddy.guddati@intel.com>
>>>>>> CC: Suraj Kandpal <suraj.kandpal@intel.com>
>>>>>> Signed-off-by: John Harrison <John.Harrison@Igalia.com>
>>>>>> Acked-by: Kamil Konieczny <kamil.konieczny@linux.intel.com>
>>>>>> ---
>>>>>>     tools/intel_hdcp.c | 15 +++++++++++++--
>>>>>>     1 file changed, 13 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/tools/intel_hdcp.c b/tools/intel_hdcp.c index cd5d2b6be..8e29c5e32
>>>>>> 100644
>>>>>> --- a/tools/intel_hdcp.c
>>>>>> +++ b/tools/intel_hdcp.c
>>>>>> @@ -482,6 +482,7 @@ static void *video_player_thread(void *arg)
>>>>>>     	char timer_str[32];
>>>>>>     	time_t current_time, elapsed;
>>>>>>     	int minutes, seconds;
>>>>>> +	const char *status = NULL;
>>>>>>
>>>>>>     	data = (data_t *)arg;
>>>>>>     	cur_type = data->hdcp_type;
>>>>>> @@ -502,6 +503,8 @@ static void *video_player_thread(void *arg)
>>>>>>     			prev_type = cur_type;
>>>>>>     		}
>>>>>>
>>>>>> +		status = get_hdcp_status(data, data->selected_connector);
>>>>>> +
>>>>>>     		switch (cur_type) {
>>>>>>     		case HDCP_TYPE_1_4:
>>>>>>     			bg_r = 0.0; bg_g = 0.7; bg_b = 0.0;
>>>>>> @@ -544,10 +547,18 @@ static void *video_player_thread(void *arg)
>>>>>>     		cairo_show_text(cr, timer_str);
>>>>>>
>>>>>>     		/* Draw HDCP status above timer */
>>>>>> +		cairo_set_font_size(cr, font_size);
>>>>>> +		cairo_text_extents(cr, status, &ext);
>>>>>> +		x = data->width - ext.width - 20;
>>>>>> +		y = data->height - ext.height - 20;
>>>>>> +
>>>>>> +		cairo_move_to(cr, x, y);
>>>>>> +		cairo_show_text(cr, status);
>>>>>> +
>>>>>>     		cairo_set_font_size(cr, font_size);
>>>>>>     		cairo_text_extents(cr, hdcp_str, &ext);
>>>>>>     		x = data->width - ext.width - 20;
>>>>>> -		y = data->height - ext.height - 60;
>>>>>> +		y = data->height - ext.height - 120;
>>>>>>
>>>>>>     		cairo_move_to(cr, x, y);
>>>>>>     		cairo_show_text(cr, hdcp_str);
>>>>>> @@ -605,8 +616,8 @@ static void stop_video_player(data_t *data)
>>>>>>
>>>>>>     static void enable_hdcp_type(data_t *data, enum hdcp_type type)  {
>>>>>> -		set_hdcp_prop(data, CP_DESIRED, type, data-
>>>>>>> selected_connector);
>>>>>>     		pthread_mutex_lock(&data->lock);
>>>>>> +		set_hdcp_prop(data, CP_DESIRED, type, data-
>>>>>>> selected_connector);
>>>>> Move this change into its own patch logically a different change
>>>> But it is not a logically different change. As per the commit description,
>>>> the only reason for making this change is because the video thread is now
>>>> displaying the HDCP status. Without the display of the status, it does not
>>>> matter that the change occurs asynchronously to that display (because there
>>>> is nothing for it to be asynchronous to). So splitting this out into a later
>>>> patch means this patch would be introducing a bug and the follow up patch
>>> Would it help if this change will be the first one in a series?
>>>
>>> If yes, then it should go first as a fix. If not, if this one
>>> change do not help in anything then imho it should stay in this
>>> one bigger change.
>> That's the thing, there is no reason for the set_hdcp_prop call to be inside
>> the mutex lock without adding the display of the HDCP status to the video
>> thread. Hence the reason it was previously not inside the lock.
>>
>> The lock is protecting the display thread. If the display thread does not
>> show HDCP status then changing the status asynchronously to that thread is
>> totally fine. But with the changes in this patch to add the status to the
>> display thread, having an asynchronous update does cause 'flickering' of the
>> display - the 'active' line changes before the 'requested' line does.
>>
>> You could move the re-order into it's own private patch. But given that it
>> is always best to do the least amount of work inside a lock, that patch
>> would have to have a description of 'moving this because of changes coming
>> in the next commit'. Which is not ideal. Hence I think it is better kept
>> together with the rest of this patch.
>>
>> Thanks,
>> John.
> Sound reasonable. Suraj could you please ack or review this series?
> Does it improve this tool and works as intended?
>
> Thanks in advance!
>
> Regards,
> Kamil
>
>>> Regards,
>>> Kamil
>>>
>>>> would need a 'Fixes' tag. Which is just pointless for the sake of a one line
>>>> delta. Or it could be a prior patch but then it would need to say 'doing
>>>> this thing that is pointless now but is required by a future patch' and you
>>>> have already said you don't like descriptions referring to future changes.
>>>>
>>>> John.
>>>>
>>>>> Regards,
>>>>> Suraj Kandpal
>>>>>
>>>>>>     		data->hdcp_type = type;
>>>>>>     		data->video_start_time = time(NULL);
>>>>>>     		pthread_mutex_unlock(&data->lock);
>>>>>> --
>>>>>> 2.43.0

RE: [PATCH i-g-t v2 1/2] tool/intel_hdcp: Show actual HDCP status

[Kandpal, Suraj]

> Subject: Re: [PATCH i-g-t v2 1/2] tool/intel_hdcp: Show actual HDCP status
> 
> @Suraj, ping?
> 
> On 3/16/26 12:06, Kamil Konieczny wrote:
> > Hi John,
> > On 2026-02-24 at 14:31:51 -0800, John Harrison wrote:
> >> On 2/24/26 11:30, Kamil Konieczny wrote:
> >>> Hi John,
> >>> On 2026-02-17 at 08:58:01 -0800, John Harrison wrote:
> >>>> On 2/16/26 19:00, Kandpal, Suraj wrote:
> >>>>>> Subject: [PATCH i-g-t v2 1/2] tool/intel_hdcp: Show actual HDCP
> >>>>>> status
> >>>>>>
> >>>>>> The 'video player' showed what the requested HDCP mode was but
> >>>>>> not the actual status. The menu system does redraw the screen
> >>>>>> with a special colour to denote the status, but that is
> >>>>>> immediately overwritten by the player thread. So all you get is a very
> brief full screen flicker (to be fixed in next patch).
> >>>>> So we do not want to refer to the next "patch" just mention what this
> patch is doing.
> >>>>> Also do not call it a "patch" since once it gets merged it becomes a
> commit.
> >>>> But this patch is part of a series and a series of tightly related
> >>>> patches has always explained what is happening as a whole.
> >>>> Otherwise, you effectively have this patch/commit saying "this is
> >>>> totally broken and I'm not doing anything about it".

This comment still stands maybe misunderstood.
I meant that in the commit message we do not say this " this patch does this ...."
But something like "this commit ...." reason is when you merge it really is not a patch
It becomes "commit" and you don't want to change the message when you are merging the series.
Also when you want to refer to the to upcoming patches it needs to be referred as "upcoming/later commits."

> >>>>
> >>>>>> Instead, add the current HDCP status as an extra text line of the
> >>>>>> video player's output so it is permanently visible and clear.
> >>>>>>
> >>>>>> Also, move the enable of HDCP inside the mutex lock with the
> >>>>>> internal state change. That way, the newly added 'enabled/desired'
> >>>>>> line does not update in advance of the 'requested' line (due to
> >>>>>> the video player thread being asynchronous and managing to turn
> >>>>>> HDCP on while the menu thread is waiting on the mutex lock).
> >>>>>> v2: Split into two patches (review feedback by Kamil)
> >>>>> You can just write (Kamil) here
> >>>>>
> >>>>>> CC: Santhosh Reddy Guddati <santhosh.reddy.guddati@intel.com>
> >>>>>> CC: Suraj Kandpal <suraj.kandpal@intel.com>
> >>>>>> Signed-off-by: John Harrison <John.Harrison@Igalia.com>
> >>>>>> Acked-by: Kamil Konieczny <kamil.konieczny@linux.intel.com>
> >>>>>> ---
> >>>>>>     tools/intel_hdcp.c | 15 +++++++++++++--
> >>>>>>     1 file changed, 13 insertions(+), 2 deletions(-)
> >>>>>>
> >>>>>> diff --git a/tools/intel_hdcp.c b/tools/intel_hdcp.c index
> >>>>>> cd5d2b6be..8e29c5e32
> >>>>>> 100644
> >>>>>> --- a/tools/intel_hdcp.c
> >>>>>> +++ b/tools/intel_hdcp.c
> >>>>>> @@ -482,6 +482,7 @@ static void *video_player_thread(void *arg)
> >>>>>>     	char timer_str[32];
> >>>>>>     	time_t current_time, elapsed;
> >>>>>>     	int minutes, seconds;
> >>>>>> +	const char *status = NULL;
> >>>>>>
> >>>>>>     	data = (data_t *)arg;
> >>>>>>     	cur_type = data->hdcp_type;
> >>>>>> @@ -502,6 +503,8 @@ static void *video_player_thread(void *arg)
> >>>>>>     			prev_type = cur_type;
> >>>>>>     		}
> >>>>>>
> >>>>>> +		status = get_hdcp_status(data, data-
> >selected_connector);
> >>>>>> +
> >>>>>>     		switch (cur_type) {
> >>>>>>     		case HDCP_TYPE_1_4:
> >>>>>>     			bg_r = 0.0; bg_g = 0.7; bg_b = 0.0; @@ -544,10
> +547,18 @@
> >>>>>> static void *video_player_thread(void *arg)
> >>>>>>     		cairo_show_text(cr, timer_str);
> >>>>>>
> >>>>>>     		/* Draw HDCP status above timer */
> >>>>>> +		cairo_set_font_size(cr, font_size);
> >>>>>> +		cairo_text_extents(cr, status, &ext);
> >>>>>> +		x = data->width - ext.width - 20;
> >>>>>> +		y = data->height - ext.height - 20;
> >>>>>> +
> >>>>>> +		cairo_move_to(cr, x, y);
> >>>>>> +		cairo_show_text(cr, status);
> >>>>>> +
> >>>>>>     		cairo_set_font_size(cr, font_size);
> >>>>>>     		cairo_text_extents(cr, hdcp_str, &ext);
> >>>>>>     		x = data->width - ext.width - 20;
> >>>>>> -		y = data->height - ext.height - 60;
> >>>>>> +		y = data->height - ext.height - 120;
> >>>>>>
> >>>>>>     		cairo_move_to(cr, x, y);
> >>>>>>     		cairo_show_text(cr, hdcp_str); @@ -605,8 +616,8 @@
> static
> >>>>>> void stop_video_player(data_t *data)
> >>>>>>
> >>>>>>     static void enable_hdcp_type(data_t *data, enum hdcp_type type)  {
> >>>>>> -		set_hdcp_prop(data, CP_DESIRED, type, data-
> >>>>>>> selected_connector);
> >>>>>>     		pthread_mutex_lock(&data->lock);
> >>>>>> +		set_hdcp_prop(data, CP_DESIRED, type, data-
> >>>>>>> selected_connector);
> >>>>> Move this change into its own patch logically a different change
> >>>> But it is not a logically different change. As per the commit
> >>>> description, the only reason for making this change is because the
> >>>> video thread is now displaying the HDCP status. Without the display
> >>>> of the status, it does not matter that the change occurs
> >>>> asynchronously to that display (because there is nothing for it to
> >>>> be asynchronous to). So splitting this out into a later patch means
> >>>> this patch would be introducing a bug and the follow up patch
> >>> Would it help if this change will be the first one in a series?
> >>>
> >>> If yes, then it should go first as a fix. If not, if this one change
> >>> do not help in anything then imho it should stay in this one bigger
> >>> change.
> >> That's the thing, there is no reason for the set_hdcp_prop call to be
> >> inside the mutex lock without adding the display of the HDCP status
> >> to the video thread. Hence the reason it was previously not inside the lock.
> >>
> >> The lock is protecting the display thread. If the display thread does
> >> not show HDCP status then changing the status asynchronously to that
> >> thread is totally fine. But with the changes in this patch to add the
> >> status to the display thread, having an asynchronous update does
> >> cause 'flickering' of the display - the 'active' line changes before the
> 'requested' line does.
> >>
> >> You could move the re-order into it's own private patch. But given
> >> that it is always best to do the least amount of work inside a lock,
> >> that patch would have to have a description of 'moving this because
> >> of changes coming in the next commit'. Which is not ideal. Hence I
> >> think it is better kept together with the rest of this patch.
> >>
> >> Thanks,
> >> John.
> > Sound reasonable. Suraj could you please ack or review this series?
> > Does it improve this tool and works as intended?
> >

Currently I am seeing some alignment issues when running the tool.
Also there can be a better approach to fix the flicker. Ideally we targeted the type to only be set
When HDCP is enabled but on revisiting the code that does not happen which is the root cause of the flicker issue.
I want set_hdcp_prop type to have a data and status variable which it only changes in the mutex when it makes sure HDCP
Has been enabled/disabled.

Then let the video player thread take the mutex read these variable and change the buffer color based on the actual status
Green if enabled and red if disabled. We can have the status ENABLED DESIRED and UNDESIRED written on top not a fan of it but it would make things
Clearer but not at the bottom right where the HDCP type is written.

Santhosh can work on it.

Regards,
Suraj Kandpal 
  
> > Thanks in advance!
> >
> > Regards,
> > Kamil
> >
> >>> Regards,
> >>> Kamil
> >>>
> >>>> would need a 'Fixes' tag. Which is just pointless for the sake of a
> >>>> one line delta. Or it could be a prior patch but then it would need
> >>>> to say 'doing this thing that is pointless now but is required by a
> >>>> future patch' and you have already said you don't like descriptions
> referring to future changes.
> >>>>
> >>>> John.
> >>>>
> >>>>> Regards,
> >>>>> Suraj Kandpal
> >>>>>
> >>>>>>     		data->hdcp_type = type;
> >>>>>>     		data->video_start_time = time(NULL);
> >>>>>>     		pthread_mutex_unlock(&data->lock);
> >>>>>> --
> >>>>>> 2.43.0

Re: [PATCH i-g-t v2 1/2] tool/intel_hdcp: Show actual HDCP status

[Reddy Guddati, Santhosh]

Hi Suraj,

On 24-03-2026 09:57, Kandpal, Suraj wrote:
>> Subject: Re: [PATCH i-g-t v2 1/2] tool/intel_hdcp: Show actual HDCP status
>>
>> @Suraj, ping?
>>
>> On 3/16/26 12:06, Kamil Konieczny wrote:
>>> Hi John,
>>> On 2026-02-24 at 14:31:51 -0800, John Harrison wrote:
>>>> On 2/24/26 11:30, Kamil Konieczny wrote:
>>>>> Hi John,
>>>>> On 2026-02-17 at 08:58:01 -0800, John Harrison wrote:
>>>>>> On 2/16/26 19:00, Kandpal, Suraj wrote:
>>>>>>>> Subject: [PATCH i-g-t v2 1/2] tool/intel_hdcp: Show actual HDCP
>>>>>>>> status
>>>>>>>>
>>>>>>>> The 'video player' showed what the requested HDCP mode was but
>>>>>>>> not the actual status. The menu system does redraw the screen
>>>>>>>> with a special colour to denote the status, but that is
>>>>>>>> immediately overwritten by the player thread. So all you get is a very
>> brief full screen flicker (to be fixed in next patch).
>>>>>>> So we do not want to refer to the next "patch" just mention what this
>> patch is doing.
>>>>>>> Also do not call it a "patch" since once it gets merged it becomes a
>> commit.
>>>>>> But this patch is part of a series and a series of tightly related
>>>>>> patches has always explained what is happening as a whole.
>>>>>> Otherwise, you effectively have this patch/commit saying "this is
>>>>>> totally broken and I'm not doing anything about it".
> 
> This comment still stands maybe misunderstood.
> I meant that in the commit message we do not say this " this patch does this ...."
> But something like "this commit ...." reason is when you merge it really is not a patch
> It becomes "commit" and you don't want to change the message when you are merging the series.
> Also when you want to refer to the to upcoming patches it needs to be referred as "upcoming/later commits."
> 
>>>>>>
>>>>>>>> Instead, add the current HDCP status as an extra text line of the
>>>>>>>> video player's output so it is permanently visible and clear.
>>>>>>>>
>>>>>>>> Also, move the enable of HDCP inside the mutex lock with the
>>>>>>>> internal state change. That way, the newly added 'enabled/desired'
>>>>>>>> line does not update in advance of the 'requested' line (due to
>>>>>>>> the video player thread being asynchronous and managing to turn
>>>>>>>> HDCP on while the menu thread is waiting on the mutex lock).
>>>>>>>> v2: Split into two patches (review feedback by Kamil)
>>>>>>> You can just write (Kamil) here
>>>>>>>
>>>>>>>> CC: Santhosh Reddy Guddati <santhosh.reddy.guddati@intel.com>
>>>>>>>> CC: Suraj Kandpal <suraj.kandpal@intel.com>
>>>>>>>> Signed-off-by: John Harrison <John.Harrison@Igalia.com>
>>>>>>>> Acked-by: Kamil Konieczny <kamil.konieczny@linux.intel.com>
>>>>>>>> ---
>>>>>>>>      tools/intel_hdcp.c | 15 +++++++++++++--
>>>>>>>>      1 file changed, 13 insertions(+), 2 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/tools/intel_hdcp.c b/tools/intel_hdcp.c index
>>>>>>>> cd5d2b6be..8e29c5e32
>>>>>>>> 100644
>>>>>>>> --- a/tools/intel_hdcp.c
>>>>>>>> +++ b/tools/intel_hdcp.c
>>>>>>>> @@ -482,6 +482,7 @@ static void *video_player_thread(void *arg)
>>>>>>>>      	char timer_str[32];
>>>>>>>>      	time_t current_time, elapsed;
>>>>>>>>      	int minutes, seconds;
>>>>>>>> +	const char *status = NULL;
>>>>>>>>
>>>>>>>>      	data = (data_t *)arg;
>>>>>>>>      	cur_type = data->hdcp_type;
>>>>>>>> @@ -502,6 +503,8 @@ static void *video_player_thread(void *arg)
>>>>>>>>      			prev_type = cur_type;
>>>>>>>>      		}
>>>>>>>>
>>>>>>>> +		status = get_hdcp_status(data, data-
>>> selected_connector);
>>>>>>>> +
>>>>>>>>      		switch (cur_type) {
>>>>>>>>      		case HDCP_TYPE_1_4:
>>>>>>>>      			bg_r = 0.0; bg_g = 0.7; bg_b = 0.0; @@ -544,10
>> +547,18 @@
>>>>>>>> static void *video_player_thread(void *arg)
>>>>>>>>      		cairo_show_text(cr, timer_str);
>>>>>>>>
>>>>>>>>      		/* Draw HDCP status above timer */
>>>>>>>> +		cairo_set_font_size(cr, font_size);
>>>>>>>> +		cairo_text_extents(cr, status, &ext);
>>>>>>>> +		x = data->width - ext.width - 20;
>>>>>>>> +		y = data->height - ext.height - 20;
>>>>>>>> +
>>>>>>>> +		cairo_move_to(cr, x, y);
>>>>>>>> +		cairo_show_text(cr, status);
>>>>>>>> +
>>>>>>>>      		cairo_set_font_size(cr, font_size);
>>>>>>>>      		cairo_text_extents(cr, hdcp_str, &ext);
>>>>>>>>      		x = data->width - ext.width - 20;
>>>>>>>> -		y = data->height - ext.height - 60;
>>>>>>>> +		y = data->height - ext.height - 120;
>>>>>>>>
>>>>>>>>      		cairo_move_to(cr, x, y);
>>>>>>>>      		cairo_show_text(cr, hdcp_str); @@ -605,8 +616,8 @@
>> static
>>>>>>>> void stop_video_player(data_t *data)
>>>>>>>>
>>>>>>>>      static void enable_hdcp_type(data_t *data, enum hdcp_type type)  {
>>>>>>>> -		set_hdcp_prop(data, CP_DESIRED, type, data-
>>>>>>>>> selected_connector);
>>>>>>>>      		pthread_mutex_lock(&data->lock);
>>>>>>>> +		set_hdcp_prop(data, CP_DESIRED, type, data-
>>>>>>>>> selected_connector);
>>>>>>> Move this change into its own patch logically a different change
>>>>>> But it is not a logically different change. As per the commit
>>>>>> description, the only reason for making this change is because the
>>>>>> video thread is now displaying the HDCP status. Without the display
>>>>>> of the status, it does not matter that the change occurs
>>>>>> asynchronously to that display (because there is nothing for it to
>>>>>> be asynchronous to). So splitting this out into a later patch means
>>>>>> this patch would be introducing a bug and the follow up patch
>>>>> Would it help if this change will be the first one in a series?
>>>>>
>>>>> If yes, then it should go first as a fix. If not, if this one change
>>>>> do not help in anything then imho it should stay in this one bigger
>>>>> change.
>>>> That's the thing, there is no reason for the set_hdcp_prop call to be
>>>> inside the mutex lock without adding the display of the HDCP status
>>>> to the video thread. Hence the reason it was previously not inside the lock.
>>>>
>>>> The lock is protecting the display thread. If the display thread does
>>>> not show HDCP status then changing the status asynchronously to that
>>>> thread is totally fine. But with the changes in this patch to add the
>>>> status to the display thread, having an asynchronous update does
>>>> cause 'flickering' of the display - the 'active' line changes before the
>> 'requested' line does.
>>>>
>>>> You could move the re-order into it's own private patch. But given
>>>> that it is always best to do the least amount of work inside a lock,
>>>> that patch would have to have a description of 'moving this because
>>>> of changes coming in the next commit'. Which is not ideal. Hence I
>>>> think it is better kept together with the rest of this patch.
>>>>
>>>> Thanks,
>>>> John.
>>> Sound reasonable. Suraj could you please ack or review this series?
>>> Does it improve this tool and works as intended?
>>>
> 
> Currently I am seeing some alignment issues when running the tool.
> Also there can be a better approach to fix the flicker. Ideally we targeted the type to only be set
> When HDCP is enabled but on revisiting the code that does not happen which is the root cause of the flicker issue.
> I want set_hdcp_prop type to have a data and status variable which it only changes in the mutex when it makes sure HDCP
> Has been enabled/disabled.
> 
> Then let the video player thread take the mutex read these variable and change the buffer color based on the actual status
> Green if enabled and red if disabled. We can have the status ENABLED DESIRED and UNDESIRED written on top not a fan of it but it would make things
> Clearer but not at the bottom right where the HDCP type is written.
> 
> Santhosh can work on it.

Sure, I will be working on this refactoring and fixing the issues on tool.
John , Can you please create a gitlab for this issue.

Regards,
Santhosh

> 
> Regards,
> Suraj Kandpal
>    
>>> Thanks in advance!
>>>
>>> Regards,
>>> Kamil
>>>
>>>>> Regards,
>>>>> Kamil
>>>>>
>>>>>> would need a 'Fixes' tag. Which is just pointless for the sake of a
>>>>>> one line delta. Or it could be a prior patch but then it would need
>>>>>> to say 'doing this thing that is pointless now but is required by a
>>>>>> future patch' and you have already said you don't like descriptions
>> referring to future changes.
>>>>>>
>>>>>> John.
>>>>>>
>>>>>>> Regards,
>>>>>>> Suraj Kandpal
>>>>>>>
>>>>>>>>      		data->hdcp_type = type;
>>>>>>>>      		data->video_start_time = time(NULL);
>>>>>>>>      		pthread_mutex_unlock(&data->lock);
>>>>>>>> --
>>>>>>>> 2.43.0
> 

Re: [PATCH i-g-t v2 1/2] tool/intel_hdcp: Show actual HDCP status

[John Harrison]

[-- Attachment #1: Type: text/plain, Size: 10537 bytes --]

On 3/23/26 21:27, Kandpal, Suraj wrote:
>> Subject: Re: [PATCH i-g-t v2 1/2] tool/intel_hdcp: Show actual HDCP status
>>
>> @Suraj, ping?
>>
>> On 3/16/26 12:06, Kamil Konieczny wrote:
>>> Hi John,
>>> On 2026-02-24 at 14:31:51 -0800, John Harrison wrote:
>>>> On 2/24/26 11:30, Kamil Konieczny wrote:
>>>>> Hi John,
>>>>> On 2026-02-17 at 08:58:01 -0800, John Harrison wrote:
>>>>>> On 2/16/26 19:00, Kandpal, Suraj wrote:
>>>>>>>> Subject: [PATCH i-g-t v2 1/2] tool/intel_hdcp: Show actual HDCP
>>>>>>>> status
>>>>>>>>
>>>>>>>> The 'video player' showed what the requested HDCP mode was but
>>>>>>>> not the actual status. The menu system does redraw the screen
>>>>>>>> with a special colour to denote the status, but that is
>>>>>>>> immediately overwritten by the player thread. So all you get is a very
>> brief full screen flicker (to be fixed in next patch).
>>>>>>> So we do not want to refer to the next "patch" just mention what this
>> patch is doing.
>>>>>>> Also do not call it a "patch" since once it gets merged it becomes a
>> commit.
>>>>>> But this patch is part of a series and a series of tightly related
>>>>>> patches has always explained what is happening as a whole.
>>>>>> Otherwise, you effectively have this patch/commit saying "this is
>>>>>> totally broken and I'm not doing anything about it".
> This comment still stands maybe misunderstood.
> I meant that in the commit message we do not say this " this patch does this ...."
> But something like "this commit ...." reason is when you merge it really is not a patch
Commits are just a superset of patches that have been accepted into a 
tree. Git is a patch management system. You can cherry-pick a commit 
into a different branch, apply it to a completely separate tree, etc. It 
is very much still a patch.

And to quote the kernel patch submission guidelines 
(https://docs.kernel.org/process/submitting-patches.html):

    If one patch depends on another patch in order for a change to be
    complete, that is OK. Simply note*“this patch depends on patch X”*in
    your patch description.


> It becomes "commit" and you don't want to change the message when you are merging the series.
> Also when you want to refer to the to upcoming patches it needs to be referred as "upcoming/later commits."
>
>>>>>>>> Instead, add the current HDCP status as an extra text line of the
>>>>>>>> video player's output so it is permanently visible and clear.
>>>>>>>>
>>>>>>>> Also, move the enable of HDCP inside the mutex lock with the
>>>>>>>> internal state change. That way, the newly added 'enabled/desired'
>>>>>>>> line does not update in advance of the 'requested' line (due to
>>>>>>>> the video player thread being asynchronous and managing to turn
>>>>>>>> HDCP on while the menu thread is waiting on the mutex lock).
>>>>>>>> v2: Split into two patches (review feedback by Kamil)
>>>>>>> You can just write (Kamil) here
>>>>>>>
>>>>>>>> CC: Santhosh Reddy Guddati<santhosh.reddy.guddati@intel.com>
>>>>>>>> CC: Suraj Kandpal<suraj.kandpal@intel.com>
>>>>>>>> Signed-off-by: John Harrison<John.Harrison@Igalia.com>
>>>>>>>> Acked-by: Kamil Konieczny<kamil.konieczny@linux.intel.com>
>>>>>>>> ---
>>>>>>>>      tools/intel_hdcp.c | 15 +++++++++++++--
>>>>>>>>      1 file changed, 13 insertions(+), 2 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/tools/intel_hdcp.c b/tools/intel_hdcp.c index
>>>>>>>> cd5d2b6be..8e29c5e32
>>>>>>>> 100644
>>>>>>>> --- a/tools/intel_hdcp.c
>>>>>>>> +++ b/tools/intel_hdcp.c
>>>>>>>> @@ -482,6 +482,7 @@ static void *video_player_thread(void *arg)
>>>>>>>>      	char timer_str[32];
>>>>>>>>      	time_t current_time, elapsed;
>>>>>>>>      	int minutes, seconds;
>>>>>>>> +	const char *status = NULL;
>>>>>>>>
>>>>>>>>      	data = (data_t *)arg;
>>>>>>>>      	cur_type = data->hdcp_type;
>>>>>>>> @@ -502,6 +503,8 @@ static void *video_player_thread(void *arg)
>>>>>>>>      			prev_type = cur_type;
>>>>>>>>      		}
>>>>>>>>
>>>>>>>> +		status = get_hdcp_status(data, data-
>>> selected_connector);
>>>>>>>> +
>>>>>>>>      		switch (cur_type) {
>>>>>>>>      		case HDCP_TYPE_1_4:
>>>>>>>>      			bg_r = 0.0; bg_g = 0.7; bg_b = 0.0; @@ -544,10
>> +547,18 @@
>>>>>>>> static void *video_player_thread(void *arg)
>>>>>>>>      		cairo_show_text(cr, timer_str);
>>>>>>>>
>>>>>>>>      		/* Draw HDCP status above timer */
>>>>>>>> +		cairo_set_font_size(cr, font_size);
>>>>>>>> +		cairo_text_extents(cr, status, &ext);
>>>>>>>> +		x = data->width - ext.width - 20;
>>>>>>>> +		y = data->height - ext.height - 20;
>>>>>>>> +
>>>>>>>> +		cairo_move_to(cr, x, y);
>>>>>>>> +		cairo_show_text(cr, status);
>>>>>>>> +
>>>>>>>>      		cairo_set_font_size(cr, font_size);
>>>>>>>>      		cairo_text_extents(cr, hdcp_str, &ext);
>>>>>>>>      		x = data->width - ext.width - 20;
>>>>>>>> -		y = data->height - ext.height - 60;
>>>>>>>> +		y = data->height - ext.height - 120;
>>>>>>>>
>>>>>>>>      		cairo_move_to(cr, x, y);
>>>>>>>>      		cairo_show_text(cr, hdcp_str); @@ -605,8 +616,8 @@
>> static
>>>>>>>> void stop_video_player(data_t *data)
>>>>>>>>
>>>>>>>>      static void enable_hdcp_type(data_t *data, enum hdcp_type type)  {
>>>>>>>> -		set_hdcp_prop(data, CP_DESIRED, type, data-
>>>>>>>>> selected_connector);
>>>>>>>>      		pthread_mutex_lock(&data->lock);
>>>>>>>> +		set_hdcp_prop(data, CP_DESIRED, type, data-
>>>>>>>>> selected_connector);
>>>>>>> Move this change into its own patch logically a different change
>>>>>> But it is not a logically different change. As per the commit
>>>>>> description, the only reason for making this change is because the
>>>>>> video thread is now displaying the HDCP status. Without the display
>>>>>> of the status, it does not matter that the change occurs
>>>>>> asynchronously to that display (because there is nothing for it to
>>>>>> be asynchronous to). So splitting this out into a later patch means
>>>>>> this patch would be introducing a bug and the follow up patch
>>>>> Would it help if this change will be the first one in a series?
>>>>>
>>>>> If yes, then it should go first as a fix. If not, if this one change
>>>>> do not help in anything then imho it should stay in this one bigger
>>>>> change.
>>>> That's the thing, there is no reason for the set_hdcp_prop call to be
>>>> inside the mutex lock without adding the display of the HDCP status
>>>> to the video thread. Hence the reason it was previously not inside the lock.
>>>>
>>>> The lock is protecting the display thread. If the display thread does
>>>> not show HDCP status then changing the status asynchronously to that
>>>> thread is totally fine. But with the changes in this patch to add the
>>>> status to the display thread, having an asynchronous update does
>>>> cause 'flickering' of the display - the 'active' line changes before the
>> 'requested' line does.
>>>> You could move the re-order into it's own private patch. But given
>>>> that it is always best to do the least amount of work inside a lock,
>>>> that patch would have to have a description of 'moving this because
>>>> of changes coming in the next commit'. Which is not ideal. Hence I
>>>> think it is better kept together with the rest of this patch.
>>>>
>>>> Thanks,
>>>> John.
>>> Sound reasonable. Suraj could you please ack or review this series?
>>> Does it improve this tool and works as intended?
>>>
> Currently I am seeing some alignment issues when running the tool.
I don't know what you mean by this? Alignment of the info text on the 
screen?

> Also there can be a better approach to fix the flicker. Ideally we targeted the type to only be set
> When HDCP is enabled but on revisiting the code that does not happen which is the root cause of the flicker issue.
Type meaning content type? I.e. only relevant to HDCP2.2? I'm not seeing 
how that is the root cause of the flicker. The fundamental cause of the 
flicker is that there are two independent threads both attempting to 
draw the entire screen at different times. Having only one thread 
responsible for drawing the display is by far the simplest and most 
robust way to ensure there is no flicker.


> I want set_hdcp_prop type to have a data and status variable which it only changes in the mutex when it makes sure HDCP
> Has been enabled/disabled.
>
> Then let the video player thread take the mutex read these variable and change the buffer color based on the actual status
> Green if enabled and red if disabled. We can have the status ENABLED DESIRED and UNDESIRED written on top not a fan of it but it would make things
> Clearer but not at the bottom right where the HDCP type is written.
I don't get this. Why are you not a fan of reporting the HDCP setting? 
The colour coding is a big visual indicator but if you are wanting to 
drop the other colours and have just red vs green then there is a lot 
more combinations of situations to report than just enabled vs disabled. 
And if you are going to keep the different colour per situation then 
having a text display means you don't have to search the code to find 
what each specific colour means.

Either way, reporting the requested state in addition to the actual 
state is definitely useful when debugging problems - is HDCP not enabled 
because it was not requested (bug in the request path) or because it 
failed (bug in the hardware). Likewise, why not have the desired and 
actual status lines next to each other? Having them at completely 
different places on the screen would just make it harder to read.

Also, unless my changes are going in completely the wrong direction to 
how you want to rework the code, is there any reason to not merge them 
as a quick fix until you or Santhosh have the time to do your rework? 
It's been almost two months since I posted the fix with no other 
alternatives being posted in the meantime.

John.

>
> Santhosh can work on it.
>
> Regards,
> Suraj Kandpal
>    
>>> Thanks in advance!
>>>
>>> Regards,
>>> Kamil
>>>
>>>>> Regards,
>>>>> Kamil
>>>>>
>>>>>> would need a 'Fixes' tag. Which is just pointless for the sake of a
>>>>>> one line delta. Or it could be a prior patch but then it would need
>>>>>> to say 'doing this thing that is pointless now but is required by a
>>>>>> future patch' and you have already said you don't like descriptions
>> referring to future changes.
>>>>>> John.
>>>>>>
>>>>>>> Regards,
>>>>>>> Suraj Kandpal
>>>>>>>
>>>>>>>>      		data->hdcp_type = type;
>>>>>>>>      		data->video_start_time = time(NULL);
>>>>>>>>      		pthread_mutex_unlock(&data->lock);
>>>>>>>> --
>>>>>>>> 2.43.0

[-- Attachment #2: Type: text/html, Size: 19072 bytes --]

Re: [PATCH i-g-t v2 1/2] tool/intel_hdcp: Show actual HDCP status

[Kamil Konieczny]

Hi John,
On 2026-04-09 at 17:37:22 -0700, John Harrison wrote:
> On 3/23/26 21:27, Kandpal, Suraj wrote:
> > > Subject: Re: [PATCH i-g-t v2 1/2] tool/intel_hdcp: Show actual HDCP status
> > > 
> > > @Suraj, ping?
> > > 
> > > On 3/16/26 12:06, Kamil Konieczny wrote:
> > > > Hi John,
> > > > On 2026-02-24 at 14:31:51 -0800, John Harrison wrote:
> > > > > On 2/24/26 11:30, Kamil Konieczny wrote:
> > > > > > Hi John,
> > > > > > On 2026-02-17 at 08:58:01 -0800, John Harrison wrote:
> > > > > > > On 2/16/26 19:00, Kandpal, Suraj wrote:
> > > > > > > > > Subject: [PATCH i-g-t v2 1/2] tool/intel_hdcp: Show actual HDCP
> > > > > > > > > status
> > > > > > > > > 
> > > > > > > > > The 'video player' showed what the requested HDCP mode was but
> > > > > > > > > not the actual status. The menu system does redraw the screen
> > > > > > > > > with a special colour to denote the status, but that is
> > > > > > > > > immediately overwritten by the player thread. So all you get is a very
> > > brief full screen flicker (to be fixed in next patch).
> > > > > > > > So we do not want to refer to the next "patch" just mention what this
> > > patch is doing.
> > > > > > > > Also do not call it a "patch" since once it gets merged it becomes a
> > > commit.
> > > > > > > But this patch is part of a series and a series of tightly related
> > > > > > > patches has always explained what is happening as a whole.
> > > > > > > Otherwise, you effectively have this patch/commit saying "this is
> > > > > > > totally broken and I'm not doing anything about it".
> > This comment still stands maybe misunderstood.
> > I meant that in the commit message we do not say this " this patch does this ...."
> > But something like "this commit ...." reason is when you merge it really is not a patch
> Commits are just a superset of patches that have been accepted into a tree.
> Git is a patch management system. You can cherry-pick a commit into a
> different branch, apply it to a completely separate tree, etc. It is very
> much still a patch.
> 
> And to quote the kernel patch submission guidelines
> (https://docs.kernel.org/process/submitting-patches.html):
> 
>    If one patch depends on another patch in order for a change to be
>    complete, that is OK. Simply note*“this patch depends on patch X”*in
>    your patch description.
> 
> 
> > It becomes "commit" and you don't want to change the message when you are merging the series.
> > Also when you want to refer to the to upcoming patches it needs to be referred as "upcoming/later commits."
> > 
> > > > > > > > > Instead, add the current HDCP status as an extra text line of the
> > > > > > > > > video player's output so it is permanently visible and clear.
> > > > > > > > > 
> > > > > > > > > Also, move the enable of HDCP inside the mutex lock with the
> > > > > > > > > internal state change. That way, the newly added 'enabled/desired'
> > > > > > > > > line does not update in advance of the 'requested' line (due to
> > > > > > > > > the video player thread being asynchronous and managing to turn
> > > > > > > > > HDCP on while the menu thread is waiting on the mutex lock).
> > > > > > > > > v2: Split into two patches (review feedback by Kamil)
> > > > > > > > You can just write (Kamil) here
> > > > > > > > 
> > > > > > > > > CC: Santhosh Reddy Guddati<santhosh.reddy.guddati@intel.com>
> > > > > > > > > CC: Suraj Kandpal<suraj.kandpal@intel.com>
> > > > > > > > > Signed-off-by: John Harrison<John.Harrison@Igalia.com>
> > > > > > > > > Acked-by: Kamil Konieczny<kamil.konieczny@linux.intel.com>
> > > > > > > > > ---
> > > > > > > > >      tools/intel_hdcp.c | 15 +++++++++++++--
> > > > > > > > >      1 file changed, 13 insertions(+), 2 deletions(-)
> > > > > > > > > 
> > > > > > > > > diff --git a/tools/intel_hdcp.c b/tools/intel_hdcp.c index
> > > > > > > > > cd5d2b6be..8e29c5e32
> > > > > > > > > 100644
> > > > > > > > > --- a/tools/intel_hdcp.c
> > > > > > > > > +++ b/tools/intel_hdcp.c
> > > > > > > > > @@ -482,6 +482,7 @@ static void *video_player_thread(void *arg)
> > > > > > > > >      	char timer_str[32];
> > > > > > > > >      	time_t current_time, elapsed;
> > > > > > > > >      	int minutes, seconds;
> > > > > > > > > +	const char *status = NULL;
> > > > > > > > > 
> > > > > > > > >      	data = (data_t *)arg;
> > > > > > > > >      	cur_type = data->hdcp_type;
> > > > > > > > > @@ -502,6 +503,8 @@ static void *video_player_thread(void *arg)
> > > > > > > > >      			prev_type = cur_type;
> > > > > > > > >      		}
> > > > > > > > > 
> > > > > > > > > +		status = get_hdcp_status(data, data-
> > > > selected_connector);
> > > > > > > > > +
> > > > > > > > >      		switch (cur_type) {
> > > > > > > > >      		case HDCP_TYPE_1_4:
> > > > > > > > >      			bg_r = 0.0; bg_g = 0.7; bg_b = 0.0; @@ -544,10
> > > +547,18 @@
> > > > > > > > > static void *video_player_thread(void *arg)
> > > > > > > > >      		cairo_show_text(cr, timer_str);
> > > > > > > > > 
> > > > > > > > >      		/* Draw HDCP status above timer */
> > > > > > > > > +		cairo_set_font_size(cr, font_size);
> > > > > > > > > +		cairo_text_extents(cr, status, &ext);
> > > > > > > > > +		x = data->width - ext.width - 20;
> > > > > > > > > +		y = data->height - ext.height - 20;
> > > > > > > > > +
> > > > > > > > > +		cairo_move_to(cr, x, y);
> > > > > > > > > +		cairo_show_text(cr, status);
> > > > > > > > > +
> > > > > > > > >      		cairo_set_font_size(cr, font_size);
> > > > > > > > >      		cairo_text_extents(cr, hdcp_str, &ext);
> > > > > > > > >      		x = data->width - ext.width - 20;
> > > > > > > > > -		y = data->height - ext.height - 60;
> > > > > > > > > +		y = data->height - ext.height - 120;
> > > > > > > > > 
> > > > > > > > >      		cairo_move_to(cr, x, y);
> > > > > > > > >      		cairo_show_text(cr, hdcp_str); @@ -605,8 +616,8 @@
> > > static
> > > > > > > > > void stop_video_player(data_t *data)
> > > > > > > > > 
> > > > > > > > >      static void enable_hdcp_type(data_t *data, enum hdcp_type type)  {
> > > > > > > > > -		set_hdcp_prop(data, CP_DESIRED, type, data-
> > > > > > > > > > selected_connector);
> > > > > > > > >      		pthread_mutex_lock(&data->lock);
> > > > > > > > > +		set_hdcp_prop(data, CP_DESIRED, type, data-
> > > > > > > > > > selected_connector);
> > > > > > > > Move this change into its own patch logically a different change
> > > > > > > But it is not a logically different change. As per the commit
> > > > > > > description, the only reason for making this change is because the
> > > > > > > video thread is now displaying the HDCP status. Without the display
> > > > > > > of the status, it does not matter that the change occurs
> > > > > > > asynchronously to that display (because there is nothing for it to
> > > > > > > be asynchronous to). So splitting this out into a later patch means
> > > > > > > this patch would be introducing a bug and the follow up patch
> > > > > > Would it help if this change will be the first one in a series?
> > > > > > 
> > > > > > If yes, then it should go first as a fix. If not, if this one change
> > > > > > do not help in anything then imho it should stay in this one bigger
> > > > > > change.
> > > > > That's the thing, there is no reason for the set_hdcp_prop call to be
> > > > > inside the mutex lock without adding the display of the HDCP status
> > > > > to the video thread. Hence the reason it was previously not inside the lock.
> > > > > 
> > > > > The lock is protecting the display thread. If the display thread does
> > > > > not show HDCP status then changing the status asynchronously to that
> > > > > thread is totally fine. But with the changes in this patch to add the
> > > > > status to the display thread, having an asynchronous update does
> > > > > cause 'flickering' of the display - the 'active' line changes before the
> > > 'requested' line does.
> > > > > You could move the re-order into it's own private patch. But given
> > > > > that it is always best to do the least amount of work inside a lock,
> > > > > that patch would have to have a description of 'moving this because
> > > > > of changes coming in the next commit'. Which is not ideal. Hence I
> > > > > think it is better kept together with the rest of this patch.
> > > > > 
> > > > > Thanks,
> > > > > John.
> > > > Sound reasonable. Suraj could you please ack or review this series?
> > > > Does it improve this tool and works as intended?
> > > > 
> > Currently I am seeing some alignment issues when running the tool.
> I don't know what you mean by this? Alignment of the info text on the
> screen?
> 
> > Also there can be a better approach to fix the flicker. Ideally we targeted the type to only be set
> > When HDCP is enabled but on revisiting the code that does not happen which is the root cause of the flicker issue.
> Type meaning content type? I.e. only relevant to HDCP2.2? I'm not seeing how
> that is the root cause of the flicker. The fundamental cause of the flicker
> is that there are two independent threads both attempting to draw the entire
> screen at different times. Having only one thread responsible for drawing
> the display is by far the simplest and most robust way to ensure there is no
> flicker.
> 
> 
> > I want set_hdcp_prop type to have a data and status variable which it only changes in the mutex when it makes sure HDCP
> > Has been enabled/disabled.
> > 
> > Then let the video player thread take the mutex read these variable and change the buffer color based on the actual status
> > Green if enabled and red if disabled. We can have the status ENABLED DESIRED and UNDESIRED written on top not a fan of it but it would make things
> > Clearer but not at the bottom right where the HDCP type is written.

I am against color coding a state or debug, it could be a problem for
color-blind people.

> I don't get this. Why are you not a fan of reporting the HDCP setting? The
> colour coding is a big visual indicator but if you are wanting to drop the
> other colours and have just red vs green then there is a lot more
> combinations of situations to report than just enabled vs disabled. And if
> you are going to keep the different colour per situation then having a text
> display means you don't have to search the code to find what each specific
> colour means.

I agree here.

> 
> Either way, reporting the requested state in addition to the actual state is
> definitely useful when debugging problems - is HDCP not enabled because it
> was not requested (bug in the request path) or because it failed (bug in the
> hardware). Likewise, why not have the desired and actual status lines next
> to each other? Having them at completely different places on the screen
> would just make it harder to read.

I agree on clear text for info or bug indicators.

> 
> Also, unless my changes are going in completely the wrong direction to how
> you want to rework the code, is there any reason to not merge them as a
> quick fix until you or Santhosh have the time to do your rework? It's been
> almost two months since I posted the fix with no other alternatives being
> posted in the meantime.
> 
> John.
> 

Writing concurrent programs are hard, bugs are hard to detect.
I am inclined to merge this next week as is as it is imho an improvment,
so Suraj or Santosh please reach out to me if you have objections.

Also +cc display devs
Cc: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
Cc: Juha-Pekka Heikkila <juha-pekka.heikkila@intel.com>
Cc: Karthik B S <karthik.b.s@intel.com>
Cc: Swati Sharma <swati2.sharma@intel.com>

Regards,
Kamil

> > 
> > Santhosh can work on it.
> > 
> > Regards,
> > Suraj Kandpal
> > > > Thanks in advance!
> > > > 
> > > > Regards,
> > > > Kamil
> > > > 
> > > > > > Regards,
> > > > > > Kamil
> > > > > > 
> > > > > > > would need a 'Fixes' tag. Which is just pointless for the sake of a
> > > > > > > one line delta. Or it could be a prior patch but then it would need
> > > > > > > to say 'doing this thing that is pointless now but is required by a
> > > > > > > future patch' and you have already said you don't like descriptions
> > > referring to future changes.
> > > > > > > John.
> > > > > > > 
> > > > > > > > Regards,
> > > > > > > > Suraj Kandpal
> > > > > > > > 
> > > > > > > > >      		data->hdcp_type = type;
> > > > > > > > >      		data->video_start_time = time(NULL);
> > > > > > > > >      		pthread_mutex_unlock(&data->lock);
> > > > > > > > > --
> > > > > > > > > 2.43.0