All of lore.kernel.org
 help / color / mirror / Atom feed
* RE: [PATCH] drm/amd/amdgpu: port of DCE v6 to new headers
From: Deucher, Alexander @ 2016-11-14 17:10 UTC (permalink / raw)
  To: 'Tom St Denis',
	amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
  Cc: StDenis, Tom
In-Reply-To: <20161114145103.10275-1-tom.stdenis-5C7GfCeVMHo@public.gmane.org>

> -----Original Message-----
> From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On Behalf
> Of Tom St Denis
> Sent: Monday, November 14, 2016 9:51 AM
> To: amd-gfx@lists.freedesktop.org
> Cc: StDenis, Tom
> Subject: [PATCH] drm/amd/amdgpu: port of DCE v6 to new headers
> 
> Port of SI DCE v6 over to new AMDGPU headers.  Tested on a
> Tahiti with GNOME through various hot
> plugs/rotations/sizes/fullscreen/windowed and
> staging drm/xf86-video-amdgpu.
> 
> Signed-off-by: Tom St Denis <tom.stdenis@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/dce_v6_0.c              | 441 +++++++++++-------
> ---
>  drivers/gpu/drm/amd/amdgpu/si_enums.h              | 319 +++++++++------
>  .../gpu/drm/amd/include/asic_reg/dce/dce_6_0_d.h   |  21 +
>  3 files changed, 447 insertions(+), 334 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
> b/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
> index b0fdc291bf43..960e8f64864d 100644
> --- a/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
> @@ -30,8 +30,19 @@
>  #include "atombios_encoders.h"
>  #include "amdgpu_pll.h"
>  #include "amdgpu_connectors.h"
> -#include "si/si_reg.h"
> -#include "si/sid.h"
> +
> +#include "bif/bif_3_0_d.h"
> +#include "bif/bif_3_0_sh_mask.h"
> +#include "oss/oss_1_0_d.h"
> +#include "oss/oss_1_0_sh_mask.h"
> +#include "gca/gfx_6_0_d.h"
> +#include "gca/gfx_6_0_sh_mask.h"
> +#include "gmc/gmc_6_0_d.h"
> +#include "gmc/gmc_6_0_sh_mask.h"
> +#include "dce/dce_6_0_d.h"
> +#include "dce/dce_6_0_sh_mask.h"
> +#include "gca/gfx_7_2_enum.h"
> +#include "si_enums.h"
> 
>  static void dce_v6_0_set_display_funcs(struct amdgpu_device *adev);
>  static void dce_v6_0_set_irq_funcs(struct amdgpu_device *adev);
> @@ -48,12 +59,12 @@ static const u32 crtc_offsets[6] =
> 
>  static const u32 hpd_offsets[] =
>  {
> -	DC_HPD1_INT_STATUS - DC_HPD1_INT_STATUS,
> -	DC_HPD2_INT_STATUS - DC_HPD1_INT_STATUS,
> -	DC_HPD3_INT_STATUS - DC_HPD1_INT_STATUS,
> -	DC_HPD4_INT_STATUS - DC_HPD1_INT_STATUS,
> -	DC_HPD5_INT_STATUS - DC_HPD1_INT_STATUS,
> -	DC_HPD6_INT_STATUS - DC_HPD1_INT_STATUS,
> +	mmDC_HPD1_INT_STATUS - mmDC_HPD1_INT_STATUS,
> +	mmDC_HPD2_INT_STATUS - mmDC_HPD1_INT_STATUS,
> +	mmDC_HPD3_INT_STATUS - mmDC_HPD1_INT_STATUS,
> +	mmDC_HPD4_INT_STATUS - mmDC_HPD1_INT_STATUS,
> +	mmDC_HPD5_INT_STATUS - mmDC_HPD1_INT_STATUS,
> +	mmDC_HPD6_INT_STATUS - mmDC_HPD1_INT_STATUS,
>  };
> 
>  static const uint32_t dig_offsets[] = {
> @@ -73,32 +84,32 @@ static const struct {
>  	uint32_t	hpd;
> 
>  } interrupt_status_offsets[6] = { {
> -	.reg = DISP_INTERRUPT_STATUS,
> +	.reg = mmDISP_INTERRUPT_STATUS,
>  	.vblank =
> DISP_INTERRUPT_STATUS__LB_D1_VBLANK_INTERRUPT_MASK,
>  	.vline =
> DISP_INTERRUPT_STATUS__LB_D1_VLINE_INTERRUPT_MASK,
>  	.hpd = DISP_INTERRUPT_STATUS__DC_HPD1_INTERRUPT_MASK
>  }, {
> -	.reg = DISP_INTERRUPT_STATUS_CONTINUE,
> +	.reg = mmDISP_INTERRUPT_STATUS_CONTINUE,
>  	.vblank =
> DISP_INTERRUPT_STATUS_CONTINUE__LB_D2_VBLANK_INTERRUPT_MASK
> ,
>  	.vline =
> DISP_INTERRUPT_STATUS_CONTINUE__LB_D2_VLINE_INTERRUPT_MASK,
>  	.hpd =
> DISP_INTERRUPT_STATUS_CONTINUE__DC_HPD2_INTERRUPT_MASK
>  }, {
> -	.reg = DISP_INTERRUPT_STATUS_CONTINUE2,
> +	.reg = mmDISP_INTERRUPT_STATUS_CONTINUE2,
>  	.vblank =
> DISP_INTERRUPT_STATUS_CONTINUE2__LB_D3_VBLANK_INTERRUPT_MAS
> K,
>  	.vline =
> DISP_INTERRUPT_STATUS_CONTINUE2__LB_D3_VLINE_INTERRUPT_MASK,
>  	.hpd =
> DISP_INTERRUPT_STATUS_CONTINUE2__DC_HPD3_INTERRUPT_MASK
>  }, {
> -	.reg = DISP_INTERRUPT_STATUS_CONTINUE3,
> +	.reg = mmDISP_INTERRUPT_STATUS_CONTINUE3,
>  	.vblank =
> DISP_INTERRUPT_STATUS_CONTINUE3__LB_D4_VBLANK_INTERRUPT_MAS
> K,
>  	.vline =
> DISP_INTERRUPT_STATUS_CONTINUE3__LB_D4_VLINE_INTERRUPT_MASK,
>  	.hpd =
> DISP_INTERRUPT_STATUS_CONTINUE3__DC_HPD4_INTERRUPT_MASK
>  }, {
> -	.reg = DISP_INTERRUPT_STATUS_CONTINUE4,
> +	.reg = mmDISP_INTERRUPT_STATUS_CONTINUE4,
>  	.vblank =
> DISP_INTERRUPT_STATUS_CONTINUE4__LB_D5_VBLANK_INTERRUPT_MAS
> K,
>  	.vline =
> DISP_INTERRUPT_STATUS_CONTINUE4__LB_D5_VLINE_INTERRUPT_MASK,
>  	.hpd =
> DISP_INTERRUPT_STATUS_CONTINUE4__DC_HPD5_INTERRUPT_MASK
>  }, {
> -	.reg = DISP_INTERRUPT_STATUS_CONTINUE5,
> +	.reg = mmDISP_INTERRUPT_STATUS_CONTINUE5,
>  	.vblank =
> DISP_INTERRUPT_STATUS_CONTINUE5__LB_D6_VBLANK_INTERRUPT_MAS
> K,
>  	.vline =
> DISP_INTERRUPT_STATUS_CONTINUE5__LB_D6_VLINE_INTERRUPT_MASK,
>  	.hpd =
> DISP_INTERRUPT_STATUS_CONTINUE5__DC_HPD6_INTERRUPT_MASK
> @@ -119,7 +130,7 @@ static void dce_v6_0_audio_endpt_wreg(struct
> amdgpu_device *adev,
> 
>  static bool dce_v6_0_is_in_vblank(struct amdgpu_device *adev, int crtc)
>  {
> -	if (RREG32(EVERGREEN_CRTC_STATUS + crtc_offsets[crtc]) &
> EVERGREEN_CRTC_V_BLANK)
> +	if (RREG32(mmCRTC_STATUS + crtc_offsets[crtc]) &
> CRTC_STATUS__CRTC_V_BLANK_MASK)
>  		return true;
>  	else
>  		return false;
> @@ -129,8 +140,8 @@ static bool dce_v6_0_is_counter_moving(struct
> amdgpu_device *adev, int crtc)
>  {
>  	u32 pos1, pos2;
> 
> -	pos1 = RREG32(EVERGREEN_CRTC_STATUS_POSITION +
> crtc_offsets[crtc]);
> -	pos2 = RREG32(EVERGREEN_CRTC_STATUS_POSITION +
> crtc_offsets[crtc]);
> +	pos1 = RREG32(mmCRTC_STATUS_POSITION + crtc_offsets[crtc]);
> +	pos2 = RREG32(mmCRTC_STATUS_POSITION + crtc_offsets[crtc]);
> 
>  	if (pos1 != pos2)
>  		return true;
> @@ -152,7 +163,7 @@ static void dce_v6_0_vblank_wait(struct
> amdgpu_device *adev, int crtc)
>  	if (crtc >= adev->mode_info.num_crtc)
>  		return;
> 
> -	if (!(RREG32(EVERGREEN_CRTC_CONTROL + crtc_offsets[crtc]) &
> EVERGREEN_CRTC_MASTER_EN))
> +	if (!(RREG32(mmCRTC_CONTROL + crtc_offsets[crtc]) &
> CRTC_CONTROL__CRTC_MASTER_EN_MASK))
>  		return;
> 
>  	/* depending on when we hit vblank, we may be close to active; if
> so,
> @@ -180,7 +191,7 @@ static u32 dce_v6_0_vblank_get_counter(struct
> amdgpu_device *adev, int crtc)
>  	if (crtc >= adev->mode_info.num_crtc)
>  		return 0;
>  	else
> -		return RREG32(CRTC_STATUS_FRAME_COUNT +
> crtc_offsets[crtc]);
> +		return RREG32(mmCRTC_STATUS_FRAME_COUNT +
> crtc_offsets[crtc]);
>  }
> 
>  static void dce_v6_0_pageflip_interrupt_init(struct amdgpu_device *adev)
> @@ -220,16 +231,16 @@ static void dce_v6_0_page_flip(struct
> amdgpu_device *adev,
>  	struct amdgpu_crtc *amdgpu_crtc = adev->mode_info.crtcs[crtc_id];
> 
>  	/* flip at hsync for async, default is vsync */
> -	WREG32(EVERGREEN_GRPH_FLIP_CONTROL + amdgpu_crtc-
> >crtc_offset, async ?
> -	       EVERGREEN_GRPH_SURFACE_UPDATE_H_RETRACE_EN : 0);
> +	WREG32(mmGRPH_FLIP_CONTROL + amdgpu_crtc->crtc_offset,
> async ?
> +
> GRPH_FLIP_CONTROL__GRPH_SURFACE_UPDATE_H_RETRACE_EN_MASK :
> 0);
>  	/* update the scanout addresses */
> -	WREG32(EVERGREEN_GRPH_PRIMARY_SURFACE_ADDRESS_HIGH +
> amdgpu_crtc->crtc_offset,
> +	WREG32(mmGRPH_PRIMARY_SURFACE_ADDRESS_HIGH +
> amdgpu_crtc->crtc_offset,
>  	       upper_32_bits(crtc_base));
> -	WREG32(EVERGREEN_GRPH_PRIMARY_SURFACE_ADDRESS +
> amdgpu_crtc->crtc_offset,
> +	WREG32(mmGRPH_PRIMARY_SURFACE_ADDRESS + amdgpu_crtc-
> >crtc_offset,
>  	       (u32)crtc_base);
> 
>  	/* post the write */
> -	RREG32(EVERGREEN_GRPH_PRIMARY_SURFACE_ADDRESS +
> amdgpu_crtc->crtc_offset);
> +	RREG32(mmGRPH_PRIMARY_SURFACE_ADDRESS + amdgpu_crtc-
> >crtc_offset);
>  }
> 
>  static int dce_v6_0_crtc_get_scanoutpos(struct amdgpu_device *adev, int
> crtc,
> @@ -237,8 +248,8 @@ static int dce_v6_0_crtc_get_scanoutpos(struct
> amdgpu_device *adev, int crtc,
>  {
>  	if ((crtc < 0) || (crtc >= adev->mode_info.num_crtc))
>  		return -EINVAL;
> -	*vbl = RREG32(EVERGREEN_CRTC_V_BLANK_START_END +
> crtc_offsets[crtc]);
> -	*position = RREG32(EVERGREEN_CRTC_STATUS_POSITION +
> crtc_offsets[crtc]);
> +	*vbl = RREG32(mmCRTC_V_BLANK_START_END + crtc_offsets[crtc]);
> +	*position = RREG32(mmCRTC_STATUS_POSITION +
> crtc_offsets[crtc]);
> 
>  	return 0;
> 
> @@ -261,7 +272,7 @@ static bool dce_v6_0_hpd_sense(struct
> amdgpu_device *adev,
>  	if (hpd >= adev->mode_info.num_hpd)
>  		return connected;
> 
> -	if (RREG32(DC_HPD1_INT_STATUS + hpd_offsets[hpd]) &
> DC_HPDx_SENSE)
> +	if (RREG32(mmDC_HPD1_INT_STATUS + hpd_offsets[hpd]) &
> DC_HPD1_INT_STATUS__DC_HPD1_SENSE_MASK)
>  		connected = true;
> 
>  	return connected;
> @@ -284,12 +295,12 @@ static void dce_v6_0_hpd_set_polarity(struct
> amdgpu_device *adev,
>  	if (hpd >= adev->mode_info.num_hpd)
>  		return;
> 
> -	tmp = RREG32(DC_HPD1_INT_CONTROL + hpd_offsets[hpd]);
> +	tmp = RREG32(mmDC_HPD1_INT_CONTROL + hpd_offsets[hpd]);
>  	if (connected)
> -		tmp &= ~DC_HPDx_INT_POLARITY;
> +		tmp &=
> ~DC_HPD1_INT_CONTROL__DC_HPD1_INT_POLARITY_MASK;
>  	else
> -		tmp |= DC_HPDx_INT_POLARITY;
> -	WREG32(DC_HPD1_INT_CONTROL + hpd_offsets[hpd], tmp);
> +		tmp |=
> DC_HPD1_INT_CONTROL__DC_HPD1_INT_POLARITY_MASK;
> +	WREG32(mmDC_HPD1_INT_CONTROL + hpd_offsets[hpd], tmp);
>  }
> 
>  /**
> @@ -312,9 +323,9 @@ static void dce_v6_0_hpd_init(struct amdgpu_device
> *adev)
>  		if (amdgpu_connector->hpd.hpd >= adev-
> >mode_info.num_hpd)
>  			continue;
> 
> -		tmp = RREG32(DC_HPD1_CONTROL +
> hpd_offsets[amdgpu_connector->hpd.hpd]);
> -		tmp |= DC_HPDx_EN;
> -		WREG32(DC_HPD1_CONTROL +
> hpd_offsets[amdgpu_connector->hpd.hpd], tmp);
> +		tmp = RREG32(mmDC_HPD1_CONTROL +
> hpd_offsets[amdgpu_connector->hpd.hpd]);
> +		tmp |= DC_HPD1_CONTROL__DC_HPD1_EN_MASK;
> +		WREG32(mmDC_HPD1_CONTROL +
> hpd_offsets[amdgpu_connector->hpd.hpd], tmp);
> 
>  		if (connector->connector_type ==
> DRM_MODE_CONNECTOR_eDP ||
>  		    connector->connector_type ==
> DRM_MODE_CONNECTOR_LVDS) {
> @@ -323,9 +334,9 @@ static void dce_v6_0_hpd_init(struct amdgpu_device
> *adev)
>  			 *
> https://bugzilla.redhat.com/show_bug.cgi?id=726143
>  			 * also avoid interrupt storms during dpms.
>  			 */
> -			tmp = RREG32(DC_HPD1_INT_CONTROL +
> hpd_offsets[amdgpu_connector->hpd.hpd]);
> -			tmp &= ~DC_HPDx_INT_EN;
> -			WREG32(DC_HPD1_INT_CONTROL +
> hpd_offsets[amdgpu_connector->hpd.hpd], tmp);
> +			tmp = RREG32(mmDC_HPD1_INT_CONTROL +
> hpd_offsets[amdgpu_connector->hpd.hpd]);
> +			tmp &=
> ~DC_HPD1_INT_CONTROL__DC_HPD1_INT_EN_MASK;
> +			WREG32(mmDC_HPD1_INT_CONTROL +
> hpd_offsets[amdgpu_connector->hpd.hpd], tmp);
>  			continue;
>  		}
> 
> @@ -355,9 +366,9 @@ static void dce_v6_0_hpd_fini(struct amdgpu_device
> *adev)
>  		if (amdgpu_connector->hpd.hpd >= adev-
> >mode_info.num_hpd)
>  			continue;
> 
> -		tmp = RREG32(DC_HPD1_CONTROL +
> hpd_offsets[amdgpu_connector->hpd.hpd]);
> -		tmp &= ~DC_HPDx_EN;
> -		WREG32(DC_HPD1_CONTROL +
> hpd_offsets[amdgpu_connector->hpd.hpd], 0);
> +		tmp = RREG32(mmDC_HPD1_CONTROL +
> hpd_offsets[amdgpu_connector->hpd.hpd]);
> +		tmp &= ~DC_HPD1_CONTROL__DC_HPD1_EN_MASK;
> +		WREG32(mmDC_HPD1_CONTROL +
> hpd_offsets[amdgpu_connector->hpd.hpd], 0);
> 
>  		amdgpu_irq_put(adev, &adev->hpd_irq,
> amdgpu_connector->hpd.hpd);
>  	}
> @@ -365,7 +376,7 @@ static void dce_v6_0_hpd_fini(struct amdgpu_device
> *adev)
> 
>  static u32 dce_v6_0_hpd_get_gpio_reg(struct amdgpu_device *adev)
>  {
> -	return SI_DC_GPIO_HPD_A;
> +	return mmDC_GPIO_HPD_A;
>  }
> 
>  static bool dce_v6_0_is_display_hung(struct amdgpu_device *adev)
> @@ -380,7 +391,7 @@ static u32 evergreen_get_vblank_counter(struct
> amdgpu_device* adev, int crtc)
>  	if (crtc >= adev->mode_info.num_crtc)
>  		return 0;
>  	else
> -		return RREG32(CRTC_STATUS_FRAME_COUNT +
> crtc_offsets[crtc]);
> +		return RREG32(mmCRTC_STATUS_FRAME_COUNT +
> crtc_offsets[crtc]);
>  }
> 
>  static void dce_v6_0_stop_mc_access(struct amdgpu_device *adev,
> @@ -389,25 +400,25 @@ static void dce_v6_0_stop_mc_access(struct
> amdgpu_device *adev,
>  	u32 crtc_enabled, tmp, frame_count;
>  	int i, j;
> 
> -	save->vga_render_control = RREG32(VGA_RENDER_CONTROL);
> -	save->vga_hdp_control = RREG32(VGA_HDP_CONTROL);
> +	save->vga_render_control = RREG32(mmVGA_RENDER_CONTROL);
> +	save->vga_hdp_control = RREG32(mmVGA_HDP_CONTROL);
> 
>  	/* disable VGA render */
> -	WREG32(VGA_RENDER_CONTROL, 0);
> +	WREG32(mmVGA_RENDER_CONTROL, 0);
> 
>  	/* blank the display controllers */
>  	for (i = 0; i < adev->mode_info.num_crtc; i++) {
> -		crtc_enabled = RREG32(EVERGREEN_CRTC_CONTROL +
> crtc_offsets[i]) & EVERGREEN_CRTC_MASTER_EN;
> +		crtc_enabled = RREG32(mmCRTC_CONTROL + crtc_offsets[i])
> & CRTC_CONTROL__CRTC_MASTER_EN_MASK;
>  		if (crtc_enabled) {
>  			save->crtc_enabled[i] = true;
> -			tmp = RREG32(EVERGREEN_CRTC_BLANK_CONTROL
> + crtc_offsets[i]);
> +			tmp = RREG32(mmCRTC_BLANK_CONTROL +
> crtc_offsets[i]);
> 
> -			if (!(tmp & EVERGREEN_CRTC_BLANK_DATA_EN)) {
> +			if (!(tmp &
> CRTC_BLANK_CONTROL__CRTC_BLANK_DATA_EN_MASK)) {
>  				dce_v6_0_vblank_wait(adev, i);
> -				WREG32(EVERGREEN_CRTC_UPDATE_LOCK +
> crtc_offsets[i], 1);
> -				tmp |= EVERGREEN_CRTC_BLANK_DATA_EN;
> -
> 	WREG32(EVERGREEN_CRTC_BLANK_CONTROL + crtc_offsets[i],
> tmp);
> -				WREG32(EVERGREEN_CRTC_UPDATE_LOCK +
> crtc_offsets[i], 0);
> +				WREG32(mmCRTC_UPDATE_LOCK +
> crtc_offsets[i], 1);
> +				tmp |=
> CRTC_BLANK_CONTROL__CRTC_BLANK_DATA_EN_MASK;
> +				WREG32(mmCRTC_BLANK_CONTROL +
> crtc_offsets[i], tmp);
> +				WREG32(mmCRTC_UPDATE_LOCK +
> crtc_offsets[i], 0);
>  			}
>  			/* wait for the next frame */
>  			frame_count =
> evergreen_get_vblank_counter(adev, i);
> @@ -418,11 +429,11 @@ static void dce_v6_0_stop_mc_access(struct
> amdgpu_device *adev,
>  			}
> 
>  			/* XXX this is a hack to avoid strange behavior with
> EFI on certain systems */
> -			WREG32(EVERGREEN_CRTC_UPDATE_LOCK +
> crtc_offsets[i], 1);
> -			tmp = RREG32(EVERGREEN_CRTC_CONTROL +
> crtc_offsets[i]);
> -			tmp &= ~EVERGREEN_CRTC_MASTER_EN;
> -			WREG32(EVERGREEN_CRTC_CONTROL +
> crtc_offsets[i], tmp);
> -			WREG32(EVERGREEN_CRTC_UPDATE_LOCK +
> crtc_offsets[i], 0);
> +			WREG32(mmCRTC_UPDATE_LOCK + crtc_offsets[i],
> 1);
> +			tmp = RREG32(mmCRTC_CONTROL + crtc_offsets[i]);
> +			tmp &=
> ~CRTC_CONTROL__CRTC_MASTER_EN_MASK;
> +			WREG32(mmCRTC_CONTROL + crtc_offsets[i], tmp);
> +			WREG32(mmCRTC_UPDATE_LOCK + crtc_offsets[i],
> 0);
>  			save->crtc_enabled[i] = false;
>  			/* ***** */
>  		} else {
> @@ -439,41 +450,41 @@ static void dce_v6_0_resume_mc_access(struct
> amdgpu_device *adev,
> 
>  	/* update crtc base addresses */
>  	for (i = 0; i < adev->mode_info.num_crtc; i++) {
> -
> 	WREG32(EVERGREEN_GRPH_PRIMARY_SURFACE_ADDRESS_HIGH +
> crtc_offsets[i],
> +		WREG32(mmGRPH_PRIMARY_SURFACE_ADDRESS_HIGH +
> crtc_offsets[i],
>  		       upper_32_bits(adev->mc.vram_start));
> -
> 	WREG32(EVERGREEN_GRPH_SECONDARY_SURFACE_ADDRESS_HIG
> H + crtc_offsets[i],
> +		WREG32(mmGRPH_SECONDARY_SURFACE_ADDRESS_HIGH
> + crtc_offsets[i],
>  		       upper_32_bits(adev->mc.vram_start));
> -		WREG32(EVERGREEN_GRPH_PRIMARY_SURFACE_ADDRESS
> + crtc_offsets[i],
> +		WREG32(mmGRPH_PRIMARY_SURFACE_ADDRESS +
> crtc_offsets[i],
>  		       (u32)adev->mc.vram_start);
> -
> 	WREG32(EVERGREEN_GRPH_SECONDARY_SURFACE_ADDRESS +
> crtc_offsets[i],
> +		WREG32(mmGRPH_SECONDARY_SURFACE_ADDRESS +
> crtc_offsets[i],
>  		       (u32)adev->mc.vram_start);
>  	}
> 
> -	WREG32(EVERGREEN_VGA_MEMORY_BASE_ADDRESS_HIGH,
> upper_32_bits(adev->mc.vram_start));
> -	WREG32(EVERGREEN_VGA_MEMORY_BASE_ADDRESS, (u32)adev-
> >mc.vram_start);
> +	WREG32(mmEVERGREEN_VGA_MEMORY_BASE_ADDRESS_HIGH,
> upper_32_bits(adev->mc.vram_start));
> +	WREG32(mmEVERGREEN_VGA_MEMORY_BASE_ADDRESS,
> (u32)adev->mc.vram_start);
> 
>  	/* unlock regs and wait for update */
>  	for (i = 0; i < adev->mode_info.num_crtc; i++) {
>  		if (save->crtc_enabled[i]) {
> -			tmp =
> RREG32(EVERGREEN_MASTER_UPDATE_MODE + crtc_offsets[i]);
> +			tmp = RREG32(mmMASTER_UPDATE_MODE +
> crtc_offsets[i]);
>  			if ((tmp & 0x7) != 3) {
>  				tmp &= ~0x7;
>  				tmp |= 0x3;
> -
> 	WREG32(EVERGREEN_MASTER_UPDATE_MODE + crtc_offsets[i],
> tmp);
> +				WREG32(mmMASTER_UPDATE_MODE +
> crtc_offsets[i], tmp);
>  			}
> -			tmp = RREG32(EVERGREEN_GRPH_UPDATE +
> crtc_offsets[i]);
> -			if (tmp & EVERGREEN_GRPH_UPDATE_LOCK) {
> -				tmp &= ~EVERGREEN_GRPH_UPDATE_LOCK;
> -				WREG32(EVERGREEN_GRPH_UPDATE +
> crtc_offsets[i], tmp);
> +			tmp = RREG32(mmGRPH_UPDATE + crtc_offsets[i]);
> +			if (tmp &
> GRPH_UPDATE__GRPH_UPDATE_LOCK_MASK) {
> +				tmp &=
> ~GRPH_UPDATE__GRPH_UPDATE_LOCK_MASK;
> +				WREG32(mmGRPH_UPDATE + crtc_offsets[i],
> tmp);
>  			}
> -			tmp = RREG32(EVERGREEN_MASTER_UPDATE_LOCK
> + crtc_offsets[i]);
> +			tmp = RREG32(mmMASTER_UPDATE_LOCK +
> crtc_offsets[i]);
>  			if (tmp & 1) {
>  				tmp &= ~1;
> -
> 	WREG32(EVERGREEN_MASTER_UPDATE_LOCK + crtc_offsets[i],
> tmp);
> +				WREG32(mmMASTER_UPDATE_LOCK +
> crtc_offsets[i], tmp);
>  			}
>  			for (j = 0; j < adev->usec_timeout; j++) {
> -				tmp = RREG32(EVERGREEN_GRPH_UPDATE +
> crtc_offsets[i]);
> -				if ((tmp &
> EVERGREEN_GRPH_SURFACE_UPDATE_PENDING) == 0)
> +				tmp = RREG32(mmGRPH_UPDATE +
> crtc_offsets[i]);
> +				if ((tmp &
> GRPH_UPDATE__GRPH_SURFACE_UPDATE_PENDING_MASK) == 0)
>  					break;
>  				udelay(1);
>  			}
> @@ -481,9 +492,9 @@ static void dce_v6_0_resume_mc_access(struct
> amdgpu_device *adev,
>  	}
> 
>  	/* Unlock vga access */
> -	WREG32(VGA_HDP_CONTROL, save->vga_hdp_control);
> +	WREG32(mmVGA_HDP_CONTROL, save->vga_hdp_control);
>  	mdelay(1);
> -	WREG32(VGA_RENDER_CONTROL, save->vga_render_control);
> +	WREG32(mmVGA_RENDER_CONTROL, save->vga_render_control);
> 
>  }
> 
> @@ -491,8 +502,8 @@ static void dce_v6_0_set_vga_render_state(struct
> amdgpu_device *adev,
>  					  bool render)
>  {
>  	if (!render)
> -		WREG32(R_000300_VGA_RENDER_CONTROL,
> -			RREG32(R_000300_VGA_RENDER_CONTROL) &
> C_000300_VGA_VSTATUS_CNTL);
> +		WREG32(mmR_000300_VGA_RENDER_CONTROL,
> +			RREG32(mmR_000300_VGA_RENDER_CONTROL) &
> C_000300_VGA_VSTATUS_CNTL);
> 
>  }
> 
> @@ -526,14 +537,14 @@ void dce_v6_0_disable_dce(struct amdgpu_device
> *adev)
> 
>  		/*Disable crtc*/
>  		for (i = 0; i < dce_v6_0_get_num_crtc(adev); i++) {
> -			crtc_enabled = RREG32(EVERGREEN_CRTC_CONTROL
> + crtc_offsets[i]) &
> -				EVERGREEN_CRTC_MASTER_EN;
> +			crtc_enabled = RREG32(mmCRTC_CONTROL +
> crtc_offsets[i]) &
> +				CRTC_CONTROL__CRTC_MASTER_EN_MASK;
>  			if (crtc_enabled) {
> -				WREG32(EVERGREEN_CRTC_UPDATE_LOCK +
> crtc_offsets[i], 1);
> -				tmp = RREG32(EVERGREEN_CRTC_CONTROL
> + crtc_offsets[i]);
> -				tmp &= ~EVERGREEN_CRTC_MASTER_EN;
> -				WREG32(EVERGREEN_CRTC_CONTROL +
> crtc_offsets[i], tmp);
> -				WREG32(EVERGREEN_CRTC_UPDATE_LOCK +
> crtc_offsets[i], 0);
> +				WREG32(mmCRTC_UPDATE_LOCK +
> crtc_offsets[i], 1);
> +				tmp = RREG32(mmCRTC_CONTROL +
> crtc_offsets[i]);
> +				tmp &=
> ~CRTC_CONTROL__CRTC_MASTER_EN_MASK;
> +				WREG32(mmCRTC_CONTROL +
> crtc_offsets[i], tmp);
> +				WREG32(mmCRTC_UPDATE_LOCK +
> crtc_offsets[i], 0);
>  			}
>  		}
>  	}
> @@ -569,19 +580,23 @@ static void dce_v6_0_program_fmt(struct
> drm_encoder *encoder)
>  	case 6:
>  		if (dither == AMDGPU_FMT_DITHER_ENABLE)
>  			/* XXX sort out optimal dither settings */
> -			tmp |= (FMT_FRAME_RANDOM_ENABLE |
> FMT_HIGHPASS_RANDOM_ENABLE |
> -				FMT_SPATIAL_DITHER_EN);
> +			tmp |=
> (FMT_BIT_DEPTH_CONTROL__FMT_FRAME_RANDOM_ENABLE_MASK |
> +
> 	FMT_BIT_DEPTH_CONTROL__FMT_HIGHPASS_RANDOM_ENABLE_
> MASK |
> +
> 	FMT_BIT_DEPTH_CONTROL__FMT_SPATIAL_DITHER_EN_MASK);
>  		else
> -			tmp |= FMT_TRUNCATE_EN;
> +			tmp |=
> FMT_BIT_DEPTH_CONTROL__FMT_TRUNCATE_EN_MASK;
>  		break;
>  	case 8:
>  		if (dither == AMDGPU_FMT_DITHER_ENABLE)
>  			/* XXX sort out optimal dither settings */
> -			tmp |= (FMT_FRAME_RANDOM_ENABLE |
> FMT_HIGHPASS_RANDOM_ENABLE |
> -				FMT_RGB_RANDOM_ENABLE |
> -				FMT_SPATIAL_DITHER_EN |
> FMT_SPATIAL_DITHER_DEPTH);
> +			tmp |=
> (FMT_BIT_DEPTH_CONTROL__FMT_FRAME_RANDOM_ENABLE_MASK |
> +
> 	FMT_BIT_DEPTH_CONTROL__FMT_HIGHPASS_RANDOM_ENABLE_
> MASK |
> +
> 	FMT_BIT_DEPTH_CONTROL__FMT_RGB_RANDOM_ENABLE_MASK
> |
> +
> 	FMT_BIT_DEPTH_CONTROL__FMT_SPATIAL_DITHER_EN_MASK |
> +
> 	FMT_BIT_DEPTH_CONTROL__FMT_SPATIAL_DITHER_DEPTH_MASK)
> ;
>  		else
> -			tmp |= (FMT_TRUNCATE_EN |
> FMT_TRUNCATE_DEPTH);
> +			tmp |=
> (FMT_BIT_DEPTH_CONTROL__FMT_TRUNCATE_EN_MASK |
> +
> 	FMT_BIT_DEPTH_CONTROL__FMT_TRUNCATE_DEPTH_MASK);
>  		break;
>  	case 10:
>  	default:
> @@ -589,7 +604,7 @@ static void dce_v6_0_program_fmt(struct
> drm_encoder *encoder)
>  		break;
>  	}
> 
> -	WREG32(FMT_BIT_DEPTH_CONTROL + amdgpu_crtc->crtc_offset,
> tmp);
> +	WREG32(mmFMT_BIT_DEPTH_CONTROL + amdgpu_crtc-
> >crtc_offset, tmp);
>  }
> 
>  /**
> @@ -603,7 +618,7 @@ static void dce_v6_0_program_fmt(struct
> drm_encoder *encoder)
>   */
>  static u32 si_get_number_of_dram_channels(struct amdgpu_device *adev)
>  {
> -	u32 tmp = RREG32(MC_SHARED_CHMAP);
> +	u32 tmp = RREG32(mmMC_SHARED_CHMAP);
> 
>  	switch ((tmp & MC_SHARED_CHMAP__NOOFCHAN_MASK) >>
> MC_SHARED_CHMAP__NOOFCHAN__SHIFT) {
>  	case 0:
> @@ -1100,28 +1115,28 @@ static void
> dce_v6_0_program_watermarks(struct amdgpu_device *adev,
>  	}
> 
>  	/* select wm A */
> -	arb_control3 = RREG32(DPG_PIPE_ARBITRATION_CONTROL3 +
> amdgpu_crtc->crtc_offset);
> +	arb_control3 = RREG32(mmDPG_PIPE_ARBITRATION_CONTROL3 +
> amdgpu_crtc->crtc_offset);
>  	tmp = arb_control3;
>  	tmp &= ~LATENCY_WATERMARK_MASK(3);
>  	tmp |= LATENCY_WATERMARK_MASK(1);
> -	WREG32(DPG_PIPE_ARBITRATION_CONTROL3 + amdgpu_crtc-
> >crtc_offset, tmp);
> -	WREG32(DPG_PIPE_LATENCY_CONTROL + amdgpu_crtc-
> >crtc_offset,
> -	       (LATENCY_LOW_WATERMARK(latency_watermark_a) |
> -		LATENCY_HIGH_WATERMARK(line_time)));
> +	WREG32(mmDPG_PIPE_ARBITRATION_CONTROL3 + amdgpu_crtc-
> >crtc_offset, tmp);
> +	WREG32(mmDPG_PIPE_URGENCY_CONTROL + amdgpu_crtc-
> >crtc_offset,
> +	       ((latency_watermark_a <<
> DPG_PIPE_URGENCY_CONTROL__URGENCY_LOW_WATERMARK__SHIFT)  |
> +		(line_time <<
> DPG_PIPE_URGENCY_CONTROL__URGENCY_HIGH_WATERMARK__SHIFT)))
> ;
>  	/* select wm B */
> -	tmp = RREG32(DPG_PIPE_ARBITRATION_CONTROL3 + amdgpu_crtc-
> >crtc_offset);
> +	tmp = RREG32(mmDPG_PIPE_ARBITRATION_CONTROL3 +
> amdgpu_crtc->crtc_offset);
>  	tmp &= ~LATENCY_WATERMARK_MASK(3);
>  	tmp |= LATENCY_WATERMARK_MASK(2);
> -	WREG32(DPG_PIPE_ARBITRATION_CONTROL3 + amdgpu_crtc-
> >crtc_offset, tmp);
> -	WREG32(DPG_PIPE_LATENCY_CONTROL + amdgpu_crtc-
> >crtc_offset,
> -	       (LATENCY_LOW_WATERMARK(latency_watermark_b) |
> -		LATENCY_HIGH_WATERMARK(line_time)));
> +	WREG32(mmDPG_PIPE_ARBITRATION_CONTROL3 + amdgpu_crtc-
> >crtc_offset, tmp);
> +	WREG32(mmDPG_PIPE_URGENCY_CONTROL + amdgpu_crtc-
> >crtc_offset,
> +	       ((latency_watermark_b <<
> DPG_PIPE_URGENCY_CONTROL__URGENCY_LOW_WATERMARK__SHIFT) |
> +		(line_time <<
> DPG_PIPE_URGENCY_CONTROL__URGENCY_HIGH_WATERMARK__SHIFT)))
> ;
>  	/* restore original selection */
> -	WREG32(DPG_PIPE_ARBITRATION_CONTROL3 + amdgpu_crtc-
> >crtc_offset, arb_control3);
> +	WREG32(mmDPG_PIPE_ARBITRATION_CONTROL3 + amdgpu_crtc-
> >crtc_offset, arb_control3);
> 
>  	/* write the priority marks */
> -	WREG32(PRIORITY_A_CNT + amdgpu_crtc->crtc_offset,
> priority_a_cnt);
> -	WREG32(PRIORITY_B_CNT + amdgpu_crtc->crtc_offset,
> priority_b_cnt);
> +	WREG32(mmPRIORITY_A_CNT + amdgpu_crtc->crtc_offset,
> priority_a_cnt);
> +	WREG32(mmPRIORITY_B_CNT + amdgpu_crtc->crtc_offset,
> priority_b_cnt);
> 
>  	/* save values for DPM */
>  	amdgpu_crtc->line_time = line_time;
> @@ -1139,7 +1154,7 @@ static u32 dce_v6_0_line_buffer_adjust(struct
> amdgpu_device *adev,
>  	/*
>  	 * Line Buffer Setup
>  	 * There are 3 line buffers, each one shared by 2 display controllers.
> -	 * DC_LB_MEMORY_SPLIT controls how that line buffer is shared
> between
> +	 * mmDC_LB_MEMORY_SPLIT controls how that line buffer is shared
> between
>  	 * the display controllers.  The paritioning is done via one of four
>  	 * preset allocations specified in bits 21:20:
>  	 *  0 - half lb
> @@ -1162,14 +1177,14 @@ static u32 dce_v6_0_line_buffer_adjust(struct
> amdgpu_device *adev,
>  		buffer_alloc = 0;
>  	}
> 
> -	WREG32(DC_LB_MEMORY_SPLIT + amdgpu_crtc->crtc_offset,
> +	WREG32(mmDC_LB_MEMORY_SPLIT + amdgpu_crtc->crtc_offset,
>  	       DC_LB_MEMORY_CONFIG(tmp));
> 
> -	WREG32(PIPE0_DMIF_BUFFER_CONTROL + pipe_offset,
> -	       DMIF_BUFFERS_ALLOCATED(buffer_alloc));
> +	WREG32(mmPIPE0_DMIF_BUFFER_CONTROL + pipe_offset,
> +	       (buffer_alloc <<
> PIPE0_DMIF_BUFFER_CONTROL__DMIF_BUFFERS_ALLOCATED__SHIFT));
>  	for (i = 0; i < adev->usec_timeout; i++) {
> -		if (RREG32(PIPE0_DMIF_BUFFER_CONTROL + pipe_offset) &
> -		    DMIF_BUFFERS_ALLOCATED_COMPLETED)
> +		if (RREG32(mmPIPE0_DMIF_BUFFER_CONTROL +
> pipe_offset) &
> +
> PIPE0_DMIF_BUFFER_CONTROL__DMIF_BUFFERS_ALLOCATION_COMPLETE
> D_MASK)
>  			break;
>  		udelay(1);
>  	}
> @@ -1411,12 +1426,12 @@ static void dce_v6_0_afmt_fini(struct
> amdgpu_device *adev)
> 
>  static const u32 vga_control_regs[6] =
>  {
> -	AVIVO_D1VGA_CONTROL,
> -	AVIVO_D2VGA_CONTROL,
> -	EVERGREEN_D3VGA_CONTROL,
> -	EVERGREEN_D4VGA_CONTROL,
> -	EVERGREEN_D5VGA_CONTROL,
> -	EVERGREEN_D6VGA_CONTROL,
> +	mmAVIVO_D1VGA_CONTROL,
> +	mmAVIVO_D2VGA_CONTROL,
> +	mmEVERGREEN_D3VGA_CONTROL,
> +	mmEVERGREEN_D4VGA_CONTROL,
> +	mmEVERGREEN_D5VGA_CONTROL,
> +	mmEVERGREEN_D6VGA_CONTROL,
>  };
> 
>  static void dce_v6_0_vga_enable(struct drm_crtc *crtc, bool enable)
> @@ -1436,7 +1451,7 @@ static void dce_v6_0_grph_enable(struct drm_crtc
> *crtc, bool enable)
>  	struct drm_device *dev = crtc->dev;
>  	struct amdgpu_device *adev = dev->dev_private;
> 
> -	WREG32(EVERGREEN_GRPH_ENABLE + amdgpu_crtc->crtc_offset,
> enable ? 1 : 0);
> +	WREG32(mmGRPH_ENABLE + amdgpu_crtc->crtc_offset, enable ? 1 :
> 0);
>  }
> 
>  static int dce_v6_0_crtc_do_set_base(struct drm_crtc *crtc,
> @@ -1590,57 +1605,57 @@ static int dce_v6_0_crtc_do_set_base(struct
> drm_crtc *crtc,
>  	/* Make sure surface address is updated at vertical blank rather than
>  	 * horizontal blank
>  	 */
> -	WREG32(EVERGREEN_GRPH_FLIP_CONTROL + amdgpu_crtc-
> >crtc_offset, 0);
> +	WREG32(mmGRPH_FLIP_CONTROL + amdgpu_crtc->crtc_offset, 0);
> 
> -	WREG32(EVERGREEN_GRPH_PRIMARY_SURFACE_ADDRESS_HIGH +
> amdgpu_crtc->crtc_offset,
> +	WREG32(mmGRPH_PRIMARY_SURFACE_ADDRESS_HIGH +
> amdgpu_crtc->crtc_offset,
>  	       upper_32_bits(fb_location));
> -
> 	WREG32(EVERGREEN_GRPH_SECONDARY_SURFACE_ADDRESS_HIG
> H + amdgpu_crtc->crtc_offset,
> +	WREG32(mmGRPH_SECONDARY_SURFACE_ADDRESS_HIGH +
> amdgpu_crtc->crtc_offset,
>  	       upper_32_bits(fb_location));
> -	WREG32(EVERGREEN_GRPH_PRIMARY_SURFACE_ADDRESS +
> amdgpu_crtc->crtc_offset,
> -	       (u32)fb_location &
> EVERGREEN_GRPH_SURFACE_ADDRESS_MASK);
> -	WREG32(EVERGREEN_GRPH_SECONDARY_SURFACE_ADDRESS +
> amdgpu_crtc->crtc_offset,
> -	       (u32) fb_location &
> EVERGREEN_GRPH_SURFACE_ADDRESS_MASK);
> -	WREG32(EVERGREEN_GRPH_CONTROL + amdgpu_crtc->crtc_offset,
> fb_format);
> -	WREG32(EVERGREEN_GRPH_SWAP_CONTROL + amdgpu_crtc-
> >crtc_offset, fb_swap);
> +	WREG32(mmGRPH_PRIMARY_SURFACE_ADDRESS + amdgpu_crtc-
> >crtc_offset,
> +	       (u32)fb_location &
> GRPH_PRIMARY_SURFACE_ADDRESS__GRPH_PRIMARY_SURFACE_ADDRES
> S_MASK);
> +	WREG32(mmGRPH_SECONDARY_SURFACE_ADDRESS +
> amdgpu_crtc->crtc_offset,
> +	       (u32) fb_location &
> GRPH_PRIMARY_SURFACE_ADDRESS__GRPH_PRIMARY_SURFACE_ADDRES
> S_MASK);
> +	WREG32(mmGRPH_CONTROL + amdgpu_crtc->crtc_offset,
> fb_format);
> +	WREG32(mmGRPH_SWAP_CNTL + amdgpu_crtc->crtc_offset,
> fb_swap);
> 
>  	/*
>  	 * The LUT only has 256 slots for indexing by a 8 bpc fb. Bypass the
> LUT
>  	 * for > 8 bpc scanout to avoid truncation of fb indices to 8 msb's, to
>  	 * retain the full precision throughout the pipeline.
>  	 */
> -	WREG32_P(EVERGREEN_GRPH_LUT_10BIT_BYPASS_CONTROL +
> amdgpu_crtc->crtc_offset,
> -		 (bypass_lut ? EVERGREEN_LUT_10BIT_BYPASS_EN : 0),
> -		 ~EVERGREEN_LUT_10BIT_BYPASS_EN);
> +	WREG32_P(mmGRPH_LUT_10BIT_BYPASS + amdgpu_crtc-
> >crtc_offset,
> +		 (bypass_lut ?
> GRPH_LUT_10BIT_BYPASS__GRPH_LUT_10BIT_BYPASS_EN_MASK : 0),
> +
> ~GRPH_LUT_10BIT_BYPASS__GRPH_LUT_10BIT_BYPASS_EN_MASK);
> 
>  	if (bypass_lut)
>  		DRM_DEBUG_KMS("Bypassing hardware LUT due to 10 bit fb
> scanout.\n");
> 
> -	WREG32(EVERGREEN_GRPH_SURFACE_OFFSET_X + amdgpu_crtc-
> >crtc_offset, 0);
> -	WREG32(EVERGREEN_GRPH_SURFACE_OFFSET_Y + amdgpu_crtc-
> >crtc_offset, 0);
> -	WREG32(EVERGREEN_GRPH_X_START + amdgpu_crtc->crtc_offset,
> 0);
> -	WREG32(EVERGREEN_GRPH_Y_START + amdgpu_crtc->crtc_offset,
> 0);
> -	WREG32(EVERGREEN_GRPH_X_END + amdgpu_crtc->crtc_offset,
> target_fb->width);
> -	WREG32(EVERGREEN_GRPH_Y_END + amdgpu_crtc->crtc_offset,
> target_fb->height);
> +	WREG32(mmGRPH_SURFACE_OFFSET_X + amdgpu_crtc-
> >crtc_offset, 0);
> +	WREG32(mmGRPH_SURFACE_OFFSET_Y + amdgpu_crtc-
> >crtc_offset, 0);
> +	WREG32(mmGRPH_X_START + amdgpu_crtc->crtc_offset, 0);
> +	WREG32(mmGRPH_Y_START + amdgpu_crtc->crtc_offset, 0);
> +	WREG32(mmGRPH_X_END + amdgpu_crtc->crtc_offset, target_fb-
> >width);
> +	WREG32(mmGRPH_Y_END + amdgpu_crtc->crtc_offset, target_fb-
> >height);
> 
>  	fb_pitch_pixels = target_fb->pitches[0] / (target_fb->bits_per_pixel
> / 8);
> -	WREG32(EVERGREEN_GRPH_PITCH + amdgpu_crtc->crtc_offset,
> fb_pitch_pixels);
> +	WREG32(mmGRPH_PITCH + amdgpu_crtc->crtc_offset,
> fb_pitch_pixels);
> 
>  	dce_v6_0_grph_enable(crtc, true);
> 
> -	WREG32(EVERGREEN_DESKTOP_HEIGHT + amdgpu_crtc-
> >crtc_offset,
> +	WREG32(mmEVERGREEN_DESKTOP_HEIGHT + amdgpu_crtc-
> >crtc_offset,
>  		       target_fb->height);
>  	x &= ~3;
>  	y &= ~1;
> -	WREG32(EVERGREEN_VIEWPORT_START + amdgpu_crtc-
> >crtc_offset,
> +	WREG32(mmVIEWPORT_START + amdgpu_crtc->crtc_offset,
>  	       (x << 16) | y);
>  	viewport_w = crtc->mode.hdisplay;
>  	viewport_h = (crtc->mode.vdisplay + 1) & ~1;
> 
> -	WREG32(EVERGREEN_VIEWPORT_SIZE + amdgpu_crtc->crtc_offset,
> +	WREG32(mmVIEWPORT_SIZE + amdgpu_crtc->crtc_offset,
>  	       (viewport_w << 16) | viewport_h);
> 
>  	/* set pageflip to happen only at start of vblank interval (front porch)
> */
> -	WREG32(EVERGREEN_MASTER_UPDATE_MODE + amdgpu_crtc-
> >crtc_offset, 3);
> +	WREG32(mmMASTER_UPDATE_MODE + amdgpu_crtc->crtc_offset,
> 3);
> 
>  	if (!atomic && fb && fb != crtc->primary->fb) {
>  		amdgpu_fb = to_amdgpu_framebuffer(fb);
> @@ -1667,10 +1682,10 @@ static void dce_v6_0_set_interleave(struct
> drm_crtc *crtc,
>  	struct amdgpu_crtc *amdgpu_crtc = to_amdgpu_crtc(crtc);
> 
>  	if (mode->flags & DRM_MODE_FLAG_INTERLACE)
> -		WREG32(EVERGREEN_DATA_FORMAT + amdgpu_crtc-
> >crtc_offset,
> +		WREG32(mmEVERGREEN_DATA_FORMAT + amdgpu_crtc-
> >crtc_offset,
>  		       EVERGREEN_INTERLEAVE_EN);
>  	else
> -		WREG32(EVERGREEN_DATA_FORMAT + amdgpu_crtc-
> >crtc_offset, 0);
> +		WREG32(mmEVERGREEN_DATA_FORMAT + amdgpu_crtc-
> >crtc_offset, 0);
>  }
> 
>  static void dce_v6_0_crtc_load_lut(struct drm_crtc *crtc)
> @@ -1683,54 +1698,52 @@ static void dce_v6_0_crtc_load_lut(struct
> drm_crtc *crtc)
> 
>  	DRM_DEBUG_KMS("%d\n", amdgpu_crtc->crtc_id);
> 
> -	WREG32(NI_INPUT_CSC_CONTROL + amdgpu_crtc->crtc_offset,
> -	       (NI_INPUT_CSC_GRPH_MODE(NI_INPUT_CSC_BYPASS) |
> -		NI_INPUT_CSC_OVL_MODE(NI_INPUT_CSC_BYPASS)));
> -	WREG32(NI_PRESCALE_GRPH_CONTROL + amdgpu_crtc-
> >crtc_offset,
> -	       NI_GRPH_PRESCALE_BYPASS);
> -	WREG32(NI_PRESCALE_OVL_CONTROL + amdgpu_crtc->crtc_offset,
> -	       NI_OVL_PRESCALE_BYPASS);
> -	WREG32(NI_INPUT_GAMMA_CONTROL + amdgpu_crtc-
> >crtc_offset,
> -
> (NI_GRPH_INPUT_GAMMA_MODE(NI_INPUT_GAMMA_USE_LUT) |
> -
> 	NI_OVL_INPUT_GAMMA_MODE(NI_INPUT_GAMMA_USE_LUT)));
> -
> -
> +	WREG32(mmINPUT_CSC_CONTROL + amdgpu_crtc->crtc_offset,
> +	       ((0 <<
> INPUT_CSC_CONTROL__INPUT_CSC_GRPH_MODE__SHIFT) |
> +		(0 <<
> INPUT_CSC_CONTROL__INPUT_CSC_OVL_MODE__SHIFT)));
> +	WREG32(mmPRESCALE_GRPH_CONTROL + amdgpu_crtc-
> >crtc_offset,
> +	       PRESCALE_GRPH_CONTROL__GRPH_PRESCALE_BYPASS_MASK);
> +	WREG32(mmPRESCALE_OVL_CONTROL + amdgpu_crtc->crtc_offset,
> +	       PRESCALE_OVL_CONTROL__OVL_PRESCALE_BYPASS_MASK);
> +	WREG32(mmINPUT_GAMMA_CONTROL + amdgpu_crtc-
> >crtc_offset,
> +	       ((0 <<
> INPUT_GAMMA_CONTROL__GRPH_INPUT_GAMMA_MODE__SHIFT) |
> +		(0 <<
> INPUT_GAMMA_CONTROL__OVL_INPUT_GAMMA_MODE__SHIFT)));
> 
> -	WREG32(EVERGREEN_DC_LUT_CONTROL + amdgpu_crtc-
> >crtc_offset, 0);
> +	WREG32(mmDC_LUT_CONTROL + amdgpu_crtc->crtc_offset, 0);
> 
> -	WREG32(EVERGREEN_DC_LUT_BLACK_OFFSET_BLUE +
> amdgpu_crtc->crtc_offset, 0);
> -	WREG32(EVERGREEN_DC_LUT_BLACK_OFFSET_GREEN +
> amdgpu_crtc->crtc_offset, 0);
> -	WREG32(EVERGREEN_DC_LUT_BLACK_OFFSET_RED + amdgpu_crtc-
> >crtc_offset, 0);
> +	WREG32(mmDC_LUT_BLACK_OFFSET_BLUE + amdgpu_crtc-
> >crtc_offset, 0);
> +	WREG32(mmDC_LUT_BLACK_OFFSET_GREEN + amdgpu_crtc-
> >crtc_offset, 0);
> +	WREG32(mmDC_LUT_BLACK_OFFSET_RED + amdgpu_crtc-
> >crtc_offset, 0);
> 
> -	WREG32(EVERGREEN_DC_LUT_WHITE_OFFSET_BLUE +
> amdgpu_crtc->crtc_offset, 0xffff);
> -	WREG32(EVERGREEN_DC_LUT_WHITE_OFFSET_GREEN +
> amdgpu_crtc->crtc_offset, 0xffff);
> -	WREG32(EVERGREEN_DC_LUT_WHITE_OFFSET_RED + amdgpu_crtc-
> >crtc_offset, 0xffff);
> +	WREG32(mmDC_LUT_WHITE_OFFSET_BLUE + amdgpu_crtc-
> >crtc_offset, 0xffff);
> +	WREG32(mmDC_LUT_WHITE_OFFSET_GREEN + amdgpu_crtc-
> >crtc_offset, 0xffff);
> +	WREG32(mmDC_LUT_WHITE_OFFSET_RED + amdgpu_crtc-
> >crtc_offset, 0xffff);
> 
> -	WREG32(EVERGREEN_DC_LUT_RW_MODE + amdgpu_crtc-
> >crtc_offset, 0);
> -	WREG32(EVERGREEN_DC_LUT_WRITE_EN_MASK + amdgpu_crtc-
> >crtc_offset, 0x00000007);
> +	WREG32(mmDC_LUT_RW_MODE + amdgpu_crtc->crtc_offset, 0);
> +	WREG32(mmDC_LUT_WRITE_EN_MASK + amdgpu_crtc-
> >crtc_offset, 0x00000007);
> 
> -	WREG32(EVERGREEN_DC_LUT_RW_INDEX + amdgpu_crtc-
> >crtc_offset, 0);
> +	WREG32(mmDC_LUT_RW_INDEX + amdgpu_crtc->crtc_offset, 0);
>  	for (i = 0; i < 256; i++) {
> -		WREG32(EVERGREEN_DC_LUT_30_COLOR + amdgpu_crtc-
> >crtc_offset,
> +		WREG32(mmDC_LUT_30_COLOR + amdgpu_crtc-
> >crtc_offset,
>  		       (amdgpu_crtc->lut_r[i] << 20) |
>  		       (amdgpu_crtc->lut_g[i] << 10) |
>  		       (amdgpu_crtc->lut_b[i] << 0));
>  	}
> 
> -	WREG32(NI_DEGAMMA_CONTROL + amdgpu_crtc->crtc_offset,
> -	       (NI_GRPH_DEGAMMA_MODE(NI_DEGAMMA_BYPASS) |
> -		NI_OVL_DEGAMMA_MODE(NI_DEGAMMA_BYPASS) |
> -		NI_ICON_DEGAMMA_MODE(NI_DEGAMMA_BYPASS) |
> -		NI_CURSOR_DEGAMMA_MODE(NI_DEGAMMA_BYPASS)));
> -	WREG32(NI_GAMUT_REMAP_CONTROL + amdgpu_crtc-
> >crtc_offset,
> -
> (NI_GRPH_GAMUT_REMAP_MODE(NI_GAMUT_REMAP_BYPASS) |
> -
> 	NI_OVL_GAMUT_REMAP_MODE(NI_GAMUT_REMAP_BYPASS)));
> -	WREG32(NI_REGAMMA_CONTROL + amdgpu_crtc->crtc_offset,
> -	       (NI_GRPH_REGAMMA_MODE(NI_REGAMMA_BYPASS) |
> -		NI_OVL_REGAMMA_MODE(NI_REGAMMA_BYPASS)));
> -	WREG32(NI_OUTPUT_CSC_CONTROL + amdgpu_crtc->crtc_offset,
> -	       (NI_OUTPUT_CSC_GRPH_MODE(0) |
> -		NI_OUTPUT_CSC_OVL_MODE(NI_OUTPUT_CSC_BYPASS)));
> +	WREG32(mmDEGAMMA_CONTROL + amdgpu_crtc->crtc_offset,
> +	       ((0 <<
> DEGAMMA_CONTROL__GRPH_DEGAMMA_MODE__SHIFT) |
> +		(0 <<
> DEGAMMA_CONTROL__OVL_DEGAMMA_MODE__SHIFT) |
> +		NI_ICON_DEGAMMA_MODE(0) |
> +		(0 <<
> DEGAMMA_CONTROL__CURSOR_DEGAMMA_MODE__SHIFT)));
> +	WREG32(mmGAMUT_REMAP_CONTROL + amdgpu_crtc-
> >crtc_offset,
> +	       ((0 <<
> GAMUT_REMAP_CONTROL__GRPH_GAMUT_REMAP_MODE__SHIFT) |
> +		(0 <<
> GAMUT_REMAP_CONTROL__OVL_GAMUT_REMAP_MODE__SHIFT)));
> +	WREG32(mmREGAMMA_CONTROL + amdgpu_crtc->crtc_offset,
> +	       ((0 <<
> REGAMMA_CONTROL__GRPH_REGAMMA_MODE__SHIFT) |
> +		(0 <<
> REGAMMA_CONTROL__OVL_REGAMMA_MODE__SHIFT)));
> +	WREG32(mmOUTPUT_CSC_CONTROL + amdgpu_crtc->crtc_offset,
> +	       ((0 <<
> OUTPUT_CSC_CONTROL__OUTPUT_CSC_GRPH_MODE__SHIFT) |
> +		(0 <<
> OUTPUT_CSC_CONTROL__OUTPUT_CSC_OVL_MODE__SHIFT)));
>  	/* XXX match this to the depth of the crtc fmt block, move to
> modeset? */
>  	WREG32(0x1a50 + amdgpu_crtc->crtc_offset, 0);
> 
> @@ -1809,12 +1822,12 @@ static void dce_v6_0_lock_cursor(struct drm_crtc
> *crtc, bool lock)
>  	struct amdgpu_crtc *amdgpu_crtc = to_amdgpu_crtc(crtc);
>  	uint32_t cur_lock;
> 
> -	cur_lock = RREG32(EVERGREEN_CUR_UPDATE + amdgpu_crtc-
> >crtc_offset);
> +	cur_lock = RREG32(mmCUR_UPDATE + amdgpu_crtc->crtc_offset);
>  	if (lock)
> -		cur_lock |= EVERGREEN_CURSOR_UPDATE_LOCK;
> +		cur_lock |= CUR_UPDATE__CURSOR_UPDATE_LOCK_MASK;
>  	else
> -		cur_lock &= ~EVERGREEN_CURSOR_UPDATE_LOCK;
> -	WREG32(EVERGREEN_CUR_UPDATE + amdgpu_crtc->crtc_offset,
> cur_lock);
> +		cur_lock &=
> ~CUR_UPDATE__CURSOR_UPDATE_LOCK_MASK;
> +	WREG32(mmCUR_UPDATE + amdgpu_crtc->crtc_offset, cur_lock);
>  }
> 
>  static void dce_v6_0_hide_cursor(struct drm_crtc *crtc)
> @@ -1822,9 +1835,9 @@ static void dce_v6_0_hide_cursor(struct drm_crtc
> *crtc)
>  	struct amdgpu_crtc *amdgpu_crtc = to_amdgpu_crtc(crtc);
>  	struct amdgpu_device *adev = crtc->dev->dev_private;
> 
> -	WREG32_IDX(EVERGREEN_CUR_CONTROL + amdgpu_crtc-
> >crtc_offset,
> -
> EVERGREEN_CURSOR_MODE(EVERGREEN_CURSOR_24_8_PRE_MULT) |
> -
> EVERGREEN_CURSOR_URGENT_CONTROL(EVERGREEN_CURSOR_URGENT_
> 1_2));
> +	WREG32_IDX(mmCUR_CONTROL + amdgpu_crtc->crtc_offset,
> +		   (EVERGREEN_CURSOR_24_8_PRE_MULT <<
> CUR_CONTROL__CURSOR_MODE__SHIFT) |
> +		   (EVERGREEN_CURSOR_URGENT_1_2 <<
> CUR_CONTROL__CURSOR_URGENT_CONTROL__SHIFT));
> 
> 
>  }
> @@ -1834,15 +1847,15 @@ static void dce_v6_0_show_cursor(struct
> drm_crtc *crtc)
>  	struct amdgpu_crtc *amdgpu_crtc = to_amdgpu_crtc(crtc);
>  	struct amdgpu_device *adev = crtc->dev->dev_private;
> 
> -	WREG32(EVERGREEN_CUR_SURFACE_ADDRESS_HIGH +
> amdgpu_crtc->crtc_offset,
> +	WREG32(mmCUR_SURFACE_ADDRESS_HIGH + amdgpu_crtc-
> >crtc_offset,
>  	       upper_32_bits(amdgpu_crtc->cursor_addr));
> -	WREG32(EVERGREEN_CUR_SURFACE_ADDRESS + amdgpu_crtc-
> >crtc_offset,
> +	WREG32(mmCUR_SURFACE_ADDRESS + amdgpu_crtc->crtc_offset,
>  	       lower_32_bits(amdgpu_crtc->cursor_addr));
> 
> -	WREG32_IDX(EVERGREEN_CUR_CONTROL + amdgpu_crtc-
> >crtc_offset,
> -		   EVERGREEN_CURSOR_EN |
> -
> EVERGREEN_CURSOR_MODE(EVERGREEN_CURSOR_24_8_PRE_MULT) |
> -
> EVERGREEN_CURSOR_URGENT_CONTROL(EVERGREEN_CURSOR_URGENT_
> 1_2));
> +	WREG32_IDX(mmCUR_CONTROL + amdgpu_crtc->crtc_offset,
> +		   CUR_CONTROL__CURSOR_EN_MASK |
> +		   (EVERGREEN_CURSOR_24_8_PRE_MULT <<
> CUR_CONTROL__CURSOR_MODE__SHIFT) |
> +		   (EVERGREEN_CURSOR_URGENT_1_2 <<
> CUR_CONTROL__CURSOR_URGENT_CONTROL__SHIFT));
> 
>  }
> 
> @@ -1869,9 +1882,9 @@ static int dce_v6_0_cursor_move_locked(struct
> drm_crtc *crtc,
>  		y = 0;
>  	}
> 
> -	WREG32(EVERGREEN_CUR_POSITION + amdgpu_crtc->crtc_offset,
> (x << 16) | y);
> -	WREG32(EVERGREEN_CUR_HOT_SPOT + amdgpu_crtc->crtc_offset,
> (xorigin << 16) | yorigin);
> -	WREG32(EVERGREEN_CUR_SIZE + amdgpu_crtc->crtc_offset,
> +	WREG32(mmCUR_POSITION + amdgpu_crtc->crtc_offset, (x << 16) |
> y);
> +	WREG32(mmCUR_HOT_SPOT + amdgpu_crtc->crtc_offset, (xorigin
> << 16) | yorigin);
> +	WREG32(mmCUR_SIZE + amdgpu_crtc->crtc_offset,
>  	       ((w - 1) << 16) | (amdgpu_crtc->cursor_height - 1));
> 
>  	amdgpu_crtc->cursor_x = x;
> @@ -2475,14 +2488,14 @@ static void
> dce_v6_0_set_crtc_vblank_interrupt_state(struct amdgpu_device *adev,
> 
>  	switch (state) {
>  	case AMDGPU_IRQ_STATE_DISABLE:
> -		interrupt_mask = RREG32(INT_MASK + reg_block);
> +		interrupt_mask = RREG32(mmINT_MASK + reg_block);
>  		interrupt_mask &= ~VBLANK_INT_MASK;
> -		WREG32(INT_MASK + reg_block, interrupt_mask);
> +		WREG32(mmINT_MASK + reg_block, interrupt_mask);
>  		break;
>  	case AMDGPU_IRQ_STATE_ENABLE:
> -		interrupt_mask = RREG32(INT_MASK + reg_block);
> +		interrupt_mask = RREG32(mmINT_MASK + reg_block);
>  		interrupt_mask |= VBLANK_INT_MASK;
> -		WREG32(INT_MASK + reg_block, interrupt_mask);
> +		WREG32(mmINT_MASK + reg_block, interrupt_mask);
>  		break;
>  	default:
>  		break;
> @@ -2510,14 +2523,14 @@ static int
> dce_v6_0_set_hpd_interrupt_state(struct amdgpu_device *adev,
> 
>  	switch (state) {
>  	case AMDGPU_IRQ_STATE_DISABLE:
> -		dc_hpd_int_cntl = RREG32(DC_HPD1_INT_CONTROL +
> hpd_offsets[type]);
> +		dc_hpd_int_cntl = RREG32(mmDC_HPD1_INT_CONTROL +
> hpd_offsets[type]);
>  		dc_hpd_int_cntl &= ~DC_HPDx_INT_EN;
> -		WREG32(DC_HPD1_INT_CONTROL + hpd_offsets[type],
> dc_hpd_int_cntl);
> +		WREG32(mmDC_HPD1_INT_CONTROL + hpd_offsets[type],
> dc_hpd_int_cntl);
>  		break;
>  	case AMDGPU_IRQ_STATE_ENABLE:
> -		dc_hpd_int_cntl = RREG32(DC_HPD1_INT_CONTROL +
> hpd_offsets[type]);
> +		dc_hpd_int_cntl = RREG32(mmDC_HPD1_INT_CONTROL +
> hpd_offsets[type]);
>  		dc_hpd_int_cntl |= DC_HPDx_INT_EN;
> -		WREG32(DC_HPD1_INT_CONTROL + hpd_offsets[type],
> dc_hpd_int_cntl);
> +		WREG32(mmDC_HPD1_INT_CONTROL + hpd_offsets[type],
> dc_hpd_int_cntl);
>  		break;
>  	default:
>  		break;
> @@ -2585,7 +2598,7 @@ static int dce_v6_0_crtc_irq(struct amdgpu_device
> *adev,
>  	switch (entry->src_data) {
>  	case 0: /* vblank */
>  		if (disp_int & interrupt_status_offsets[crtc].vblank)
> -			WREG32(VBLANK_STATUS + crtc_offsets[crtc],
> VBLANK_ACK);
> +			WREG32(mmVBLANK_STATUS + crtc_offsets[crtc],
> VBLANK_ACK);
>  		else
>  			DRM_DEBUG("IH: IH event w/o asserted irq bit?\n");
> 
> @@ -2596,7 +2609,7 @@ static int dce_v6_0_crtc_irq(struct amdgpu_device
> *adev,
>  		break;
>  	case 1: /* vline */
>  		if (disp_int & interrupt_status_offsets[crtc].vline)
> -			WREG32(VLINE_STATUS + crtc_offsets[crtc],
> VLINE_ACK);
> +			WREG32(mmVLINE_STATUS + crtc_offsets[crtc],
> VLINE_ACK);
>  		else
>  			DRM_DEBUG("IH: IH event w/o asserted irq bit?\n");
> 
> @@ -2622,12 +2635,12 @@ static int
> dce_v6_0_set_pageflip_interrupt_state(struct amdgpu_device *adev,
>  		return -EINVAL;
>  	}
> 
> -	reg = RREG32(GRPH_INT_CONTROL + crtc_offsets[type]);
> +	reg = RREG32(mmGRPH_INTERRUPT_CONTROL +
> crtc_offsets[type]);
>  	if (state == AMDGPU_IRQ_STATE_DISABLE)
> -		WREG32(GRPH_INT_CONTROL + crtc_offsets[type],
> +		WREG32(mmGRPH_INTERRUPT_CONTROL +
> crtc_offsets[type],
>  		       reg &
> ~GRPH_INTERRUPT_CONTROL__GRPH_PFLIP_INT_MASK_MASK);
>  	else
> -		WREG32(GRPH_INT_CONTROL + crtc_offsets[type],
> +		WREG32(mmGRPH_INTERRUPT_CONTROL +
> crtc_offsets[type],
>  		       reg |
> GRPH_INTERRUPT_CONTROL__GRPH_PFLIP_INT_MASK_MASK);
> 
>  	return 0;
> @@ -2650,9 +2663,9 @@ static int dce_v6_0_pageflip_irq(struct
> amdgpu_device *adev,
>  		return -EINVAL;
>  	}
> 
> -	if (RREG32(GRPH_INT_STATUS + crtc_offsets[crtc_id]) &
> +	if (RREG32(mmGRPH_INTERRUPT_STATUS + crtc_offsets[crtc_id]) &
> 
> GRPH_INTERRUPT_STATUS__GRPH_PFLIP_INT_OCCURRED_MASK)
> -		WREG32(GRPH_INT_STATUS + crtc_offsets[crtc_id],
> +		WREG32(mmGRPH_INTERRUPT_STATUS +
> crtc_offsets[crtc_id],
> 
> GRPH_INTERRUPT_STATUS__GRPH_PFLIP_INT_CLEAR_MASK);
> 
>  	/* IRQ could occur when in initial stage */
> @@ -2703,9 +2716,9 @@ static int dce_v6_0_hpd_irq(struct amdgpu_device
> *adev,
>  	mask = interrupt_status_offsets[hpd].hpd;
> 
>  	if (disp_int & mask) {
> -		tmp = RREG32(DC_HPD1_INT_CONTROL +
> hpd_offsets[hpd]);
> +		tmp = RREG32(mmDC_HPD1_INT_CONTROL +
> hpd_offsets[hpd]);
>  		tmp |=
> DC_HPD1_INT_CONTROL__DC_HPD1_INT_ACK_MASK;
> -		WREG32(DC_HPD1_INT_CONTROL + hpd_offsets[hpd],
> tmp);
> +		WREG32(mmDC_HPD1_INT_CONTROL + hpd_offsets[hpd],
> tmp);
>  		schedule_work(&adev->hotplug_work);
>  		DRM_INFO("IH: HPD%d\n", hpd + 1);
>  	}
> diff --git a/drivers/gpu/drm/amd/amdgpu/si_enums.h
> b/drivers/gpu/drm/amd/amdgpu/si_enums.h
> index 3ecd36f30e2a..a57054fcb448 100644
> --- a/drivers/gpu/drm/amd/amdgpu/si_enums.h
> +++ b/drivers/gpu/drm/amd/amdgpu/si_enums.h
> @@ -23,6 +23,84 @@
>  #ifndef SI_ENUMS_H
>  #define SI_ENUMS_H
> 
> +#define VBLANK_INT_MASK                          (1 << 0)
> +#define DC_HPDx_INT_EN                           (1 << 16)
> +#define VBLANK_ACK                               (1 << 4)
> +#define VLINE_ACK                                (1 << 4)
> +
> +#define CURSOR_WIDTH 64
> +#define CURSOR_HEIGHT 64
> +
> +#define C_000300_VGA_VSTATUS_CNTL                0xFFFCFFFF
> +#define PRIORITY_MARK_MASK                       0x7fff
> +#define PRIORITY_OFF                             (1 << 16)
> +#define PRIORITY_ALWAYS_ON                       (1 << 20)
> +#define EVERGREEN_INTERLEAVE_EN                  (1 << 0)

Drop the EVERGREEN prefix

> +
> +#define LATENCY_WATERMARK_MASK(x)                ((x) << 16)
> +#define DC_LB_MEMORY_CONFIG(x)                   ((x) << 20)
> +#define NI_ICON_DEGAMMA_MODE(x)                  (((x) & 0x3) << 8)

Pleas drop the NI prefix

> +
> +#define EVERGREEN_GRPH_ENDIAN_SWAP(x)            (((x) & 0x3) << 0)
> +#define EVERGREEN_GRPH_ENDIAN_NONE               0
> +#define EVERGREEN_GRPH_ENDIAN_8IN16              1
> +#define EVERGREEN_GRPH_ENDIAN_8IN32              2
> +#define EVERGREEN_GRPH_ENDIAN_8IN64              3
> +
> +#define EVERGREEN_GRPH_DEPTH(x)                  (((x) & 0x3) << 0)
> +#define EVERGREEN_GRPH_DEPTH_8BPP                0
> +#define EVERGREEN_GRPH_DEPTH_16BPP               1
> +#define EVERGREEN_GRPH_DEPTH_32BPP               2
> +
> +#define EVERGREEN_GRPH_FORMAT(x)                 (((x) & 0x7) << 8)
> +#define EVERGREEN_GRPH_FORMAT_INDEXED            0
> +#define EVERGREEN_GRPH_FORMAT_ARGB1555           0
> +#define EVERGREEN_GRPH_FORMAT_ARGB565            1
> +#define EVERGREEN_GRPH_FORMAT_ARGB4444           2
> +#define EVERGREEN_GRPH_FORMAT_AI88               3
> +#define EVERGREEN_GRPH_FORMAT_MONO16             4
> +#define EVERGREEN_GRPH_FORMAT_BGRA5551           5
> +#define EVERGREEN_GRPH_FORMAT_ARGB8888           0
> +#define EVERGREEN_GRPH_FORMAT_ARGB2101010        1
> +#define EVERGREEN_GRPH_FORMAT_32BPP_DIG          2
> +#define EVERGREEN_GRPH_FORMAT_8B_ARGB2101010     3
> +#define EVERGREEN_GRPH_FORMAT_BGRA1010102        4
> +#define EVERGREEN_GRPH_FORMAT_8B_BGRA1010102     5
> +#define EVERGREEN_GRPH_FORMAT_RGB111110          6
> +#define EVERGREEN_GRPH_FORMAT_BGR101111          7
> +
> +#define EVERGREEN_GRPH_NUM_BANKS(x)              (((x) & 0x3) << 2)
> +#define EVERGREEN_GRPH_ARRAY_MODE(x)             (((x) & 0x7) << 20)
> +#define EVERGREEN_GRPH_ARRAY_LINEAR_GENERAL      0
> +#define EVERGREEN_GRPH_ARRAY_LINEAR_ALIGNED      1
> +#define EVERGREEN_GRPH_ARRAY_1D_TILED_THIN1      2
> +#define EVERGREEN_GRPH_ARRAY_2D_TILED_THIN1      4
> +#define EVERGREEN_GRPH_TILE_SPLIT(x)             (((x) & 0x7) << 13)
> +#define EVERGREEN_GRPH_BANK_WIDTH(x)             (((x) & 0x3) << 6)
> +#define EVERGREEN_GRPH_BANK_HEIGHT(x)            (((x) & 0x3) << 11)
> +#define EVERGREEN_GRPH_MACRO_TILE_ASPECT(x)      (((x) & 0x3) << 18)
> +#define EVERGREEN_GRPH_ARRAY_MODE(x)             (((x) & 0x7) << 20)

Please drop the EVERGREEN prefix

> +#define SI_GRPH_PIPE_CONFIG(x)                   (((x) & 0x1f) << 24)

And SI prefix

> +
> +#define EVERGREEN_CURSOR_EN                      (1 << 0)
> +#define EVERGREEN_CURSOR_MODE(x)                 (((x) & 0x3) << 8)
> +#define EVERGREEN_CURSOR_MONO                    0
> +#define EVERGREEN_CURSOR_24_1                    1
> +#define EVERGREEN_CURSOR_24_8_PRE_MULT           2
> +#define EVERGREEN_CURSOR_24_8_UNPRE_MULT         3
> +#define EVERGREEN_CURSOR_2X_MAGNIFY              (1 << 16)
> +#define EVERGREEN_CURSOR_FORCE_MC_ON             (1 << 20)
> +#define EVERGREEN_CURSOR_URGENT_CONTROL(x)       (((x) & 0x7) << 24)
> +#define EVERGREEN_CURSOR_URGENT_ALWAYS           0
> +#define EVERGREEN_CURSOR_URGENT_1_8              1
> +#define EVERGREEN_CURSOR_URGENT_1_4              2
> +#define EVERGREEN_CURSOR_URGENT_3_8              3
> +#define EVERGREEN_CURSOR_URGENT_1_2              4
> +#define EVERGREEN_CURSOR_UPDATE_PENDING          (1 << 0)
> +#define EVERGREEN_CURSOR_UPDATE_TAKEN            (1 << 1)
> +#define EVERGREEN_CURSOR_UPDATE_LOCK             (1 << 16)
> +#define EVERGREEN_CURSOR_DISABLE_MULTIPLE_UPDATE (1 << 24)
> +

Drop the EVERGREEN prefix

>  #define AMDGPU_NUM_OF_VMIDS                     8
>  #define SI_CRTC0_REGISTER_OFFSET                0
>  #define SI_CRTC1_REGISTER_OFFSET                0x300
> @@ -68,127 +146,128 @@
>  #define VERDE_GB_ADDR_CONFIG_GOLDEN         0x12010002
>  #define HAINAN_GB_ADDR_CONFIG_GOLDEN        0x02010001
> 
> -#define PACKET3(op, n)  ((RADEON_PACKET_TYPE3 << 30) |                  \
> -                         (((op) & 0xFF) << 8) |                         \
> +#define PACKET3(op, n)  ((RADEON_PACKET_TYPE3 << 30) | \
> +                         (((op) & 0xFF) << 8) |        \
>                           ((n) & 0x3FFF) << 16)
> +
>  #define PACKET3_COMPUTE(op, n) (PACKET3(op, n) | 1 << 1)
> -#define	PACKET3_NOP					0x10
> -#define	PACKET3_SET_BASE				0x11
> -#define		PACKET3_BASE_INDEX(x)                  ((x) << 0)
> -#define	PACKET3_CLEAR_STATE				0x12
> -#define	PACKET3_INDEX_BUFFER_SIZE			0x13
> -#define	PACKET3_DISPATCH_DIRECT				0x15
> -#define	PACKET3_DISPATCH_INDIRECT			0x16
> -#define	PACKET3_ALLOC_GDS				0x1B
> -#define	PACKET3_WRITE_GDS_RAM				0x1C
> -#define	PACKET3_ATOMIC_GDS				0x1D
> -#define	PACKET3_ATOMIC					0x1E
> -#define	PACKET3_OCCLUSION_QUERY				0x1F
> -#define	PACKET3_SET_PREDICATION				0x20
> -#define	PACKET3_REG_RMW					0x21
> -#define	PACKET3_COND_EXEC				0x22
> -#define	PACKET3_PRED_EXEC				0x23
> -#define	PACKET3_DRAW_INDIRECT				0x24
> -#define	PACKET3_DRAW_INDEX_INDIRECT			0x25
> -#define	PACKET3_INDEX_BASE				0x26
> -#define	PACKET3_DRAW_INDEX_2				0x27
> -#define	PACKET3_CONTEXT_CONTROL				0x28
> -#define	PACKET3_INDEX_TYPE				0x2A
> -#define	PACKET3_DRAW_INDIRECT_MULTI			0x2C
> -#define	PACKET3_DRAW_INDEX_AUTO
> 	0x2D
> -#define	PACKET3_DRAW_INDEX_IMMD
> 	0x2E
> -#define	PACKET3_NUM_INSTANCES				0x2F
> -#define	PACKET3_DRAW_INDEX_MULTI_AUTO
> 	0x30
> -#define	PACKET3_INDIRECT_BUFFER_CONST			0x31
> -#define	PACKET3_INDIRECT_BUFFER				0x3F
> -#define	PACKET3_STRMOUT_BUFFER_UPDATE			0x34
> -#define	PACKET3_DRAW_INDEX_OFFSET_2			0x35
> -#define	PACKET3_DRAW_INDEX_MULTI_ELEMENT		0x36
> -#define	PACKET3_WRITE_DATA				0x37
> -#define	PACKET3_DRAW_INDEX_INDIRECT_MULTI		0x38
> -#define	PACKET3_MEM_SEMAPHORE				0x39
> -#define	PACKET3_MPEG_INDEX				0x3A
> -#define	PACKET3_COPY_DW					0x3B
> -#define	PACKET3_WAIT_REG_MEM				0x3C
> -#define	PACKET3_MEM_WRITE				0x3D
> -#define	PACKET3_COPY_DATA				0x40
> -#define	PACKET3_CP_DMA					0x41
> -#              define PACKET3_CP_DMA_DST_SEL(x)    ((x) << 20)
> -#              define PACKET3_CP_DMA_ENGINE(x)     ((x) << 27)
> -#              define PACKET3_CP_DMA_SRC_SEL(x)    ((x) << 29)
> -#              define PACKET3_CP_DMA_CP_SYNC       (1 << 31)
> -#              define PACKET3_CP_DMA_DIS_WC        (1 << 21)
> -#              define PACKET3_CP_DMA_CMD_SRC_SWAP(x) ((x) << 22)
> -#              define PACKET3_CP_DMA_CMD_DST_SWAP(x) ((x) << 24)
> -#              define PACKET3_CP_DMA_CMD_SAS       (1 << 26)
> -#              define PACKET3_CP_DMA_CMD_DAS       (1 << 27)
> -#              define PACKET3_CP_DMA_CMD_SAIC      (1 << 28)
> -#              define PACKET3_CP_DMA_CMD_DAIC      (1 << 29)
> -#              define PACKET3_CP_DMA_CMD_RAW_WAIT  (1 << 30)
> -#define	PACKET3_PFP_SYNC_ME				0x42
> -#define	PACKET3_SURFACE_SYNC				0x43
> -#              define PACKET3_DEST_BASE_0_ENA      (1 << 0)
> -#              define PACKET3_DEST_BASE_1_ENA      (1 << 1)
> -#              define PACKET3_CB0_DEST_BASE_ENA    (1 << 6)
> -#              define PACKET3_CB1_DEST_BASE_ENA    (1 << 7)
> -#              define PACKET3_CB2_DEST_BASE_ENA    (1 << 8)
> -#              define PACKET3_CB3_DEST_BASE_ENA    (1 << 9)
> -#              define PACKET3_CB4_DEST_BASE_ENA    (1 << 10)
> -#              define PACKET3_CB5_DEST_BASE_ENA    (1 << 11)
> -#              define PACKET3_CB6_DEST_BASE_ENA    (1 << 12)
> -#              define PACKET3_CB7_DEST_BASE_ENA    (1 << 13)
> -#              define PACKET3_DB_DEST_BASE_ENA     (1 << 14)
> -#              define PACKET3_DEST_BASE_2_ENA      (1 << 19)
> -#              define PACKET3_DEST_BASE_3_ENA      (1 << 21)
> -#              define PACKET3_TCL1_ACTION_ENA      (1 << 22)
> -#              define PACKET3_TC_ACTION_ENA        (1 << 23)
> -#              define PACKET3_CB_ACTION_ENA        (1 << 25)
> -#              define PACKET3_DB_ACTION_ENA        (1 << 26)
> -#              define PACKET3_SH_KCACHE_ACTION_ENA (1 << 27)
> -#              define PACKET3_SH_ICACHE_ACTION_ENA (1 << 29)
> -#define	PACKET3_ME_INITIALIZE				0x44
> -#define		PACKET3_ME_INITIALIZE_DEVICE_ID(x) ((x) << 16)
> -#define	PACKET3_COND_WRITE				0x45
> -#define	PACKET3_EVENT_WRITE				0x46
> -#define	PACKET3_EVENT_WRITE_EOP				0x47
> -#define	PACKET3_EVENT_WRITE_EOS				0x48
> -#define	PACKET3_PREAMBLE_CNTL				0x4A
> -#              define PACKET3_PREAMBLE_BEGIN_CLEAR_STATE     (2 << 28)
> -#              define PACKET3_PREAMBLE_END_CLEAR_STATE       (3 << 28)
> -#define	PACKET3_ONE_REG_WRITE				0x57
> -#define	PACKET3_LOAD_CONFIG_REG				0x5F
> -#define	PACKET3_LOAD_CONTEXT_REG			0x60
> -#define	PACKET3_LOAD_SH_REG				0x61
> -#define	PACKET3_SET_CONFIG_REG				0x68
> -#define		PACKET3_SET_CONFIG_REG_START
> 	0x00002000
> -#define		PACKET3_SET_CONFIG_REG_END
> 	0x00002c00
> -#define	PACKET3_SET_CONTEXT_REG				0x69
> -#define		PACKET3_SET_CONTEXT_REG_START
> 	0x000a000
> -#define		PACKET3_SET_CONTEXT_REG_END
> 	0x000a400
> -#define	PACKET3_SET_CONTEXT_REG_INDIRECT		0x73
> -#define	PACKET3_SET_RESOURCE_INDIRECT			0x74
> -#define	PACKET3_SET_SH_REG				0x76
> -#define		PACKET3_SET_SH_REG_START
> 	0x00002c00
> -#define		PACKET3_SET_SH_REG_END
> 	0x00003000
> -#define	PACKET3_SET_SH_REG_OFFSET			0x77
> -#define	PACKET3_ME_WRITE				0x7A
> -#define	PACKET3_SCRATCH_RAM_WRITE			0x7D
> -#define	PACKET3_SCRATCH_RAM_READ			0x7E
> -#define	PACKET3_CE_WRITE				0x7F
> -#define	PACKET3_LOAD_CONST_RAM				0x80
> -#define	PACKET3_WRITE_CONST_RAM				0x81
> -#define	PACKET3_WRITE_CONST_RAM_OFFSET
> 	0x82
> -#define	PACKET3_DUMP_CONST_RAM				0x83
> -#define	PACKET3_INCREMENT_CE_COUNTER			0x84
> -#define	PACKET3_INCREMENT_DE_COUNTER			0x85
> -#define	PACKET3_WAIT_ON_CE_COUNTER			0x86
> -#define	PACKET3_WAIT_ON_DE_COUNTER			0x87
> -#define	PACKET3_WAIT_ON_DE_COUNTER_DIFF
> 	0x88
> -#define	PACKET3_SET_CE_DE_COUNTERS			0x89
> -#define	PACKET3_WAIT_ON_AVAIL_BUFFER			0x8A
> -#define	PACKET3_SWITCH_BUFFER				0x8B
> -#define PACKET3_SEM_WAIT_ON_SIGNAL    (0x1 << 12)
> -#define PACKET3_SEM_SEL_SIGNAL	    (0x6 << 29)
> -#define PACKET3_SEM_SEL_WAIT	    (0x7 << 29)
> +#define PACKET3_NOP                                    0x10
> +#define PACKET3_SET_BASE                               0x11
> +#define PACKET3_BASE_INDEX(x)                          ((x) << 0)
> +#define PACKET3_CLEAR_STATE                            0x12
> +#define PACKET3_INDEX_BUFFER_SIZE                      0x13
> +#define PACKET3_DISPATCH_DIRECT                        0x15
> +#define PACKET3_DISPATCH_INDIRECT                      0x16
> +#define PACKET3_ALLOC_GDS                              0x1B
> +#define PACKET3_WRITE_GDS_RAM                          0x1C
> +#define PACKET3_ATOMIC_GDS                             0x1D
> +#define PACKET3_ATOMIC                                 0x1E
> +#define PACKET3_OCCLUSION_QUERY                        0x1F
> +#define PACKET3_SET_PREDICATION                        0x20
> +#define PACKET3_REG_RMW                                0x21
> +#define PACKET3_COND_EXEC                              0x22
> +#define PACKET3_PRED_EXEC                              0x23
> +#define PACKET3_DRAW_INDIRECT                          0x24
> +#define PACKET3_DRAW_INDEX_INDIRECT                    0x25
> +#define PACKET3_INDEX_BASE                             0x26
> +#define PACKET3_DRAW_INDEX_2                           0x27
> +#define PACKET3_CONTEXT_CONTROL                        0x28
> +#define PACKET3_INDEX_TYPE                             0x2A
> +#define PACKET3_DRAW_INDIRECT_MULTI                    0x2C
> +#define PACKET3_DRAW_INDEX_AUTO                        0x2D
> +#define PACKET3_DRAW_INDEX_IMMD                        0x2E
> +#define PACKET3_NUM_INSTANCES                          0x2F
> +#define PACKET3_DRAW_INDEX_MULTI_AUTO                  0x30
> +#define PACKET3_INDIRECT_BUFFER_CONST                  0x31
> +#define PACKET3_INDIRECT_BUFFER                        0x3F
> +#define PACKET3_STRMOUT_BUFFER_UPDATE                  0x34
> +#define PACKET3_DRAW_INDEX_OFFSET_2                    0x35
> +#define PACKET3_DRAW_INDEX_MULTI_ELEMENT               0x36
> +#define PACKET3_WRITE_DATA                             0x37
> +#define PACKET3_DRAW_INDEX_INDIRECT_MULTI              0x38
> +#define PACKET3_MEM_SEMAPHORE                          0x39
> +#define PACKET3_MPEG_INDEX                             0x3A
> +#define PACKET3_COPY_DW                                0x3B
> +#define PACKET3_WAIT_REG_MEM                           0x3C
> +#define PACKET3_MEM_WRITE                              0x3D
> +#define PACKET3_COPY_DATA                              0x40
> +#define PACKET3_CP_DMA                                 0x41
> +#define PACKET3_CP_DMA_DST_SEL(x)                      ((x) << 20)
> +#define PACKET3_CP_DMA_ENGINE(x)                       ((x) << 27)
> +#define PACKET3_CP_DMA_SRC_SEL(x)                      ((x) << 29)
> +#define PACKET3_CP_DMA_CP_SYNC                         (1 << 31)
> +#define PACKET3_CP_DMA_DIS_WC                          (1 << 21)
> +#define PACKET3_CP_DMA_CMD_SRC_SWAP(x)                 ((x) << 22)
> +#define PACKET3_CP_DMA_CMD_DST_SWAP(x)                 ((x) << 24)
> +#define PACKET3_CP_DMA_CMD_SAS                         (1 << 26)
> +#define PACKET3_CP_DMA_CMD_DAS                         (1 << 27)
> +#define PACKET3_CP_DMA_CMD_SAIC                        (1 << 28)
> +#define PACKET3_CP_DMA_CMD_DAIC                        (1 << 29)
> +#define PACKET3_CP_DMA_CMD_RAW_WAIT                    (1 << 30)
> +#define PACKET3_PFP_SYNC_ME                            0x42
> +#define PACKET3_SURFACE_SYNC                           0x43
> +#define PACKET3_DEST_BASE_0_ENA                        (1 << 0)
> +#define PACKET3_DEST_BASE_1_ENA                        (1 << 1)
> +#define PACKET3_CB0_DEST_BASE_ENA                      (1 << 6)
> +#define PACKET3_CB1_DEST_BASE_ENA                      (1 << 7)
> +#define PACKET3_CB2_DEST_BASE_ENA                      (1 << 8)
> +#define PACKET3_CB3_DEST_BASE_ENA                      (1 << 9)
> +#define PACKET3_CB4_DEST_BASE_ENA                      (1 << 10)
> +#define PACKET3_CB5_DEST_BASE_ENA                      (1 << 11)
> +#define PACKET3_CB6_DEST_BASE_ENA                      (1 << 12)
> +#define PACKET3_CB7_DEST_BASE_ENA                      (1 << 13)
> +#define PACKET3_DB_DEST_BASE_ENA                       (1 << 14)
> +#define PACKET3_DEST_BASE_2_ENA                        (1 << 19)
> +#define PACKET3_DEST_BASE_3_ENA                        (1 << 21)
> +#define PACKET3_TCL1_ACTION_ENA                        (1 << 22)
> +#define PACKET3_TC_ACTION_ENA                          (1 << 23)
> +#define PACKET3_CB_ACTION_ENA                          (1 << 25)
> +#define PACKET3_DB_ACTION_ENA                          (1 << 26)
> +#define PACKET3_SH_KCACHE_ACTION_ENA                   (1 << 27)
> +#define PACKET3_SH_ICACHE_ACTION_ENA                   (1 << 29)
> +#define PACKET3_ME_INITIALIZE                          0x44
> +#define PACKET3_ME_INITIALIZE_DEVICE_ID(x)             ((x) << 16)
> +#define PACKET3_COND_WRITE                             0x45
> +#define PACKET3_EVENT_WRITE                            0x46
> +#define PACKET3_EVENT_WRITE_EOP                        0x47
> +#define PACKET3_EVENT_WRITE_EOS                        0x48
> +#define PACKET3_PREAMBLE_CNTL                          0x4A
> +#define PACKET3_PREAMBLE_BEGIN_CLEAR_STATE             (2 << 28)
> +#define PACKET3_PREAMBLE_END_CLEAR_STATE               (3 << 28)
> +#define PACKET3_ONE_REG_WRITE                          0x57
> +#define PACKET3_LOAD_CONFIG_REG                        0x5F
> +#define PACKET3_LOAD_CONTEXT_REG                       0x60
> +#define PACKET3_LOAD_SH_REG                            0x61
> +#define PACKET3_SET_CONFIG_REG                         0x68
> +#define PACKET3_SET_CONFIG_REG_START                   0x00002000
> +#define PACKET3_SET_CONFIG_REG_END                     0x00002c00
> +#define PACKET3_SET_CONTEXT_REG                        0x69
> +#define PACKET3_SET_CONTEXT_REG_START                  0x000a000
> +#define PACKET3_SET_CONTEXT_REG_END                    0x000a400
> +#define PACKET3_SET_CONTEXT_REG_INDIRECT               0x73
> +#define PACKET3_SET_RESOURCE_INDIRECT                  0x74
> +#define PACKET3_SET_SH_REG                             0x76
> +#define PACKET3_SET_SH_REG_START                       0x00002c00
> +#define PACKET3_SET_SH_REG_END                         0x00003000
> +#define PACKET3_SET_SH_REG_OFFSET                      0x77
> +#define PACKET3_ME_WRITE                               0x7A
> +#define PACKET3_SCRATCH_RAM_WRITE                      0x7D
> +#define PACKET3_SCRATCH_RAM_READ                       0x7E
> +#define PACKET3_CE_WRITE                               0x7F
> +#define PACKET3_LOAD_CONST_RAM                         0x80
> +#define PACKET3_WRITE_CONST_RAM                        0x81
> +#define PACKET3_WRITE_CONST_RAM_OFFSET                 0x82
> +#define PACKET3_DUMP_CONST_RAM                         0x83
> +#define PACKET3_INCREMENT_CE_COUNTER                   0x84
> +#define PACKET3_INCREMENT_DE_COUNTER                   0x85
> +#define PACKET3_WAIT_ON_CE_COUNTER                     0x86
> +#define PACKET3_WAIT_ON_DE_COUNTER                     0x87
> +#define PACKET3_WAIT_ON_DE_COUNTER_DIFF                0x88
> +#define PACKET3_SET_CE_DE_COUNTERS                     0x89
> +#define PACKET3_WAIT_ON_AVAIL_BUFFER                   0x8A
> +#define PACKET3_SWITCH_BUFFER                          0x8B
> +#define PACKET3_SEM_WAIT_ON_SIGNAL                     (0x1 << 12)
> +#define PACKET3_SEM_SEL_SIGNAL                         (0x6 << 29)
> +#define PACKET3_SEM_SEL_WAIT                           (0x7 << 29)
> 

All of these CP packet changes seem unrelated.  Please split them out into a separate patch.

>  #endif
> diff --git a/drivers/gpu/drm/amd/include/asic_reg/dce/dce_6_0_d.h
> b/drivers/gpu/drm/amd/include/asic_reg/dce/dce_6_0_d.h
> index a17973bb63a6..3e8f576d2a14 100644
> --- a/drivers/gpu/drm/amd/include/asic_reg/dce/dce_6_0_d.h
> +++ b/drivers/gpu/drm/amd/include/asic_reg/dce/dce_6_0_d.h
> @@ -4442,4 +4442,25 @@
>  #define mmXDMA_TEST_DEBUG_DATA 0x041D
>  #define mmXDMA_TEST_DEBUG_INDEX 0x041C
> 
> +/* Registers that spilled out of sid.h */
> +#define mmAVIVO_D1VGA_CONTROL                      0xcc
> +#define mmAVIVO_D2VGA_CONTROL                      0xce

Drop the AVIVO prefix

> +#define mmEVERGREEN_VGA_MEMORY_BASE_ADDRESS_HIGH   0xc9
> +#define mmEVERGREEN_VGA_MEMORY_BASE_ADDRESS        0xc4

Drop the EVERGREEN prefix

> +#define mmR_000300_VGA_RENDER_CONTROL              0xc0

Drop the R_0003000 prefix

> +#define mmEVERGREEN_D3VGA_CONTROL                  0xf8
> +#define mmEVERGREEN_D4VGA_CONTROL                  0xf9
> +#define mmEVERGREEN_D5VGA_CONTROL                  0xfa
> +#define mmEVERGREEN_D6VGA_CONTROL                  0xfb
> +#define mmEVERGREEN_DATA_FORMAT                    0x1AC0
> +#define mmEVERGREEN_DESKTOP_HEIGHT                 0x1AC1

Drop the EVERGREEN prefix

> +#define mmDC_LB_MEMORY_SPLIT                       0x1AC3
> +#define mmPRIORITY_A_CNT                           0x1AC6
> +#define mmPRIORITY_B_CNT                           0x1AC7
> +#define mmDPG_PIPE_ARBITRATION_CONTROL3            0x1B32
> +#define mmINT_MASK                                 0x1AD0
> +#define mmVLINE_STATUS                             0x1AEE
> +#define mmVBLANK_STATUS                            0x1AEF
> +
> +
>  #endif
> --
> 2.10.0
> 
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

^ permalink raw reply

* Re: [PATCH 3.16 000/346] 3.16.39-rc1 review
From: Ben Hutchings @ 2016-11-14 17:10 UTC (permalink / raw)
  To: Guenter Roeck, linux-kernel, stable; +Cc: torvalds, akpm
In-Reply-To: <9a9de2e3-1aa4-a08b-6193-5459a52307e8@roeck-us.net>

[-- Attachment #1: Type: text/plain, Size: 802 bytes --]

On Sun, 2016-11-13 at 21:49 -0800, Guenter Roeck wrote:
> On 11/13/2016 04:14 PM, Ben Hutchings wrote:
> > This is the start of the stable review cycle for the 3.16.39
> > release.
> > There are 346 patches in this series, which will be posted as
> > responses
> > to this one.  If anyone has any issues with these being applied,
> > please
> > let me know.
> > 
> > Responses should be made by Sat Nov 10 00:00:00 UTC 2016.
> > Anything received after that time might be too late.
> > 
> 
> Build results:
> 	total: 140 pass: 140 fail: 0
> Qemu test results:
> 	total: 99 pass: 99 fail: 0
> 
> Details are available at http://kerneltests.org/builders.

Thanks for checking.

Ben.

-- 
Ben Hutchings
If more than one person is responsible for a bug, no one is at fault.


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

^ permalink raw reply

* [PATCH 2/2] ARM: dts: vf610-zii-dev: Add .dts file for rev. C
From: Andrew Lunn @ 2016-11-14 17:10 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <1479141306-15141-2-git-send-email-andrew.smirnov@gmail.com>

> +		mdio_mux_1: mdio at 1 {
> +			reg = <1>;
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			switch0: switch0 at 0 {
> +				compatible = "marvell,mv88e6390";

Hi Andrey

The driver for this is not in net-next yet. And when it is, it will
probably be called "marvell,mv88e6190", keeping to the pattern of the
lowest product ID which supports these features.

> +					port at 4 {
> +						reg = <4>;
> +						label = "lan4";
> +					};
> +
> +					port at 9 {
> +						reg = <9>;
> +						label = "lan4";
> +						phy-handle = <&switch0phy0>;
> +					};
> +

You have two "lan4". I would also suggest leaving port 9 out for the
moment. It needs clause 45 MDIO to talk to the PHY, which we don't
have yet. Hence it cannot find it, and so give an error.

> +
> +					switch0port10: port at 10 {
> +						reg = <10>;
> +						label = "dsa";
> +						phy-mode = "xgmii";
> +						link = <&switch1port10>;
> +						fixed-link {
> +							speed = <10000>;
> +							full-duplex;
> +						};

This fixed-link node is wrong, and invalid. 10000 is not supported by
the fixed link driver, only 10, 100, and 1000. Also, it is not
required. The DSA driver should configure the link to the fastest
possible speed the port supports. You only need a fixed-link property
when you need to configure it at a lower speed. Rev B also gets this
wrong.

> +					};
> +				};
> +
> +				mdio {
> +					#address-cells = <1>;
> +					#size-cells = <0>;
> +					switch0phy0: switch0phy0 at 0 {
> +						reg = <0>;
> +					};

I think the strapping for the PHY is such that it is at address 9.
Also, it is on the external mdio bus, not the internal mdio bus. The
6390 family has two MDIO busses. I have patches to support this, which
will appear eventually.

  Andrew

^ permalink raw reply

* Re: [PATCH 2/2] ARM: dts: vf610-zii-dev: Add .dts file for rev. C
From: Andrew Lunn @ 2016-11-14 17:10 UTC (permalink / raw)
  To: Andrey Smirnov
  Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Shawn Guo,
	Rob Herring, Mark Rutland, Russell King, Sascha Hauer,
	Stefan Agner, devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	cphealy-Re5JQEeQqe8AvxtiuMwx3w
In-Reply-To: <1479141306-15141-2-git-send-email-andrew.smirnov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

> +		mdio_mux_1: mdio@1 {
> +			reg = <1>;
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			switch0: switch0@0 {
> +				compatible = "marvell,mv88e6390";

Hi Andrey

The driver for this is not in net-next yet. And when it is, it will
probably be called "marvell,mv88e6190", keeping to the pattern of the
lowest product ID which supports these features.

> +					port@4 {
> +						reg = <4>;
> +						label = "lan4";
> +					};
> +
> +					port@9 {
> +						reg = <9>;
> +						label = "lan4";
> +						phy-handle = <&switch0phy0>;
> +					};
> +

You have two "lan4". I would also suggest leaving port 9 out for the
moment. It needs clause 45 MDIO to talk to the PHY, which we don't
have yet. Hence it cannot find it, and so give an error.

> +
> +					switch0port10: port@10 {
> +						reg = <10>;
> +						label = "dsa";
> +						phy-mode = "xgmii";
> +						link = <&switch1port10>;
> +						fixed-link {
> +							speed = <10000>;
> +							full-duplex;
> +						};

This fixed-link node is wrong, and invalid. 10000 is not supported by
the fixed link driver, only 10, 100, and 1000. Also, it is not
required. The DSA driver should configure the link to the fastest
possible speed the port supports. You only need a fixed-link property
when you need to configure it at a lower speed. Rev B also gets this
wrong.

> +					};
> +				};
> +
> +				mdio {
> +					#address-cells = <1>;
> +					#size-cells = <0>;
> +					switch0phy0: switch0phy0@0 {
> +						reg = <0>;
> +					};

I think the strapping for the PHY is such that it is at address 9.
Also, it is on the external mdio bus, not the internal mdio bus. The
6390 family has two MDIO busses. I have patches to support this, which
will appear eventually.

  Andrew
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* [PATCH v6 5/9] drm/hisilicon/hibmc: Add crtc for DE
From: Sean Paul @ 2016-11-14 17:10 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <5826ECA6.6030900@huawei.com>

On Sat, Nov 12, 2016 at 5:19 AM, Rongrong Zou <zourongrong@huawei.com> wrote:
> ? 2016/11/11 6:14, Sean Paul ??:
>>
>> On Fri, Oct 28, 2016 at 3:27 AM, Rongrong Zou <zourongrong@gmail.com>
>> wrote:
>>>
>>> Add crtc funcs and helper funcs for DE.
>>>
>>> Signed-off-by: Rongrong Zou <zourongrong@gmail.com>
>>> ---
>>>   drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c  | 318
>>> ++++++++++++++++++++++++
>>>   drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c |   6 +
>>>   drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h |   2 +
>>>   3 files changed, 326 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
>>> b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
>>> index 9c1a68c..9b5d0d0 100644
>>> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
>>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
>>> @@ -23,6 +23,7 @@
>>>
>>>   #include "hibmc_drm_drv.h"
>>>   #include "hibmc_drm_regs.h"
>>> +#include "hibmc_drm_de.h"
>>>   #include "hibmc_drm_power.h"
>>
>>
>> nit: alphabetize
>
>
> ok, thanks.
>
>>
>>>
>>>   /*
>>> ---------------------------------------------------------------------- */
>>
>>
>> Remove
>
>
> will do, thanks.
>
>>
>>> @@ -168,3 +169,320 @@ int hibmc_plane_init(struct hibmc_drm_device
>>> *hidev)
>>>          drm_plane_helper_add(plane, &hibmc_plane_helper_funcs);
>>>          return 0;
>>>   }
>>> +
>>> +static void hibmc_crtc_enable(struct drm_crtc *crtc)
>>> +{
>>> +       unsigned int reg;
>>> +       /* power mode 0 is default. */
>>
>>
>> This comment seems to be in the wrong place
>
>
> will remove it, thanks.
>
>
>>
>>> +       struct hibmc_drm_device *hidev = crtc->dev->dev_private;
>>> +
>>> +       hibmc_set_power_mode(hidev, HIBMC_PW_MODE_CTL_MODE_MODE0);
>>> +
>>> +       /* Enable display power gate & LOCALMEM power gate*/
>>> +       reg = readl(hidev->mmio + HIBMC_CURRENT_GATE);
>>> +       reg &= ~HIBMC_CURR_GATE_LOCALMEM_MASK;
>>> +       reg &= ~HIBMC_CURR_GATE_DISPLAY_MASK;
>>> +       reg |= HIBMC_CURR_GATE_LOCALMEM(ON);
>>> +       reg |= HIBMC_CURR_GATE_DISPLAY(ON);
>>> +       hibmc_set_current_gate(hidev, reg);
>>> +       drm_crtc_vblank_on(crtc);
>>> +}
>>> +
>>> +static void hibmc_crtc_disable(struct drm_crtc *crtc)
>>> +{
>>> +       unsigned int reg;
>>> +       struct hibmc_drm_device *hidev = crtc->dev->dev_private;
>>> +
>>> +       drm_crtc_vblank_off(crtc);
>>> +
>>> +       hibmc_set_power_mode(hidev, HIBMC_PW_MODE_CTL_MODE_SLEEP);
>>> +
>>> +       /* Enable display power gate & LOCALMEM power gate*/
>>> +       reg = readl(hidev->mmio + HIBMC_CURRENT_GATE);
>>> +       reg &= ~HIBMC_CURR_GATE_LOCALMEM_MASK;
>>> +       reg &= ~HIBMC_CURR_GATE_DISPLAY_MASK;
>>> +       reg |= HIBMC_CURR_GATE_LOCALMEM(OFF);
>>> +       reg |= HIBMC_CURR_GATE_DISPLAY(OFF);
>>> +       hibmc_set_current_gate(hidev, reg);
>>> +}
>>> +
>>> +static int hibmc_crtc_atomic_check(struct drm_crtc *crtc,
>>> +                                  struct drm_crtc_state *state)
>>> +{
>>> +       return 0;
>>> +}
>>
>>
>> Caller NULL-checks, no need for stub
>
>
> thanks for pointing it out.
>
>
>>
>>> +
>>> +static unsigned int format_pll_reg(void)
>>> +{
>>> +       unsigned int pllreg = 0;
>>> +       struct panel_pll pll = {0};
>>> +
>>> +       /* Note that all PLL's have the same format. Here,
>>> +        * we just use Panel PLL parameter to work out the bit
>>> +        * fields in the register.On returning a 32 bit number, the value
>>> can
>>> +        * be applied to any PLL in the calling function.
>>> +        */
>>> +       pllreg |= HIBMC_PLL_CTRL_BYPASS(OFF) &
>>> HIBMC_PLL_CTRL_BYPASS_MASK;
>>> +       pllreg |= HIBMC_PLL_CTRL_POWER(ON) & HIBMC_PLL_CTRL_POWER_MASK;
>>> +       pllreg |= HIBMC_PLL_CTRL_INPUT(OSC) & HIBMC_PLL_CTRL_INPUT_MASK;
>>> +       pllreg |= HIBMC_PLL_CTRL_POD(pll.POD) & HIBMC_PLL_CTRL_POD_MASK;
>>> +       pllreg |= HIBMC_PLL_CTRL_OD(pll.OD) & HIBMC_PLL_CTRL_OD_MASK;
>>> +       pllreg |= HIBMC_PLL_CTRL_N(pll.N) & HIBMC_PLL_CTRL_N_MASK;
>>> +       pllreg |= HIBMC_PLL_CTRL_M(pll.M) & HIBMC_PLL_CTRL_M_MASK;
>>> +
>>> +       return pllreg;
>>> +}
>>> +
>>> +static void set_vclock_hisilicon(struct drm_device *dev, unsigned long
>>> pll)
>>> +{
>>> +       unsigned long tmp0, tmp1;
>>> +       struct hibmc_drm_device *hidev = dev->dev_private;
>>> +
>>> +       /* 1. outer_bypass_n=0 */
>>> +       tmp0 = readl(hidev->mmio + CRT_PLL1_HS);
>>> +       tmp0 &= 0xBFFFFFFF;
>>> +       writel(tmp0, hidev->mmio + CRT_PLL1_HS);
>>> +
>>> +       /* 2. pll_pd=1?inter_bypass=1 */
>>> +       writel(0x21000000, hidev->mmio + CRT_PLL1_HS);
>>> +
>>> +       /* 3. config pll */
>>> +       writel(pll, hidev->mmio + CRT_PLL1_HS);
>>> +
>>> +       /* 4. delay  */
>>> +       mdelay(1);
>>
>>
>> These should be usleep_range() see
>> https://www.kernel.org/doc/Documentation/timers/timers-howto.txt
>
>
> This looks better to me. i think a 'usleep_range(1000, 2000)' is ok.
>
>>
>>> +
>>> +       /* 5. pll_pd =0 */
>>> +       tmp1 = pll & ~0x01000000;
>>> +       writel(tmp1, hidev->mmio + CRT_PLL1_HS);
>>> +
>>> +       /* 6. delay  */
>>> +       mdelay(1);
>>> +
>>> +       /* 7. inter_bypass=0 */
>>> +       tmp1 &= ~0x20000000;
>>> +       writel(tmp1, hidev->mmio + CRT_PLL1_HS);
>>> +
>>> +       /* 8. delay  */
>>> +       mdelay(1);
>>> +
>>> +       /* 9. outer_bypass_n=1 */
>>> +       tmp1 |= 0x40000000;
>>> +       writel(tmp1, hidev->mmio + CRT_PLL1_HS);
>>
>>
>> This function is a whole lot of magic. Any chance you can pull the
>> values out into #defines?
>
>
> will do. thanks.
>
>>
>>> +}
>>> +
>>> +/* This function takes care the extra registers and bit fields required
>>> to
>>
>>
>> nit: multi-line comments have a leading /* line with the comment
>> starting on the following line
>
>
> thanks for pointing it out.
>
>>
>> applies below as well
>>
>>
>>> + *setup a mode in board.
>>
>>
>> nit: space between * and comment, ie: * setup a mode in board
>
>
> understood, thanks.
>
>
>>
>> applies to the rest of the comment too
>>
>>
>>> + *Explanation about Display Control register:
>>> + *FPGA only supports 7 predefined pixel clocks, and clock select is
>>> + *in bit 4:0 of new register 0x802a8.
>>> + */
>>> +static unsigned int display_ctrl_adjust(struct drm_device *dev,
>>> +                                       struct drm_display_mode *mode,
>>> +                                       unsigned int ctrl)
>>> +{
>>> +       unsigned long x, y;
>>> +       unsigned long pll1; /* bit[31:0] of PLL */
>>> +       unsigned long pll2; /* bit[63:32] of PLL */
>>> +       struct hibmc_drm_device *hidev = dev->dev_private;
>>> +
>>> +       x = mode->hdisplay;
>>> +       y = mode->vdisplay;
>>> +
>>> +       /* Hisilicon has to set up a new register for PLL control
>>> +        *(CRT_PLL1_HS & CRT_PLL2_HS).
>>> +        */
>>> +       if (x == 800 && y == 600) {
>>> +               pll1 = CRT_PLL1_HS_40MHZ;
>>> +               pll2 = CRT_PLL2_HS_40MHZ;
>>> +       } else if (x == 1024 && y == 768) {
>>> +               pll1 = CRT_PLL1_HS_65MHZ;
>>> +               pll2 = CRT_PLL2_HS_65MHZ;
>>> +       } else if (x == 1152 && y == 864) {
>>> +               pll1 = CRT_PLL1_HS_80MHZ_1152;
>>> +               pll2 = CRT_PLL2_HS_80MHZ;
>>> +       } else if (x == 1280 && y == 768) {
>>> +               pll1 = CRT_PLL1_HS_80MHZ;
>>> +               pll2 = CRT_PLL2_HS_80MHZ;
>>> +       } else if (x == 1280 && y == 720) {
>>> +               pll1 = CRT_PLL1_HS_74MHZ;
>>> +               pll2 = CRT_PLL2_HS_74MHZ;
>>> +       } else if (x == 1280 && y == 960) {
>>> +               pll1 = CRT_PLL1_HS_108MHZ;
>>> +               pll2 = CRT_PLL2_HS_108MHZ;
>>> +       } else if (x == 1280 && y == 1024) {
>>> +               pll1 = CRT_PLL1_HS_108MHZ;
>>> +               pll2 = CRT_PLL2_HS_108MHZ;
>>> +       } else if (x == 1600 && y == 1200) {
>>> +               pll1 = CRT_PLL1_HS_162MHZ;
>>> +               pll2 = CRT_PLL2_HS_162MHZ;
>>> +       } else if (x == 1920 && y == 1080) {
>>> +               pll1 = CRT_PLL1_HS_148MHZ;
>>> +               pll2 = CRT_PLL2_HS_148MHZ;
>>> +       } else if (x == 1920 && y == 1200) {
>>> +               pll1 = CRT_PLL1_HS_193MHZ;
>>> +               pll2 = CRT_PLL2_HS_193MHZ;
>>> +       } else /* default to VGA clock */ {
>>> +               pll1 = CRT_PLL1_HS_25MHZ;
>>> +               pll2 = CRT_PLL2_HS_25MHZ;
>>> +       }
>>
>>
>> This seems like something that should be checked in atomic_check so
>> you can be sure the mode is supported.
>>
>> It would also be nice to pull this out into a separate function (and a
>> lookup table if you're feeling adventurous)
>
>
> a lookup table seems good, thanks.
>
>
>>
>>> +
>>> +       writel(pll2, hidev->mmio + CRT_PLL2_HS);
>>> +       set_vclock_hisilicon(dev, pll1);
>>> +
>>> +       /* Hisilicon has to set up the top-left and bottom-right
>>> +        * registers as well.
>>> +        * Note that normal chip only use those two register for
>>> +        * auto-centering mode.
>>> +        */
>>> +       writel((HIBMC_CRT_AUTO_CENTERING_TL_TOP(0) &
>>> +               HIBMC_CRT_AUTO_CENTERING_TL_TOP_MSK) |
>>> +              (HIBMC_CRT_AUTO_CENTERING_TL_LEFT(0) &
>>> +               HIBMC_CRT_AUTO_CENTERING_TL_LEFT_MSK),
>>> +              hidev->mmio + HIBMC_CRT_AUTO_CENTERING_TL);
>>> +
>>> +       writel((HIBMC_CRT_AUTO_CENTERING_BR_BOTTOM(y - 1) &
>>> +               HIBMC_CRT_AUTO_CENTERING_BR_BOTTOM_MASK) |
>>> +              (HIBMC_CRT_AUTO_CENTERING_BR_RIGHT(x - 1) &
>>> +               HIBMC_CRT_AUTO_CENTERING_BR_RIGHT_MASK),
>>> +               hidev->mmio + HIBMC_CRT_AUTO_CENTERING_BR);
>>> +
>>> +       /* Assume common fields in ctrl have been properly set before
>>> +        * calling this function.
>>> +        * This function only sets the extra fields in ctrl.
>>> +        */
>>> +
>>> +       /* Set bit 25 of display controller: Select CRT or VGA clock */
>>> +       ctrl &= ~HIBMC_CRT_DISP_CTL_CRTSELECT_MASK;
>>> +       ctrl &= ~HIBMC_CRT_DISP_CTL_CLOCK_PHASE_MASK;
>>> +
>>> +       ctrl |= HIBMC_CRT_DISP_CTL_CRTSELECT(CRTSELECT_CRT);
>>> +
>>> +       /*ctrl = FIELD_SET(ctrl, HIBMC_CRT_DISP_CTL, CRTSELECT, CRT);*/
>>
>>
>> What's the deal with this commented code?
>
>
> sorry, will clean up.
>
>>
>>> +
>>> +       /* Set bit 14 of display controller */
>>> +       /*ctrl &= FIELD_CLEAR(HIBMC_CRT_DISP_CTL, CLOCK_PHASE);*/
>>> +
>>> +       /* clock_phase_polarity is 0 */
>>> +       ctrl |= HIBMC_CRT_DISP_CTL_CLOCK_PHASE(PHASE_ACTIVE_HIGH);
>>> +       /*ctrl = FIELD_SET(ctrl, HIBMC_CRT_DISP_CTL,*/
>>> +       /*CLOCK_PHASE, ACTIVE_HIGH);*/
>>
>>
>> Here too...
>
>
> ditto.
>
>>
>>> +
>>> +       writel(ctrl, hidev->mmio + HIBMC_CRT_DISP_CTL);
>>> +
>>> +       return ctrl;
>>> +}
>>> +
>>> +static void hibmc_crtc_mode_set_nofb(struct drm_crtc *crtc)
>>> +{
>>> +       unsigned int val;
>>> +       struct drm_display_mode *mode = &crtc->state->mode;
>>> +       struct drm_device *dev = crtc->dev;
>>> +       struct hibmc_drm_device *hidev = dev->dev_private;
>>> +
>>> +       writel(format_pll_reg(), hidev->mmio + HIBMC_CRT_PLL_CTRL);
>>> +       writel((HIBMC_CRT_HORZ_TOTAL_TOTAL(mode->htotal - 1) &
>>> +               HIBMC_CRT_HORZ_TOTAL_TOTAL_MASK) |
>>> +               (HIBMC_CRT_HORZ_TOTAL_DISPLAY_END(mode->hdisplay - 1) &
>>> +               HIBMC_CRT_HORZ_TOTAL_DISPLAY_END_MASK),
>>
>>
>> You could probably macroize this code to make it more readable
>
>
>
>         #define HIBMC_FIELD(field, value) (field(value) & filed##_MASK)
>
>         writel(HIBMC_FIELD(HIBMC_CRT_HORZ_TOTAL_TOTAL, mode->htotal - 1) |
>                HIBMC_FIELD(HIBMC_CRT_HORZ_TOTAL_DISPLAY_END, mode->hdisplay
> - 1),
>                hidev->mmio + HIBMC_CRT_HORZ_TOTAL);
>
> Is above ok?
>

Seems like an improvement.

>
>
>
>>
>>> +               hidev->mmio + HIBMC_CRT_HORZ_TOTAL);
>>> +
>>> +       writel((HIBMC_CRT_HORZ_SYNC_WIDTH(mode->hsync_end -
>>> mode->hsync_start)
>>> +               & HIBMC_CRT_HORZ_SYNC_WIDTH_MASK) |
>>> +               (HIBMC_CRT_HORZ_SYNC_START(mode->hsync_start - 1)
>>> +               & HIBMC_CRT_HORZ_SYNC_START_MASK),
>>> +               hidev->mmio + HIBMC_CRT_HORZ_SYNC);
>>> +
>>> +       writel((HIBMC_CRT_VERT_TOTAL_TOTAL(mode->vtotal - 1) &
>>> +               HIBMC_CRT_VERT_TOTAL_TOTAL_MASK) |
>>> +               (HIBMC_CRT_VERT_TOTAL_DISPLAY_END(mode->vdisplay - 1) &
>>> +               HIBMC_CRT_VERT_TOTAL_DISPLAY_END_MASK),
>>> +               hidev->mmio + HIBMC_CRT_VERT_TOTAL);
>>> +
>>> +       writel((HIBMC_CRT_VERT_SYNC_HEIGHT(mode->vsync_end -
>>> mode->vsync_start)
>>> +               & HIBMC_CRT_VERT_SYNC_HEIGHT_MASK) |
>>> +              (HIBMC_CRT_VERT_SYNC_START(mode->vsync_start - 1) &
>>> +               HIBMC_CRT_VERT_SYNC_START_MASK),
>>> +               hidev->mmio + HIBMC_CRT_VERT_SYNC);
>>> +
>>> +       val = HIBMC_CRT_DISP_CTL_VSYNC_PHASE(0) &
>>> +             HIBMC_CRT_DISP_CTL_VSYNC_PHASE_MASK;
>>> +       val |= HIBMC_CRT_DISP_CTL_HSYNC_PHASE(0) &
>>> +              HIBMC_CRT_DISP_CTL_HSYNC_PHASE_MASK;
>>> +       val |= HIBMC_CRT_DISP_CTL_TIMING(ENABLE);
>>> +       val |= HIBMC_CRT_DISP_CTL_PLANE(ENABLE);
>>> +
>>> +       display_ctrl_adjust(dev, mode, val);
>>> +}
>>> +
>>> +static void hibmc_crtc_atomic_begin(struct drm_crtc *crtc,
>>> +                                   struct drm_crtc_state *old_state)
>>> +{
>>> +       unsigned int reg;
>>> +       struct drm_device *dev = crtc->dev;
>>> +       struct hibmc_drm_device *hidev = dev->dev_private;
>>> +
>>> +       hibmc_set_power_mode(hidev, HIBMC_PW_MODE_CTL_MODE_MODE0);
>>> +
>>> +       /* Enable display power gate & LOCALMEM power gate*/
>>> +       reg = readl(hidev->mmio + HIBMC_CURRENT_GATE);
>>> +       reg &= ~HIBMC_CURR_GATE_DISPLAY_MASK;
>>> +       reg &= ~HIBMC_CURR_GATE_LOCALMEM_MASK;
>>> +       reg |= HIBMC_CURR_GATE_DISPLAY(ON);
>>> +       reg |= HIBMC_CURR_GATE_LOCALMEM(ON);
>>> +       hibmc_set_current_gate(hidev, reg);
>>> +
>>> +       /* We can add more initialization as needed. */
>>> +}
>>> +
>>> +static void hibmc_crtc_atomic_flush(struct drm_crtc *crtc,
>>> +                                   struct drm_crtc_state *old_state)
>>> +
>>> +{
>>> +       unsigned long flags;
>>> +
>>> +       spin_lock_irqsave(&crtc->dev->event_lock, flags);
>>> +       if (crtc->state->event)
>>> +               drm_crtc_send_vblank_event(crtc, crtc->state->event);
>>> +       crtc->state->event = NULL;
>>> +
>>> +       spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
>>> +}
>>> +
>>> +/* These provide the minimum set of functions required to handle a CRTC
>>> */
>>
>>
>> nit: don't need this comment
>
>
> will delete, thanks.
>
>
>>
>>> +static const struct drm_crtc_funcs hibmc_crtc_funcs = {
>>> +       .page_flip = drm_atomic_helper_page_flip,
>>> +       .set_config = drm_atomic_helper_set_config,
>>> +       .destroy = drm_crtc_cleanup,
>>> +       .reset = drm_atomic_helper_crtc_reset,
>>> +       .atomic_duplicate_state =
>>> drm_atomic_helper_crtc_duplicate_state,
>>> +       .atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
>>> +};
>>> +
>>> +static const struct drm_crtc_helper_funcs hibmc_crtc_helper_funcs = {
>>> +       .enable         = hibmc_crtc_enable,
>>> +       .disable        = hibmc_crtc_disable,
>>> +       .mode_set_nofb  = hibmc_crtc_mode_set_nofb,
>>> +       .atomic_check   = hibmc_crtc_atomic_check,
>>> +       .atomic_begin   = hibmc_crtc_atomic_begin,
>>> +       .atomic_flush   = hibmc_crtc_atomic_flush,
>>> +};
>>> +
>>> +int hibmc_crtc_init(struct hibmc_drm_device *hidev)
>>> +{
>>> +       struct drm_device *dev = hidev->dev;
>>> +       struct drm_crtc *crtc = &hidev->crtc;
>>> +       struct drm_plane *plane = &hidev->plane;
>>> +       int ret;
>>> +
>>> +       ret = drm_crtc_init_with_planes(dev, crtc, plane,
>>> +                                       NULL, &hibmc_crtc_funcs, NULL);
>>> +       if (ret) {
>>> +               DRM_ERROR("failed to init crtc.\n");
>>
>>
>> print return code
>
>
> agreed, thanks.
>
>>
>>> +               return ret;
>>> +       }
>>> +
>>> +       drm_mode_crtc_set_gamma_size(crtc, 256);
>>
>>
>> check return code
>
>
> agreed though none of other drivers has done this,
> thanks.
>
>>
>>> +       drm_crtc_helper_add(crtc, &hibmc_crtc_helper_funcs);
>>> +       return 0;
>>> +}
>>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
>>> b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
>>> index 7d96583..303cd36 100644
>>> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
>>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
>>> @@ -119,6 +119,12 @@ static int hibmc_kms_init(struct hibmc_drm_device
>>> *hidev)
>>>                  return ret;
>>>          }
>>>
>>> +       ret = hibmc_crtc_init(hidev);
>>> +       if (ret) {
>>> +               DRM_ERROR("failed to init crtc.\n");
>>> +               return ret;
>>> +       }
>>
>>
>> Typically the plane is initialized internally in the crtc driver. I
>> think this is a good design pattern, and you should probably use it.
>>
>> So how about squashing this down with the plane patch and keeping the
>> plane inside hibmc_drm_de.c?
>
>
> understood after i looked at intel_display.c, this file will be merged
> with patch 4/9, and the tile will be: 'drm/hisilicon/hibmc: Add display
> engine'.
>

Great, thanks.

>>
>>> +
>>>          return 0;
>>>   }
>>>
>>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
>>> b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
>>> index 49e39d2..5731ec2 100644
>>> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
>>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
>>> @@ -46,6 +46,7 @@ struct hibmc_drm_device {
>>>          /* drm */
>>>          struct drm_device  *dev;
>>>          struct drm_plane plane;
>>
>>
>> I don't think you should be keeping track of plane here. plane is only
>> used in the crtc init, which should be addressed by the previous
>> comment.
>
>
> so allocate with devm_kzalloc(sizeof(*plane)) and remove it from
> hibmc_drm_device?
>
>>
>>
>>> +       struct drm_crtc crtc;
>>
>>
>> crtc is only used in the irq handler, so you could remove this here
>> and just call drm_handle_vblank(dev, 0) in the handler.
>
>
> so allocate with devm_kzalloc(sizeof(*crtc)) and remove it from
> hibmc_drm_device, when driver unload drm_crtc_cleanup() will be
> called and finally memory will be freed before quit.
>

Yep, precisely.

Sean

>>
>>
>>>          bool mode_config_initialized;
>>>
>>>          /* ttm */
>>> @@ -85,6 +86,7 @@ static inline struct hibmc_bo *gem_to_hibmc_bo(struct
>>> drm_gem_object *gem)
>>>   #define DRM_FILE_PAGE_OFFSET (0x100000000ULL >> PAGE_SHIFT)
>>>
>>>   int hibmc_plane_init(struct hibmc_drm_device *hidev);
>>> +int hibmc_crtc_init(struct hibmc_drm_device *hidev);
>>>   int hibmc_fbdev_init(struct hibmc_drm_device *hidev);
>>>   void hibmc_fbdev_fini(struct hibmc_drm_device *hidev);
>>>
>>> --
>>> 1.9.1
>>>
>>>
>>> _______________________________________________
>>> linux-arm-kernel mailing list
>>> linux-arm-kernel at lists.infradead.org
>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>
>> _______________________________________________
>> linuxarm mailing list
>> linuxarm at huawei.com
>> http://rnd-openeuler.huawei.com/mailman/listinfo/linuxarm
>>
>> .
>>
>
>
> --
> Regards, Rongrong
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: [PATCH 2/2] ARM: dts: vf610-zii-dev: Add .dts file for rev. C
From: Andrew Lunn @ 2016-11-14 17:10 UTC (permalink / raw)
  To: Andrey Smirnov
  Cc: linux-arm-kernel, Shawn Guo, Rob Herring, Mark Rutland,
	Russell King, Sascha Hauer, Stefan Agner, devicetree,
	linux-kernel, cphealy
In-Reply-To: <1479141306-15141-2-git-send-email-andrew.smirnov@gmail.com>

> +		mdio_mux_1: mdio@1 {
> +			reg = <1>;
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			switch0: switch0@0 {
> +				compatible = "marvell,mv88e6390";

Hi Andrey

The driver for this is not in net-next yet. And when it is, it will
probably be called "marvell,mv88e6190", keeping to the pattern of the
lowest product ID which supports these features.

> +					port@4 {
> +						reg = <4>;
> +						label = "lan4";
> +					};
> +
> +					port@9 {
> +						reg = <9>;
> +						label = "lan4";
> +						phy-handle = <&switch0phy0>;
> +					};
> +

You have two "lan4". I would also suggest leaving port 9 out for the
moment. It needs clause 45 MDIO to talk to the PHY, which we don't
have yet. Hence it cannot find it, and so give an error.

> +
> +					switch0port10: port@10 {
> +						reg = <10>;
> +						label = "dsa";
> +						phy-mode = "xgmii";
> +						link = <&switch1port10>;
> +						fixed-link {
> +							speed = <10000>;
> +							full-duplex;
> +						};

This fixed-link node is wrong, and invalid. 10000 is not supported by
the fixed link driver, only 10, 100, and 1000. Also, it is not
required. The DSA driver should configure the link to the fastest
possible speed the port supports. You only need a fixed-link property
when you need to configure it at a lower speed. Rev B also gets this
wrong.

> +					};
> +				};
> +
> +				mdio {
> +					#address-cells = <1>;
> +					#size-cells = <0>;
> +					switch0phy0: switch0phy0@0 {
> +						reg = <0>;
> +					};

I think the strapping for the PHY is such that it is at address 9.
Also, it is on the external mdio bus, not the internal mdio bus. The
6390 family has two MDIO busses. I have patches to support this, which
will appear eventually.

  Andrew

^ permalink raw reply

* Re: [PATCH v5 1/3] dt-bindings: mediatek: Add a binding for Mediatek JPEG Decoder
From: Rob Herring @ 2016-11-14 17:11 UTC (permalink / raw)
  To: Rick Chang
  Cc: Hans Verkuil, Laurent Pinchart, Mauro Carvalho Chehab,
	Matthias Brugger, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-media-u79uwXL29TY76Z2rM5mHXA,
	srv_heupstream-NuS5LvNUpcJWk0Htik3J/w,
	linux-mediatek-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	devicetree-u79uwXL29TY76Z2rM5mHXA, Minghsiu Tsai
In-Reply-To: <1478586880-3923-2-git-send-email-rick.chang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>

On Tue, Nov 08, 2016 at 02:34:38PM +0800, Rick Chang wrote:
> Add a DT binding documentation for Mediatek JPEG Decoder of
> MT2701 SoC.
> 
> Signed-off-by: Rick Chang <rick.chang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> Signed-off-by: Minghsiu Tsai <minghsiu.tsai-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> ---
>  .../bindings/media/mediatek-jpeg-decoder.txt       | 37 ++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/mediatek-jpeg-decoder.txt

Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* [PATCH v5 1/3] dt-bindings: mediatek: Add a binding for Mediatek JPEG Decoder
From: Rob Herring @ 2016-11-14 17:11 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <1478586880-3923-2-git-send-email-rick.chang@mediatek.com>

On Tue, Nov 08, 2016 at 02:34:38PM +0800, Rick Chang wrote:
> Add a DT binding documentation for Mediatek JPEG Decoder of
> MT2701 SoC.
> 
> Signed-off-by: Rick Chang <rick.chang@mediatek.com>
> Signed-off-by: Minghsiu Tsai <minghsiu.tsai@mediatek.com>
> ---
>  .../bindings/media/mediatek-jpeg-decoder.txt       | 37 ++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/mediatek-jpeg-decoder.txt

Acked-by: Rob Herring <robh@kernel.org>

^ permalink raw reply

* Re: [PATCH v5 1/3] dt-bindings: mediatek: Add a binding for Mediatek JPEG Decoder
From: Rob Herring @ 2016-11-14 17:11 UTC (permalink / raw)
  To: Rick Chang
  Cc: Hans Verkuil, Laurent Pinchart, Mauro Carvalho Chehab,
	Matthias Brugger, linux-kernel, linux-media, srv_heupstream,
	linux-mediatek, linux-arm-kernel, devicetree, Minghsiu Tsai
In-Reply-To: <1478586880-3923-2-git-send-email-rick.chang@mediatek.com>

On Tue, Nov 08, 2016 at 02:34:38PM +0800, Rick Chang wrote:
> Add a DT binding documentation for Mediatek JPEG Decoder of
> MT2701 SoC.
> 
> Signed-off-by: Rick Chang <rick.chang@mediatek.com>
> Signed-off-by: Minghsiu Tsai <minghsiu.tsai@mediatek.com>
> ---
>  .../bindings/media/mediatek-jpeg-decoder.txt       | 37 ++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/mediatek-jpeg-decoder.txt

Acked-by: Rob Herring <robh@kernel.org>

^ permalink raw reply

* Re: [PATCH v6 5/9] drm/hisilicon/hibmc: Add crtc for DE
From: Sean Paul @ 2016-11-14 17:10 UTC (permalink / raw)
  To: Rongrong Zou
  Cc: Mark Rutland, lijianhua, Will Deacon, Tomeu Vizoso,
	Jonathan Corbet, catalin.marinas, Emil Velikov, linuxarm,
	dri-devel, james.xiong, shenhui, Rongrong Zou, Linux ARM Kernel
In-Reply-To: <5826ECA6.6030900@huawei.com>

On Sat, Nov 12, 2016 at 5:19 AM, Rongrong Zou <zourongrong@huawei.com> wrote:
> 在 2016/11/11 6:14, Sean Paul 写道:
>>
>> On Fri, Oct 28, 2016 at 3:27 AM, Rongrong Zou <zourongrong@gmail.com>
>> wrote:
>>>
>>> Add crtc funcs and helper funcs for DE.
>>>
>>> Signed-off-by: Rongrong Zou <zourongrong@gmail.com>
>>> ---
>>>   drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c  | 318
>>> ++++++++++++++++++++++++
>>>   drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c |   6 +
>>>   drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h |   2 +
>>>   3 files changed, 326 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
>>> b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
>>> index 9c1a68c..9b5d0d0 100644
>>> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
>>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
>>> @@ -23,6 +23,7 @@
>>>
>>>   #include "hibmc_drm_drv.h"
>>>   #include "hibmc_drm_regs.h"
>>> +#include "hibmc_drm_de.h"
>>>   #include "hibmc_drm_power.h"
>>
>>
>> nit: alphabetize
>
>
> ok, thanks.
>
>>
>>>
>>>   /*
>>> ---------------------------------------------------------------------- */
>>
>>
>> Remove
>
>
> will do, thanks.
>
>>
>>> @@ -168,3 +169,320 @@ int hibmc_plane_init(struct hibmc_drm_device
>>> *hidev)
>>>          drm_plane_helper_add(plane, &hibmc_plane_helper_funcs);
>>>          return 0;
>>>   }
>>> +
>>> +static void hibmc_crtc_enable(struct drm_crtc *crtc)
>>> +{
>>> +       unsigned int reg;
>>> +       /* power mode 0 is default. */
>>
>>
>> This comment seems to be in the wrong place
>
>
> will remove it, thanks.
>
>
>>
>>> +       struct hibmc_drm_device *hidev = crtc->dev->dev_private;
>>> +
>>> +       hibmc_set_power_mode(hidev, HIBMC_PW_MODE_CTL_MODE_MODE0);
>>> +
>>> +       /* Enable display power gate & LOCALMEM power gate*/
>>> +       reg = readl(hidev->mmio + HIBMC_CURRENT_GATE);
>>> +       reg &= ~HIBMC_CURR_GATE_LOCALMEM_MASK;
>>> +       reg &= ~HIBMC_CURR_GATE_DISPLAY_MASK;
>>> +       reg |= HIBMC_CURR_GATE_LOCALMEM(ON);
>>> +       reg |= HIBMC_CURR_GATE_DISPLAY(ON);
>>> +       hibmc_set_current_gate(hidev, reg);
>>> +       drm_crtc_vblank_on(crtc);
>>> +}
>>> +
>>> +static void hibmc_crtc_disable(struct drm_crtc *crtc)
>>> +{
>>> +       unsigned int reg;
>>> +       struct hibmc_drm_device *hidev = crtc->dev->dev_private;
>>> +
>>> +       drm_crtc_vblank_off(crtc);
>>> +
>>> +       hibmc_set_power_mode(hidev, HIBMC_PW_MODE_CTL_MODE_SLEEP);
>>> +
>>> +       /* Enable display power gate & LOCALMEM power gate*/
>>> +       reg = readl(hidev->mmio + HIBMC_CURRENT_GATE);
>>> +       reg &= ~HIBMC_CURR_GATE_LOCALMEM_MASK;
>>> +       reg &= ~HIBMC_CURR_GATE_DISPLAY_MASK;
>>> +       reg |= HIBMC_CURR_GATE_LOCALMEM(OFF);
>>> +       reg |= HIBMC_CURR_GATE_DISPLAY(OFF);
>>> +       hibmc_set_current_gate(hidev, reg);
>>> +}
>>> +
>>> +static int hibmc_crtc_atomic_check(struct drm_crtc *crtc,
>>> +                                  struct drm_crtc_state *state)
>>> +{
>>> +       return 0;
>>> +}
>>
>>
>> Caller NULL-checks, no need for stub
>
>
> thanks for pointing it out.
>
>
>>
>>> +
>>> +static unsigned int format_pll_reg(void)
>>> +{
>>> +       unsigned int pllreg = 0;
>>> +       struct panel_pll pll = {0};
>>> +
>>> +       /* Note that all PLL's have the same format. Here,
>>> +        * we just use Panel PLL parameter to work out the bit
>>> +        * fields in the register.On returning a 32 bit number, the value
>>> can
>>> +        * be applied to any PLL in the calling function.
>>> +        */
>>> +       pllreg |= HIBMC_PLL_CTRL_BYPASS(OFF) &
>>> HIBMC_PLL_CTRL_BYPASS_MASK;
>>> +       pllreg |= HIBMC_PLL_CTRL_POWER(ON) & HIBMC_PLL_CTRL_POWER_MASK;
>>> +       pllreg |= HIBMC_PLL_CTRL_INPUT(OSC) & HIBMC_PLL_CTRL_INPUT_MASK;
>>> +       pllreg |= HIBMC_PLL_CTRL_POD(pll.POD) & HIBMC_PLL_CTRL_POD_MASK;
>>> +       pllreg |= HIBMC_PLL_CTRL_OD(pll.OD) & HIBMC_PLL_CTRL_OD_MASK;
>>> +       pllreg |= HIBMC_PLL_CTRL_N(pll.N) & HIBMC_PLL_CTRL_N_MASK;
>>> +       pllreg |= HIBMC_PLL_CTRL_M(pll.M) & HIBMC_PLL_CTRL_M_MASK;
>>> +
>>> +       return pllreg;
>>> +}
>>> +
>>> +static void set_vclock_hisilicon(struct drm_device *dev, unsigned long
>>> pll)
>>> +{
>>> +       unsigned long tmp0, tmp1;
>>> +       struct hibmc_drm_device *hidev = dev->dev_private;
>>> +
>>> +       /* 1. outer_bypass_n=0 */
>>> +       tmp0 = readl(hidev->mmio + CRT_PLL1_HS);
>>> +       tmp0 &= 0xBFFFFFFF;
>>> +       writel(tmp0, hidev->mmio + CRT_PLL1_HS);
>>> +
>>> +       /* 2. pll_pd=1?inter_bypass=1 */
>>> +       writel(0x21000000, hidev->mmio + CRT_PLL1_HS);
>>> +
>>> +       /* 3. config pll */
>>> +       writel(pll, hidev->mmio + CRT_PLL1_HS);
>>> +
>>> +       /* 4. delay  */
>>> +       mdelay(1);
>>
>>
>> These should be usleep_range() see
>> https://www.kernel.org/doc/Documentation/timers/timers-howto.txt
>
>
> This looks better to me. i think a 'usleep_range(1000, 2000)' is ok.
>
>>
>>> +
>>> +       /* 5. pll_pd =0 */
>>> +       tmp1 = pll & ~0x01000000;
>>> +       writel(tmp1, hidev->mmio + CRT_PLL1_HS);
>>> +
>>> +       /* 6. delay  */
>>> +       mdelay(1);
>>> +
>>> +       /* 7. inter_bypass=0 */
>>> +       tmp1 &= ~0x20000000;
>>> +       writel(tmp1, hidev->mmio + CRT_PLL1_HS);
>>> +
>>> +       /* 8. delay  */
>>> +       mdelay(1);
>>> +
>>> +       /* 9. outer_bypass_n=1 */
>>> +       tmp1 |= 0x40000000;
>>> +       writel(tmp1, hidev->mmio + CRT_PLL1_HS);
>>
>>
>> This function is a whole lot of magic. Any chance you can pull the
>> values out into #defines?
>
>
> will do. thanks.
>
>>
>>> +}
>>> +
>>> +/* This function takes care the extra registers and bit fields required
>>> to
>>
>>
>> nit: multi-line comments have a leading /* line with the comment
>> starting on the following line
>
>
> thanks for pointing it out.
>
>>
>> applies below as well
>>
>>
>>> + *setup a mode in board.
>>
>>
>> nit: space between * and comment, ie: * setup a mode in board
>
>
> understood, thanks.
>
>
>>
>> applies to the rest of the comment too
>>
>>
>>> + *Explanation about Display Control register:
>>> + *FPGA only supports 7 predefined pixel clocks, and clock select is
>>> + *in bit 4:0 of new register 0x802a8.
>>> + */
>>> +static unsigned int display_ctrl_adjust(struct drm_device *dev,
>>> +                                       struct drm_display_mode *mode,
>>> +                                       unsigned int ctrl)
>>> +{
>>> +       unsigned long x, y;
>>> +       unsigned long pll1; /* bit[31:0] of PLL */
>>> +       unsigned long pll2; /* bit[63:32] of PLL */
>>> +       struct hibmc_drm_device *hidev = dev->dev_private;
>>> +
>>> +       x = mode->hdisplay;
>>> +       y = mode->vdisplay;
>>> +
>>> +       /* Hisilicon has to set up a new register for PLL control
>>> +        *(CRT_PLL1_HS & CRT_PLL2_HS).
>>> +        */
>>> +       if (x == 800 && y == 600) {
>>> +               pll1 = CRT_PLL1_HS_40MHZ;
>>> +               pll2 = CRT_PLL2_HS_40MHZ;
>>> +       } else if (x == 1024 && y == 768) {
>>> +               pll1 = CRT_PLL1_HS_65MHZ;
>>> +               pll2 = CRT_PLL2_HS_65MHZ;
>>> +       } else if (x == 1152 && y == 864) {
>>> +               pll1 = CRT_PLL1_HS_80MHZ_1152;
>>> +               pll2 = CRT_PLL2_HS_80MHZ;
>>> +       } else if (x == 1280 && y == 768) {
>>> +               pll1 = CRT_PLL1_HS_80MHZ;
>>> +               pll2 = CRT_PLL2_HS_80MHZ;
>>> +       } else if (x == 1280 && y == 720) {
>>> +               pll1 = CRT_PLL1_HS_74MHZ;
>>> +               pll2 = CRT_PLL2_HS_74MHZ;
>>> +       } else if (x == 1280 && y == 960) {
>>> +               pll1 = CRT_PLL1_HS_108MHZ;
>>> +               pll2 = CRT_PLL2_HS_108MHZ;
>>> +       } else if (x == 1280 && y == 1024) {
>>> +               pll1 = CRT_PLL1_HS_108MHZ;
>>> +               pll2 = CRT_PLL2_HS_108MHZ;
>>> +       } else if (x == 1600 && y == 1200) {
>>> +               pll1 = CRT_PLL1_HS_162MHZ;
>>> +               pll2 = CRT_PLL2_HS_162MHZ;
>>> +       } else if (x == 1920 && y == 1080) {
>>> +               pll1 = CRT_PLL1_HS_148MHZ;
>>> +               pll2 = CRT_PLL2_HS_148MHZ;
>>> +       } else if (x == 1920 && y == 1200) {
>>> +               pll1 = CRT_PLL1_HS_193MHZ;
>>> +               pll2 = CRT_PLL2_HS_193MHZ;
>>> +       } else /* default to VGA clock */ {
>>> +               pll1 = CRT_PLL1_HS_25MHZ;
>>> +               pll2 = CRT_PLL2_HS_25MHZ;
>>> +       }
>>
>>
>> This seems like something that should be checked in atomic_check so
>> you can be sure the mode is supported.
>>
>> It would also be nice to pull this out into a separate function (and a
>> lookup table if you're feeling adventurous)
>
>
> a lookup table seems good, thanks.
>
>
>>
>>> +
>>> +       writel(pll2, hidev->mmio + CRT_PLL2_HS);
>>> +       set_vclock_hisilicon(dev, pll1);
>>> +
>>> +       /* Hisilicon has to set up the top-left and bottom-right
>>> +        * registers as well.
>>> +        * Note that normal chip only use those two register for
>>> +        * auto-centering mode.
>>> +        */
>>> +       writel((HIBMC_CRT_AUTO_CENTERING_TL_TOP(0) &
>>> +               HIBMC_CRT_AUTO_CENTERING_TL_TOP_MSK) |
>>> +              (HIBMC_CRT_AUTO_CENTERING_TL_LEFT(0) &
>>> +               HIBMC_CRT_AUTO_CENTERING_TL_LEFT_MSK),
>>> +              hidev->mmio + HIBMC_CRT_AUTO_CENTERING_TL);
>>> +
>>> +       writel((HIBMC_CRT_AUTO_CENTERING_BR_BOTTOM(y - 1) &
>>> +               HIBMC_CRT_AUTO_CENTERING_BR_BOTTOM_MASK) |
>>> +              (HIBMC_CRT_AUTO_CENTERING_BR_RIGHT(x - 1) &
>>> +               HIBMC_CRT_AUTO_CENTERING_BR_RIGHT_MASK),
>>> +               hidev->mmio + HIBMC_CRT_AUTO_CENTERING_BR);
>>> +
>>> +       /* Assume common fields in ctrl have been properly set before
>>> +        * calling this function.
>>> +        * This function only sets the extra fields in ctrl.
>>> +        */
>>> +
>>> +       /* Set bit 25 of display controller: Select CRT or VGA clock */
>>> +       ctrl &= ~HIBMC_CRT_DISP_CTL_CRTSELECT_MASK;
>>> +       ctrl &= ~HIBMC_CRT_DISP_CTL_CLOCK_PHASE_MASK;
>>> +
>>> +       ctrl |= HIBMC_CRT_DISP_CTL_CRTSELECT(CRTSELECT_CRT);
>>> +
>>> +       /*ctrl = FIELD_SET(ctrl, HIBMC_CRT_DISP_CTL, CRTSELECT, CRT);*/
>>
>>
>> What's the deal with this commented code?
>
>
> sorry, will clean up.
>
>>
>>> +
>>> +       /* Set bit 14 of display controller */
>>> +       /*ctrl &= FIELD_CLEAR(HIBMC_CRT_DISP_CTL, CLOCK_PHASE);*/
>>> +
>>> +       /* clock_phase_polarity is 0 */
>>> +       ctrl |= HIBMC_CRT_DISP_CTL_CLOCK_PHASE(PHASE_ACTIVE_HIGH);
>>> +       /*ctrl = FIELD_SET(ctrl, HIBMC_CRT_DISP_CTL,*/
>>> +       /*CLOCK_PHASE, ACTIVE_HIGH);*/
>>
>>
>> Here too...
>
>
> ditto.
>
>>
>>> +
>>> +       writel(ctrl, hidev->mmio + HIBMC_CRT_DISP_CTL);
>>> +
>>> +       return ctrl;
>>> +}
>>> +
>>> +static void hibmc_crtc_mode_set_nofb(struct drm_crtc *crtc)
>>> +{
>>> +       unsigned int val;
>>> +       struct drm_display_mode *mode = &crtc->state->mode;
>>> +       struct drm_device *dev = crtc->dev;
>>> +       struct hibmc_drm_device *hidev = dev->dev_private;
>>> +
>>> +       writel(format_pll_reg(), hidev->mmio + HIBMC_CRT_PLL_CTRL);
>>> +       writel((HIBMC_CRT_HORZ_TOTAL_TOTAL(mode->htotal - 1) &
>>> +               HIBMC_CRT_HORZ_TOTAL_TOTAL_MASK) |
>>> +               (HIBMC_CRT_HORZ_TOTAL_DISPLAY_END(mode->hdisplay - 1) &
>>> +               HIBMC_CRT_HORZ_TOTAL_DISPLAY_END_MASK),
>>
>>
>> You could probably macroize this code to make it more readable
>
>
>
>         #define HIBMC_FIELD(field, value) (field(value) & filed##_MASK)
>
>         writel(HIBMC_FIELD(HIBMC_CRT_HORZ_TOTAL_TOTAL, mode->htotal - 1) |
>                HIBMC_FIELD(HIBMC_CRT_HORZ_TOTAL_DISPLAY_END, mode->hdisplay
> - 1),
>                hidev->mmio + HIBMC_CRT_HORZ_TOTAL);
>
> Is above ok?
>

Seems like an improvement.

>
>
>
>>
>>> +               hidev->mmio + HIBMC_CRT_HORZ_TOTAL);
>>> +
>>> +       writel((HIBMC_CRT_HORZ_SYNC_WIDTH(mode->hsync_end -
>>> mode->hsync_start)
>>> +               & HIBMC_CRT_HORZ_SYNC_WIDTH_MASK) |
>>> +               (HIBMC_CRT_HORZ_SYNC_START(mode->hsync_start - 1)
>>> +               & HIBMC_CRT_HORZ_SYNC_START_MASK),
>>> +               hidev->mmio + HIBMC_CRT_HORZ_SYNC);
>>> +
>>> +       writel((HIBMC_CRT_VERT_TOTAL_TOTAL(mode->vtotal - 1) &
>>> +               HIBMC_CRT_VERT_TOTAL_TOTAL_MASK) |
>>> +               (HIBMC_CRT_VERT_TOTAL_DISPLAY_END(mode->vdisplay - 1) &
>>> +               HIBMC_CRT_VERT_TOTAL_DISPLAY_END_MASK),
>>> +               hidev->mmio + HIBMC_CRT_VERT_TOTAL);
>>> +
>>> +       writel((HIBMC_CRT_VERT_SYNC_HEIGHT(mode->vsync_end -
>>> mode->vsync_start)
>>> +               & HIBMC_CRT_VERT_SYNC_HEIGHT_MASK) |
>>> +              (HIBMC_CRT_VERT_SYNC_START(mode->vsync_start - 1) &
>>> +               HIBMC_CRT_VERT_SYNC_START_MASK),
>>> +               hidev->mmio + HIBMC_CRT_VERT_SYNC);
>>> +
>>> +       val = HIBMC_CRT_DISP_CTL_VSYNC_PHASE(0) &
>>> +             HIBMC_CRT_DISP_CTL_VSYNC_PHASE_MASK;
>>> +       val |= HIBMC_CRT_DISP_CTL_HSYNC_PHASE(0) &
>>> +              HIBMC_CRT_DISP_CTL_HSYNC_PHASE_MASK;
>>> +       val |= HIBMC_CRT_DISP_CTL_TIMING(ENABLE);
>>> +       val |= HIBMC_CRT_DISP_CTL_PLANE(ENABLE);
>>> +
>>> +       display_ctrl_adjust(dev, mode, val);
>>> +}
>>> +
>>> +static void hibmc_crtc_atomic_begin(struct drm_crtc *crtc,
>>> +                                   struct drm_crtc_state *old_state)
>>> +{
>>> +       unsigned int reg;
>>> +       struct drm_device *dev = crtc->dev;
>>> +       struct hibmc_drm_device *hidev = dev->dev_private;
>>> +
>>> +       hibmc_set_power_mode(hidev, HIBMC_PW_MODE_CTL_MODE_MODE0);
>>> +
>>> +       /* Enable display power gate & LOCALMEM power gate*/
>>> +       reg = readl(hidev->mmio + HIBMC_CURRENT_GATE);
>>> +       reg &= ~HIBMC_CURR_GATE_DISPLAY_MASK;
>>> +       reg &= ~HIBMC_CURR_GATE_LOCALMEM_MASK;
>>> +       reg |= HIBMC_CURR_GATE_DISPLAY(ON);
>>> +       reg |= HIBMC_CURR_GATE_LOCALMEM(ON);
>>> +       hibmc_set_current_gate(hidev, reg);
>>> +
>>> +       /* We can add more initialization as needed. */
>>> +}
>>> +
>>> +static void hibmc_crtc_atomic_flush(struct drm_crtc *crtc,
>>> +                                   struct drm_crtc_state *old_state)
>>> +
>>> +{
>>> +       unsigned long flags;
>>> +
>>> +       spin_lock_irqsave(&crtc->dev->event_lock, flags);
>>> +       if (crtc->state->event)
>>> +               drm_crtc_send_vblank_event(crtc, crtc->state->event);
>>> +       crtc->state->event = NULL;
>>> +
>>> +       spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
>>> +}
>>> +
>>> +/* These provide the minimum set of functions required to handle a CRTC
>>> */
>>
>>
>> nit: don't need this comment
>
>
> will delete, thanks.
>
>
>>
>>> +static const struct drm_crtc_funcs hibmc_crtc_funcs = {
>>> +       .page_flip = drm_atomic_helper_page_flip,
>>> +       .set_config = drm_atomic_helper_set_config,
>>> +       .destroy = drm_crtc_cleanup,
>>> +       .reset = drm_atomic_helper_crtc_reset,
>>> +       .atomic_duplicate_state =
>>> drm_atomic_helper_crtc_duplicate_state,
>>> +       .atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
>>> +};
>>> +
>>> +static const struct drm_crtc_helper_funcs hibmc_crtc_helper_funcs = {
>>> +       .enable         = hibmc_crtc_enable,
>>> +       .disable        = hibmc_crtc_disable,
>>> +       .mode_set_nofb  = hibmc_crtc_mode_set_nofb,
>>> +       .atomic_check   = hibmc_crtc_atomic_check,
>>> +       .atomic_begin   = hibmc_crtc_atomic_begin,
>>> +       .atomic_flush   = hibmc_crtc_atomic_flush,
>>> +};
>>> +
>>> +int hibmc_crtc_init(struct hibmc_drm_device *hidev)
>>> +{
>>> +       struct drm_device *dev = hidev->dev;
>>> +       struct drm_crtc *crtc = &hidev->crtc;
>>> +       struct drm_plane *plane = &hidev->plane;
>>> +       int ret;
>>> +
>>> +       ret = drm_crtc_init_with_planes(dev, crtc, plane,
>>> +                                       NULL, &hibmc_crtc_funcs, NULL);
>>> +       if (ret) {
>>> +               DRM_ERROR("failed to init crtc.\n");
>>
>>
>> print return code
>
>
> agreed, thanks.
>
>>
>>> +               return ret;
>>> +       }
>>> +
>>> +       drm_mode_crtc_set_gamma_size(crtc, 256);
>>
>>
>> check return code
>
>
> agreed though none of other drivers has done this,
> thanks.
>
>>
>>> +       drm_crtc_helper_add(crtc, &hibmc_crtc_helper_funcs);
>>> +       return 0;
>>> +}
>>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
>>> b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
>>> index 7d96583..303cd36 100644
>>> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
>>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
>>> @@ -119,6 +119,12 @@ static int hibmc_kms_init(struct hibmc_drm_device
>>> *hidev)
>>>                  return ret;
>>>          }
>>>
>>> +       ret = hibmc_crtc_init(hidev);
>>> +       if (ret) {
>>> +               DRM_ERROR("failed to init crtc.\n");
>>> +               return ret;
>>> +       }
>>
>>
>> Typically the plane is initialized internally in the crtc driver. I
>> think this is a good design pattern, and you should probably use it.
>>
>> So how about squashing this down with the plane patch and keeping the
>> plane inside hibmc_drm_de.c?
>
>
> understood after i looked at intel_display.c, this file will be merged
> with patch 4/9, and the tile will be: 'drm/hisilicon/hibmc: Add display
> engine'.
>

Great, thanks.

>>
>>> +
>>>          return 0;
>>>   }
>>>
>>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
>>> b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
>>> index 49e39d2..5731ec2 100644
>>> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
>>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
>>> @@ -46,6 +46,7 @@ struct hibmc_drm_device {
>>>          /* drm */
>>>          struct drm_device  *dev;
>>>          struct drm_plane plane;
>>
>>
>> I don't think you should be keeping track of plane here. plane is only
>> used in the crtc init, which should be addressed by the previous
>> comment.
>
>
> so allocate with devm_kzalloc(sizeof(*plane)) and remove it from
> hibmc_drm_device?
>
>>
>>
>>> +       struct drm_crtc crtc;
>>
>>
>> crtc is only used in the irq handler, so you could remove this here
>> and just call drm_handle_vblank(dev, 0) in the handler.
>
>
> so allocate with devm_kzalloc(sizeof(*crtc)) and remove it from
> hibmc_drm_device, when driver unload drm_crtc_cleanup() will be
> called and finally memory will be freed before quit.
>

Yep, precisely.

Sean

>>
>>
>>>          bool mode_config_initialized;
>>>
>>>          /* ttm */
>>> @@ -85,6 +86,7 @@ static inline struct hibmc_bo *gem_to_hibmc_bo(struct
>>> drm_gem_object *gem)
>>>   #define DRM_FILE_PAGE_OFFSET (0x100000000ULL >> PAGE_SHIFT)
>>>
>>>   int hibmc_plane_init(struct hibmc_drm_device *hidev);
>>> +int hibmc_crtc_init(struct hibmc_drm_device *hidev);
>>>   int hibmc_fbdev_init(struct hibmc_drm_device *hidev);
>>>   void hibmc_fbdev_fini(struct hibmc_drm_device *hidev);
>>>
>>> --
>>> 1.9.1
>>>
>>>
>>> _______________________________________________
>>> linux-arm-kernel mailing list
>>> linux-arm-kernel@lists.infradead.org
>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>
>> _______________________________________________
>> linuxarm mailing list
>> linuxarm@huawei.com
>> http://rnd-openeuler.huawei.com/mailman/listinfo/linuxarm
>>
>> .
>>
>
>
> --
> Regards, Rongrong
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply

* [RESEND][PATCH 1/6] dt-bindings: arm: Update bindings for LS2088A targets
From: Rob Herring @ 2016-11-14 17:11 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <1478597664-14799-2-git-send-email-abhimanyu.saini@nxp.com>

On Tue, Nov 08, 2016 at 03:04:19PM +0530, Abhimanyu Saini wrote:
> Add compatible strings for LS2088A RDB and QDS board.
> 
> Signed-off-by: Priyanka Jain <priyanka.jain@nxp.com>
> Signed-off-by: Ashish Kumar <ashish.kumar@nxp.com>
> Signed-off-by: Abhimanyu Saini <abhimanyu.saini@nxp.com>
> ---
>  Documentation/devicetree/bindings/arm/fsl.txt | 7 +++++++
>  1 file changed, 7 insertions(+)

Acked-by: Rob Herring <robh@kernel.org>

^ permalink raw reply

* Re: [RESEND][PATCH 1/6] dt-bindings: arm: Update bindings for LS2088A targets
From: Rob Herring @ 2016-11-14 17:11 UTC (permalink / raw)
  To: Abhimanyu Saini
  Cc: devicetree-u79uwXL29TY76Z2rM5mHXA, mark.rutland-5wv7dgnIgG8,
	shawnguo-DgEjT+Ai2ygdnm+yROfE0A, scott.wood-3arQi8VN3Tc,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Priyanka Jain,
	Ashish Kumar
In-Reply-To: <1478597664-14799-2-git-send-email-abhimanyu.saini-3arQi8VN3Tc@public.gmane.org>

On Tue, Nov 08, 2016 at 03:04:19PM +0530, Abhimanyu Saini wrote:
> Add compatible strings for LS2088A RDB and QDS board.
> 
> Signed-off-by: Priyanka Jain <priyanka.jain-3arQi8VN3Tc@public.gmane.org>
> Signed-off-by: Ashish Kumar <ashish.kumar-3arQi8VN3Tc@public.gmane.org>
> Signed-off-by: Abhimanyu Saini <abhimanyu.saini-3arQi8VN3Tc@public.gmane.org>
> ---
>  Documentation/devicetree/bindings/arm/fsl.txt | 7 +++++++
>  1 file changed, 7 insertions(+)

Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* [RESEND][PATCH 2/6] dt-bindings: pci: Update bindings for LS2088A
From: Rob Herring @ 2016-11-14 17:12 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <1478597664-14799-3-git-send-email-abhimanyu.saini@nxp.com>

On Tue, Nov 08, 2016 at 03:04:20PM +0530, Abhimanyu Saini wrote:
> Signed-off-by: Priyanka Jain <priyanka.jain@nxp.com>
> Signed-off-by: Ashish Kumar <ashish.kumar@nxp.com>
> Signed-off-by: Abhimanyu Saini <abhimanyu.saini@nxp.com>
> ---
>  Documentation/devicetree/bindings/pci/layerscape-pci.txt | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)

Acked-by: Rob Herring <robh@kernel.org>

^ permalink raw reply

* Re: [RESEND][PATCH 2/6] dt-bindings: pci: Update bindings for LS2088A
From: Rob Herring @ 2016-11-14 17:12 UTC (permalink / raw)
  To: Abhimanyu Saini
  Cc: devicetree-u79uwXL29TY76Z2rM5mHXA, mark.rutland-5wv7dgnIgG8,
	shawnguo-DgEjT+Ai2ygdnm+yROfE0A, scott.wood-3arQi8VN3Tc,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Priyanka Jain,
	Ashish Kumar
In-Reply-To: <1478597664-14799-3-git-send-email-abhimanyu.saini-3arQi8VN3Tc@public.gmane.org>

On Tue, Nov 08, 2016 at 03:04:20PM +0530, Abhimanyu Saini wrote:
> Signed-off-by: Priyanka Jain <priyanka.jain-3arQi8VN3Tc@public.gmane.org>
> Signed-off-by: Ashish Kumar <ashish.kumar-3arQi8VN3Tc@public.gmane.org>
> Signed-off-by: Abhimanyu Saini <abhimanyu.saini-3arQi8VN3Tc@public.gmane.org>
> ---
>  Documentation/devicetree/bindings/pci/layerscape-pci.txt | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)

Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: debugfs question...
From: Mike Marshall @ 2016-11-14 17:12 UTC (permalink / raw)
  To: Nicolai Stange
  Cc: Greg KH, Al Viro, linux-fsdevel, Linus Torvalds,
	Martin Brandenburg
In-Reply-To: <877f876wo9.fsf@gmail.com>

OK,

I did this:

diff --git a/fs/orangefs/orangefs-debugfs.c b/fs/orangefs/orangefs-debugfs.c
index d484068..38887cc 100644
--- a/fs/orangefs/orangefs-debugfs.c
+++ b/fs/orangefs/orangefs-debugfs.c
@@ -114,6 +114,7 @@ static ssize_t orangefs_debug_write(struct file *,
 };

 const struct file_operations debug_help_fops = {
+       .owner          = THIS_MODULE,
        .open           = orangefs_debug_help_open,
        .read           = seq_read,
        .release        = seq_release,
@@ -121,6 +122,7 @@ static ssize_t orangefs_debug_write(struct file *,
 };

 static const struct file_operations kernel_debug_fops = {
+       .owner          = THIS_MODULE,
        .open           = orangefs_debug_open,
        .read           = orangefs_debug_read,
        .write          = orangefs_debug_write,

I changed my little tester program to read from
/sys/kernel/debug/orangefs/debug-help a byte at a time and sleep for
a second between bytes. Now I get nice error messages if I try
to unload the module while someone is reading debug-help, and it
unloads as normal when the reader is done:

[root@be1 hubcap]# rmmod orangefs.ko
rmmod: ERROR: Module orangefs is in use
[root@be1 hubcap]# rmmod orangefs.ko
rmmod: ERROR: Module orangefs is in use
[root@be1 hubcap]# rmmod orangefs.ko
rmmod: ERROR: Module orangefs is in use
[root@be1 hubcap]# rmmod orangefs.ko
[root@be1 hubcap]#

If this seems right, I'll see about getting it pulled...

Thanks!

-Mike

On Sun, Nov 13, 2016 at 1:51 PM, Nicolai Stange <nicstange@gmail.com> wrote:
> Hi again,
>
> bad news: my previous analysis was completely wrong, c.f. below.
> Good news (from my point of view): debugfs is correct, no fix needed for
> it.
>
> Apologies for the confusion...
>
>
> Nicolai Stange <nicstange@gmail.com> writes:
>
>> Greg KH <greg@kroah.com> writes:
>>
>>> On Mon, Oct 31, 2016 at 02:32:56PM -0400, Mike Marshall wrote:
>>>
>>>> But... really bad things happen if someone unloads the Orangefs
>>>> module after my test program does the open and before the read
>>>> starts. So I picked another debugfs-using-filesystem (f2fs) and
>>>> pointed my tester-program at /sys/kernel/debug/f2fs/status, and
>>>> the same bad thing happens there.
>>
>> [...]
>>
>>>> [ 1240.144316] Call Trace:
>>>> [ 1240.144450]  [<ffffffff8122907f>] __fput+0xdf/0x1d0
>>>> [ 1240.144704]  [<ffffffff812291ae>] ____fput+0xe/0x10
>>>> [ 1240.144962]  [<ffffffff810b97de>] task_work_run+0x8e/0xc0
>>>> [ 1240.145243]  [<ffffffff8109b98e>] do_exit+0x2ae/0xae0
>>
>>
>> Thank you very much for this detailed report!
>>
>> At least for the .../f2fs/status file, your splat at fput() can be
>> readily explained with the full proxy's releaser not being protected
>> against file removals in any way.
>>
>> Partly this is on purpose, c.f. the comment in full_proxy_release().
>>
>> However, I should have at least tried to acquire a reference to the
>> owning module before accessing some static struct file_operations or
>> even calling some ->release() within it. Meh.
>
> This is what I got wrong: debugfs does acquire the needed references
> correctly (details below). For the case of f2fs' "status" file, the
> file_operations ->owner is simply not set as it should have been,
> i.e. to THIS_MODULE.
>
>
> Details on debugfs' reference acquisition:
> The open proxy, full_proxy_open(), gets a reference to the "real"
> file_operations, hence to its module. (Only in its error path it
> releases it again).
>
> full_proxy_release() is in charge of dropping that reference again, but
> only *after* it has attempted to call the "real" ->release().
>
> So, as long as a file has been (successfully) opened, a reference to the
> original file_operation's ->owner is owned, preventing it from getting
> unloaded.
>
>
> Can you confirm that you didn't set ->owner in your Orangefs related
> tests, too?
>
>
> Thanks,
>
> Nicolai

^ permalink raw reply related

* new helpers to clean up extent tree lookups
From: Christoph Hellwig @ 2016-11-14 17:12 UTC (permalink / raw)
  To: linux-xfs

Now that Eric send out xfs_iext_count before I could get to it I need
to get my other iext helpers out ASAP before running into another
rebase :)

This series adds two new helpers that make iterating the extent list
a lot cleaner.  They are the first step towards better abstracting out
access to the extent list and thus allowing us to experiment with
better data structures for it.  The next step is the manipulations
in xfs_bmap_{add,del}_extent*, which will be more work than these
helpers, but I have some of that in progress.

^ permalink raw reply

* [PATCH 01/14] xfs: new inode extent list lookup helpers
From: Christoph Hellwig @ 2016-11-14 17:12 UTC (permalink / raw)
  To: linux-xfs
In-Reply-To: <1479143565-30615-1-git-send-email-hch@lst.de>

xfs_iext_lookup_extent looks up a single extent at the passed in offset,
and returns the extent covering the area, or the one behind it in case
of a hole, as well as the index of the returned extent in arguments,
as well as a simple bool as return value that is set to false if no
extent could be found because the offset is behind EOF.  It is a simpler
replacement for xfs_bmap_search_extent that leaves looking up the rarely
needed previous extent to the caller and has a nicer calling convention.

xfs_iext_get_extent is a helper for iterating over the extent list,
it takes an extent index as input, and returns the extent at that index
in it's expanded form in an argument if it exists.  The actual return
value is a bool whether the index is valid or not.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_inode_fork.c | 46 ++++++++++++++++++++++++++++++++++++++++++
 fs/xfs/libxfs/xfs_inode_fork.h |  6 ++++++
 2 files changed, 52 insertions(+)

diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
index 5fbe24c..749fbfb 100644
--- a/fs/xfs/libxfs/xfs_inode_fork.c
+++ b/fs/xfs/libxfs/xfs_inode_fork.c
@@ -2003,3 +2003,49 @@ xfs_ifork_init_cow(
 	ip->i_cformat = XFS_DINODE_FMT_EXTENTS;
 	ip->i_cnextents = 0;
 }
+
+/*
+ * Lookup the extent covering bno.
+ *
+ * If there is an extent covering bno return the extent index, and store the
+ * expanded extent structure in *gotp, and the extent indes in *idx.
+ * If there is no extent covering bno, but there is an extent after it (e.g.
+ * it lies in a hole) return that extent in *gotp and its index in *idx
+ * instead.
+ * If bno is beyond the last extent return false, and return the index after
+ * the last valid index in *idxp.
+ */
+bool
+xfs_iext_lookup_extent(
+	struct xfs_inode	*ip,
+	struct xfs_ifork	*ifp,
+	xfs_fileoff_t		bno,
+	xfs_extnum_t		*idxp,
+	struct xfs_bmbt_irec	*gotp)
+{
+	struct xfs_bmbt_rec_host *ep;
+
+	XFS_STATS_INC(ip->i_mount, xs_look_exlist);
+
+	ep = xfs_iext_bno_to_ext(ifp, bno, idxp);
+	if (!ep)
+		return false;
+	xfs_bmbt_get_all(ep, gotp);
+	return true;
+}
+
+/*
+ * Return true if there is an extent at index idx, and return the expanded
+ * extent structure at idx in that case.  Else return false.
+ */
+bool
+xfs_iext_get_extent(
+	struct xfs_ifork	*ifp,
+	xfs_extnum_t		idx,
+	struct xfs_bmbt_irec	*gotp)
+{
+	if (idx < 0 || idx >= xfs_iext_count(ifp))
+		return false;
+	xfs_bmbt_get_all(xfs_iext_get_ext(ifp, idx), gotp);
+	return true;
+}
diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
index 8bf112e..7fb8365 100644
--- a/fs/xfs/libxfs/xfs_inode_fork.h
+++ b/fs/xfs/libxfs/xfs_inode_fork.h
@@ -182,6 +182,12 @@ void		xfs_iext_irec_compact_pages(struct xfs_ifork *);
 void		xfs_iext_irec_compact_full(struct xfs_ifork *);
 void		xfs_iext_irec_update_extoffs(struct xfs_ifork *, int, int);
 
+bool		xfs_iext_lookup_extent(struct xfs_inode *ip,
+			struct xfs_ifork *ifp, xfs_fileoff_t bno,
+			xfs_extnum_t *idxp, struct xfs_bmbt_irec *gotp);
+bool		xfs_iext_get_extent(struct xfs_ifork *ifp, xfs_extnum_t idx,
+			struct xfs_bmbt_irec *gotp);
+
 extern struct kmem_zone	*xfs_ifork_zone;
 
 extern void xfs_ifork_init_cow(struct xfs_inode *ip);
-- 
2.1.4


^ permalink raw reply related

* [PATCH 02/14] xfs: cleanup xfs_bmap_last_before
From: Christoph Hellwig @ 2016-11-14 17:12 UTC (permalink / raw)
  To: linux-xfs
In-Reply-To: <1479143565-30615-1-git-send-email-hch@lst.de>

Rewrite the function using xfs_iext_lookup_extent and xfs_iext_get_extent,
and massage the flow into something easily understandable.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_bmap.c | 64 ++++++++++++++++++++++++------------------------
 1 file changed, 32 insertions(+), 32 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 5c3c4dd..98f490b 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -1523,44 +1523,44 @@ xfs_bmap_first_unused(
  */
 int						/* error */
 xfs_bmap_last_before(
-	xfs_trans_t	*tp,			/* transaction pointer */
-	xfs_inode_t	*ip,			/* incore inode */
-	xfs_fileoff_t	*last_block,		/* last block */
-	int		whichfork)		/* data or attr fork */
+	struct xfs_trans	*tp,		/* transaction pointer */
+	struct xfs_inode	*ip,		/* incore inode */
+	xfs_fileoff_t		*last_block,	/* last block */
+	int			whichfork)	/* data or attr fork */
 {
-	xfs_fileoff_t	bno;			/* input file offset */
-	int		eof;			/* hit end of file */
-	xfs_bmbt_rec_host_t *ep;		/* pointer to last extent */
-	int		error;			/* error return value */
-	xfs_bmbt_irec_t	got;			/* current extent value */
-	xfs_ifork_t	*ifp;			/* inode fork pointer */
-	xfs_extnum_t	lastx;			/* last extent used */
-	xfs_bmbt_irec_t	prev;			/* previous extent value */
+	struct xfs_ifork	*ifp = XFS_IFORK_PTR(ip, whichfork);
+	struct xfs_bmbt_irec	got;
+	xfs_extnum_t		idx;
+	int			error;
 
-	if (XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_BTREE &&
-	    XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_EXTENTS &&
-	    XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_LOCAL)
-	       return -EIO;
-	if (XFS_IFORK_FORMAT(ip, whichfork) == XFS_DINODE_FMT_LOCAL) {
+	switch (XFS_IFORK_FORMAT(ip, whichfork)) {
+	case XFS_DINODE_FMT_LOCAL:
 		*last_block = 0;
 		return 0;
+	case XFS_DINODE_FMT_BTREE:
+	case XFS_DINODE_FMT_EXTENTS:
+		break;
+	default:
+		return -EIO;
 	}
-	ifp = XFS_IFORK_PTR(ip, whichfork);
-	if (!(ifp->if_flags & XFS_IFEXTENTS) &&
-	    (error = xfs_iread_extents(tp, ip, whichfork)))
-		return error;
-	bno = *last_block - 1;
-	ep = xfs_bmap_search_extents(ip, bno, whichfork, &eof, &lastx, &got,
-		&prev);
-	if (eof || xfs_bmbt_get_startoff(ep) > bno) {
-		if (prev.br_startoff == NULLFILEOFF)
-			*last_block = 0;
-		else
-			*last_block = prev.br_startoff + prev.br_blockcount;
+
+	if (!(ifp->if_flags & XFS_IFEXTENTS)) {
+		error = xfs_iread_extents(tp, ip, whichfork);
+		if (error)
+			return error;
 	}
-	/*
-	 * Otherwise *last_block is already the right answer.
-	 */
+
+	if (xfs_iext_lookup_extent(ip, ifp, *last_block - 1, &idx, &got)) {
+		if (got.br_startoff <= *last_block - 1)
+			return 0;
+	}
+
+	if (xfs_iext_get_extent(ifp, idx - 1, &got)) {
+		*last_block = got.br_startoff + got.br_blockcount;
+		return 0;
+	}
+
+	*last_block = 0;
 	return 0;
 }
 
-- 
2.1.4


^ permalink raw reply related

* [PATCH 03/14] xfs: use new extent lookup helpers in xfs_bmapi_read
From: Christoph Hellwig @ 2016-11-14 17:12 UTC (permalink / raw)
  To: linux-xfs
In-Reply-To: <1479143565-30615-1-git-send-email-hch@lst.de>

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_bmap.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 98f490b..1a0fee4 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -4145,12 +4145,11 @@ xfs_bmapi_read(
 	struct xfs_mount	*mp = ip->i_mount;
 	struct xfs_ifork	*ifp;
 	struct xfs_bmbt_irec	got;
-	struct xfs_bmbt_irec	prev;
 	xfs_fileoff_t		obno;
 	xfs_fileoff_t		end;
-	xfs_extnum_t		lastx;
+	xfs_extnum_t		idx;
 	int			error;
-	int			eof;
+	bool			eof = false;
 	int			n = 0;
 	int			whichfork = xfs_bmapi_whichfork(flags);
 
@@ -4190,7 +4189,8 @@ xfs_bmapi_read(
 			return error;
 	}
 
-	xfs_bmap_search_extents(ip, bno, whichfork, &eof, &lastx, &got, &prev);
+	if (!xfs_iext_lookup_extent(ip, ifp, bno, &idx, &got))
+		eof = true;
 	end = bno + len;
 	obno = bno;
 
@@ -4221,10 +4221,8 @@ xfs_bmapi_read(
 			break;
 
 		/* Else go on to the next record. */
-		if (++lastx < xfs_iext_count(ifp))
-			xfs_bmbt_get_all(xfs_iext_get_ext(ifp, lastx), &got);
-		else
-			eof = 1;
+		if (!xfs_iext_get_extent(ifp, ++idx, &got))
+			eof = true;
 	}
 	*nmap = n;
 	return 0;
-- 
2.1.4


^ permalink raw reply related

* [PATCH 04/14] xfs: use new extent lookup helpers in xfs_bmapi_write
From: Christoph Hellwig @ 2016-11-14 17:12 UTC (permalink / raw)
  To: linux-xfs
In-Reply-To: <1479143565-30615-1-git-send-email-hch@lst.de>

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_bmap.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 1a0fee4..9a8621d 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -4561,7 +4561,7 @@ xfs_bmapi_write(
 	struct xfs_ifork	*ifp;
 	struct xfs_bmalloca	bma = { NULL };	/* args for xfs_bmap_alloc */
 	xfs_fileoff_t		end;		/* end of mapped file region */
-	int			eof;		/* after the end of extents */
+	bool			eof = false;	/* after the end of extents */
 	int			error;		/* error return */
 	int			n;		/* current extent index */
 	xfs_fileoff_t		obno;		/* old block number (offset) */
@@ -4639,12 +4639,14 @@ xfs_bmapi_write(
 			goto error0;
 	}
 
-	xfs_bmap_search_extents(ip, bno, whichfork, &eof, &bma.idx, &bma.got,
-				&bma.prev);
 	n = 0;
 	end = bno + len;
 	obno = bno;
 
+	if (!xfs_iext_lookup_extent(ip, ifp, bno, &bma.idx, &bma.got))
+		eof = true;
+	if (!xfs_iext_get_extent(ifp, bma.idx - 1, &bma.prev))
+		bma.prev.br_startoff = NULLFILEOFF;
 	bma.tp = tp;
 	bma.ip = ip;
 	bma.total = total;
@@ -4731,11 +4733,8 @@ xfs_bmapi_write(
 
 		/* Else go on to the next record. */
 		bma.prev = bma.got;
-		if (++bma.idx < xfs_iext_count(ifp)) {
-			xfs_bmbt_get_all(xfs_iext_get_ext(ifp, bma.idx),
-					 &bma.got);
-		} else
-			eof = 1;
+		if (!xfs_iext_get_extent(ifp, ++bma.idx, &bma.got))
+			eof = true;
 	}
 	*nmap = n;
 
-- 
2.1.4


^ permalink raw reply related

* [PATCH 05/14] xfs: use new extent lookup helpers in __xfs_bunmapi
From: Christoph Hellwig @ 2016-11-14 17:12 UTC (permalink / raw)
  To: linux-xfs
In-Reply-To: <1479143565-30615-1-git-send-email-hch@lst.de>

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_bmap.c | 39 ++++++++++++---------------------------
 1 file changed, 12 insertions(+), 27 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 9a8621d..18de89c 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -5433,8 +5433,6 @@ __xfs_bunmapi(
 {
 	xfs_btree_cur_t		*cur;		/* bmap btree cursor */
 	xfs_bmbt_irec_t		del;		/* extent being deleted */
-	int			eof;		/* is deleting at eof */
-	xfs_bmbt_rec_host_t	*ep;		/* extent record pointer */
 	int			error;		/* error return value */
 	xfs_extnum_t		extno;		/* extent number in list */
 	xfs_bmbt_irec_t		got;		/* current extent record */
@@ -5444,7 +5442,6 @@ __xfs_bunmapi(
 	int			logflags;	/* transaction logging flags */
 	xfs_extlen_t		mod;		/* rt extent offset */
 	xfs_mount_t		*mp;		/* mount structure */
-	xfs_bmbt_irec_t		prev;		/* previous extent record */
 	xfs_fileoff_t		start;		/* first file offset deleted */
 	int			tmp_logflags;	/* partial logging flags */
 	int			wasdel;		/* was a delayed alloc extent */
@@ -5483,18 +5480,17 @@ __xfs_bunmapi(
 	isrt = (whichfork == XFS_DATA_FORK) && XFS_IS_REALTIME_INODE(ip);
 	start = bno;
 	bno = start + len - 1;
-	ep = xfs_bmap_search_extents(ip, bno, whichfork, &eof, &lastx, &got,
-		&prev);
 
 	/*
 	 * Check to see if the given block number is past the end of the
 	 * file, back up to the last block if so...
 	 */
-	if (eof) {
-		ep = xfs_iext_get_ext(ifp, --lastx);
-		xfs_bmbt_get_all(ep, &got);
+	if (!xfs_iext_lookup_extent(ip, ifp, bno, &lastx, &got)) {
+		ASSERT(lastx > 0);
+		xfs_iext_get_extent(ifp, --lastx, &got);
 		bno = got.br_startoff + got.br_blockcount - 1;
 	}
+
 	logflags = 0;
 	if (ifp->if_flags & XFS_IFBROOT) {
 		ASSERT(XFS_IFORK_FORMAT(ip, whichfork) == XFS_DINODE_FMT_BTREE);
@@ -5525,8 +5521,7 @@ __xfs_bunmapi(
 		if (got.br_startoff > bno) {
 			if (--lastx < 0)
 				break;
-			ep = xfs_iext_get_ext(ifp, lastx);
-			xfs_bmbt_get_all(ep, &got);
+			xfs_iext_get_extent(ifp, lastx, &got);
 		}
 		/*
 		 * Is the last block of this extent before the range
@@ -5540,7 +5535,6 @@ __xfs_bunmapi(
 		 * Then deal with the (possibly delayed) allocated space
 		 * we found.
 		 */
-		ASSERT(ep != NULL);
 		del = got;
 		wasdel = isnullstartblock(del.br_startblock);
 		if (got.br_startoff < start) {
@@ -5621,15 +5615,11 @@ __xfs_bunmapi(
 				 */
 				ASSERT(bno >= del.br_blockcount);
 				bno -= del.br_blockcount;
-				if (got.br_startoff > bno) {
-					if (--lastx >= 0) {
-						ep = xfs_iext_get_ext(ifp,
-								      lastx);
-						xfs_bmbt_get_all(ep, &got);
-					}
-				}
+				if (got.br_startoff > bno && --lastx >= 0)
+					xfs_iext_get_extent(ifp, lastx, &got);
 				continue;
 			} else if (del.br_state == XFS_EXT_UNWRITTEN) {
+				xfs_bmbt_irec_t		prev;
 				/*
 				 * This one is already unwritten.
 				 * It must have a written left neighbor.
@@ -5637,8 +5627,7 @@ __xfs_bunmapi(
 				 * try again.
 				 */
 				ASSERT(lastx > 0);
-				xfs_bmbt_get_all(xfs_iext_get_ext(ifp,
-						lastx - 1), &prev);
+				xfs_iext_get_extent(ifp, lastx - 1, &prev);
 				ASSERT(prev.br_state == XFS_EXT_NORM);
 				ASSERT(!isnullstartblock(prev.br_startblock));
 				ASSERT(del.br_startblock ==
@@ -5736,13 +5725,9 @@ __xfs_bunmapi(
 		 */
 		if (bno != (xfs_fileoff_t)-1 && bno >= start) {
 			if (lastx >= 0) {
-				ep = xfs_iext_get_ext(ifp, lastx);
-				if (xfs_bmbt_get_startoff(ep) > bno) {
-					if (--lastx >= 0)
-						ep = xfs_iext_get_ext(ifp,
-								      lastx);
-				}
-				xfs_bmbt_get_all(ep, &got);
+				xfs_iext_get_extent(ifp, lastx, &got);
+				if (got.br_startoff > bno && --lastx >= 0)
+					xfs_iext_get_extent(ifp, lastx, &got);
 			}
 			extno++;
 		}
-- 
2.1.4


^ permalink raw reply related

* [PATCH 06/14] xfs: remove prev argument to xfs_bmapi_reserve_delalloc
From: Christoph Hellwig @ 2016-11-14 17:12 UTC (permalink / raw)
  To: linux-xfs
In-Reply-To: <1479143565-30615-1-git-send-email-hch@lst.de>

We can easily lookup the previous extent for the cases where we need it,
which saves the callers from looking it up for us later in the series.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_bmap.c | 8 ++++++--
 fs/xfs/libxfs/xfs_bmap.h | 3 +--
 fs/xfs/xfs_iomap.c       | 3 +--
 fs/xfs/xfs_reflink.c     | 2 +-
 4 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 18de89c..4aa9c07 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -4235,7 +4235,6 @@ xfs_bmapi_reserve_delalloc(
 	xfs_fileoff_t		aoff,
 	xfs_filblks_t		len,
 	struct xfs_bmbt_irec	*got,
-	struct xfs_bmbt_irec	*prev,
 	xfs_extnum_t		*lastx,
 	int			eof)
 {
@@ -4257,7 +4256,12 @@ xfs_bmapi_reserve_delalloc(
 	else
 		extsz = xfs_get_extsz_hint(ip);
 	if (extsz) {
-		error = xfs_bmap_extsize_align(mp, got, prev, extsz, rt, eof,
+		struct xfs_bmbt_irec	prev;
+
+		if (!xfs_iext_get_extent(ifp, *lastx - 1, &prev))
+			prev.br_startoff = NULLFILEOFF;
+
+		error = xfs_bmap_extsize_align(mp, got, &prev, extsz, rt, eof,
 					       1, 0, &aoff, &alen);
 		ASSERT(!error);
 	}
diff --git a/fs/xfs/libxfs/xfs_bmap.h b/fs/xfs/libxfs/xfs_bmap.h
index 7cae6ec..e3c2b5a 100644
--- a/fs/xfs/libxfs/xfs_bmap.h
+++ b/fs/xfs/libxfs/xfs_bmap.h
@@ -243,8 +243,7 @@ struct xfs_bmbt_rec_host *
 		struct xfs_bmbt_irec *gotp, struct xfs_bmbt_irec *prevp);
 int	xfs_bmapi_reserve_delalloc(struct xfs_inode *ip, int whichfork,
 		xfs_fileoff_t aoff, xfs_filblks_t len,
-		struct xfs_bmbt_irec *got, struct xfs_bmbt_irec *prev,
-		xfs_extnum_t *lastx, int eof);
+		struct xfs_bmbt_irec *got, xfs_extnum_t *lastx, int eof);
 
 enum xfs_bmap_intent_type {
 	XFS_BMAP_MAP = 1,
diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index 436e109..59ffcac 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -622,8 +622,7 @@ xfs_file_iomap_begin_delay(
 
 retry:
 	error = xfs_bmapi_reserve_delalloc(ip, XFS_DATA_FORK, offset_fsb,
-			end_fsb - offset_fsb, &got,
-			&prev, &idx, eof);
+			end_fsb - offset_fsb, &got, &idx, eof);
 	switch (error) {
 	case 0:
 		break;
diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index 0edf835..52cdfba 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -293,7 +293,7 @@ xfs_reflink_reserve_cow(
 
 retry:
 	error = xfs_bmapi_reserve_delalloc(ip, XFS_COW_FORK, imap->br_startoff,
-			end_fsb - imap->br_startoff, &got, &prev, &idx, eof);
+			end_fsb - imap->br_startoff, &got, &idx, eof);
 	switch (error) {
 	case 0:
 		break;
-- 
2.1.4


^ permalink raw reply related

* [PATCH 07/14] xfs: use new extent lookup helpers xfs_file_iomap_begin_delay
From: Christoph Hellwig @ 2016-11-14 17:12 UTC (permalink / raw)
  To: linux-xfs
In-Reply-To: <1479143565-30615-1-git-send-email-hch@lst.de>

And only lookup the previous extent inside xfs_iomap_prealloc_size
if we actually need it.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_iomap.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index 59ffcac..2272190 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -395,11 +395,12 @@ xfs_iomap_prealloc_size(
 	struct xfs_inode	*ip,
 	loff_t			offset,
 	loff_t			count,
-	xfs_extnum_t		idx,
-	struct xfs_bmbt_irec	*prev)
+	xfs_extnum_t		idx)
 {
 	struct xfs_mount	*mp = ip->i_mount;
+	struct xfs_ifork	*ifp = XFS_IFORK_PTR(ip, XFS_DATA_FORK);
 	xfs_fileoff_t		offset_fsb = XFS_B_TO_FSBT(mp, offset);
+	struct xfs_bmbt_irec	prev;
 	int			shift = 0;
 	int64_t			freesp;
 	xfs_fsblock_t		qblocks;
@@ -419,8 +420,8 @@ xfs_iomap_prealloc_size(
 	 */
 	if ((mp->m_flags & XFS_MOUNT_DFLT_IOSIZE) ||
 	    XFS_ISIZE(ip) < XFS_FSB_TO_B(mp, mp->m_dalign) ||
-	    idx == 0 ||
-	    prev->br_startoff + prev->br_blockcount < offset_fsb)
+	    !xfs_iext_get_extent(ifp, idx - 1, &prev) ||
+	    prev.br_startoff + prev.br_blockcount < offset_fsb)
 		return mp->m_writeio_blocks;
 
 	/*
@@ -439,8 +440,8 @@ xfs_iomap_prealloc_size(
 	 * always extends to MAXEXTLEN rather than falling short due to things
 	 * like stripe unit/width alignment of real extents.
 	 */
-	if (prev->br_blockcount <= (MAXEXTLEN >> 1))
-		alloc_blocks = prev->br_blockcount << 1;
+	if (prev.br_blockcount <= (MAXEXTLEN >> 1))
+		alloc_blocks = prev.br_blockcount << 1;
 	else
 		alloc_blocks = XFS_B_TO_FSB(mp, offset);
 	if (!alloc_blocks)
@@ -538,7 +539,6 @@ xfs_file_iomap_begin_delay(
 	xfs_fileoff_t		end_fsb, orig_end_fsb;
 	int			error = 0, eof = 0;
 	struct xfs_bmbt_irec	got;
-	struct xfs_bmbt_irec	prev;
 	xfs_extnum_t		idx;
 
 	ASSERT(!XFS_IS_REALTIME_INODE(ip));
@@ -563,8 +563,7 @@ xfs_file_iomap_begin_delay(
 			goto out_unlock;
 	}
 
-	xfs_bmap_search_extents(ip, offset_fsb, XFS_DATA_FORK, &eof, &idx,
-			&got, &prev);
+	eof = !xfs_iext_lookup_extent(ip, ifp, offset_fsb, &idx, &got);
 	if (!eof && got.br_startoff <= offset_fsb) {
 		if (xfs_is_reflink_inode(ip)) {
 			bool		shared;
@@ -601,8 +600,7 @@ xfs_file_iomap_begin_delay(
 	if (eof) {
 		xfs_fsblock_t	prealloc_blocks;
 
-		prealloc_blocks =
-			xfs_iomap_prealloc_size(ip, offset, count, idx, &prev);
+		prealloc_blocks = xfs_iomap_prealloc_size(ip, offset, count, idx);
 		if (prealloc_blocks) {
 			xfs_extlen_t	align;
 			xfs_off_t	end_offset;
-- 
2.1.4


^ permalink raw reply related

* [PATCH 08/14] xfs: use new extent lookup helpers in __xfs_reflink_reserve_cow
From: Christoph Hellwig @ 2016-11-14 17:12 UTC (permalink / raw)
  To: linux-xfs
In-Reply-To: <1479143565-30615-1-git-send-email-hch@lst.de>

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_reflink.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index 52cdfba..35e02ce 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -243,10 +243,11 @@ xfs_reflink_reserve_cow(
 	struct xfs_bmbt_irec	*imap,
 	bool			*shared)
 {
-	struct xfs_bmbt_irec	got, prev;
+	struct xfs_ifork	*ifp = XFS_IFORK_PTR(ip, XFS_COW_FORK);
+	struct xfs_bmbt_irec	got;
 	xfs_fileoff_t		end_fsb, orig_end_fsb;
-	int			eof = 0, error = 0;
-	bool			trimmed;
+	int			error = 0;
+	bool			eof = false, trimmed;
 	xfs_extnum_t		idx;
 	xfs_extlen_t		align;
 
@@ -258,8 +259,9 @@ xfs_reflink_reserve_cow(
 	 * extent list is generally faster than going out to the shared extent
 	 * tree.
 	 */
-	xfs_bmap_search_extents(ip, imap->br_startoff, XFS_COW_FORK, &eof, &idx,
-			&got, &prev);
+
+	if (!xfs_iext_lookup_extent(ip, ifp, imap->br_startoff, &idx, &got))
+		eof = true;
 	if (!eof && got.br_startoff <= imap->br_startoff) {
 		trace_xfs_reflink_cow_found(ip, imap);
 		xfs_trim_extent(imap, got.br_startoff, got.br_blockcount);
-- 
2.1.4


^ permalink raw reply related

* Re: [Qemu-devel] [RFC 0/3] aio: experimental virtio-blk polling mode
From: Fam Zheng @ 2016-11-14 17:13 UTC (permalink / raw)
  To: Stefan Hajnoczi
  Cc: Paolo Bonzini, Karl Rister, Stefan Hajnoczi, qemu-devel,
	Andrew Theurer
In-Reply-To: <20161114170611.GE1352@stefanha-x1.localdomain>

On Mon, 11/14 17:06, Stefan Hajnoczi wrote:
> On Mon, Nov 14, 2016 at 04:29:49PM +0100, Paolo Bonzini wrote:
> > On 14/11/2016 16:26, Stefan Hajnoczi wrote:
> > > On Fri, Nov 11, 2016 at 01:59:25PM -0600, Karl Rister wrote:
> > >> QEMU_AIO_POLL_MAX_NS      IOPs
> > >>                unset    31,383
> > >>                    1    46,860
> > >>                    2    46,440
> > >>                    4    35,246
> > >>                    8    34,973
> > >>                   16    46,794
> > >>                   32    46,729
> > >>                   64    35,520
> > >>                  128    45,902
> > > 
> > > The environment variable is in nanoseconds.  The range of values you
> > > tried are very small (all <1 usec).  It would be interesting to try
> > > larger values in the ballpark of the latencies you have traced.  For
> > > example 2000, 4000, 8000, 16000, and 32000 ns.
> > > 
> > > Very interesting that QEMU_AIO_POLL_MAX_NS=1 performs so well without
> > > much CPU overhead.
> > 
> > That basically means "avoid a syscall if you already know there's
> > something to do", so in retrospect it's not that surprising.  Still
> > interesting though, and it means that the feature is useful even if you
> > don't have CPU to waste.
> 
> Can you spell out which syscall you mean?  Reading the ioeventfd?
> 
> The benchmark uses virtio-blk dataplane and iodepth=1 so there shouldn't
> be much IOThread event loop activity besides the single I/O request.
> 
> The reason this puzzles me is that I wouldn't expect poll to succeed
> with QEMU_AIO_POLL_MAX_NS and iodepth=1.

I see the guest shouldn't send more requests, but isn't it possible for
the linux-aio poll to succeed?

Fam

> 
> Thanks,
> Stefan

^ permalink raw reply


This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.