Hi,
On 04/09/2015 12:54 PM, Jani Nikula wrote:
> On Thu, 09 Apr 2015, Archit Taneja <architt at codeaurora.org> wrote:
>> Allow drm_bridge objects to link to each other in order to form an encoder
>> chain. The requirement for creating a chain of bridges comes because the
>> MSM drm driver uses up its encoder and bridge objects for blocks within
>> the SoC itself. There isn't anything left to use if the SoC display output
>> is connected to an external encoder IC. Having an additional bridge
>> connected to the existing bridge helps here. In general, it is possible for
>> platforms to have multiple devices between the encoder and the
>> connector/panel that require some sort of configuration.
>>
>> We create drm bridge helper functions corresponding to each op in
>> 'drm_bridge_funcs'. These helpers call the corresponding 'drm_bridge_funcs'
>> op for the entire chain of bridges. These helpers are used internally by
>> drm_atomic_helper.c and drm_crtc_helper.c.
>>
>> The drm_bridge_enable/pre_enable helpers execute enable/pre_enable ops of the
>> bridge closet to the encoder, and proceed until the last bridge in the chain
>> is enabled. The same holds for drm_bridge_mode_set/mode_fixup helpers. The
>> drm_bridge_disable/post_disable helpers disable the last bridge in the chain
>> first, and proceed until the first bridge in the chain is disabled.
>>
>> drm_bridge_attach() remains the same. As before, the driver calling this
>> function should make sure it has set the links correctly. The order in which
>> the bridges are connected to each other determines the order in which the
>> calls are made. One requirement is that every bridge in the chain should
>> point
>> the parent encoder object. This is required since bridge drivers expect a
>> valid encoder pointer in drm_bridge. For example, consider a chain where an
>> encoder's output is connected to bridge1, and bridge1's output is connected
>> to
>> bridge2:
>>
>> /* Like before, attach bridge to an encoder */
>> bridge1->encoder = encoder;
>> ret = drm_bridge_attach(dev, bridge1);
>> ..
>>
>> /*
>> * set the first bridge's 'next' bridge to bridge2, set its encoder
>> * as bridge1's encoder
>> */
>> bridge1->next = bridge2
>> bridge2->encoder = bridge1->encoder;
>> ret = drm_bridge_attach(dev, bridge2);
>>
>> ...
>> ...
>>
>> This method of bridge chaining isn't intrusive and existing drivers that use
>> drm_bridge will behave the same way as before. The bridge helpers also cleans
>> up the atomic and crtc helper files a bit.
>>
>> Signed-off-by: Archit Taneja <architt at codeaurora.org>
>> ---
>> drivers/gpu/drm/drm_atomic_helper.c | 29 ++++++----------
>> drivers/gpu/drm/drm_bridge.c | 68
>> +++++++++++++++++++++++++++++++++++++
>> drivers/gpu/drm/drm_crtc_helper.c | 54 +++++++++++------------------
>> include/drm/drm_crtc.h | 14 ++++++++
>> 4 files changed, 112 insertions(+), 53 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
>> b/drivers/gpu/drm/drm_atomic_helper.c
>> index 7e3a52b..0b4574e 100644
>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>> @@ -287,14 +287,11 @@ mode_fixup(struct drm_atomic_state *state)
>> encoder = conn_state->best_encoder;
>> funcs = encoder->helper_private;
>>
>> - if (encoder->bridge && encoder->bridge->funcs->mode_fixup) {
>> - ret = encoder->bridge->funcs->mode_fixup(
>> - encoder->bridge, &crtc_state->mode,
>> - &crtc_state->adjusted_mode);
>> - if (!ret) {
>> - DRM_DEBUG_KMS("Bridge fixup failed\n");
>> - return -EINVAL;
>> - }
>> + ret = drm_bridge_mode_fixup(encoder->bridge, &crtc_state->mode,
>> + &crtc_state->adjusted_mode);
>> + if (!ret) {
>> + DRM_DEBUG_KMS("Bridge fixup failed\n");
>> + return -EINVAL;
>> }
>>
>> if (funcs->atomic_check) {
>> @@ -607,8 +604,7 @@ disable_outputs(struct drm_device *dev, struct
>> drm_atomic_state *old_state)
>> * Each encoder has at most one connector (since we always steal
>> * it away), so we won't call call disable hooks twice.
>> */
>> - if (encoder->bridge)
>> - encoder->bridge->funcs->disable(encoder->bridge);
>> + drm_bridge_disable(encoder->bridge);
>>
>> /* Right function depends upon target state. */
>> if (connector->state->crtc && funcs->prepare)
>> @@ -618,8 +614,7 @@ disable_outputs(struct drm_device *dev, struct
>> drm_atomic_state *old_state)
>> else
>> funcs->dpms(encoder, DRM_MODE_DPMS_OFF);
>>
>> - if (encoder->bridge)
>> - encoder->bridge->funcs->post_disable(encoder->bridge);
>> + drm_bridge_post_disable(encoder->bridge);
>> }
>>
>> for (i = 0; i < ncrtcs; i++) {
>> @@ -761,9 +756,7 @@ crtc_set_mode(struct drm_device *dev, struct
>> drm_atomic_state *old_state)
>> */
>> funcs->mode_set(encoder, mode, adjusted_mode);
>>
>> - if (encoder->bridge && encoder->bridge->funcs->mode_set)
>> - encoder->bridge->funcs->mode_set(encoder->bridge,
>> - mode, adjusted_mode);
>> + drm_bridge_mode_set(encoder->bridge, mode, adjusted_mode);
>> }
>> }
>>
>> @@ -849,16 +842,14 @@ void drm_atomic_helper_commit_post_planes(struct
>> drm_device *dev,
>> * Each encoder has at most one connector (since we always steal
>> * it away), so we won't call call enable hooks twice.
>> */
>> - if (encoder->bridge)
>> - encoder->bridge->funcs->pre_enable(encoder->bridge);
>> + drm_bridge_pre_enable(encoder->bridge);
>>
>> if (funcs->enable)
>> funcs->enable(encoder);
>> else
>> funcs->commit(encoder);
>>
>> - if (encoder->bridge)
>> - encoder->bridge->funcs->enable(encoder->bridge);
>> + drm_bridge_enable(encoder->bridge);
>> }
>> }
>> EXPORT_SYMBOL(drm_atomic_helper_commit_post_planes);
>> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
>> index d1187e5..b52c387 100644
>> --- a/drivers/gpu/drm/drm_bridge.c
>> +++ b/drivers/gpu/drm/drm_bridge.c
>> @@ -66,6 +66,74 @@ extern int drm_bridge_attach(struct drm_device *dev,
>> struct drm_bridge *bridge)
>> }
>> EXPORT_SYMBOL(drm_bridge_attach);
>>
>> +bool drm_bridge_mode_fixup(struct drm_bridge *bridge,
>> + const struct drm_display_mode *mode,
>> + struct drm_display_mode *adjusted_mode)
>> +{
>> + bool ret = true;
>> +
>> + if (!bridge)
>> + return true;
>> +
>> + if (bridge->funcs->mode_fixup)
>> + ret = bridge->funcs->mode_fixup(bridge, mode, adjusted_mode);
>> +
>> + return ret & drm_bridge_mode_fixup(bridge->next, mode, adjusted_mode);
>
> Is that an intentional bitwise rather than logical AND? If it is, please
> don't combine this into the return statement.
This looks like a mistake from my end. I'll fix this.
>
> How about EXPORT_SYMBOLs for the new functions?
I'll add this.
>
> Other than these, the patch LGTM.
Thanks for the review!
Archit
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
