Re: [Intel-gfx] [RFC v4 10/11] drm/dp: Extract i915's eDP backlight code into DRM helpers
On Thu, 2021-02-18 at 17:31 +0200, Ville Syrjälä wrote: > On Thu, Feb 18, 2021 at 10:35:05AM +0200, Jani Nikula wrote: > > On Fri, 12 Feb 2021, Lyude Paul wrote: > > > I think it wouldn't be a bad idea to just address this with a followup > > > series > > > instead and use the old DRM_DEBUG_* macros in the mean time. > > > > aux->dev is there, could also use dev_dbg et al. in the mean time. They > > handle NULL dev gracefully too if the driver didn't set that. > > Last I looked aux->dev was random. Some drivers point it at the > connector vs. some at the the pci/platform device. > That's correct-for most SoCs the AUX channel is actually a standalone platform device that isn't associated with the DRM device by default. /But/ I went through the tree yesterday and the day before and did a bunch of cleanup around DP aux registration, added a drm_dev field and hooked it up in every driver with an aux channel, and then converted all of the DP helpers (including dual mode and MST) over to using drm_dbg_*() variants. Once I've gotten through reading all my email for today I'm going to do a quick sanity check on it and then post the series to dri-devel. -- Sincerely, Lyude Paul (she/her) Software Engineer at Red Hat Note: I deal with a lot of emails and have a lot of bugs on my plate. If you've asked me a question, are waiting for a review/merge on a patch, etc. and I haven't responded in a while, please feel free to send me another email to check on my status. I don't bite!
Re: [Intel-gfx] [RFC v4 10/11] drm/dp: Extract i915's eDP backlight code into DRM helpers
On Thu, Feb 18, 2021 at 10:35:05AM +0200, Jani Nikula wrote: > On Fri, 12 Feb 2021, Lyude Paul wrote: > > I think it wouldn't be a bad idea to just address this with a followup > > series > > instead and use the old DRM_DEBUG_* macros in the mean time. > > aux->dev is there, could also use dev_dbg et al. in the mean time. They > handle NULL dev gracefully too if the driver didn't set that. Last I looked aux->dev was random. Some drivers point it at the connector vs. some at the the pci/platform device. -- Ville Syrjälä Intel
Re: [Intel-gfx] [RFC v4 10/11] drm/dp: Extract i915's eDP backlight code into DRM helpers
On Fri, 12 Feb 2021, Lyude Paul wrote: > I think it wouldn't be a bad idea to just address this with a followup series > instead and use the old DRM_DEBUG_* macros in the mean time. aux->dev is there, could also use dev_dbg et al. in the mean time. They handle NULL dev gracefully too if the driver didn't set that. BR, Jani. -- Jani Nikula, Intel Open Source Graphics Center
Re: [Intel-gfx] [RFC v4 10/11] drm/dp: Extract i915's eDP backlight code into DRM helpers
On Wed, 2021-02-10 at 23:15 -0500, Rodrigo Vivi wrote:
> On Mon, Feb 08, 2021 at 06:39:00PM -0500, Lyude Paul wrote:
> > Since we're about to implement eDP backlight support in nouveau using the
> > standard protocol from VESA, we might as well just take the code that's
> > already written for this and move it into a set of shared DRM helpers.
> >
> > Note that these helpers are intended to handle DPCD related backlight
> > control bits such as setting the brightness level over AUX, probing the
> > backlight's TCON, enabling/disabling the backlight over AUX if supported,
> > etc. Any PWM-related portions of backlight control are explicitly left up
> > to the driver, as these will vary from platform to platform.
> >
> > The only exception to this is the calculation of the PWM frequency
> > pre-divider value. This is because the only platform-specific information
> > required for this is the PWM frequency of the panel, which the driver is
> > expected to provide if available. The actual algorithm for calculating this
> > value is standard and is defined in the eDP specification from VESA.
> >
> > Note that these helpers do not yet implement the full range of features
> > the VESA backlight interface provides, and only provide the following
> > functionality (all of which was already present in i915's DPCD backlight
> > support):
> >
> > * Basic control of brightness levels
> > * Basic probing of backlight capabilities
> > * Helpers for enabling and disabling the backlight
> >
> > v3:
> > * Split out changes to i915's backlight code to separate patches to make it
> > easier to review
> > v4:
> > * Style/spelling changes from Thomas Zimmermann
> >
> > Signed-off-by: Lyude Paul
> > Cc: Jani Nikula
> > Cc: Dave Airlie
> > Cc: greg.depo...@gmail.com
> > ---
> > drivers/gpu/drm/drm_dp_helper.c | 332 ++
> > .../drm/i915/display/intel_display_types.h | 5 +-
> > .../drm/i915/display/intel_dp_aux_backlight.c | 285 ++-
> > include/drm/drm_dp_helper.h | 48 +++
> > 4 files changed, 412 insertions(+), 258 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_dp_helper.c
> > b/drivers/gpu/drm/drm_dp_helper.c
> > index eedbb48815b7..1a3d4679d720 100644
> > --- a/drivers/gpu/drm/drm_dp_helper.c
> > +++ b/drivers/gpu/drm/drm_dp_helper.c
> > @@ -3082,3 +3082,335 @@ int drm_dp_pcon_convert_rgb_to_ycbcr(struct
> > drm_dp_aux *aux, u8 color_spc)
> > return 0;
> > }
> > EXPORT_SYMBOL(drm_dp_pcon_convert_rgb_to_ycbcr);
> > +
> > +/**
> > + * drm_edp_backlight_set_level() - Set the backlight level of an eDP panel
> > via AUX
> > + * @aux: The DP AUX channel to use
> > + * @bl: Backlight capability info from drm_edp_backlight_init()
> > + * @level: The brightness level to set
> > + *
> > + * Sets the brightness level of an eDP panel's backlight. Note that the
> > panel's backlight must
> > + * already have been enabled by the driver by calling
> > drm_edp_backlight_enable().
> > + *
> > + * Returns: %0 on success, negative error code on failure
> > + */
> > +int drm_edp_backlight_set_level(struct drm_dp_aux *aux, const struct
> > drm_edp_backlight_info *bl,
> > + u16 level)
> > +{
> > + int ret;
> > + u8 buf[2] = { 0 };
> > +
> > + if (bl->lsb_reg_used) {
> > + buf[0] = (level & 0xff00) >> 8;
> > + buf[1] = (level & 0x00ff);
> > + } else {
> > + buf[0] = level;
> > + }
> > +
> > + ret = drm_dp_dpcd_write(aux, DP_EDP_BACKLIGHT_BRIGHTNESS_MSB, buf,
> > sizeof(buf));
> > + if (ret != sizeof(buf)) {
> > + DRM_ERROR("%s: Failed to write aux backlight level: %d\n",
> > aux->name, ret);
> > + return ret < 0 ? ret : -EIO;
> > + }
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL(drm_edp_backlight_set_level);
> > +
> > +static int
> > +drm_edp_backlight_set_enable(struct drm_dp_aux *aux, const struct
> > drm_edp_backlight_info *bl,
> > + bool enable)
> > +{
> > + int ret;
> > + u8 buf;
> > +
> > + /* The panel uses something other then DPCD for enabling its
> > backlight */
> > + if (!bl->aux_enable)
> > + return 0;
> > +
> > + ret = drm_dp_dpcd_readb(aux, DP_EDP_DISPLAY_CONTROL_REGISTER, );
> > + if (ret != 1) {
> > + DRM_ERROR("%s: Failed to read eDP display control register:
> > %d\n", aux->name, ret);
> > + return ret < 0 ? ret : -EIO;
> > + }
> > + if (enable)
> > + buf |= DP_EDP_BACKLIGHT_ENABLE;
> > + else
> > + buf &= ~DP_EDP_BACKLIGHT_ENABLE;
> > +
> > + ret = drm_dp_dpcd_writeb(aux, DP_EDP_DISPLAY_CONTROL_REGISTER, buf);
> > + if (ret != 1) {
> > + DRM_ERROR("%s: Failed to write eDP display control register:
> > %d\n", aux->name, ret);
> > + return ret < 0 ? ret : -EIO;
> > + }
> > +
> > +
Re: [Intel-gfx] [RFC v4 10/11] drm/dp: Extract i915's eDP backlight code into DRM helpers
On Wed, 2021-02-10 at 23:15 -0500, Rodrigo Vivi wrote:
> On Mon, Feb 08, 2021 at 06:39:00PM -0500, Lyude Paul wrote:
> > Since we're about to implement eDP backlight support in nouveau using the
> > standard protocol from VESA, we might as well just take the code that's
> > already written for this and move it into a set of shared DRM helpers.
> >
> > Note that these helpers are intended to handle DPCD related backlight
> > control bits such as setting the brightness level over AUX, probing the
> > backlight's TCON, enabling/disabling the backlight over AUX if supported,
> > etc. Any PWM-related portions of backlight control are explicitly left up
> > to the driver, as these will vary from platform to platform.
> >
> > The only exception to this is the calculation of the PWM frequency
> > pre-divider value. This is because the only platform-specific information
> > required for this is the PWM frequency of the panel, which the driver is
> > expected to provide if available. The actual algorithm for calculating this
> > value is standard and is defined in the eDP specification from VESA.
> >
> > Note that these helpers do not yet implement the full range of features
> > the VESA backlight interface provides, and only provide the following
> > functionality (all of which was already present in i915's DPCD backlight
> > support):
> >
> > * Basic control of brightness levels
> > * Basic probing of backlight capabilities
> > * Helpers for enabling and disabling the backlight
> >
> > v3:
> > * Split out changes to i915's backlight code to separate patches to make it
> > easier to review
> > v4:
> > * Style/spelling changes from Thomas Zimmermann
> >
> > Signed-off-by: Lyude Paul
> > Cc: Jani Nikula
> > Cc: Dave Airlie
> > Cc: greg.depo...@gmail.com
> > ---
> > drivers/gpu/drm/drm_dp_helper.c | 332 ++
> > .../drm/i915/display/intel_display_types.h | 5 +-
> > .../drm/i915/display/intel_dp_aux_backlight.c | 285 ++-
> > include/drm/drm_dp_helper.h | 48 +++
> > 4 files changed, 412 insertions(+), 258 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_dp_helper.c
> > b/drivers/gpu/drm/drm_dp_helper.c
> > index eedbb48815b7..1a3d4679d720 100644
> > --- a/drivers/gpu/drm/drm_dp_helper.c
> > +++ b/drivers/gpu/drm/drm_dp_helper.c
> > @@ -3082,3 +3082,335 @@ int drm_dp_pcon_convert_rgb_to_ycbcr(struct
> > drm_dp_aux *aux, u8 color_spc)
> > return 0;
> > }
> > EXPORT_SYMBOL(drm_dp_pcon_convert_rgb_to_ycbcr);
> > +
> > +/**
> > + * drm_edp_backlight_set_level() - Set the backlight level of an eDP panel
> > via AUX
> > + * @aux: The DP AUX channel to use
> > + * @bl: Backlight capability info from drm_edp_backlight_init()
> > + * @level: The brightness level to set
> > + *
> > + * Sets the brightness level of an eDP panel's backlight. Note that the
> > panel's backlight must
> > + * already have been enabled by the driver by calling
> > drm_edp_backlight_enable().
> > + *
> > + * Returns: %0 on success, negative error code on failure
> > + */
> > +int drm_edp_backlight_set_level(struct drm_dp_aux *aux, const struct
> > drm_edp_backlight_info *bl,
> > + u16 level)
> > +{
> > + int ret;
> > + u8 buf[2] = { 0 };
> > +
> > + if (bl->lsb_reg_used) {
> > + buf[0] = (level & 0xff00) >> 8;
> > + buf[1] = (level & 0x00ff);
> > + } else {
> > + buf[0] = level;
> > + }
> > +
> > + ret = drm_dp_dpcd_write(aux, DP_EDP_BACKLIGHT_BRIGHTNESS_MSB, buf,
> > sizeof(buf));
> > + if (ret != sizeof(buf)) {
> > + DRM_ERROR("%s: Failed to write aux backlight level: %d\n",
> > aux->name, ret);
> > + return ret < 0 ? ret : -EIO;
> > + }
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL(drm_edp_backlight_set_level);
> > +
> > +static int
> > +drm_edp_backlight_set_enable(struct drm_dp_aux *aux, const struct
> > drm_edp_backlight_info *bl,
> > + bool enable)
> > +{
> > + int ret;
> > + u8 buf;
> > +
> > + /* The panel uses something other then DPCD for enabling its
> > backlight */
> > + if (!bl->aux_enable)
> > + return 0;
> > +
> > + ret = drm_dp_dpcd_readb(aux, DP_EDP_DISPLAY_CONTROL_REGISTER, );
> > + if (ret != 1) {
> > + DRM_ERROR("%s: Failed to read eDP display control register:
> > %d\n", aux->name, ret);
> > + return ret < 0 ? ret : -EIO;
> > + }
> > + if (enable)
> > + buf |= DP_EDP_BACKLIGHT_ENABLE;
> > + else
> > + buf &= ~DP_EDP_BACKLIGHT_ENABLE;
> > +
> > + ret = drm_dp_dpcd_writeb(aux, DP_EDP_DISPLAY_CONTROL_REGISTER, buf);
> > + if (ret != 1) {
> > + DRM_ERROR("%s: Failed to write eDP display control register:
> > %d\n", aux->name, ret);
> > + return ret < 0 ? ret : -EIO;
> > + }
> > +
> > +
Re: [Intel-gfx] [RFC v4 10/11] drm/dp: Extract i915's eDP backlight code into DRM helpers
On Mon, Feb 08, 2021 at 06:39:00PM -0500, Lyude Paul wrote:
> Since we're about to implement eDP backlight support in nouveau using the
> standard protocol from VESA, we might as well just take the code that's
> already written for this and move it into a set of shared DRM helpers.
>
> Note that these helpers are intended to handle DPCD related backlight
> control bits such as setting the brightness level over AUX, probing the
> backlight's TCON, enabling/disabling the backlight over AUX if supported,
> etc. Any PWM-related portions of backlight control are explicitly left up
> to the driver, as these will vary from platform to platform.
>
> The only exception to this is the calculation of the PWM frequency
> pre-divider value. This is because the only platform-specific information
> required for this is the PWM frequency of the panel, which the driver is
> expected to provide if available. The actual algorithm for calculating this
> value is standard and is defined in the eDP specification from VESA.
>
> Note that these helpers do not yet implement the full range of features
> the VESA backlight interface provides, and only provide the following
> functionality (all of which was already present in i915's DPCD backlight
> support):
>
> * Basic control of brightness levels
> * Basic probing of backlight capabilities
> * Helpers for enabling and disabling the backlight
>
> v3:
> * Split out changes to i915's backlight code to separate patches to make it
> easier to review
> v4:
> * Style/spelling changes from Thomas Zimmermann
>
> Signed-off-by: Lyude Paul
> Cc: Jani Nikula
> Cc: Dave Airlie
> Cc: greg.depo...@gmail.com
> ---
> drivers/gpu/drm/drm_dp_helper.c | 332 ++
> .../drm/i915/display/intel_display_types.h| 5 +-
> .../drm/i915/display/intel_dp_aux_backlight.c | 285 ++-
> include/drm/drm_dp_helper.h | 48 +++
> 4 files changed, 412 insertions(+), 258 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> index eedbb48815b7..1a3d4679d720 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -3082,3 +3082,335 @@ int drm_dp_pcon_convert_rgb_to_ycbcr(struct
> drm_dp_aux *aux, u8 color_spc)
> return 0;
> }
> EXPORT_SYMBOL(drm_dp_pcon_convert_rgb_to_ycbcr);
> +
> +/**
> + * drm_edp_backlight_set_level() - Set the backlight level of an eDP panel
> via AUX
> + * @aux: The DP AUX channel to use
> + * @bl: Backlight capability info from drm_edp_backlight_init()
> + * @level: The brightness level to set
> + *
> + * Sets the brightness level of an eDP panel's backlight. Note that the
> panel's backlight must
> + * already have been enabled by the driver by calling
> drm_edp_backlight_enable().
> + *
> + * Returns: %0 on success, negative error code on failure
> + */
> +int drm_edp_backlight_set_level(struct drm_dp_aux *aux, const struct
> drm_edp_backlight_info *bl,
> + u16 level)
> +{
> + int ret;
> + u8 buf[2] = { 0 };
> +
> + if (bl->lsb_reg_used) {
> + buf[0] = (level & 0xff00) >> 8;
> + buf[1] = (level & 0x00ff);
> + } else {
> + buf[0] = level;
> + }
> +
> + ret = drm_dp_dpcd_write(aux, DP_EDP_BACKLIGHT_BRIGHTNESS_MSB, buf,
> sizeof(buf));
> + if (ret != sizeof(buf)) {
> + DRM_ERROR("%s: Failed to write aux backlight level: %d\n",
> aux->name, ret);
> + return ret < 0 ? ret : -EIO;
> + }
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(drm_edp_backlight_set_level);
> +
> +static int
> +drm_edp_backlight_set_enable(struct drm_dp_aux *aux, const struct
> drm_edp_backlight_info *bl,
> + bool enable)
> +{
> + int ret;
> + u8 buf;
> +
> + /* The panel uses something other then DPCD for enabling its backlight
> */
> + if (!bl->aux_enable)
> + return 0;
> +
> + ret = drm_dp_dpcd_readb(aux, DP_EDP_DISPLAY_CONTROL_REGISTER, );
> + if (ret != 1) {
> + DRM_ERROR("%s: Failed to read eDP display control register:
> %d\n", aux->name, ret);
> + return ret < 0 ? ret : -EIO;
> + }
> + if (enable)
> + buf |= DP_EDP_BACKLIGHT_ENABLE;
> + else
> + buf &= ~DP_EDP_BACKLIGHT_ENABLE;
> +
> + ret = drm_dp_dpcd_writeb(aux, DP_EDP_DISPLAY_CONTROL_REGISTER, buf);
> + if (ret != 1) {
> + DRM_ERROR("%s: Failed to write eDP display control register:
> %d\n", aux->name, ret);
> + return ret < 0 ? ret : -EIO;
> + }
> +
> + return 0;
> +}
> +
> +/**
> + * drm_edp_backlight_enable() - Enable an eDP panel's backlight using DPCD
> + * @aux: The DP AUX channel to use
> + * @bl: Backlight capability info from drm_edp_backlight_init()
> + * @level: The initial backlight level to set via AUX, if there is one
> + *
> + * This function handles enabling DPCD backlight controls on a panel over
> DPCD,
