mbox series

[v2,0/4] drm/dp, drm/i915: Finish basic PWM support for VESA backlight helpers

Message ID 20211001225344.1752203-1-lyude@redhat.com (mailing list archive)
Headers show
Series drm/dp, drm/i915: Finish basic PWM support for VESA backlight helpers | expand

Message

Lyude Paul Oct. 1, 2021, 10:53 p.m. UTC
When I originally moved all of the VESA backlight code in i915 into DRM
helpers, one of the things I didn't have the hardware or time for
testing was machines that used a combination of PWM and DPCD in order to
control their backlights. This has since then caused some breakages and
resulted in us disabling DPCD backlight support on such machines. This
works fine, unless you have a machine that actually needs this
functionality for backlight controls to work at all. Additionally, we
will need to support PWM for when we start adding support for VESA's
product (as in the product of multiplication) control mode for better
brightness ranges.

So - let's finally finish up implementing basic support for these types
of backlights to solve these problems in our DP helpers, along with
implementing support for this in i915. And since digging into this issue
solved the last questions we really had about probing backlights in i915
for the most part, let's update some of the comments around that as
well!

Changes:
* Fixup docs
* Add patch to stop us from breaking nouveau

Lyude Paul (4):
  drm/i915: Add support for panels with VESA backlights with PWM
    enable/disable
  drm/nouveau/kms/nv50-: Explicitly check DPCD backlights for aux
    enable/brightness
  drm/dp, drm/i915: Add support for VESA backlights using PWM for
    brightness control
  drm/i915: Clarify probing order in intel_dp_aux_init_backlight_funcs()

 drivers/gpu/drm/drm_dp_helper.c               | 75 +++++++++++------
 .../drm/i915/display/intel_dp_aux_backlight.c | 80 ++++++++++++++-----
 drivers/gpu/drm/nouveau/nouveau_backlight.c   |  5 +-
 include/drm/drm_dp_helper.h                   |  7 +-
 4 files changed, 122 insertions(+), 45 deletions(-)

Comments

Hans de Goede Oct. 2, 2021, 9:14 a.m. UTC | #1
Hi Lyude,

On 10/2/21 12:53 AM, Lyude Paul wrote:
> When I originally moved all of the VESA backlight code in i915 into DRM
> helpers, one of the things I didn't have the hardware or time for
> testing was machines that used a combination of PWM and DPCD in order to
> control their backlights. This has since then caused some breakages and
> resulted in us disabling DPCD backlight support on such machines. This
> works fine, unless you have a machine that actually needs this
> functionality for backlight controls to work at all. Additionally, we
> will need to support PWM for when we start adding support for VESA's
> product (as in the product of multiplication) control mode for better
> brightness ranges.
> 
> So - let's finally finish up implementing basic support for these types
> of backlights to solve these problems in our DP helpers, along with
> implementing support for this in i915. And since digging into this issue
> solved the last questions we really had about probing backlights in i915
> for the most part, let's update some of the comments around that as
> well!

Backlight control is a topic which I'm reasonably familiar with,
do you want me to review this series for you ?

Regards,

Hans



> 
> Changes:
> * Fixup docs
> * Add patch to stop us from breaking nouveau
> 
> Lyude Paul (4):
>   drm/i915: Add support for panels with VESA backlights with PWM
>     enable/disable
>   drm/nouveau/kms/nv50-: Explicitly check DPCD backlights for aux
>     enable/brightness
>   drm/dp, drm/i915: Add support for VESA backlights using PWM for
>     brightness control
>   drm/i915: Clarify probing order in intel_dp_aux_init_backlight_funcs()
> 
>  drivers/gpu/drm/drm_dp_helper.c               | 75 +++++++++++------
>  .../drm/i915/display/intel_dp_aux_backlight.c | 80 ++++++++++++++-----
>  drivers/gpu/drm/nouveau/nouveau_backlight.c   |  5 +-
>  include/drm/drm_dp_helper.h                   |  7 +-
>  4 files changed, 122 insertions(+), 45 deletions(-)
>
Lyude Paul Oct. 5, 2021, 8:26 p.m. UTC | #2
On Sat, 2021-10-02 at 11:14 +0200, Hans de Goede wrote:
> Hi Lyude,
> 
> On 10/2/21 12:53 AM, Lyude Paul wrote:
> > When I originally moved all of the VESA backlight code in i915 into DRM
> > helpers, one of the things I didn't have the hardware or time for
> > testing was machines that used a combination of PWM and DPCD in order to
> > control their backlights. This has since then caused some breakages and
> > resulted in us disabling DPCD backlight support on such machines. This
> > works fine, unless you have a machine that actually needs this
> > functionality for backlight controls to work at all. Additionally, we
> > will need to support PWM for when we start adding support for VESA's
> > product (as in the product of multiplication) control mode for better
> > brightness ranges.
> > 
> > So - let's finally finish up implementing basic support for these types
> > of backlights to solve these problems in our DP helpers, along with
> > implementing support for this in i915. And since digging into this issue
> > solved the last questions we really had about probing backlights in i915
> > for the most part, let's update some of the comments around that as
> > well!
> 
> Backlight control is a topic which I'm reasonably familiar with,
> do you want me to review this series for you ?

