| Message ID | 20181030003413.8144-1-rodrigo.vivi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC,1/4] drm/i915: Add Display Gen info. | expand |
On Mon, 29 Oct 2018, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote: > Introduce Display Gen. The goal is to use this to minimize > the amount of platform codename checks we have nowdays on > display code. > > The introduction of a new platform should be just > gen >= current. So the patches 1-3 look nice for GLK. The thing that bugs me here is that this doesn't help VLV/CHV GMCH display at all. We'll still continue to have the more feature oriented HAS_GMCH_DISPLAY, HAS_DDI, and HAS_PCH_SPLIT. Haswell display is still better represented by HAS_DDI than gen because it's 7.5. Patch 4 means continued pedantic review about not mixing up IS_GEN and IS_DISPLAY_GEN. If we aren't strict about the separation, then what's the point? It's not immediately obvious that it's worth the hassle. Only time will tell. I'll want to hear more opinions before merging. One note inline below. BR, Jani. > > Just a gen++ without exposing any new feature or ip. > so this would minimize the amount of patches needed > for a bring-up specially holding them on internal branches. > > Cc: Jani Nikula <jani.nikula@intel.com> > Cc: Lucas De Marchi <lucas.demarchi@intel.com> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 28 ++++++++++++++++++++++-- > drivers/gpu/drm/i915/i915_pci.c | 5 ++++- > drivers/gpu/drm/i915/intel_device_info.h | 2 ++ > 3 files changed, 32 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index c9e5bab6861b..3242229688e3 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -2349,8 +2349,9 @@ intel_info(const struct drm_i915_private *dev_priv) > #define INTEL_INFO(dev_priv) intel_info((dev_priv)) > #define DRIVER_CAPS(dev_priv) (&(dev_priv)->caps) > > -#define INTEL_GEN(dev_priv) ((dev_priv)->info.gen) > -#define INTEL_DEVID(dev_priv) ((dev_priv)->info.device_id) > +#define INTEL_GEN(dev_priv) ((dev_priv)->info.gen) > +#define INTEL_DISPLAY_GEN(dev_priv) ((dev_priv)->info.display_gen) > +#define INTEL_DEVID(dev_priv) ((dev_priv)->info.device_id) > > #define REVID_FOREVER 0xff > #define INTEL_REVID(dev_priv) ((dev_priv)->drm.pdev->revision) > @@ -2363,6 +2364,8 @@ intel_info(const struct drm_i915_private *dev_priv) > /* Returns true if Gen is in inclusive range [Start, End] */ > #define IS_GEN(dev_priv, s, e) \ > (!!((dev_priv)->info.gen_mask & INTEL_GEN_MASK((s), (e)))) > +#define IS_DISPLAY_GEN(dev_priv, s, e) \ > + (!!((dev_priv)->info.display_gen_mask & INTEL_GEN_MASK((s), (e)))) > > /* > * Return true if revision is in range [since,until] inclusive. > @@ -2532,6 +2535,27 @@ intel_info(const struct drm_i915_private *dev_priv) > #define IS_GEN10(dev_priv) (!!((dev_priv)->info.gen_mask & BIT(9))) > #define IS_GEN11(dev_priv) (!!((dev_priv)->info.gen_mask & BIT(10))) > > +#define IS_DISPLAY_GEN2(dev_priv) (!!((dev_priv)->info.display_gen_mask \ > + & BIT(2))) > +#define IS_DISPLAY_GEN3(dev_priv) (!!((dev_priv)->info.display_gen_mask \ > + & BIT(3))) > +#define IS_DISPLAY_GEN4(dev_priv) (!!((dev_priv)->info.display_gen_mask \ > + & BIT(4))) > +#define IS_DISPLAY_GEN5(dev_priv) (!!((dev_priv)->info.display_gen_mask \ > + & BIT(5))) > +#define IS_DISPLAY_GEN6(dev_priv) (!!((dev_priv)->info.display_gen_mask \ > + & BIT(6))) > +#define IS_DISPLAY_GEN7(dev_priv) (!!((dev_priv)->info.display_gen_mask \ > + & BIT(7))) > +#define IS_DISPLAY_GEN8(dev_priv) (!!((dev_priv)->info.display_gen_mask \ > + & BIT(8))) > +#define IS_DISPLAY_GEN9(dev_priv) (!!((dev_priv)->info.display_gen_mask \ > + & BIT(9))) > +#define IS_DISPLAY_GEN10(dev_priv) (!!((dev_priv)->info.display_gen_mask \ > + & BIT(10))) > +#define IS_DISPLAY_GEN11(dev_priv) (!!((dev_priv)->info.display_gen_mask \ > + & BIT(11))) I know this is the same pattern as in IS_GEN<N> above, but shouldn't the compiler end up with the same result if these were simply: #define IS_DISPLAY_GEN2(dev_priv) IS_DISPLAY_GEN(dev_priv, 2, 2) > + > #define IS_LP(dev_priv) (INTEL_INFO(dev_priv)->is_lp) > #define IS_GEN9_LP(dev_priv) (IS_GEN9(dev_priv) && IS_LP(dev_priv)) > #define IS_GEN9_BC(dev_priv) (IS_GEN9(dev_priv) && !IS_LP(dev_priv)) > diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c > index 44e745921ac1..fb8caf846c02 100644 > --- a/drivers/gpu/drm/i915/i915_pci.c > +++ b/drivers/gpu/drm/i915/i915_pci.c > @@ -30,7 +30,10 @@ > #include "i915_selftest.h" > > #define PLATFORM(x) .platform = (x), .platform_mask = BIT(x) > -#define GEN(x) .gen = (x), .gen_mask = BIT((x) - 1) > +#define GEN(x) .gen = (x), .gen_mask = BIT((x) - 1), \ > + .display_gen = (x), .display_gen_mask = BIT((x)) > +/* Unless explicitly stated otherwise Display gen inherits platform gen */ > +#define DISPLAY_GEN(x) .display_gen = (x), .display_gen_mask = BIT((x)) > > #define GEN_DEFAULT_PIPEOFFSETS \ > .pipe_offsets = { PIPE_A_OFFSET, PIPE_B_OFFSET, \ > diff --git a/drivers/gpu/drm/i915/intel_device_info.h b/drivers/gpu/drm/i915/intel_device_info.h > index b4c2c4eae78b..9f31f29186a8 100644 > --- a/drivers/gpu/drm/i915/intel_device_info.h > +++ b/drivers/gpu/drm/i915/intel_device_info.h > @@ -151,8 +151,10 @@ typedef u8 intel_ring_mask_t; > struct intel_device_info { > u16 device_id; > u16 gen_mask; > + u16 display_gen_mask; > > u8 gen; > + u8 display_gen; > u8 gt; /* GT number, 0 if undefined */ > u8 num_rings; > intel_ring_mask_t ring_mask; /* Rings supported by the HW */
+cc Ville On Tue, Oct 30, 2018 at 11:52:30AM +0200, Jani Nikula wrote: > On Mon, 29 Oct 2018, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote: > > Introduce Display Gen. The goal is to use this to minimize > > the amount of platform codename checks we have nowdays on > > display code. > > > > The introduction of a new platform should be just > > gen >= current. > > So the patches 1-3 look nice for GLK. The thing that bugs me here is > that this doesn't help VLV/CHV Couldn't we define cherryview platform with DISPLAY_GEN(7) /* 7.5 */ and also a #define IS_DISPLAY_GEN7_LP(dev_priv) IS_DISPLAY_GEN7(dev_priv) && IS_LP(dev_priv) this would cover most of IS_CHERRYVIEW || IS_VALLEYVIEW code. > GMCH display at all. For GMCH I believe with this gen display range in place we could use IS_DISPLAY(dev_priv, 2, 4) for most cases. For the intersection with VLV/CHV I would define a info.has_cxsr_wm This would be a real feature based ting instead of "gmch_display" > We'll still continue > to have the more feature oriented HAS_GMCH_DISPLAY, maybe very little cases would be: IS_DISPLAY(dev_priv, 2, 4) || IS_DISPLAY_GEN7_LP(dev_priv) so we could kill GMCH entirely. > HAS_DDI, and > HAS_PCH_SPLIT. HAS_PCH_SPLIT makes sense in my opinion because of the south display stuff that is on PCH. That one I would keep. > Haswell display is still better represented by HAS_DDI > than gen because it's 7.5. Maybe this is also a place to define Haswell with: DISPLAY_GEN(7) /* 7.5 */ And kill all HAS_DDI in favor of Display ranges as well. > > Patch 4 means continued pedantic review about not mixing up IS_GEN and > IS_DISPLAY_GEN. If we aren't strict about the separation, then what's > the point? It's not immediately obvious that it's worth the hassle. Only > time will tell. I agree it is not clear yet it's worth the hassle. I believe that the conversion can be slowly with only places that we are changing to have IS_DISPLAY_GEN(dev_priv) >= 7 we change the entire block to use DISPLAY_GEN instead of IS_GEN or platform codenames. > > I'll want to hear more opinions before merging. Yeap, that's the idea here ;) My ultimate goal is to stop the current mixed cases where we have if INTEL_GEN > 11 else PLATFORM_CODENAME and also expand the places where we have IS_ICELAKE to be INTEL_GEN > 11 So adding the next gen gets easier and without bureaucracy just gen++... Thanks a lot for the comments, Rodrigo. > > One note inline below. > > > BR, > Jani. > > > > > > Just a gen++ without exposing any new feature or ip. > > so this would minimize the amount of patches needed > > for a bring-up specially holding them on internal branches. > > > > Cc: Jani Nikula <jani.nikula@intel.com> > > Cc: Lucas De Marchi <lucas.demarchi@intel.com> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > --- > > drivers/gpu/drm/i915/i915_drv.h | 28 ++++++++++++++++++++++-- > > drivers/gpu/drm/i915/i915_pci.c | 5 ++++- > > drivers/gpu/drm/i915/intel_device_info.h | 2 ++ > > 3 files changed, 32 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > > index c9e5bab6861b..3242229688e3 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -2349,8 +2349,9 @@ intel_info(const struct drm_i915_private *dev_priv) > > #define INTEL_INFO(dev_priv) intel_info((dev_priv)) > > #define DRIVER_CAPS(dev_priv) (&(dev_priv)->caps) > > > > -#define INTEL_GEN(dev_priv) ((dev_priv)->info.gen) > > -#define INTEL_DEVID(dev_priv) ((dev_priv)->info.device_id) > > +#define INTEL_GEN(dev_priv) ((dev_priv)->info.gen) > > +#define INTEL_DISPLAY_GEN(dev_priv) ((dev_priv)->info.display_gen) > > +#define INTEL_DEVID(dev_priv) ((dev_priv)->info.device_id) > > > > #define REVID_FOREVER 0xff > > #define INTEL_REVID(dev_priv) ((dev_priv)->drm.pdev->revision) > > @@ -2363,6 +2364,8 @@ intel_info(const struct drm_i915_private *dev_priv) > > /* Returns true if Gen is in inclusive range [Start, End] */ > > #define IS_GEN(dev_priv, s, e) \ > > (!!((dev_priv)->info.gen_mask & INTEL_GEN_MASK((s), (e)))) > > +#define IS_DISPLAY_GEN(dev_priv, s, e) \ > > + (!!((dev_priv)->info.display_gen_mask & INTEL_GEN_MASK((s), (e)))) > > > > /* > > * Return true if revision is in range [since,until] inclusive. > > @@ -2532,6 +2535,27 @@ intel_info(const struct drm_i915_private *dev_priv) > > #define IS_GEN10(dev_priv) (!!((dev_priv)->info.gen_mask & BIT(9))) > > #define IS_GEN11(dev_priv) (!!((dev_priv)->info.gen_mask & BIT(10))) > > > > +#define IS_DISPLAY_GEN2(dev_priv) (!!((dev_priv)->info.display_gen_mask \ > > + & BIT(2))) > > +#define IS_DISPLAY_GEN3(dev_priv) (!!((dev_priv)->info.display_gen_mask \ > > + & BIT(3))) > > +#define IS_DISPLAY_GEN4(dev_priv) (!!((dev_priv)->info.display_gen_mask \ > > + & BIT(4))) > > +#define IS_DISPLAY_GEN5(dev_priv) (!!((dev_priv)->info.display_gen_mask \ > > + & BIT(5))) > > +#define IS_DISPLAY_GEN6(dev_priv) (!!((dev_priv)->info.display_gen_mask \ > > + & BIT(6))) > > +#define IS_DISPLAY_GEN7(dev_priv) (!!((dev_priv)->info.display_gen_mask \ > > + & BIT(7))) > > +#define IS_DISPLAY_GEN8(dev_priv) (!!((dev_priv)->info.display_gen_mask \ > > + & BIT(8))) > > +#define IS_DISPLAY_GEN9(dev_priv) (!!((dev_priv)->info.display_gen_mask \ > > + & BIT(9))) > > +#define IS_DISPLAY_GEN10(dev_priv) (!!((dev_priv)->info.display_gen_mask \ > > + & BIT(10))) > > +#define IS_DISPLAY_GEN11(dev_priv) (!!((dev_priv)->info.display_gen_mask \ > > + & BIT(11))) > > I know this is the same pattern as in IS_GEN<N> above, but shouldn't the > compiler end up with the same result if these were simply: > > #define IS_DISPLAY_GEN2(dev_priv) IS_DISPLAY_GEN(dev_priv, 2, 2) > > > > + > > #define IS_LP(dev_priv) (INTEL_INFO(dev_priv)->is_lp) > > #define IS_GEN9_LP(dev_priv) (IS_GEN9(dev_priv) && IS_LP(dev_priv)) > > #define IS_GEN9_BC(dev_priv) (IS_GEN9(dev_priv) && !IS_LP(dev_priv)) > > diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c > > index 44e745921ac1..fb8caf846c02 100644 > > --- a/drivers/gpu/drm/i915/i915_pci.c > > +++ b/drivers/gpu/drm/i915/i915_pci.c > > @@ -30,7 +30,10 @@ > > #include "i915_selftest.h" > > > > #define PLATFORM(x) .platform = (x), .platform_mask = BIT(x) > > -#define GEN(x) .gen = (x), .gen_mask = BIT((x) - 1) > > +#define GEN(x) .gen = (x), .gen_mask = BIT((x) - 1), \ > > + .display_gen = (x), .display_gen_mask = BIT((x)) > > +/* Unless explicitly stated otherwise Display gen inherits platform gen */ > > +#define DISPLAY_GEN(x) .display_gen = (x), .display_gen_mask = BIT((x)) > > > > #define GEN_DEFAULT_PIPEOFFSETS \ > > .pipe_offsets = { PIPE_A_OFFSET, PIPE_B_OFFSET, \ > > diff --git a/drivers/gpu/drm/i915/intel_device_info.h b/drivers/gpu/drm/i915/intel_device_info.h > > index b4c2c4eae78b..9f31f29186a8 100644 > > --- a/drivers/gpu/drm/i915/intel_device_info.h > > +++ b/drivers/gpu/drm/i915/intel_device_info.h > > @@ -151,8 +151,10 @@ typedef u8 intel_ring_mask_t; > > struct intel_device_info { > > u16 device_id; > > u16 gen_mask; > > + u16 display_gen_mask; > > > > u8 gen; > > + u8 display_gen; > > u8 gt; /* GT number, 0 if undefined */ > > u8 num_rings; > > intel_ring_mask_t ring_mask; /* Rings supported by the HW */ > > -- > Jani Nikula, Intel Open Source Graphics Center
On Tue, Oct 30, 2018 at 11:52:30AM +0200, Jani Nikula wrote: > On Mon, 29 Oct 2018, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote: > > Introduce Display Gen. The goal is to use this to minimize > > the amount of platform codename checks we have nowdays on > > display code. > > > > The introduction of a new platform should be just > > gen >= current. > > So the patches 1-3 look nice for GLK. The thing that bugs me here is > that this doesn't help VLV/CHV GMCH display at all. We'll still continue > to have the more feature oriented HAS_GMCH_DISPLAY, HAS_DDI, and > HAS_PCH_SPLIT. Haswell display is still better represented by HAS_DDI > than gen because it's 7.5. > > Patch 4 means continued pedantic review about not mixing up IS_GEN and > IS_DISPLAY_GEN. If we aren't strict about the separation, then what's > the point? It's not immediately obvious that it's worth the hassle. Only > time will tell. I think the real point of having a display_gen in the device_info struct is to group together features that would otherwise be too fine-grained. That would lead to growing the device_info a lot for little benefit. But, as you say even inside a single display gen we have small differences or some features that are not specific do a display gen. Just like we have from gem gen. What we maybe need is a way to represent the .5 thing, but I'm not sure it's worth... IMO we should approximate it to the closest one and differentiate the specific places with the more fine-grained feature checks. > > I'll want to hear more opinions before merging. > > One note inline below. > > > BR, > Jani. > > > > > > Just a gen++ without exposing any new feature or ip. > > so this would minimize the amount of patches needed > > for a bring-up specially holding them on internal branches. > > > > Cc: Jani Nikula <jani.nikula@intel.com> > > Cc: Lucas De Marchi <lucas.demarchi@intel.com> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > --- > > drivers/gpu/drm/i915/i915_drv.h | 28 ++++++++++++++++++++++-- > > drivers/gpu/drm/i915/i915_pci.c | 5 ++++- > > drivers/gpu/drm/i915/intel_device_info.h | 2 ++ > > 3 files changed, 32 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > > index c9e5bab6861b..3242229688e3 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -2349,8 +2349,9 @@ intel_info(const struct drm_i915_private *dev_priv) > > #define INTEL_INFO(dev_priv) intel_info((dev_priv)) > > #define DRIVER_CAPS(dev_priv) (&(dev_priv)->caps) > > > > -#define INTEL_GEN(dev_priv) ((dev_priv)->info.gen) > > -#define INTEL_DEVID(dev_priv) ((dev_priv)->info.device_id) > > +#define INTEL_GEN(dev_priv) ((dev_priv)->info.gen) > > +#define INTEL_DISPLAY_GEN(dev_priv) ((dev_priv)->info.display_gen) > > +#define INTEL_DEVID(dev_priv) ((dev_priv)->info.device_id) > > > > #define REVID_FOREVER 0xff > > #define INTEL_REVID(dev_priv) ((dev_priv)->drm.pdev->revision) > > @@ -2363,6 +2364,8 @@ intel_info(const struct drm_i915_private *dev_priv) > > /* Returns true if Gen is in inclusive range [Start, End] */ > > #define IS_GEN(dev_priv, s, e) \ > > (!!((dev_priv)->info.gen_mask & INTEL_GEN_MASK((s), (e)))) > > +#define IS_DISPLAY_GEN(dev_priv, s, e) \ > > + (!!((dev_priv)->info.display_gen_mask & INTEL_GEN_MASK((s), (e)))) > > > > /* > > * Return true if revision is in range [since,until] inclusive. > > @@ -2532,6 +2535,27 @@ intel_info(const struct drm_i915_private *dev_priv) > > #define IS_GEN10(dev_priv) (!!((dev_priv)->info.gen_mask & BIT(9))) > > #define IS_GEN11(dev_priv) (!!((dev_priv)->info.gen_mask & BIT(10))) > > > > +#define IS_DISPLAY_GEN2(dev_priv) (!!((dev_priv)->info.display_gen_mask \ > > + & BIT(2))) > > +#define IS_DISPLAY_GEN3(dev_priv) (!!((dev_priv)->info.display_gen_mask \ > > + & BIT(3))) > > +#define IS_DISPLAY_GEN4(dev_priv) (!!((dev_priv)->info.display_gen_mask \ > > + & BIT(4))) > > +#define IS_DISPLAY_GEN5(dev_priv) (!!((dev_priv)->info.display_gen_mask \ > > + & BIT(5))) > > +#define IS_DISPLAY_GEN6(dev_priv) (!!((dev_priv)->info.display_gen_mask \ > > + & BIT(6))) > > +#define IS_DISPLAY_GEN7(dev_priv) (!!((dev_priv)->info.display_gen_mask \ > > + & BIT(7))) > > +#define IS_DISPLAY_GEN8(dev_priv) (!!((dev_priv)->info.display_gen_mask \ > > + & BIT(8))) > > +#define IS_DISPLAY_GEN9(dev_priv) (!!((dev_priv)->info.display_gen_mask \ > > + & BIT(9))) > > +#define IS_DISPLAY_GEN10(dev_priv) (!!((dev_priv)->info.display_gen_mask \ > > + & BIT(10))) > > +#define IS_DISPLAY_GEN11(dev_priv) (!!((dev_priv)->info.display_gen_mask \ > > + & BIT(11))) > > I know this is the same pattern as in IS_GEN<N> above, but shouldn't the > compiler end up with the same result if these were simply: > > #define IS_DISPLAY_GEN2(dev_priv) IS_DISPLAY_GEN(dev_priv, 2, 2) humn... maybe this is too magic, but it works for me and I didn't add any additional macro to the kernel to implement it :) [CI, DON'T TEST THIS] diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h [CI, DON'T TEST THIS] index 554627dc623c..02a8b51fd733 100644 [CI, DON'T TEST THIS] --- a/drivers/gpu/drm/i915/i915_drv.h [CI, DON'T TEST THIS] +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -2385,9 +2385,15 @@ intel_info(const struct drm_i915_private *dev_priv) GENMASK((e) - 1, (s) - 1)) /* Returns true if Gen is in inclusive range [Start, End] */ -#define IS_GEN(dev_priv, s, e) \ +#define _IS_GEN_ARG2(dev_priv, s, e) \ (!!((dev_priv)->info.gen_mask & INTEL_GEN_MASK((s), (e)))) +#define _IS_GEN_ARG1(dev_priv, g) \ + (!!((dev_priv)->info.gen_mask & BIT((g) - 1))) + +#define IS_GEN(dev_priv, ...) \ + CONCATENATE(_IS_GEN_ARG, COUNT_ARGS(__VA_ARGS__))((dev_priv), ##__VA_ARGS__) + /* * Return true if revision is in range [since,until] inclusive. * So we could use IS_GEN(dev_priv, 2) as well as IS_GEN(dev_priv, 2, 4), which IMO is very clear. The same would apply for IS_DISPLAY_GEN() version. And if they generate the same code, we could just change the expansion to repeat the argument. Lucas De Marchi > > > > + > > #define IS_LP(dev_priv) (INTEL_INFO(dev_priv)->is_lp) > > #define IS_GEN9_LP(dev_priv) (IS_GEN9(dev_priv) && IS_LP(dev_priv)) > > #define IS_GEN9_BC(dev_priv) (IS_GEN9(dev_priv) && !IS_LP(dev_priv)) > > diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c > > index 44e745921ac1..fb8caf846c02 100644 > > --- a/drivers/gpu/drm/i915/i915_pci.c > > +++ b/drivers/gpu/drm/i915/i915_pci.c > > @@ -30,7 +30,10 @@ > > #include "i915_selftest.h" > > > > #define PLATFORM(x) .platform = (x), .platform_mask = BIT(x) > > -#define GEN(x) .gen = (x), .gen_mask = BIT((x) - 1) > > +#define GEN(x) .gen = (x), .gen_mask = BIT((x) - 1), \ > > + .display_gen = (x), .display_gen_mask = BIT((x)) > > +/* Unless explicitly stated otherwise Display gen inherits platform gen */ > > +#define DISPLAY_GEN(x) .display_gen = (x), .display_gen_mask = BIT((x)) > > > > #define GEN_DEFAULT_PIPEOFFSETS \ > > .pipe_offsets = { PIPE_A_OFFSET, PIPE_B_OFFSET, \ > > diff --git a/drivers/gpu/drm/i915/intel_device_info.h b/drivers/gpu/drm/i915/intel_device_info.h > > index b4c2c4eae78b..9f31f29186a8 100644 > > --- a/drivers/gpu/drm/i915/intel_device_info.h > > +++ b/drivers/gpu/drm/i915/intel_device_info.h > > @@ -151,8 +151,10 @@ typedef u8 intel_ring_mask_t; > > struct intel_device_info { > > u16 device_id; > > u16 gen_mask; > > + u16 display_gen_mask; > > > > u8 gen; > > + u8 display_gen; > > u8 gt; /* GT number, 0 if undefined */ > > u8 num_rings; > > intel_ring_mask_t ring_mask; /* Rings supported by the HW */ > > -- > Jani Nikula, Intel Open Source Graphics Center > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, 30 Oct 2018, Lucas De Marchi <lucas.de.marchi@gmail.com> wrote: > On Tue, Oct 30, 2018 at 11:52:30AM +0200, Jani Nikula wrote: >> On Mon, 29 Oct 2018, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote: >> > +#define IS_DISPLAY_GEN2(dev_priv) (!!((dev_priv)->info.display_gen_mask \ >> > + & BIT(2))) >> > +#define IS_DISPLAY_GEN3(dev_priv) (!!((dev_priv)->info.display_gen_mask \ >> > + & BIT(3))) >> > +#define IS_DISPLAY_GEN4(dev_priv) (!!((dev_priv)->info.display_gen_mask \ >> > + & BIT(4))) >> > +#define IS_DISPLAY_GEN5(dev_priv) (!!((dev_priv)->info.display_gen_mask \ >> > + & BIT(5))) >> > +#define IS_DISPLAY_GEN6(dev_priv) (!!((dev_priv)->info.display_gen_mask \ >> > + & BIT(6))) >> > +#define IS_DISPLAY_GEN7(dev_priv) (!!((dev_priv)->info.display_gen_mask \ >> > + & BIT(7))) >> > +#define IS_DISPLAY_GEN8(dev_priv) (!!((dev_priv)->info.display_gen_mask \ >> > + & BIT(8))) >> > +#define IS_DISPLAY_GEN9(dev_priv) (!!((dev_priv)->info.display_gen_mask \ >> > + & BIT(9))) >> > +#define IS_DISPLAY_GEN10(dev_priv) (!!((dev_priv)->info.display_gen_mask \ >> > + & BIT(10))) >> > +#define IS_DISPLAY_GEN11(dev_priv) (!!((dev_priv)->info.display_gen_mask \ >> > + & BIT(11))) >> >> I know this is the same pattern as in IS_GEN<N> above, but shouldn't the >> compiler end up with the same result if these were simply: >> >> #define IS_DISPLAY_GEN2(dev_priv) IS_DISPLAY_GEN(dev_priv, 2, 2) > > > humn... maybe this is too magic, but it works for me and I didn't add any additional > macro to the kernel to implement it :) > > [CI, DON'T TEST THIS] diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > [CI, DON'T TEST THIS] index 554627dc623c..02a8b51fd733 100644 > [CI, DON'T TEST THIS] --- a/drivers/gpu/drm/i915/i915_drv.h > [CI, DON'T TEST THIS] +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -2385,9 +2385,15 @@ intel_info(const struct drm_i915_private *dev_priv) > GENMASK((e) - 1, (s) - 1)) > > /* Returns true if Gen is in inclusive range [Start, End] */ > -#define IS_GEN(dev_priv, s, e) \ > +#define _IS_GEN_ARG2(dev_priv, s, e) \ > (!!((dev_priv)->info.gen_mask & INTEL_GEN_MASK((s), (e)))) > > +#define _IS_GEN_ARG1(dev_priv, g) \ > + (!!((dev_priv)->info.gen_mask & BIT((g) - 1))) > + > +#define IS_GEN(dev_priv, ...) \ > + CONCATENATE(_IS_GEN_ARG, COUNT_ARGS(__VA_ARGS__))((dev_priv), ##__VA_ARGS__) > + > /* > * Return true if revision is in range [since,until] inclusive. > * > > > So we could use IS_GEN(dev_priv, 2) as well as IS_GEN(dev_priv, 2, 4), which IMO is very clear. > The same would apply for IS_DISPLAY_GEN() version. And if they generate the same code, we could > just change the expansion to repeat the argument. I like this stuff. So I'd prefer IS_GEN(dev_priv, 2) in favor of IS_GEN2(dev_priv) throughout. As long as it doesn't increase the code size too much, but this being macro magic I don't think it should. Care to cook up a patch against current tip to make IS_GEN() take 1 or 2 args? If I read the above right, you'll get a build error for using IS_GEN(dev_priv, 1, 2, 3), is that correct? BR, Jani.
On 31/10/2018 08:13, Jani Nikula wrote: > On Tue, 30 Oct 2018, Lucas De Marchi <lucas.de.marchi@gmail.com> wrote: >> On Tue, Oct 30, 2018 at 11:52:30AM +0200, Jani Nikula wrote: >>> On Mon, 29 Oct 2018, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote: >>>> +#define IS_DISPLAY_GEN2(dev_priv) (!!((dev_priv)->info.display_gen_mask \ >>>> + & BIT(2))) >>>> +#define IS_DISPLAY_GEN3(dev_priv) (!!((dev_priv)->info.display_gen_mask \ >>>> + & BIT(3))) >>>> +#define IS_DISPLAY_GEN4(dev_priv) (!!((dev_priv)->info.display_gen_mask \ >>>> + & BIT(4))) >>>> +#define IS_DISPLAY_GEN5(dev_priv) (!!((dev_priv)->info.display_gen_mask \ >>>> + & BIT(5))) >>>> +#define IS_DISPLAY_GEN6(dev_priv) (!!((dev_priv)->info.display_gen_mask \ >>>> + & BIT(6))) >>>> +#define IS_DISPLAY_GEN7(dev_priv) (!!((dev_priv)->info.display_gen_mask \ >>>> + & BIT(7))) >>>> +#define IS_DISPLAY_GEN8(dev_priv) (!!((dev_priv)->info.display_gen_mask \ >>>> + & BIT(8))) >>>> +#define IS_DISPLAY_GEN9(dev_priv) (!!((dev_priv)->info.display_gen_mask \ >>>> + & BIT(9))) >>>> +#define IS_DISPLAY_GEN10(dev_priv) (!!((dev_priv)->info.display_gen_mask \ >>>> + & BIT(10))) >>>> +#define IS_DISPLAY_GEN11(dev_priv) (!!((dev_priv)->info.display_gen_mask \ >>>> + & BIT(11))) >>> >>> I know this is the same pattern as in IS_GEN<N> above, but shouldn't the >>> compiler end up with the same result if these were simply: >>> >>> #define IS_DISPLAY_GEN2(dev_priv) IS_DISPLAY_GEN(dev_priv, 2, 2) >> >> >> humn... maybe this is too magic, but it works for me and I didn't add any additional >> macro to the kernel to implement it :) >> >> [CI, DON'T TEST THIS] diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h >> [CI, DON'T TEST THIS] index 554627dc623c..02a8b51fd733 100644 >> [CI, DON'T TEST THIS] --- a/drivers/gpu/drm/i915/i915_drv.h >> [CI, DON'T TEST THIS] +++ b/drivers/gpu/drm/i915/i915_drv.h >> @@ -2385,9 +2385,15 @@ intel_info(const struct drm_i915_private *dev_priv) >> GENMASK((e) - 1, (s) - 1)) >> >> /* Returns true if Gen is in inclusive range [Start, End] */ >> -#define IS_GEN(dev_priv, s, e) \ >> +#define _IS_GEN_ARG2(dev_priv, s, e) \ >> (!!((dev_priv)->info.gen_mask & INTEL_GEN_MASK((s), (e)))) >> >> +#define _IS_GEN_ARG1(dev_priv, g) \ >> + (!!((dev_priv)->info.gen_mask & BIT((g) - 1))) >> + >> +#define IS_GEN(dev_priv, ...) \ >> + CONCATENATE(_IS_GEN_ARG, COUNT_ARGS(__VA_ARGS__))((dev_priv), ##__VA_ARGS__) >> + >> /* >> * Return true if revision is in range [since,until] inclusive. >> * >> >> >> So we could use IS_GEN(dev_priv, 2) as well as IS_GEN(dev_priv, 2, 4), which IMO is very clear. >> The same would apply for IS_DISPLAY_GEN() version. And if they generate the same code, we could >> just change the expansion to repeat the argument. > > I like this stuff. > > So I'd prefer IS_GEN(dev_priv, 2) in favor of IS_GEN2(dev_priv) > throughout. > > As long as it doesn't increase the code size too much, but this being > macro magic I don't think it should. > > Care to cook up a patch against current tip to make IS_GEN() take 1 or 2 > args? If I read the above right, you'll get a build error for using > IS_GEN(dev_priv, 1, 2, 3), is that correct? I saw some mention somewhere on IS_GEN_RANGE, which looked clearer than IS_GEN(dev_priv, s, e). Presumably that did not go anywhere since now the proposal is the above? I have to say I am not sure it reads completely intuitive when seen near in code: IS_GEN(dev_priv, 9) IS_GEN(dev_priv, 8, 9) Looks like a variable arg list and the difference in semantics does not come through. As such I am leaning towards thinking it is too much churn for unclear benefit. Or in other words I thought IS_GEN_RANGE was a better direction. Regards, Tvrtko
On Wed, 31 Oct 2018, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote: > I saw some mention somewhere on IS_GEN_RANGE, which looked clearer than > IS_GEN(dev_priv, s, e). Presumably that did not go anywhere since now > the proposal is the above? I have to say I am not sure it reads > completely intuitive when seen near in code: > > IS_GEN(dev_priv, 9) > IS_GEN(dev_priv, 8, 9) > > Looks like a variable arg list and the difference in semantics does not > come through. As such I am leaning towards thinking it is too much churn > for unclear benefit. Or in other words I thought IS_GEN_RANGE was a > better direction. Okay, thanks for the feedback. I'm not locked into any resolution yet, apart from not churning anything until we have a better picture where we're going. BR, Jani.
On Wed, Oct 31, 2018 at 11:00:54AM +0200, Jani Nikula wrote: > On Wed, 31 Oct 2018, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote: > > I saw some mention somewhere on IS_GEN_RANGE, which looked clearer than > > IS_GEN(dev_priv, s, e). Presumably that did not go anywhere since now > > the proposal is the above? I have to say I am not sure it reads > > completely intuitive when seen near in code: > > > > IS_GEN(dev_priv, 9) > > IS_GEN(dev_priv, 8, 9) > > > > Looks like a variable arg list and the difference in semantics does not > > come through. As such I am leaning towards thinking it is too much churn > > for unclear benefit. Or in other words I thought IS_GEN_RANGE was a > > better direction. > > Okay, thanks for the feedback. I'm not locked into any resolution yet, > apart from not churning anything until we have a better picture where > we're going. I believe we have 2 orthogonal discussions here where they shouldn't block each other. 1. The addition of DISPLAY_GEN checks to group platforms and prefer display gen checks over platform codenames. By doing this all platform enabling work for next platforms gets easier and less bureaucratic. 2. consolidated IS_GEN macro vs GEN_RANGE vs leave the way it currently is. > > BR, > Jani. > > -- > Jani Nikula, Intel Open Source Graphics Center
On Wed, 31 Oct 2018, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote: > On Wed, Oct 31, 2018 at 11:00:54AM +0200, Jani Nikula wrote: >> On Wed, 31 Oct 2018, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote: >> > I saw some mention somewhere on IS_GEN_RANGE, which looked clearer than >> > IS_GEN(dev_priv, s, e). Presumably that did not go anywhere since now >> > the proposal is the above? I have to say I am not sure it reads >> > completely intuitive when seen near in code: >> > >> > IS_GEN(dev_priv, 9) >> > IS_GEN(dev_priv, 8, 9) >> > >> > Looks like a variable arg list and the difference in semantics does not >> > come through. As such I am leaning towards thinking it is too much churn >> > for unclear benefit. Or in other words I thought IS_GEN_RANGE was a >> > better direction. >> >> Okay, thanks for the feedback. I'm not locked into any resolution yet, >> apart from not churning anything until we have a better picture where >> we're going. > > I believe we have 2 orthogonal discussions here where they shouldn't block > each other. > > 1. The addition of DISPLAY_GEN checks to group platforms and prefer display > gen checks over platform codenames. By doing this all platform enabling work for > next platforms gets easier and less bureaucratic. > > 2. consolidated IS_GEN macro vs GEN_RANGE vs leave the way it currently is. IMO if we add some display gen macro, it better be aligned with whatever will be done with IS_GEN and friends from the start. BR, Jani. > >> >> BR, >> Jani. >> >> -- >> Jani Nikula, Intel Open Source Graphics Center
On Wed, Oct 31, 2018 at 10:13:43AM +0200, Jani Nikula wrote: > On Tue, 30 Oct 2018, Lucas De Marchi <lucas.de.marchi@gmail.com> wrote: > > On Tue, Oct 30, 2018 at 11:52:30AM +0200, Jani Nikula wrote: > >> On Mon, 29 Oct 2018, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote: > >> > +#define IS_DISPLAY_GEN2(dev_priv) (!!((dev_priv)->info.display_gen_mask \ > >> > + & BIT(2))) > >> > +#define IS_DISPLAY_GEN3(dev_priv) (!!((dev_priv)->info.display_gen_mask \ > >> > + & BIT(3))) > >> > +#define IS_DISPLAY_GEN4(dev_priv) (!!((dev_priv)->info.display_gen_mask \ > >> > + & BIT(4))) > >> > +#define IS_DISPLAY_GEN5(dev_priv) (!!((dev_priv)->info.display_gen_mask \ > >> > + & BIT(5))) > >> > +#define IS_DISPLAY_GEN6(dev_priv) (!!((dev_priv)->info.display_gen_mask \ > >> > + & BIT(6))) > >> > +#define IS_DISPLAY_GEN7(dev_priv) (!!((dev_priv)->info.display_gen_mask \ > >> > + & BIT(7))) > >> > +#define IS_DISPLAY_GEN8(dev_priv) (!!((dev_priv)->info.display_gen_mask \ > >> > + & BIT(8))) > >> > +#define IS_DISPLAY_GEN9(dev_priv) (!!((dev_priv)->info.display_gen_mask \ > >> > + & BIT(9))) > >> > +#define IS_DISPLAY_GEN10(dev_priv) (!!((dev_priv)->info.display_gen_mask \ > >> > + & BIT(10))) > >> > +#define IS_DISPLAY_GEN11(dev_priv) (!!((dev_priv)->info.display_gen_mask \ > >> > + & BIT(11))) > >> > >> I know this is the same pattern as in IS_GEN<N> above, but shouldn't the > >> compiler end up with the same result if these were simply: > >> > >> #define IS_DISPLAY_GEN2(dev_priv) IS_DISPLAY_GEN(dev_priv, 2, 2) > > > > > > humn... maybe this is too magic, but it works for me and I didn't add any additional > > macro to the kernel to implement it :) > > > > [CI, DON'T TEST THIS] diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > > [CI, DON'T TEST THIS] index 554627dc623c..02a8b51fd733 100644 > > [CI, DON'T TEST THIS] --- a/drivers/gpu/drm/i915/i915_drv.h > > [CI, DON'T TEST THIS] +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -2385,9 +2385,15 @@ intel_info(const struct drm_i915_private *dev_priv) > > GENMASK((e) - 1, (s) - 1)) > > > > /* Returns true if Gen is in inclusive range [Start, End] */ > > -#define IS_GEN(dev_priv, s, e) \ > > +#define _IS_GEN_ARG2(dev_priv, s, e) \ > > (!!((dev_priv)->info.gen_mask & INTEL_GEN_MASK((s), (e)))) > > > > +#define _IS_GEN_ARG1(dev_priv, g) \ > > + (!!((dev_priv)->info.gen_mask & BIT((g) - 1))) > > + > > +#define IS_GEN(dev_priv, ...) \ > > + CONCATENATE(_IS_GEN_ARG, COUNT_ARGS(__VA_ARGS__))((dev_priv), ##__VA_ARGS__) > > + > > /* > > * Return true if revision is in range [since,until] inclusive. > > * > > > > > > So we could use IS_GEN(dev_priv, 2) as well as IS_GEN(dev_priv, 2, 4), which IMO is very clear. > > The same would apply for IS_DISPLAY_GEN() version. And if they generate the same code, we could > > just change the expansion to repeat the argument. > > I like this stuff. > > So I'd prefer IS_GEN(dev_priv, 2) in favor of IS_GEN2(dev_priv) > throughout. > > As long as it doesn't increase the code size too much, but this being > macro magic I don't think it should. > > Care to cook up a patch against current tip to make IS_GEN() take 1 or 2 > args? If I read the above right, you'll get a build error for using > IS_GEN(dev_priv, 1, 2, 3), is that correct? Correct. I'll prepare a patch for that so we can discuss on the approach with a concrete example. Lucas De Marchi > > BR, > Jani. > > > -- > Jani Nikula, Intel Open Source Graphics Center
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index c9e5bab6861b..3242229688e3 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -2349,8 +2349,9 @@ intel_info(const struct drm_i915_private *dev_priv) #define INTEL_INFO(dev_priv) intel_info((dev_priv)) #define DRIVER_CAPS(dev_priv) (&(dev_priv)->caps) -#define INTEL_GEN(dev_priv) ((dev_priv)->info.gen) -#define INTEL_DEVID(dev_priv) ((dev_priv)->info.device_id) +#define INTEL_GEN(dev_priv) ((dev_priv)->info.gen) +#define INTEL_DISPLAY_GEN(dev_priv) ((dev_priv)->info.display_gen) +#define INTEL_DEVID(dev_priv) ((dev_priv)->info.device_id) #define REVID_FOREVER 0xff #define INTEL_REVID(dev_priv) ((dev_priv)->drm.pdev->revision) @@ -2363,6 +2364,8 @@ intel_info(const struct drm_i915_private *dev_priv) /* Returns true if Gen is in inclusive range [Start, End] */ #define IS_GEN(dev_priv, s, e) \ (!!((dev_priv)->info.gen_mask & INTEL_GEN_MASK((s), (e)))) +#define IS_DISPLAY_GEN(dev_priv, s, e) \ + (!!((dev_priv)->info.display_gen_mask & INTEL_GEN_MASK((s), (e)))) /* * Return true if revision is in range [since,until] inclusive. @@ -2532,6 +2535,27 @@ intel_info(const struct drm_i915_private *dev_priv) #define IS_GEN10(dev_priv) (!!((dev_priv)->info.gen_mask & BIT(9))) #define IS_GEN11(dev_priv) (!!((dev_priv)->info.gen_mask & BIT(10))) +#define IS_DISPLAY_GEN2(dev_priv) (!!((dev_priv)->info.display_gen_mask \ + & BIT(2))) +#define IS_DISPLAY_GEN3(dev_priv) (!!((dev_priv)->info.display_gen_mask \ + & BIT(3))) +#define IS_DISPLAY_GEN4(dev_priv) (!!((dev_priv)->info.display_gen_mask \ + & BIT(4))) +#define IS_DISPLAY_GEN5(dev_priv) (!!((dev_priv)->info.display_gen_mask \ + & BIT(5))) +#define IS_DISPLAY_GEN6(dev_priv) (!!((dev_priv)->info.display_gen_mask \ + & BIT(6))) +#define IS_DISPLAY_GEN7(dev_priv) (!!((dev_priv)->info.display_gen_mask \ + & BIT(7))) +#define IS_DISPLAY_GEN8(dev_priv) (!!((dev_priv)->info.display_gen_mask \ + & BIT(8))) +#define IS_DISPLAY_GEN9(dev_priv) (!!((dev_priv)->info.display_gen_mask \ + & BIT(9))) +#define IS_DISPLAY_GEN10(dev_priv) (!!((dev_priv)->info.display_gen_mask \ + & BIT(10))) +#define IS_DISPLAY_GEN11(dev_priv) (!!((dev_priv)->info.display_gen_mask \ + & BIT(11))) + #define IS_LP(dev_priv) (INTEL_INFO(dev_priv)->is_lp) #define IS_GEN9_LP(dev_priv) (IS_GEN9(dev_priv) && IS_LP(dev_priv)) #define IS_GEN9_BC(dev_priv) (IS_GEN9(dev_priv) && !IS_LP(dev_priv)) diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c index 44e745921ac1..fb8caf846c02 100644 --- a/drivers/gpu/drm/i915/i915_pci.c +++ b/drivers/gpu/drm/i915/i915_pci.c @@ -30,7 +30,10 @@ #include "i915_selftest.h" #define PLATFORM(x) .platform = (x), .platform_mask = BIT(x) -#define GEN(x) .gen = (x), .gen_mask = BIT((x) - 1) +#define GEN(x) .gen = (x), .gen_mask = BIT((x) - 1), \ + .display_gen = (x), .display_gen_mask = BIT((x)) +/* Unless explicitly stated otherwise Display gen inherits platform gen */ +#define DISPLAY_GEN(x) .display_gen = (x), .display_gen_mask = BIT((x)) #define GEN_DEFAULT_PIPEOFFSETS \ .pipe_offsets = { PIPE_A_OFFSET, PIPE_B_OFFSET, \ diff --git a/drivers/gpu/drm/i915/intel_device_info.h b/drivers/gpu/drm/i915/intel_device_info.h index b4c2c4eae78b..9f31f29186a8 100644 --- a/drivers/gpu/drm/i915/intel_device_info.h +++ b/drivers/gpu/drm/i915/intel_device_info.h @@ -151,8 +151,10 @@ typedef u8 intel_ring_mask_t; struct intel_device_info { u16 device_id; u16 gen_mask; + u16 display_gen_mask; u8 gen; + u8 display_gen; u8 gt; /* GT number, 0 if undefined */ u8 num_rings; intel_ring_mask_t ring_mask; /* Rings supported by the HW */
Introduce Display Gen. The goal is to use this to minimize the amount of platform codename checks we have nowdays on display code. The introduction of a new platform should be just gen >= current. Just a gen++ without exposing any new feature or ip. so this would minimize the amount of patches needed for a bring-up specially holding them on internal branches. Cc: Jani Nikula <jani.nikula@intel.com> Cc: Lucas De Marchi <lucas.demarchi@intel.com> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> --- drivers/gpu/drm/i915/i915_drv.h | 28 ++++++++++++++++++++++-- drivers/gpu/drm/i915/i915_pci.c | 5 ++++- drivers/gpu/drm/i915/intel_device_info.h | 2 ++ 3 files changed, 32 insertions(+), 3 deletions(-)