Re: [Intel-gfx] [PATCH v2 2/5] drm/i915: hdmi: add CEC notifier to intel_hdmi

[Neil Armstrong]

Hi Ville,

On 16/05/2018 16:07, Ville SyrjÃlà wrote:
> On Wed, May 16, 2018 at 09:40:17AM +0200, Neil Armstrong wrote:
>> On 16/05/2018 09:31, Neil Armstrong wrote:
>>> Hi Ville,
>>>
>>> On 15/05/2018 17:35, Ville SyrjÃlà wrote:
>>>> On Tue, May 15, 2018 at 04:42:19PM +0200, Neil Armstrong wrote:
>>>>> This patchs adds the cec_notifier feature to the intel_hdmi part
>>>>> of the i915 DRM driver. It uses the HDMI DRM connector name to differentiate
>>>>> between each HDMI ports.
>>>>> The changes will allow the i915 HDMI code to notify EDID and HPD changes
>>>>> to an eventual CEC adapter.
>>>>>
>>>>> Signed-off-by: Neil Armstrong <narmstrong@xxxxxxxxxxxx>
>>>>> ---
>>>>>  drivers/gpu/drm/i915/Kconfig      |  1 +
>>>>>  drivers/gpu/drm/i915/intel_drv.h  |  2 ++
>>>>>  drivers/gpu/drm/i915/intel_hdmi.c | 12 ++++++++++++
>>>>>  3 files changed, 15 insertions(+)
>>>>>
>>>
>>> [..]
>>>
>>>>>  }
>>>>>  
>>>>> @@ -2031,6 +2037,8 @@ static void chv_hdmi_pre_enable(struct intel_encoder *encoder,
>>>>>  
>>>>>  static void intel_hdmi_destroy(struct drm_connector *connector)
>>>>>  {
>>>>> +	if (intel_attached_hdmi(connector)->notifier)
>>>>> +		cec_notifier_put(intel_attached_hdmi(connector)->notifier);
>>>>>  	kfree(to_intel_connector(connector)->detect_edid);
>>>>>  	drm_connector_cleanup(connector);
>>>>>  	kfree(connector);
>>>>> @@ -2358,6 +2366,10 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
>>>>>  		u32 temp = I915_READ(PEG_BAND_GAP_DATA);
>>>>>  		I915_WRITE(PEG_BAND_GAP_DATA, (temp & ~0xf) | 0xd);
>>>>>  	}
>>>>> +
>>>>> +	intel_hdmi->notifier = cec_notifier_get_conn(dev->dev, connector->name);
>>>>
>>>> What are we doing with the connector name here? Those are not actually
>>>> guaranteed to be stable, and any change in the connector probe order
>>>> may cause the name to change.
>>>
>>> The i915 driver can expose multiple HDMI connectors, but each connected will
>>> have a different EDID and CEC physical address, so we will need a different notifier
>>> for each connector.
>>>
>>> The connector name is DRM dependent, but user-space actually uses this name for
>>> operations, so I supposed it was kind of stable.
>>>
>>> Anyway, is there another stable id/name/whatever that can be used to make a name to
>>> distinguish the HDMI connectors ?
>>
>> Would "HDMI %c", port_name(port) be OK to use ?
> 
> Yeah, something like seems like a reasonable approach. That does mean
> we have to be a little careful with changing enum port or port_name()
> in the future, but I guess we could just add a small function to
> generated the name/id specifically for this thing.
> 
> We're also going to have to think what to do with enum port and
> port_name() on ICL+ though. There we won't just have A-F but also
> TC1-TC<n>. Hmm. Looks like what we have for those ports in our
> codebase ATM is a bit wonky since we haven't even changed
> port_name() to give us the TC<n> type names.
> 
> Also we might not want "HDMI" in the identifier since the physical
> port is not HDMI specific. There are different things we could call
> these but I think we could just go with a generic "Port A-F" and
> "Port TC1-TC<n>" approach. I think something like that should work
> fine for current and upcoming hardware. And in theory that could
> then be used for other things as well which need a stable
> identifier.
> 
> Oh, and now I recall that input subsystem at least has some kind
> of "physical path" property on devices. So I guess what we're
> dicussing is a somewhat similar idea. I think input drivers
> generally include the pci/usb/etc. device in the path as well.
> Even though we currently only ever have the one pci device it
> would seem like decent idea to include that information in our
> identifiers as well. Or what do you think?
> 

Thanks for the clarifications !

Having a "Port <id>" will be great indeed, no need to specify HDMI since
only HDMI connectors will get a CEC notifier anyway.

The issue is that port_name() returns a character, which is lame.
Would it be acceptable to introduce a :

const char *port_identifier(struct drm_i915_private *dev_priv,
			    enum port port)
{
	char *id = devm_kzalloc(dev_priv->drm->dev, 16, GFP_KERNEL);

	if (id)
		return NULL;

	snprintf("Port %c", port_name(port));

	return id;
}

and use the result of this for the cec_notifier connector name ?

Neil