On 07/06/2019 15:38, Laurent Pinchart wrote:
> Hi Neil,
>
> Thank you for the patch.
>
> On Mon, May 20, 2019 at 03:37:50PM +0200, Neil Armstrong wrote:
>> This patch adds a new format_set() callback to the bridge ops permitting
>> the encoder to specify the new input format and encoding.
>>
>> This allows supporting the very specific HDMI2.0 YUV420 output mode
>> when the bridge cannot convert from RGB or YUV444 to YUV420.
>>
>> In this case, the encode must downsample before the bridge and must
>> specify the bridge the new input bus format differs.
>>
>> This will also help supporting the YUV420 mode where the bridge cannot
>> downsample, and also support 10bit, 12bit and 16bit output modes
>> when the bridge cannot convert between different bit depths.
>>
>> Signed-off-by: Neil Armstrong <narmstrong@xxxxxxxxxxxx>
>> ---
>> drivers/gpu/drm/drm_bridge.c | 35 +++++++++++++++++++++++++++++++++++
>> include/drm/drm_bridge.h | 19 +++++++++++++++++++
>> 2 files changed, 54 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
>> index 138b2711d389..33be74a977f7 100644
>> --- a/drivers/gpu/drm/drm_bridge.c
>> +++ b/drivers/gpu/drm/drm_bridge.c
>> @@ -307,6 +307,41 @@ void drm_bridge_mode_set(struct drm_bridge *bridge,
>> }
>> EXPORT_SYMBOL(drm_bridge_mode_set);
>>
>> +/**
>> + * drm_bridge_format_set - setup with proposed input format and encoding for
>> + * all bridges in the encoder chain
>> + * @bridge: bridge control structure
>> + * @input_bus_format: proposed input bus format for the bridge
>> + * @input_encoding: proposed input encoding for this bridge
>> + *
>> + * Calls &drm_bridge_funcs.format_set op for all the bridges in the
>> + * encoder chain, starting from the first bridge to the last.
>> + *
>> + * Note: the bridge passed should be the one closest to the encoder
>> + *
>> + * RETURNS:
>> + * true on success, false if one of the bridge cannot handle the format
>
> I would return an int to propagate the failure reason upstream. It will
> reach the commit tail handler in any case, so will be dropped there, but
> could help debugging issues if we print it in the right place.
Indeed.
>
>> + */
>> +bool drm_bridge_format_set(struct drm_bridge *bridge,
>> + const u32 input_bus_format,
>> + const u32 input_encoding)
>
> You don't need a const here.
Noted
>
>> +{
>> + bool ret = true;
>> +
>> + if (!bridge)
>> + return true;
>> +
>> + if (bridge->funcs->format_set)
>> + ret = bridge->funcs->format_set(bridge, input_bus_format,
>> + input_encoding);
>> + if (!ret)
>> + return ret;
>> +
>> + return drm_bridge_format_set(bridge->next, input_bus_format,
>> + input_encoding);
>
> I don't think this will scale. It's not that uncommon for bridges to
> change the format (most likely converting from YUV to RGB or the other
> way around, or reducing the number of bits per sample) and the encoding.
> We thus can't propagate it from bridge to bridge and expect that to
> work.
>
> At the very least, the bridge should report its output bus format and
> encoding, to be applied to the next bridge, but this won't allow
> checking if the configuration can be applied ahead of time, resulting in
> possible failures of a commit tail handler. I wonder if this wouldn't be
> a good time to introduce bridge states...
Ok for chaining the format_set, I thought about it when writing it.
And what if a added a second call drm_bridge_format_check() to check
if a future format_set chaining would work ? then we could either do the
format_set, try another or fallback to a known default format setup.
Would that be sufficient ?
Bridge states would be interesting, but it's really out of the scope of
our current needs in term of bridge changes.
IMHO we can start with format_check/format_set and switch to bridges states
when we have a sufficient use case needing a finer handling of states.
Neil
>
>> +}
>> +EXPORT_SYMBOL(drm_bridge_format_set);
>> +
>> /**
>> * drm_bridge_pre_enable - prepares for enabling all
>> * bridges in the encoder chain
>> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
>> index d4428913a4e1..7a79e61b7825 100644
>> --- a/include/drm/drm_bridge.h
>> +++ b/include/drm/drm_bridge.h
>> @@ -198,6 +198,22 @@ struct drm_bridge_funcs {
>> void (*mode_set)(struct drm_bridge *bridge,
>> const struct drm_display_mode *mode,
>> const struct drm_display_mode *adjusted_mode);
>> +
>> + /**
>> + * @format_set:
>> + *
>> + * This callback should configure the bridge for the given input bus
>> + * format and encoding. It is called after the @format_set callback
>> + * for the preceding element in the display pipeline has been called
>> + * already. If the bridge is the first element then this would be
>> + * &drm_encoder_helper_funcs.format_set. The display pipe (i.e.
>> + * clocks and timing signals) is off when this function is called.
>> + *
>> + * @returns: true in success, false is a bridge refuses the format
>> + */
>> + bool (*format_set)(struct drm_bridge *bridge,
>> + const u32 input_bus_format,
>> + const u32 input_encoding);
>> /**
>> * @pre_enable:
>> *
>> @@ -311,6 +327,9 @@ void drm_bridge_post_disable(struct drm_bridge *bridge);
>> void drm_bridge_mode_set(struct drm_bridge *bridge,
>> const struct drm_display_mode *mode,
>> const struct drm_display_mode *adjusted_mode);
>> +bool drm_bridge_format_set(struct drm_bridge *bridge,
>> + const u32 input_bus_format,
>> + const u32 input_encoding);
>> void drm_bridge_pre_enable(struct drm_bridge *bridge);
>> void drm_bridge_enable(struct drm_bridge *bridge);
>>
>
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
