[PATCH] drm/i915: Use intel_panel_actually_set_backlight() to disable PWM backlight

[PATCH] drm/i915: Use intel_panel_actually_set_backlight() to disable PWM backlight

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Use intel_panel_actually_set_backlight() instead of a direct
call to pwm_config() in pwm_disable_backlight().

The main benefit is consistent debug logging when we turn off the
backlight. Currently we see nothing in dmesg which made me wonder
whether the backlight was even getting turned off properly.

The second benefit is consistency; This is what we do for all
the other backlight implementations.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_panel.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
index c860116669fa..b74f4bc7143c 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -767,7 +767,7 @@ static void pwm_disable_backlight(const struct drm_connector_state *old_conn_sta
 	struct intel_panel *panel = &connector->panel;
 
 	/* Disable the backlight */
-	pwm_config(panel->backlight.pwm, 0, CRC_PMIC_PWM_PERIOD_NS);
+	intel_panel_actually_set_backlight(old_conn_state, 0);
 	usleep_range(2000, 3000);
 	pwm_disable(panel->backlight.pwm);
 }
-- 
2.18.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Use intel_panel_actually_set_backlight() to disable PWM backlight

[Chris Wilson]

Quoting Ville Syrjala (2018-10-24 16:52:08)
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Use intel_panel_actually_set_backlight() instead of a direct
> call to pwm_config() in pwm_disable_backlight().
> 
> The main benefit is consistent debug logging when we turn off the
> backlight. Currently we see nothing in dmesg which made me wonder
> whether the backlight was even getting turned off properly.
> 
> The second benefit is consistency; This is what we do for all
> the other backlight implementations.

It will also have the effect of calling
intel_panel_compute_brightness(0) which one presumes is desired?

Just worrying if the inverted brightness quirk is ever used with pwm.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Use intel_panel_actually_set_backlight() to disable PWM backlight

[Ville Syrjälä]

On Wed, Oct 24, 2018 at 05:02:18PM +0100, Chris Wilson wrote:
> Quoting Ville Syrjala (2018-10-24 16:52:08)
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Use intel_panel_actually_set_backlight() instead of a direct
> > call to pwm_config() in pwm_disable_backlight().
> > 
> > The main benefit is consistent debug logging when we turn off the
> > backlight. Currently we see nothing in dmesg which made me wonder
> > whether the backlight was even getting turned off properly.
> > 
> > The second benefit is consistency; This is what we do for all
> > the other backlight implementations.
> 
> It will also have the effect of calling
> intel_panel_compute_brightness(0) which one presumes is desired?

We do it for everything else so it must be good?

> 
> Just worrying if the inverted brightness quirk is ever used with pwm.

If we have to invert for normal operation I don't know why
we wouldn't want to invert when shutting down the backlight.

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Use intel_panel_actually_set_backlight() to disable PWM backlight

[Chris Wilson]

Quoting Ville Syrjälä (2018-10-24 17:33:17)
> On Wed, Oct 24, 2018 at 05:02:18PM +0100, Chris Wilson wrote:
> > Quoting Ville Syrjala (2018-10-24 16:52:08)
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > Use intel_panel_actually_set_backlight() instead of a direct
> > > call to pwm_config() in pwm_disable_backlight().
> > > 
> > > The main benefit is consistent debug logging when we turn off the
> > > backlight. Currently we see nothing in dmesg which made me wonder
> > > whether the backlight was even getting turned off properly.
> > > 
> > > The second benefit is consistency; This is what we do for all
> > > the other backlight implementations.
> > 
> > It will also have the effect of calling
> > intel_panel_compute_brightness(0) which one presumes is desired?
> 
> We do it for everything else so it must be good?
> 
> > 
> > Just worrying if the inverted brightness quirk is ever used with pwm.
> 
> If we have to invert for normal operation I don't know why
> we wouldn't want to invert when shutting down the backlight.

Neither do I, just seems weird to set pwm to full to turn it off.

Whatever, the disparity is silly,
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Use intel_panel_actually_set_backlight() to disable PWM backlight

[Ville Syrjälä]

