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

["Reddy Guddati, Santhosh" <santhosh.reddy.guddati@intel.com>]

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
>