Re: [PATCH v6 4/4] drm/i915/display: Split i915 specific code away from intel_fb.c

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, 2023-11-28 at 15:29 +0200, Ville Syrjälä wrote:
> On Thu, Nov 23, 2023 at 09:41:20AM +0200, Jouni Högander wrote:
> > We are preparing for Xe driver. Backing object implementation is
> > differing
> > between i915 and Xe. Split i915 specific code into separate source
> > file
> > built only for i915.
> > 
> > v6: Add missing intel_fb_bo.[ch]
> > v5:
> >   - Keep drm_any_plane_has_format check in intel_fb.c
> >   - Use mode_cmd instead of user_mode_cmd for
> > intel_fb_bo_lookup_valid_bo
> > v4: Move drm_any_plane_has_format check into intel_fb_bo.c
> > v3: Fix failure handling in intel_framebuffer_init
> > v2: Couple of fixes to error value handling
> > 
> > Signed-off-by: Jouni Högander <jouni.hogander@xxxxxxxxx>
> > ---
> >  drivers/gpu/drm/i915/Makefile              |  1 +
> >  drivers/gpu/drm/i915/display/intel_fb.c    | 69 ++--------------
> >  drivers/gpu/drm/i915/display/intel_fb_bo.c | 93
> > ++++++++++++++++++++++
> >  drivers/gpu/drm/i915/display/intel_fb_bo.h | 24 ++++++
> >  4 files changed, 126 insertions(+), 61 deletions(-)
> >  create mode 100644 drivers/gpu/drm/i915/display/intel_fb_bo.c
> >  create mode 100644 drivers/gpu/drm/i915/display/intel_fb_bo.h
> > 
> > diff --git a/drivers/gpu/drm/i915/Makefile
> > b/drivers/gpu/drm/i915/Makefile
> > index 7e5d6a39d450..c14ba1212b84 100644
> > --- a/drivers/gpu/drm/i915/Makefile
> > +++ b/drivers/gpu/drm/i915/Makefile
> > @@ -279,6 +279,7 @@ i915-y += \
> >         display/intel_dsb.o \
> >         display/intel_dsb_buffer.o \
> >         display/intel_fb.o \
> > +       display/intel_fb_bo.o \
> >         display/intel_fb_pin.o \
> >         display/intel_fbc.o \
> >         display/intel_fdi.o \
> > diff --git a/drivers/gpu/drm/i915/display/intel_fb.c
> > b/drivers/gpu/drm/i915/display/intel_fb.c
> > index f63f56b24b11..d5de213be2c0 100644
> > --- a/drivers/gpu/drm/i915/display/intel_fb.c
> > +++ b/drivers/gpu/drm/i915/display/intel_fb.c
> > @@ -4,7 +4,6 @@
> >   */
> >  
> >  #include <drm/drm_blend.h>
> > -#include <drm/drm_framebuffer.h>
> >  #include <drm/drm_modeset_helper.h>
> >  
> >  #include <linux/dma-fence.h>
> > @@ -15,6 +14,7 @@
> >  #include "intel_display_types.h"
> >  #include "intel_dpt.h"
> >  #include "intel_fb.h"
> > +#include "intel_fb_bo.h"
> >  #include "intel_frontbuffer.h"
> >  
> >  #define check_array_bounds(i915, a, i) drm_WARN_ON(&(i915)->drm,
> > (i) >= ARRAY_SIZE(a))
> > @@ -1985,7 +1985,6 @@ int intel_framebuffer_init(struct
> > intel_framebuffer *intel_fb,
> >         struct drm_i915_private *dev_priv =
> > to_i915(intel_bo_to_drm_bo(obj)->dev);
> >         struct drm_framebuffer *fb = &intel_fb->base;
> >         u32 max_stride;
> > -       unsigned int tiling, stride;
> >         int ret = -EINVAL;
> >         int i;
> >  
> > @@ -1993,32 +1992,11 @@ int intel_framebuffer_init(struct
> > intel_framebuffer *intel_fb,
> >         if (!intel_fb->frontbuffer)
> >                 return -ENOMEM;
> >  
> > -       i915_gem_object_lock(obj, NULL);
> > -       tiling = i915_gem_object_get_tiling(obj);
> > -       stride = i915_gem_object_get_stride(obj);
> > -       i915_gem_object_unlock(obj);
> > -
> > -       if (mode_cmd->flags & DRM_MODE_FB_MODIFIERS) {
> > -               /*
> > -                * If there's a fence, enforce that
> > -                * the fb modifier and tiling mode match.
> > -                */
> > -               if (tiling != I915_TILING_NONE &&
> > -                   tiling != intel_fb_modifier_to_tiling(mode_cmd-
> > >modifier[0])) {
> > -                       drm_dbg_kms(&dev_priv->drm,
> > -                                   "tiling_mode doesn't match fb
> > modifier\n");
> > -                       goto err;
> > -               }
> > -       } else {
> > -               if (tiling == I915_TILING_X) {
> > -                       mode_cmd->modifier[0] =
> > I915_FORMAT_MOD_X_TILED;
> > -               } else if (tiling == I915_TILING_Y) {
> > -                       drm_dbg_kms(&dev_priv->drm,
> > -                                   "No Y tiling for legacy
> > addfb\n");
> > -                       goto err;
> > -               }
> > -       }
> > +       ret = intel_fb_bo_framebuffer_init(intel_fb, obj,
> > mode_cmd);
> > +       if (ret)
> > +               goto err;
> >  
> > +       ret = -EINVAL;
> >         if (!drm_any_plane_has_format(&dev_priv->drm,
> >                                       mode_cmd->pixel_format,
> >                                       mode_cmd->modifier[0])) {
> > @@ -2028,17 +2006,6 @@ int intel_framebuffer_init(struct
> > intel_framebuffer *intel_fb,
> >                 goto err;
> >         }
> >  
> > -       /*
> > -        * gen2/3 display engine uses the fence if present,
> > -        * so the tiling mode must match the fb modifier exactly.
> > -        */
> > -       if (DISPLAY_VER(dev_priv) < 4 &&
> > -           tiling != intel_fb_modifier_to_tiling(mode_cmd-
> > >modifier[0])) {
> > -               drm_dbg_kms(&dev_priv->drm,
> > -                           "tiling_mode must match fb modifier
> > exactly on gen2/3\n");
> > -               goto err;
> > -       }
> > -
> >         max_stride = intel_fb_max_stride(dev_priv, mode_cmd-
> > >pixel_format,
> >                                          mode_cmd->modifier[0]);
> >         if (mode_cmd->pitches[0] > max_stride) {
> > @@ -2050,17 +2017,6 @@ int intel_framebuffer_init(struct
> > intel_framebuffer *intel_fb,
> >                 goto err;
> >         }
> >  
> > -       /*
> > -        * If there's a fence, enforce that
> > -        * the fb pitch and fence stride match.
> > -        */
> > -       if (tiling != I915_TILING_NONE && mode_cmd->pitches[0] !=
> > stride) {
> > -               drm_dbg_kms(&dev_priv->drm,
> > -                           "pitch (%d) must match tiling stride
> > (%d)\n",
> > -                           mode_cmd->pitches[0], stride);
> > -               goto err;
> > -       }
> > -
> >         /* FIXME need to adjust LINOFF/TILEOFF accordingly. */
> >         if (mode_cmd->offsets[0] != 0) {
> >                 drm_dbg_kms(&dev_priv->drm,
> > @@ -2144,21 +2100,12 @@ intel_user_framebuffer_create(struct
> > drm_device *dev,
> >         struct drm_framebuffer *fb;
> >         struct drm_i915_gem_object *obj;
> >         struct drm_mode_fb_cmd2 mode_cmd = *user_mode_cmd;
> > -       struct drm_i915_private *i915;
> > +       struct drm_i915_private *i915 = to_i915(dev);
> >  
> > -       obj = i915_gem_object_lookup(filp, mode_cmd.handles[0]);
> > -       if (!obj)
> > +       obj = intel_fb_bo_lookup_valid_bo(i915, filp, &mode_cmd);
> > +       if (IS_ERR(obj))
> >                 return ERR_PTR(-ENOENT);
> >  
> > -       /* object is backed with LMEM for discrete */
> > -       i915 = to_i915(obj->base.dev);
> > -       if (HAS_LMEM(i915) && !i915_gem_object_can_migrate(obj,
> > INTEL_REGION_LMEM_0)) {
> > -               /* object is "remote", not in local memory */
> > -               i915_gem_object_put(obj);
> > -               drm_dbg_kms(&i915->drm, "framebuffer must reside in
> > local memory\n");
> > -               return ERR_PTR(-EREMOTE);
> > -       }
> > -
> >         fb = intel_framebuffer_create(obj, &mode_cmd);
> >         drm_gem_object_put(intel_bo_to_drm_bo(obj));
> >  
> > diff --git a/drivers/gpu/drm/i915/display/intel_fb_bo.c
> > b/drivers/gpu/drm/i915/display/intel_fb_bo.c
> > new file mode 100644
> > index 000000000000..9844b100fa0c
> > --- /dev/null
> > +++ b/drivers/gpu/drm/i915/display/intel_fb_bo.c
> > @@ -0,0 +1,93 @@
> > +/* SPDX-License-Identifier: MIT */
> > +/*
> > + * Copyright © 2021 Intel Corporation
> > + */
> > +
> > +#include <drm/drm_framebuffer.h>
> > +#include <drm/drm_plane.h>
> 
> Do we still need that?
> 
> > +
> > +#include "gem/i915_gem_object.h"
> > +
> > +#include "i915_drv.h"
> > +#include "intel_fb.h"
> > +#include "intel_fb_bo.h"
> > +
> > +int intel_fb_bo_framebuffer_init(struct intel_framebuffer
> > *intel_fb,
> > +                                struct drm_i915_gem_object *obj,
> > +                                struct drm_mode_fb_cmd2 *mode_cmd)
> > +{
> > +       struct drm_i915_private *i915 = to_i915(obj->base.dev);
> > +       unsigned int tiling, stride;
> > +
> > +       i915_gem_object_lock(obj, NULL);
> > +       tiling = i915_gem_object_get_tiling(obj);
> > +       stride = i915_gem_object_get_stride(obj);
> > +       i915_gem_object_unlock(obj);
> > +
> > +       if (mode_cmd->flags & DRM_MODE_FB_MODIFIERS) {
> > +               /*
> > +                * If there's a fence, enforce that
> > +                * the fb modifier and tiling mode match.
> > +                */
> > +               if (tiling != I915_TILING_NONE &&
> > +                   tiling != intel_fb_modifier_to_tiling(mode_cmd-
> > >modifier[0])) {
> > +                       drm_dbg_kms(&i915->drm,
> > +                                   "tiling_mode doesn't match fb
> > modifier\n");
> > +                       return -EINVAL;
> > +               }
> > +       } else {
> > +               if (tiling == I915_TILING_X) {
> > +                       mode_cmd->modifier[0] =
> > I915_FORMAT_MOD_X_TILED;
> > +               } else if (tiling == I915_TILING_Y) {
> > +                       drm_dbg_kms(&i915->drm,
> > +                                   "No Y tiling for legacy
> > addfb\n");
> > +                       return -EINVAL;
> > +               }
> > +       }
> > +
> > +       /*
> > +        * gen2/3 display engine uses the fence if present,
> > +        * so the tiling mode must match the fb modifier exactly.
> > +        */
> > +       if (DISPLAY_VER(i915) < 4 &&
> > +           tiling != intel_fb_modifier_to_tiling(mode_cmd-
> > >modifier[0])) {
> > +               drm_dbg_kms(&i915->drm,
> > +                           "tiling_mode must match fb modifier
> > exactly on gen2/3\n");
> > +               return -EINVAL;
> > +       }
> > +
> > +       /*
> > +        * If there's a fence, enforce that
> > +        * the fb pitch and fence stride match.
> > +        */
> > +       if (tiling != I915_TILING_NONE && mode_cmd->pitches[0] !=
> > stride) {
> > +               drm_dbg_kms(&i915->drm,
> > +                           "pitch (%d) must match tiling stride
> > (%d)\n",
> > +                           mode_cmd->pitches[0], stride);
> > +               return -EINVAL;
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +struct drm_i915_gem_object *
> > +intel_fb_bo_lookup_valid_bo(struct drm_i915_private *i915,
> > +                           struct drm_file *filp,
> > +                           const struct drm_mode_fb_cmd2
> > *user_mode_cmd)
> 
> I'd so a s/user_mode_cmd/mode_cmd/ here too. Or we could pass just
> the
> handle.
> 
> > +{
> > +       struct drm_i915_gem_object *obj =
> > i915_gem_object_lookup(filp, user_mode_cmd->handles[0]);
> 
> I'd split the declaration and assignment to separate statements since
> this can fail. More obvious what the check below does if the
> assignment
> is just before it.
> 
> > +
> > +       if (!obj)
> > +               return ERR_PTR(-ENOENT);
> > +
> > +       /* object is backed with LMEM for discrete */
> > +       i915 = to_i915(obj->base.dev);
> 
> You already passed that in explicitly. Doing both makes it look
> as if there's some magic going on here where it's possible
> that the passed i915 != obj->base.dev. I'd probably go with
> passing it in explicitly since then we don't have to worry
> about the obj==NULL, or try to dig out the device from the file.

Thank you Ville for your comments. I have addressed them in new
version. Please check.

BR,

Jouni Högander

> 
> > +       if (HAS_LMEM(i915) && !i915_gem_object_can_migrate(obj,
> > INTEL_REGION_LMEM_0)) {
> > +               /* object is "remote", not in local memory */
> > +               i915_gem_object_put(obj);
> > +               drm_dbg_kms(&i915->drm, "framebuffer must reside in
> > local memory\n");
> > +               return ERR_PTR(-EREMOTE);
> > +       }
> > +
> > +       return obj;
> > +}
> > diff --git a/drivers/gpu/drm/i915/display/intel_fb_bo.h
> > b/drivers/gpu/drm/i915/display/intel_fb_bo.h
> > new file mode 100644
> > index 000000000000..dd06ceec8601
> > --- /dev/null
> > +++ b/drivers/gpu/drm/i915/display/intel_fb_bo.h
> > @@ -0,0 +1,24 @@
> > +/* SPDX-License-Identifier: MIT */
> > +/*
> > + * Copyright © 2021 Intel Corporation
> > + */
> > +
> > +#ifndef __INTEL_FB_BO_H__
> > +#define __INTEL_FB_BO_H__
> > +
> > +struct drm_file;
> > +struct drm_mode_fb_cmd2;
> > +struct drm_i915_gem_object;
> > +struct drm_i915_private;
> > +struct intel_framebuffer;
> > +
> > +int intel_fb_bo_framebuffer_init(struct intel_framebuffer
> > *intel_fb,
> > +                                struct drm_i915_gem_object *obj,
> > +                                struct drm_mode_fb_cmd2
> > *mode_cmd);
> > +
> > +struct drm_i915_gem_object *
> > +intel_fb_bo_lookup_valid_bo(struct drm_i915_private *i915,
> > +                           struct drm_file *filp,
> > +                           const struct drm_mode_fb_cmd2
> > *user_mode_cmd);
> 
> s/user_mode_cmd/mode_cmd/ here too.
> 
> > +
> > +#endif
> > -- 
> > 2.34.1
> 





[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux