Re: [PATCH 05/17] drm/mgag200: Clean up mga_set_start_address()

[Sam Ravnborg <sam@ravnborg.org>]

Hi Thomas,

On Wed, Apr 29, 2020 at 04:32:26PM +0200, Thomas Zimmermann wrote:
> All register names and fields are now named according to the
> MGA programming manuals. The function doesn't need the CRTC, so
> callers pass in the device structure directly. The logging now
> uses device-specific macros.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/mgag200/mgag200_drv.h  |  5 ++
>  drivers/gpu/drm/mgag200/mgag200_mode.c | 82 +++++++++++++++-----------
>  2 files changed, 53 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h
> index 4403145e3593c..9b957d9fc7e04 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_drv.h
> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
> @@ -61,6 +61,11 @@
>  		WREG8(MGAREG_CRTC_DATA, v);			\
>  	} while (0)						\
>  
> +#define RREG_ECRT(reg, v)					\
> +	do {							\
> +		WREG8(MGAREG_CRTCEXT_INDEX, reg);		\
> +		v = RREG8(MGAREG_CRTCEXT_DATA);			\
> +	} while (0)						\
>  
>  #define WREG_ECRT(reg, v)					\
>  	do {							\
> diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
> index 3d894b37a0812..b16a73c8617d6 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_mode.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
> @@ -819,49 +819,53 @@ static void mga_g200wb_commit(struct drm_crtc *crtc)
>  }
>  
>  /*
> -   This is how the framebuffer base address is stored in g200 cards:
> -   * Assume @offset is the gpu_addr variable of the framebuffer object
> -   * Then addr is the number of _pixels_ (not bytes) from the start of
> -     VRAM to the first pixel we want to display. (divided by 2 for 32bit
> -     framebuffers)
> -   * addr is stored in the CRTCEXT0, CRTCC and CRTCD registers
> -   addr<20> -> CRTCEXT0<6>
> -   addr<19-16> -> CRTCEXT0<3-0>
> -   addr<15-8> -> CRTCC<7-0>
> -   addr<7-0> -> CRTCD<7-0>
> -   CRTCEXT0 has to be programmed last to trigger an update and make the
> -   new addr variable take effect.
> + * This is how the framebuffer base address is stored in g200 cards:
> + *   * Assume @offset is the gpu_addr variable of the framebuffer object
> + *   * Then addr is the number of _pixels_ (not bytes) from the start of
> + *     VRAM to the first pixel we want to display. (divided by 2 for 32bit
> + *     framebuffers)
> + *   * addr is stored in the CRTCEXT0, CRTCC and CRTCD registers
> + *      addr<20> -> CRTCEXT0<6>
> + *      addr<19-16> -> CRTCEXT0<3-0>
> + *      addr<15-8> -> CRTCC<7-0>
> + *      addr<7-0> -> CRTCD<7-0>
> + *
> + *  CRTCEXT0 has to be programmed last to trigger an update and make the
> + *  new addr variable take effect.
>   */
> -static void mga_set_start_address(struct drm_crtc *crtc, unsigned offset)
> +static void mgag200_set_startadd(struct mga_device *mdev,
> +				 unsigned long offset)
>  {
> -	struct mga_device *mdev = crtc->dev->dev_private;
> -	u32 addr;
> -	int count;
> -	u8 crtcext0;
> +	struct drm_device *dev = mdev->dev;
> +	uint32_t startadd;
> +	uint8_t crtcc, crtcd, crtcext0;
>  
> -	while (RREG8(0x1fda) & 0x08);
> -	while (!(RREG8(0x1fda) & 0x08));
> +	startadd = offset / 8;
>  
> -	count = RREG8(MGAREG_VCOUNT) + 2;
> -	while (RREG8(MGAREG_VCOUNT) < count);
> -
> -	WREG8(MGAREG_CRTCEXT_INDEX, 0);
> -	crtcext0 = RREG8(MGAREG_CRTCEXT_DATA);
> -	crtcext0 &= 0xB0;
> -	addr = offset / 8;
> -	/* Can't store addresses any higher than that...
> -	   but we also don't have more than 16MB of memory, so it should be fine. */
> -	WARN_ON(addr > 0x1fffff);
> -	crtcext0 |= (!!(addr & (1<<20)))<<6;
> -	WREG_CRT(0x0d, (u8)(addr & 0xff));
> -	WREG_CRT(0x0c, (u8)(addr >> 8) & 0xff);
> -	WREG_ECRT(0x0, ((u8)(addr >> 16) & 0xf) | crtcext0);
> +	/*
> +	 * Can't store addresses any higher than that, but we also
> +	 * don't have more than 16MB of memory, so it should be fine.
> +	 */
> +	drm_WARN_ON(dev, startadd > 0x1fffff);
> +
> +	RREG_ECRT(0x00, crtcext0);
> +
> +	crtcc = (startadd >> 8) & 0xff;
> +	crtcd = startadd & 0xff;
> +	crtcext0 &= 0xb0;

> +	crtcext0 |= ((startadd >> 14) & BIT(6)) |
It is not so obvious that the value of bit 20 is stored in bit 6 here.

Maybe:
	crtcext0 |= ((startadd & BIT(20) >> 14) |

I would find the above easier to parse.

> +		    ((startadd >> 16) & 0x0f);

> +
> +	WREG_CRT(0x0c, crtcc);
> +	WREG_CRT(0x0d, crtcd);
> +	WREG_ECRT(0x00, crtcext0);
>  }
>  
>  static int mga_crtc_do_set_base(struct drm_crtc *crtc,
>  				struct drm_framebuffer *fb,
>  				int x, int y, int atomic)
>  {
> +	struct mga_device *mdev = crtc->dev->dev_private;
Could you use a crtc_to_mdev() macro here.
So we avoid adding new users of dev_private?

>  	struct drm_gem_vram_object *gbo;
>  	int ret;
>  	s64 gpu_addr;
Make this unsigned long and..

> @@ -882,7 +886,7 @@ static int mga_crtc_do_set_base(struct drm_crtc *crtc,
>  		goto err_drm_gem_vram_unpin;
>  	}
>  
> -	mga_set_start_address(crtc, (u32)gpu_addr);
> +	mgag200_set_startadd(mdev, (unsigned long)gpu_addr);
drop this cast.


>  
>  	return 0;
>  
> @@ -894,6 +898,16 @@ static int mga_crtc_do_set_base(struct drm_crtc *crtc,
>  static int mga_crtc_mode_set_base(struct drm_crtc *crtc, int x, int y,
>  				  struct drm_framebuffer *old_fb)
>  {
> +	struct drm_device *dev = crtc->dev;
> +	struct mga_device *mdev = dev->dev_private;
> +	unsigned int count;
> +
> +	while (RREG8(0x1fda) & 0x08) { }
> +	while (!(RREG8(0x1fda) & 0x08)) { }
> +
> +	count = RREG8(MGAREG_VCOUNT) + 2;
> +	while (RREG8(MGAREG_VCOUNT) < count) { }
> +
>  	return mga_crtc_do_set_base(crtc, old_fb, x, y, 0);
>  }
I do not really see why this code was lifter two functions up.
Before is was deep in mga_set_start_address(), now it is in
mga_crtc_mode_set_base().
Puzzeled?

	Sam



>  
> -- 
> 2.26.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel