Re: UAPI Re: [PATCH 1/3] drm: Add DRM_MODE_TV_MODE_MONOCHROME

[Harry Wentland <harry.wentland@amd.com>]

On 2024-02-21 04:07, Pekka Paalanen wrote:
> On Fri, 16 Feb 2024 18:48:55 +0000
> Dave Stevenson <dave.stevenson@raspberrypi.com> wrote:
> 
>> From: Nick Hollinghurst <nick.hollinghurst@raspberrypi.com>
>>
>> Add this as a value for enum_drm_connector_tv_mode, represented
>> by the string "Mono", to generate video with no colour encoding
>> or bursts. Define it to have no pedestal (since only NTSC-M calls
>> for a pedestal).
>>
>> Change default mode creation to acommodate the new tv_mode value
>> which comprises both 525-line and 625-line formats.
>>
>> Signed-off-by: Nick Hollinghurst <nick.hollinghurst@raspberrypi.com>
>> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> 
> Hi Dave and Nick,
> 
> since no-one else commented yet, I'd like to remind of the new UAPI
> requirements:
> https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#open-source-userspace-requirements
> 
> AFAIU, adding a new value to an existing enum still counts as new UAPI.
> 

I tend to agree with Pekka. I'm getting tired of seeing new DRM properties
without knowing which canonical upstream user-space project uses it.

Can someone describe this for the "TV mode" property?

Harry

> Maybe there is no need for the full treatment here, or maybe there is,
> I'm not sure. I think you should make some statement about how the new
> UAPI requirements have been addressed.
> 
> Btw. no-one has submitted a record with "TV mode" to
> https://drmdb.emersion.fr/
> It only lists the radeon-specific "tv standard" property. I first
> thought you had mistaken the property name in the cover letter.
> 
> 
> Thanks,
> pq
> 
>> ---
>>  drivers/gpu/drm/drm_connector.c    | 7 +++++++
>>  drivers/gpu/drm/drm_modes.c        | 5 ++++-
>>  drivers/gpu/drm/drm_probe_helper.c | 5 +++--
>>  include/drm/drm_connector.h        | 7 +++++++
>>  4 files changed, 21 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
>> index b0516505f7ae..fe05d27f3404 100644
>> --- a/drivers/gpu/drm/drm_connector.c
>> +++ b/drivers/gpu/drm/drm_connector.c
>> @@ -1005,6 +1005,7 @@ static const struct drm_prop_enum_list drm_tv_mode_enum_list[] = {
>>  	{ DRM_MODE_TV_MODE_PAL_M, "PAL-M" },
>>  	{ DRM_MODE_TV_MODE_PAL_N, "PAL-N" },
>>  	{ DRM_MODE_TV_MODE_SECAM, "SECAM" },
>> +	{ DRM_MODE_TV_MODE_MONOCHROME, "Mono" },
>>  };
>>  DRM_ENUM_NAME_FN(drm_get_tv_mode_name, drm_tv_mode_enum_list)
>>  
>> @@ -1697,6 +1698,12 @@ EXPORT_SYMBOL(drm_connector_attach_dp_subconnector_property);
>>   *		TV Mode is CCIR System B (aka 625-lines) together with
>>   *		the SECAM Color Encoding.
>>   *
>> + *	Mono:
>> + *
>> + *		Use timings appropriate to the DRM mode, including
>> + *		equalizing pulses for a 525-line or 625-line mode,
>> + *		with no pedestal or color encoding.
>> + *
>>   *	Drivers can set up this property by calling
>>   *	drm_mode_create_tv_properties().
>>   */
>> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
>> index c4f88c3a93b7..d274e7b00b79 100644
>> --- a/drivers/gpu/drm/drm_modes.c
>> +++ b/drivers/gpu/drm/drm_modes.c
>> @@ -530,7 +530,8 @@ static int fill_analog_mode(struct drm_device *dev,
>>   * @interlace: whether to compute an interlaced mode
>>   *
>>   * This function creates a struct drm_display_mode instance suited for
>> - * an analog TV output, for one of the usual analog TV mode.
>> + * an analog TV output, for one of the usual analog TV modes. Where
>> + * this is DRM_MODE_TV_MODE_MONOCHROME, a 625-line mode will be created.
>>   *
>>   * Note that @hdisplay is larger than the usual constraints for the PAL
>>   * and NTSC timings, and we'll choose to ignore most timings constraints
>> @@ -568,6 +569,8 @@ struct drm_display_mode *drm_analog_tv_mode(struct drm_device *dev,
>>  	case DRM_MODE_TV_MODE_PAL_N:
>>  		fallthrough;
>>  	case DRM_MODE_TV_MODE_SECAM:
>> +		fallthrough;
>> +	case DRM_MODE_TV_MODE_MONOCHROME:
>>  		analog = DRM_MODE_ANALOG_PAL;
>>  		break;
>>  
>> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
>> index d1e1ade66f81..9254dc2af873 100644
>> --- a/drivers/gpu/drm/drm_probe_helper.c
>> +++ b/drivers/gpu/drm/drm_probe_helper.c
>> @@ -1211,8 +1211,9 @@ int drm_connector_helper_tv_get_modes(struct drm_connector *connector)
>>  	for (i = 0; i < tv_mode_property->num_values; i++)
>>  		supported_tv_modes |= BIT(tv_mode_property->values[i]);
>>  
>> -	if ((supported_tv_modes & ntsc_modes) &&
>> -	    (supported_tv_modes & pal_modes)) {
>> +	if (((supported_tv_modes & ntsc_modes) &&
>> +	     (supported_tv_modes & pal_modes)) ||
>> +	    (supported_tv_modes & BIT(DRM_MODE_TV_MODE_MONOCHROME))) {
>>  		uint64_t default_mode;
>>  
>>  		if (drm_object_property_get_default_value(&connector->base,
>> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
>> index fe88d7fc6b8f..90fd0ea0ca09 100644
>> --- a/include/drm/drm_connector.h
>> +++ b/include/drm/drm_connector.h
>> @@ -200,6 +200,13 @@ enum drm_connector_tv_mode {
>>  	 */
>>  	DRM_MODE_TV_MODE_SECAM,
>>  
>> +	/**
>> +	 * @DRM_MODE_TV_MODE_MONOCHROME: Use timings appropriate to
>> +	 * the DRM mode, including equalizing pulses for a 525-line
>> +	 * or 625-line mode, with no pedestal or color encoding.
>> +	 */
>> +	DRM_MODE_TV_MODE_MONOCHROME,
>> +
>>  	/**
>>  	 * @DRM_MODE_TV_MODE_MAX: Number of analog TV output modes.
>>  	 *
>