Re: [Intel-xe] [PATCH v2 3/9] drm/i915/display: split i915 specific code from intel_fbdev

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Tue, Nov 14, 2023 at 03:04:37PM +0200, Jouni Högander wrote:
> Split out code from intel_fbdev that can not be share between i915 and
> xe. Create new i915 specific source/header file intel_fbdev_fb.[ch] which
> contains this code.
> 
> v2: Add missing forward declarations into intel_fbdev_fb.h
> 
> Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> ---
>  drivers/gpu/drm/i915/Makefile                 |   3 +-
>  drivers/gpu/drm/i915/display/intel_fbdev.c    | 108 ++--------------
>  drivers/gpu/drm/i915/display/intel_fbdev_fb.c | 116 ++++++++++++++++++
>  drivers/gpu/drm/i915/display/intel_fbdev_fb.h |  21 ++++
>  4 files changed, 147 insertions(+), 101 deletions(-)
>  create mode 100644 drivers/gpu/drm/i915/display/intel_fbdev_fb.c
>  create mode 100644 drivers/gpu/drm/i915/display/intel_fbdev_fb.h
> 
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index 3c26e9ae3722..401fc5778843 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -302,7 +302,8 @@ i915-$(CONFIG_ACPI) += \
>  	display/intel_acpi.o \
>  	display/intel_opregion.o
>  i915-$(CONFIG_DRM_FBDEV_EMULATION) += \
> -	display/intel_fbdev.o
> +	display/intel_fbdev.o \
> +	display/intel_fbdev_fb.o
>  
>  # modesetting output/encoder code
>  i915-y += \
> diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c b/drivers/gpu/drm/i915/display/intel_fbdev.c
> index 31d0d695d567..252f254345a2 100644
> --- a/drivers/gpu/drm/i915/display/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/display/intel_fbdev.c
> @@ -43,7 +43,6 @@
>  #include <drm/drm_fourcc.h>
>  #include <drm/drm_gem_framebuffer_helper.h>
>  
> -#include "gem/i915_gem_lmem.h"
>  #include "gem/i915_gem_mman.h"
>  
>  #include "i915_drv.h"
> @@ -51,6 +50,7 @@
>  #include "intel_fb.h"
>  #include "intel_fb_pin.h"
>  #include "intel_fbdev.h"
> +#include "intel_fbdev_fb.h"
>  #include "intel_frontbuffer.h"
>  
>  struct intel_fbdev {
> @@ -146,65 +146,6 @@ static const struct fb_ops intelfb_ops = {
>  	.fb_mmap = intel_fbdev_mmap,
>  };
>  
> -static int intelfb_alloc(struct drm_fb_helper *helper,
> -			 struct drm_fb_helper_surface_size *sizes)
> -{
> -	struct intel_fbdev *ifbdev = to_intel_fbdev(helper);
> -	struct drm_framebuffer *fb;
> -	struct drm_device *dev = helper->dev;
> -	struct drm_i915_private *dev_priv = to_i915(dev);
> -	struct drm_mode_fb_cmd2 mode_cmd = {};
> -	struct drm_i915_gem_object *obj;
> -	int size;
> -
> -	/* we don't do packed 24bpp */
> -	if (sizes->surface_bpp == 24)
> -		sizes->surface_bpp = 32;
> -
> -	mode_cmd.width = sizes->surface_width;
> -	mode_cmd.height = sizes->surface_height;
> -
> -	mode_cmd.pitches[0] = ALIGN(mode_cmd.width *
> -				    DIV_ROUND_UP(sizes->surface_bpp, 8), 64);
> -	mode_cmd.pixel_format = drm_mode_legacy_fb_format(sizes->surface_bpp,
> -							  sizes->surface_depth);
> -
> -	size = mode_cmd.pitches[0] * mode_cmd.height;
> -	size = PAGE_ALIGN(size);
> -
> -	obj = ERR_PTR(-ENODEV);
> -	if (HAS_LMEM(dev_priv)) {
> -		obj = i915_gem_object_create_lmem(dev_priv, size,
> -						  I915_BO_ALLOC_CONTIGUOUS |
> -						  I915_BO_ALLOC_USER);
> -	} else {
> -		/*
> -		 * If the FB is too big, just don't use it since fbdev is not very
> -		 * important and we should probably use that space with FBC or other
> -		 * features.
> -		 *
> -		 * Also skip stolen on MTL as Wa_22018444074 mitigation.
> -		 */
> -		if (!(IS_METEORLAKE(dev_priv)) && size * 2 < dev_priv->dsm.usable_size)
> -			obj = i915_gem_object_create_stolen(dev_priv, size);
> -		if (IS_ERR(obj))
> -			obj = i915_gem_object_create_shmem(dev_priv, size);
> -	}
> -
> -	if (IS_ERR(obj)) {
> -		drm_err(&dev_priv->drm, "failed to allocate framebuffer (%pe)\n", obj);
> -		return PTR_ERR(obj);
> -	}
> -
> -	fb = intel_framebuffer_create(obj, &mode_cmd);
> -	i915_gem_object_put(obj);
> -	if (IS_ERR(fb))
> -		return PTR_ERR(fb);
> -
> -	ifbdev->fb = to_intel_framebuffer(fb);
> -	return 0;
> -}
> -
>  static int intelfb_create(struct drm_fb_helper *helper,
>  			  struct drm_fb_helper_surface_size *sizes)
>  {
> @@ -213,7 +154,6 @@ static int intelfb_create(struct drm_fb_helper *helper,
>  	struct drm_device *dev = helper->dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct pci_dev *pdev = to_pci_dev(dev_priv->drm.dev);
> -	struct i915_ggtt *ggtt = to_gt(dev_priv)->ggtt;
>  	const struct i915_gtt_view view = {
>  		.type = I915_GTT_VIEW_NORMAL,
>  	};
> @@ -222,9 +162,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
>  	struct i915_vma *vma;
>  	unsigned long flags = 0;
>  	bool prealloc = false;
> -	void __iomem *vaddr;
>  	struct drm_i915_gem_object *obj;
> -	struct i915_gem_ww_ctx ww;
>  	int ret;
>  
>  	mutex_lock(&ifbdev->hpd_lock);
> @@ -245,12 +183,13 @@ static int intelfb_create(struct drm_fb_helper *helper,
>  		intel_fb = ifbdev->fb = NULL;
>  	}
>  	if (!intel_fb || drm_WARN_ON(dev, !intel_fb_obj(&intel_fb->base))) {
> +		struct intel_framebuffer *fb;
>  		drm_dbg_kms(&dev_priv->drm,
>  			    "no BIOS fb, allocating a new one\n");
> -		ret = intelfb_alloc(helper, sizes);
> -		if (ret)
> -			return ret;
> -		intel_fb = ifbdev->fb;
> +		fb = to_intel_framebuffer(intel_fbdev_fb_alloc(helper, sizes));
> +		if (IS_ERR(fb))
> +			return PTR_ERR(fb);
> +		intel_fb = ifbdev->fb = fb;
>  	} else {
>  		drm_dbg_kms(&dev_priv->drm, "re-using BIOS fb\n");
>  		prealloc = true;
> @@ -283,49 +222,18 @@ static int intelfb_create(struct drm_fb_helper *helper,
>  	info->fbops = &intelfb_ops;
>  
>  	obj = intel_fb_obj(&intel_fb->base);
> -	if (i915_gem_object_is_lmem(obj)) {
> -		struct intel_memory_region *mem = obj->mm.region;
> -
> -		/* Use fbdev's framebuffer from lmem for discrete */
> -		info->fix.smem_start =
> -			(unsigned long)(mem->io_start +
> -					i915_gem_object_get_dma_address(obj, 0));
> -		info->fix.smem_len = obj->base.size;
> -	} else {
> -		/* Our framebuffer is the entirety of fbdev's system memory */
> -		info->fix.smem_start =
> -			(unsigned long)(ggtt->gmadr.start + i915_ggtt_offset(vma));
> -		info->fix.smem_len = vma->size;
> -	}
> -
> -	for_i915_gem_ww(&ww, ret, false) {
> -		ret = i915_gem_object_lock(vma->obj, &ww);
> -
> -		if (ret)
> -			continue;
> -
> -		vaddr = i915_vma_pin_iomap(vma);
> -		if (IS_ERR(vaddr)) {
> -			drm_err(&dev_priv->drm,
> -				"Failed to remap framebuffer into virtual memory (%pe)\n", vaddr);
> -			ret = PTR_ERR(vaddr);
> -			continue;
> -		}
> -	}
>  
> +	ret = intel_fbdev_fb_fill_info(dev_priv, info, obj, vma);
>  	if (ret)
>  		goto out_unpin;
>  
> -	info->screen_base = vaddr;
> -	info->screen_size = vma->size;
> -
>  	drm_fb_helper_fill_info(info, &ifbdev->helper, sizes);
>  
>  	/* If the object is shmemfs backed, it will have given us zeroed pages.
>  	 * If the object is stolen however, it will be full of whatever
>  	 * garbage was left in there.
>  	 */
> -	if (!i915_gem_object_is_shmem(vma->obj) && !prealloc)
> +	if (!i915_gem_object_is_shmem(obj) && !prealloc)
>  		memset_io(info->screen_base, 0, info->screen_size);
>  
>  	/* Use default scratch pixmap (info->pixmap.flags = FB_PIXMAP_SYSTEM) */
> diff --git a/drivers/gpu/drm/i915/display/intel_fbdev_fb.c b/drivers/gpu/drm/i915/display/intel_fbdev_fb.c
> new file mode 100644
> index 000000000000..288ca4ec09d8
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/display/intel_fbdev_fb.c
> @@ -0,0 +1,116 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2023 Intel Corporation
> + */
> +
> +#include <drm/drm_fb_helper.h>
> +
> +#include "gem/i915_gem_lmem.h"
> +
> +#include "i915_drv.h"
> +#include "intel_display_types.h"
> +
> +struct drm_framebuffer *intel_fbdev_fb_alloc(struct drm_fb_helper *helper,
> +					     struct drm_fb_helper_surface_size *sizes)
> +{
> +	struct drm_framebuffer *fb;
> +	struct drm_device *dev = helper->dev;
> +	struct drm_i915_private *dev_priv = to_i915(dev);
> +	struct drm_mode_fb_cmd2 mode_cmd = {};
> +	struct drm_i915_gem_object *obj;
> +	int size;
> +
> +	/* we don't do packed 24bpp */
> +	if (sizes->surface_bpp == 24)
> +		sizes->surface_bpp = 32;
> +
> +	mode_cmd.width = sizes->surface_width;
> +	mode_cmd.height = sizes->surface_height;
> +
> +	mode_cmd.pitches[0] = ALIGN(mode_cmd.width *
> +				    DIV_ROUND_UP(sizes->surface_bpp, 8), 64);
> +	mode_cmd.pixel_format = drm_mode_legacy_fb_format(sizes->surface_bpp,
> +							  sizes->surface_depth);
> +
> +	size = mode_cmd.pitches[0] * mode_cmd.height;
> +	size = PAGE_ALIGN(size);
> +
> +	obj = ERR_PTR(-ENODEV);
> +	if (HAS_LMEM(dev_priv)) {
> +		obj = i915_gem_object_create_lmem(dev_priv, size,
> +						  I915_BO_ALLOC_CONTIGUOUS |
> +						  I915_BO_ALLOC_USER);
> +	} else {
> +		/*
> +		 * If the FB is too big, just don't use it since fbdev is not very
> +		 * important and we should probably use that space with FBC or other
> +		 * features.
> +		 *
> +		 * Also skip stolen on MTL as Wa_22018444074 mitigation.
> +		 */
> +		if (!(IS_METEORLAKE(dev_priv)) && size * 2 < dev_priv->dsm.usable_size)
> +			obj = i915_gem_object_create_stolen(dev_priv, size);
> +		if (IS_ERR(obj))
> +			obj = i915_gem_object_create_shmem(dev_priv, size);
> +	}
> +
> +	if (IS_ERR(obj)) {
> +		drm_err(&dev_priv->drm, "failed to allocate framebuffer (%pe)\n", obj);
> +		return ERR_PTR(-ENOMEM);
> +	}
> +
> +	fb = intel_framebuffer_create(obj, &mode_cmd);
> +	i915_gem_object_put(obj);
> +	if (IS_ERR(fb))
> +		return fb;

if you are returning fb anyway you don't need to check for the error, right?

> +
> +	return fb;
> +}
> +
> +int intel_fbdev_fb_fill_info(struct drm_i915_private *i915, struct fb_info *info,
> +			     struct drm_i915_gem_object *obj, struct i915_vma *vma)
> +{
> +	struct i915_gem_ww_ctx ww;
> +	void __iomem *vaddr;
> +	int ret;
> +
> +	if (i915_gem_object_is_lmem(obj)) {
> +		struct intel_memory_region *mem = obj->mm.region;
> +
> +		/* Use fbdev's framebuffer from lmem for discrete */
> +		info->fix.smem_start =
> +			(unsigned long)(mem->io_start +
> +					i915_gem_object_get_dma_address(obj, 0));
> +		info->fix.smem_len = obj->base.size;
> +	} else {
> +		struct i915_ggtt *ggtt = to_gt(i915)->ggtt;
> +
> +		/* Our framebuffer is the entirety of fbdev's system memory */
> +		info->fix.smem_start =
> +			(unsigned long)(ggtt->gmadr.start + i915_ggtt_offset(vma));
> +		info->fix.smem_len = vma->size;
> +	}
> +
> +	for_i915_gem_ww(&ww, ret, false) {
> +		ret = i915_gem_object_lock(vma->obj, &ww);
> +
> +		if (ret)
> +			continue;
> +
> +		vaddr = i915_vma_pin_iomap(vma);
> +		if (IS_ERR(vaddr)) {
> +			drm_err(&i915->drm,
> +				"Failed to remap framebuffer into virtual memory (%pe)\n", vaddr);
> +			ret = PTR_ERR(vaddr);
> +			continue;

why don't you simply return ret; in here?

with this 2 returns cleared up or clarified, let's already push this to drm-intel-next
with:
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> +		}
> +	}
> +
> +	if (ret)
> +		return ret;
> +
> +	info->screen_base = vaddr;
> +	info->screen_size = intel_bo_to_drm_bo(obj)->size;
> +
> +	return 0;
> +}
> diff --git a/drivers/gpu/drm/i915/display/intel_fbdev_fb.h b/drivers/gpu/drm/i915/display/intel_fbdev_fb.h
> new file mode 100644
> index 000000000000..d4976874239f
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/display/intel_fbdev_fb.h
> @@ -0,0 +1,21 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2023 Intel Corporation
> + */
> +
> +#ifndef __INTEL_FBDEV_FB_H__
> +#define __INTEL_FBDEV_FB_H__
> +
> +struct drm_fb_helper;
> +struct drm_fb_helper_surface_size;
> +struct drm_i915_gem_object;
> +struct drm_i915_private;
> +struct fb_info;
> +struct i915_vma;
> +
> +struct drm_framebuffer *intel_fbdev_fb_alloc(struct drm_fb_helper *helper,
> +					     struct drm_fb_helper_surface_size *sizes);
> +int intel_fbdev_fb_fill_info(struct drm_i915_private *i915, struct fb_info *info,
> +			     struct drm_i915_gem_object *obj, struct i915_vma *vma);
> +
> +#endif
> -- 
> 2.34.1
>