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

[Cristian Ciocaltea <cristian.ciocaltea@collabora.com>]

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
>>
>