drm/bridge: dw-hdmi: Sync comments with actual bus formats order

[PATCH] drm/bridge: dw-hdmi: Sync comments with actual bus formats order

Posted by Cristian Ciocaltea 3 weeks, 6 days ago

Commit d3d6b1bf85ae ("drm: bridge: dw_hdmi: fix preference of RGB modes
over YUV420") changed the order of the output bus formats, but missed to
update accordingly the affected comment blocks related to
dw_hdmi_bridge_atomic_get_output_bus_fmts().

Fix the misleading comments and a context related typo.

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
index 996733ed2c004c83a989cb8da54d8b630d9f2c02..d76aede757175d7ad5873c5d7623abf2d12da73c 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
@@ -2621,6 +2621,7 @@ static int dw_hdmi_connector_create(struct dw_hdmi *hdmi)
  * - MEDIA_BUS_FMT_UYYVYY12_0_5X36,
  * - MEDIA_BUS_FMT_UYYVYY10_0_5X30,
  * - MEDIA_BUS_FMT_UYYVYY8_0_5X24,
+ * - MEDIA_BUS_FMT_RGB888_1X24,
  * - MEDIA_BUS_FMT_YUV16_1X48,
  * - MEDIA_BUS_FMT_RGB161616_1X48,
  * - MEDIA_BUS_FMT_UYVY12_1X24,
@@ -2631,7 +2632,6 @@ static int dw_hdmi_connector_create(struct dw_hdmi *hdmi)
  * - MEDIA_BUS_FMT_RGB101010_1X30,
  * - MEDIA_BUS_FMT_UYVY8_1X16,
  * - MEDIA_BUS_FMT_YUV8_1X24,
- * - MEDIA_BUS_FMT_RGB888_1X24,
  */
 
 /* Can return a maximum of 11 possible output formats for a mode/connector */
@@ -2669,7 +2669,7 @@ static u32 *dw_hdmi_bridge_atomic_get_output_bus_fmts(struct drm_bridge *bridge,
 	}
 
 	/*
-	 * If the current mode enforces 4:2:0, force the output but format
+	 * If the current mode enforces 4:2:0, force the output bus format
 	 * to 4:2:0 and do not add the YUV422/444/RGB formats
 	 */
 	if (conn->ycbcr_420_allowed &&
@@ -2698,14 +2698,14 @@ static u32 *dw_hdmi_bridge_atomic_get_output_bus_fmts(struct drm_bridge *bridge,
 		}
 	}
 
+	/* Default 8bit RGB fallback */
+	output_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24;
+
 	/*
 	 * Order bus formats from 16bit to 8bit and from YUV422 to RGB
-	 * if supported. In any case the default RGB888 format is added
+	 * if supported.
 	 */
 
-	/* Default 8bit RGB fallback */
-	output_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24;
-
 	if (max_bpc >= 16 && info->bpc == 16) {
 		if (info->color_formats & DRM_COLOR_FORMAT_YCBCR444)
 			output_fmts[i++] = MEDIA_BUS_FMT_YUV16_1X48;

---
base-commit: f486c8aa16b8172f63bddc70116a0c897a7f3f02
change-id: 20241130-dw-hdmi-bus-fmt-order-7f6db5db2f0a

Re: [PATCH] drm/bridge: dw-hdmi: Sync comments with actual bus formats order

Posted by Neil Armstrong 3 weeks, 3 days ago

Hi,

On 30/11/2024 00:04, Cristian Ciocaltea wrote:
> Commit d3d6b1bf85ae ("drm: bridge: dw_hdmi: fix preference of RGB modes
> over YUV420") changed the order of the output bus formats, but missed to
> update accordingly the affected comment blocks related to
> dw_hdmi_bridge_atomic_get_output_bus_fmts().
> 
> Fix the misleading comments and a context related typo.
> 
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> ---
>   drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> index 996733ed2c004c83a989cb8da54d8b630d9f2c02..d76aede757175d7ad5873c5d7623abf2d12da73c 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> @@ -2621,6 +2621,7 @@ static int dw_hdmi_connector_create(struct dw_hdmi *hdmi)
>    * - MEDIA_BUS_FMT_UYYVYY12_0_5X36,
>    * - MEDIA_BUS_FMT_UYYVYY10_0_5X30,
>    * - MEDIA_BUS_FMT_UYYVYY8_0_5X24,
> + * - MEDIA_BUS_FMT_RGB888_1X24,
>    * - MEDIA_BUS_FMT_YUV16_1X48,
>    * - MEDIA_BUS_FMT_RGB161616_1X48,
>    * - MEDIA_BUS_FMT_UYVY12_1X24,
> @@ -2631,7 +2632,6 @@ static int dw_hdmi_connector_create(struct dw_hdmi *hdmi)
>    * - MEDIA_BUS_FMT_RGB101010_1X30,
>    * - MEDIA_BUS_FMT_UYVY8_1X16,
>    * - MEDIA_BUS_FMT_YUV8_1X24,
> - * - MEDIA_BUS_FMT_RGB888_1X24,
>    */
>   
>   /* Can return a maximum of 11 possible output formats for a mode/connector */
> @@ -2669,7 +2669,7 @@ static u32 *dw_hdmi_bridge_atomic_get_output_bus_fmts(struct drm_bridge *bridge,
>   	}
>   
>   	/*
> -	 * If the current mode enforces 4:2:0, force the output but format
> +	 * If the current mode enforces 4:2:0, force the output bus format
>   	 * to 4:2:0 and do not add the YUV422/444/RGB formats
>   	 */
>   	if (conn->ycbcr_420_allowed &&
> @@ -2698,14 +2698,14 @@ static u32 *dw_hdmi_bridge_atomic_get_output_bus_fmts(struct drm_bridge *bridge,
>   		}
>   	}
>   
> +	/* Default 8bit RGB fallback */
> +	output_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24;

Why did you move this ? the following comment mentions it

> +
>   	/*
>   	 * Order bus formats from 16bit to 8bit and from YUV422 to RGB
> -	 * if supported. In any case the default RGB888 format is added
> +	 * if supported.
>   	 */
>   
> -	/* Default 8bit RGB fallback */
> -	output_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24;
> -
>   	if (max_bpc >= 16 && info->bpc == 16) {
>   		if (info->color_formats & DRM_COLOR_FORMAT_YCBCR444)
>   			output_fmts[i++] = MEDIA_BUS_FMT_YUV16_1X48;
> 
> ---
> base-commit: f486c8aa16b8172f63bddc70116a0c897a7f3f02
> change-id: 20241130-dw-hdmi-bus-fmt-order-7f6db5db2f0a
>

Re: [PATCH] drm/bridge: dw-hdmi: Sync comments with actual bus formats order

Posted by Cristian Ciocaltea 3 weeks, 3 days ago

On 12/2/24 5:45 PM, Neil Armstrong wrote:
> Hi,
> 
> On 30/11/2024 00:04, Cristian Ciocaltea wrote:
>> Commit d3d6b1bf85ae ("drm: bridge: dw_hdmi: fix preference of RGB modes
>> over YUV420") changed the order of the output bus formats, but missed to
>> update accordingly the affected comment blocks related to
>> dw_hdmi_bridge_atomic_get_output_bus_fmts().
>>
>> Fix the misleading comments and a context related typo.
>>
>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
>> ---
>>   drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 12 ++++++------
>>   1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/
>> drm/bridge/synopsys/dw-hdmi.c
>> index
>> 996733ed2c004c83a989cb8da54d8b630d9f2c02..d76aede757175d7ad5873c5d7623abf2d12da73c 100644
>> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>> @@ -2621,6 +2621,7 @@ static int dw_hdmi_connector_create(struct
>> dw_hdmi *hdmi)
>>    * - MEDIA_BUS_FMT_UYYVYY12_0_5X36,
>>    * - MEDIA_BUS_FMT_UYYVYY10_0_5X30,
>>    * - MEDIA_BUS_FMT_UYYVYY8_0_5X24,
>> + * - MEDIA_BUS_FMT_RGB888_1X24,
>>    * - MEDIA_BUS_FMT_YUV16_1X48,
>>    * - MEDIA_BUS_FMT_RGB161616_1X48,
>>    * - MEDIA_BUS_FMT_UYVY12_1X24,
>> @@ -2631,7 +2632,6 @@ static int dw_hdmi_connector_create(struct
>> dw_hdmi *hdmi)
>>    * - MEDIA_BUS_FMT_RGB101010_1X30,
>>    * - MEDIA_BUS_FMT_UYVY8_1X16,
>>    * - MEDIA_BUS_FMT_YUV8_1X24,
>> - * - MEDIA_BUS_FMT_RGB888_1X24,
>>    */
>>     /* Can return a maximum of 11 possible output formats for a mode/
>> connector */
>> @@ -2669,7 +2669,7 @@ static u32
>> *dw_hdmi_bridge_atomic_get_output_bus_fmts(struct drm_bridge *bridge,
>>       }
>>         /*
>> -     * If the current mode enforces 4:2:0, force the output but format
>> +     * If the current mode enforces 4:2:0, force the output bus format
>>        * to 4:2:0 and do not add the YUV422/444/RGB formats
>>        */
>>       if (conn->ycbcr_420_allowed &&
>> @@ -2698,14 +2698,14 @@ static u32
>> *dw_hdmi_bridge_atomic_get_output_bus_fmts(struct drm_bridge *bridge,
>>           }
>>       }
>>   +    /* Default 8bit RGB fallback */
>> +    output_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24;
> 
> Why did you move this ? the following comment mentions it

Before d3d6b1bf85ae ("drm: bridge: dw_hdmi: fix preference of RGB modes
over YUV420"), this was the last format added to the list.  Hence I
interpreted the comment below as referring to this particular last entry
as a fallback, which is not the case anymore.

If you still prefer to keep the original comment, then maybe we should
simply drop the "Default 8bit RGB fallback" one, as it's pretty
redundant now.

Thanks,
Cristian

>> +
>>       /*
>>        * Order bus formats from 16bit to 8bit and from YUV422 to RGB
>> -     * if supported. In any case the default RGB888 format is added
>> +     * if supported.
>>        */
>>   -    /* Default 8bit RGB fallback */
>> -    output_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24;
>> -
>>       if (max_bpc >= 16 && info->bpc == 16) {
>>           if (info->color_formats & DRM_COLOR_FORMAT_YCBCR444)
>>               output_fmts[i++] = MEDIA_BUS_FMT_YUV16_1X48;
>>
>> ---
>> base-commit: f486c8aa16b8172f63bddc70116a0c897a7f3f02
>> change-id: 20241130-dw-hdmi-bus-fmt-order-7f6db5db2f0a
>>
>

Re: [PATCH] drm/bridge: dw-hdmi: Sync comments with actual bus formats order

Posted by Cristian Ciocaltea 3 days, 8 hours ago

Hi Neil,

On 12/2/24 7:32 PM, Cristian Ciocaltea wrote:
> On 12/2/24 5:45 PM, Neil Armstrong wrote:
>> Hi,
>>
>> On 30/11/2024 00:04, Cristian Ciocaltea wrote:
>>> Commit d3d6b1bf85ae ("drm: bridge: dw_hdmi: fix preference of RGB modes
>>> over YUV420") changed the order of the output bus formats, but missed to
>>> update accordingly the affected comment blocks related to
>>> dw_hdmi_bridge_atomic_get_output_bus_fmts().
>>>
>>> Fix the misleading comments and a context related typo.
>>>
>>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
>>> ---
>>>   drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 12 ++++++------
>>>   1 file changed, 6 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/
>>> drm/bridge/synopsys/dw-hdmi.c
>>> index
>>> 996733ed2c004c83a989cb8da54d8b630d9f2c02..d76aede757175d7ad5873c5d7623abf2d12da73c 100644
>>> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>>> @@ -2621,6 +2621,7 @@ static int dw_hdmi_connector_create(struct
>>> dw_hdmi *hdmi)
>>>    * - MEDIA_BUS_FMT_UYYVYY12_0_5X36,
>>>    * - MEDIA_BUS_FMT_UYYVYY10_0_5X30,
>>>    * - MEDIA_BUS_FMT_UYYVYY8_0_5X24,
>>> + * - MEDIA_BUS_FMT_RGB888_1X24,
>>>    * - MEDIA_BUS_FMT_YUV16_1X48,
>>>    * - MEDIA_BUS_FMT_RGB161616_1X48,
>>>    * - MEDIA_BUS_FMT_UYVY12_1X24,
>>> @@ -2631,7 +2632,6 @@ static int dw_hdmi_connector_create(struct
>>> dw_hdmi *hdmi)
>>>    * - MEDIA_BUS_FMT_RGB101010_1X30,
>>>    * - MEDIA_BUS_FMT_UYVY8_1X16,
>>>    * - MEDIA_BUS_FMT_YUV8_1X24,
>>> - * - MEDIA_BUS_FMT_RGB888_1X24,
>>>    */
>>>     /* Can return a maximum of 11 possible output formats for a mode/
>>> connector */
>>> @@ -2669,7 +2669,7 @@ static u32
>>> *dw_hdmi_bridge_atomic_get_output_bus_fmts(struct drm_bridge *bridge,
>>>       }
>>>         /*
>>> -     * If the current mode enforces 4:2:0, force the output but format
>>> +     * If the current mode enforces 4:2:0, force the output bus format
>>>        * to 4:2:0 and do not add the YUV422/444/RGB formats
>>>        */
>>>       if (conn->ycbcr_420_allowed &&
>>> @@ -2698,14 +2698,14 @@ static u32
>>> *dw_hdmi_bridge_atomic_get_output_bus_fmts(struct drm_bridge *bridge,
>>>           }
>>>       }
>>>   +    /* Default 8bit RGB fallback */
>>> +    output_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24;
>>
>> Why did you move this ? the following comment mentions it
> 
> Before d3d6b1bf85ae ("drm: bridge: dw_hdmi: fix preference of RGB modes
> over YUV420"), this was the last format added to the list.  Hence I
> interpreted the comment below as referring to this particular last entry
> as a fallback, which is not the case anymore.
> 
> If you still prefer to keep the original comment, then maybe we should
> simply drop the "Default 8bit RGB fallback" one, as it's pretty
> redundant now.

Please let me know what is your preference here and I'll send a new
revision if necessary.

Thanks,
Cristian

>>> +
>>>       /*
>>>        * Order bus formats from 16bit to 8bit and from YUV422 to RGB
>>> -     * if supported. In any case the default RGB888 format is added
>>> +     * if supported.
>>>        */
>>>   -    /* Default 8bit RGB fallback */
>>> -    output_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24;
>>> -
>>>       if (max_bpc >= 16 && info->bpc == 16) {
>>>           if (info->color_formats & DRM_COLOR_FORMAT_YCBCR444)
>>>               output_fmts[i++] = MEDIA_BUS_FMT_YUV16_1X48;
>>>
>>> ---
>>> base-commit: f486c8aa16b8172f63bddc70116a0c897a7f3f02
>>> change-id: 20241130-dw-hdmi-bus-fmt-order-7f6db5db2f0a
>>>
>>
>