Possibly, although I definitely want to make sure that someone from Intel gets
a chance to review this as well. I'm more curious if you might happen to have
any systems that would be able to test this.

> 
> Regards,
> 
> Hans
> 
> 
> 
> > 
> > Changes:
> > * Fixup docs
> > * Add patch to stop us from breaking nouveau
> > 
> > Lyude Paul (4):
> >   drm/i915: Add support for panels with VESA backlights with PWM
> >     enable/disable
> >   drm/nouveau/kms/nv50-: Explicitly check DPCD backlights for aux
> >     enable/brightness
> >   drm/dp, drm/i915: Add support for VESA backlights using PWM for
> >     brightness control
> >   drm/i915: Clarify probing order in intel_dp_aux_init_backlight_funcs()
> > 
> >  drivers/gpu/drm/drm_dp_helper.c               | 75 +++++++++++------
> >  .../drm/i915/display/intel_dp_aux_backlight.c | 80 ++++++++++++++-----
> >  drivers/gpu/drm/nouveau/nouveau_backlight.c   |  5 +-
> >  include/drm/drm_dp_helper.h                   |  7 +-
> >  4 files changed, 122 insertions(+), 45 deletions(-)
> > 
>
Hans de Goede Oct. 7, 2021, 9:54 a.m. UTC | #3
Hi,

On 10/5/21 10:26 PM, Lyude Paul wrote:
> On Sat, 2021-10-02 at 11:14 +0200, Hans de Goede wrote:
>> Hi Lyude,
>>
>> On 10/2/21 12:53 AM, Lyude Paul wrote:
>>> When I originally moved all of the VESA backlight code in i915 into DRM
>>> helpers, one of the things I didn't have the hardware or time for
>>> testing was machines that used a combination of PWM and DPCD in order to
>>> control their backlights. This has since then caused some breakages and
>>> resulted in us disabling DPCD backlight support on such machines. This
>>> works fine, unless you have a machine that actually needs this
>>> functionality for backlight controls to work at all. Additionally, we
>>> will need to support PWM for when we start adding support for VESA's
>>> product (as in the product of multiplication) control mode for better
>>> brightness ranges.
>>>
>>> So - let's finally finish up implementing basic support for these types
>>> of backlights to solve these problems in our DP helpers, along with
>>> implementing support for this in i915. And since digging into this issue
>>> solved the last questions we really had about probing backlights in i915
>>> for the most part, let's update some of the comments around that as
>>> well!
>>
>> Backlight control is a topic which I'm reasonably familiar with,
>> do you want me to review this series for you ?
> 
> Possibly, although I definitely want to make sure that someone from Intel gets
> a chance to review this as well.

I already expected you would also want someone from Intel to take a look
as well :)

So to figure out if I have system to test this I ended up taking a close
look at the code, which was a nice learning experience. Interesting how
some panels use a combination of DCPD + pwm/gpio for enable/disable and/or
pwm for brightness level control.

I wonder though what DCPD still does if pwm is used for both the
enable/disable and brightness-level control ?

Since I was taking a close look already I decided to do turn this into
a full, the entire set looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

> I'm more curious if you might happen to have
> any systems that would be able to test this.

I don't believe I have a system where I can test this, The only systems
which I have which presumably (I did not check) use eDP / DCPD backlight
control are boring ThinkPads with only i915 gfx.

Regards,

Hans



>>> Changes:
>>> * Fixup docs
>>> * Add patch to stop us from breaking nouveau
>>>
>>> Lyude Paul (4):
>>>   drm/i915: Add support for panels with VESA backlights with PWM
>>>     enable/disable
>>>   drm/nouveau/kms/nv50-: Explicitly check DPCD backlights for aux
>>>     enable/brightness
>>>   drm/dp, drm/i915: Add support for VESA backlights using PWM for
>>>     brightness control
>>>   drm/i915: Clarify probing order in intel_dp_aux_init_backlight_funcs()
>>>
>>>  drivers/gpu/drm/drm_dp_helper.c               | 75 +++++++++++------
>>>  .../drm/i915/display/intel_dp_aux_backlight.c | 80 ++++++++++++++-----
>>>  drivers/gpu/drm/nouveau/nouveau_backlight.c   |  5 +-
>>>  include/drm/drm_dp_helper.h                   |  7 +-
>>>  4 files changed, 122 insertions(+), 45 deletions(-)
>>>
>>
>
Hans de Goede Oct. 7, 2021, 10:07 a.m. UTC | #4
Hi,

On 10/7/21 11:54 AM, Hans de Goede wrote:
> Hi,
> 
> On 10/5/21 10:26 PM, Lyude Paul wrote:
>> On Sat, 2021-10-02 at 11:14 +0200, Hans de Goede wrote:
>>> Hi Lyude,
>>>
>>> On 10/2/21 12:53 AM, Lyude Paul wrote:
>>>> When I originally moved all of the VESA backlight code in i915 into DRM
>>>> helpers, one of the things I didn't have the hardware or time for
>>>> testing was machines that used a combination of PWM and DPCD in order to
>>>> control their backlights. This has since then caused some breakages and
>>>> resulted in us disabling DPCD backlight support on such machines. This
>>>> works fine, unless you have a machine that actually needs this
>>>> functionality for backlight controls to work at all. Additionally, we
>>>> will need to support PWM for when we start adding support for VESA's
>>>> product (as in the product of multiplication) control mode for better
>>>> brightness ranges.
>>>>
>>>> So - let's finally finish up implementing basic support for these types
>>>> of backlights to solve these problems in our DP helpers, along with
>>>> implementing support for this in i915. And since digging into this issue
>>>> solved the last questions we really had about probing backlights in i915
>>>> for the most part, let's update some of the comments around that as
>>>> well!
>>>
>>> Backlight control is a topic which I'm reasonably familiar with,
>>> do you want me to review this series for you ?
>>
>> Possibly, although I definitely want to make sure that someone from Intel gets
>> a chance to review this as well.
> 
> I already expected you would also want someone from Intel to take a look
> as well :)
> 
> So to figure out if I have system to test this I ended up taking a close
> look at the code, which was a nice learning experience. Interesting how
> some panels use a combination of DCPD + pwm/gpio for enable/disable and/or
> pwm for brightness level control.
> 
> I wonder though what DCPD still does if pwm is used for both the
> enable/disable and brightness-level control ?
> 
> Since I was taking a close look already I decided to do turn this into
> a full, the entire set looks good to me:
> 
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> 
>> I'm more curious if you might happen to have
>> any systems that would be able to test this.
> 
> I don't believe I have a system where I can test this, The only systems
> which I have which presumably (I did not check) use eDP / DCPD backlight
> control are boring ThinkPads with only i915 gfx.

Continuing with reading my email I see that you've also posted a v5
with 1 new patch:

[PATCH v3 3/5] drm/dp: Disable unsupported features in DP_EDP_BACKLIGHT_MODE_SET_REGISTER

That patch looks good to me too, so you may also add my:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

To that one / to the entire v3 series.

Regards,

Hans



>>>> Changes:
>>>> * Fixup docs
>>>> * Add patch to stop us from breaking nouveau
>>>>
>>>> Lyude Paul (4):
>>>>   drm/i915: Add support for panels with VESA backlights with PWM
>>>>     enable/disable
>>>>   drm/nouveau/kms/nv50-: Explicitly check DPCD backlights for aux
>>>>     enable/brightness
>>>>   drm/dp, drm/i915: Add support for VESA backlights using PWM for
>>>>     brightness control
>>>>   drm/i915: Clarify probing order in intel_dp_aux_init_backlight_funcs()
>>>>
>>>>  drivers/gpu/drm/drm_dp_helper.c               | 75 +++++++++++------
>>>>  .../drm/i915/display/intel_dp_aux_backlight.c | 80 ++++++++++++++-----
>>>>  drivers/gpu/drm/nouveau/nouveau_backlight.c   |  5 +-
>>>>  include/drm/drm_dp_helper.h                   |  7 +-
>>>>  4 files changed, 122 insertions(+), 45 deletions(-)
>>>>
>>>
>>