mbox series

[v7,0/3] CRTC background color

Message ID 20190930224707.14904-1-matthew.d.roper@intel.com (mailing list archive)
Headers show
Series CRTC background color | expand

Message

Matt Roper Sept. 30, 2019, 10:47 p.m. UTC
The previous version of this series was posted in February here:
        https://lists.freedesktop.org/archives/dri-devel/2019-February/208068.html

Right before we merged this in February Maarten noticed that we should
be setting up the initial property state in a CRTC reset function (which
didn't exist yet) instead of when the property was attached.  Maarten
landed the CRTC reset functionality a week or two later, but I was busy
with travel and other work at the time, so revisiting and rebasing this
background color series fell through the cracks and I'm just getting
back to it now.

Userspace consumer is chromeos; these are the links the ChromeOS folks
gave me back in February:
  https://chromium-review.googlesource.com/c/chromium/src/+/1278858
  https://chromium-review.googlesource.com/c/chromiumos/platform/drm-tests/+/1241436

IGT is still the same as posted in February:
  https://lists.freedesktop.org/archives/igt-dev/2019-February/009637.html

Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Matt Roper (3):
  drm: Add CRTC background color property
  drm/i915/gen9+: Add support for pipe background color
  drm/i915: Add background color hardware readout and state check

 drivers/gpu/drm/drm_atomic_state_helper.c    |  4 +-
 drivers/gpu/drm/drm_atomic_uapi.c            |  4 ++
 drivers/gpu/drm/drm_blend.c                  | 35 +++++++++++++--
 drivers/gpu/drm/drm_mode_config.c            |  6 +++
 drivers/gpu/drm/i915/display/intel_color.c   | 11 +++--
 drivers/gpu/drm/i915/display/intel_display.c | 45 ++++++++++++++++++++
 drivers/gpu/drm/i915/i915_debugfs.c          |  9 ++++
 include/drm/drm_blend.h                      |  1 +
 include/drm/drm_crtc.h                       | 12 ++++++
 include/drm/drm_mode_config.h                |  5 +++
 include/uapi/drm/drm_mode.h                  | 28 ++++++++++++
 11 files changed, 153 insertions(+), 7 deletions(-)

Comments

Daniele Castagna Oct. 9, 2019, 9:01 p.m. UTC | #1
On Wed, Oct 9, 2019 at 1:34 PM Matt Roper <matthew.d.roper@intel.com> wrote:
>
> The previous version of this series was posted in February here:
>         https://lists.freedesktop.org/archives/dri-devel/2019-February/208068.html
>
> Right before we merged this in February Maarten noticed that we should
> be setting up the initial property state in a CRTC reset function (which
> didn't exist yet) instead of when the property was attached.  Maarten
> landed the CRTC reset functionality a week or two later, but I was busy
> with travel and other work at the time, so revisiting and rebasing this
> background color series fell through the cracks and I'm just getting
> back to it now.
>
> Userspace consumer is chromeos; these are the links the ChromeOS folks
> gave me back in February:
>   https://chromium-review.googlesource.com/c/chromium/src/+/1278858
>   https://chromium-review.googlesource.com/c/chromiumos/platform/drm-tests/+/1241436

Please note that the current state of the background color on Chrome
OS side is that while the property plumbing landed, the property is
currently not used by the compositor and no one is working on making
that happen.
The kernel patches have not landed on the ChromeOS kernel either.


>
>
> IGT is still the same as posted in February:
>   https://lists.freedesktop.org/archives/igt-dev/2019-February/009637.html
>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>
> Matt Roper (3):
>   drm: Add CRTC background color property
>   drm/i915/gen9+: Add support for pipe background color
>   drm/i915: Add background color hardware readout and state check
>
>  drivers/gpu/drm/drm_atomic_state_helper.c    |  4 +-
>  drivers/gpu/drm/drm_atomic_uapi.c            |  4 ++
>  drivers/gpu/drm/drm_blend.c                  | 35 +++++++++++++--
>  drivers/gpu/drm/drm_mode_config.c            |  6 +++
>  drivers/gpu/drm/i915/display/intel_color.c   | 11 +++--
>  drivers/gpu/drm/i915/display/intel_display.c | 45 ++++++++++++++++++++
>  drivers/gpu/drm/i915/i915_debugfs.c          |  9 ++++
>  include/drm/drm_blend.h                      |  1 +
>  include/drm/drm_crtc.h                       | 12 ++++++
>  include/drm/drm_mode_config.h                |  5 +++
>  include/uapi/drm/drm_mode.h                  | 28 ++++++++++++
>  11 files changed, 153 insertions(+), 7 deletions(-)
>
> --
> 2.21.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Matt Roper Oct. 9, 2019, 9:27 p.m. UTC | #2
On Wed, Oct 09, 2019 at 05:01:20PM -0400, Daniele Castagna wrote:
> On Wed, Oct 9, 2019 at 1:34 PM Matt Roper <matthew.d.roper@intel.com> wrote:
> >
> > The previous version of this series was posted in February here:
> >         https://lists.freedesktop.org/archives/dri-devel/2019-February/208068.html
> >
> > Right before we merged this in February Maarten noticed that we should
> > be setting up the initial property state in a CRTC reset function (which
> > didn't exist yet) instead of when the property was attached.  Maarten
> > landed the CRTC reset functionality a week or two later, but I was busy
> > with travel and other work at the time, so revisiting and rebasing this
> > background color series fell through the cracks and I'm just getting
> > back to it now.
> >
> > Userspace consumer is chromeos; these are the links the ChromeOS folks
> > gave me back in February:
> >   https://chromium-review.googlesource.com/c/chromium/src/+/1278858
> >   https://chromium-review.googlesource.com/c/chromiumos/platform/drm-tests/+/1241436
> 
> Please note that the current state of the background color on Chrome
> OS side is that while the property plumbing landed, the property is
> currently not used by the compositor and no one is working on making
> that happen.

Hmm, in that case it sounds like we probably don't actually have enough
userspace support yet to justify merging the kernel changes at this
time.  I'm not too familiar with Chrome OS' userspace stack; is the rest
of the work to actually make use of ozone stuff in the links above a
heavy lift?  Is it something someone is likely to work on that once
they're freed up from other tasks or is there just not enough benefit to
justify the effort of utilizing it at the compositor level right now?


Matt

> The kernel patches have not landed on the ChromeOS kernel either.
> 
> 
> >
> >
> > IGT is still the same as posted in February:
> >   https://lists.freedesktop.org/archives/igt-dev/2019-February/009637.html
> >
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >
> > Matt Roper (3):
> >   drm: Add CRTC background color property
> >   drm/i915/gen9+: Add support for pipe background color
> >   drm/i915: Add background color hardware readout and state check
> >
> >  drivers/gpu/drm/drm_atomic_state_helper.c    |  4 +-
> >  drivers/gpu/drm/drm_atomic_uapi.c            |  4 ++
> >  drivers/gpu/drm/drm_blend.c                  | 35 +++++++++++++--
> >  drivers/gpu/drm/drm_mode_config.c            |  6 +++
> >  drivers/gpu/drm/i915/display/intel_color.c   | 11 +++--
> >  drivers/gpu/drm/i915/display/intel_display.c | 45 ++++++++++++++++++++
> >  drivers/gpu/drm/i915/i915_debugfs.c          |  9 ++++
> >  include/drm/drm_blend.h                      |  1 +
> >  include/drm/drm_crtc.h                       | 12 ++++++
> >  include/drm/drm_mode_config.h                |  5 +++
> >  include/uapi/drm/drm_mode.h                  | 28 ++++++++++++
> >  11 files changed, 153 insertions(+), 7 deletions(-)
> >
> > --
> > 2.21.0
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Sean Paul Oct. 11, 2019, 4:09 p.m. UTC | #3
On Wed, Oct 09, 2019 at 02:27:41PM -0700, Matt Roper wrote:
> On Wed, Oct 09, 2019 at 05:01:20PM -0400, Daniele Castagna wrote:
> > On Wed, Oct 9, 2019 at 1:34 PM Matt Roper <matthew.d.roper@intel.com> wrote:
> > >
> > > The previous version of this series was posted in February here:
> > >         https://lists.freedesktop.org/archives/dri-devel/2019-February/208068.html
> > >
> > > Right before we merged this in February Maarten noticed that we should
> > > be setting up the initial property state in a CRTC reset function (which
> > > didn't exist yet) instead of when the property was attached.  Maarten
> > > landed the CRTC reset functionality a week or two later, but I was busy
> > > with travel and other work at the time, so revisiting and rebasing this
> > > background color series fell through the cracks and I'm just getting
> > > back to it now.
> > >
> > > Userspace consumer is chromeos; these are the links the ChromeOS folks
> > > gave me back in February:
> > >   https://chromium-review.googlesource.com/c/chromium/src/+/1278858
> > >   https://chromium-review.googlesource.com/c/chromiumos/platform/drm-tests/+/1241436
> > 
> > Please note that the current state of the background color on Chrome
> > OS side is that while the property plumbing landed, the property is
> > currently not used by the compositor and no one is working on making
> > that happen.
> 
> Hmm, in that case it sounds like we probably don't actually have enough
> userspace support yet to justify merging the kernel changes at this
> time.  I'm not too familiar with Chrome OS' userspace stack; is the rest
> of the work to actually make use of ozone stuff in the links above a
> heavy lift?  Is it something someone is likely to work on that once
> they're freed up from other tasks or is there just not enough benefit to
> justify the effort of utilizing it at the compositor level right now?
> 

AFAIK, there are no plans for the Chrome team to spend time utilising this
feature. If you look at the bug [1], there's no correspondance with CrOS and
there is no clear usecase for the feature. The patches are basically just
copy/paste of how other properties are surfaced and that's it.

A few months later, we get more stub implementations [2]/[3] again with no
feedback from the Chrome team on the bugs [4]/[5]. On the first review, Daniele
asked if the submitter was going to finish the background work before adding
more properties. The answer is that CRTC BG isn't seen on Chrome, so it's not
useful on the platform.

We should not have landed the crtc-bg stub in the first place, it's just dead
code and it's clear from the comments in [2] that it's going to stay that way.

So I think the best course of action is to revert this change from Chrome until
we have a usecase for the feature which is blessed by the Chrome team and it is
implemented fully.

Going forward, we're going to keep a closer eye on the HW enablement in Chrome
so as to avoid adding dead code to Chrome and to avoid skirting the spirit of
the "opensource userspace" rule by just implementing getters/setters.

Sean

ps:
As for the stubs referenced in [2] and [3], that's more of a Chrome matter.
There are already other existing userspace implementations for these 2 features,
so Chrome is not being used as an upstreaming vehicle for the kernel patches.


[1] https://bugs.chromium.org/p/chromium/issues/detail?id=894259
[2] https://polymer2-chromium-review.googlesource.com/c/chromium/src/+/1504300
[3] https://polymer2-chromium-review.googlesource.com/c/chromium/src/+/1572247
[4] https://bugs.chromium.org/p/chromium/issues/detail?id=940683
[5] https://bugs.chromium.org/p/chromium/issues/detail?id=938536

> 
> Matt
> 
> > The kernel patches have not landed on the ChromeOS kernel either.
> > 
> > 
> > >
> > >
> > > IGT is still the same as posted in February:
> > >   https://lists.freedesktop.org/archives/igt-dev/2019-February/009637.html
> > >
> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > >
> > > Matt Roper (3):
> > >   drm: Add CRTC background color property
> > >   drm/i915/gen9+: Add support for pipe background color
> > >   drm/i915: Add background color hardware readout and state check
> > >
> > >  drivers/gpu/drm/drm_atomic_state_helper.c    |  4 +-
> > >  drivers/gpu/drm/drm_atomic_uapi.c            |  4 ++
> > >  drivers/gpu/drm/drm_blend.c                  | 35 +++++++++++++--
> > >  drivers/gpu/drm/drm_mode_config.c            |  6 +++
> > >  drivers/gpu/drm/i915/display/intel_color.c   | 11 +++--
> > >  drivers/gpu/drm/i915/display/intel_display.c | 45 ++++++++++++++++++++
> > >  drivers/gpu/drm/i915/i915_debugfs.c          |  9 ++++
> > >  include/drm/drm_blend.h                      |  1 +
> > >  include/drm/drm_crtc.h                       | 12 ++++++
> > >  include/drm/drm_mode_config.h                |  5 +++
> > >  include/uapi/drm/drm_mode.h                  | 28 ++++++++++++
> > >  11 files changed, 153 insertions(+), 7 deletions(-)
> > >
> > > --
> > > 2.21.0
> > >
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Matt Roper
> Graphics Software Engineer
> VTT-OSGC Platform Enablement
> Intel Corporation
> (916) 356-2795
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx