On Wed, Jul 19, 2023 at 1:20 AM Srinivasan Shanmugam
<srinivasan.shanmugam@xxxxxxx> wrote:
>
> 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?
>
I would drop this part. I don't think it's a net win.
> 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
This part looks good. Maybe split this out as a separate patch.
Alex
>
> 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
>
