| Message ID | 24f51db0ce2b1efede9033153f6479cbca395342.1490203284.git.joabreu@synopsys.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Mar 22, 2017 at 05:35:57PM +0000, Jose Abreu wrote: > We can't expect userspace to have full support for all HDMI 2.0+ > features. Instead of expecting/waiting for userspace to support > the new features add a knob, much like the stereo knob, so that > DRM core will only expose the features when asked too. > > Signed-off-by: Jose Abreu <joabreu@synopsys.com> > Cc: Carlos Palminha <palminha@synopsys.com> > Cc: dri-devel@lists.freedesktop.org > --- > drivers/gpu/drm/drm_ioctl.c | 5 +++++ > include/drm/drm_file.h | 8 ++++++++ > include/uapi/drm/drm.h | 7 +++++++ > 3 files changed, 20 insertions(+) > > diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c > index a7c61c2..2430e2e 100644 > --- a/drivers/gpu/drm/drm_ioctl.c > +++ b/drivers/gpu/drm/drm_ioctl.c > @@ -318,6 +318,11 @@ static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_ > file_priv->atomic = req->value; > file_priv->universal_planes = req->value; > break; > + case DRM_CLIENT_CAP_HDMI2: > + if (req->value > 1) > + return -EINVAL; > + file_priv->hdmi2_allowed = req->value; Needs some userspace to make use of this (like the -modesetting x.org driver). -Daniel > + break; > default: > return -EINVAL; > } > diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h > index 5dd27ae..7b97d85 100644 > --- a/include/drm/drm_file.h > +++ b/include/drm/drm_file.h > @@ -192,6 +192,14 @@ struct drm_file { > unsigned is_master:1; > > /** > + * @hdmi2_allowed: > + * > + * True if client understands HDMI 2.0+ features like, for example, > + * extended aspect ratios > + */ > + unsigned hdmi2_allowed:1; > + > + /** > * @master: > * > * Master this node is currently associated with. Only relevant if > diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h > index b2c5284..9e25138 100644 > --- a/include/uapi/drm/drm.h > +++ b/include/uapi/drm/drm.h > @@ -678,6 +678,13 @@ struct drm_get_cap { > */ > #define DRM_CLIENT_CAP_ATOMIC 3 > > +/** > + * DRM_CLIENT_CAP_HDMI2 > + * > + * If set to 1, the DRM core will expose HDMI 2.0+ features to userspace > + */ > +#define DRM_CLIENT_CAP_HDMI2 4 > + > /** DRM_IOCTL_SET_CLIENT_CAP ioctl argument type */ > struct drm_set_client_cap { > __u64 capability; > -- > 1.9.1 > > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi Daniel, On 23-03-2017 07:22, Daniel Vetter wrote: > On Wed, Mar 22, 2017 at 05:35:57PM +0000, Jose Abreu wrote: >> We can't expect userspace to have full support for all HDMI 2.0+ >> features. Instead of expecting/waiting for userspace to support >> the new features add a knob, much like the stereo knob, so that >> DRM core will only expose the features when asked too. >> >> Signed-off-by: Jose Abreu <joabreu@synopsys.com> >> Cc: Carlos Palminha <palminha@synopsys.com> >> Cc: dri-devel@lists.freedesktop.org >> --- >> drivers/gpu/drm/drm_ioctl.c | 5 +++++ >> include/drm/drm_file.h | 8 ++++++++ >> include/uapi/drm/drm.h | 7 +++++++ >> 3 files changed, 20 insertions(+) >> >> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c >> index a7c61c2..2430e2e 100644 >> --- a/drivers/gpu/drm/drm_ioctl.c >> +++ b/drivers/gpu/drm/drm_ioctl.c >> @@ -318,6 +318,11 @@ static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_ >> file_priv->atomic = req->value; >> file_priv->universal_planes = req->value; >> break; >> + case DRM_CLIENT_CAP_HDMI2: >> + if (req->value > 1) >> + return -EINVAL; >> + file_priv->hdmi2_allowed = req->value; > Needs some userspace to make use of this (like the -modesetting x.org > driver). > -Daniel I see your point but shouldn't this be merged before actually starting to work in userspace? Best regards, Jose Miguel Abreu > >> + break; >> default: >> return -EINVAL; >> } >> diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h >> index 5dd27ae..7b97d85 100644 >> --- a/include/drm/drm_file.h >> +++ b/include/drm/drm_file.h >> @@ -192,6 +192,14 @@ struct drm_file { >> unsigned is_master:1; >> >> /** >> + * @hdmi2_allowed: >> + * >> + * True if client understands HDMI 2.0+ features like, for example, >> + * extended aspect ratios >> + */ >> + unsigned hdmi2_allowed:1; >> + >> + /** >> * @master: >> * >> * Master this node is currently associated with. Only relevant if >> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h >> index b2c5284..9e25138 100644 >> --- a/include/uapi/drm/drm.h >> +++ b/include/uapi/drm/drm.h >> @@ -678,6 +678,13 @@ struct drm_get_cap { >> */ >> #define DRM_CLIENT_CAP_ATOMIC 3 >> >> +/** >> + * DRM_CLIENT_CAP_HDMI2 >> + * >> + * If set to 1, the DRM core will expose HDMI 2.0+ features to userspace >> + */ >> +#define DRM_CLIENT_CAP_HDMI2 4 >> + >> /** DRM_IOCTL_SET_CLIENT_CAP ioctl argument type */ >> struct drm_set_client_cap { >> __u64 capability; >> -- >> 1.9.1 >> >> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.freedesktop.org_mailman_listinfo_dri-2Ddevel&d=DwIBAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=4lBpQovdR7O8jeC7mV8SX9xvTypMtV8CBDEDg3bGgzw&s=h-gqFlO62i53pir0j2lJ-upE_bXQxhn3BK1nhdxoB6Y&e=
On Thu, Mar 23, 2017 at 10:44:16AM +0000, Jose Abreu wrote: > Hi Daniel, > > > On 23-03-2017 07:22, Daniel Vetter wrote: > > On Wed, Mar 22, 2017 at 05:35:57PM +0000, Jose Abreu wrote: > >> We can't expect userspace to have full support for all HDMI 2.0+ > >> features. Instead of expecting/waiting for userspace to support > >> the new features add a knob, much like the stereo knob, so that > >> DRM core will only expose the features when asked too. > >> > >> Signed-off-by: Jose Abreu <joabreu@synopsys.com> > >> Cc: Carlos Palminha <palminha@synopsys.com> > >> Cc: dri-devel@lists.freedesktop.org > >> --- > >> drivers/gpu/drm/drm_ioctl.c | 5 +++++ > >> include/drm/drm_file.h | 8 ++++++++ > >> include/uapi/drm/drm.h | 7 +++++++ > >> 3 files changed, 20 insertions(+) > >> > >> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c > >> index a7c61c2..2430e2e 100644 > >> --- a/drivers/gpu/drm/drm_ioctl.c > >> +++ b/drivers/gpu/drm/drm_ioctl.c > >> @@ -318,6 +318,11 @@ static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_ > >> file_priv->atomic = req->value; > >> file_priv->universal_planes = req->value; > >> break; > >> + case DRM_CLIENT_CAP_HDMI2: > >> + if (req->value > 1) > >> + return -EINVAL; > >> + file_priv->hdmi2_allowed = req->value; > > Needs some userspace to make use of this (like the -modesetting x.org > > driver). > > -Daniel > > I see your point but shouldn't this be merged before actually > starting to work in userspace? No, see https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#open-source-userspace-requirements for reasons behind this and the precise flow. Cheers, Daniel > > Best regards, > Jose Miguel Abreu > > > > >> + break; > >> default: > >> return -EINVAL; > >> } > >> diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h > >> index 5dd27ae..7b97d85 100644 > >> --- a/include/drm/drm_file.h > >> +++ b/include/drm/drm_file.h > >> @@ -192,6 +192,14 @@ struct drm_file { > >> unsigned is_master:1; > >> > >> /** > >> + * @hdmi2_allowed: > >> + * > >> + * True if client understands HDMI 2.0+ features like, for example, > >> + * extended aspect ratios > >> + */ > >> + unsigned hdmi2_allowed:1; > >> + > >> + /** > >> * @master: > >> * > >> * Master this node is currently associated with. Only relevant if > >> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h > >> index b2c5284..9e25138 100644 > >> --- a/include/uapi/drm/drm.h > >> +++ b/include/uapi/drm/drm.h > >> @@ -678,6 +678,13 @@ struct drm_get_cap { > >> */ > >> #define DRM_CLIENT_CAP_ATOMIC 3 > >> > >> +/** > >> + * DRM_CLIENT_CAP_HDMI2 > >> + * > >> + * If set to 1, the DRM core will expose HDMI 2.0+ features to userspace > >> + */ > >> +#define DRM_CLIENT_CAP_HDMI2 4 > >> + > >> /** DRM_IOCTL_SET_CLIENT_CAP ioctl argument type */ > >> struct drm_set_client_cap { > >> __u64 capability; > >> -- > >> 1.9.1 > >> > >> > >> _______________________________________________ > >> dri-devel mailing list > >> dri-devel@lists.freedesktop.org > >> https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.freedesktop.org_mailman_listinfo_dri-2Ddevel&d=DwIBAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=4lBpQovdR7O8jeC7mV8SX9xvTypMtV8CBDEDg3bGgzw&s=h-gqFlO62i53pir0j2lJ-upE_bXQxhn3BK1nhdxoB6Y&e= >
Hi Daniel, On 23-03-2017 11:00, Daniel Vetter wrote: > On Thu, Mar 23, 2017 at 10:44:16AM +0000, Jose Abreu wrote: >> Hi Daniel, >> >> >> On 23-03-2017 07:22, Daniel Vetter wrote: >>> On Wed, Mar 22, 2017 at 05:35:57PM +0000, Jose Abreu wrote: >>>> We can't expect userspace to have full support for all HDMI 2.0+ >>>> features. Instead of expecting/waiting for userspace to support >>>> the new features add a knob, much like the stereo knob, so that >>>> DRM core will only expose the features when asked too. >>>> >>>> Signed-off-by: Jose Abreu <joabreu@synopsys.com> >>>> Cc: Carlos Palminha <palminha@synopsys.com> >>>> Cc: dri-devel@lists.freedesktop.org >>>> --- >>>> drivers/gpu/drm/drm_ioctl.c | 5 +++++ >>>> include/drm/drm_file.h | 8 ++++++++ >>>> include/uapi/drm/drm.h | 7 +++++++ >>>> 3 files changed, 20 insertions(+) >>>> >>>> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c >>>> index a7c61c2..2430e2e 100644 >>>> --- a/drivers/gpu/drm/drm_ioctl.c >>>> +++ b/drivers/gpu/drm/drm_ioctl.c >>>> @@ -318,6 +318,11 @@ static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_ >>>> file_priv->atomic = req->value; >>>> file_priv->universal_planes = req->value; >>>> break; >>>> + case DRM_CLIENT_CAP_HDMI2: >>>> + if (req->value > 1) >>>> + return -EINVAL; >>>> + file_priv->hdmi2_allowed = req->value; >>> Needs some userspace to make use of this (like the -modesetting x.org >>> driver). >>> -Daniel >> I see your point but shouldn't this be merged before actually >> starting to work in userspace? > No, see > > https://urldefense.proofpoint.com/v2/url?u=https-3A__dri.freedesktop.org_docs_drm_gpu_drm-2Duapi.html-23open-2Dsource-2Duserspace-2Drequirements&d=DwIBAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=tcc2TQFpF7ats3OIzYgmFrHaeymBrPtqHphO4Ui4AUY&s=qWySmgpGaOwxC8Jl3A5qZVf4aS2qz3LXshVdv3FehJc&e= > > for reasons behind this and the precise flow. > > Cheers, Daniel Thanks, I will do that. I can leave the flag and just limit the export of aspect ratios for now, we can limit more stuff later if needed. What do you think? Another thing: Is it safe to enable this in the modesetting driver, shouldn't the modesetting clients also know about the flag or does modesetting handles this correctly? (I'm not very familiar with this driver, sorry). Best regards, Jose Miguel Abreu > >> Best regards, >> Jose Miguel Abreu >> >>>> + break; >>>> default: >>>> return -EINVAL; >>>> } >>>> diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h >>>> index 5dd27ae..7b97d85 100644 >>>> --- a/include/drm/drm_file.h >>>> +++ b/include/drm/drm_file.h >>>> @@ -192,6 +192,14 @@ struct drm_file { >>>> unsigned is_master:1; >>>> >>>> /** >>>> + * @hdmi2_allowed: >>>> + * >>>> + * True if client understands HDMI 2.0+ features like, for example, >>>> + * extended aspect ratios >>>> + */ >>>> + unsigned hdmi2_allowed:1; >>>> + >>>> + /** >>>> * @master: >>>> * >>>> * Master this node is currently associated with. Only relevant if >>>> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h >>>> index b2c5284..9e25138 100644 >>>> --- a/include/uapi/drm/drm.h >>>> +++ b/include/uapi/drm/drm.h >>>> @@ -678,6 +678,13 @@ struct drm_get_cap { >>>> */ >>>> #define DRM_CLIENT_CAP_ATOMIC 3 >>>> >>>> +/** >>>> + * DRM_CLIENT_CAP_HDMI2 >>>> + * >>>> + * If set to 1, the DRM core will expose HDMI 2.0+ features to userspace >>>> + */ >>>> +#define DRM_CLIENT_CAP_HDMI2 4 >>>> + >>>> /** DRM_IOCTL_SET_CLIENT_CAP ioctl argument type */ >>>> struct drm_set_client_cap { >>>> __u64 capability; >>>> -- >>>> 1.9.1 >>>> >>>> >>>> _______________________________________________ >>>> dri-devel mailing list >>>> dri-devel@lists.freedesktop.org >>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.freedesktop.org_mailman_listinfo_dri-2Ddevel&d=DwIBAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=4lBpQovdR7O8jeC7mV8SX9xvTypMtV8CBDEDg3bGgzw&s=h-gqFlO62i53pir0j2lJ-upE_bXQxhn3BK1nhdxoB6Y&e=
On Mon, Mar 27, 2017 at 02:47:08PM +0100, Jose Abreu wrote: > Hi Daniel, > > > On 23-03-2017 11:00, Daniel Vetter wrote: > > On Thu, Mar 23, 2017 at 10:44:16AM +0000, Jose Abreu wrote: > >> Hi Daniel, > >> > >> > >> On 23-03-2017 07:22, Daniel Vetter wrote: > >>> On Wed, Mar 22, 2017 at 05:35:57PM +0000, Jose Abreu wrote: > >>>> We can't expect userspace to have full support for all HDMI 2.0+ > >>>> features. Instead of expecting/waiting for userspace to support > >>>> the new features add a knob, much like the stereo knob, so that > >>>> DRM core will only expose the features when asked too. > >>>> > >>>> Signed-off-by: Jose Abreu <joabreu@synopsys.com> > >>>> Cc: Carlos Palminha <palminha@synopsys.com> > >>>> Cc: dri-devel@lists.freedesktop.org > >>>> --- > >>>> drivers/gpu/drm/drm_ioctl.c | 5 +++++ > >>>> include/drm/drm_file.h | 8 ++++++++ > >>>> include/uapi/drm/drm.h | 7 +++++++ > >>>> 3 files changed, 20 insertions(+) > >>>> > >>>> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c > >>>> index a7c61c2..2430e2e 100644 > >>>> --- a/drivers/gpu/drm/drm_ioctl.c > >>>> +++ b/drivers/gpu/drm/drm_ioctl.c > >>>> @@ -318,6 +318,11 @@ static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_ > >>>> file_priv->atomic = req->value; > >>>> file_priv->universal_planes = req->value; > >>>> break; > >>>> + case DRM_CLIENT_CAP_HDMI2: > >>>> + if (req->value > 1) > >>>> + return -EINVAL; > >>>> + file_priv->hdmi2_allowed = req->value; > >>> Needs some userspace to make use of this (like the -modesetting x.org > >>> driver). > >>> -Daniel > >> I see your point but shouldn't this be merged before actually > >> starting to work in userspace? > > No, see > > > > https://urldefense.proofpoint.com/v2/url?u=https-3A__dri.freedesktop.org_docs_drm_gpu_drm-2Duapi.html-23open-2Dsource-2Duserspace-2Drequirements&d=DwIBAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=tcc2TQFpF7ats3OIzYgmFrHaeymBrPtqHphO4Ui4AUY&s=qWySmgpGaOwxC8Jl3A5qZVf4aS2qz3LXshVdv3FehJc&e= > > > > for reasons behind this and the precise flow. > > > > Cheers, Daniel > > Thanks, I will do that. I can leave the flag and just limit the > export of aspect ratios for now, we can limit more stuff later if > needed. What do you think? > > Another thing: Is it safe to enable this in the modesetting > driver, shouldn't the modesetting clients also know about the > flag or does modesetting handles this correctly? (I'm not very > familiar with this driver, sorry). Probably needs an xrandr revision too. -Daniel > > Best regards, > Jose Miguel Abreu > > > > >> Best regards, > >> Jose Miguel Abreu > >> > >>>> + break; > >>>> default: > >>>> return -EINVAL; > >>>> } > >>>> diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h > >>>> index 5dd27ae..7b97d85 100644 > >>>> --- a/include/drm/drm_file.h > >>>> +++ b/include/drm/drm_file.h > >>>> @@ -192,6 +192,14 @@ struct drm_file { > >>>> unsigned is_master:1; > >>>> > >>>> /** > >>>> + * @hdmi2_allowed: > >>>> + * > >>>> + * True if client understands HDMI 2.0+ features like, for example, > >>>> + * extended aspect ratios > >>>> + */ > >>>> + unsigned hdmi2_allowed:1; > >>>> + > >>>> + /** > >>>> * @master: > >>>> * > >>>> * Master this node is currently associated with. Only relevant if > >>>> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h > >>>> index b2c5284..9e25138 100644 > >>>> --- a/include/uapi/drm/drm.h > >>>> +++ b/include/uapi/drm/drm.h > >>>> @@ -678,6 +678,13 @@ struct drm_get_cap { > >>>> */ > >>>> #define DRM_CLIENT_CAP_ATOMIC 3 > >>>> > >>>> +/** > >>>> + * DRM_CLIENT_CAP_HDMI2 > >>>> + * > >>>> + * If set to 1, the DRM core will expose HDMI 2.0+ features to userspace > >>>> + */ > >>>> +#define DRM_CLIENT_CAP_HDMI2 4 > >>>> + > >>>> /** DRM_IOCTL_SET_CLIENT_CAP ioctl argument type */ > >>>> struct drm_set_client_cap { > >>>> __u64 capability; > >>>> -- > >>>> 1.9.1 > >>>> > >>>> > >>>> _______________________________________________ > >>>> dri-devel mailing list > >>>> dri-devel@lists.freedesktop.org > >>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.freedesktop.org_mailman_listinfo_dri-2Ddevel&d=DwIBAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=4lBpQovdR7O8jeC7mV8SX9xvTypMtV8CBDEDg3bGgzw&s=h-gqFlO62i53pir0j2lJ-upE_bXQxhn3BK1nhdxoB6Y&e= >
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index a7c61c2..2430e2e 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -318,6 +318,11 @@ static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_ file_priv->atomic = req->value; file_priv->universal_planes = req->value; break; + case DRM_CLIENT_CAP_HDMI2: + if (req->value > 1) + return -EINVAL; + file_priv->hdmi2_allowed = req->value; + break; default: return -EINVAL; } diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h index 5dd27ae..7b97d85 100644 --- a/include/drm/drm_file.h +++ b/include/drm/drm_file.h @@ -192,6 +192,14 @@ struct drm_file { unsigned is_master:1; /** + * @hdmi2_allowed: + * + * True if client understands HDMI 2.0+ features like, for example, + * extended aspect ratios + */ + unsigned hdmi2_allowed:1; + + /** * @master: * * Master this node is currently associated with. Only relevant if diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h index b2c5284..9e25138 100644 --- a/include/uapi/drm/drm.h +++ b/include/uapi/drm/drm.h @@ -678,6 +678,13 @@ struct drm_get_cap { */ #define DRM_CLIENT_CAP_ATOMIC 3 +/** + * DRM_CLIENT_CAP_HDMI2 + * + * If set to 1, the DRM core will expose HDMI 2.0+ features to userspace + */ +#define DRM_CLIENT_CAP_HDMI2 4 + /** DRM_IOCTL_SET_CLIENT_CAP ioctl argument type */ struct drm_set_client_cap { __u64 capability;
We can't expect userspace to have full support for all HDMI 2.0+ features. Instead of expecting/waiting for userspace to support the new features add a knob, much like the stereo knob, so that DRM core will only expose the features when asked too. Signed-off-by: Jose Abreu <joabreu@synopsys.com> Cc: Carlos Palminha <palminha@synopsys.com> Cc: dri-devel@lists.freedesktop.org --- drivers/gpu/drm/drm_ioctl.c | 5 +++++ include/drm/drm_file.h | 8 ++++++++ include/uapi/drm/drm.h | 7 +++++++ 3 files changed, 20 insertions(+)