On Wed, Oct 24, 2018 at 05:35:51PM +0100, Chris Wilson wrote:
> Quoting Ville Syrjälä (2018-10-24 17:33:17)
> > On Wed, Oct 24, 2018 at 05:02:18PM +0100, Chris Wilson wrote:
> > > Quoting Ville Syrjala (2018-10-24 16:52:08)
> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > 
> > > > Use intel_panel_actually_set_backlight() instead of a direct
> > > > call to pwm_config() in pwm_disable_backlight().
> > > > 
> > > > The main benefit is consistent debug logging when we turn off the
> > > > backlight. Currently we see nothing in dmesg which made me wonder
> > > > whether the backlight was even getting turned off properly.
> > > > 
> > > > The second benefit is consistency; This is what we do for all
> > > > the other backlight implementations.
> > > 
> > > It will also have the effect of calling
> > > intel_panel_compute_brightness(0) which one presumes is desired?
> > 
> > We do it for everything else so it must be good?
> > 
> > > 
> > > Just worrying if the inverted brightness quirk is ever used with pwm.
> > 
> > If we have to invert for normal operation I don't know why
> > we wouldn't want to invert when shutting down the backlight.
> 
> Neither do I, just seems weird to set pwm to full to turn it off.

I guess it's just a matter of active low vs. active high. But yeah, not
sure we should be doing this at all when going through the pwm
subsystem. Looks like it already has something to handle the inverted
polarity.

> 
> Whatever, the disparity is silly,
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Ta.

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for drm/i915: Use intel_panel_actually_set_backlight() to disable PWM backlight

[Patchwork]

== Series Details ==

Series: drm/i915: Use intel_panel_actually_set_backlight() to disable PWM backlight
URL   : https://patchwork.freedesktop.org/series/51462/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_5029 -> Patchwork_10565 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/51462/revisions/1/mbox/

