RE: [PATCH 1/2] drm/amd/display: Convert macros to functions in amdgpu_display.c & amdgpu_display.h

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



[Public]

Instead of converting all to functions, below improvement may be a bit more simple. Can you please double check?

-#define amdgpu_display_vblank_get_counter(adev, crtc) (adev)->mode_info.funcs->vblank_get_counter((adev), (crtc))
+#define amdgpu_display_vblank_get_counter(_adev, crtc) \
+       ({typeof(_adev) (adev) = (_adev); \
+       (adev)->mode_info.funcs->vblank_get_counter((adev), (crtc));})

Regards,
Guchun

> -----Original Message-----
> From: amd-gfx <amd-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx> On Behalf Of
> Srinivasan Shanmugam
> Sent: Wednesday, July 19, 2023 1:20 PM
> To: Koenig, Christian <Christian.Koenig@xxxxxxx>; Deucher, Alexander
> <Alexander.Deucher@xxxxxxx>
> Cc: SHANMUGAM, SRINIVASAN <SRINIVASAN.SHANMUGAM@xxxxxxx>;
> amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> Subject: [PATCH 1/2] drm/amd/display: Convert macros to functions in
> amdgpu_display.c & amdgpu_display.h
>
> Convert macros to functions to fix the following & for better readability:
>
> drivers/gpu/drm/amd/amdgpu/amdgpu_display.h:26: Macro argument
> reuse 'adev' - possible side-effects?
> drivers/gpu/drm/amd/amdgpu/amdgpu_display.h:32: Macro argument
> reuse 'adev' - possible side-effects?
> drivers/gpu/drm/amd/amdgpu/amdgpu_display.h:34: Macro argument
> reuse 'adev' - possible side-effects?
> drivers/gpu/drm/amd/amdgpu/amdgpu_display.h:36: Macro argument
> reuse 'adev' - possible side-effects?
> drivers/gpu/drm/amd/amdgpu/amdgpu_display.h:38: Macro argument
> reuse 'adev' - possible side-effects?
> drivers/gpu/drm/amd/amdgpu/amdgpu_display.h:40: Macro argument
> reuse 'adev' - possible side-effects?
> drivers/gpu/drm/amd/amdgpu/amdgpu_display.h:42: Macro argument
> reuse 'adev' - possible side-effects?
> drivers/gpu/drm/amd/amdgpu/amdgpu_display.h:44: Macro argument
> reuse 'adev' - possible side-effects?
>
> And other warnings:
>
> WARNING: Block comments use * on subsequent lines
> WARNING: Block comments use a trailing */ on a separate line
> WARNING: suspect code indent for conditional statements (8, 12)
> WARNING: braces {} are not necessary for single statement blocks
>
> Cc: Christian König <christian.koenig@xxxxxxx>
> Cc: Alex Deucher <alexander.deucher@xxxxxxx>
> Signed-off-by: Srinivasan Shanmugam <srinivasan.shanmugam@xxxxxxx>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 118
> +++++++++++++++++---  drivers/gpu/drm/amd/amdgpu/amdgpu_display.h |
> 46 ++++++--
>  2 files changed, 136 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> index b702f499f5fb..6eea92cef97c 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> @@ -45,6 +45,82 @@
>  #include <drm/drm_modeset_helper.h>
>  #include <drm/drm_vblank.h>
>
> +u32 amdgpu_display_vblank_get_counter(struct amdgpu_device *adev, int
> +crtc) {
> +     return (adev)->mode_info.funcs->vblank_get_counter((adev),
> (crtc)); }
> +
> +void amdgpu_display_backlight_set_level(struct amdgpu_device *adev,
> +                                     struct amdgpu_encoder
> *amdgpu_encoder,
> +                                     u8 level)
> +{
> +     (adev)->mode_info.funcs->backlight_set_level((amdgpu_encoder),
> +(level)); }
> +
> +u8 amdgpu_display_backlight_get_level(struct amdgpu_device *adev,
> +                                   struct amdgpu_encoder
> *amdgpu_encoder) {
> +     return (adev)->mode_info.funcs-
> >backlight_get_level(amdgpu_encoder);
> +}
> +
> +bool amdgpu_display_hpd_sense(struct amdgpu_device *adev,
> +                           enum amdgpu_hpd_id hpd)
> +{
> +     return (adev)->mode_info.funcs->hpd_sense((adev), (hpd)); }
> +
> +void amdgpu_display_hpd_set_polarity(struct amdgpu_device *adev,
> +                                  enum amdgpu_hpd_id hpd)
> +{
> +     (adev)->mode_info.funcs->hpd_set_polarity((adev), (hpd)); }
> +
> +u32 amdgpu_display_hpd_get_gpio_reg(struct amdgpu_device *adev) {
> +     return (adev)->mode_info.funcs->hpd_get_gpio_reg(adev);
> +}
> +
> +void amdgpu_display_bandwidth_update(struct amdgpu_device *adev) {
> +     (adev)->mode_info.funcs->bandwidth_update(adev);
> +}
> +
> +void amdgpu_display_page_flip(struct amdgpu_device *adev,
> +                           int crtc_id, u64 crtc_base,
> +                           bool async)
> +{
> +     (adev)->mode_info.funcs->page_flip((adev), (crtc_id), (crtc_base),
> +(async)); }
> +
> +int amdgpu_display_page_flip_get_scanoutpos(struct amdgpu_device
> *adev, int crtc,
> +                                         u32 *vbl, u32 *pos)
> +{
> +     return (adev)->mode_info.funcs->page_flip_get_scanoutpos((adev),
> +(crtc), (vbl), (pos)); }
> +
> +void amdgpu_display_add_encoder(struct amdgpu_device *adev,
> +                             u32 encoder_enum,
> +                             u32 supported_device,
> +                             u16 caps)
> +{
> +     (adev)->mode_info.funcs->add_encoder((adev), (encoder_enum),
> +(supported_device), (caps)); }
> +
> +void amdgpu_display_add_connector(struct amdgpu_device *adev,
> +                               u32 connector_id,
> +                               u32 supported_device,
> +                               int connector_type,
> +                               struct amdgpu_i2c_bus_rec *i2c_bus,
> +                               u16 connector_object_id,
> +                               struct amdgpu_hpd *hpd,
> +                               struct amdgpu_router *router)
> +{
> +     (adev)->mode_info.funcs->add_connector((adev), (connector_id),
> +                                            (supported_device),
> (connector_type),
> +                                            (i2c_bus), (connector_object_id),
> +                                            (hpd), (router));
> +}
> +
>  /**
>   * amdgpu_display_hotplug_work_func - work handler for display hotplug
> event
>   *
> @@ -124,7 +200,7 @@ static void amdgpu_display_flip_work_func(struct
> work_struct *__work)
>
>       struct drm_crtc *crtc = &amdgpu_crtc->base;
>       unsigned long flags;
> -     unsigned i;
> +     unsigned int i;
>       int vpos, hpos;
>
>       for (i = 0; i < work->shared_count; ++i) @@ -201,7 +277,7 @@ int
> amdgpu_display_crtc_page_flip_target(struct drm_crtc *crtc,
>       u64 tiling_flags;
>       int i, r;
>
> -     work = kzalloc(sizeof *work, GFP_KERNEL);
> +     work = kzalloc(sizeof(*work), GFP_KERNEL);
>       if (work == NULL)
>               return -ENOMEM;
>
> @@ -332,13 +408,15 @@ int amdgpu_display_crtc_set_config(struct
> drm_mode_set *set,
>
>       adev = drm_to_adev(dev);
>       /* if we have active crtcs and we don't have a power ref,
> -        take the current one */
> +      * take the current one
> +      */
>       if (active && !adev->have_disp_power_ref) {
>               adev->have_disp_power_ref = true;
>               return ret;
>       }
>       /* if we have no active crtcs, then drop the power ref
> -        we got before */
> +      * we got before
> +      */
>       if (!active && adev->have_disp_power_ref) {
>               pm_runtime_put_autosuspend(dev->dev);
>               adev->have_disp_power_ref = false;
> @@ -507,11 +585,10 @@ bool amdgpu_display_ddc_probe(struct
> amdgpu_connector *amdgpu_connector,
>       if (amdgpu_connector->router.ddc_valid)
>               amdgpu_i2c_router_select_ddc_port(amdgpu_connector);
>
> -     if (use_aux) {
> +     if (use_aux)
>               ret = i2c_transfer(&amdgpu_connector->ddc_bus->aux.ddc,
> msgs, 2);
> -     } else {
> +     else
>               ret = i2c_transfer(&amdgpu_connector->ddc_bus->adapter,
> msgs, 2);
> -     }
>
>       if (ret != 2)
>               /* Couldn't find an accessible DDC on this connector */ @@ -
> 520,10 +597,12 @@ bool amdgpu_display_ddc_probe(struct
> amdgpu_connector *amdgpu_connector,
>        * EDID header starts with:
>        * 0x00,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0x00.
>        * Only the first 6 bytes must be valid as
> -      * drm_edid_block_valid() can fix the last 2 bytes */
> +      * drm_edid_block_valid() can fix the last 2 bytes
> +      */
>       if (drm_edid_header_is_valid(buf) < 6) {
>               /* Couldn't find an accessible EDID on this
> -              * connector */
> +              * connector
> +              */
>               return false;
>       }
>       return true;
> @@ -1216,8 +1295,10 @@ amdgpu_display_user_framebuffer_create(struct
> drm_device *dev,
>
>       obj = drm_gem_object_lookup(file_priv, mode_cmd->handles[0]);
>       if (obj ==  NULL) {
> -             drm_dbg_kms(dev, "No GEM object associated to handle
> 0x%08X, "
> -                         "can't create framebuffer\n", mode_cmd-
> >handles[0]);
> +             drm_dbg_kms(dev, "No GEM object associated to handle
> 0x%08X\n",
> +                         mode_cmd->handles[0]);
> +             drm_dbg_kms(dev, "  - Can't create framebuffer\n");
> +
>               return ERR_PTR(-ENOENT);
>       }
>
> @@ -1410,6 +1491,7 @@ bool
> amdgpu_display_crtc_scaling_mode_fixup(struct drm_crtc *crtc,
>       }
>       if (amdgpu_crtc->rmx_type != RMX_OFF) {
>               fixed20_12 a, b;
> +
>               a.full = dfixed_const(src_v);
>               b.full = dfixed_const(dst_v);
>               amdgpu_crtc->vsc.full = dfixed_div(a, b); @@ -1429,7
> +1511,7 @@ bool amdgpu_display_crtc_scaling_mode_fixup(struct drm_crtc
> *crtc,
>   *
>   * \param dev Device to query.
>   * \param pipe Crtc to query.
> - * \param flags Flags from caller (DRM_CALLED_FROM_VBLIRQ or 0).
> + * \param flags from caller (DRM_CALLED_FROM_VBLIRQ or 0).
>   *              For driver internal use only also supports these flags:
>   *
>   *              USE_REAL_VBLANKSTART to use the real start of vblank instead
> @@ -1504,8 +1586,8 @@ int amdgpu_display_get_crtc_scanoutpos(struct
> drm_device *dev,
>
>       /* Called from driver internal vblank counter query code? */
>       if (flags & GET_DISTANCE_TO_VBLANKSTART) {
> -         /* Caller wants distance from real vbl_start in *hpos */
> -         *hpos = *vpos - vbl_start;
> +             /* Caller wants distance from real vbl_start in *hpos */
> +             *hpos = *vpos - vbl_start;
>       }
>
>       /* Fudge vblank to start a few scanlines earlier to handle the @@ -
> 1527,7 +1609,7 @@ int amdgpu_display_get_crtc_scanoutpos(struct
> drm_device *dev,
>
>       /* In vblank? */
>       if (in_vbl)
> -         ret |= DRM_SCANOUTPOS_IN_VBLANK;
> +             ret |= DRM_SCANOUTPOS_IN_VBLANK;
>
>       /* Called from driver internal vblank counter query code? */
>       if (flags & GET_DISTANCE_TO_VBLANKSTART) { @@ -1635,6 +1717,7
> @@ int amdgpu_display_suspend_helper(struct amdgpu_device *adev)
>
>               if (amdgpu_crtc->cursor_bo && !adev-
> >enable_virtual_display) {
>                       struct amdgpu_bo *aobj =
> gem_to_amdgpu_bo(amdgpu_crtc->cursor_bo);
> +
>                       r = amdgpu_bo_reserve(aobj, true);
>                       if (r == 0) {
>                               amdgpu_bo_unpin(aobj);
> @@ -1642,9 +1725,9 @@ int amdgpu_display_suspend_helper(struct
> amdgpu_device *adev)
>                       }
>               }
>
> -             if (fb == NULL || fb->obj[0] == NULL) {
> +             if (!fb || !fb->obj[0])
>                       continue;
> -             }
> +
>               robj = gem_to_amdgpu_bo(fb->obj[0]);
>               if (!amdgpu_display_robj_is_fb(adev, robj)) {
>                       r = amdgpu_bo_reserve(robj, true);
> @@ -1671,6 +1754,7 @@ int amdgpu_display_resume_helper(struct
> amdgpu_device *adev)
>
>               if (amdgpu_crtc->cursor_bo && !adev-
> >enable_virtual_display) {
>                       struct amdgpu_bo *aobj =
> gem_to_amdgpu_bo(amdgpu_crtc->cursor_bo);
> +
>                       r = amdgpu_bo_reserve(aobj, true);
>                       if (r == 0) {
>                               r = amdgpu_bo_pin(aobj,
> AMDGPU_GEM_DOMAIN_VRAM); diff --git
> a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.h
> index 9d19940f73c8..4cefaec6a495 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.h
> @@ -23,17 +23,41 @@
>  #ifndef __AMDGPU_DISPLAY_H__
>  #define __AMDGPU_DISPLAY_H__
>
> -#define amdgpu_display_vblank_get_counter(adev, crtc) (adev)-
> >mode_info.funcs->vblank_get_counter((adev), (crtc)) -#define
> amdgpu_display_backlight_set_level(adev, e, l) (adev)->mode_info.funcs-
> >backlight_set_level((e), (l)) -#define
> amdgpu_display_backlight_get_level(adev, e) (adev)->mode_info.funcs-
> >backlight_get_level((e))
> -#define amdgpu_display_hpd_sense(adev, h) (adev)->mode_info.funcs-
> >hpd_sense((adev), (h)) -#define amdgpu_display_hpd_set_polarity(adev, h)
> (adev)->mode_info.funcs->hpd_set_polarity((adev), (h)) -#define
> amdgpu_display_hpd_get_gpio_reg(adev) (adev)->mode_info.funcs-
> >hpd_get_gpio_reg((adev))
> -#define amdgpu_display_bandwidth_update(adev) (adev)-
> >mode_info.funcs->bandwidth_update((adev))
> -#define amdgpu_display_page_flip(adev, crtc, base, async) (adev)-
> >mode_info.funcs->page_flip((adev), (crtc), (base), (async)) -#define
> amdgpu_display_page_flip_get_scanoutpos(adev, crtc, vbl, pos) (adev)-
> >mode_info.funcs->page_flip_get_scanoutpos((adev), (crtc), (vbl), (pos)) -
> #define amdgpu_display_add_encoder(adev, e, s, c) (adev)-
> >mode_info.funcs->add_encoder((adev), (e), (s), (c)) -#define
> amdgpu_display_add_connector(adev, ci, sd, ct, ib, coi, h, r) (adev)-
> >mode_info.funcs->add_connector((adev), (ci), (sd), (ct), (ib), (coi), (h), (r))
> +u32 amdgpu_display_vblank_get_counter(struct amdgpu_device *adev,
> +                                   int crtc);
> +bool amdgpu_display_hpd_sense(struct amdgpu_device *adev,
> +                           enum amdgpu_hpd_id hpd);
> +void amdgpu_display_hpd_set_polarity(struct amdgpu_device *adev,
> +                                  enum amdgpu_hpd_id hpd);
> +void amdgpu_display_backlight_set_level(struct amdgpu_device *adev,
> +                                     struct amdgpu_encoder
> *amdgpu_encoder,
> +                                     u8 level);
> +u8 amdgpu_display_backlight_get_level(struct amdgpu_device *adev,
> +                                   struct amdgpu_encoder
> *amdgpu_encoder); bool
> +amdgpu_display_hpd_sense(struct amdgpu_device *adev,
> +                           enum amdgpu_hpd_id hpd);
> +void amdgpu_display_hpd_set_polarity(struct amdgpu_device *adev,
> +                                  enum amdgpu_hpd_id hpd);
> +u32 amdgpu_display_hpd_get_gpio_reg(struct amdgpu_device *adev); void
> +amdgpu_display_bandwidth_update(struct amdgpu_device *adev); void
> +amdgpu_display_page_flip(struct amdgpu_device *adev,
> +                           int crtc_id, u64 crtc_base,
> +                            bool async);
> +int amdgpu_display_page_flip_get_scanoutpos(struct amdgpu_device
> *adev, int crtc,
> +                                         u32 *vbl, u32 *position);
> +
> +void amdgpu_display_add_encoder(struct amdgpu_device *adev,
> +                             u32 encoder_enum,
> +                             u32 supported_device,
> +                             u16 caps);
> +void amdgpu_display_add_connector(struct amdgpu_device *adev,
> +                               u32 connector_id,
> +                               u32 supported_device,
> +                               int connector_type,
> +                               struct amdgpu_i2c_bus_rec *i2c_bus,
> +                               u16 connector_object_id,
> +                               struct amdgpu_hpd *hpd,
> +                               struct amdgpu_router *router);
>
>  void amdgpu_display_hotplug_work_func(struct work_struct *work);  void
> amdgpu_display_update_priority(struct amdgpu_device *adev);
> --
> 2.25.1

<<attachment: winmail.dat>>


[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux