Re: [RFC PATCH v2 5/5] drm/msm/dp: remove extra wrappers and public functions

[Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx>]

On 3/16/2022 9:31 AM, Sankeerth Billakanti (QUIC) wrote:
> > Subject: [RFC PATCH v2 5/5] drm/msm/dp: remove extra wrappers and
> > public functions
> > Date: Sat, 12 Feb 2022 01:40:06 +0300
> > From: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> > To: Bjorn Andersson <bjorn.andersson@xxxxxxxxxx>, Rob Clark
> > <robdclark@xxxxxxxxx>, Sean Paul <sean@xxxxxxxxxx>, Abhinav Kumar
> > <quic_abhinavk@xxxxxxxxxxx>, Kuogee Hsieh <quic_khsieh@xxxxxxxxxxx>
> > CC: Stephen Boyd <swboyd@xxxxxxxxxxxx>, David Airlie <airlied@xxxxxxxx>,
> > Daniel Vetter <daniel@xxxxxxxx>, linux-arm-msm@xxxxxxxxxxxxxxx, dri-
> > devel@xxxxxxxxxxxxxxxxxxxxx, freedreno@xxxxxxxxxxxxxxxxxxxxx
> >
> > dp_bridge's functions are thin wrappers around the msm_dp_display_*
> > family. Squash dp_bridge callbacks into respective msm_dp_display
> > functions, removing the latter functions from public space.
> >
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
>
>
> Tested-by: Sankeerth Billakanti <quic_sbillaka@xxxxxxxxxxx>
Reviewed-by: Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx>

> > ---
> >    drivers/gpu/drm/msm/dp/dp_display.c | 54 +++++++++++++++-------
> >    drivers/gpu/drm/msm/dp/dp_display.h |  1 -
> >    drivers/gpu/drm/msm/dp/dp_drm.c     | 72 ++---------------------------
> >    drivers/gpu/drm/msm/dp/dp_drm.h     | 22 ++++++++-
> >    drivers/gpu/drm/msm/msm_drv.h       | 31 -------------
> >    5 files changed, 60 insertions(+), 120 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> > b/drivers/gpu/drm/msm/dp/dp_display.c
> > index 59e5e5b8e5b4..a9b641a68bff 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_display.c
> > +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> > @@ -10,7 +10,6 @@
> >    #include <linux/component.h>
> >    #include <linux/of_irq.h>
> >    #include <linux/delay.h>
> > -#include <drm/drm_panel.h>
> >     #include "msm_drv.h"
> >    #include "msm_kms.h"
> > @@ -945,18 +944,36 @@ int dp_display_set_plugged_cb(struct msm_dp
> > *dp_display,
> >        return 0;
> >    }
> >    -int dp_display_validate_mode(struct msm_dp *dp, u32 mode_pclk_khz)
> > +/**
> > + * dp_bridge_mode_valid - callback to determine if specified mode is
> > +valid
> > + * @bridge: Pointer to drm bridge structure
> > + * @info: display info
> > + * @mode: Pointer to drm mode structure
> > + * Returns: Validity status for specified mode  */ enum drm_mode_status
> > +dp_bridge_mode_valid(struct drm_bridge *bridge,
> > +                                       const struct drm_display_info *info,
> > +                                       const struct drm_display_mode
> > *mode)
> >    {
> >        const u32 num_components = 3, default_bpp = 24;
> >        struct dp_display_private *dp_display;
> >        struct dp_link_info *link_info;
> >        u32 mode_rate_khz = 0, supported_rate_khz = 0, mode_bpp = 0;
> > +     struct msm_dp *dp;
> > +     int mode_pclk_khz = mode->clock;
> > +
> > +     dp = to_dp_bridge(bridge)->dp_display;
> >        if (!dp || !mode_pclk_khz || !dp->connector) {
> >                DRM_ERROR("invalid params\n");
> >                return -EINVAL;
> >        }
> >    +   if ((dp->max_pclk_khz <= 0) ||
> > +                     (dp->max_pclk_khz > DP_MAX_PIXEL_CLK_KHZ) ||
> > +                     (mode->clock > dp->max_pclk_khz))
> > +             return MODE_BAD;
> > +
> >        dp_display = container_of(dp, struct dp_display_private,
> > dp_display);
> >        link_info = &dp_display->panel->link_info;
> >    @@ -1501,7 +1518,7 @@ int msm_dp_modeset_init(struct msm_dp
> > *dp_display, struct drm_device *dev,
> >        dp_display->encoder = encoder;
> >    -   dp_display->bridge = msm_dp_bridge_init(dp_display, dev,
> > encoder);
> > +     dp_display->bridge = dp_bridge_init(dp_display, dev, encoder);
> >        if (IS_ERR(dp_display->bridge)) {
> >                ret = PTR_ERR(dp_display->bridge);
> >                DRM_DEV_ERROR(dev->dev,
> > @@ -1528,8 +1545,10 @@ int msm_dp_modeset_init(struct msm_dp
> > *dp_display, struct drm_device *dev,
> >        return 0;
> >    }
> >    -int msm_dp_display_enable(struct msm_dp *dp, struct drm_encoder
> > *encoder)
> > +void dp_bridge_enable(struct drm_bridge *drm_bridge)
> >    {
> > +     struct msm_dp_bridge *dp_bridge = to_dp_bridge(drm_bridge);
> > +     struct msm_dp *dp = dp_bridge->dp_display;
> >        int rc = 0;
> >        struct dp_display_private *dp_display;
> >        u32 state;
> > @@ -1537,7 +1556,7 @@ int msm_dp_display_enable(struct msm_dp *dp,
> > struct drm_encoder *encoder)
> >        dp_display = container_of(dp, struct dp_display_private,
> > dp_display);
> >        if (!dp_display->dp_mode.drm_mode.clock) {
> >                DRM_ERROR("invalid params\n");
> > -             return -EINVAL;
> > +             return;
> >        }
> >        mutex_lock(&dp_display->event_mutex);
> > @@ -1549,14 +1568,14 @@ int msm_dp_display_enable(struct msm_dp *dp,
> > struct drm_encoder *encoder)
> >        if (rc) {
> >                DRM_ERROR("Failed to perform a mode set, rc=%d\n", rc);
> >                mutex_unlock(&dp_display->event_mutex);
> > -             return rc;
> > +             return;
> >        }
> >        rc = dp_display_prepare(dp);
> >        if (rc) {
> >                DRM_ERROR("DP display prepare failed, rc=%d\n", rc);
> >                mutex_unlock(&dp_display->event_mutex);
> > -             return rc;
> > +             return;
> >        }
> >        state =  dp_display->hpd_state;
> > @@ -1581,23 +1600,23 @@ int msm_dp_display_enable(struct msm_dp *dp,
> > struct drm_encoder *encoder)
> >        dp_display->hpd_state = ST_CONNECTED;
> >        mutex_unlock(&dp_display->event_mutex);
> > -
> > -     return rc;
> >    }
> >    -int msm_dp_display_pre_disable(struct msm_dp *dp, struct drm_encoder
> > *encoder)
> > +void dp_bridge_disable(struct drm_bridge *drm_bridge)
> >    {
> > +     struct msm_dp_bridge *dp_bridge = to_dp_bridge(drm_bridge);
> > +     struct msm_dp *dp = dp_bridge->dp_display;
> >        struct dp_display_private *dp_display;
> >        dp_display = container_of(dp, struct dp_display_private,
> > dp_display);
> >        dp_ctrl_push_idle(dp_display->ctrl);
> > -
> > -     return 0;
> >    }
> >    -int msm_dp_display_disable(struct msm_dp *dp, struct drm_encoder
> > *encoder)
> > +void dp_bridge_post_disable(struct drm_bridge *drm_bridge)
> >    {
> > +     struct msm_dp_bridge *dp_bridge = to_dp_bridge(drm_bridge);
> > +     struct msm_dp *dp = dp_bridge->dp_display;
> >        int rc = 0;
> >        u32 state;
> >        struct dp_display_private *dp_display; @@ -1624,13 +1643,14 @@
> > int msm_dp_display_disable(struct msm_dp *dp, struct drm_encoder
> > *encoder)
> >        }
> >        mutex_unlock(&dp_display->event_mutex);
> > -     return rc;
> >    }
> >    -void msm_dp_display_mode_set(struct msm_dp *dp, struct drm_encoder
> > *encoder,
> > -                             const struct drm_display_mode *mode,
> > -                             const struct drm_display_mode
> > *adjusted_mode)
> > +void dp_bridge_mode_set(struct drm_bridge *drm_bridge,
> > +                     const struct drm_display_mode *mode,
> > +                     const struct drm_display_mode *adjusted_mode)
> >    {
> > +     struct msm_dp_bridge *dp_bridge = to_dp_bridge(drm_bridge);
> > +     struct msm_dp *dp = dp_bridge->dp_display;
> >        struct dp_display_private *dp_display;
> >        dp_display = container_of(dp, struct dp_display_private,
> > dp_display); diff --git a/drivers/gpu/drm/msm/dp/dp_display.h
> > b/drivers/gpu/drm/msm/dp/dp_display.h
> > index 7af2b186d2d9..49a1d89681ac 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_display.h
> > +++ b/drivers/gpu/drm/msm/dp/dp_display.h
> > @@ -32,7 +32,6 @@ struct msm_dp {
> >     int dp_display_set_plugged_cb(struct msm_dp *dp_display,
> >                hdmi_codec_plugged_cb fn, struct device *codec_dev); -int
> > dp_display_validate_mode(struct msm_dp *dp_display, u32
> > mode_pclk_khz);
> >    int dp_display_get_modes(struct msm_dp *dp_display,
> >                struct dp_display_mode *dp_mode);
> >    int dp_display_request_irq(struct msm_dp *dp_display); diff --git
> > a/drivers/gpu/drm/msm/dp/dp_drm.c
> > b/drivers/gpu/drm/msm/dp/dp_drm.c index 57800b865fe6..7ce1aca48687
> > 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_drm.c
> > +++ b/drivers/gpu/drm/msm/dp/dp_drm.c
> > @@ -13,14 +13,6 @@
> >    #include "msm_kms.h"
> >    #include "dp_drm.h"
> >    -
> > -struct msm_dp_bridge {
> > -     struct drm_bridge bridge;
> > -     struct msm_dp *dp_display;
> > -};
> > -
> > -#define to_dp_display(x)     container_of((x), struct msm_dp_bridge,
> > bridge)
> > -
> >    /**
> >     * dp_bridge_detect - callback to determine if connector is connected
> >     * @bridge: Pointer to drm bridge structure @@ -30,7 +22,7 @@ static enum
> > drm_connector_status dp_bridge_detect(struct drm_bridge *bridge)
> >    {
> >        struct msm_dp *dp;
> >    -   dp = to_dp_display(bridge)->dp_display;
> > +     dp = to_dp_bridge(bridge)->dp_display;
> >        DRM_DEBUG_DP("is_connected = %s\n",
> >                (dp->is_connected) ? "true" : "false"); @@ -55,7 +47,7 @@
> > static int dp_bridge_get_modes(struct drm_bridge *bridge, struct
> > drm_connector *
> >        if (!connector)
> >                return 0;
> >    -   dp = to_dp_display(bridge)->dp_display;
> > +     dp = to_dp_bridge(bridge)->dp_display;
> >        dp_mode = kzalloc(sizeof(*dp_mode),  GFP_KERNEL);
> >        if (!dp_mode)
> > @@ -95,64 +87,6 @@ static int dp_bridge_get_modes(struct drm_bridge
> > *bridge, struct drm_connector *
> >        return rc;
> >    }
> >    -/**
> > - * dp_bridge_mode_valid - callback to determine if specified mode is valid
> > - * @bridge: Pointer to drm bridge structure
> > - * @info: display info
> > - * @mode: Pointer to drm mode structure
> > - * Returns: Validity status for specified mode
> > - */
> > -static enum drm_mode_status dp_bridge_mode_valid(
> > -             struct drm_bridge *bridge,
> > -             const struct drm_display_info *info,
> > -             const struct drm_display_mode *mode)
> > -{
> > -     struct msm_dp *dp_disp;
> > -
> > -     dp_disp = to_dp_display(bridge)->dp_display;
> > -
> > -     if ((dp_disp->max_pclk_khz <= 0) ||
> > -                     (dp_disp->max_pclk_khz >
> > DP_MAX_PIXEL_CLK_KHZ) ||
> > -                     (mode->clock > dp_disp->max_pclk_khz))
> > -             return MODE_BAD;
> > -
> > -     return dp_display_validate_mode(dp_disp, mode->clock);
> > -}
> > -
> > -static void dp_bridge_mode_set(struct drm_bridge *drm_bridge,
> > -                             const struct drm_display_mode *mode,
> > -                             const struct drm_display_mode
> > *adjusted_mode)
> > -{
> > -     struct msm_dp_bridge *dp_bridge = to_dp_display(drm_bridge);
> > -     struct msm_dp *dp_display = dp_bridge->dp_display;
> > -
> > -     msm_dp_display_mode_set(dp_display, drm_bridge->encoder,
> > mode,
> > adjusted_mode);
> > -}
> > -
> > -static void dp_bridge_enable(struct drm_bridge *drm_bridge) -{
> > -     struct msm_dp_bridge *dp_bridge = to_dp_display(drm_bridge);
> > -     struct msm_dp *dp_display = dp_bridge->dp_display;
> > -
> > -     msm_dp_display_enable(dp_display, drm_bridge->encoder);
> > -}
> > -
> > -static void dp_bridge_disable(struct drm_bridge *drm_bridge) -{
> > -     struct msm_dp_bridge *dp_bridge = to_dp_display(drm_bridge);
> > -     struct msm_dp *dp_display = dp_bridge->dp_display;
> > -
> > -     msm_dp_display_pre_disable(dp_display, drm_bridge->encoder);
> > -}
> > -
> > -static void dp_bridge_post_disable(struct drm_bridge *drm_bridge) -{
> > -     struct msm_dp_bridge *dp_bridge = to_dp_display(drm_bridge);
> > -     struct msm_dp *dp_display = dp_bridge->dp_display;
> > -
> > -     msm_dp_display_disable(dp_display, drm_bridge->encoder);
> > -}
> > -
> >    static const struct drm_bridge_funcs dp_bridge_ops = {
> >        .enable       = dp_bridge_enable,
> >        .disable      = dp_bridge_disable,
> > @@ -163,7 +97,7 @@ static const struct drm_bridge_funcs dp_bridge_ops =
> > {
> >        .detect       = dp_bridge_detect,
> >    };
> >    -struct drm_bridge *msm_dp_bridge_init(struct msm_dp *dp_display,
> > struct drm_device *dev,
> > +struct drm_bridge *dp_bridge_init(struct msm_dp *dp_display, struct
> > drm_device *dev,
> >                        struct drm_encoder *encoder)
> >    {
> >        int rc;
> > diff --git a/drivers/gpu/drm/msm/dp/dp_drm.h
> > b/drivers/gpu/drm/msm/dp/dp_drm.h index c27bfceefdf0..f4b1ed1e24f7
> > 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_drm.h
> > +++ b/drivers/gpu/drm/msm/dp/dp_drm.h
> > @@ -7,12 +7,30 @@
> >    #define _DP_DRM_H_
> >     #include <linux/types.h>
> > -#include <drm/drm_crtc.h>
> > -#include <drm/drm_crtc_helper.h>
> > +#include <drm/drm_bridge.h>
> >     #include "msm_drv.h"
> >    #include "dp_display.h"
> >    +struct msm_dp_bridge {
> > +     struct drm_bridge bridge;
> > +     struct msm_dp *dp_display;
> > +};
> > +
> > +#define to_dp_bridge(x)     container_of((x), struct msm_dp_bridge,
> > bridge)
> > +
> >    struct drm_connector *dp_drm_connector_init(struct msm_dp
> > *dp_display);
> > +struct drm_bridge *dp_bridge_init(struct msm_dp *dp_display, struct
> > drm_device *dev,
> > +                     struct drm_encoder *encoder);
> > +
> > +void dp_bridge_enable(struct drm_bridge *drm_bridge); void
> > +dp_bridge_disable(struct drm_bridge *drm_bridge); void
> > +dp_bridge_post_disable(struct drm_bridge *drm_bridge); enum
> > +drm_mode_status dp_bridge_mode_valid(struct drm_bridge *bridge,
> > +                                       const struct drm_display_info *info,
> > +                                       const struct drm_display_mode
> > *mode); void
> > +dp_bridge_mode_set(struct drm_bridge *drm_bridge,
> > +                     const struct drm_display_mode *mode,
> > +                     const struct drm_display_mode *adjusted_mode);
> >     #endif /* _DP_DRM_H_ */
> > diff --git a/drivers/gpu/drm/msm/msm_drv.h
> > b/drivers/gpu/drm/msm/msm_drv.h index d7574e6bd4e4..b698a3be6072
> > 100644
> > --- a/drivers/gpu/drm/msm/msm_drv.h
> > +++ b/drivers/gpu/drm/msm/msm_drv.h
> > @@ -385,16 +385,6 @@ int __init msm_dp_register(void);
> >    void __exit msm_dp_unregister(void);
> >    int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device
> > *dev,
> >                         struct drm_encoder *encoder);
> > -int msm_dp_display_enable(struct msm_dp *dp, struct drm_encoder
> > *encoder); -int msm_dp_display_disable(struct msm_dp *dp, struct
> > drm_encoder *encoder); -int msm_dp_display_pre_disable(struct msm_dp
> > *dp, struct drm_encoder *encoder); -void
> > msm_dp_display_mode_set(struct msm_dp *dp, struct drm_encoder
> > *encoder,
> > -                             const struct drm_display_mode *mode,
> > -                             const struct drm_display_mode
> > *adjusted_mode);
> > -
> > -struct drm_bridge *msm_dp_bridge_init(struct msm_dp *dp_display,
> > -                                     struct drm_device *dev,
> > -                                     struct drm_encoder *encoder);
> >    void msm_dp_irq_postinstall(struct msm_dp *dp_display);
> >    void msm_dp_snapshot(struct msm_disp_state *disp_state, struct
> > msm_dp *dp_display);
> >    @@ -414,27 +404,6 @@ static inline int msm_dp_modeset_init(struct
> > msm_dp *dp_display,
> >    {
> >        return -EINVAL;
> >    }
> > -static inline int msm_dp_display_enable(struct msm_dp *dp,
> > -                                     struct drm_encoder *encoder)
> > -{
> > -     return -EINVAL;
> > -}
> > -static inline int msm_dp_display_disable(struct msm_dp *dp,
> > -                                     struct drm_encoder *encoder)
> > -{
> > -     return -EINVAL;
> > -}
> > -static inline int msm_dp_display_pre_disable(struct msm_dp *dp,
> > -                                     struct drm_encoder *encoder)
> > -{
> > -     return -EINVAL;
> > -}
> > -static inline void msm_dp_display_mode_set(struct msm_dp *dp,
> > -                             struct drm_encoder *encoder,
> > -                             const struct drm_display_mode *mode,
> > -                             const struct drm_display_mode
> > *adjusted_mode)
> > -{
> > -}
> >     static inline void msm_dp_irq_postinstall(struct msm_dp *dp_display)
> >    {
> > --
> > 2.34.1