Re: [PATCH 7/9] drm/{i915,xe}/fbdev: deduplicate fbdev creation

[Jani Nikula <jani.nikula@intel.com>]

On Thu, 04 Sep 2025, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Wed, Sep 03, 2025 at 11:32:04PM +0300, Jani Nikula wrote:
>> With the bo creation helper in place, we can lift
>> intel_framebuffer_create() part to common code.
>> 
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  drivers/gpu/drm/i915/display/intel_fbdev.c    | 31 ++++++++++++++--
>>  drivers/gpu/drm/i915/display/intel_fbdev_fb.c | 29 ---------------
>>  drivers/gpu/drm/i915/display/intel_fbdev_fb.h |  2 --
>>  drivers/gpu/drm/xe/display/intel_fbdev_fb.c   | 35 -------------------
>>  4 files changed, 28 insertions(+), 69 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c b/drivers/gpu/drm/i915/display/intel_fbdev.c
>> index 5ac66fb88fcf..d7131de5c7ac 100644
>> --- a/drivers/gpu/drm/i915/display/intel_fbdev.c
>> +++ b/drivers/gpu/drm/i915/display/intel_fbdev.c
>> @@ -227,13 +227,38 @@ __intel_fbdev_fb_alloc(struct intel_display *display,
>>  		       struct drm_fb_helper_surface_size *sizes)
>>  {
>>  	struct drm_mode_fb_cmd2 mode_cmd = {};
>> -	struct intel_framebuffer *fb;
>> +	struct drm_framebuffer *fb;
>> +	struct drm_gem_object *obj;
>> +	int size;
>>  
>>  	intel_fbdev_fill_mode_cmd(sizes, &mode_cmd);
>>  
>> -	fb = intel_fbdev_fb_alloc(display->drm, &mode_cmd);
>> +	size = mode_cmd.pitches[0] * mode_cmd.height;
>> +	size = PAGE_ALIGN(size);
>> +
>> +	obj = intel_fbdev_fb_bo_create(display->drm, size);
>> +	if (IS_ERR(obj)) {
>> +		fb = ERR_CAST(obj);
>> +		goto err;
>
> This use of goto doesn't make much sense to me in this function.

Fair, probably an artefact of umpteen rebases and refactors of trying to
make this series make more sense...

BR,
Jani.

>
>> +	}
>> +
>> +	fb = intel_framebuffer_create(obj,
>> +				      drm_get_format_info(display->drm,
>> +							  mode_cmd.pixel_format,
>> +							  mode_cmd.modifier[0]),
>> +				      &mode_cmd);
>> +	if (IS_ERR(fb)) {
>> +		intel_fbdev_fb_bo_destroy(obj);
>> +		goto err;
>> +	}
>> +
>> +	drm_gem_object_put(obj);
>> +
>> +	return to_intel_framebuffer(fb);
>> +
>> +err:
>> +	return ERR_CAST(fb);
>>  
>> -	return fb;
>>  }
>>  
>>  int intel_fbdev_driver_fbdev_probe(struct drm_fb_helper *helper,
>> diff --git a/drivers/gpu/drm/i915/display/intel_fbdev_fb.c b/drivers/gpu/drm/i915/display/intel_fbdev_fb.c
>> index 6b70823ce5ef..1ac62a064936 100644
>> --- a/drivers/gpu/drm/i915/display/intel_fbdev_fb.c
>> +++ b/drivers/gpu/drm/i915/display/intel_fbdev_fb.c
>> @@ -10,7 +10,6 @@
>>  #include "i915_drv.h"
>>  #include "intel_display_core.h"
>>  #include "intel_display_types.h"
>> -#include "intel_fb.h"
>>  #include "intel_fbdev_fb.h"
>>  
>>  u32 intel_fbdev_fb_pitch_align(u32 stride)
>> @@ -56,34 +55,6 @@ void intel_fbdev_fb_bo_destroy(struct drm_gem_object *obj)
>>  	/* nop? */
>>  }
>>  
>> -struct intel_framebuffer *intel_fbdev_fb_alloc(struct drm_device *drm,
>> -					       struct drm_mode_fb_cmd2 *mode_cmd)
>> -{
>> -	struct drm_framebuffer *fb;
>> -	struct drm_gem_object *obj;
>> -	int size;
>> -
>> -	size = mode_cmd->pitches[0] * mode_cmd->height;
>> -	size = PAGE_ALIGN(size);
>> -
>> -	obj = intel_fbdev_fb_bo_create(drm, size);
>> -	if (IS_ERR(obj)) {
>> -		fb = ERR_CAST(obj);
>> -		goto err;
>> -	}
>> -
>> -	fb = intel_framebuffer_create(obj,
>> -				      drm_get_format_info(drm,
>> -							  mode_cmd->pixel_format,
>> -							  mode_cmd->modifier[0]),
>> -				      mode_cmd);
>> -	drm_gem_object_put(obj);
>> -
>> -	return to_intel_framebuffer(fb);
>> -err:
>> -	return ERR_CAST(fb);
>> -}
>> -
>>  int intel_fbdev_fb_fill_info(struct intel_display *display, struct fb_info *info,
>>  			     struct drm_gem_object *_obj, struct i915_vma *vma)
>>  {
>> diff --git a/drivers/gpu/drm/i915/display/intel_fbdev_fb.h b/drivers/gpu/drm/i915/display/intel_fbdev_fb.h
>> index 6d6f316834df..bb0ce3a85ff9 100644
>> --- a/drivers/gpu/drm/i915/display/intel_fbdev_fb.h
>> +++ b/drivers/gpu/drm/i915/display/intel_fbdev_fb.h
>> @@ -18,8 +18,6 @@ struct intel_display;
>>  u32 intel_fbdev_fb_pitch_align(u32 stride);
>>  struct drm_gem_object *intel_fbdev_fb_bo_create(struct drm_device *drm, int size);
>>  void intel_fbdev_fb_bo_destroy(struct drm_gem_object *obj);
>> -struct intel_framebuffer *intel_fbdev_fb_alloc(struct drm_device *drm,
>> -					       struct drm_mode_fb_cmd2 *mode_cmd);
>>  int intel_fbdev_fb_fill_info(struct intel_display *display, struct fb_info *info,
>>  			     struct drm_gem_object *obj, struct i915_vma *vma);
>>  
>> diff --git a/drivers/gpu/drm/xe/display/intel_fbdev_fb.c b/drivers/gpu/drm/xe/display/intel_fbdev_fb.c
>> index 9a5cf50ea7de..2e8490a8cdb1 100644
>> --- a/drivers/gpu/drm/xe/display/intel_fbdev_fb.c
>> +++ b/drivers/gpu/drm/xe/display/intel_fbdev_fb.c
>> @@ -7,7 +7,6 @@
>>  
>>  #include "intel_display_core.h"
>>  #include "intel_display_types.h"
>> -#include "intel_fb.h"
>>  #include "intel_fbdev_fb.h"
>>  #include "xe_bo.h"
>>  #include "xe_ttm_stolen_mgr.h"
>> @@ -59,40 +58,6 @@ void intel_fbdev_fb_bo_destroy(struct drm_gem_object *obj)
>>  	xe_bo_unpin_map_no_vm(gem_to_xe_bo(obj));
>>  }
>>  
>> -struct intel_framebuffer *intel_fbdev_fb_alloc(struct drm_device *drm,
>> -					       struct drm_mode_fb_cmd2 *mode_cmd)
>> -{
>> -	struct drm_framebuffer *fb;
>> -	struct drm_gem_object *obj;
>> -	int size;
>> -
>> -	size = mode_cmd->pitches[0] * mode_cmd->height;
>> -	size = PAGE_ALIGN(size);
>> -
>> -	obj = intel_fbdev_fb_bo_create(drm, size);
>> -	if (IS_ERR(obj)) {
>> -		fb = ERR_CAST(obj);
>> -		goto err;
>> -	}
>> -
>> -	fb = intel_framebuffer_create(obj,
>> -				      drm_get_format_info(drm,
>> -							  mode_cmd->pixel_format,
>> -							  mode_cmd->modifier[0]),
>> -				      mode_cmd);
>> -	if (IS_ERR(fb)) {
>> -		intel_fbdev_fb_bo_destroy(obj);
>> -		goto err;
>> -	}
>> -
>> -	drm_gem_object_put(obj);
>> -
>> -	return to_intel_framebuffer(fb);
>> -
>> -err:
>> -	return ERR_CAST(fb);
>> -}
>> -
>>  int intel_fbdev_fb_fill_info(struct intel_display *display, struct fb_info *info,
>>  			     struct drm_gem_object *_obj, struct i915_vma *vma)
>>  {
>> -- 
>> 2.47.2

-- 
Jani Nikula, Intel