== Known issues ==

  Here are the changes found in Patchwork_10565 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@kms_flip@basic-flip-vs-dpms:
      fi-skl-6700hq:      PASS -> DMESG-WARN (fdo#105998)

    igt@kms_flip@basic-flip-vs-wf_vblank:
      fi-ilk-650:         PASS -> DMESG-WARN (fdo#106387)

    igt@kms_frontbuffer_tracking@basic:
      fi-hsw-peppy:       PASS -> DMESG-WARN (fdo#102614)

    
    ==== Possible fixes ====

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
      fi-blb-e6850:       INCOMPLETE (fdo#107718) -> PASS

    
  fdo#102614 https://bugs.freedesktop.org/show_bug.cgi?id=102614
  fdo#105998 https://bugs.freedesktop.org/show_bug.cgi?id=105998
  fdo#106387 https://bugs.freedesktop.org/show_bug.cgi?id=106387
  fdo#107718 https://bugs.freedesktop.org/show_bug.cgi?id=107718


== Participating hosts (47 -> 42) ==

  Missing    (5): fi-kbl-soraka fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 


== Build changes ==

    * Linux: CI_DRM_5029 -> Patchwork_10565

  CI_DRM_5029: 3d6302c20ddeff4b564c71fa479871b8afc11c4b @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4691: d445be01f5edc7e7a324444c73e221c9ed75602e @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10565: afd4f2c368b10bdeedd838f41bc8611d24bf11fc @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

afd4f2c368b1 drm/i915: Use intel_panel_actually_set_backlight() to disable PWM backlight

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_10565/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for drm/i915: Use intel_panel_actually_set_backlight() to disable PWM backlight

[Patchwork]

== Series Details ==

Series: drm/i915: Use intel_panel_actually_set_backlight() to disable PWM backlight
URL   : https://patchwork.freedesktop.org/series/51462/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_5029_full -> Patchwork_10565_full =

== Summary - SUCCESS ==

  No regressions found.

  

== Known issues ==

  Here are the changes found in Patchwork_10565_full that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@debugfs_test@read_all_entries_display_off:
      shard-skl:          NOTRUN -> INCOMPLETE (fdo#104108)

    igt@drv_suspend@debugfs-reader:
      shard-skl:          PASS -> INCOMPLETE (fdo#104108)

    igt@gem_exec_await@wide-contexts:
      shard-apl:          PASS -> FAIL (fdo#106680)

    igt@kms_busy@extended-modeset-hang-newfb-render-a:
      shard-skl:          NOTRUN -> DMESG-WARN (fdo#107956) +2

    igt@kms_ccs@pipe-b-crc-sprite-planes-basic:
      shard-glk:          PASS -> FAIL (fdo#108145)

    igt@kms_chv_cursor_fail@pipe-a-128x128-bottom-edge:
      shard-skl:          NOTRUN -> FAIL (fdo#104671)

    igt@kms_cursor_crc@cursor-64x64-sliding:
      shard-skl:          NOTRUN -> FAIL (fdo#103232)

    igt@kms_draw_crc@draw-method-xrgb2101010-mmap-cpu-xtiled:
      shard-skl:          NOTRUN -> FAIL (fdo#103184)

    igt@kms_draw_crc@draw-method-xrgb8888-mmap-cpu-xtiled:
      shard-glk:          PASS -> FAIL (fdo#107791)

    igt@kms_fbcon_fbt@fbc-suspend:
      shard-skl:          NOTRUN -> FAIL (fdo#103833, fdo#105682)

    igt@kms_flip@flip-vs-expired-vblank-interruptible:
      shard-kbl:          PASS -> FAIL (fdo#102887, fdo#105363)

    igt@kms_frontbuffer_tracking@fbc-1p-shrfb-fliptrack:
      shard-skl:          NOTRUN -> FAIL (fdo#103167) +2

    igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-shrfb-msflip-blt:
      shard-skl:          NOTRUN -> FAIL (fdo#105682) +1

    igt@kms_mmap_write_crc:
      shard-skl:          NOTRUN -> FAIL (fdo#108228)

    igt@kms_plane@plane-position-covered-pipe-a-planes:
      shard-glk:          PASS -> FAIL (fdo#103166)

    igt@kms_plane_alpha_blend@pipe-a-alpha-basic:
      shard-skl:          NOTRUN -> FAIL (fdo#107815, fdo#108145)

    igt@kms_plane_alpha_blend@pipe-b-alpha-opaque-fb:
      shard-skl:          NOTRUN -> FAIL (fdo#108145) +1

    igt@kms_plane_alpha_blend@pipe-c-coverage-7efc:
      shard-skl:          NOTRUN -> FAIL (fdo#108146)

    
    ==== Possible fixes ====

    igt@gem_ctx_switch@basic-all-light:
      shard-apl:          INCOMPLETE (fdo#103927) -> PASS

    igt@gem_eio@in-flight-contexts-immediate:
      shard-glk:          FAIL (fdo#107799) -> PASS

    igt@kms_cursor_crc@cursor-128x128-onscreen:
      shard-apl:          FAIL (fdo#103232) -> PASS

    igt@kms_cursor_crc@cursor-64x64-onscreen:
      shard-glk:          FAIL (fdo#103232) -> PASS

    igt@kms_cursor_crc@cursor-64x64-suspend:
      shard-skl:          INCOMPLETE (fdo#104108) -> PASS

    igt@kms_flip@flip-vs-expired-vblank:
      shard-skl:          FAIL (fdo#105363) -> PASS

    igt@kms_flip@wf_vblank-ts-check:
      shard-apl:          DMESG-WARN (fdo#105602, fdo#103558) -> PASS +4

    igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-blt:
      shard-glk:          FAIL (fdo#103167) -> PASS

    igt@kms_frontbuffer_tracking@fbc-1p-rte:
      shard-apl:          DMESG-WARN (fdo#108131, fdo#103558) -> PASS

    igt@kms_setmode@basic:
      shard-kbl:          FAIL (fdo#99912) -> PASS

    igt@pm_rpm@system-suspend-execbuf:
      shard-apl:          DMESG-WARN -> PASS

    
    ==== Warnings ====

    igt@kms_frontbuffer_tracking@fbc-1p-offscren-pri-shrfb-draw-mmap-gtt:
      shard-snb:          DMESG-WARN (fdo#107469) -> INCOMPLETE (fdo#105411)

    
  fdo#102887 https://bugs.freedesktop.org/show_bug.cgi?id=102887
  fdo#103166 https://bugs.freedesktop.org/show_bug.cgi?id=103166
  fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
  fdo#103184 https://bugs.freedesktop.org/show_bug.cgi?id=103184
  fdo#103232 https://bugs.freedesktop.org/show_bug.cgi?id=103232
  fdo#103558 https://bugs.freedesktop.org/show_bug.cgi?id=103558
  fdo#103833 https://bugs.freedesktop.org/show_bug.cgi?id=103833
  fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927
  fdo#104108 https://bugs.freedesktop.org/show_bug.cgi?id=104108
  fdo#104671 https://bugs.freedesktop.org/show_bug.cgi?id=104671
  fdo#105363 https://bugs.freedesktop.org/show_bug.cgi?id=105363
  fdo#105411 https://bugs.freedesktop.org/show_bug.cgi?id=105411
  fdo#105602 https://bugs.freedesktop.org/show_bug.cgi?id=105602
  fdo#105682 https://bugs.freedesktop.org/show_bug.cgi?id=105682
  fdo#106680 https://bugs.freedesktop.org/show_bug.cgi?id=106680
  fdo#107469 https://bugs.freedesktop.org/show_bug.cgi?id=107469
  fdo#107791 https://bugs.freedesktop.org/show_bug.cgi?id=107791
  fdo#107799 https://bugs.freedesktop.org/show_bug.cgi?id=107799
  fdo#107815 https://bugs.freedesktop.org/show_bug.cgi?id=107815
  fdo#107956 https://bugs.freedesktop.org/show_bug.cgi?id=107956
  fdo#108131 https://bugs.freedesktop.org/show_bug.cgi?id=108131
  fdo#108145 https://bugs.freedesktop.org/show_bug.cgi?id=108145
  fdo#108146 https://bugs.freedesktop.org/show_bug.cgi?id=108146
  fdo#108228 https://bugs.freedesktop.org/show_bug.cgi?id=108228
  fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912


== Participating hosts (6 -> 6) ==

  No changes in participating hosts


== Build changes ==

    * Linux: CI_DRM_5029 -> Patchwork_10565

  CI_DRM_5029: 3d6302c20ddeff4b564c71fa479871b8afc11c4b @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4691: d445be01f5edc7e7a324444c73e221c9ed75602e @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10565: afd4f2c368b10bdeedd838f41bc8611d24bf11fc @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_10565/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Use intel_panel_actually_set_backlight() to disable PWM backlight

[Jani Nikula]

On Wed, 24 Oct 2018, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Wed, Oct 24, 2018 at 05:35:51PM +0100, Chris Wilson wrote:
>> Quoting Ville Syrjälä (2018-10-24 17:33:17)
>> > On Wed, Oct 24, 2018 at 05:02:18PM +0100, Chris Wilson wrote:
>> > > Quoting Ville Syrjala (2018-10-24 16:52:08)
>> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> > > > 
>> > > > Use intel_panel_actually_set_backlight() instead of a direct
>> > > > call to pwm_config() in pwm_disable_backlight().
>> > > > 
>> > > > The main benefit is consistent debug logging when we turn off the
>> > > > backlight. Currently we see nothing in dmesg which made me wonder
>> > > > whether the backlight was even getting turned off properly.
>> > > > 
>> > > > The second benefit is consistency; This is what we do for all
>> > > > the other backlight implementations.
>> > > 
>> > > It will also have the effect of calling
>> > > intel_panel_compute_brightness(0) which one presumes is desired?
>> > 
>> > We do it for everything else so it must be good?
>> > 
>> > > 
>> > > Just worrying if the inverted brightness quirk is ever used with pwm.
>> > 
>> > If we have to invert for normal operation I don't know why
>> > we wouldn't want to invert when shutting down the backlight.
>> 
>> Neither do I, just seems weird to set pwm to full to turn it off.
>
> I guess it's just a matter of active low vs. active high. But yeah, not
> sure we should be doing this at all when going through the pwm
> subsystem. Looks like it already has something to handle the inverted
> polarity.

I haven't seen a valid use of invert brightness outside of PCI id 0x2a42
and haven't seen a bug report about it in years. I think otherwise we
should be covered with the proper use of the polarity bit.

I wouldn't oppose a patch warning about invert brightness use outside of
gen4 or even more specific.

On the patch at hand,

Acked-by: Jani Nikula <jani.nikula@intel.com>


BR,
Jani.


>
>> 
>> Whatever, the disparity is silly,
>> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
>
> Ta.

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx