Re: [PATCH v9 30/52] drm-dyndbg: adapt drm core to use dyndbg classmaps-v2
On Tue, Jul 02, 2024 at 08:34:39PM -0600, jim.cro...@gmail.com wrote: > On Tue, Jul 2, 2024 at 5:33 PM Ville Syrjälä > wrote: > > > > On Tue, Jul 02, 2024 at 03:57:20PM -0600, Jim Cromie wrote: > > > dyndbg's CLASSMAP-v1 api was broken; DECLARE_DYNDBG_CLASSMAP tried to > > > do too much. Its replaced by DRM_CLASSMAP_DEFINE, which creates & > > > EXPORTs the classmap when CONFIG_DRM_USE_DYNAMIC_DEBUG=y, for direct > > > reference by drivers. > > > > > > The drivers still use DECLARE_DYNDBG_CLASSMAP for now, so they still > > > redundantly re-declare the classmap, but we can convert the drivers > > > later to DYNDBG_CLASSMAP_USE > > > > > > Signed-off-by: Jim Cromie > > > --- > > > drivers/gpu/drm/drm_print.c | 25 + > > > include/drm/drm_print.h | 8 > > > 2 files changed, 21 insertions(+), 12 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c > > > index 699b7dbffd7b..4a5f2317229b 100644 > > > --- a/drivers/gpu/drm/drm_print.c > > > +++ b/drivers/gpu/drm/drm_print.c > > > @@ -55,18 +55,19 @@ MODULE_PARM_DESC(debug, "Enable debug output, where > > > each bit enables a debug cat > > > #if !defined(CONFIG_DRM_USE_DYNAMIC_DEBUG) > > > module_param_named(debug, __drm_debug, ulong, 0600); > > > #else > > > -/* classnames must match vals of enum drm_debug_category */ > > > -DECLARE_DYNDBG_CLASSMAP(drm_debug_classes, DD_CLASS_TYPE_DISJOINT_BITS, > > > 0, > > > - "DRM_UT_CORE", > > > - "DRM_UT_DRIVER", > > > - "DRM_UT_KMS", > > > - "DRM_UT_PRIME", > > > - "DRM_UT_ATOMIC", > > > - "DRM_UT_VBL", > > > - "DRM_UT_STATE", > > > - "DRM_UT_LEASE", > > > - "DRM_UT_DP", > > > - "DRM_UT_DRMRES"); > > > +/* classnames must match value-symbols of enum drm_debug_category */ > > > +DRM_CLASSMAP_DEFINE(drm_debug_classes, DD_CLASS_TYPE_DISJOINT_BITS, > > > + DRM_UT_CORE, > > > + "DRM_UT_CORE", > > > + "DRM_UT_DRIVER", > > > + "DRM_UT_KMS", > > > + "DRM_UT_PRIME", > > > + "DRM_UT_ATOMIC", > > > + "DRM_UT_VBL", > > > + "DRM_UT_STATE", > > > + "DRM_UT_LEASE", > > > + "DRM_UT_DP", > > > + "DRM_UT_DRMRES"); > > > > Looks like this stuff just ends up in an array, so presumably > > it should be possible to use designated initializers to make this > > less fragile? > > Im not sure I got your whole point, but: I mean using [DRM_UT_CORE] = "DRM_UT_CORE" instead of "DRM_UT_CORE" so there is no chance of screwing up the order. Or maybe the order doesn't even matter here? Could also stringify to avoid accidental of typos. > > the fragility is the repetitive re-statement of the map, > in those un-modified DECLARE_s, > once replaced, the USEs just ref the struct built by the _DEFINE > (once, and exported) > > I dont really like the _DEFINEs restatement of the enum-values: DRM_UT_* > especially as "strings". > I can automate the stringification with an APPLY_FN_(__stringify, ...) > but the enum-list DRM_UT_* (w.o quotes) is still needed as args. > > unless there is something C can do thats like Enum.values() ? > > > > > > > -- > > Ville Syrjälä > > Intel -- Ville Syrjälä Intel
Re: [PATCH v9 30/52] drm-dyndbg: adapt drm core to use dyndbg classmaps-v2
On Tue, Jul 02, 2024 at 03:57:20PM -0600, Jim Cromie wrote: > dyndbg's CLASSMAP-v1 api was broken; DECLARE_DYNDBG_CLASSMAP tried to > do too much. Its replaced by DRM_CLASSMAP_DEFINE, which creates & > EXPORTs the classmap when CONFIG_DRM_USE_DYNAMIC_DEBUG=y, for direct > reference by drivers. > > The drivers still use DECLARE_DYNDBG_CLASSMAP for now, so they still > redundantly re-declare the classmap, but we can convert the drivers > later to DYNDBG_CLASSMAP_USE > > Signed-off-by: Jim Cromie > --- > drivers/gpu/drm/drm_print.c | 25 + > include/drm/drm_print.h | 8 > 2 files changed, 21 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c > index 699b7dbffd7b..4a5f2317229b 100644 > --- a/drivers/gpu/drm/drm_print.c > +++ b/drivers/gpu/drm/drm_print.c > @@ -55,18 +55,19 @@ MODULE_PARM_DESC(debug, "Enable debug output, where each > bit enables a debug cat > #if !defined(CONFIG_DRM_USE_DYNAMIC_DEBUG) > module_param_named(debug, __drm_debug, ulong, 0600); > #else > -/* classnames must match vals of enum drm_debug_category */ > -DECLARE_DYNDBG_CLASSMAP(drm_debug_classes, DD_CLASS_TYPE_DISJOINT_BITS, 0, > - "DRM_UT_CORE", > - "DRM_UT_DRIVER", > - "DRM_UT_KMS", > - "DRM_UT_PRIME", > - "DRM_UT_ATOMIC", > - "DRM_UT_VBL", > - "DRM_UT_STATE", > - "DRM_UT_LEASE", > - "DRM_UT_DP", > - "DRM_UT_DRMRES"); > +/* classnames must match value-symbols of enum drm_debug_category */ > +DRM_CLASSMAP_DEFINE(drm_debug_classes, DD_CLASS_TYPE_DISJOINT_BITS, > + DRM_UT_CORE, > + "DRM_UT_CORE", > + "DRM_UT_DRIVER", > + "DRM_UT_KMS", > + "DRM_UT_PRIME", > + "DRM_UT_ATOMIC", > + "DRM_UT_VBL", > + "DRM_UT_STATE", > + "DRM_UT_LEASE", > + "DRM_UT_DP", > + "DRM_UT_DRMRES"); Looks like this stuff just ends up in an array, so presumably it should be possible to use designated initializers to make this less fragile? -- Ville Syrjälä Intel
Re: [PATCH v6 0/8] drm: Support per-plane async flip configuration
On Fri, Jun 14, 2024 at 04:37:41PM -0300, André Almeida wrote: > Hi Dmitry, > > Em 14/06/2024 14:32, Dmitry Baryshkov escreveu: > > On Fri, Jun 14, 2024 at 12:35:27PM GMT, André Almeida wrote: > >> AMD hardware can do async flips with overlay planes, but currently there's > >> no > >> easy way to enable that in DRM. To solve that, this patchset creates a new > >> drm_plane field, bool async_flip, that allows drivers to choose which > >> plane can > >> or cannot do async flips. This is latter used on drm_atomic_set_property > >> when > >> users want to do async flips. > >> > >> Patch 1 allows async commits with IN_FENCE_ID in any driver. > >> > >> Patches 2 to 7 have no function change. As per current code, every driver > >> that > >> allows async page flips using the atomic API, allows doing it only in the > >> primary plane. Those patches then enable it for every driver. > >> > >> Patch 8 finally enables async flip on overlay planes for amdgpu. > >> > >> Changes from v5: > >> - Instead of enabling plane->async_flip in the common code, move it to > >> driver > >> code. > >> - Enable primary plane async flip on every driver > >> https://lore.kernel.org/dri-devel/20240612193713.167448-1-andrealm...@igalia.com/ > >> > >> André Almeida (8): > >>drm/atomic: Allow userspace to use explicit sync with atomic async > >> flips > >>drm: Support per-plane async flip configuration > >>drm/amdgpu: Enable async flips on the primary plane > >>drm: atmel-hlcdc: Enable async flips on the primary plane > >>drm/i915: Enable async flips on the primary plane > >>drm/nouveau: Enable async flips on the primary plane > >>drm/vc4: Enable async flips on the primary plane > >>drm/amdgpu: Make it possible to async flip overlay planes > >> > >> drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c | 2 ++ > >> drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c | 3 +++ > >> drivers/gpu/drm/drm_atomic_uapi.c | 8 +--- > >> drivers/gpu/drm/i915/display/i9xx_plane.c | 3 +++ > >> drivers/gpu/drm/nouveau/dispnv04/crtc.c | 4 > >> drivers/gpu/drm/nouveau/dispnv50/wndw.c | 4 > >> drivers/gpu/drm/vc4/vc4_plane.c | 4 +++- > > > > The main question is why only these drivers were updated. > > > > According to `git grep async_page_flip`, only those drivers supports > async page flip. The only corner case is radeon, that does supports > async but doesn't support planes. The primary plane will alwyas exist (drm_crtc_init() will create one for the old drivers that don't do it explicitly). So you should be able to convert radeon as well. And looks like some pre-dc amdgpu stuff is in a similar situation. That should presumably allow the old flag to be removed entirely? Hmm, I suppose drm_getcap() would need a bit of work to eg. go through all the planes to see if any of them support async flips. > > Do you know any other driver that should be updated to? > > >> include/drm/drm_plane.h | 5 + > >> 8 files changed, 29 insertions(+), 4 deletions(-) > >> > >> -- > >> 2.45.2 > >> > > -- Ville Syrjälä Intel
Re: [PATCH v5 2/3] drm: Allow drivers to choose plane types to async flip
On Wed, Jun 12, 2024 at 04:37:12PM -0300, André Almeida wrote:
> Different planes may have different capabilities of doing async flips,
> so create a field to let drivers allow async flip per plane type.
>
> Signed-off-by: André Almeida
> ---
> drivers/gpu/drm/drm_atomic_uapi.c | 4 ++--
> drivers/gpu/drm/drm_plane.c | 3 +++
> include/drm/drm_plane.h | 5 +
> 3 files changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
> b/drivers/gpu/drm/drm_atomic_uapi.c
> index 2e1d9391febe..dd4b1578f141 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -1079,9 +1079,9 @@ int drm_atomic_set_property(struct drm_atomic_state
> *state,
> break;
> }
>
> - if (async_flip && plane_state->plane->type !=
> DRM_PLANE_TYPE_PRIMARY) {
> + if (async_flip && !plane_state->plane->async_flip) {
You alreayd have 'plane', no need to dog it out again.
> drm_dbg_atomic(prop->dev,
> -"[OBJECT:%d] Only primary planes can be
> changed during async flip\n",
> +"[OBJECT:%d] This type of plane cannot
> be changed during async flip\n",
> obj->id);
"[PLANE:%d:%s] does not support async flips"
or something like it would make more sense to me.
> ret = -EINVAL;
> break;
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index 57662a1fd345..bbcec3940636 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -385,6 +385,9 @@ static int __drm_universal_plane_init(struct drm_device
> *dev,
>
> drm_modeset_lock_init(&plane->mutex);
>
> + if (type == DRM_PLANE_TYPE_PRIMARY)
> + plane->async_flip = true;
Setting that would be the job of the driver.
You could probably just nuke mode_config.async_page_flip
and replace it fully with plane.async_flip checks.
> +
> plane->base.properties = &plane->properties;
> plane->dev = dev;
> plane->funcs = funcs;
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index 9507542121fa..0bebc72af5c3 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -786,6 +786,11 @@ struct drm_plane {
>* @kmsg_panic: Used to register a panic notifier for this plane
> */
> struct kmsg_dumper kmsg_panic;
> +
> + /**
> + * @async_flip: indicates if a plane can do async flips
> + */
> + bool async_flip;
> };
>
> #define obj_to_plane(x) container_of(x, struct drm_plane, base)
> --
> 2.45.2
--
Ville Syrjälä
Intel
Re: [PATCH v2] drm/client: Detect when ACPI lid is closed during initialization
On Thu, Jun 06, 2024 at 10:21:07AM +0300, Jani Nikula wrote:
> On Wed, 05 Jun 2024, Chris Bainbridge wrote:
> > On Tue, Jun 04, 2024 at 10:02:29AM +0800, kernel test robot wrote:
> >> Hi Mario,
> >>
> >> kernel test robot noticed the following build errors:
> >>
> >> [auto build test ERROR on drm-misc/drm-misc-next]
> >> [also build test ERROR on drm/drm-next drm-exynos/exynos-drm-next
> >> drm-intel/for-linux-next drm-intel/for-linux-next-fixes drm-tip/drm-tip
> >> linus/master v6.10-rc2 next-20240603]
> >> [If your patch is applied to the wrong git tree, kindly drop us a note.
> >> And when submitting patch, we suggest to use '--base' as documented in
> >> https://git-scm.com/docs/git-format-patch#_base_tree_information]
> >>
> >> url:
> >> https://github.com/intel-lab-lkp/linux/commits/Mario-Limonciello/drm-client-Detect-when-ACPI-lid-is-closed-during-initialization/20240529-050440
> >> base: git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
> >> patch link:
> >> https://lore.kernel.org/r/20240528210319.1242-1-mario.limonciello%40amd.com
> >> patch subject: [PATCH v2] drm/client: Detect when ACPI lid is closed
> >> during initialization
> >> config: i386-randconfig-053-20240604
> >> (https://download.01.org/0day-ci/archive/20240604/202406040928.eu1griwv-...@intel.com/config)
> >> compiler: gcc-9 (Ubuntu 9.5.0-4ubuntu2) 9.5.0
> >> reproduce (this is a W=1 build):
> >> (https://download.01.org/0day-ci/archive/20240604/202406040928.eu1griwv-...@intel.com/reproduce)
> >>
> >> If you fix the issue in a separate patch/commit (i.e. not just a new
> >> version of
> >> the same patch/commit), kindly add following tags
> >> | Reported-by: kernel test robot
> >> | Closes:
> >> https://lore.kernel.org/oe-kbuild-all/202406040928.eu1griwv-...@intel.com/
> >>
> >> All errors (new ones prefixed by >>):
> >>
> >>ld: drivers/gpu/drm/drm_client_modeset.o: in function
> >> `drm_client_match_edp_lid':
> >> >> drivers/gpu/drm/drm_client_modeset.c:281:(.text+0x221b): undefined
> >> >> reference to `acpi_lid_open'
> >>
> >>
> >> vim +281 drivers/gpu/drm/drm_client_modeset.c
> >>
> >>260
> >>261 static void drm_client_match_edp_lid(struct drm_device *dev,
> >>262 struct drm_connector
> >> **connectors,
> >>263 unsigned int
> >> connector_count,
> >>264 bool *enabled)
> >>265 {
> >>266 int i;
> >>267
> >>268 for (i = 0; i < connector_count; i++) {
> >>269 struct drm_connector *connector = connectors[i];
> >>270
> >>271 switch (connector->connector_type) {
> >>272 case DRM_MODE_CONNECTOR_LVDS:
> >>273 case DRM_MODE_CONNECTOR_eDP:
> >>274 if (!enabled[i])
> >>275 continue;
> >>276 break;
> >>277 default:
> >>278 continue;
> >>279 }
> >>280
> >> > 281 if (!acpi_lid_open()) {
> >>282 drm_dbg_kms(dev, "[CONNECTOR:%d:%s] lid
> >> is closed, disabling\n",
> >>283 connector->base.id,
> >> connector->name);
> >>284 enabled[i] = false;
> >>285 }
> >>286 }
> >>287 }
> >>288
> >>
> >> --
> >> 0-DAY CI Kernel Test Service
> >> https://github.com/intel/lkp-tests/wiki
> >
> > The failed config has CONFIG_ACPI_BUTTON=m. The build failure can be
> > fixed with:
> >
> > diff --git a/drivers/gpu/drm/drm_client_modeset.c
> > b/drivers/gpu/drm/drm_client_modeset.c
> > index b76438c31761..0271e66f44f8 100644
> > --- a/drivers/gpu/drm/drm_client_modeset.c
> > +++ b/drivers/gpu/drm/drm_client_modeset.c
> > @@ -271,11 +271,13 @@ static void drm_client_match_edp_lid(struct
> > drm_device *dev,
> > if (connector->connector_type != DRM_MODE_CONNECTOR_eDP ||
> > !enabled[i])
> > continue;
> >
> > +#if defined(CONFIG_ACPI_BUTTON)
> > if (!acpi_lid_open()) {
> > drm_dbg_kms(dev, "[CONNECTOR:%d:%s] lid is closed,
> > disabling\n",
> > connector->base.id, connector->name);
> > enabled[i] = false;
> > }
> > +#endif
> > }
> > }
>
> No. This is because
>
> CONFIG_DRM=y
> CONFIG_ACPI_BUTTON=m
>
> The pedantically correct fix is probably having DRM
>
> depends on ACPI_BUTTON || ACPI_BUTTON=n
>
> but seeing how i915 and xe just
>
> select ACPI_BUTTON if ACPI
Huh. We should nuke that as we haven't used this lid stuff in ages.
--
Ville Syrjälä
Intel
Re: [PATCH v2] drm/client: Detect when ACPI lid is closed during initialization
On Wed, May 29, 2024 at 09:45:55AM -0500, Mario Limonciello wrote:
> On 5/29/2024 09:14, Ville Syrjälä wrote:
> > On Tue, May 28, 2024 at 04:03:19PM -0500, Mario Limonciello wrote:
> >> If the lid on a laptop is closed when eDP connectors are populated
> >> then it remains enabled when the initial framebuffer configuration
> >> is built.
> >>
> >> When creating the initial framebuffer configuration detect the ACPI
> >> lid status and if it's closed disable any eDP connectors.
> >>
> >> Reported-by: Chris Bainbridge
> >> Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/3349
> >> Signed-off-by: Mario Limonciello
> >> ---
> >> Cc: hughsi...@gmail.com
> >> v1->v2:
> >> * Match LVDS as well
> >> ---
> >> drivers/gpu/drm/drm_client_modeset.c | 30
> >> 1 file changed, 30 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/drm_client_modeset.c
> >> b/drivers/gpu/drm/drm_client_modeset.c
> >> index 31af5cf37a09..0b0411086e76 100644
> >> --- a/drivers/gpu/drm/drm_client_modeset.c
> >> +++ b/drivers/gpu/drm/drm_client_modeset.c
> >> @@ -8,6 +8,7 @@
> >>*/
> >>
> >> #include "drm/drm_modeset_lock.h"
> >> +#include
> >> #include
> >> #include
> >> #include
> >> @@ -257,6 +258,34 @@ static void drm_client_connectors_enabled(struct
> >> drm_connector **connectors,
> >>enabled[i] = drm_connector_enabled(connectors[i], false);
> >> }
> >>
> >> +static void drm_client_match_edp_lid(struct drm_device *dev,
> >> + struct drm_connector **connectors,
> >> + unsigned int connector_count,
> >> + bool *enabled)
> >> +{
> >> + int i;
> >> +
> >> + for (i = 0; i < connector_count; i++) {
> >> + struct drm_connector *connector = connectors[i];
> >> +
> >> + switch (connector->connector_type) {
> >> + case DRM_MODE_CONNECTOR_LVDS:
> >> + case DRM_MODE_CONNECTOR_eDP:
> >> + if (!enabled[i])
> >> + continue;
> >> + break;
> >> + default:
> >> + continue;
> >> + }
> >> +
> >> + if (!acpi_lid_open()) {
> >> + drm_dbg_kms(dev, "[CONNECTOR:%d:%s] lid is closed,
> >> disabling\n",
> >> + connector->base.id, connector->name);
> >> + enabled[i] = false;
> >> + }
> >> + }
> >> +}
> >
> > If you don't hook into some lid notify event how is one supposed to get
> > the display back to life after opening the lid?
>
> I guess in my mind it's a tangential to the "initial modeset". The DRM
> master can issue a modeset to enable the combination as desired.
This code is run whenever there's a hotplug/etc. Not sure why you're
only thinking about the initial modeset.
>
> When I tested I did confirm that with mutter such an event is received
> and it does the modeset to enable the eDP when lid is opened.
This code isn't relevant when you have a userspace drm master
calling the shots.
>
> Let me ask this - what happens if no DRM master running and you hotplug
> a DP cable? Does a "new" clone configuration get done?
Yes, this code reprobes the displays and comes up with a new
config to suit the new situation.
The other potential issue here is whether acpi_lid_open() is actually
trustworthy. Some kms drivers have/had some lid handling in their own
code, and I'm pretty sure those have often needed quirks/modparams
to actually do sensible things on certain machines.
FWIW I ripped out all the lid crap from i915 long ago since it was
half backed, mostly broken, and ugly, and I'm not looking to add it
back there. But I do think handling that in drm_client does seem
somewhat sane, as that should more or less match what userspace
clients would do. Just a question of how bad the quirk situation
will get...
Also a direct acpi_lid_open() call seems a bit iffy. But I guess if
someone needs this to work on non-ACPI system they get to figure out
how to abstract it better. acpi_lid_open() does seem to return != 0
when ACPI is not supported, so at least it would err on the side
of enabling everything.
--
Ville Syrjälä
Intel
Re: [PATCH v2] drm/client: Detect when ACPI lid is closed during initialization
On Tue, May 28, 2024 at 04:03:19PM -0500, Mario Limonciello wrote:
> If the lid on a laptop is closed when eDP connectors are populated
> then it remains enabled when the initial framebuffer configuration
> is built.
>
> When creating the initial framebuffer configuration detect the ACPI
> lid status and if it's closed disable any eDP connectors.
>
> Reported-by: Chris Bainbridge
> Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/3349
> Signed-off-by: Mario Limonciello
> ---
> Cc: hughsi...@gmail.com
> v1->v2:
> * Match LVDS as well
> ---
> drivers/gpu/drm/drm_client_modeset.c | 30
> 1 file changed, 30 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_client_modeset.c
> b/drivers/gpu/drm/drm_client_modeset.c
> index 31af5cf37a09..0b0411086e76 100644
> --- a/drivers/gpu/drm/drm_client_modeset.c
> +++ b/drivers/gpu/drm/drm_client_modeset.c
> @@ -8,6 +8,7 @@
> */
>
> #include "drm/drm_modeset_lock.h"
> +#include
> #include
> #include
> #include
> @@ -257,6 +258,34 @@ static void drm_client_connectors_enabled(struct
> drm_connector **connectors,
> enabled[i] = drm_connector_enabled(connectors[i], false);
> }
>
> +static void drm_client_match_edp_lid(struct drm_device *dev,
> + struct drm_connector **connectors,
> + unsigned int connector_count,
> + bool *enabled)
> +{
> + int i;
> +
> + for (i = 0; i < connector_count; i++) {
> + struct drm_connector *connector = connectors[i];
> +
> + switch (connector->connector_type) {
> + case DRM_MODE_CONNECTOR_LVDS:
> + case DRM_MODE_CONNECTOR_eDP:
> + if (!enabled[i])
> + continue;
> + break;
> + default:
> + continue;
> + }
> +
> + if (!acpi_lid_open()) {
> + drm_dbg_kms(dev, "[CONNECTOR:%d:%s] lid is closed,
> disabling\n",
> + connector->base.id, connector->name);
> + enabled[i] = false;
> + }
> + }
> +}
If you don't hook into some lid notify event how is one supposed to get
the display back to life after opening the lid?
> +
> static bool drm_client_target_cloned(struct drm_device *dev,
>struct drm_connector **connectors,
>unsigned int connector_count,
> @@ -844,6 +873,7 @@ int drm_client_modeset_probe(struct drm_client_dev
> *client, unsigned int width,
> memset(crtcs, 0, connector_count * sizeof(*crtcs));
> memset(offsets, 0, connector_count * sizeof(*offsets));
>
> + drm_client_match_edp_lid(dev, connectors, connector_count,
> enabled);
> if (!drm_client_target_cloned(dev, connectors, connector_count,
> modes,
> offsets, enabled, width, height)
> &&
> !drm_client_target_preferred(dev, connectors,
> connector_count, modes,
> --
> 2.43.0
--
Ville Syrjälä
Intel
Re: [PATCH 3/4] drm/imx: fix -Wformat-truncation warning in imx_ldb_probe()
On Thu, May 23, 2024 at 06:51:08PM +0300, Jani Nikula wrote:
> Enabling -Wformat-truncation yields the following warning:
>
> ../drivers/gpu/drm/imx/ipuv3/imx-ldb.c: In function ‘imx_ldb_probe’:
> ../drivers/gpu/drm/imx/ipuv3/imx-ldb.c:658:57: error: ‘_sel’ directive output
> may be truncated writing 4 bytes into a region of size between 3 and 13
> [-Werror=format-truncation=]
> 658 | snprintf(clkname, sizeof(clkname), "di%d_sel", i);
> | ^~~~
> ../drivers/gpu/drm/imx/ipuv3/imx-ldb.c:658:17: note: ‘snprintf’ output
> between 8 and 18 bytes into a destination of size 16
> 658 | snprintf(clkname, sizeof(clkname), "di%d_sel", i);
> | ^
If only the compiler could count to three...
>
> Silence the warning by checking the snprintf() return value.
>
> Signed-off-by: Jani Nikula
>
> ---
>
> Cc: Philipp Zabel
> Cc: Shawn Guo
> Cc: Sascha Hauer
> Cc: Pengutronix Kernel Team
> Cc: Fabio Estevam
> Cc: dri-de...@lists.freedesktop.org
> Cc: i...@lists.linux.dev
> ---
> drivers/gpu/drm/imx/ipuv3/imx-ldb.c | 6 +-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/imx/ipuv3/imx-ldb.c
> b/drivers/gpu/drm/imx/ipuv3/imx-ldb.c
> index 71d70194fcbd..46f779fe60ee 100644
> --- a/drivers/gpu/drm/imx/ipuv3/imx-ldb.c
> +++ b/drivers/gpu/drm/imx/ipuv3/imx-ldb.c
> @@ -654,8 +654,12 @@ static int imx_ldb_probe(struct platform_device *pdev)
>*/
> for (i = 0; i < 4; i++) {
> char clkname[16];
> + int len;
> +
> + len = snprintf(clkname, sizeof(clkname), "di%d_sel", i);
> + if (len >= sizeof(clkname))
> + dev_err(dev, "clkname truncated\n");
>
> - snprintf(clkname, sizeof(clkname), "di%d_sel", i);
> imx_ldb->clk_sel[i] = devm_clk_get(imx_ldb->dev, clkname);
> if (IS_ERR(imx_ldb->clk_sel[i])) {
> ret = PTR_ERR(imx_ldb->clk_sel[i]);
> --
> 2.39.2
--
Ville Syrjälä
Intel
Re: [PATCH 1/2] drm: Allow mode object properties to be added after a device is registered
On Wed, May 08, 2024 at 02:43:07PM -0500, Mario Limonciello wrote:
> When the colorspace property is registered on MST devices there is
> no `obj_free_cb` callback for it in drm_mode_object_add().
>
> Don't show a warning trace for __drm_mode_object_add() calls for
> DRM_MODE_OBJECT_PROPERTY.
You need to create the property ahead of time. See eg.
commit 1b9bd09630d4 ("drm/i915: Do not create a new max_bpc prop for MST
connectors")
>
> Reported-and-tested-by: Tyler Schneider
> Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/3353
> Signed-off-by: Mario Limonciello
> ---
> drivers/gpu/drm/drm_mode_object.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_mode_object.c
> b/drivers/gpu/drm/drm_mode_object.c
> index 0e8355063eee..b077547a2db4 100644
> --- a/drivers/gpu/drm/drm_mode_object.c
> +++ b/drivers/gpu/drm/drm_mode_object.c
> @@ -42,7 +42,7 @@ int __drm_mode_object_add(struct drm_device *dev, struct
> drm_mode_object *obj,
> {
> int ret;
>
> - WARN_ON(!dev->driver->load && dev->registered && !obj_free_cb);
> + WARN_ON(!dev->driver->load && dev->registered && !obj_free_cb &&
> obj_type != DRM_MODE_OBJECT_PROPERTY);
>
> mutex_lock(&dev->mode_config.idr_mutex);
> ret = idr_alloc(&dev->mode_config.object_idr, register_obj ? obj : NULL,
> --
> 2.43.0
--
Ville Syrjälä
Intel
Re: [PATCH 2/5] drm/amdgpu: Use drm_crtc_vblank_crtc()
On Wed, May 08, 2024 at 09:47:50AM -0400, Alex Deucher wrote: > On Mon, Apr 8, 2024 at 3:06 PM Ville Syrjala > wrote: > > > > From: Ville Syrjälä > > > > Replace the open coded drm_crtc_vblank_crtc() with the real > > thing. > > > > Cc: Alex Deucher > > Cc: "Christian König" > > Cc: "Pan, Xinhui" > > Cc: amd-gfx@lists.freedesktop.org > > Signed-off-by: Ville Syrjälä > > Reviewed-by: Alex Deucher Thanks. Pushed to drm-misc-next. -- Ville Syrjälä Intel
Re: [PATCH v0 02/14] drm/amdgpu,drm/radeon: Make I2C terminology more inclusive
On Fri, Mar 29, 2024 at 06:38:10PM +0100, Andi Shyti wrote: > Hi, > > On Fri, Mar 29, 2024 at 10:28:14AM -0700, Easwar Hariharan wrote: > > On 3/29/2024 10:16 AM, Andi Shyti wrote: > > > Hi Easwar, > > > > > > On Fri, Mar 29, 2024 at 05:00:26PM +, Easwar Hariharan wrote: > > >> I2C v7, SMBus 3.2, and I3C specifications have replaced "master/slave" > > > > > > I don't understand why we forget that i3c is 1.1.1 :-) > > > > That's because it's a copy-paste error from Wolfram's cover letter. :) I'll > > update > > next go-around. > > not a binding comment, though. Just for completeness, because we > are giving the version to the i2c and smbus, but not i3c. > > > >> with more appropriate terms. Inspired by and following on to Wolfram's > > >> series to fix drivers/i2c/[1], fix the terminology for users of > > >> I2C_ALGOBIT bitbanging interface, now that the approved verbiage exists > > >> in the specification. > > > > > > The specification talks about: > > > > > > - master -> controller > > > - slave -> target (and not client) > > > > > > But both you and Wolfram have used client. I'd like to reach > > > some more consistency here. > > > > I had the impression that remote targets (i.e external to the device) were > > to be called clients, > > e.g. the QSFP FRUs in drivers/infiniband, and internal ones targets. > > I chose the terminology according to that understanding, but now I can't > > find where I got that > > information. > > The word "client" does not even appear in the documentation (only > one instance in the i3c document), so that the change is not > related to the document as stated in the commit log. Unless, of > course, I am missing something. > > I'm OK with choosing a "customized" naming, but we need to reach > an agreement. > > I raised the same question to Wolfram. I don't know where that discussion happened, but my opinion is NAK to "client". Life is already confusing enough with these renames, so let's not make it even more confusing by inventing new names nowhere to be found in the spec. And let's especially not invent names that don't even fit the purpose. "Client" makes me think of "client/server" or some real world analogy. Neither of which seem to have any resemblence to how the term would be used for i2c. -- Ville Syrjälä Intel
Re: [PATCH v3 6/8] drm: add drm_mode_atomic_commit event
ntry->flags = flags;
> + ),
> + TP_printk("file=%p, pid=%8d, flags=%08x, crtcs=%s", __entry->file,
> + pid_nr(__entry->file->pid), __entry->flags,
> + __print_array(__get_dynamic_array(crtcs),
> __entry->ncrtcs, 4))
> +);
> +
> #endif /* _DRM_TRACE_H_ */
>
> /* This part must be outside protection */
> --
> 2.40.1
--
Ville Syrjälä
Intel
Re: [PATCH v6 3/5] drm: Add support to get EDID from ACPI
On Thu, Feb 15, 2024 at 12:20:56PM -0600, Mario Limonciello wrote:
> On 2/14/2024 17:13, Ville Syrjälä wrote:
> > On Wed, Feb 14, 2024 at 03:57:54PM -0600, Mario Limonciello wrote:
> >> Some manufacturers have intentionally put an EDID that differs from
> >> the EDID on the internal panel on laptops. Drivers that prefer to
> >> fetch this EDID can set a bit on the drm_connector to indicate that
> >> the DRM EDID helpers should try to fetch it and it is preferred if
> >> it's present.
> >>
> >> Signed-off-by: Mario Limonciello
> >> ---
> >> drivers/gpu/drm/Kconfig | 1 +
> >> drivers/gpu/drm/drm_edid.c | 109 +---
> >> include/drm/drm_connector.h | 6 ++
> >> include/drm/drm_edid.h | 1 +
> >> 4 files changed, 109 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> >> index 872edb47bb53..3db89e6af01d 100644
> >> --- a/drivers/gpu/drm/Kconfig
> >> +++ b/drivers/gpu/drm/Kconfig
> >> @@ -8,6 +8,7 @@
> >> menuconfig DRM
> >>tristate "Direct Rendering Manager (XFree86 4.1.0 and higher DRI
> >> support)"
> >>depends on (AGP || AGP=n) && !EMULATED_CMPXCHG && HAS_DMA
> >> + depends on (ACPI_VIDEO || ACPI_VIDEO=n)
> >>select DRM_PANEL_ORIENTATION_QUIRKS
> >>select DRM_KMS_HELPER if DRM_FBDEV_EMULATION
> >>select FB_CORE if DRM_FBDEV_EMULATION
> >> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> >> index 923c4423151c..cdc30c6d05d5 100644
> >> --- a/drivers/gpu/drm/drm_edid.c
> >> +++ b/drivers/gpu/drm/drm_edid.c
> >> @@ -28,6 +28,7 @@
> >>* DEALINGS IN THE SOFTWARE.
> >>*/
> >>
> >> +#include
> >> #include
> >> #include
> >> #include
> >> @@ -2188,6 +2189,58 @@ drm_do_probe_ddc_edid(void *data, u8 *buf, unsigned
> >> int block, size_t len)
> >>return ret == xfers ? 0 : -1;
> >> }
> >>
> >> +/**
> >> + * drm_do_probe_acpi_edid() - get EDID information via ACPI _DDC
> >> + * @data: struct drm_connector
> >> + * @buf: EDID data buffer to be filled
> >> + * @block: 128 byte EDID block to start fetching from
> >> + * @len: EDID data buffer length to fetch
> >> + *
> >> + * Try to fetch EDID information by calling acpi_video_get_edid()
> >> function.
> >> + *
> >> + * Return: 0 on success or error code on failure.
> >> + */
> >> +static int
> >> +drm_do_probe_acpi_edid(void *data, u8 *buf, unsigned int block, size_t
> >> len)
> >> +{
> >> + struct drm_connector *connector = data;
> >> + struct drm_device *ddev = connector->dev;
> >> + struct acpi_device *acpidev = ACPI_COMPANION(ddev->dev);
> >> + unsigned char start = block * EDID_LENGTH;
> >> + void *edid;
> >> + int r;
> >> +
> >> + if (!acpidev)
> >> + return -ENODEV;
> >> +
> >> + switch (connector->connector_type) {
> >> + case DRM_MODE_CONNECTOR_LVDS:
> >> + case DRM_MODE_CONNECTOR_eDP:
> >> + break;
> >> + default:
> >> + return -EINVAL;
> >> + }
> >
> > We could have other types of connectors that want this too.
> > I don't see any real benefit in having this check tbh. Drivers
> > should simply notset the flag on connectors where it won't work,
> > and only the driver can really know that.
>
> Ack.
>
> >
> >> + /* fetch the entire edid from BIOS */
> >> + r = acpi_video_get_edid(acpidev, ACPI_VIDEO_DISPLAY_LCD, -1, &edid);
> >> + if (r < 0) {
> >> + DRM_DEBUG_KMS("Failed to get EDID from ACPI: %d\n", r);
> >> + return r;
> >> + }
> >> + if (len > r || start > r || start + len > r) {
> >> + r = -EINVAL;
> >> + goto cleanup;
> >> + }
> >> +
> >> + memcpy(buf, edid + start, len);
> >> + r = 0;
> >> +
> >> +cleanup:
> >> + kfree(edid);
> >> +
> >> + return r;
> >> +}
> >> +
> >> static void connector_bad_edid(struct drm_connector *connector,
> >> const struct edid *edid, int num_blocks)
> >> {
> >> @@ -2621,7 +2674,8 @@ EXPORT_SYMBO
Re: [PATCH v6 3/5] drm: Add support to get EDID from ACPI
onnector->acpi_edid_allowed)
> + edid = _drm_do_get_edid(connector, drm_do_probe_acpi_edid,
> connector, NULL);
> +
> + if (!edid) {
> + if (connector->force == DRM_FORCE_UNSPECIFIED &&
> !drm_probe_ddc(adapter))
> + return NULL;
> + edid = _drm_do_get_edid(connector, drm_do_probe_ddc_edid,
> adapter, NULL);
> + }
>
> - edid = _drm_do_get_edid(connector, drm_do_probe_ddc_edid, adapter,
> NULL);
> drm_connector_update_edid_property(connector, edid);
> return edid;
> }
> EXPORT_SYMBOL(drm_get_edid);
>
> +/**
> + * drm_edid_read_acpi - get EDID data, if available
> + * @connector: connector we're probing
> + *
> + * Use the BIOS to attempt to grab EDID data if possible.
> + *
> + * The returned pointer must be freed using drm_edid_free().
> + *
> + * Return: Pointer to valid EDID or NULL if we couldn't find any.
> + */
> +const struct drm_edid *drm_edid_read_acpi(struct drm_connector *connector)
> +{
> + const struct drm_edid *drm_edid;
> +
> + if (connector->force == DRM_FORCE_OFF)
> + return NULL;
> +
> + drm_edid = drm_edid_read_custom(connector, drm_do_probe_acpi_edid,
> connector);
> +
> + /* Note: Do *not* call connector updates here. */
> +
> + return drm_edid;
> +}
> +EXPORT_SYMBOL(drm_edid_read_acpi);
> +
> /**
> * drm_edid_read_custom - Read EDID data using given EDID block read function
> * @connector: Connector to use
> @@ -2727,10 +2811,11 @@ const struct drm_edid *drm_edid_read_ddc(struct
> drm_connector *connector,
> EXPORT_SYMBOL(drm_edid_read_ddc);
>
> /**
> - * drm_edid_read - Read EDID data using connector's I2C adapter
> + * drm_edid_read - Read EDID data using BIOS or connector's I2C adapter
> * @connector: Connector to use
> *
> - * Read EDID using the connector's I2C adapter.
> + * Read EDID from BIOS if allowed by connector or by using the connector's
> + * I2C adapter.
> *
> * The EDID may be overridden using debugfs override_edid or firmware EDID
> * (drm_edid_load_firmware() and drm.edid_firmware parameter), in this
> priority
> @@ -2742,10 +2827,18 @@ EXPORT_SYMBOL(drm_edid_read_ddc);
> */
> const struct drm_edid *drm_edid_read(struct drm_connector *connector)
> {
> + const struct drm_edid *drm_edid = NULL;
> +
> if (drm_WARN_ON(connector->dev, !connector->ddc))
> return NULL;
>
> - return drm_edid_read_ddc(connector, connector->ddc);
> + if (connector->acpi_edid_allowed)
That should probably be called 'prefer_acpi_edid' or something
since it's the first choice when the flag is set.
But I'm not so sure there's any real benefit in having this
flag at all. You anyway have to modify the driver to use this,
so why not just have the driver do the call directly instead of
adding this extra detour via the flag?
> + drm_edid = drm_edid_read_acpi(connector);
> +
> + if (!drm_edid)
> + drm_edid = drm_edid_read_ddc(connector, connector->ddc);
> +
> + return drm_edid;
> }
> EXPORT_SYMBOL(drm_edid_read);
>
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index fe88d7fc6b8f..74ed47f37a69 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -1886,6 +1886,12 @@ struct drm_connector {
>
> /** @hdr_sink_metadata: HDR Metadata Information read from sink */
> struct hdr_sink_metadata hdr_sink_metadata;
> +
> + /**
> + * @acpi_edid_allowed: Get the EDID from the BIOS, if available.
> + * This is only applicable to eDP and LVDS displays.
> + */
> + bool acpi_edid_allowed;
Aren't there other bools/small stuff in there for tighter packing?
> };
>
> #define obj_to_connector(x) container_of(x, struct drm_connector, base)
> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> index 7923bc00dc7a..1c1ee927de9c 100644
> --- a/include/drm/drm_edid.h
> +++ b/include/drm/drm_edid.h
> @@ -459,5 +459,6 @@ bool drm_edid_is_digital(const struct drm_edid *drm_edid);
>
> const u8 *drm_find_edid_extension(const struct drm_edid *drm_edid,
> int ext_id, int *ext_index);
> +const struct drm_edid *drm_edid_read_acpi(struct drm_connector *connector);
>
> #endif /* __DRM_EDID_H__ */
> --
> 2.34.1
--
Ville Syrjälä
Intel
Re: [PATCH v2 2/2] drm/amdgpu: Implement check_async_props for planes
On Wed, Jan 24, 2024 at 11:14:40AM -0300, André Almeida wrote:
> Hi Ville,
>
> Em 19/01/2024 15:25, Ville Syrjälä escreveu:
> > On Fri, Jan 19, 2024 at 03:12:35PM -0300, André Almeida wrote:
> >> AMD GPUs can do async flips with changes on more properties than just
> >> the FB ID, so implement a custom check_async_props for AMD planes.
> >>
> >> Allow amdgpu to do async flips with IN_FENCE_ID and FB_DAMAGE_CLIPS
> >> properties. For userspace to check if a driver support this two
> >> properties, the strategy for now is to use TEST_ONLY commits.
> >>
> >> Signed-off-by: André Almeida
> >> ---
> >> v2: Drop overlay plane option for now
> >>
> >> .../amd/display/amdgpu_dm/amdgpu_dm_plane.c | 29 +++
> >> 1 file changed, 29 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c
> >> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c
> >> index 116121e647ca..7afe8c1b62d4 100644
> >> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c
> >> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c
> >> @@ -25,6 +25,7 @@
> >>*/
> >>
> >> #include
> >> +#include
> >> #include
> >> #include
> >> #include
> >> @@ -1430,6 +1431,33 @@ static void
> >> amdgpu_dm_plane_drm_plane_destroy_state(struct drm_plane *plane,
> >>drm_atomic_helper_plane_destroy_state(plane, state);
> >> }
> >>
> >> +static int amdgpu_dm_plane_check_async_props(struct drm_property *prop,
> >> +struct drm_plane *plane,
> >> +struct drm_plane_state *plane_state,
> >> +struct drm_mode_object *obj,
> >> +u64 prop_value, u64 old_val)
> >> +{
> >> + struct drm_mode_config *config = &plane->dev->mode_config;
> >> + int ret;
> >> +
> >> + if (prop != config->prop_fb_id &&
> >> + prop != config->prop_in_fence_fd &&
> >
> > IN_FENCE should just be allowed always.
>
> Do you mean that the common path should allow IN_FENCE_FD for all drivers?
Yes.
--
Ville Syrjälä
Intel
Re: [PATCH v2 2/2] drm/amdgpu: Implement check_async_props for planes
On Fri, Jan 19, 2024 at 03:12:35PM -0300, André Almeida wrote:
> AMD GPUs can do async flips with changes on more properties than just
> the FB ID, so implement a custom check_async_props for AMD planes.
>
> Allow amdgpu to do async flips with IN_FENCE_ID and FB_DAMAGE_CLIPS
> properties. For userspace to check if a driver support this two
> properties, the strategy for now is to use TEST_ONLY commits.
>
> Signed-off-by: André Almeida
> ---
> v2: Drop overlay plane option for now
>
> .../amd/display/amdgpu_dm/amdgpu_dm_plane.c | 29 +++
> 1 file changed, 29 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c
> index 116121e647ca..7afe8c1b62d4 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c
> @@ -25,6 +25,7 @@
> */
>
> #include
> +#include
> #include
> #include
> #include
> @@ -1430,6 +1431,33 @@ static void
> amdgpu_dm_plane_drm_plane_destroy_state(struct drm_plane *plane,
> drm_atomic_helper_plane_destroy_state(plane, state);
> }
>
> +static int amdgpu_dm_plane_check_async_props(struct drm_property *prop,
> + struct drm_plane *plane,
> + struct drm_plane_state *plane_state,
> + struct drm_mode_object *obj,
> + u64 prop_value, u64 old_val)
> +{
> + struct drm_mode_config *config = &plane->dev->mode_config;
> + int ret;
> +
> + if (prop != config->prop_fb_id &&
> + prop != config->prop_in_fence_fd &&
IN_FENCE should just be allowed always.
> + prop != config->prop_fb_damage_clips) {
This seems a bit dubious to me. How is amdgpu using the damage
information during async flips?
> + ret = drm_atomic_plane_get_property(plane, plane_state,
> + prop, &old_val);
> + return drm_atomic_check_prop_changes(ret, old_val, prop_value,
> prop);
> + }
> +
> + if (plane_state->plane->type != DRM_PLANE_TYPE_PRIMARY) {
> + drm_dbg_atomic(prop->dev,
> +"[OBJECT:%d] Only primary planes can be changed
> during async flip\n",
> +obj->id);
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> static const struct drm_plane_funcs dm_plane_funcs = {
> .update_plane = drm_atomic_helper_update_plane,
> .disable_plane = drm_atomic_helper_disable_plane,
> @@ -1438,6 +1466,7 @@ static const struct drm_plane_funcs dm_plane_funcs = {
> .atomic_duplicate_state = amdgpu_dm_plane_drm_plane_duplicate_state,
> .atomic_destroy_state = amdgpu_dm_plane_drm_plane_destroy_state,
> .format_mod_supported = amdgpu_dm_plane_format_mod_supported,
> + .check_async_props = amdgpu_dm_plane_check_async_props,
> };
>
> int amdgpu_dm_plane_init(struct amdgpu_display_manager *dm,
> --
> 2.43.0
--
Ville Syrjälä
Intel
Re: [PATCH v2 01/11] drm/dp_mst: Store the MST PBN divider value in fixed point format
On Thu, Nov 16, 2023 at 03:18:31PM +0200, Imre Deak wrote:
> On UHBR links the PBN divider is a fractional number, accordingly store
> it in fixed point format. For now drm_dp_get_vc_payload_bw() always
> returns a whole number and all callers will use only the integer part of
> it which should preserve the current behavior. The next patch will fix
> drm_dp_get_vc_payload_bw() for UHBR rates returning a fractional number
> for those (also accounting for the channel coding efficiency correctly).
>
> Cc: Lyude Paul
> Cc: Harry Wentland
> Cc: Alex Deucher
> Cc: Wayne Lin
> Cc: amd-gfx@lists.freedesktop.org
> Cc: dri-de...@lists.freedesktop.org
> Signed-off-by: Imre Deak
> ---
> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 5 +++--
> .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 3 ++-
> .../display/amdgpu_dm/amdgpu_dm_mst_types.c | 5 +++--
> drivers/gpu/drm/display/drm_dp_mst_topology.c | 22 +--
> drivers/gpu/drm/i915/display/intel_dp_mst.c | 3 ++-
> drivers/gpu/drm/nouveau/dispnv50/disp.c | 6 +++--
> include/drm/display/drm_dp_mst_helper.h | 7 +++---
> 7 files changed, 33 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 74f9f02abcdec..12346b21d0b05 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -85,6 +85,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -6909,8 +6910,8 @@ static int dm_encoder_helper_atomic_check(struct
> drm_encoder *encoder,
> if (IS_ERR(mst_state))
> return PTR_ERR(mst_state);
>
> - if (!mst_state->pbn_div)
> - mst_state->pbn_div =
> dm_mst_get_pbn_divider(aconnector->mst_root->dc_link);
> + if (!mst_state->pbn_div.full)
> + mst_state->pbn_div.full =
> dfixed_const(dm_mst_get_pbn_divider(aconnector->mst_root->dc_link));
Why doesn't that dfixed stuff return the correct type?
Anyways looks mostly mechanical
Reviewed-by: Ville Syrjälä
>
> if (!state->duplicated) {
> int max_bpc = conn_state->max_requested_bpc;
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> index ed784cf27d396..63024393b516e 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> @@ -31,6 +31,7 @@
> #include
> #include
> #include
> +#include
>
> #include "dm_services.h"
> #include "amdgpu.h"
> @@ -210,7 +211,7 @@ static void dm_helpers_construct_old_payload(
> struct drm_dp_mst_atomic_payload *old_payload)
> {
> struct drm_dp_mst_atomic_payload *pos;
> - int pbn_per_slot = mst_state->pbn_div;
> + int pbn_per_slot = dfixed_trunc(mst_state->pbn_div);
> u8 next_payload_vc_start = mgr->next_start_slot;
> u8 payload_vc_start = new_payload->vc_start_slot;
> u8 allocated_time_slots;
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> index 9a58e1a4c5f49..d1ba3ae228b08 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> @@ -27,6 +27,7 @@
> #include
> #include
> #include
> +#include
> #include "dm_services.h"
> #include "amdgpu.h"
> #include "amdgpu_dm.h"
> @@ -941,10 +942,10 @@ static int increase_dsc_bpp(struct drm_atomic_state
> *state,
> link_timeslots_used = 0;
>
> for (i = 0; i < count; i++)
> - link_timeslots_used += DIV_ROUND_UP(vars[i + k].pbn,
> mst_state->pbn_div);
> + link_timeslots_used += DIV_ROUND_UP(vars[i + k].pbn,
> dfixed_trunc(mst_state->pbn_div));
>
> fair_pbn_alloc =
> - (63 - link_timeslots_used) / remaining_to_increase *
> mst_state->pbn_div;
> + (63 - link_timeslots_used) / remaining_to_increase *
> dfixed_trunc(mst_state->pbn_div);
>
> if (initial_slack[next_index] > fair_pbn_alloc) {
> vars[next_index].pbn += fair_pbn_alloc;
> diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> index 4d72c9a32026e..000d05e80352a 100644
> --- a/drivers/gpu/drm/dis
Re: [PATCH v6 6/6] drm/doc: Define KMS atomic state set
On Mon, Oct 16, 2023 at 10:00:51PM +, Simon Ser wrote: > On Monday, October 16th, 2023 at 17:10, Ville Syrjälä > wrote: > > > On Mon, Oct 16, 2023 at 05:52:22PM +0300, Pekka Paalanen wrote: > > > > > On Mon, 16 Oct 2023 15:42:16 +0200 > > > André Almeida andrealm...@igalia.com wrote: > > > > > > > Hi Pekka, > > > > > > > > On 10/16/23 14:18, Pekka Paalanen wrote: > > > > > > > > > On Mon, 16 Oct 2023 12:52:32 +0200 > > > > > André Almeida andrealm...@igalia.com wrote: > > > > > > > > > > > Hi Michel, > > > > > > > > > > > > On 8/17/23 12:37, Michel Dänzer wrote: > > > > > > > > > > > > > On 8/15/23 20:57, André Almeida wrote: > > > > > > > > > > > > > > > From: Pekka Paalanen pekka.paala...@collabora.com > > > > > > > > > > > > > > > > Specify how the atomic state is maintained between userspace and > > > > > > > > kernel, plus the special case for async flips. > > > > > > > > > > > > > > > > Signed-off-by: Pekka Paalanen pekka.paala...@collabora.com > > > > > > > > Signed-off-by: André Almeida andrealm...@igalia.com > > > > > > > > [...] > > > > > > > > > > > > > > > +An atomic commit with the flag DRM_MODE_PAGE_FLIP_ASYNC is > > > > > > > > allowed to > > > > > > > > +effectively change only the FB_ID property on any planes. > > > > > > > > No-operation changes > > > > > > > > +are ignored as always. [...] > > > > > > > > During the hackfest in Brno, it was mentioned that a commit > > > > > > > > which re-sets the same FB_ID could actually have an effect with > > > > > > > > VRR: It could trigger scanout of the next frame before vertical > > > > > > > > blank has reached its maximum duration. Some kind of mechanism > > > > > > > > is required for this in order to allow user space to perform > > > > > > > > low frame rate compensation. > > > > > > > > > > > > Xaver tested this hypothesis in a flipping the same fb in a VRR > > > > > > monitor > > > > > > and it worked as expected, so this shouldn't be a concern. > > > > > > Right, so it must have some effect. It cannot be simply ignored > > > > > > like in > > > > > > the proposed doc wording. Do we special-case re-setting the same > > > > > > FB_ID > > > > > > as "not a no-op" or "not ignored" or some other way? > > > > > > There's an effect in the refresh rate, the image won't change but it > > > > > > will report that a flip had happened asynchronously so the reported > > > > > > framerate will be increased. Maybe an additional wording could be > > > > > > like: > > > > > > > > Flipping to the same FB_ID will result in a immediate flip as if it was > > > > changing to a different one, with no effect on the image but effecting > > > > the reported frame rate. > > > > > > Re-setting FB_ID to its current value is a special case regardless of > > > PAGE_FLIP_ASYNC, is it not? > > > > No. The rule has so far been that all side effects are observed > > even if you flip to the same fb. And that is one of my annoyances > > with this proposal. The rules will now be different for async flips > > vs. everything else. > > Well with the patches the async page-flip case is exactly the same as > the non-async page-flip case. In both cases, if a FB_ID is included in > an atomic commit then the side effects are triggered even if the property > value didn't change. The rules are the same for everything. I see it only checking if FB_ID changes or not. If it doesn't change then the implication is that the side effects will in fact be skipped as not all planes may even support async flips. -- Ville Syrjälä Intel
Re: [PATCH v6 6/6] drm/doc: Define KMS atomic state set
On Mon, Oct 16, 2023 at 05:52:22PM +0300, Pekka Paalanen wrote: > On Mon, 16 Oct 2023 15:42:16 +0200 > André Almeida wrote: > > > Hi Pekka, > > > > On 10/16/23 14:18, Pekka Paalanen wrote: > > > On Mon, 16 Oct 2023 12:52:32 +0200 > > > André Almeida wrote: > > > > > >> Hi Michel, > > >> > > >> On 8/17/23 12:37, Michel Dänzer wrote: > > >>> On 8/15/23 20:57, André Almeida wrote: > > >>>> From: Pekka Paalanen > > >>>> > > >>>> Specify how the atomic state is maintained between userspace and > > >>>> kernel, plus the special case for async flips. > > >>>> > > >>>> Signed-off-by: Pekka Paalanen > > >>>> Signed-off-by: André Almeida > > >>> [...] > > >>> > > >>>> +An atomic commit with the flag DRM_MODE_PAGE_FLIP_ASYNC is allowed to > > >>>> +effectively change only the FB_ID property on any planes. > > >>>> No-operation changes > > >>>> +are ignored as always. [...] > > >>> During the hackfest in Brno, it was mentioned that a commit which > > >>> re-sets the same FB_ID could actually have an effect with VRR: It could > > >>> trigger scanout of the next frame before vertical blank has reached its > > >>> maximum duration. Some kind of mechanism is required for this in order > > >>> to allow user space to perform low frame rate compensation. > > >>> > > >> Xaver tested this hypothesis in a flipping the same fb in a VRR monitor > > >> and it worked as expected, so this shouldn't be a concern. > > > Right, so it must have some effect. It cannot be simply ignored like in > > > the proposed doc wording. Do we special-case re-setting the same FB_ID > > > as "not a no-op" or "not ignored" or some other way? > > There's an effect in the refresh rate, the image won't change but it > > will report that a flip had happened asynchronously so the reported > > framerate will be increased. Maybe an additional wording could be like: > > > > Flipping to the same FB_ID will result in a immediate flip as if it was > > changing to a different one, with no effect on the image but effecting > > the reported frame rate. > > Re-setting FB_ID to its current value is a special case regardless of > PAGE_FLIP_ASYNC, is it not? No. The rule has so far been that all side effects are observed even if you flip to the same fb. And that is one of my annoyances with this proposal. The rules will now be different for async flips vs. everything else. The other issues (mainly relating to hardware where not all planes support async flips) I've already highlighted in some earlier mail. -- Ville Syrjälä Intel
Re: [PATCH] drm/edid: add 8 bpc quirk to the BenQ GW2765
On Thu, Oct 12, 2023 at 02:49:27PM -0400, Hamza Mahfooz wrote:
> The BenQ GW2765 reports that it supports higher (> 8) bpc modes, but
> when trying to set them we end up with a black screen. So, limit it to 8
> bpc modes.
Bad cable/etc was ruled out as the cause?
>
> Cc: sta...@vger.kernel.org # 6.5+
> Link: https://gitlab.freedesktop.org/drm/amd/-/issues/2610
> Signed-off-by: Hamza Mahfooz
> ---
> drivers/gpu/drm/drm_edid.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 0454da505687..bca2af4fe1fc 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -123,6 +123,9 @@ static const struct edid_quirk {
> /* AEO model 0 reports 8 bpc, but is a 6 bpc panel */
> EDID_QUIRK('A', 'E', 'O', 0, EDID_QUIRK_FORCE_6BPC),
>
> + /* BenQ GW2765 */
> + EDID_QUIRK('B', 'N', 'Q', 0x78d6, EDID_QUIRK_FORCE_8BPC),
> +
> /* BOE model on HP Pavilion 15-n233sl reports 8 bpc, but is a 6 bpc
> panel */
> EDID_QUIRK('B', 'O', 'E', 0x78b, EDID_QUIRK_FORCE_6BPC),
>
> --
> 2.42.0
--
Ville Syrjälä
Intel
Re: [PATCH] drm/dp_mst: Clear MSG_RDY flag before sending new message
display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -3939,6 +3939,9 @@ intel_dp_check_mst_status(struct intel_dp *intel_dp)
> if (!memchr_inv(ack, 0, sizeof(ack)))
> break;
>
> + /* MSG_RDY ack is done in drm*/
> + ack[1] &= ~(DP_DOWN_REP_MSG_RDY | DP_UP_REQ_MSG_RDY);
> +
> if (!intel_dp_ack_sink_irq_esi(intel_dp, ack))
> drm_dbg_kms(&i915->drm, "Failed to ack ESI\n");
> }
> diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c
> b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> index edcb2529b402..e905987104ed 100644
> --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
> +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> @@ -1336,6 +1336,8 @@ nv50_mstm_service(struct nouveau_drm *drm,
> if (!handled)
> break;
>
> + /* MSG_RDY ack is done in drm*/
> + esi[1] &= ~(DP_DOWN_REP_MSG_RDY | DP_UP_REQ_MSG_RDY);
> rc = drm_dp_dpcd_write(aux, DP_SINK_COUNT_ESI + 1, &esi[1],
> 3);
> if (rc != 3) {
> --
> 2.37.3
--
Ville Syrjälä
Intel
Re: [PATCH v3 09/17] drm/amd/display: Register Colorspace property for DP and HDMI
On Fri, Mar 17, 2023 at 07:47:52PM +0100, Sebastian Wick wrote: > On Fri, Mar 17, 2023 at 7:38 PM Ville Syrjälä > wrote: > > > > On Fri, Mar 17, 2023 at 06:40:53PM +0100, Sebastian Wick wrote: > > > On Fri, Mar 17, 2023 at 5:34 PM Ville Syrjälä > > > wrote: > > > > > > > > On Fri, Mar 17, 2023 at 05:37:51PM +0200, Pekka Paalanen wrote: > > > > > On Fri, 17 Mar 2023 16:14:38 +0200 > > > > > Ville Syrjälä wrote: > > > > > > > > > > > On Fri, Mar 17, 2023 at 03:35:53PM +0200, Pekka Paalanen wrote: > > > > > > > On Fri, 17 Mar 2023 14:50:40 +0200 > > > > > > > Ville Syrjälä wrote: > > > > > > > > > > > > > > > On Fri, Mar 17, 2023 at 10:53:35AM +0200, Pekka Paalanen wrote: > > > > > > > > > On Fri, 17 Mar 2023 01:01:38 +0200 > > > > > > > > > Ville Syrjälä wrote: > > > > > > > > > > > > > > > > > > > On Thu, Mar 16, 2023 at 10:13:54PM +0100, Sebastian Wick > > > > > > > > > > wrote: > > > > > > > > > > > On Thu, Mar 16, 2023 at 1:35 PM Ville Syrjälä > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > On Thu, Mar 16, 2023 at 01:34:49PM +0200, Pekka > > > > > > > > > > > > Paalanen wrote: > > > > > > > > > > > > > On Thu, 16 Mar 2023 12:47:51 +0200 > > > > > > > > > > > > > Ville Syrjälä wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > On Thu, Mar 16, 2023 at 12:07:01PM +0200, Pekka > > > > > > > > > > > > > > Paalanen wrote: > > > > > > > > > > > > > > > On Thu, 16 Mar 2023 11:50:27 +0200 > > > > > > > > > > > > > > > Ville Syrjälä > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Thu, Mar 16, 2023 at 01:37:24AM +0100, > > > > > > > > > > > > > > > > Sebastian Wick wrote: > > > > > > > > > > > > > > > > > On Tue, Mar 7, 2023 at 4:12 PM Harry Wentland > > > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > We want compositors to be able to set the > > > > > > > > > > > > > > > > > > output > > > > > > > > > > > > > > > > > > colorspace on DP and HDMI outputs, based on > > > > > > > > > > > > > > > > > > the > > > > > > > > > > > > > > > > > > caps reported from the receiver via EDID. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > About that... The documentation says that > > > > > > > > > > > > > > > > > user space has to check the > > > > > > > > > > > > > > > > > EDID for what the sink actually supports. So > > > > > > > > > > > > > > > > > whatever is in > > > > > > > > > > > > > > > > > supported_colorspaces is just what the > > > > > > > > > > > > > > > > > driver/hardware is able to set > > > > > > > > > > > > > > > > > but doesn't actually indicate that the sink > > > > > > > > > > > > > > > > > supports it. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > So the only way to enable bt2020 is by > > > > > > > > > > > > > > > > > checking if the sink supports > > > >
Re: [PATCH v3 09/17] drm/amd/display: Register Colorspace property for DP and HDMI
On Fri, Mar 17, 2023 at 06:40:53PM +0100, Sebastian Wick wrote: > On Fri, Mar 17, 2023 at 5:34 PM Ville Syrjälä > wrote: > > > > On Fri, Mar 17, 2023 at 05:37:51PM +0200, Pekka Paalanen wrote: > > > On Fri, 17 Mar 2023 16:14:38 +0200 > > > Ville Syrjälä wrote: > > > > > > > On Fri, Mar 17, 2023 at 03:35:53PM +0200, Pekka Paalanen wrote: > > > > > On Fri, 17 Mar 2023 14:50:40 +0200 > > > > > Ville Syrjälä wrote: > > > > > > > > > > > On Fri, Mar 17, 2023 at 10:53:35AM +0200, Pekka Paalanen wrote: > > > > > > > On Fri, 17 Mar 2023 01:01:38 +0200 > > > > > > > Ville Syrjälä wrote: > > > > > > > > > > > > > > > On Thu, Mar 16, 2023 at 10:13:54PM +0100, Sebastian Wick wrote: > > > > > > > > > On Thu, Mar 16, 2023 at 1:35 PM Ville Syrjälä > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > On Thu, Mar 16, 2023 at 01:34:49PM +0200, Pekka Paalanen > > > > > > > > > > wrote: > > > > > > > > > > > On Thu, 16 Mar 2023 12:47:51 +0200 > > > > > > > > > > > Ville Syrjälä wrote: > > > > > > > > > > > > > > > > > > > > > > > On Thu, Mar 16, 2023 at 12:07:01PM +0200, Pekka > > > > > > > > > > > > Paalanen wrote: > > > > > > > > > > > > > On Thu, 16 Mar 2023 11:50:27 +0200 > > > > > > > > > > > > > Ville Syrjälä wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > On Thu, Mar 16, 2023 at 01:37:24AM +0100, Sebastian > > > > > > > > > > > > > > Wick wrote: > > > > > > > > > > > > > > > On Tue, Mar 7, 2023 at 4:12 PM Harry Wentland > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > We want compositors to be able to set the output > > > > > > > > > > > > > > > > colorspace on DP and HDMI outputs, based on the > > > > > > > > > > > > > > > > caps reported from the receiver via EDID. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > About that... The documentation says that user > > > > > > > > > > > > > > > space has to check the > > > > > > > > > > > > > > > EDID for what the sink actually supports. So > > > > > > > > > > > > > > > whatever is in > > > > > > > > > > > > > > > supported_colorspaces is just what the > > > > > > > > > > > > > > > driver/hardware is able to set > > > > > > > > > > > > > > > but doesn't actually indicate that the sink > > > > > > > > > > > > > > > supports it. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > So the only way to enable bt2020 is by checking > > > > > > > > > > > > > > > if the sink supports > > > > > > > > > > > > > > > both RGB and YUV variants because both could be > > > > > > > > > > > > > > > used by the driver. > > > > > > > > > > > > > > > Not great at all. Something to remember for the > > > > > > > > > > > > > > > new property. > > > > > > > > > > > > > > > > > > > > > > > > > > > > Hmm. I wonder if that's even legal... Looks like > > > > > > > > > > > > > > maybe it > > > > > > > > > > > > > > is since I can't immediately spot anything in > > > > > > > > > > > > > > CTA-861 to > > > > > > > > > > > > > > forbid
Re: [PATCH v3 09/17] drm/amd/display: Register Colorspace property for DP and HDMI
On Fri, Mar 17, 2023 at 05:37:51PM +0200, Pekka Paalanen wrote: > On Fri, 17 Mar 2023 16:14:38 +0200 > Ville Syrjälä wrote: > > > On Fri, Mar 17, 2023 at 03:35:53PM +0200, Pekka Paalanen wrote: > > > On Fri, 17 Mar 2023 14:50:40 +0200 > > > Ville Syrjälä wrote: > > > > > > > On Fri, Mar 17, 2023 at 10:53:35AM +0200, Pekka Paalanen wrote: > > > > > On Fri, 17 Mar 2023 01:01:38 +0200 > > > > > Ville Syrjälä wrote: > > > > > > > > > > > On Thu, Mar 16, 2023 at 10:13:54PM +0100, Sebastian Wick wrote: > > > > > > > On Thu, Mar 16, 2023 at 1:35 PM Ville Syrjälä > > > > > > > wrote: > > > > > > > > > > > > > > > > On Thu, Mar 16, 2023 at 01:34:49PM +0200, Pekka Paalanen wrote: > > > > > > > > > > > > > > > > > On Thu, 16 Mar 2023 12:47:51 +0200 > > > > > > > > > Ville Syrjälä wrote: > > > > > > > > > > > > > > > > > > > On Thu, Mar 16, 2023 at 12:07:01PM +0200, Pekka Paalanen > > > > > > > > > > wrote: > > > > > > > > > > > On Thu, 16 Mar 2023 11:50:27 +0200 > > > > > > > > > > > Ville Syrjälä wrote: > > > > > > > > > > > > > > > > > > > > > > > On Thu, Mar 16, 2023 at 01:37:24AM +0100, Sebastian > > > > > > > > > > > > Wick wrote: > > > > > > > > > > > > > On Tue, Mar 7, 2023 at 4:12 PM Harry Wentland > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > We want compositors to be able to set the output > > > > > > > > > > > > > > colorspace on DP and HDMI outputs, based on the > > > > > > > > > > > > > > caps reported from the receiver via EDID. > > > > > > > > > > > > > > > > > > > > > > > > > > About that... The documentation says that user space > > > > > > > > > > > > > has to check the > > > > > > > > > > > > > EDID for what the sink actually supports. So whatever > > > > > > > > > > > > > is in > > > > > > > > > > > > > supported_colorspaces is just what the > > > > > > > > > > > > > driver/hardware is able to set > > > > > > > > > > > > > but doesn't actually indicate that the sink supports > > > > > > > > > > > > > it. > > > > > > > > > > > > > > > > > > > > > > > > > > So the only way to enable bt2020 is by checking if > > > > > > > > > > > > > the sink supports > > > > > > > > > > > > > both RGB and YUV variants because both could be used > > > > > > > > > > > > > by the driver. > > > > > > > > > > > > > Not great at all. Something to remember for the new > > > > > > > > > > > > > property. > > > > > > > > > > > > > > > > > > > > > > > > Hmm. I wonder if that's even legal... Looks like maybe > > > > > > > > > > > > it > > > > > > > > > > > > is since I can't immediately spot anything in CTA-861 to > > > > > > > > > > > > forbid it :/ > > > > > > > > > > > > > > > > > > > > > > Wouldn't the driver do the same EDID check before > > > > > > > > > > > choosing whether it > > > > > > > > > > > uses RGB or YCbCr signalling? > > > > > > > > > > > > > > > > > > > > I suppose it could. The modeset would then fail, which is > > > > > > > > > > perhaps > > > > > > > > > > > > > > > > > > Could? What are they missin
Re: [PATCH v3 09/17] drm/amd/display: Register Colorspace property for DP and HDMI
On Fri, Mar 17, 2023 at 03:35:53PM +0200, Pekka Paalanen wrote: > On Fri, 17 Mar 2023 14:50:40 +0200 > Ville Syrjälä wrote: > > > On Fri, Mar 17, 2023 at 10:53:35AM +0200, Pekka Paalanen wrote: > > > On Fri, 17 Mar 2023 01:01:38 +0200 > > > Ville Syrjälä wrote: > > > > > > > On Thu, Mar 16, 2023 at 10:13:54PM +0100, Sebastian Wick wrote: > > > > > On Thu, Mar 16, 2023 at 1:35 PM Ville Syrjälä > > > > > wrote: > > > > > > > > > > > > On Thu, Mar 16, 2023 at 01:34:49PM +0200, Pekka Paalanen wrote: > > > > > > > On Thu, 16 Mar 2023 12:47:51 +0200 > > > > > > > Ville Syrjälä wrote: > > > > > > > > > > > > > > > On Thu, Mar 16, 2023 at 12:07:01PM +0200, Pekka Paalanen wrote: > > > > > > > > > > > > > > > > > On Thu, 16 Mar 2023 11:50:27 +0200 > > > > > > > > > Ville Syrjälä wrote: > > > > > > > > > > > > > > > > > > > On Thu, Mar 16, 2023 at 01:37:24AM +0100, Sebastian Wick > > > > > > > > > > wrote: > > > > > > > > > > > On Tue, Mar 7, 2023 at 4:12 PM Harry Wentland > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > We want compositors to be able to set the output > > > > > > > > > > > > colorspace on DP and HDMI outputs, based on the > > > > > > > > > > > > caps reported from the receiver via EDID. > > > > > > > > > > > > > > > > > > > > > > About that... The documentation says that user space has > > > > > > > > > > > to check the > > > > > > > > > > > EDID for what the sink actually supports. So whatever is > > > > > > > > > > > in > > > > > > > > > > > supported_colorspaces is just what the driver/hardware is > > > > > > > > > > > able to set > > > > > > > > > > > but doesn't actually indicate that the sink supports it. > > > > > > > > > > > > > > > > > > > > > > So the only way to enable bt2020 is by checking if the > > > > > > > > > > > sink supports > > > > > > > > > > > both RGB and YUV variants because both could be used by > > > > > > > > > > > the driver. > > > > > > > > > > > Not great at all. Something to remember for the new > > > > > > > > > > > property. > > > > > > > > > > > > > > > > > > > > Hmm. I wonder if that's even legal... Looks like maybe it > > > > > > > > > > is since I can't immediately spot anything in CTA-861 to > > > > > > > > > > forbid it :/ > > > > > > > > > > > > > > > > > > Wouldn't the driver do the same EDID check before choosing > > > > > > > > > whether it > > > > > > > > > uses RGB or YCbCr signalling? > > > > > > > > > > > > > > > > I suppose it could. The modeset would then fail, which is > > > > > > > > perhaps > > > > > > > > > > > > > > Could? What are they missing? > > > > > > > > > > > > The fact that the new property that also affects the rgb->ycbcr > > > > > > matrix > > > > > > doesn't even exist? > > > > > > > > > > I think the question was about the current Colorspace property. > > > > > > Yes. > > > > > > We need to be able to set ColourPrimaries infoframe field for the sink. > > > Only userspace knows what ColourPrimaries it uses, and the driver has > > > no need to care at all, other than tell the sink what we have. > > > > > > When a driver chooses to use YCbCr, it needs to use the > > > MatrixCoefficients the sink expects. > > > > > > If we send the infoframe to the sink telling the signal uses BT.2020 > > &g
Re: [PATCH v3 09/17] drm/amd/display: Register Colorspace property for DP and HDMI
On Fri, Mar 17, 2023 at 10:53:35AM +0200, Pekka Paalanen wrote: > On Fri, 17 Mar 2023 01:01:38 +0200 > Ville Syrjälä wrote: > > > On Thu, Mar 16, 2023 at 10:13:54PM +0100, Sebastian Wick wrote: > > > On Thu, Mar 16, 2023 at 1:35 PM Ville Syrjälä > > > wrote: > > > > > > > > On Thu, Mar 16, 2023 at 01:34:49PM +0200, Pekka Paalanen wrote: > > > > > On Thu, 16 Mar 2023 12:47:51 +0200 > > > > > Ville Syrjälä wrote: > > > > > > > > > > > On Thu, Mar 16, 2023 at 12:07:01PM +0200, Pekka Paalanen wrote: > > > > > > > On Thu, 16 Mar 2023 11:50:27 +0200 > > > > > > > Ville Syrjälä wrote: > > > > > > > > > > > > > > > On Thu, Mar 16, 2023 at 01:37:24AM +0100, Sebastian Wick wrote: > > > > > > > > > > > > > > > > > On Tue, Mar 7, 2023 at 4:12 PM Harry Wentland > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > We want compositors to be able to set the output > > > > > > > > > > colorspace on DP and HDMI outputs, based on the > > > > > > > > > > caps reported from the receiver via EDID. > > > > > > > > > > > > > > > > > > About that... The documentation says that user space has to > > > > > > > > > check the > > > > > > > > > EDID for what the sink actually supports. So whatever is in > > > > > > > > > supported_colorspaces is just what the driver/hardware is > > > > > > > > > able to set > > > > > > > > > but doesn't actually indicate that the sink supports it. > > > > > > > > > > > > > > > > > > So the only way to enable bt2020 is by checking if the sink > > > > > > > > > supports > > > > > > > > > both RGB and YUV variants because both could be used by the > > > > > > > > > driver. > > > > > > > > > Not great at all. Something to remember for the new property. > > > > > > > > > > > > > > > > > > > > > > > > > Hmm. I wonder if that's even legal... Looks like maybe it > > > > > > > > is since I can't immediately spot anything in CTA-861 to > > > > > > > > forbid it :/ > > > > > > > > > > > > > > Wouldn't the driver do the same EDID check before choosing > > > > > > > whether it > > > > > > > uses RGB or YCbCr signalling? > > > > > > > > > > > > I suppose it could. The modeset would then fail, which is perhaps > > > > > > > > > > Could? What are they missing? > > > > > > > > The fact that the new property that also affects the rgb->ycbcr matrix > > > > doesn't even exist? > > > > > > I think the question was about the current Colorspace property. > > Yes. > > We need to be able to set ColourPrimaries infoframe field for the sink. > Only userspace knows what ColourPrimaries it uses, and the driver has > no need to care at all, other than tell the sink what we have. > > When a driver chooses to use YCbCr, it needs to use the > MatrixCoefficients the sink expects. > > If we send the infoframe to the sink telling the signal uses BT.2020 > ColourPrimaries, does that same bit pattern also tell the sink we are > using the BT.2020 NCL MatrixCoefficients if the driver chooses YCbCr? > > Do drivers actually use BT.2020 NCL MatrixCoefficients in that case? No. I think I've repeated this same line a thousand times already: The current colorspace property *only* affects the infoframe/msa/sdp, nothing else. > > If they don't, then YCbCr BT.2020 has never worked, which is another > nail in the coffin for "Colorspace" property. That is the same nail we've been talking about all along I thought. > But it still means that > RGB BT.2020 may have worked correctly, and then drivers would regress > if they started picking YCbCr for any reason where they previously used > RGB. The policy has been to use RGB if at all possible. Only falling back to YCbCr 4:2:0 if absolutely necessary (eg. EDID says 4:2:0 must be used, or there's not enough bandwidth for 4:4:4, etc.). If the behaviour sudd
Re: [PATCH v3 09/17] drm/amd/display: Register Colorspace property for DP and HDMI
On Thu, Mar 16, 2023 at 10:13:54PM +0100, Sebastian Wick wrote: > On Thu, Mar 16, 2023 at 1:35 PM Ville Syrjälä > wrote: > > > > On Thu, Mar 16, 2023 at 01:34:49PM +0200, Pekka Paalanen wrote: > > > On Thu, 16 Mar 2023 12:47:51 +0200 > > > Ville Syrjälä wrote: > > > > > > > On Thu, Mar 16, 2023 at 12:07:01PM +0200, Pekka Paalanen wrote: > > > > > On Thu, 16 Mar 2023 11:50:27 +0200 > > > > > Ville Syrjälä wrote: > > > > > > > > > > > On Thu, Mar 16, 2023 at 01:37:24AM +0100, Sebastian Wick wrote: > > > > > > > On Tue, Mar 7, 2023 at 4:12 PM Harry Wentland > > > > > > > wrote: > > > > > > > > > > > > > > > > We want compositors to be able to set the output > > > > > > > > colorspace on DP and HDMI outputs, based on the > > > > > > > > caps reported from the receiver via EDID. > > > > > > > > > > > > > > About that... The documentation says that user space has to check > > > > > > > the > > > > > > > EDID for what the sink actually supports. So whatever is in > > > > > > > supported_colorspaces is just what the driver/hardware is able to > > > > > > > set > > > > > > > but doesn't actually indicate that the sink supports it. > > > > > > > > > > > > > > So the only way to enable bt2020 is by checking if the sink > > > > > > > supports > > > > > > > both RGB and YUV variants because both could be used by the > > > > > > > driver. > > > > > > > Not great at all. Something to remember for the new property. > > > > > > > > > > > > Hmm. I wonder if that's even legal... Looks like maybe it > > > > > > is since I can't immediately spot anything in CTA-861 to > > > > > > forbid it :/ > > > > > > > > > > Wouldn't the driver do the same EDID check before choosing whether it > > > > > uses RGB or YCbCr signalling? > > > > > > > > I suppose it could. The modeset would then fail, which is perhaps > > > > > > Could? What are they missing? > > > > The fact that the new property that also affects the rgb->ycbcr matrix > > doesn't even exist? > > I think the question was about the current Colorspace property. > > > > > > > I mean, drivers are already automatically choosing between RGB and YCbCr > > > signalling based on e.g. available bandwidth. Surely they already will > > > not attempt to send a signal format to a monitor that does not say it > > > supports that? > > That's exactly what they do. The drivers don't check the EDID for the > colorimetry the sink supports and the responsibility is punted off to > user space. > > > > > We just signal default/bt.709 colorimetry. There is nothing to > > check for those IIRC. > > You do support bt.2020, no? Not for rgb->ycbcr conversion. > > > > > > > > not a huge issue, except maybe for suspend+resume if we fail in > > > > the resume path. Although I guess the EDID/etc. should not yet > > > > be refreshed at that point so if the modeset worked before suspend > > > > resume should be able to restore it without failures. > > > > > > I assumed that if a monitor can be driven, and it supports any BT2020 > > > format, then it always supports the BT2020 format it is being driven > > > in (RGB vs. YCbCr flavors). Bad assumption? > > > > I didn't spot any rule that both must be there. But didn't look > > too hard either. > > Didn't see anything like that either and I looked a bit harder as well. > > > > > -- > > Ville Syrjälä > > Intel > > -- Ville Syrjälä Intel
Re: [PATCH v3 09/17] drm/amd/display: Register Colorspace property for DP and HDMI
On Thu, Mar 16, 2023 at 01:34:49PM +0200, Pekka Paalanen wrote: > On Thu, 16 Mar 2023 12:47:51 +0200 > Ville Syrjälä wrote: > > > On Thu, Mar 16, 2023 at 12:07:01PM +0200, Pekka Paalanen wrote: > > > On Thu, 16 Mar 2023 11:50:27 +0200 > > > Ville Syrjälä wrote: > > > > > > > On Thu, Mar 16, 2023 at 01:37:24AM +0100, Sebastian Wick wrote: > > > > > On Tue, Mar 7, 2023 at 4:12 PM Harry Wentland > > > > > wrote: > > > > > > > > > > > > We want compositors to be able to set the output > > > > > > colorspace on DP and HDMI outputs, based on the > > > > > > caps reported from the receiver via EDID. > > > > > > > > > > About that... The documentation says that user space has to check the > > > > > EDID for what the sink actually supports. So whatever is in > > > > > supported_colorspaces is just what the driver/hardware is able to set > > > > > but doesn't actually indicate that the sink supports it. > > > > > > > > > > So the only way to enable bt2020 is by checking if the sink supports > > > > > both RGB and YUV variants because both could be used by the driver. > > > > > Not great at all. Something to remember for the new property. > > > > > > > > Hmm. I wonder if that's even legal... Looks like maybe it > > > > is since I can't immediately spot anything in CTA-861 to > > > > forbid it :/ > > > > > > Wouldn't the driver do the same EDID check before choosing whether it > > > uses RGB or YCbCr signalling? > > > > I suppose it could. The modeset would then fail, which is perhaps > > Could? What are they missing? The fact that the new property that also affects the rgb->ycbcr matrix doesn't even exist? > > I mean, drivers are already automatically choosing between RGB and YCbCr > signalling based on e.g. available bandwidth. Surely they already will > not attempt to send a signal format to a monitor that does not say it > supports that? We just signal default/bt.709 colorimetry. There is nothing to check for those IIRC. > > > not a huge issue, except maybe for suspend+resume if we fail in > > the resume path. Although I guess the EDID/etc. should not yet > > be refreshed at that point so if the modeset worked before suspend > > resume should be able to restore it without failures. > > I assumed that if a monitor can be driven, and it supports any BT2020 > format, then it always supports the BT2020 format it is being driven > in (RGB vs. YCbCr flavors). Bad assumption? I didn't spot any rule that both must be there. But didn't look too hard either. -- Ville Syrjälä Intel
Re: [PATCH v3 09/17] drm/amd/display: Register Colorspace property for DP and HDMI
On Thu, Mar 16, 2023 at 12:07:01PM +0200, Pekka Paalanen wrote:
> On Thu, 16 Mar 2023 11:50:27 +0200
> Ville Syrjälä wrote:
>
> > On Thu, Mar 16, 2023 at 01:37:24AM +0100, Sebastian Wick wrote:
> > > On Tue, Mar 7, 2023 at 4:12 PM Harry Wentland
> > > wrote:
> > > >
> > > > We want compositors to be able to set the output
> > > > colorspace on DP and HDMI outputs, based on the
> > > > caps reported from the receiver via EDID.
> > >
> > > About that... The documentation says that user space has to check the
> > > EDID for what the sink actually supports. So whatever is in
> > > supported_colorspaces is just what the driver/hardware is able to set
> > > but doesn't actually indicate that the sink supports it.
> > >
> > > So the only way to enable bt2020 is by checking if the sink supports
> > > both RGB and YUV variants because both could be used by the driver.
> > > Not great at all. Something to remember for the new property.
> >
> > Hmm. I wonder if that's even legal... Looks like maybe it
> > is since I can't immediately spot anything in CTA-861 to
> > forbid it :/
>
> Wouldn't the driver do the same EDID check before choosing whether it
> uses RGB or YCbCr signalling?
I suppose it could. The modeset would then fail, which is perhaps
not a huge issue, except maybe for suspend+resume if we fail in
the resume path. Although I guess the EDID/etc. should not yet
be refreshed at that point so if the modeset worked before suspend
resume should be able to restore it without failures.
>
> So if EDID says only one of them is supported, userspace should be
> confident that that is the BT2020 mode the driver will match?
>
>
> Thanks,
> pq
>
> >
> > >
> > > > Signed-off-by: Harry Wentland
> > > > Cc: Pekka Paalanen
> > > > Cc: Sebastian Wick
> > > > Cc: vitaly.pros...@amd.com
> > > > Cc: Joshua Ashton
> > > > Cc: dri-de...@lists.freedesktop.org
> > > > Cc: amd-gfx@lists.freedesktop.org
> > > > Reviewed-By: Joshua Ashton
> > > > ---
> > > > drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 15 +++
> > > > 1 file changed, 15 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > > > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > > > index f91b2ea13d96..2d883c6dae90 100644
> > > > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > > > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > > > @@ -7184,6 +7184,12 @@ static int amdgpu_dm_connector_get_modes(struct
> > > > drm_connector *connector)
> > > > return amdgpu_dm_connector->num_modes;
> > > > }
> > > >
> > > > +static const u32 supported_colorspaces =
> > > > + BIT(DRM_MODE_COLORIMETRY_BT709_YCC) |
> > > > + BIT(DRM_MODE_COLORIMETRY_OPRGB) |
> > > > + BIT(DRM_MODE_COLORIMETRY_BT2020) |
> > > > + BIT(DRM_MODE_COLORIMETRY_BT2020_DEPRECATED);
> > > > +
> > > > void amdgpu_dm_connector_init_helper(struct amdgpu_display_manager *dm,
> > > > struct amdgpu_dm_connector
> > > > *aconnector,
> > > > int connector_type,
> > > > @@ -7264,6 +7270,15 @@ void amdgpu_dm_connector_init_helper(struct
> > > > amdgpu_display_manager *dm,
> > > > adev->mode_info.abm_level_property, 0);
> > > > }
> > > >
> > > > + if (connector_type == DRM_MODE_CONNECTOR_HDMIA) {
> > > > + if
> > > > (!drm_mode_create_hdmi_colorspace_property(&aconnector->base,
> > > > supported_colorspaces))
> > > > +
> > > > drm_connector_attach_colorspace_property(&aconnector->base);
> > > > + } else if (connector_type == DRM_MODE_CONNECTOR_DisplayPort ||
> > > > + connector_type == DRM_MODE_CONNECTOR_eDP) {
> > > > + if
> > > > (!drm_mode_create_dp_colorspace_property(&aconnector->base,
> > > > supported_colorspaces))
> > > > +
> > > > drm_connector_attach_colorspace_property(&aconnector->base);
> > > > + }
> > > > +
> > > > if (connector_type == DRM_MODE_CONNECTOR_HDMIA ||
> > > > connector_type == DRM_MODE_CONNECTOR_DisplayPort ||
> > > > connector_type == DRM_MODE_CONNECTOR_eDP) {
> > > > --
> > > > 2.39.2
> > > >
> >
>
--
Ville Syrjälä
Intel
Re: [PATCH v3 09/17] drm/amd/display: Register Colorspace property for DP and HDMI
On Thu, Mar 16, 2023 at 01:37:24AM +0100, Sebastian Wick wrote:
> On Tue, Mar 7, 2023 at 4:12 PM Harry Wentland wrote:
> >
> > We want compositors to be able to set the output
> > colorspace on DP and HDMI outputs, based on the
> > caps reported from the receiver via EDID.
>
> About that... The documentation says that user space has to check the
> EDID for what the sink actually supports. So whatever is in
> supported_colorspaces is just what the driver/hardware is able to set
> but doesn't actually indicate that the sink supports it.
>
> So the only way to enable bt2020 is by checking if the sink supports
> both RGB and YUV variants because both could be used by the driver.
> Not great at all. Something to remember for the new property.
Hmm. I wonder if that's even legal... Looks like maybe it
is since I can't immediately spot anything in CTA-861 to
forbid it :/
>
> > Signed-off-by: Harry Wentland
> > Cc: Pekka Paalanen
> > Cc: Sebastian Wick
> > Cc: vitaly.pros...@amd.com
> > Cc: Joshua Ashton
> > Cc: dri-de...@lists.freedesktop.org
> > Cc: amd-gfx@lists.freedesktop.org
> > Reviewed-By: Joshua Ashton
> > ---
> > drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 15 +++
> > 1 file changed, 15 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > index f91b2ea13d96..2d883c6dae90 100644
> > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > @@ -7184,6 +7184,12 @@ static int amdgpu_dm_connector_get_modes(struct
> > drm_connector *connector)
> > return amdgpu_dm_connector->num_modes;
> > }
> >
> > +static const u32 supported_colorspaces =
> > + BIT(DRM_MODE_COLORIMETRY_BT709_YCC) |
> > + BIT(DRM_MODE_COLORIMETRY_OPRGB) |
> > + BIT(DRM_MODE_COLORIMETRY_BT2020) |
> > + BIT(DRM_MODE_COLORIMETRY_BT2020_DEPRECATED);
> > +
> > void amdgpu_dm_connector_init_helper(struct amdgpu_display_manager *dm,
> > struct amdgpu_dm_connector *aconnector,
> > int connector_type,
> > @@ -7264,6 +7270,15 @@ void amdgpu_dm_connector_init_helper(struct
> > amdgpu_display_manager *dm,
> > adev->mode_info.abm_level_property, 0);
> > }
> >
> > + if (connector_type == DRM_MODE_CONNECTOR_HDMIA) {
> > + if
> > (!drm_mode_create_hdmi_colorspace_property(&aconnector->base,
> > supported_colorspaces))
> > +
> > drm_connector_attach_colorspace_property(&aconnector->base);
> > + } else if (connector_type == DRM_MODE_CONNECTOR_DisplayPort ||
> > + connector_type == DRM_MODE_CONNECTOR_eDP) {
> > + if
> > (!drm_mode_create_dp_colorspace_property(&aconnector->base,
> > supported_colorspaces))
> > +
> > drm_connector_attach_colorspace_property(&aconnector->base);
> > + }
> > +
> > if (connector_type == DRM_MODE_CONNECTOR_HDMIA ||
> > connector_type == DRM_MODE_CONNECTOR_DisplayPort ||
> > connector_type == DRM_MODE_CONNECTOR_eDP) {
> > --
> > 2.39.2
> >
--
Ville Syrjälä
Intel
Re: [PATCH] drm/amd/display: use a more accurate check in dm_helpers_dp_read_dpcd()
On Fri, Mar 10, 2023 at 07:48:04PM +0200, Ville Syrjälä wrote: > On Thu, Mar 09, 2023 at 04:30:27PM -0500, Hamza Mahfooz wrote: > > We should be checking if drm_dp_dpcd_read() returns the size that we are > > asking it to read instead of just checking if it is greater than zero. > > Also, we should WARN_ON() here since this condition is only ever met, if > > there is an issue worth investigating. So, compare the return value of > > drm_dp_dpcd_read() to size and WARN_ON() if they aren't equal. > > > > Signed-off-by: Hamza Mahfooz > > --- > > drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c > > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c > > index 8d598b322e5b..ed2ed7b1d869 100644 > > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c > > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c > > @@ -511,8 +511,8 @@ bool dm_helpers_dp_read_dpcd( > > return false; > > } > > > > - return drm_dp_dpcd_read(&aconnector->dm_dp_aux.aux, address, > > - data, size) > 0; > > + return !WARN_ON(drm_dp_dpcd_read(&aconnector->dm_dp_aux.aux, address, > > +data, size) != size); > > Just FYI there are devices out there that violate the DP spec and reads > from specific DPCD registers simply fail instead of returning the > expected 0. And of course anyone can yank the cable anytime, so in fact pretty much any DPCD read can fail. -- Ville Syrjälä Intel
Re: [PATCH] drm/amd/display: use a more accurate check in dm_helpers_dp_read_dpcd()
On Thu, Mar 09, 2023 at 04:30:27PM -0500, Hamza Mahfooz wrote: > We should be checking if drm_dp_dpcd_read() returns the size that we are > asking it to read instead of just checking if it is greater than zero. > Also, we should WARN_ON() here since this condition is only ever met, if > there is an issue worth investigating. So, compare the return value of > drm_dp_dpcd_read() to size and WARN_ON() if they aren't equal. > > Signed-off-by: Hamza Mahfooz > --- > drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c > index 8d598b322e5b..ed2ed7b1d869 100644 > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c > @@ -511,8 +511,8 @@ bool dm_helpers_dp_read_dpcd( > return false; > } > > - return drm_dp_dpcd_read(&aconnector->dm_dp_aux.aux, address, > - data, size) > 0; > + return !WARN_ON(drm_dp_dpcd_read(&aconnector->dm_dp_aux.aux, address, > + data, size) != size); Just FYI there are devices out there that violate the DP spec and reads from specific DPCD registers simply fail instead of returning the expected 0. > } > > bool dm_helpers_dp_write_dpcd( > -- > 2.39.2 -- Ville Syrjälä Intel
Re: [PATCH v3 03/17] drm/connector: Deprecate split for BT.2020 in drm_colorspace enum
On Thu, Mar 09, 2023 at 02:05:55AM +0100, Sebastian Wick wrote:
> On Wed, Mar 8, 2023 at 10:09 AM Pekka Paalanen wrote:
> >
> > On Tue, 7 Mar 2023 10:10:53 -0500
> > Harry Wentland wrote:
> >
> > > From: Joshua Ashton
> > >
> > > Userspace has no way of controlling or knowing the pixel encoding
> > > currently, so there is no way for it to ever get the right values here.
> > >
> > > When we do add pixel_encoding control from userspace,we can pick the
> > > right value for the colorimetry packet based on the
> > > pixel_encoding + the colorspace.
> > >
> > > Let's deprecate these values, and have one BT.2020 colorspace entry
> > > that userspace can use.
> > >
> > > v2:
> > > - leave CYCC alone for now; it serves a purpose
> > > - leave BT2020_RGB the new default BT2020
> > >
> > > Signed-off-by: Joshua Ashton
> > > Signed-off-by: Harry Wentland
> > > Reviewed-by: Harry Wentland
> > >
> > > Cc: Pekka Paalanen
> > > Cc: Sebastian Wick
> > > Cc: vitaly.pros...@amd.com
> > > Cc: Uma Shankar
> > > Cc: Ville Syrjälä
> > > Cc: Joshua Ashton
> > > Cc: dri-de...@lists.freedesktop.org
> > > Cc: amd-gfx@lists.freedesktop.org
> > > ---
> > > drivers/gpu/drm/display/drm_hdmi_helper.c | 7 +++
> > > drivers/gpu/drm/drm_connector.c | 8
> > > drivers/gpu/drm/i915/display/intel_dp.c | 14 +++---
> > > include/drm/drm_connector.h | 15 +--
> > > 4 files changed, 23 insertions(+), 21 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/display/drm_hdmi_helper.c
> > > b/drivers/gpu/drm/display/drm_hdmi_helper.c
> > > index faf5e9efa7d3..05a0d03ffcda 100644
> > > --- a/drivers/gpu/drm/display/drm_hdmi_helper.c
> > > +++ b/drivers/gpu/drm/display/drm_hdmi_helper.c
> > > @@ -97,8 +97,7 @@ EXPORT_SYMBOL(drm_hdmi_infoframe_set_hdr_metadata);
> > > #define HDMI_COLORIMETRY_OPYCC_601 (C(3) | EC(3) | ACE(0))
> > > #define HDMI_COLORIMETRY_OPRGB (C(3) | EC(4) |
> > > ACE(0))
> > > #define HDMI_COLORIMETRY_BT2020_CYCC (C(3) | EC(5) | ACE(0))
> > > -#define HDMI_COLORIMETRY_BT2020_RGB (C(3) | EC(6) | ACE(0))
> > > -#define HDMI_COLORIMETRY_BT2020_YCC (C(3) | EC(6) | ACE(0))
> > > +#define HDMI_COLORIMETRY_BT2020 (C(3) | EC(6) |
> > > ACE(0))
> > > #define HDMI_COLORIMETRY_DCI_P3_RGB_D65 (C(3) | EC(7) |
> > > ACE(0))
> > > #define HDMI_COLORIMETRY_DCI_P3_RGB_THEATER (C(3) | EC(7) | ACE(1))
> > >
> > > @@ -112,8 +111,8 @@ static const u32 hdmi_colorimetry_val[] = {
> > > [DRM_MODE_COLORIMETRY_OPYCC_601] = HDMI_COLORIMETRY_OPYCC_601,
> > > [DRM_MODE_COLORIMETRY_OPRGB] = HDMI_COLORIMETRY_OPRGB,
> > > [DRM_MODE_COLORIMETRY_BT2020_CYCC] = HDMI_COLORIMETRY_BT2020_CYCC,
> > > - [DRM_MODE_COLORIMETRY_BT2020_RGB] = HDMI_COLORIMETRY_BT2020_RGB,
> > > - [DRM_MODE_COLORIMETRY_BT2020_YCC] = HDMI_COLORIMETRY_BT2020_YCC,
> > > + [DRM_MODE_COLORIMETRY_BT2020_DEPRECATED] = HDMI_COLORIMETRY_BT2020,
> > > + [DRM_MODE_COLORIMETRY_BT2020] = HDMI_COLORIMETRY_BT2020,
> > > };
> > >
> > > #undef C
> > > diff --git a/drivers/gpu/drm/drm_connector.c
> > > b/drivers/gpu/drm/drm_connector.c
> > > index 61c29ce74b03..fe7eab15f727 100644
> > > --- a/drivers/gpu/drm/drm_connector.c
> > > +++ b/drivers/gpu/drm/drm_connector.c
> > > @@ -1031,9 +1031,9 @@ static const struct drm_prop_enum_list
> > > hdmi_colorspaces[] = {
> > > /* Colorimetry based on ITU-R BT.2020 */
> > > { DRM_MODE_COLORIMETRY_BT2020_CYCC, "BT2020_CYCC" },
> > > /* Colorimetry based on ITU-R BT.2020 */
> > > - { DRM_MODE_COLORIMETRY_BT2020_RGB, "BT2020_RGB" },
> > > + { DRM_MODE_COLORIMETRY_BT2020, "BT2020" },
> > > /* Colorimetry based on ITU-R BT.2020 */
> > > - { DRM_MODE_COLORIMETRY_BT2020_YCC, "BT2020_YCC" },
> > > + { DRM_MODE_COLORIMETRY_BT2020_DEPRECATED, "BT2020_DEPRECATED" },
> > > /* Added as part of Additional Colorimetry Extension in 861.G */
> > > { DRM_MODE_COLORIMETRY_DCI_P3_RGB_D65, "DCI-P3_RGB_D65" },
> > > { DRM_MODE_COLORIMETRY_DCI_P3_RGB_THEATER, "DCI-P3_RGB_Theater" },
> >
Re: [PATCH 3/3] drm/connector: Deprecate split for BT.2020 in drm_colorspace enum
On Wed, Feb 15, 2023 at 12:01:25PM +0200, Pekka Paalanen wrote: > On Tue, 14 Feb 2023 22:10:35 +0200 > Ville Syrjälä wrote: > > > On Tue, Feb 14, 2023 at 08:45:00PM +0100, Sebastian Wick wrote: > > ... > > > > We also have to figure out how a user space which doesn't > > > know about the new property behaves when another client has set that > > > property. If any property which currently might change the pixel > > > values is used, we can't expose the entire color pipeline because the > > > kernel might have to use some element in it to achieve its magic > > > conversion. So essentially you already have this hard device between > > > "old" and "new" and you can't use the new stuff incrementally. > > > > That problem exists with any new property. Old userspace and new > > userspace may interact badly enought that nothing works right. > > In that sense I think these props might even be pretty mundane > > as the worst you might get from setting the infoframe wrong is > > perhaps wrong colors on your display. > > > > To solve that particular problem there has been talk (for years) > > about some kind of "reset all" knob to make sure everything is > > at a safe default value. I have a feeling there was even some > > kind of semi-real proposal in recent times, but maybe I imgained > > it? > > I've been talking about that too, but I think it all collapsed into > "let's just fix all KMS apps to always set all KMS properties" which > results in patches like > https://gitlab.freedesktop.org/wayland/weston/-/merge_requests/952 That requires some knowledge about the property in question to pick the value. I think for some prop types (enums at least) we could guarantee that the first value is always the safe default, but for eg. range properties there is no way to know. So doing that fully blind is not possible atm. I guess one option might be to include a "reset value" in the props somehow, and just have everyclient set all unknown props to that. But there are of course other options too (reset flag to atomic ioctl, etc.). > > It does not seem to be a serious enough problem for anyone to put in > the work. And why would it be, when you can easily fix it in your own > project like that Weston example. The Weston example is not even > representative, because I did it before I saw any real problems. > > Other musings have been in the direction that maybe logind (since it > opens DRM devices for you) should save the full KMS state on the very > first open after a reboot, and then KMS applications can ask logind > what the boot-up state was. This is a variation of "save all KMS state > from the moment you launch, and use that as the base if you ever let > something else touch KMS in between". > > You also never see the problem to begin with, if you never let > something else touch KMS in between, so that already makes the problem > rare outside of the tiny set of compositor developers. Yeah, it's a pretty rare problem so not much interest I guess. -- Ville Syrjälä Intel
Re: [PATCH 3/3] drm/connector: Deprecate split for BT.2020 in drm_colorspace enum
On Tue, Feb 14, 2023 at 10:18:49PM +0100, Sebastian Wick wrote: > On Tue, Feb 14, 2023 at 9:10 PM Ville Syrjälä > wrote: > > > > On Tue, Feb 14, 2023 at 08:45:00PM +0100, Sebastian Wick wrote: > > > On Tue, Feb 14, 2023 at 5:57 PM Harry Wentland > > > wrote: > > > > > > > > > > > > > > > > On 2/14/23 10:49, Sebastian Wick wrote: > > > > > On Fri, Feb 3, 2023 at 5:00 PM Ville Syrjälä > > > > > wrote: > > > > >> > > > > >> On Fri, Feb 03, 2023 at 10:24:52AM -0500, Harry Wentland wrote: > > > > >>> > > > > >>> > > > > >>> On 2/3/23 10:19, Ville Syrjälä wrote: > > > > >>>> On Fri, Feb 03, 2023 at 09:39:42AM -0500, Harry Wentland wrote: > > > > >>>>> > > > > >>>>> > > > > >>>>> On 2/3/23 07:59, Sebastian Wick wrote: > > > > >>>>>> On Fri, Feb 3, 2023 at 11:40 AM Ville Syrjälä > > > > >>>>>> wrote: > > > > >>>>>>> > > > > >>>>>>> On Fri, Feb 03, 2023 at 02:07:44AM +, Joshua Ashton wrote: > > > > >>>>>>>> Userspace has no way of controlling or knowing the pixel > > > > >>>>>>>> encoding > > > > >>>>>>>> currently, so there is no way for it to ever get the right > > > > >>>>>>>> values here. > > > > >>>>>>> > > > > >>>>>>> That applies to a lot of the other values as well (they are > > > > >>>>>>> explicitly RGB or YCC). The idea was that this property sets the > > > > >>>>>>> infoframe/MSA/SDP value exactly, and other properties should be > > > > >>>>>>> added to for use userspace to control the pixel > > > > >>>>>>> encoding/colorspace > > > > >>>>>>> conversion(if desired, or userspace just makes sure to > > > > >>>>>>> directly feed in correct kind of data). > > > > >>>>>> > > > > >>>>>> I'm all for getting userspace control over pixel encoding but > > > > >>>>>> even > > > > >>>>>> then the kernel always knows which pixel encoding is selected and > > > > >>>>>> which InfoFrame has to be sent. Is there a reason why userspace > > > > >>>>>> would > > > > >>>>>> want to control the variant explicitly to the wrong value? > > > > >>>>>> > > > > >>>>> > > > > >>>>> I've asked this before but haven't seen an answer: Is there an > > > > >>>>> existing > > > > >>>>> upstream userspace project that makes use of this property (other > > > > >>>>> than > > > > >>>>> what Joshua is working on in gamescope right now)? That would > > > > >>>>> help us > > > > >>>>> understand the intent better. > > > > >>>> > > > > >>>> The intent was to control the infoframe colorimetry bits, > > > > >>>> nothing more. No idea what real userspace there was, if any. > > > > >>>> > > > > >>>>> > > > > >>>>> I don't think giving userspace explicit control over the exact > > > > >>>>> infoframe > > > > >>>>> values is the right thing to do. > > > > >>>> > > > > >>>> Only userspace knows what kind of data it's stuffing into > > > > >>>> the pixels (and/or how it configures the csc units/etc.) to > > > > >>>> generate them. > > > > >>>> > > > > >>> > > > > >>> Yes, but userspace doesn't control or know whether we drive > > > > >>> RGB or YCbCr on the wire. In fact, in some cases our driver > > > > >>> needs to fallback to YCbCr420 for bandwidth reasons. There > > > > >>> is currently no way for userspace to know that and I don't > > > > >>> think it makes sense.
Re: [PATCH 3/3] drm/connector: Deprecate split for BT.2020 in drm_colorspace enum
On Tue, Feb 14, 2023 at 08:45:00PM +0100, Sebastian Wick wrote: > On Tue, Feb 14, 2023 at 5:57 PM Harry Wentland wrote: > > > > > > > > On 2/14/23 10:49, Sebastian Wick wrote: > > > On Fri, Feb 3, 2023 at 5:00 PM Ville Syrjälä > > > wrote: > > >> > > >> On Fri, Feb 03, 2023 at 10:24:52AM -0500, Harry Wentland wrote: > > >>> > > >>> > > >>> On 2/3/23 10:19, Ville Syrjälä wrote: > > >>>> On Fri, Feb 03, 2023 at 09:39:42AM -0500, Harry Wentland wrote: > > >>>>> > > >>>>> > > >>>>> On 2/3/23 07:59, Sebastian Wick wrote: > > >>>>>> On Fri, Feb 3, 2023 at 11:40 AM Ville Syrjälä > > >>>>>> wrote: > > >>>>>>> > > >>>>>>> On Fri, Feb 03, 2023 at 02:07:44AM +, Joshua Ashton wrote: > > >>>>>>>> Userspace has no way of controlling or knowing the pixel encoding > > >>>>>>>> currently, so there is no way for it to ever get the right values > > >>>>>>>> here. > > >>>>>>> > > >>>>>>> That applies to a lot of the other values as well (they are > > >>>>>>> explicitly RGB or YCC). The idea was that this property sets the > > >>>>>>> infoframe/MSA/SDP value exactly, and other properties should be > > >>>>>>> added to for use userspace to control the pixel encoding/colorspace > > >>>>>>> conversion(if desired, or userspace just makes sure to > > >>>>>>> directly feed in correct kind of data). > > >>>>>> > > >>>>>> I'm all for getting userspace control over pixel encoding but even > > >>>>>> then the kernel always knows which pixel encoding is selected and > > >>>>>> which InfoFrame has to be sent. Is there a reason why userspace would > > >>>>>> want to control the variant explicitly to the wrong value? > > >>>>>> > > >>>>> > > >>>>> I've asked this before but haven't seen an answer: Is there an > > >>>>> existing > > >>>>> upstream userspace project that makes use of this property (other than > > >>>>> what Joshua is working on in gamescope right now)? That would help us > > >>>>> understand the intent better. > > >>>> > > >>>> The intent was to control the infoframe colorimetry bits, > > >>>> nothing more. No idea what real userspace there was, if any. > > >>>> > > >>>>> > > >>>>> I don't think giving userspace explicit control over the exact > > >>>>> infoframe > > >>>>> values is the right thing to do. > > >>>> > > >>>> Only userspace knows what kind of data it's stuffing into > > >>>> the pixels (and/or how it configures the csc units/etc.) to > > >>>> generate them. > > >>>> > > >>> > > >>> Yes, but userspace doesn't control or know whether we drive > > >>> RGB or YCbCr on the wire. In fact, in some cases our driver > > >>> needs to fallback to YCbCr420 for bandwidth reasons. There > > >>> is currently no way for userspace to know that and I don't > > >>> think it makes sense. > > >> > > >> People want that control as well for whatever reason. We've > > >> been asked to allow YCbCr 4:4:4 output many times. > > > > > > I don't really think it's a question of if we want it but rather how > > > we get there. Harry is completely right that if we would make the > > > subsampling controllable by user space instead of the kernel handling > > > it magically, user space which does not adapt to the new control won't > > > be able to light up some modes which worked before. > > > > > > > Thanks for continuing this discussion and touching on the model of how > > we get to where we want to go. > > > > > This is obviously a problem and not one we can easily fix. We would > > > need a new cap for user space to signal "I know that I can control > > > bpc, subsampling and compression to lower the bandwidth and light up > > > modes which otherwise
Re: [RFC PATCH v2 00/18] Add DRM CRTC 3D LUT interface
. > > > > > > > Maybe it makes sense to just drop the bit_depth field. > > Well, it's really interesting information for userspace, but maybe it > should have a more holistic design. Precision is a factor, when > userspace considers whether it can use KMS hardware for a conversion or > not. Unfortunately, none of the existing KMS color pipeline elements > have any information on precision IIRC, so there is more to be fixed. > > The interesting thing is the minimum guaranteed precision of each > element and the connections between them. It might be different for > pass-through vs. not. Another interesting thing is the usable value > range. > > This is probably a complex problem, so there should be no need to solve > it before a 3D LUT interface can land, given old elements already have > the issue. Yeah, I think all the precision stuff is all better handled by eg. the proposed GAMMA_MODE property or something similar. It's going to be needed for 1D LUTs as well. 1D LUTs would also need it to expose diffrent LUT sizes with different precision tradeoffs. As for the 3D LUT blob, I don't think the blob needs any strides/etc. either. I had none of that for my i915 version: https://github.com/vsyrjala/linux/commits/3dlut Just the LUT entries + blob size is sufficient. At least for cube shaped LUTs. Dunno if anyone would have a need for something else? The two things the are absolutely needed: - Position of the LUT in the pipeline. We've already seen at least two variants of this IIRC, so we'll likely need to define a unique property for each tap point. - The order the elements are stored in the blob. I didn't check if all the already known hw (amdgpu, i915, rcar-du, were there also others?) would agree on this or not. If yes, maybe just follow the hw order for all those, and if not, then I guess flip a few coins. -- Ville Syrjälä Intel
Re: [PATCH 3/3] drm/connector: Deprecate split for BT.2020 in drm_colorspace enum
On Thu, Feb 09, 2023 at 05:03:19PM +, Simon Ser wrote: > On Thursday, February 9th, 2023 at 17:38, Joshua Ashton > wrote: > > > > I mean, the strings are the uAPI, not the integers, right? > > > > Both are uAPI these days. > > Yes. The integers are uAPI, if you change them you'll break libliftoff > users. There is an old thread discussing this somewhere. The tl;dr was > that there is no use-case for exposing the same string with a different > integer, so no good reason to justify the substantial complexity of > handling this case. If people actually depend on that we should probably have tests to make sure no one breaks it by accident. -- Ville Syrjälä Intel
Re: [PATCH 3/3] drm/connector: Deprecate split for BT.2020 in drm_colorspace enum
On Wed, Feb 08, 2023 at 11:18:42AM +0200, Pekka Paalanen wrote: > On Fri, 3 Feb 2023 16:02:51 +0200 > Ville Syrjälä wrote: > > > On Fri, Feb 03, 2023 at 02:52:50PM +0100, Sebastian Wick wrote: > > > On Fri, Feb 3, 2023 at 2:35 PM Ville Syrjälä > > > wrote: > > > > > > > > On Fri, Feb 03, 2023 at 01:59:07PM +0100, Sebastian Wick wrote: > > > > > On Fri, Feb 3, 2023 at 11:40 AM Ville Syrjälä > > > > > wrote: > > > > > > > > > > > > On Fri, Feb 03, 2023 at 02:07:44AM +, Joshua Ashton wrote: > > > > > > > Userspace has no way of controlling or knowing the pixel encoding > > > > > > > currently, so there is no way for it to ever get the right values > > > > > > > here. > > > > > > > > > > > > That applies to a lot of the other values as well (they are > > > > > > explicitly RGB or YCC). The idea was that this property sets the > > > > > > infoframe/MSA/SDP value exactly, and other properties should be > > > > > > added to for use userspace to control the pixel encoding/colorspace > > > > > > conversion(if desired, or userspace just makes sure to > > > > > > directly feed in correct kind of data). > > > > > > > > > > I'm all for getting userspace control over pixel encoding but even > > > > > then the kernel always knows which pixel encoding is selected and > > > > > which InfoFrame has to be sent. Is there a reason why userspace would > > > > > want to control the variant explicitly to the wrong value? > > > > > > > > What do you mean wrong value? Userspace sets it based on what > > > > kind of data it has generated (or asked the display hardware > > > > to generate if/when we get explicit control over that part). > > > > > > Wrong in the sense of sending the YCC variant when the pixel encoding > > > is RGB for example. > > > > > > Maybe I'm missing something here but my assumption is that the kernel > > > always has to know the pixel encoding anyway. The color pipeline also > > > assumes that the pixel values are RGB. User space might be able to > > > generate YCC content but for subsampling etc the pixel encoding still > > > has to be explicitly set. > > > > The kernel doesn't really know much atm. In theory you can just > > configure the thing to do a straight passthough and put anything you > > want into your pixels. > > But it's impossible to use a YCbCr framebuffer and have that *not* > converted to RGB for the KMS color pipeline even if userspace wanted it > to be strictly pass-through, only to be converted again to YCbCr for > the cable, is it not? > > Even more so with 4:2:0. > > How could it be possible to stop the driver from doing those two > YUV-to-RGB and RGB-to-YCbCr conversions at the beginning and at the end > of the KMS color pipeline? You can stop the conversion at the start of the pipeline by using a "RGB" framebuffer. At the end of the pipe it's not possible with the current props. -- Ville Syrjälä Intel
Re: [PATCH 3/3] drm/connector: Deprecate split for BT.2020 in drm_colorspace enum
On Sat, Feb 04, 2023 at 06:09:45AM +, Joshua Ashton wrote: > > > On 2/3/23 19:34, Ville Syrjälä wrote: > > On Fri, Feb 03, 2023 at 09:25:38PM +0200, Ville Syrjälä wrote: > >> On Fri, Feb 03, 2023 at 08:56:55PM +0200, Ville Syrjälä wrote: > >>> On Fri, Feb 03, 2023 at 01:28:20PM -0500, Harry Wentland wrote: > >>>> > >>>> > >>>> On 2/3/23 11:00, Ville Syrjälä wrote: > >>>>> On Fri, Feb 03, 2023 at 10:24:52AM -0500, Harry Wentland wrote: > >>>>>> > >>>>>> > >>>>>> On 2/3/23 10:19, Ville Syrjälä wrote: > >>>>>>> On Fri, Feb 03, 2023 at 09:39:42AM -0500, Harry Wentland wrote: > >>>>>>>> > >>>>>>>> > >>>>>>>> On 2/3/23 07:59, Sebastian Wick wrote: > >>>>>>>>> On Fri, Feb 3, 2023 at 11:40 AM Ville Syrjälä > >>>>>>>>> wrote: > >>>>>>>>>> > >>>>>>>>>> On Fri, Feb 03, 2023 at 02:07:44AM +, Joshua Ashton wrote: > >>>>>>>>>>> Userspace has no way of controlling or knowing the pixel encoding > >>>>>>>>>>> currently, so there is no way for it to ever get the right values > >>>>>>>>>>> here. > >>>>>>>>>> > >>>>>>>>>> That applies to a lot of the other values as well (they are > >>>>>>>>>> explicitly RGB or YCC). The idea was that this property sets the > >>>>>>>>>> infoframe/MSA/SDP value exactly, and other properties should be > >>>>>>>>>> added to for use userspace to control the pixel encoding/colorspace > >>>>>>>>>> conversion(if desired, or userspace just makes sure to > >>>>>>>>>> directly feed in correct kind of data). > >>>>>>>>> > >>>>>>>>> I'm all for getting userspace control over pixel encoding but even > >>>>>>>>> then the kernel always knows which pixel encoding is selected and > >>>>>>>>> which InfoFrame has to be sent. Is there a reason why userspace > >>>>>>>>> would > >>>>>>>>> want to control the variant explicitly to the wrong value? > >>>>>>>>> > >>>>>>>> > >>>>>>>> I've asked this before but haven't seen an answer: Is there an > >>>>>>>> existing > >>>>>>>> upstream userspace project that makes use of this property (other > >>>>>>>> than > >>>>>>>> what Joshua is working on in gamescope right now)? That would help us > >>>>>>>> understand the intent better. > >>>>>>> > >>>>>>> The intent was to control the infoframe colorimetry bits, > >>>>>>> nothing more. No idea what real userspace there was, if any. > > Controlling the infoframe alone isn't useful at all unless you can > guarantee the wire encoding, which we cannot do. > > >>>>>>> > >>>>>>>> > >>>>>>>> I don't think giving userspace explicit control over the exact > >>>>>>>> infoframe > >>>>>>>> values is the right thing to do. > > +1 > > >>>>>>> > >>>>>>> Only userspace knows what kind of data it's stuffing into > >>>>>>> the pixels (and/or how it configures the csc units/etc.) to > >>>>>>> generate them. > >>>>>>> > >>>>>> > >>>>>> Yes, but userspace doesn't control or know whether we drive > >>>>>> RGB or YCbCr on the wire. In fact, in some cases our driver > >>>>>> needs to fallback to YCbCr420 for bandwidth reasons. There > >>>>>> is currently no way for userspace to know that and I don't > >>>>>> think it makes sense. > >>>>> > >>>>> People want that control as well for whatever reason. We've > >>>>> been asked to allow YCbCr 4:4:4 output many times. > >>>>> > >>>>> The automagic 4:2:0 fallback I think is rather fu
Re: [PATCH 3/3] drm/connector: Deprecate split for BT.2020 in drm_colorspace enum
On Fri, Feb 03, 2023 at 09:25:38PM +0200, Ville Syrjälä wrote: > On Fri, Feb 03, 2023 at 08:56:55PM +0200, Ville Syrjälä wrote: > > On Fri, Feb 03, 2023 at 01:28:20PM -0500, Harry Wentland wrote: > > > > > > > > > On 2/3/23 11:00, Ville Syrjälä wrote: > > > > On Fri, Feb 03, 2023 at 10:24:52AM -0500, Harry Wentland wrote: > > > >> > > > >> > > > >> On 2/3/23 10:19, Ville Syrjälä wrote: > > > >>> On Fri, Feb 03, 2023 at 09:39:42AM -0500, Harry Wentland wrote: > > > >>>> > > > >>>> > > > >>>> On 2/3/23 07:59, Sebastian Wick wrote: > > > >>>>> On Fri, Feb 3, 2023 at 11:40 AM Ville Syrjälä > > > >>>>> wrote: > > > >>>>>> > > > >>>>>> On Fri, Feb 03, 2023 at 02:07:44AM +, Joshua Ashton wrote: > > > >>>>>>> Userspace has no way of controlling or knowing the pixel encoding > > > >>>>>>> currently, so there is no way for it to ever get the right values > > > >>>>>>> here. > > > >>>>>> > > > >>>>>> That applies to a lot of the other values as well (they are > > > >>>>>> explicitly RGB or YCC). The idea was that this property sets the > > > >>>>>> infoframe/MSA/SDP value exactly, and other properties should be > > > >>>>>> added to for use userspace to control the pixel encoding/colorspace > > > >>>>>> conversion(if desired, or userspace just makes sure to > > > >>>>>> directly feed in correct kind of data). > > > >>>>> > > > >>>>> I'm all for getting userspace control over pixel encoding but even > > > >>>>> then the kernel always knows which pixel encoding is selected and > > > >>>>> which InfoFrame has to be sent. Is there a reason why userspace > > > >>>>> would > > > >>>>> want to control the variant explicitly to the wrong value? > > > >>>>> > > > >>>> > > > >>>> I've asked this before but haven't seen an answer: Is there an > > > >>>> existing > > > >>>> upstream userspace project that makes use of this property (other > > > >>>> than > > > >>>> what Joshua is working on in gamescope right now)? That would help us > > > >>>> understand the intent better. > > > >>> > > > >>> The intent was to control the infoframe colorimetry bits, > > > >>> nothing more. No idea what real userspace there was, if any. > > > >>> > > > >>>> > > > >>>> I don't think giving userspace explicit control over the exact > > > >>>> infoframe > > > >>>> values is the right thing to do. > > > >>> > > > >>> Only userspace knows what kind of data it's stuffing into > > > >>> the pixels (and/or how it configures the csc units/etc.) to > > > >>> generate them. > > > >>> > > > >> > > > >> Yes, but userspace doesn't control or know whether we drive > > > >> RGB or YCbCr on the wire. In fact, in some cases our driver > > > >> needs to fallback to YCbCr420 for bandwidth reasons. There > > > >> is currently no way for userspace to know that and I don't > > > >> think it makes sense. > > > > > > > > People want that control as well for whatever reason. We've > > > > been asked to allow YCbCr 4:4:4 output many times. > > > > > > > > The automagic 4:2:0 fallback I think is rather fundementally > > > > incompatible with fancy color management. How would we even > > > > know whether to use eg. BT.2020 vs. BT.709 matrix? In i915 > > > > that stuff is just always BT.709 limited range, no questions > > > > asked. > > > > > > > > > > We use what we're telling the display, i.e., the value in the > > > colorspace property. That way we know whether to use a BT.2020 > > > or BT.709 matrix. > > > > And given how these things have gone in the past I think > > that is likey to bite someone at in the future. Also not > > what this property wa
Re: [PATCH 3/3] drm/connector: Deprecate split for BT.2020 in drm_colorspace enum
On Fri, Feb 03, 2023 at 08:56:55PM +0200, Ville Syrjälä wrote: > On Fri, Feb 03, 2023 at 01:28:20PM -0500, Harry Wentland wrote: > > > > > > On 2/3/23 11:00, Ville Syrjälä wrote: > > > On Fri, Feb 03, 2023 at 10:24:52AM -0500, Harry Wentland wrote: > > >> > > >> > > >> On 2/3/23 10:19, Ville Syrjälä wrote: > > >>> On Fri, Feb 03, 2023 at 09:39:42AM -0500, Harry Wentland wrote: > > >>>> > > >>>> > > >>>> On 2/3/23 07:59, Sebastian Wick wrote: > > >>>>> On Fri, Feb 3, 2023 at 11:40 AM Ville Syrjälä > > >>>>> wrote: > > >>>>>> > > >>>>>> On Fri, Feb 03, 2023 at 02:07:44AM +, Joshua Ashton wrote: > > >>>>>>> Userspace has no way of controlling or knowing the pixel encoding > > >>>>>>> currently, so there is no way for it to ever get the right values > > >>>>>>> here. > > >>>>>> > > >>>>>> That applies to a lot of the other values as well (they are > > >>>>>> explicitly RGB or YCC). The idea was that this property sets the > > >>>>>> infoframe/MSA/SDP value exactly, and other properties should be > > >>>>>> added to for use userspace to control the pixel encoding/colorspace > > >>>>>> conversion(if desired, or userspace just makes sure to > > >>>>>> directly feed in correct kind of data). > > >>>>> > > >>>>> I'm all for getting userspace control over pixel encoding but even > > >>>>> then the kernel always knows which pixel encoding is selected and > > >>>>> which InfoFrame has to be sent. Is there a reason why userspace would > > >>>>> want to control the variant explicitly to the wrong value? > > >>>>> > > >>>> > > >>>> I've asked this before but haven't seen an answer: Is there an existing > > >>>> upstream userspace project that makes use of this property (other than > > >>>> what Joshua is working on in gamescope right now)? That would help us > > >>>> understand the intent better. > > >>> > > >>> The intent was to control the infoframe colorimetry bits, > > >>> nothing more. No idea what real userspace there was, if any. > > >>> > > >>>> > > >>>> I don't think giving userspace explicit control over the exact > > >>>> infoframe > > >>>> values is the right thing to do. > > >>> > > >>> Only userspace knows what kind of data it's stuffing into > > >>> the pixels (and/or how it configures the csc units/etc.) to > > >>> generate them. > > >>> > > >> > > >> Yes, but userspace doesn't control or know whether we drive > > >> RGB or YCbCr on the wire. In fact, in some cases our driver > > >> needs to fallback to YCbCr420 for bandwidth reasons. There > > >> is currently no way for userspace to know that and I don't > > >> think it makes sense. > > > > > > People want that control as well for whatever reason. We've > > > been asked to allow YCbCr 4:4:4 output many times. > > > > > > The automagic 4:2:0 fallback I think is rather fundementally > > > incompatible with fancy color management. How would we even > > > know whether to use eg. BT.2020 vs. BT.709 matrix? In i915 > > > that stuff is just always BT.709 limited range, no questions > > > asked. > > > > > > > We use what we're telling the display, i.e., the value in the > > colorspace property. That way we know whether to use a BT.2020 > > or BT.709 matrix. > > And given how these things have gone in the past I think > that is likey to bite someone at in the future. Also not > what this property was meant to do nor does on any other > driver AFAIK. > > > I don't see how it's fundamentally incompatible with fancy > > color management stuff. > > > > If we start forbidding drivers from falling back to YCbCr > > (whether 4:4:4 or 4:2:0) we will break existing behavior on > > amdgpu and will see bug reports. > > The compositors could deal with that if/when they start doing > the full color management stuff. The current stuff only really > works when the kernel is all
Re: [PATCH 3/3] drm/connector: Deprecate split for BT.2020 in drm_colorspace enum
On Fri, Feb 03, 2023 at 01:28:20PM -0500, Harry Wentland wrote: > > > On 2/3/23 11:00, Ville Syrjälä wrote: > > On Fri, Feb 03, 2023 at 10:24:52AM -0500, Harry Wentland wrote: > >> > >> > >> On 2/3/23 10:19, Ville Syrjälä wrote: > >>> On Fri, Feb 03, 2023 at 09:39:42AM -0500, Harry Wentland wrote: > >>>> > >>>> > >>>> On 2/3/23 07:59, Sebastian Wick wrote: > >>>>> On Fri, Feb 3, 2023 at 11:40 AM Ville Syrjälä > >>>>> wrote: > >>>>>> > >>>>>> On Fri, Feb 03, 2023 at 02:07:44AM +, Joshua Ashton wrote: > >>>>>>> Userspace has no way of controlling or knowing the pixel encoding > >>>>>>> currently, so there is no way for it to ever get the right values > >>>>>>> here. > >>>>>> > >>>>>> That applies to a lot of the other values as well (they are > >>>>>> explicitly RGB or YCC). The idea was that this property sets the > >>>>>> infoframe/MSA/SDP value exactly, and other properties should be > >>>>>> added to for use userspace to control the pixel encoding/colorspace > >>>>>> conversion(if desired, or userspace just makes sure to > >>>>>> directly feed in correct kind of data). > >>>>> > >>>>> I'm all for getting userspace control over pixel encoding but even > >>>>> then the kernel always knows which pixel encoding is selected and > >>>>> which InfoFrame has to be sent. Is there a reason why userspace would > >>>>> want to control the variant explicitly to the wrong value? > >>>>> > >>>> > >>>> I've asked this before but haven't seen an answer: Is there an existing > >>>> upstream userspace project that makes use of this property (other than > >>>> what Joshua is working on in gamescope right now)? That would help us > >>>> understand the intent better. > >>> > >>> The intent was to control the infoframe colorimetry bits, > >>> nothing more. No idea what real userspace there was, if any. > >>> > >>>> > >>>> I don't think giving userspace explicit control over the exact infoframe > >>>> values is the right thing to do. > >>> > >>> Only userspace knows what kind of data it's stuffing into > >>> the pixels (and/or how it configures the csc units/etc.) to > >>> generate them. > >>> > >> > >> Yes, but userspace doesn't control or know whether we drive > >> RGB or YCbCr on the wire. In fact, in some cases our driver > >> needs to fallback to YCbCr420 for bandwidth reasons. There > >> is currently no way for userspace to know that and I don't > >> think it makes sense. > > > > People want that control as well for whatever reason. We've > > been asked to allow YCbCr 4:4:4 output many times. > > > > The automagic 4:2:0 fallback I think is rather fundementally > > incompatible with fancy color management. How would we even > > know whether to use eg. BT.2020 vs. BT.709 matrix? In i915 > > that stuff is just always BT.709 limited range, no questions > > asked. > > > > We use what we're telling the display, i.e., the value in the > colorspace property. That way we know whether to use a BT.2020 > or BT.709 matrix. And given how these things have gone in the past I think that is likey to bite someone at in the future. Also not what this property was meant to do nor does on any other driver AFAIK. > I don't see how it's fundamentally incompatible with fancy > color management stuff. > > If we start forbidding drivers from falling back to YCbCr > (whether 4:4:4 or 4:2:0) we will break existing behavior on > amdgpu and will see bug reports. The compositors could deal with that if/when they start doing the full color management stuff. The current stuff only really works when the kernel is allowed to do whatever it wants. > > > So I think if userspace wants real color management it's > > going to have to set up the whole pipeline. And for that > > we need at least one new property to control the RGB->YCbCr > > conversion (or to explicitly avoid it). > > > > And given that the proposed patch just swept all the > > non-BT.2020 issues under the rug makes me think no > > one has actually come up with any kind of consistent > > plan for anything else really. > > > > Does anyone actually use the non-BT.2020 colorspace stuff? No idea if anyone is using any of it. It's a bit hard to do right now outside the full passthrough case since we have no properties to control how the hardware will convert stuff. Anyways, sounds like what you're basically proposing is getting rid of this property and starting from scratch. -- Ville Syrjälä Intel
Re: [PATCH 3/3] drm/connector: Deprecate split for BT.2020 in drm_colorspace enum
On Fri, Feb 03, 2023 at 10:24:52AM -0500, Harry Wentland wrote: > > > On 2/3/23 10:19, Ville Syrjälä wrote: > > On Fri, Feb 03, 2023 at 09:39:42AM -0500, Harry Wentland wrote: > >> > >> > >> On 2/3/23 07:59, Sebastian Wick wrote: > >>> On Fri, Feb 3, 2023 at 11:40 AM Ville Syrjälä > >>> wrote: > >>>> > >>>> On Fri, Feb 03, 2023 at 02:07:44AM +, Joshua Ashton wrote: > >>>>> Userspace has no way of controlling or knowing the pixel encoding > >>>>> currently, so there is no way for it to ever get the right values here. > >>>> > >>>> That applies to a lot of the other values as well (they are > >>>> explicitly RGB or YCC). The idea was that this property sets the > >>>> infoframe/MSA/SDP value exactly, and other properties should be > >>>> added to for use userspace to control the pixel encoding/colorspace > >>>> conversion(if desired, or userspace just makes sure to > >>>> directly feed in correct kind of data). > >>> > >>> I'm all for getting userspace control over pixel encoding but even > >>> then the kernel always knows which pixel encoding is selected and > >>> which InfoFrame has to be sent. Is there a reason why userspace would > >>> want to control the variant explicitly to the wrong value? > >>> > >> > >> I've asked this before but haven't seen an answer: Is there an existing > >> upstream userspace project that makes use of this property (other than > >> what Joshua is working on in gamescope right now)? That would help us > >> understand the intent better. > > > > The intent was to control the infoframe colorimetry bits, > > nothing more. No idea what real userspace there was, if any. > > > >> > >> I don't think giving userspace explicit control over the exact infoframe > >> values is the right thing to do. > > > > Only userspace knows what kind of data it's stuffing into > > the pixels (and/or how it configures the csc units/etc.) to > > generate them. > > > > Yes, but userspace doesn't control or know whether we drive > RGB or YCbCr on the wire. In fact, in some cases our driver > needs to fallback to YCbCr420 for bandwidth reasons. There > is currently no way for userspace to know that and I don't > think it makes sense. People want that control as well for whatever reason. We've been asked to allow YCbCr 4:4:4 output many times. The automagic 4:2:0 fallback I think is rather fundementally incompatible with fancy color management. How would we even know whether to use eg. BT.2020 vs. BT.709 matrix? In i915 that stuff is just always BT.709 limited range, no questions asked. So I think if userspace wants real color management it's going to have to set up the whole pipeline. And for that we need at least one new property to control the RGB->YCbCr conversion (or to explicitly avoid it). And given that the proposed patch just swept all the non-BT.2020 issues under the rug makes me think no one has actually come up with any kind of consistent plan for anything else really. > > Userspace needs full control of framebuffer pixel formats, > as well as control over DEGAMMA, GAMMA, CTM color operations. > It also needs to be able to select whether to drive the panel > as sRGB or BT.2020/PQ but it doesn't make sense for it to > control the pixel encoding on the wire (RGB vs YCbCr). > > > I really don't want a repeat of the disaster of the > > 'Broadcast RGB' which has coupled together the infoframe > > and automagic conversion stuff. And I think this one would > > be about 100x worse given this property has something > > to do with actual colorspaces as well. > > > > I'm unaware of this disaster. Could you elaborate? The property now controls both the infoframe stuff (and whatever super vague stuff DP has for it in MSA) and full->limited range compression in the display pipeline. And as a result there is no way to eg. allow already limited range input, which is what some people wanted. And naturally it's all made a lot more terrible by all the displays that fail to implement the spec correctly, but that's another topic. -- Ville Syrjälä Intel
Re: [PATCH 3/3] drm/connector: Deprecate split for BT.2020 in drm_colorspace enum
On Fri, Feb 03, 2023 at 09:39:42AM -0500, Harry Wentland wrote: > > > On 2/3/23 07:59, Sebastian Wick wrote: > > On Fri, Feb 3, 2023 at 11:40 AM Ville Syrjälä > > wrote: > >> > >> On Fri, Feb 03, 2023 at 02:07:44AM +, Joshua Ashton wrote: > >>> Userspace has no way of controlling or knowing the pixel encoding > >>> currently, so there is no way for it to ever get the right values here. > >> > >> That applies to a lot of the other values as well (they are > >> explicitly RGB or YCC). The idea was that this property sets the > >> infoframe/MSA/SDP value exactly, and other properties should be > >> added to for use userspace to control the pixel encoding/colorspace > >> conversion(if desired, or userspace just makes sure to > >> directly feed in correct kind of data). > > > > I'm all for getting userspace control over pixel encoding but even > > then the kernel always knows which pixel encoding is selected and > > which InfoFrame has to be sent. Is there a reason why userspace would > > want to control the variant explicitly to the wrong value? > > > > I've asked this before but haven't seen an answer: Is there an existing > upstream userspace project that makes use of this property (other than > what Joshua is working on in gamescope right now)? That would help us > understand the intent better. The intent was to control the infoframe colorimetry bits, nothing more. No idea what real userspace there was, if any. > > I don't think giving userspace explicit control over the exact infoframe > values is the right thing to do. Only userspace knows what kind of data it's stuffing into the pixels (and/or how it configures the csc units/etc.) to generate them. I really don't want a repeat of the disaster of the 'Broadcast RGB' which has coupled together the infoframe and automagic conversion stuff. And I think this one would be about 100x worse given this property has something to do with actual colorspaces as well. -- Ville Syrjälä Intel
Re: [PATCH 3/3] drm/connector: Deprecate split for BT.2020 in drm_colorspace enum
On Fri, Feb 03, 2023 at 02:52:50PM +0100, Sebastian Wick wrote: > On Fri, Feb 3, 2023 at 2:35 PM Ville Syrjälä > wrote: > > > > On Fri, Feb 03, 2023 at 01:59:07PM +0100, Sebastian Wick wrote: > > > On Fri, Feb 3, 2023 at 11:40 AM Ville Syrjälä > > > wrote: > > > > > > > > On Fri, Feb 03, 2023 at 02:07:44AM +, Joshua Ashton wrote: > > > > > Userspace has no way of controlling or knowing the pixel encoding > > > > > currently, so there is no way for it to ever get the right values > > > > > here. > > > > > > > > That applies to a lot of the other values as well (they are > > > > explicitly RGB or YCC). The idea was that this property sets the > > > > infoframe/MSA/SDP value exactly, and other properties should be > > > > added to for use userspace to control the pixel encoding/colorspace > > > > conversion(if desired, or userspace just makes sure to > > > > directly feed in correct kind of data). > > > > > > I'm all for getting userspace control over pixel encoding but even > > > then the kernel always knows which pixel encoding is selected and > > > which InfoFrame has to be sent. Is there a reason why userspace would > > > want to control the variant explicitly to the wrong value? > > > > What do you mean wrong value? Userspace sets it based on what > > kind of data it has generated (or asked the display hardware > > to generate if/when we get explicit control over that part). > > Wrong in the sense of sending the YCC variant when the pixel encoding > is RGB for example. > > Maybe I'm missing something here but my assumption is that the kernel > always has to know the pixel encoding anyway. The color pipeline also > assumes that the pixel values are RGB. User space might be able to > generate YCC content but for subsampling etc the pixel encoding still > has to be explicitly set. The kernel doesn't really know much atm. In theory you can just configure the thing to do a straight passthough and put anything you want into your pixels. > > So with the kernel always knowing exactly what pixel encoding is sent, > why do we need those variants? I just don't see why this is necessary. > > > > > -- > > Ville Syrjälä > > Intel > > -- Ville Syrjälä Intel
Re: [PATCH 3/3] drm/connector: Deprecate split for BT.2020 in drm_colorspace enum
On Fri, Feb 03, 2023 at 01:59:07PM +0100, Sebastian Wick wrote: > On Fri, Feb 3, 2023 at 11:40 AM Ville Syrjälä > wrote: > > > > On Fri, Feb 03, 2023 at 02:07:44AM +, Joshua Ashton wrote: > > > Userspace has no way of controlling or knowing the pixel encoding > > > currently, so there is no way for it to ever get the right values here. > > > > That applies to a lot of the other values as well (they are > > explicitly RGB or YCC). The idea was that this property sets the > > infoframe/MSA/SDP value exactly, and other properties should be > > added to for use userspace to control the pixel encoding/colorspace > > conversion(if desired, or userspace just makes sure to > > directly feed in correct kind of data). > > I'm all for getting userspace control over pixel encoding but even > then the kernel always knows which pixel encoding is selected and > which InfoFrame has to be sent. Is there a reason why userspace would > want to control the variant explicitly to the wrong value? What do you mean wrong value? Userspace sets it based on what kind of data it has generated (or asked the display hardware to generate if/when we get explicit control over that part). -- Ville Syrjälä Intel
Re: [PATCH 3/3] drm/connector: Deprecate split for BT.2020 in drm_colorspace enum
On Fri, Feb 03, 2023 at 02:07:44AM +, Joshua Ashton wrote:
> Userspace has no way of controlling or knowing the pixel encoding
> currently, so there is no way for it to ever get the right values here.
That applies to a lot of the other values as well (they are
explicitly RGB or YCC). The idea was that this property sets the
infoframe/MSA/SDP value exactly, and other properties should be
added to for use userspace to control the pixel encoding/colorspace
conversion(if desired, or userspace just makes sure to
directly feed in correct kind of data).
>
> When we do add pixel_encoding control from userspace,we can pick the
> right value for the colorimetry packet based on the
> pixel_encoding + the colorspace.
>
> Let's deprecate these values, and have one BT.2020 colorspace entry
> that userspace can use.
>
> Note: _CYCC was effectively 'removed' by this change, but that was not
> possible to be taken advantage of anyway, as there is currently no
> pixel_encoding control so it would not be possible to output
> linear YCbCr.
>
> Signed-off-by: Joshua Ashton
>
> Cc: Pekka Paalanen
> Cc: Sebastian Wick
> Cc: vitaly.pros...@amd.com
> Cc: Uma Shankar
> Cc: Ville Syrjälä
> Cc: Joshua Ashton
> Cc: dri-de...@lists.freedesktop.org
> Cc: amd-gfx@lists.freedesktop.org
> ---
> drivers/gpu/drm/display/drm_hdmi_helper.c | 9 -
> drivers/gpu/drm/drm_connector.c | 12 ++--
> drivers/gpu/drm/i915/display/intel_dp.c | 20 +---
> include/drm/drm_connector.h | 19 ++-
> 4 files changed, 29 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/gpu/drm/display/drm_hdmi_helper.c
> b/drivers/gpu/drm/display/drm_hdmi_helper.c
> index 0264abe55278..c85860600395 100644
> --- a/drivers/gpu/drm/display/drm_hdmi_helper.c
> +++ b/drivers/gpu/drm/display/drm_hdmi_helper.c
> @@ -99,8 +99,7 @@ EXPORT_SYMBOL(drm_hdmi_infoframe_set_hdr_metadata);
> #define HDMI_COLORIMETRY_OPYCC_601 (C(3) | EC(3) | ACE(0))
> #define HDMI_COLORIMETRY_OPRGB (C(3) | EC(4) | ACE(0))
> #define HDMI_COLORIMETRY_BT2020_CYCC (C(3) | EC(5) | ACE(0))
> -#define HDMI_COLORIMETRY_BT2020_RGB (C(3) | EC(6) | ACE(0))
> -#define HDMI_COLORIMETRY_BT2020_YCC (C(3) | EC(6) | ACE(0))
> +#define HDMI_COLORIMETRY_BT2020 (C(3) | EC(6) | ACE(0))
> #define HDMI_COLORIMETRY_DCI_P3_RGB_D65 (C(3) | EC(7) | ACE(0))
> #define HDMI_COLORIMETRY_DCI_P3_RGB_THEATER (C(3) | EC(7) | ACE(1))
>
> @@ -113,9 +112,9 @@ static const u32 hdmi_colorimetry_val[] = {
> [DRM_MODE_COLORIMETRY_SYCC_601] = HDMI_COLORIMETRY_SYCC_601,
> [DRM_MODE_COLORIMETRY_OPYCC_601] = HDMI_COLORIMETRY_OPYCC_601,
> [DRM_MODE_COLORIMETRY_OPRGB] = HDMI_COLORIMETRY_OPRGB,
> - [DRM_MODE_COLORIMETRY_BT2020_CYCC] = HDMI_COLORIMETRY_BT2020_CYCC,
> - [DRM_MODE_COLORIMETRY_BT2020_RGB] = HDMI_COLORIMETRY_BT2020_RGB,
> - [DRM_MODE_COLORIMETRY_BT2020_YCC] = HDMI_COLORIMETRY_BT2020_YCC,
> + [DRM_MODE_COLORIMETRY_BT2020_DEPRECATED_1] = HDMI_COLORIMETRY_BT2020,
> + [DRM_MODE_COLORIMETRY_BT2020_DEPRECATED_2] = HDMI_COLORIMETRY_BT2020,
> + [DRM_MODE_COLORIMETRY_BT2020] = HDMI_COLORIMETRY_BT2020,
> };
>
> #undef C
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 61c29ce74b03..58699ab15a6a 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -1029,11 +1029,11 @@ static const struct drm_prop_enum_list
> hdmi_colorspaces[] = {
> /* Colorimetry based on IEC 61966-2-5 */
> { DRM_MODE_COLORIMETRY_OPRGB, "opRGB" },
> /* Colorimetry based on ITU-R BT.2020 */
> - { DRM_MODE_COLORIMETRY_BT2020_CYCC, "BT2020_CYCC" },
> + { DRM_MODE_COLORIMETRY_BT2020_DEPRECATED_1, "BT2020_DEPRECATED_1" },
> /* Colorimetry based on ITU-R BT.2020 */
> - { DRM_MODE_COLORIMETRY_BT2020_RGB, "BT2020_RGB" },
> + { DRM_MODE_COLORIMETRY_BT2020_DEPRECATED_2, "BT2020_DEPRECATED_2" },
> /* Colorimetry based on ITU-R BT.2020 */
> - { DRM_MODE_COLORIMETRY_BT2020_YCC, "BT2020_YCC" },
> + { DRM_MODE_COLORIMETRY_BT2020, "BT2020" },
> /* Added as part of Additional Colorimetry Extension in 861.G */
> { DRM_MODE_COLORIMETRY_DCI_P3_RGB_D65, "DCI-P3_RGB_D65" },
> { DRM_MODE_COLORIMETRY_DCI_P3_RGB_THEATER, "DCI-P3_RGB_Theater" },
> @@ -1054,7 +1054,7 @@ static const struct drm_prop_enum_list dp_colorspaces[]
> = {
> /* Colorimetry based on SMPTE RP 431-2 */
> { DRM_MODE_COLORIMETRY_DCI_P3_RGB_D65, "DCI-P3_RGB_D65" },
>
Re: [PATCH v2 02/10] drm: Include where needed
On Fri, Jan 13, 2023 at 04:11:48PM +0100, Sam Ravnborg wrote: > Hi Ville, > On Wed, Jan 11, 2023 at 06:08:42PM +0200, Ville Syrjälä wrote: > > On Wed, Jan 11, 2023 at 02:01:58PM +0100, Thomas Zimmermann wrote: > > > Include in source files that need it. Some of DRM's > > > source code gets OF header via drm_crtc_helper.h and , > > > which can leed to unnecessary recompilation. > > > > > > In drm_modes.c, add a comment on the reason for still including > > > . The header file is required to get KHZ2PICOS(). The > > > macro is part of the UAPI headers, so it cannot be moved to a less > > > prominent location. > > > > I never liked that KHZ2PICOS() thing in there. Maybe we should > > just nuke it and see if anyone notices? > https://github.com/search?q=KHZ2PICOS&type=code No idea what relevance any of those have. > > tells me that it will be noticed. > So it is part of the UAPI The only thing it does it potentially mistake some modes for being equal when they are not. So basically just second guessing what the driver/hardware is actually capable of doing. -- Ville Syrjälä Intel
Re: [PATCH v2 1/3] drm/framebuffer: Check for valid formats
On Fri, Jan 13, 2023 at 08:27:42AM -0300, Maíra Canal wrote:
> Currently, framebuffer_check() doesn't check if the pixel format is
> supported, which can lead to the acceptance of invalid pixel formats
> e.g. the acceptance of invalid modifiers. Therefore, add a check for
> valid formats on framebuffer_check(), so that the ADDFB2 IOCTL rejects
> calls with invalid formats.
>
> Moreover, note that this check is only valid for atomic drivers,
> because, for non-atomic drivers, checking drm_any_plane_has_format() is
> not possible since the format list for the primary plane is fake, and
> we'd therefore reject valid formats.
>
> Reviewed-by: Daniel Vetter
> Signed-off-by: Maíra Canal
> ---
> Documentation/gpu/todo.rst| 9 -
> drivers/gpu/drm/drm_framebuffer.c | 8
> 2 files changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
> index 1f8a5ebe188e..3a79c26c5cc7 100644
> --- a/Documentation/gpu/todo.rst
> +++ b/Documentation/gpu/todo.rst
> @@ -276,11 +276,10 @@ Various hold-ups:
> - Need to switch to drm_fbdev_generic_setup(), otherwise a lot of the custom
> fb
>setup code can't be deleted.
>
> -- Many drivers wrap drm_gem_fb_create() only to check for valid formats. For
> - atomic drivers we could check for valid formats by calling
> - drm_plane_check_pixel_format() against all planes, and pass if any plane
> - supports the format. For non-atomic that's not possible since like the
> format
> - list for the primary plane is fake and we'd therefor reject valid formats.
> +- Need to switch to drm_gem_fb_create(), as now framebuffer_check() checks
> for
> + valid formats for atomic drivers.
> +
> +- Add an addfb format validation for non-atomic drivers.
>
> - Many drivers subclass drm_framebuffer, we'd need a embedding compatible
>version of the varios drm_gem_fb_create functions. Maybe called
> diff --git a/drivers/gpu/drm/drm_framebuffer.c
> b/drivers/gpu/drm/drm_framebuffer.c
> index aff3746dedfb..605642bf3650 100644
> --- a/drivers/gpu/drm/drm_framebuffer.c
> +++ b/drivers/gpu/drm/drm_framebuffer.c
> @@ -280,6 +280,14 @@ static int framebuffer_check(struct drm_device *dev,
> }
> }
>
> + /* Verify that the modifier is supported. */
> + if (drm_drv_uses_atomic_modeset(dev) &&
> + !drm_any_plane_has_format(dev, r->pixel_format, r->modifier[0])) {
> + drm_dbg_kms(dev, "Unsupported pixel format %p4cc / modifier
> 0x%llx\n",
> + &r->pixel_format, r->modifier[0]);
> + return -EINVAL;
> + }
Like I said this is still wrong for the !modifiers case.
> +
> return 0;
> }
>
> --
> 2.39.0
--
Ville Syrjälä
Intel
Re: [PATCH v2 02/10] drm: Include where needed
On Wed, Jan 11, 2023 at 02:01:58PM +0100, Thomas Zimmermann wrote: > Include in source files that need it. Some of DRM's > source code gets OF header via drm_crtc_helper.h and , > which can leed to unnecessary recompilation. > > In drm_modes.c, add a comment on the reason for still including > . The header file is required to get KHZ2PICOS(). The > macro is part of the UAPI headers, so it cannot be moved to a less > prominent location. I never liked that KHZ2PICOS() thing in there. Maybe we should just nuke it and see if anyone notices? > > v2: > * include in komeda_drv.c (kernel test robot) > > Signed-off-by: Thomas Zimmermann > --- > drivers/gpu/drm/arm/display/komeda/komeda_drv.c | 1 + > drivers/gpu/drm/drm_modes.c | 5 +++-- > drivers/gpu/drm/panel/panel-ronbo-rb070d30.c| 1 + > 3 files changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_drv.c > b/drivers/gpu/drm/arm/display/komeda/komeda_drv.c > index 3f4e719eebd8..28f76e07dd95 100644 > --- a/drivers/gpu/drm/arm/display/komeda/komeda_drv.c > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_drv.c > @@ -6,6 +6,7 @@ > */ > #include > #include > +#include > #include > #include > #include > diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c > index be030f4a5311..40d482a01178 100644 > --- a/drivers/gpu/drm/drm_modes.c > +++ b/drivers/gpu/drm/drm_modes.c > @@ -31,10 +31,11 @@ > */ > > #include > +#include > +#include /* for KHZ2PICOS() */ > #include > #include > -#include > -#include > +#include > > #include > #include > diff --git a/drivers/gpu/drm/panel/panel-ronbo-rb070d30.c > b/drivers/gpu/drm/panel/panel-ronbo-rb070d30.c > index a8a98c91b13c..866d1bf5530e 100644 > --- a/drivers/gpu/drm/panel/panel-ronbo-rb070d30.c > +++ b/drivers/gpu/drm/panel/panel-ronbo-rb070d30.c > @@ -15,6 +15,7 @@ > #include > #include > #include > +#include > > #include > #include > -- > 2.39.0 -- Ville Syrjälä Intel
Re: [PATCH v2 06/10] drm: Define enum mode_set_atomic in drm_modeset_helper_tables.h
On Wed, Jan 11, 2023 at 02:02:02PM +0100, Thomas Zimmermann wrote:
> Define enum mode_set_atomic next to the only interface that uses
> the type. This will allow for removing several include statements
> for drm_fb_helper.h. No functional changes.
>
> Signed-off-by: Thomas Zimmermann
Reviewed-by: Ville Syrjälä
> ---
> include/drm/drm_fb_helper.h | 5 -
> include/drm/drm_modeset_helper_vtables.h | 6 +-
> 2 files changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
> index b111dc7ada78..f443e1f11654 100644
> --- a/include/drm/drm_fb_helper.h
> +++ b/include/drm/drm_fb_helper.h
> @@ -37,11 +37,6 @@ struct drm_fb_helper;
>
> #include
>
> -enum mode_set_atomic {
> - LEAVE_ATOMIC_MODE_SET,
> - ENTER_ATOMIC_MODE_SET,
> -};
> -
> /**
> * struct drm_fb_helper_surface_size - describes fbdev size and scanout
> surface size
> * @fb_width: fbdev width
> diff --git a/include/drm/drm_modeset_helper_vtables.h
> b/include/drm/drm_modeset_helper_vtables.h
> index 77a540ad7dcd..206f495bbf06 100644
> --- a/include/drm/drm_modeset_helper_vtables.h
> +++ b/include/drm/drm_modeset_helper_vtables.h
> @@ -48,10 +48,14 @@
> * To make this clear all the helper vtables are pulled together in this
> location here.
> */
>
> -enum mode_set_atomic;
> struct drm_writeback_connector;
> struct drm_writeback_job;
>
> +enum mode_set_atomic {
> + LEAVE_ATOMIC_MODE_SET,
> + ENTER_ATOMIC_MODE_SET,
> +};
> +
> /**
> * struct drm_crtc_helper_funcs - helper operations for CRTCs
> *
> --
> 2.39.0
--
Ville Syrjälä
Intel
Re: [PATCH v2 03/10] drm: Don't include in drm_crtc_helper.h
On Wed, Jan 11, 2023 at 02:01:59PM +0100, Thomas Zimmermann wrote: > Including in drm_crtc_helper.h is not required. Remove > the include statement and avoid rebuilding DRM whenever the fbdev > header changes. > > Signed-off-by: Thomas Zimmermann > --- > include/drm/drm_crtc_helper.h | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/include/drm/drm_crtc_helper.h b/include/drm/drm_crtc_helper.h > index 1840db247f69..072bc4f90349 100644 > --- a/include/drm/drm_crtc_helper.h > +++ b/include/drm/drm_crtc_helper.h > @@ -37,8 +37,6 @@ > #include > #include ^ bunch of other unnecessary headers there as well. Reviewed-by: Ville Syrjälä > > -#include > - > #include > #include > #include > -- > 2.39.0 -- Ville Syrjälä Intel
Re: [Intel-gfx] [PATCH 1/7] drm: mark drm.debug-on-dyndbg as BROKEN for now
On Fri, Nov 11, 2022 at 03:17:09PM -0700, Jim Cromie wrote: > drm.debug-on-dyndbg has a regression, due to a chicken-egg > initialization problem: > > 1- modprobe i915 >i915 needs drm.ko, which is loaded 1st > > 2- "modprobe drm drm.debug=0x1ff" (virtual/implied) >drm.debug is set post-initialization, from boot-args etc > > 3- `modprobe i915` finishes > > W/O drm.debug-on-dyndbg that just works, because all drm_dbg* > callsites use drm_debug_enabled() to check __drm_debug & DEM_UT_ > before printing. > > But the whole point of drm.debug-on-dyndbg is to avoid that runtime > test, by enabling (at post-modinit) a static-key at each callsite in > the just-loaded module. > > And since drm.ko is loaded before all dependent modules, none are > "just-loaded", and no drm.debug callsites are present yet, except > those in drm.ko itself. > > Signed-off-by: Jim Cromie > --- > drivers/gpu/drm/Kconfig | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig > index 34f5a092c99e..0d1e59e6bb7e 100644 > --- a/drivers/gpu/drm/Kconfig > +++ b/drivers/gpu/drm/Kconfig > @@ -54,6 +54,7 @@ config DRM_DEBUG_MM > config DRM_USE_DYNAMIC_DEBUG > bool "use dynamic debug to implement drm.debug" > default y Could you switch to 'default n' as well? i915 CI actually enables BROKEN so that we can test some more experimental stuff which is hidden behind BROKEN for normal users. > + depends on BROKEN # chicken-egg initial enable problem > depends on DRM > depends on DYNAMIC_DEBUG || DYNAMIC_DEBUG_CORE > depends on JUMP_LABEL > -- > 2.38.1 -- Ville Syrjälä Intel
Re: [Intel-gfx] [PATCH v7 0/9] dyndbg: drm.debug adaptation
On Mon, Oct 31, 2022 at 08:20:54PM -0400, Jason Baron wrote:
>
>
> On 10/31/22 6:11 PM, jim.cro...@gmail.com wrote:
> > On Mon, Oct 31, 2022 at 7:07 AM Ville Syrjälä
> > wrote:
> >> On Sun, Oct 30, 2022 at 08:42:52AM -0600, jim.cro...@gmail.com wrote:
> >>> On Thu, Oct 27, 2022 at 2:10 PM Ville Syrjälä
> >>> wrote:
> >>>> On Thu, Oct 27, 2022 at 01:55:39PM -0600, jim.cro...@gmail.com wrote:
> >>>>> On Thu, Oct 27, 2022 at 9:59 AM Ville Syrjälä
> >>>>> wrote:
> >>>>>> On Thu, Oct 27, 2022 at 09:37:52AM -0600, jim.cro...@gmail.com wrote:
> >>>>>>> On Thu, Oct 27, 2022 at 9:08 AM Jason Baron wrote:
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> On 10/21/22 05:18, Jani Nikula wrote:
> >>>>>>>>> On Thu, 20 Oct 2022, Ville Syrjälä
> >>>>>>>>> wrote:
> >>>>>>>>>> On Sat, Sep 24, 2022 at 03:02:34PM +0200, Greg KH wrote:
> >>>>>>>>>>> On Sun, Sep 11, 2022 at 11:28:43PM -0600, Jim Cromie wrote:
> >>>>>>>>>>>> hi Greg, Dan, Jason, DRM-folk,
> >>>>>>>>>>>>
> >>>>>>>>>>>> heres follow-up to V6:
> >>>>>>>>>>>>rebased on driver-core/driver-core-next for -v6 applied bits
> >>>>>>>>>>>> (thanks)
> >>>>>>>>>>>>rework drm_debug_enabled{_raw,_instrumented,} per Dan.
> >>>>>>>>>>>>
> >>>>>>>>>>>> It excludes:
> >>>>>>>>>>>>nouveau parts (immature)
> >>>>>>>>>>>>tracefs parts (I missed --to=Steve on v6)
> >>>>>>>>>>>>split _ddebug_site and de-duplicate experiment (way unready)
> >>>>>>>>>>>>
> >>>>>>>>>>>> IOW, its the remaining commits of V6 on which Dan gave his
> >>>>>>>>>>>> Reviewed-by.
> >>>>>>>>>>>>
> >>>>>>>>>>>> If these are good to apply, I'll rebase and repost the rest
> >>>>>>>>>>>> separately.
> >>>>>>>>>>> All now queued up, thanks.
> >>>>>>>>>> This stuff broke i915 debugs. When I first load i915 no debug
> >>>>>>>>>> prints are
> >>>>>>>>>> produced. If I then go fiddle around in
> >>>>>>>>>> /sys/module/drm/parameters/debug
> >>>>>>>>>> the debug prints start to suddenly work.
> >>>>>>>>> Wait what? I always assumed the default behaviour would stay the
> >>>>>>>>> same,
> >>>>>>>>> which is usually how we roll. It's a regression in my books. We've
> >>>>>>>>> got a
> >>>>>>>>> CI farm that's not very helpful in terms of dmesg logging right now
> >>>>>>>>> because of this.
> >>>>>>>>>
> >>>>>>>>> BR,
> >>>>>>>>> Jani.
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>> That doesn't sound good - so you are saying that prior to this
> >>>>>>>> change some
> >>>>>>>> of the drm debugs were default enabled. But now you have to manually
> >>>>>>>> enable
> >>>>>>>> them?
> >>>>>>>>
> >>>>>>>> Thanks,
> >>>>>>>>
> >>>>>>>> -Jason
> >>>>>>>
> >>>>>>> Im just seeing this now.
> >>>>>>> Any new details ?
> >>>>>> No. We just disabled it as BROKEN for now. I was just today thinking
> >>>>>> about sending that patch out if no solutin is forthcoming soon since
> >>>>>> we need this working before 6.1 is released.
> >>>>>>
> >>>>>> Pretty sure you should see the problem immediately with any driver
> >>>>>> (at least if it's built as
Re: [Intel-gfx] [PATCH v7 0/9] dyndbg: drm.debug adaptation
On Sun, Oct 30, 2022 at 08:42:52AM -0600, jim.cro...@gmail.com wrote:
> On Thu, Oct 27, 2022 at 2:10 PM Ville Syrjälä
> wrote:
> >
> > On Thu, Oct 27, 2022 at 01:55:39PM -0600, jim.cro...@gmail.com wrote:
> > > On Thu, Oct 27, 2022 at 9:59 AM Ville Syrjälä
> > > wrote:
> > > >
> > > > On Thu, Oct 27, 2022 at 09:37:52AM -0600, jim.cro...@gmail.com wrote:
> > > > > On Thu, Oct 27, 2022 at 9:08 AM Jason Baron wrote:
> > > > > >
> > > > > >
> > > > > >
> > > > > > On 10/21/22 05:18, Jani Nikula wrote:
> > > > > > > On Thu, 20 Oct 2022, Ville Syrjälä
> > > > > > > wrote:
> > > > > > >> On Sat, Sep 24, 2022 at 03:02:34PM +0200, Greg KH wrote:
> > > > > > >>> On Sun, Sep 11, 2022 at 11:28:43PM -0600, Jim Cromie wrote:
> > > > > > >>>> hi Greg, Dan, Jason, DRM-folk,
> > > > > > >>>>
> > > > > > >>>> heres follow-up to V6:
> > > > > > >>>> rebased on driver-core/driver-core-next for -v6 applied bits
> > > > > > >>>> (thanks)
> > > > > > >>>> rework drm_debug_enabled{_raw,_instrumented,} per Dan.
> > > > > > >>>>
> > > > > > >>>> It excludes:
> > > > > > >>>> nouveau parts (immature)
> > > > > > >>>> tracefs parts (I missed --to=Steve on v6)
> > > > > > >>>> split _ddebug_site and de-duplicate experiment (way unready)
> > > > > > >>>>
> > > > > > >>>> IOW, its the remaining commits of V6 on which Dan gave his
> > > > > > >>>> Reviewed-by.
> > > > > > >>>>
> > > > > > >>>> If these are good to apply, I'll rebase and repost the rest
> > > > > > >>>> separately.
> > > > > > >>>
> > > > > > >>> All now queued up, thanks.
> > > > > > >>
> > > > > > >> This stuff broke i915 debugs. When I first load i915 no debug
> > > > > > >> prints are
> > > > > > >> produced. If I then go fiddle around in
> > > > > > >> /sys/module/drm/parameters/debug
> > > > > > >> the debug prints start to suddenly work.
> > > > > > >
> > > > > > > Wait what? I always assumed the default behaviour would stay the
> > > > > > > same,
> > > > > > > which is usually how we roll. It's a regression in my books.
> > > > > > > We've got a
> > > > > > > CI farm that's not very helpful in terms of dmesg logging right
> > > > > > > now
> > > > > > > because of this.
> > > > > > >
> > > > > > > BR,
> > > > > > > Jani.
> > > > > > >
> > > > > > >
> > > > > >
> > > > > > That doesn't sound good - so you are saying that prior to this
> > > > > > change some
> > > > > > of the drm debugs were default enabled. But now you have to
> > > > > > manually enable
> > > > > > them?
> > > > > >
> > > > > > Thanks,
> > > > > >
> > > > > > -Jason
> > > > >
> > > > >
> > > > > Im just seeing this now.
> > > > > Any new details ?
> > > >
> > > > No. We just disabled it as BROKEN for now. I was just today thinking
> > > > about sending that patch out if no solutin is forthcoming soon since
> > > > we need this working before 6.1 is released.
> > > >
> > > > Pretty sure you should see the problem immediately with any driver
> > > > (at least if it's built as a module, didn't try builtin). Or at least
> > > > can't think what would make i915 any more special.
> > > >
> > >
> > > So, I should note -
> > > 99% of my time & energy on this dyndbg + drm patchset
> > > has been done using virtme,
> > > so my world-view (and dev-hack-test env) has been smaller, simpler
> > > maybe its been fatally simplistic.
> > &
Re: [Intel-gfx] [PATCH v7 0/9] dyndbg: drm.debug adaptation
On Thu, Oct 27, 2022 at 01:55:39PM -0600, jim.cro...@gmail.com wrote:
> On Thu, Oct 27, 2022 at 9:59 AM Ville Syrjälä
> wrote:
> >
> > On Thu, Oct 27, 2022 at 09:37:52AM -0600, jim.cro...@gmail.com wrote:
> > > On Thu, Oct 27, 2022 at 9:08 AM Jason Baron wrote:
> > > >
> > > >
> > > >
> > > > On 10/21/22 05:18, Jani Nikula wrote:
> > > > > On Thu, 20 Oct 2022, Ville Syrjälä
> > > > > wrote:
> > > > >> On Sat, Sep 24, 2022 at 03:02:34PM +0200, Greg KH wrote:
> > > > >>> On Sun, Sep 11, 2022 at 11:28:43PM -0600, Jim Cromie wrote:
> > > > >>>> hi Greg, Dan, Jason, DRM-folk,
> > > > >>>>
> > > > >>>> heres follow-up to V6:
> > > > >>>> rebased on driver-core/driver-core-next for -v6 applied bits
> > > > >>>> (thanks)
> > > > >>>> rework drm_debug_enabled{_raw,_instrumented,} per Dan.
> > > > >>>>
> > > > >>>> It excludes:
> > > > >>>> nouveau parts (immature)
> > > > >>>> tracefs parts (I missed --to=Steve on v6)
> > > > >>>> split _ddebug_site and de-duplicate experiment (way unready)
> > > > >>>>
> > > > >>>> IOW, its the remaining commits of V6 on which Dan gave his
> > > > >>>> Reviewed-by.
> > > > >>>>
> > > > >>>> If these are good to apply, I'll rebase and repost the rest
> > > > >>>> separately.
> > > > >>>
> > > > >>> All now queued up, thanks.
> > > > >>
> > > > >> This stuff broke i915 debugs. When I first load i915 no debug prints
> > > > >> are
> > > > >> produced. If I then go fiddle around in
> > > > >> /sys/module/drm/parameters/debug
> > > > >> the debug prints start to suddenly work.
> > > > >
> > > > > Wait what? I always assumed the default behaviour would stay the same,
> > > > > which is usually how we roll. It's a regression in my books. We've
> > > > > got a
> > > > > CI farm that's not very helpful in terms of dmesg logging right now
> > > > > because of this.
> > > > >
> > > > > BR,
> > > > > Jani.
> > > > >
> > > > >
> > > >
> > > > That doesn't sound good - so you are saying that prior to this change
> > > > some
> > > > of the drm debugs were default enabled. But now you have to manually
> > > > enable
> > > > them?
> > > >
> > > > Thanks,
> > > >
> > > > -Jason
> > >
> > >
> > > Im just seeing this now.
> > > Any new details ?
> >
> > No. We just disabled it as BROKEN for now. I was just today thinking
> > about sending that patch out if no solutin is forthcoming soon since
> > we need this working before 6.1 is released.
> >
> > Pretty sure you should see the problem immediately with any driver
> > (at least if it's built as a module, didn't try builtin). Or at least
> > can't think what would make i915 any more special.
> >
>
> So, I should note -
> 99% of my time & energy on this dyndbg + drm patchset
> has been done using virtme,
> so my world-view (and dev-hack-test env) has been smaller, simpler
> maybe its been fatally simplistic.
>
> ive just rebuilt v6.0 (before the trouble)
> and run it thru my virtual home box,
> I didnt see any unfamiliar drm-debug output
> that I might have inadvertently altered somehow
>
> I have some real HW I can put a reference kernel on,0
> to look for the missing output, but its all gonna take some time,
> esp to tighten up my dev-test-env
>
> in the meantime, there is:
>
> config DRM_USE_DYNAMIC_DEBUG
> bool "use dynamic debug to implement drm.debug"
> default y
> depends on DRM
> depends on DYNAMIC_DEBUG || DYNAMIC_DEBUG_CORE
> depends on JUMP_LABEL
> help
> Use dynamic-debug to avoid drm_debug_enabled() runtime overheads.
> Due to callsite counts in DRM drivers (~4k in amdgpu) and 56
> bytes per callsite, the .data costs can be substantial, and
> are therefore configurable.
>
> Does changing the default fix things for i915 dmesg ?
I think we want to mark it BROKEN in addition to make sure no one
enables it by accident. We already got one bug report where I had to
ask the reporter to rebuild their kernel since this had gotten
enabled and we didn't get any debugs from the driver probe.
> or is the problem deeper ?
>
> theres also this Makefile addition, which I might have oversimplified
>
> CFLAGS-$(CONFIG_DRM_USE_DYNAMIC_DEBUG) += -DDYNAMIC_DEBUG_MODULE
--
Ville Syrjälä
Intel
Re: [Intel-gfx] [PATCH v7 0/9] dyndbg: drm.debug adaptation
On Thu, Oct 27, 2022 at 09:37:52AM -0600, jim.cro...@gmail.com wrote:
> On Thu, Oct 27, 2022 at 9:08 AM Jason Baron wrote:
> >
> >
> >
> > On 10/21/22 05:18, Jani Nikula wrote:
> > > On Thu, 20 Oct 2022, Ville Syrjälä wrote:
> > >> On Sat, Sep 24, 2022 at 03:02:34PM +0200, Greg KH wrote:
> > >>> On Sun, Sep 11, 2022 at 11:28:43PM -0600, Jim Cromie wrote:
> > >>>> hi Greg, Dan, Jason, DRM-folk,
> > >>>>
> > >>>> heres follow-up to V6:
> > >>>> rebased on driver-core/driver-core-next for -v6 applied bits (thanks)
> > >>>> rework drm_debug_enabled{_raw,_instrumented,} per Dan.
> > >>>>
> > >>>> It excludes:
> > >>>> nouveau parts (immature)
> > >>>> tracefs parts (I missed --to=Steve on v6)
> > >>>> split _ddebug_site and de-duplicate experiment (way unready)
> > >>>>
> > >>>> IOW, its the remaining commits of V6 on which Dan gave his Reviewed-by.
> > >>>>
> > >>>> If these are good to apply, I'll rebase and repost the rest separately.
> > >>>
> > >>> All now queued up, thanks.
> > >>
> > >> This stuff broke i915 debugs. When I first load i915 no debug prints are
> > >> produced. If I then go fiddle around in /sys/module/drm/parameters/debug
> > >> the debug prints start to suddenly work.
> > >
> > > Wait what? I always assumed the default behaviour would stay the same,
> > > which is usually how we roll. It's a regression in my books. We've got a
> > > CI farm that's not very helpful in terms of dmesg logging right now
> > > because of this.
> > >
> > > BR,
> > > Jani.
> > >
> > >
> >
> > That doesn't sound good - so you are saying that prior to this change some
> > of the drm debugs were default enabled. But now you have to manually enable
> > them?
> >
> > Thanks,
> >
> > -Jason
>
>
> Im just seeing this now.
> Any new details ?
No. We just disabled it as BROKEN for now. I was just today thinking
about sending that patch out if no solutin is forthcoming soon since
we need this working before 6.1 is released.
Pretty sure you should see the problem immediately with any driver
(at least if it's built as a module, didn't try builtin). Or at least
can't think what would make i915 any more special.
>
> I didnt knowingly change something, but since its apparently happening,
> heres a 1st WAG at a possible cause
>
> commit ccc2b496324c13e917ef05f563626f4e7826bef1
> Author: Jim Cromie
> Date: Sun Sep 11 23:28:51 2022 -0600
>
> drm_print: prefer bare printk KERN_DEBUG on generic fn
>
> drm_print.c calls pr_debug() just once, from __drm_printfn_debug(),
> which is a generic/service fn. The callsite is compile-time enabled
> by DEBUG in both DYNAMIC_DEBUG=y/n builds.
>
> For dyndbg builds, reverting this callsite back to bare printk is
> correcting a few anti-features:
>
> 1- callsite is generic, serves multiple drm users.
>it is soft-wired on currently by #define DEBUG
>could accidentally: #> echo -p > /proc/dynamic_debug/control
>
> 2- optional "decorations" by dyndbg are unhelpful/misleading here,
>they describe only the generic site, not end users
>
> IOW, 1,2 are unhelpful at best, and possibly confusing.
>
> reverting yields a nominal data and text shrink:
>
>textdata bss dec hex filename
> 462583 36604 54592 553779 87333 /kernel/drivers/gpu/drm/drm.ko
> 462515 36532 54592 553639 872a7
> -dirty/kernel/drivers/gpu/drm/drm.ko
>
> Signed-off-by: Jim Cromie
> Link:
> https://lore.kernel.org/r/20220912052852.1123868-9-jim.cro...@gmail.com
> Signed-off-by: Greg Kroah-Hartman
--
Ville Syrjälä
Intel
Re: [PATCH v7 0/9] dyndbg: drm.debug adaptation
On Sat, Sep 24, 2022 at 03:02:34PM +0200, Greg KH wrote:
> On Sun, Sep 11, 2022 at 11:28:43PM -0600, Jim Cromie wrote:
> > hi Greg, Dan, Jason, DRM-folk,
> >
> > heres follow-up to V6:
> > rebased on driver-core/driver-core-next for -v6 applied bits (thanks)
> > rework drm_debug_enabled{_raw,_instrumented,} per Dan.
> >
> > It excludes:
> > nouveau parts (immature)
> > tracefs parts (I missed --to=Steve on v6)
> > split _ddebug_site and de-duplicate experiment (way unready)
> >
> > IOW, its the remaining commits of V6 on which Dan gave his Reviewed-by.
> >
> > If these are good to apply, I'll rebase and repost the rest separately.
>
> All now queued up, thanks.
This stuff broke i915 debugs. When I first load i915 no debug prints are
produced. If I then go fiddle around in /sys/module/drm/parameters/debug
the debug prints start to suddenly work.
--
Ville Syrjälä
Intel
Re: KMS atomic state sets, full vs. minimal (Re: [PATCH v3 0/6] Add support for atomic async page-flips)
On Mon, Oct 03, 2022 at 11:48:49AM +0300, Pekka Paalanen wrote: > On Fri, 30 Sep 2022 18:45:09 +0300 > Ville Syrjälä wrote: > > > On Fri, Sep 30, 2022 at 06:37:00PM +0300, Pekka Paalanen wrote: > > > On Fri, 30 Sep 2022 18:09:55 +0300 > > > Ville Syrjälä wrote: > > > > > > > That would actively discourage people from even attempting the > > > > "just dump all the state into the ioctl" approach with async flips > > > > since even the props whose value isn't even changing would be rejected. > > > > > > > > > > About that. > > > > > > To me it looks like just a classic case of broken communication. > > > > > > The kernel developers assume that of course userspace will minimize the > > > state set to only those properties that change, because...? > > > > > > Userspace developers think that of course the kernel will optimize the > > > state set into minimal changes, because the kernel actually has the > > > authoritative knowledge of what the current state is, and the driver > > > actually knows which properties are costly and need to be optimized and > > > which ones don't matter. It has never even occurred to me that the > > > kernel would not compare next state to current state. > > > > > > No-one ever documented any expectations, did they? > > > > Do you really want that for async flips? Async flips can't be > > done atomically with anything else, so why are you even asking > > the kernel to do that? > > I'm not talking about async flips only. > > I'm talking about atomic commits in general. I don't think it's a good > idea to make async atomic commits behave fundamentally different from > sync atomic commits wrt. what non-changing state you are allowed to > list in your state set or not. > > Isn't there common DRM code to convert an atomic commit state set into > state objects? It probably starts by copying the current state, and > then playing through the commit state set, setting all listed > properties to their new values? Why wouldn't that loop maintain the > knowledge of what actually changed? Any such book keeping is entirely ad-hoc atm. It's also not super obvious how much of it is actually useful. You have to do a real commit on the crtc anyway if the crtc (or on any of its associated objects) is listed in the commit, so there's not necessarily much to be gained by tracking chages in all properties. And that behaviour again enters very muddy waters when combined with the async flip flag for the entire commit. The current approach being proposed seems to suggest that we can instead short circuit async commits when nothing has changed. That is not at all how sync atomic commits work. > When you copy the current data, reset all changed-flags to false. When > you apply each property from the commit set, compare the value and set > the changed-flag only if the value changes. > > This manufacturing of the new tentative state set happens at atomic > commit ioctl time before the ioctl returns, right? So the current state > is well-defined: any previous atomic sync or async commit can be assumed to > have succeeded even if it hasn't applied in hardware yet if the commit > ioctl for it succeeded, right? Yes. We could certainly try to fully track all changes in properties, but no has measured if there's any real benefit of doing so. -- Ville Syrjälä Intel
Re: KMS atomic state sets, full vs. minimal (Re: [PATCH v3 0/6] Add support for atomic async page-flips)
On Fri, Sep 30, 2022 at 06:45:09PM +0300, Ville Syrjälä wrote: > On Fri, Sep 30, 2022 at 06:37:00PM +0300, Pekka Paalanen wrote: > > On Fri, 30 Sep 2022 18:09:55 +0300 > > Ville Syrjälä wrote: > > > > > That would actively discourage people from even attempting the > > > "just dump all the state into the ioctl" approach with async flips > > > since even the props whose value isn't even changing would be rejected. > > > > About that. > > > > To me it looks like just a classic case of broken communication. > > > > The kernel developers assume that of course userspace will minimize the > > state set to only those properties that change, because...? > > > > Userspace developers think that of course the kernel will optimize the > > state set into minimal changes, because the kernel actually has the > > authoritative knowledge of what the current state is, and the driver > > actually knows which properties are costly and need to be optimized and > > which ones don't matter. It has never even occurred to me that the > > kernel would not compare next state to current state. > > > > No-one ever documented any expectations, did they? > > Do you really want that for async flips? Async flips can't be > done atomically with anything else, so why are you even asking > the kernel to do that? Also what if you want plane 1 to do an async flip, and plane 2 to do a sync flip? With the single async flag per commit you will end up with one of the following results: plane 1 plane 2 outcome can asynccan asyncasync flip on both planes (totally not what you wanted) can asynccan't async -EINVAL (what you actually wanted but got unfairly rejected) can't async can async-EINVAL can't async can't async -EINVAL Those last two are actually reasonable outcomes since the plane where you did want to async flip didn't support it. But the first two are just nonsense results. -- Ville Syrjälä Intel
Re: KMS atomic state sets, full vs. minimal (Re: [PATCH v3 0/6] Add support for atomic async page-flips)
On Fri, Sep 30, 2022 at 06:37:00PM +0300, Pekka Paalanen wrote: > On Fri, 30 Sep 2022 18:09:55 +0300 > Ville Syrjälä wrote: > > > That would actively discourage people from even attempting the > > "just dump all the state into the ioctl" approach with async flips > > since even the props whose value isn't even changing would be rejected. > > About that. > > To me it looks like just a classic case of broken communication. > > The kernel developers assume that of course userspace will minimize the > state set to only those properties that change, because...? > > Userspace developers think that of course the kernel will optimize the > state set into minimal changes, because the kernel actually has the > authoritative knowledge of what the current state is, and the driver > actually knows which properties are costly and need to be optimized and > which ones don't matter. It has never even occurred to me that the > kernel would not compare next state to current state. > > No-one ever documented any expectations, did they? Do you really want that for async flips? Async flips can't be done atomically with anything else, so why are you even asking the kernel to do that? -- Ville Syrjälä Intel
Re: [PATCH v3 0/6] Add support for atomic async page-flips
On Fri, Sep 30, 2022 at 05:19:07PM +0300, Ville Syrjälä wrote:
> On Fri, Sep 30, 2022 at 04:52:56PM +0300, Ville Syrjälä wrote:
> > On Thu, Sep 29, 2022 at 06:43:15PM +, Simon Ser wrote:
> > > This series adds support for DRM_MODE_PAGE_FLIP_ASYNC for atomic
> > > commits, aka. "immediate flip" (which might result in tearing).
> > > The feature was only available via the legacy uAPI, however for
> > > gaming use-cases it may be desirable to enable it via the atomic
> > > uAPI too.
> > >
> > > - Patchwork: https://patchwork.freedesktop.org/series/107683/
> > > - User-space patch: https://github.com/Plagman/gamescope/pull/595
> > > - IGT patch: https://patchwork.freedesktop.org/series/107681/
> >
> > So no tests that actually verify that the kernel properly rejects
> > stuff stuff like modesets, gamma LUT updates, plane movement,
> > etc.?
>
> Pondering this a bit more, it just occurred to me the current driver
> level checks might easily lead to confusing behaviour. Eg. is
> the ioctl going to succeed if you ask for an async change of some
> random property while the crtc disabled, but fails if you ask for
> the same async property change when the crtc is active?
>
> So another reason why rejecting most properties already at
> the uapi level might be a good idea.
And just to be clear this is pretty much what I suggest:
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
b/drivers/gpu/drm/drm_atomic_uapi.c
index 79730fa1dd8e..471a2c703847 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -1392,6 +1392,13 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
goto out;
}
+ if (arg->flags & DRM_MODE_PAGE_FLIP_ASYNC &&
+ prop != dev->mode_config.prop_fb_id) {
+ drm_mode_object_put(obj);
+ ret = -EINVAL;
+ goto out;
+ }
+
if (copy_from_user(&prop_value,
prop_values_ptr + copied_props,
sizeof(prop_value))) {
That would actively discourage people from even attempting the
"just dump all the state into the ioctl" approach with async flips
since even the props whose value isn't even changing would be rejected.
--
Ville Syrjälä
Intel
Re: [PATCH v3 0/6] Add support for atomic async page-flips
On Fri, Sep 30, 2022 at 04:52:56PM +0300, Ville Syrjälä wrote: > On Thu, Sep 29, 2022 at 06:43:15PM +, Simon Ser wrote: > > This series adds support for DRM_MODE_PAGE_FLIP_ASYNC for atomic > > commits, aka. "immediate flip" (which might result in tearing). > > The feature was only available via the legacy uAPI, however for > > gaming use-cases it may be desirable to enable it via the atomic > > uAPI too. > > > > - Patchwork: https://patchwork.freedesktop.org/series/107683/ > > - User-space patch: https://github.com/Plagman/gamescope/pull/595 > > - IGT patch: https://patchwork.freedesktop.org/series/107681/ > > So no tests that actually verify that the kernel properly rejects > stuff stuff like modesets, gamma LUT updates, plane movement, > etc.? Pondering this a bit more, it just occurred to me the current driver level checks might easily lead to confusing behaviour. Eg. is the ioctl going to succeed if you ask for an async change of some random property while the crtc disabled, but fails if you ask for the same async property change when the crtc is active? So another reason why rejecting most properties already at the uapi level might be a good idea. -- Ville Syrjälä Intel
Re: [PATCH v3 3/6] drm: introduce drm_mode_config.atomic_async_page_flip_not_supported
On Fri, Sep 30, 2022 at 01:56:25PM +, Simon Ser wrote: > On Friday, September 30th, 2022 at 15:53, Ville Syrjälä > wrote: > > > > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > > > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > > > index 44235345fd57..7500e82cf06a 100644 > > > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > > > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > > > @@ -3808,6 +3808,7 @@ static int amdgpu_dm_mode_config_init(struct > > > amdgpu_device *adev) > > > adev_to_drm(adev)->mode_config.prefer_shadow = 1; > > > /* indicates support for immediate flip */ > > > adev_to_drm(adev)->mode_config.async_page_flip = true; > > > + adev_to_drm(adev)->mode_config.atomic_async_page_flip_not_supported = > > > true; > > > > The flag polarity seems weird. Why opt out and not opt in? > > Explained in the commit message. Seems a bit ambitious, but sure. -- Ville Syrjälä Intel
Re: [PATCH v3 0/6] Add support for atomic async page-flips
On Thu, Sep 29, 2022 at 06:43:15PM +, Simon Ser wrote: > This series adds support for DRM_MODE_PAGE_FLIP_ASYNC for atomic > commits, aka. "immediate flip" (which might result in tearing). > The feature was only available via the legacy uAPI, however for > gaming use-cases it may be desirable to enable it via the atomic > uAPI too. > > - Patchwork: https://patchwork.freedesktop.org/series/107683/ > - User-space patch: https://github.com/Plagman/gamescope/pull/595 > - IGT patch: https://patchwork.freedesktop.org/series/107681/ So no tests that actually verify that the kernel properly rejects stuff stuff like modesets, gamma LUT updates, plane movement, etc.? > > Main changes in v2: add docs, fail atomic commit if async flip isn't > possible. > > Changes in v3: add a note in the documentation about Intel hardware, > add R-b tags. > > Tested on an AMD Picasso iGPU (Simon) and an AMD Vangogh GPU (André). > > Simon Ser (6): > drm: document DRM_MODE_PAGE_FLIP_ASYNC > amd/display: only accept async flips for fast updates > drm: introduce drm_mode_config.atomic_async_page_flip_not_supported > drm: allow DRM_MODE_PAGE_FLIP_ASYNC for atomic commits > drm: introduce DRM_CAP_ATOMIC_ASYNC_PAGE_FLIP > amd/display: indicate support for atomic async page-flips on DC > > .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 8 ++ > .../amd/display/amdgpu_dm/amdgpu_dm_crtc.c| 10 +++ > drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 1 + > drivers/gpu/drm/drm_atomic_uapi.c | 28 +-- > drivers/gpu/drm/drm_ioctl.c | 5 > drivers/gpu/drm/i915/display/intel_display.c | 1 + > drivers/gpu/drm/nouveau/nouveau_display.c | 1 + > drivers/gpu/drm/vc4/vc4_kms.c | 1 + > include/drm/drm_mode_config.h | 11 > include/uapi/drm/drm.h| 10 ++- > include/uapi/drm/drm_mode.h | 16 +++ > 11 files changed, 88 insertions(+), 4 deletions(-) > > -- > 2.37.3 > -- Ville Syrjälä Intel
Re: [PATCH v3 3/6] drm: introduce drm_mode_config.atomic_async_page_flip_not_supported
On Thu, Sep 29, 2022 at 06:43:42PM +, Simon Ser wrote: > This new field indicates whether the driver has the necessary logic > to support async page-flips via the atomic uAPI. This is leveraged by > the next commit to allow user-space to use this functionality. > > All atomic drivers setting drm_mode_config.async_page_flip are updated > to also set drm_mode_config.atomic_async_page_flip_not_supported. We > will gradually check and update these drivers to properly handle > drm_crtc_state.async_flip in their atomic logic. > > The goal of this negative flag is the same as > fb_modifiers_not_supported: we want to eventually get rid of all > drivers missing atomic support for async flips. New drivers should not > set this flag, instead they should support atomic async flips (if > they support async flips at all). IOW, we don't want more drivers > with async flip support for legacy but not atomic. > > v2: only set the flag on atomic drivers (remove it on amdgpu DCE and > on radeon) > > Signed-off-by: Simon Ser > Reviewed-by: André Almeida > Reviewed-by: Alex Deucher > Cc: Daniel Vetter > Cc: Joshua Ashton > Cc: Melissa Wen > Cc: Harry Wentland > Cc: Nicholas Kazlauskas > Cc: Ville Syrjälä > --- > drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 1 + > drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 1 + > drivers/gpu/drm/i915/display/intel_display.c | 1 + > drivers/gpu/drm/nouveau/nouveau_display.c | 1 + > drivers/gpu/drm/vc4/vc4_kms.c | 1 + > include/drm/drm_mode_config.h | 11 +++ > 6 files changed, 16 insertions(+) > > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > index 44235345fd57..7500e82cf06a 100644 > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > @@ -3808,6 +3808,7 @@ static int amdgpu_dm_mode_config_init(struct > amdgpu_device *adev) > adev_to_drm(adev)->mode_config.prefer_shadow = 1; > /* indicates support for immediate flip */ > adev_to_drm(adev)->mode_config.async_page_flip = true; > + adev_to_drm(adev)->mode_config.atomic_async_page_flip_not_supported = > true; The flag polarity seems weird. Why opt out and not opt in? > > adev_to_drm(adev)->mode_config.fb_base = adev->gmc.aper_base; > > diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c > b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c > index f7e7f4e919c7..ffb3a2fa797f 100644 > --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c > +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c > @@ -639,6 +639,7 @@ static int atmel_hlcdc_dc_modeset_init(struct drm_device > *dev) > dev->mode_config.max_height = dc->desc->max_height; > dev->mode_config.funcs = &mode_config_funcs; > dev->mode_config.async_page_flip = true; > + dev->mode_config.atomic_async_page_flip_not_supported = true; > > return 0; > } > diff --git a/drivers/gpu/drm/i915/display/intel_display.c > b/drivers/gpu/drm/i915/display/intel_display.c > index 40fbf8a296e2..e025b3499c9d 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -8621,6 +8621,7 @@ static void intel_mode_config_init(struct > drm_i915_private *i915) > mode_config->helper_private = &intel_mode_config_funcs; > > mode_config->async_page_flip = HAS_ASYNC_FLIPS(i915); > + mode_config->atomic_async_page_flip_not_supported = true; > > /* >* Maximum framebuffer dimensions, chosen to match > diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c > b/drivers/gpu/drm/nouveau/nouveau_display.c > index a2f5df568ca5..2b5c4f24aedd 100644 > --- a/drivers/gpu/drm/nouveau/nouveau_display.c > +++ b/drivers/gpu/drm/nouveau/nouveau_display.c > @@ -699,6 +699,7 @@ nouveau_display_create(struct drm_device *dev) > dev->mode_config.async_page_flip = false; > else > dev->mode_config.async_page_flip = true; > + dev->mode_config.atomic_async_page_flip_not_supported = true; > > drm_kms_helper_poll_init(dev); > drm_kms_helper_poll_disable(dev); > diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c > index 4419e810103d..3fe59c6b2cf0 100644 > --- a/drivers/gpu/drm/vc4/vc4_kms.c > +++ b/drivers/gpu/drm/vc4/vc4_kms.c > @@ -1047,6 +1047,7 @@ int vc4_kms_load(struct drm_device *dev) > dev->mode_config.helper_private = &vc4_mode_config_helpers; > dev->mode_config.preferred_depth = 24; >
Re: [PATCH v2 4/6] drm: allow DRM_MODE_PAGE_FLIP_ASYNC for atomic commits
On Wed, Aug 31, 2022 at 02:56:12PM +, Simon Ser wrote:
> On Wednesday, August 31st, 2022 at 09:50, Pekka Paalanen
> wrote:
>
> > > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> > > index 86a292c3185a..cce1a1bea645 100644
> > > --- a/include/uapi/drm/drm_mode.h
> > > +++ b/include/uapi/drm/drm_mode.h
> > > @@ -942,6 +942,10 @@ struct hdr_output_metadata {
> > > * Request that the page-flip is performed as soon as possible, ie. with no
> > > * delay due to waiting for vblank. This may cause tearing to be visible on
> > > * the screen.
> > > + *
> > > + * When used with atomic uAPI, the driver will return an error if the
> > > hardware
> > > + * doesn't support performing an asynchronous page-flip for this update.
> > > + * User-space should handle this, e.g. by falling back to a regular
> > > page-flip.
> > > */
> > > #define DRM_MODE_PAGE_FLIP_ASYNC 0x02
> > > #define DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE 0x4
> >
> > Hi Simon,
> >
> > recalling what Ville explained that enabling async flips might require
> > one more sync flip first, how is that supposed to work?
> >
> > A TEST_ONLY commit is not allowed to change hardware state, and a
> > failing real commit is not allowed to change hardware state either
> > (right?), therefore a failing async flip cannot prepare the next flip
> > to be async, meaning async will never work.
>
> I'd blame it on bad hw, and make it one special quirk in the driver,
> just like it does now.
>
> Ville, thoughts?
I suppose it might be worth mentioning that special case here,
to avoid people getting confused why the first async flip was
accepted but took a full frame to complete.
--
Ville Syrjälä
Intel
Re: [PATCH 3/4] drm: introduce DRM_CAP_ATOMIC_ASYNC_PAGE_FLIP
On Tue, Aug 30, 2022 at 11:40:10AM +0300, Pekka Paalanen wrote: > On Tue, 30 Aug 2022 11:08:22 +0300 > Ville Syrjälä wrote: > > > On Mon, Aug 29, 2022 at 04:01:44PM +, Simon Ser wrote: > > > On Friday, August 26th, 2022 at 10:19, Ville Syrjälä > > > wrote: > > > > > > > On Wed, Aug 24, 2022 at 03:08:55PM +, Simon Ser wrote: > > > > > This new kernel capability indicates whether async page-flips are > > > > > supported via the atomic uAPI. DRM clients can use it to check > > > > > for support before feeding DRM_MODE_PAGE_FLIP_ASYNC to the kernel. > > > > > > > > I think we'd need to clarify the semantics of the async flag > > > > for atomic commits. > > > > > > > > Eg. on Intel hw only pure page flips are possible async, if you do > > > > anything else (change plane size/pos/scaling/etc.) you will need > > > > to do a sync update. Technically not even all page flips (from the > > > > uapi POV) might be possible as the exact scanout source address > > > > is specified via two registers, only one of which can be update > > > > async. So technically the two framebuffers might be laid out > > > > just slightly differently which could prevent an async flip. > > > > Also only some subset of planes actually support async flips. > > > > > > Also IIRC some format modifiers don't support async flip at all (CCS)? > > > > Yeah, that too. Also planar YUV formats aren't allowed. > > > > > > > > > And on hw where multiple planes support it on the same crtc, only one > > > > plane can do it at a time. Well, more accurately we can only select > > > > one plane at a time to give us the "flip done" interrupt. I guess > > > > if the user wants to async flip multiple planes at the same time > > > > we could do them serially as opposed to in parallel to make sure > > > > all the flips actually happened before we signal completion of the > > > > entire commit. Async flips of multiple planes probably won't > > > > happen atomically anyway so doing them serially seems fine. > > > > > > > > ATM in i915 we probably don't have sufficient state checks in > > > > place to catch all the restrictions, and instead in part we rely > > > > on the limited scope of the legacy async flip ioctl to make sure > > > > the operation doesn't attempt something the hw can't do. > > > > > > Yeah, that makes sense. > > > > > > In the documentation patch discussion [1], it appears it's not clear what > > > drivers should do when async flip isn't possible with the legacy uAPI. > > > > > > For the atomic uAPI, we need to pick on of these two approaches: > > > > > > 1. Let the kernel fall back to a sync flip if async isn't possible. This > > >simplifies user-space, but then user-space has no reliable way to > > > figure out > > >what really happened (sync or async?). That could be fixed with a new > > >read-only CRTC prop indicating whether the last flip was async or not. > > >However, maybe someone will come up in the future with user-space which > > >needs to only apply an update if async flip is possible, in which case > > > this > > >approach falls short. > > There is the pageflip completion timestamp in the DRM event. If > userspace knows the phase and period of the scanout cycle, the > completion timestamp should tell quite reliably if the update was > tearing. > > For the phase, one can query KMS for the last vblank timestamp. This > should work also for VRR I assume. > > For the period, fixed-frequency video mode has it straight. VRR gives > only a range or a minimum period. > > > > 2. Make the kernel return EINVAL if async flip isn't possible. This adds > > > more > > >complexity to user-space, but enables a more reliable and deterministic > > >uAPI. This is also more consistent with the rest of the existing atomic > > >uAPI. > > > > The current behaviour is somewhat a combination of the two. We return > > an error if async flip is not possible at all given the current state. > > > > When async flip is possible we return success, but may still internally > > fall back to a sync flip for the first flip. That is required on some > > borked hardware that can't switch from sync flips to async
Re: [PATCH 3/4] drm: introduce DRM_CAP_ATOMIC_ASYNC_PAGE_FLIP
On Mon, Aug 29, 2022 at 04:01:44PM +, Simon Ser wrote: > On Friday, August 26th, 2022 at 10:19, Ville Syrjälä > wrote: > > > On Wed, Aug 24, 2022 at 03:08:55PM +, Simon Ser wrote: > > > This new kernel capability indicates whether async page-flips are > > > supported via the atomic uAPI. DRM clients can use it to check > > > for support before feeding DRM_MODE_PAGE_FLIP_ASYNC to the kernel. > > > > I think we'd need to clarify the semantics of the async flag > > for atomic commits. > > > > Eg. on Intel hw only pure page flips are possible async, if you do > > anything else (change plane size/pos/scaling/etc.) you will need > > to do a sync update. Technically not even all page flips (from the > > uapi POV) might be possible as the exact scanout source address > > is specified via two registers, only one of which can be update > > async. So technically the two framebuffers might be laid out > > just slightly differently which could prevent an async flip. > > Also only some subset of planes actually support async flips. > > Also IIRC some format modifiers don't support async flip at all (CCS)? Yeah, that too. Also planar YUV formats aren't allowed. > > > And on hw where multiple planes support it on the same crtc, only one > > plane can do it at a time. Well, more accurately we can only select > > one plane at a time to give us the "flip done" interrupt. I guess > > if the user wants to async flip multiple planes at the same time > > we could do them serially as opposed to in parallel to make sure > > all the flips actually happened before we signal completion of the > > entire commit. Async flips of multiple planes probably won't > > happen atomically anyway so doing them serially seems fine. > > > > ATM in i915 we probably don't have sufficient state checks in > > place to catch all the restrictions, and instead in part we rely > > on the limited scope of the legacy async flip ioctl to make sure > > the operation doesn't attempt something the hw can't do. > > Yeah, that makes sense. > > In the documentation patch discussion [1], it appears it's not clear what > drivers should do when async flip isn't possible with the legacy uAPI. > > For the atomic uAPI, we need to pick on of these two approaches: > > 1. Let the kernel fall back to a sync flip if async isn't possible. This >simplifies user-space, but then user-space has no reliable way to figure > out >what really happened (sync or async?). That could be fixed with a new >read-only CRTC prop indicating whether the last flip was async or not. >However, maybe someone will come up in the future with user-space which >needs to only apply an update if async flip is possible, in which case this >approach falls short. > 2. Make the kernel return EINVAL if async flip isn't possible. This adds more >complexity to user-space, but enables a more reliable and deterministic >uAPI. This is also more consistent with the rest of the existing atomic >uAPI. The current behaviour is somewhat a combination of the two. We return an error if async flip is not possible at all given the current state. When async flip is possible we return success, but may still internally fall back to a sync flip for the first flip. That is required on some borked hardware that can't switch from sync flips to async flips without doing an extra sync flip. Also on some other hardware we intentionally fall back to a sync flip for the first async flip, so that we can reprogram some display FIFO stuff (aimed to make the subsequent async flips faster). > > Note, current user-space would only need to opportunistically enable async > flip. IOW, I think that for current user-space plans "async if possible, > otherwise sync" is good enough. That behavior maps well to the Vulkan present > modes as well (which says that "this mode *may* result in visible tearing", > but > doesn't guarantee it). The current behaviour is to fall back to a blit if the async flip fails. So you still get the same effective behaviour, just not as efficient. I think that's a reasonable way to handle it. > > Another possible shortcoming of the proposed new uAPI here is that user-space > cannot submit a single atomic commit which updates multiple CRTCs, and > individually select which CRTC does an async flip. This could be fixed with > a "ASYNC_FLIP" CRTC prop which the kernel always resets to 0 on commit. I'm > not > sure we want/need to cross that bridge right now, it would be easy enough to > add as a second step if some user-space would benefit
Re: [PATCH 3/4] drm: introduce DRM_CAP_ATOMIC_ASYNC_PAGE_FLIP
On Wed, Aug 24, 2022 at 03:08:55PM +, Simon Ser wrote:
> This new kernel capability indicates whether async page-flips are
> supported via the atomic uAPI. DRM clients can use it to check
> for support before feeding DRM_MODE_PAGE_FLIP_ASYNC to the kernel.
I think we'd need to clarify the semantics of the async flag
for atomic commits.
Eg. on Intel hw only pure page flips are possible async, if you do
anything else (change plane size/pos/scaling/etc.) you will need
to do a sync update. Technically not even all page flips (from the
uapi POV) might be possible as the exact scanout source address
is specified via two registers, only one of which can be update
async. So technically the two framebuffers might be laid out
just slightly differently which could prevent an async flip.
Also only some subset of planes actually support async flips.
And on hw where multiple planes support it on the same crtc, only one
plane can do it at a time. Well, more accurately we can only select
one plane at a time to give us the "flip done" interrupt. I guess
if the user wants to async flip multiple planes at the same time
we could do them serially as opposed to in parallel to make sure
all the flips actually happened before we signal completion of the
entire commit. Async flips of multiple planes probably won't
happen atomically anyway so doing them serially seems fine.
ATM in i915 we probably don't have sufficient state checks in
place to catch all the restrictions, and instead in part we rely
on the limited scope of the legacy async flip ioctl to make sure
the operation doesn't attempt something the hw can't do.
> Make it clear that DRM_CAP_ASYNC_PAGE_FLIP is for legacy uAPI only.
>
> Signed-off-by: Simon Ser
> Cc: Daniel Vetter
> Cc: Joshua Ashton
> Cc: Melissa Wen
> Cc: Alex Deucher
> Cc: Harry Wentland
> Cc: Nicholas Kazlauskas
> ---
> drivers/gpu/drm/drm_ioctl.c | 5 +
> include/uapi/drm/drm.h | 10 +-
> 2 files changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index ca2a6e6101dc..5b1591e2b46c 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -302,6 +302,11 @@ static int drm_getcap(struct drm_device *dev, void
> *data, struct drm_file *file_
> case DRM_CAP_CRTC_IN_VBLANK_EVENT:
> req->value = 1;
> break;
> + case DRM_CAP_ATOMIC_ASYNC_PAGE_FLIP:
> + req->value = drm_core_check_feature(dev, DRIVER_ATOMIC) &&
> + dev->mode_config.async_page_flip &&
> +
> !dev->mode_config.atomic_async_page_flip_not_supported;
> + break;
> default:
> return -EINVAL;
> }
> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> index 642808520d92..b1962628ecda 100644
> --- a/include/uapi/drm/drm.h
> +++ b/include/uapi/drm/drm.h
> @@ -706,7 +706,8 @@ struct drm_gem_open {
> /**
> * DRM_CAP_ASYNC_PAGE_FLIP
> *
> - * If set to 1, the driver supports &DRM_MODE_PAGE_FLIP_ASYNC.
> + * If set to 1, the driver supports &DRM_MODE_PAGE_FLIP_ASYNC for legacy
> + * page-flips.
> */
> #define DRM_CAP_ASYNC_PAGE_FLIP 0x7
> /**
> @@ -767,6 +768,13 @@ struct drm_gem_open {
> * Documentation/gpu/drm-mm.rst, section "DRM Sync Objects".
> */
> #define DRM_CAP_SYNCOBJ_TIMELINE 0x14
> +/**
> + * DRM_CAP_ATOMIC_ASYNC_PAGE_FLIP
> + *
> + * If set to 1, the driver supports &DRM_MODE_PAGE_FLIP_ASYNC for atomic
> + * commits.
> + */
> +#define DRM_CAP_ATOMIC_ASYNC_PAGE_FLIP 0x15
>
> /* DRM_IOCTL_GET_CAP ioctl argument type */
> struct drm_get_cap {
> --
> 2.37.2
>
--
Ville Syrjälä
Intel
Re: [RFC PATCH 4/5] drm/drm_color_mgmt: add 3D LUT to color mgmt properties
On Sun, Jun 19, 2022 at 09:31:03PM -0100, Melissa Wen wrote: > Add 3D LUT for gammar correction using a 3D lookup table. The position > in the color correction pipeline where 3D LUT is applied depends on hw > design, being after CTM or gamma. If just after CTM, a shaper lut must > be set to shape the content for a non-linear space. That details should > be handled by the driver according to its color capabilities. I also cooked up a WIP 3D LUT support some time ago for Intel hw: https://github.com/vsyrjala/linux/commits/3dlut But that dried up due to having no userspace for it. I also cooked up some basic igts for it: https://patchwork.freedesktop.org/series/90165/ > + * “LUT3D”: > + * Blob property to set the 3D LUT mapping pixel data after the color > + * transformation matrix and before gamma 1D lut correction. On Intel hw the 3DLUT is after the gamma LUT in the pipeline, which is where I placed it in my branch. There is now some discussion happening about exposing some kind of color pipeline description/configuration properties: https://gitlab.freedesktop.org/pq/color-and-hdr/-/issues/11 -- Ville Syrjälä Intel
Re: [Intel-gfx] [RFC 09/19] drm/edid: convert drm_edid_to_sad() to use cea db iter
On Tue, Mar 22, 2022 at 11:40:38PM +0200, Jani Nikula wrote:
> Use the cea db iterator for short audio descriptors. We'll still stop at
> the first audio data block, but not at the first CEA extension if that
> doesn't have the info.
This stuff should probably be converted over to the drm_edid_to_eld()
approach which looks up all the SADs from the whole EDID. But that's
something for amdgpu/radeon folks to think about since they're the only
user.
>
> Signed-off-by: Jani Nikula
> ---
> drivers/gpu/drm/drm_edid.c | 34 +-
> 1 file changed, 9 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 992b3578a73f..e341790521d6 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -4854,40 +4854,21 @@ static void drm_edid_to_eld(struct drm_connector
> *connector, struct edid *edid)
> */
> int drm_edid_to_sad(struct edid *edid, struct cea_sad **sads)
> {
> + const struct cea_db *db;
> + struct cea_db_iter iter;
> int count = 0;
> - int i, start, end, dbl;
> - const u8 *cea;
> -
> - cea = drm_find_cea_extension(edid);
> - if (!cea) {
> - DRM_DEBUG_KMS("SAD: no CEA Extension found\n");
> - return 0;
> - }
> -
> - if (cea_revision(cea) < 3) {
> - DRM_DEBUG_KMS("SAD: wrong CEA revision\n");
> - return 0;
> - }
> -
> - if (cea_db_offsets(cea, &start, &end)) {
> - DRM_DEBUG_KMS("SAD: invalid data block offsets\n");
> - return -EPROTO;
> - }
> -
> - for_each_cea_db(cea, i, start, end) {
> - const u8 *db = &cea[i];
>
> + cea_db_iter_edid_begin(edid, &iter);
> + cea_db_iter_for_each(db, &iter) {
> if (cea_db_tag(db) == CEA_DB_AUDIO) {
> int j;
>
> - dbl = cea_db_payload_len(db);
> -
> - count = dbl / 3; /* SAD is 3B */
> + count = cea_db_payload_len(db) / 3; /* SAD is 3B */
> *sads = kcalloc(count, sizeof(**sads), GFP_KERNEL);
> if (!*sads)
> return -ENOMEM;
> for (j = 0; j < count; j++) {
> - const u8 *sad = &db[1 + j * 3];
> + const u8 *sad = &db->data[j * 3];
>
> (*sads)[j].format = (sad[0] & 0x78) >> 3;
> (*sads)[j].channels = sad[0] & 0x7;
> @@ -4897,6 +4878,9 @@ int drm_edid_to_sad(struct edid *edid, struct cea_sad
> **sads)
> break;
> }
> }
> + cea_db_iter_end(&iter);
> +
> + DRM_DEBUG_KMS("Found %d Short Audio Descriptors\n", count);
>
> return count;
> }
> --
> 2.30.2
--
Ville Syrjälä
Intel
Re: [PATCH 00/22] drm: Review of mode copies
On Wed, Mar 23, 2022 at 01:39:44PM +0300, Dmitry Baryshkov wrote: > On 22/03/2022 01:37, Ville Syrjälä wrote: > > On Tue, Mar 15, 2022 at 02:52:38PM -0400, Alex Deucher wrote: > >> On Mon, Mar 14, 2022 at 6:12 PM Ville Syrjälä > >> wrote: > >>> > >>> On Fri, Feb 18, 2022 at 12:03:41PM +0200, Ville Syrjala wrote: > >>>>drm: Add drm_mode_init() > >>>>drm/bridge: Use drm_mode_copy() > >>>>drm/imx: Use drm_mode_duplicate() > >>>>drm/panel: Use drm_mode_duplicate() > >>>>drm/vc4: Use drm_mode_copy() > >>> These have been pushed to drm-misc-next. > >>> > >>>>drm/amdgpu: Remove pointless on stack mode copies > >>>>drm/amdgpu: Use drm_mode_init() for on-stack modes > >>>>drm/amdgpu: Use drm_mode_copy() > >>> amdgpu ones are reviewed, but I'll leave them for the > >>> AMD folks to push to whichever tree they prefer. > >> > >> I pulled patches 2, 4, 5 into my tree. > > > > Thanks. > > > >> For 3, I'm happy to have it > >> land via drm-misc with the rest of the mode_init changes if you'd > >> prefer. > > > > Either way works for me. I don't yet have reviews yet for > > the other drivers, so I'll proably hold off for a bit more > > at least. And the i915 patch I'll be merging via drm-intel. > > > >>>>drm/radeon: Use drm_mode_copy() > >>>>drm/gma500: Use drm_mode_copy() > >>>>drm/tilcdc: Use drm_mode_copy() > >>>>drm/i915: Use drm_mode_copy() > > > > Those are now all in. > > > > Which leaves us with these stragglers: > >>>>drm/hisilicon: Use drm_mode_init() for on-stack modes > > >>>>drm/msm: Nuke weird on stack mode copy > >>>>drm/msm: Use drm_mode_init() for on-stack modes > >>>>drm/msm: Use drm_mode_copy() > > These three patches went beneath my radar for some reason. > > I have just sent a replacement for the first patch ([1]). Other two > patches can be pulled in msm/msm-next (or msm/msm-fixes) during the next > development cycle if that works for you. That'll do fine for me. Thanks. > > [1] https://patchwork.freedesktop.org/series/101682/ > > >>>>drm/mtk: Use drm_mode_init() for on-stack modes > >>>>drm/rockchip: Use drm_mode_copy() > >>>>drm/sti: Use drm_mode_copy() > >>>>drm: Use drm_mode_init() for on-stack modes > >>>>drm: Use drm_mode_copy() > > > > > -- > With best wishes > Dmitry -- Ville Syrjälä Intel
Re: [PATCH 00/22] drm: Review of mode copies
On Tue, Mar 15, 2022 at 02:52:38PM -0400, Alex Deucher wrote: > On Mon, Mar 14, 2022 at 6:12 PM Ville Syrjälä > wrote: > > > > On Fri, Feb 18, 2022 at 12:03:41PM +0200, Ville Syrjala wrote: > > > drm: Add drm_mode_init() > > > drm/bridge: Use drm_mode_copy() > > > drm/imx: Use drm_mode_duplicate() > > > drm/panel: Use drm_mode_duplicate() > > > drm/vc4: Use drm_mode_copy() > > These have been pushed to drm-misc-next. > > > > > drm/amdgpu: Remove pointless on stack mode copies > > > drm/amdgpu: Use drm_mode_init() for on-stack modes > > > drm/amdgpu: Use drm_mode_copy() > > amdgpu ones are reviewed, but I'll leave them for the > > AMD folks to push to whichever tree they prefer. > > I pulled patches 2, 4, 5 into my tree. Thanks. > For 3, I'm happy to have it > land via drm-misc with the rest of the mode_init changes if you'd > prefer. Either way works for me. I don't yet have reviews yet for the other drivers, so I'll proably hold off for a bit more at least. And the i915 patch I'll be merging via drm-intel. > > > drm/radeon: Use drm_mode_copy() > > > drm/gma500: Use drm_mode_copy() > > > drm/tilcdc: Use drm_mode_copy() > > > drm/i915: Use drm_mode_copy() Those are now all in. Which leaves us with these stragglers: > > > drm/hisilicon: Use drm_mode_init() for on-stack modes > > > drm/msm: Nuke weird on stack mode copy > > > drm/msm: Use drm_mode_init() for on-stack modes > > > drm/msm: Use drm_mode_copy() > > > drm/mtk: Use drm_mode_init() for on-stack modes > > > drm/rockchip: Use drm_mode_copy() > > > drm/sti: Use drm_mode_copy() > > > drm: Use drm_mode_init() for on-stack modes > > > drm: Use drm_mode_copy() -- Ville Syrjälä Intel
Re: [PATCH 00/22] drm: Review of mode copies
On Fri, Feb 18, 2022 at 12:03:41PM +0200, Ville Syrjala wrote: > drm: Add drm_mode_init() > drm/bridge: Use drm_mode_copy() > drm/imx: Use drm_mode_duplicate() > drm/panel: Use drm_mode_duplicate() > drm/vc4: Use drm_mode_copy() These have been pushed to drm-misc-next. > drm/amdgpu: Remove pointless on stack mode copies > drm/amdgpu: Use drm_mode_init() for on-stack modes > drm/amdgpu: Use drm_mode_copy() amdgpu ones are reviewed, but I'll leave them for the AMD folks to push to whichever tree they prefer. The rest are still in need of review: > drm/radeon: Use drm_mode_copy() > drm/gma500: Use drm_mode_copy() > drm/hisilicon: Use drm_mode_init() for on-stack modes > drm/msm: Nuke weird on stack mode copy > drm/msm: Use drm_mode_init() for on-stack modes > drm/msm: Use drm_mode_copy() > drm/mtk: Use drm_mode_init() for on-stack modes > drm/rockchip: Use drm_mode_copy() > drm/sti: Use drm_mode_copy() > drm/tilcdc: Use drm_mode_copy() > drm/i915: Use drm_mode_init() for on-stack modes > drm/i915: Use drm_mode_copy() > drm: Use drm_mode_init() for on-stack modes > drm: Use drm_mode_copy() -- Ville Syrjälä Intel
Re: [Intel-gfx] [PATCH v7 1/3] gpu: drm: separate panel orientation property creating and value setting
On Tue, Feb 08, 2022 at 03:37:12PM +0800, Hsin-Yi Wang wrote:
> +int drm_connector_init_panel_orientation_property(
> + struct drm_connector *connector)
> +{
> + struct drm_device *dev = connector->dev;
> + struct drm_property *prop;
> +
> + prop = drm_property_create_enum(dev, DRM_MODE_PROP_IMMUTABLE,
> + "panel orientation",
> + drm_panel_orientation_enum_list,
> + ARRAY_SIZE(drm_panel_orientation_enum_list));
> + if (!prop)
> + return -ENOMEM;
> +
> + dev->mode_config.panel_orientation_property = prop;
Leak when called multiple times. I guess you could just put
this into drm_connector_create_standard_properties() instead
and avoid that issue entirely.
--
Ville Syrjälä
Intel
Re: [PATCH 1/7] drm/i915: Replace kmap() with kmap_local_page()
On Fri, Dec 10, 2021 at 03:23:58PM -0800, ira.we...@intel.com wrote:
> From: Ira Weiny
>
> kmap() is being deprecated and these usages are all local to the thread
> so there is no reason kmap_local_page() can't be used.
>
> Replace kmap() calls with kmap_local_page().
>
> Signed-off-by: Ira Weiny
> ---
> drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 4 ++--
> drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c | 8
> drivers/gpu/drm/i915/gt/intel_ggtt_fencing.c | 4 ++--
> drivers/gpu/drm/i915/gt/shmem_utils.c | 4 ++--
> drivers/gpu/drm/i915/i915_gem.c| 8
> drivers/gpu/drm/i915/i915_gpu_error.c | 4 ++--
> 6 files changed, 16 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> index d77da59fae04..fa8b820e14aa 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> @@ -597,9 +597,9 @@ i915_gem_object_create_shmem_from_data(struct
> drm_i915_private *dev_priv,
> if (err < 0)
> goto fail;
>
> - vaddr = kmap(page);
> + vaddr = kmap_local_page(page);
> memcpy(vaddr, data, len);
> - kunmap(page);
> + kunmap_local(vaddr);
>
> err = pagecache_write_end(file, file->f_mapping,
> offset, len, len,
> diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c
> b/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c
> index 6d30cdfa80f3..e59e1725e29d 100644
> --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c
> +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c
> @@ -144,7 +144,7 @@ static int check_partial_mapping(struct
> drm_i915_gem_object *obj,
> intel_gt_flush_ggtt_writes(&to_i915(obj->base.dev)->gt);
>
> p = i915_gem_object_get_page(obj, offset >> PAGE_SHIFT);
> - cpu = kmap(p) + offset_in_page(offset);
> + cpu = kmap_local_page(p) + offset_in_page(offset);
Does kunmap_local() do some magic to make it work even when you
don't pass in the same value you got from kmap_local_page()?
> drm_clflush_virt_range(cpu, sizeof(*cpu));
> if (*cpu != (u32)page) {
> pr_err("Partial view for %lu [%u] (offset=%llu, size=%u [%llu,
> row size %u], fence=%d, tiling=%d, stride=%d) misalignment, expected write to
> page (%llu + %u [0x%llx]) of 0x%x, found 0x%x\n",
> @@ -162,7 +162,7 @@ static int check_partial_mapping(struct
> drm_i915_gem_object *obj,
> }
> *cpu = 0;
> drm_clflush_virt_range(cpu, sizeof(*cpu));
> - kunmap(p);
> + kunmap_local(cpu);
>
> out:
> __i915_vma_put(vma);
--
Ville Syrjälä
Intel
Re: [PATCH] drm/amdkfd: Use max() instead of doing it manually
On Fri, Dec 03, 2021 at 05:31:53PM +0800, Jiapeng Chong wrote:
> Fix following coccicheck warning:
>
> ./drivers/gpu/drm/amd/amdkfd/kfd_svm.c:2193:16-17: WARNING opportunity
> for max().
>
> Reported-by: Abaci Robot
> Signed-off-by: Jiapeng Chong
> ---
> drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> index f2db49c..4f7e7b1 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> @@ -2190,7 +2190,7 @@ void schedule_deferred_list_work(struct svm_range_list
> *svms)
>
> start = mni->interval_tree.start;
> last = mni->interval_tree.last;
> - start = (start > range->start ? start : range->start) >> PAGE_SHIFT;
> + start = max(start, range->start) >> PAGE_SHIFT;
> last = (last < (range->end - 1) ? last : range->end - 1) >> PAGE_SHIFT;
There's an open coded min() on the very next line.
> pr_debug("[0x%lx 0x%lx] range[0x%lx 0x%lx] notifier[0x%lx 0x%lx] %d\n",
>start, last, range->start >> PAGE_SHIFT,
> --
> 1.8.3.1
--
Ville Syrjälä
Intel
Re: [PATCH 01/13] drm/connector: Add define for HDMI 1.4 Maximum Pixel Rate
On Thu, Nov 04, 2021 at 09:48:41AM +0100, Maxime Ripard wrote:
> Hi Ville,
>
> On Wed, Nov 03, 2021 at 08:05:16PM +0200, Ville Syrjälä wrote:
> > On Wed, Nov 03, 2021 at 01:02:11PM +0200, Ville Syrjälä wrote:
> > > On Tue, Nov 02, 2021 at 03:59:32PM +0100, Maxime Ripard wrote:
> > > > --- a/drivers/gpu/drm/drm_edid.c
> > > > +++ b/drivers/gpu/drm/drm_edid.c
> > > > @@ -4966,7 +4966,7 @@ static void drm_parse_hdmi_forum_vsdb(struct
> > > > drm_connector *connector,
> > > > u32 max_tmds_clock = hf_vsdb[5] * 5000;
> > > > struct drm_scdc *scdc = &hdmi->scdc;
> > > >
> > > > - if (max_tmds_clock > 34) {
> > > > + if (max_tmds_clock > DRM_HDMI_14_MAX_TMDS_CLK_KHZ) {
> > > > display->max_tmds_clock = max_tmds_clock;
> > > > DRM_DEBUG_KMS("HF-VSDB: max TMDS clock %d
> > > > kHz\n",
> > > > display->max_tmds_clock);
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c
> > > > b/drivers/gpu/drm/i915/display/intel_hdmi.c
> > > > index d2e61f6c6e08..0666203d52b7 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_hdmi.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
> > > > @@ -2226,7 +2226,7 @@ int intel_hdmi_compute_config(struct
> > > > intel_encoder *encoder,
> > > > if (scdc->scrambling.low_rates)
> > > > pipe_config->hdmi_scrambling = true;
> > > >
> > > > - if (pipe_config->port_clock > 34) {
> > > > + if (pipe_config->port_clock >
> > > > DRM_HDMI_14_MAX_TMDS_CLK_KHZ) {
> > > > pipe_config->hdmi_scrambling = true;
> > > > pipe_config->hdmi_high_tmds_clock_ratio = true;
> > > > }
> > >
> > > All of that is HDMI 2.0 stuff. So this just makes it all super
> > > confusing IMO. Nak.
> >
> > So reading throgh HDMI 1.4 again it does specify 340 MHz as some kind
> > of upper limit for the physical cable. But nowhere else is that number
> > really mentioned AFAICS. HDMI 2.0 does talk quite a bit about the 340
> > Mcsc limit in various places.
> >
> > I wonder what people would think of a couple of helpers like:
> > - drm_hdmi_{can,must}_use_scrambling()
> > - drm_hdmi_is_high_tmds_clock_ratio()
> > or something along those lines? At least with those the code would
> > read decently and I wouldn't have to wonder what this HDMI 1.4 TMDS
> > clock limit really is.
>
> Patch 2 introduces something along those lines.
>
> It doesn't cover everything though, we're using this define in vc4 to
> limit the available modes in mode_valid on HDMI controllers not
> 4k-capable
I wouldn't want to use this kind of define for those kinds of checks
anyway. If the hardware has specific limits in what kind of clocks it
can generate (or what it was validated for) IMO you should spell
those out explicitly instead of assuming they happen to match
some standard defined max value.
--
Ville Syrjälä
Intel
Re: [PATCH 01/13] drm/connector: Add define for HDMI 1.4 Maximum Pixel Rate
On Wed, Nov 03, 2021 at 01:02:11PM +0200, Ville Syrjälä wrote:
> On Tue, Nov 02, 2021 at 03:59:32PM +0100, Maxime Ripard wrote:
> > --- a/drivers/gpu/drm/drm_edid.c
> > +++ b/drivers/gpu/drm/drm_edid.c
> > @@ -4966,7 +4966,7 @@ static void drm_parse_hdmi_forum_vsdb(struct
> > drm_connector *connector,
> > u32 max_tmds_clock = hf_vsdb[5] * 5000;
> > struct drm_scdc *scdc = &hdmi->scdc;
> >
> > - if (max_tmds_clock > 34) {
> > + if (max_tmds_clock > DRM_HDMI_14_MAX_TMDS_CLK_KHZ) {
> > display->max_tmds_clock = max_tmds_clock;
> > DRM_DEBUG_KMS("HF-VSDB: max TMDS clock %d kHz\n",
> > display->max_tmds_clock);
> > diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c
> > b/drivers/gpu/drm/i915/display/intel_hdmi.c
> > index d2e61f6c6e08..0666203d52b7 100644
> > --- a/drivers/gpu/drm/i915/display/intel_hdmi.c
> > +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
> > @@ -2226,7 +2226,7 @@ int intel_hdmi_compute_config(struct intel_encoder
> > *encoder,
> > if (scdc->scrambling.low_rates)
> > pipe_config->hdmi_scrambling = true;
> >
> > - if (pipe_config->port_clock > 34) {
> > + if (pipe_config->port_clock > DRM_HDMI_14_MAX_TMDS_CLK_KHZ) {
> > pipe_config->hdmi_scrambling = true;
> > pipe_config->hdmi_high_tmds_clock_ratio = true;
> > }
>
> All of that is HDMI 2.0 stuff. So this just makes it all super
> confusing IMO. Nak.
So reading throgh HDMI 1.4 again it does specify 340 MHz as some kind
of upper limit for the physical cable. But nowhere else is that number
really mentioned AFAICS. HDMI 2.0 does talk quite a bit about the 340
Mcsc limit in various places.
I wonder what people would think of a couple of helpers like:
- drm_hdmi_{can,must}_use_scrambling()
- drm_hdmi_is_high_tmds_clock_ratio()
or something along those lines? At least with those the code would
read decently and I wouldn't have to wonder what this HDMI 1.4 TMDS
clock limit really is.
--
Ville Syrjälä
Intel
Re: [PATCH 01/13] drm/connector: Add define for HDMI 1.4 Maximum Pixel Rate
On Tue, Nov 02, 2021 at 03:59:32PM +0100, Maxime Ripard wrote:
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -4966,7 +4966,7 @@ static void drm_parse_hdmi_forum_vsdb(struct
> drm_connector *connector,
> u32 max_tmds_clock = hf_vsdb[5] * 5000;
> struct drm_scdc *scdc = &hdmi->scdc;
>
> - if (max_tmds_clock > 34) {
> + if (max_tmds_clock > DRM_HDMI_14_MAX_TMDS_CLK_KHZ) {
> display->max_tmds_clock = max_tmds_clock;
> DRM_DEBUG_KMS("HF-VSDB: max TMDS clock %d kHz\n",
> display->max_tmds_clock);
> diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c
> b/drivers/gpu/drm/i915/display/intel_hdmi.c
> index d2e61f6c6e08..0666203d52b7 100644
> --- a/drivers/gpu/drm/i915/display/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
> @@ -2226,7 +2226,7 @@ int intel_hdmi_compute_config(struct intel_encoder
> *encoder,
> if (scdc->scrambling.low_rates)
> pipe_config->hdmi_scrambling = true;
>
> - if (pipe_config->port_clock > 34) {
> + if (pipe_config->port_clock > DRM_HDMI_14_MAX_TMDS_CLK_KHZ) {
> pipe_config->hdmi_scrambling = true;
> pipe_config->hdmi_high_tmds_clock_ratio = true;
> }
All of that is HDMI 2.0 stuff. So this just makes it all super
confusing IMO. Nak.
--
Ville Syrjälä
Intel
Re: [Intel-gfx] [PATCH v3 13/13] drm/i915: replace drm_detect_hdmi_monitor() with drm_display_info.is_hdmi
On Fri, Oct 22, 2021 at 03:01:52PM +0300, Ville Syrjälä wrote:
> On Fri, Oct 22, 2021 at 12:25:33PM +0200, Claudio Suarez wrote:
> > On Thu, Oct 21, 2021 at 04:49:59PM +0300, Ville Syrjälä wrote:
> > > On Wed, Oct 20, 2021 at 12:51:21AM +0200, Claudio Suarez wrote:
> > > > drm_get_edid() internally calls to drm_connector_update_edid_property()
> > > > and then drm_add_display_info(), which parses the EDID.
> > > > This happens in the function intel_hdmi_set_edid() and
> > > > intel_sdvo_tmds_sink_detect() (via intel_sdvo_get_edid()).
> > > >
> > > > Once EDID is parsed, the monitor HDMI support information is available
> > > > through drm_display_info.is_hdmi. Retriving the same information with
> > > > drm_detect_hdmi_monitor() is less efficient. Change to
> > > > drm_display_info.is_hdmi
> > >
> > > I meant we need to examine all call chains that can lead to
> > > .detect() to make sure all of them do in fact update the
> > > display_info beforehand.
> >
> > Well, I studied it carefully and, yes, all call chains that can lead to
> > drm_display_info.is_hdmi / drm_detect_hdmi_monitor() update display_info
> > beforehand. In the case that this doesn't happen, the code is unchanged.
> >
> > Do you want I explain the changes in the code here again ? Or do you want
> > to me change the commit message to be more clear ? In the first case, I can
> > write here a detailed explanation. In the second case I can make a longer
> > commit
> > message.
> >
> > Or both?
>
> I want all those call chains explained in the commit message,
> otherwise I have no easy way to confirm whether the change
> is correct or not.
Hmm. OK, so I had a bit of a dig around and seems that what we do now
.detect()->drm_get_edid()->drm_connector_update_edid_property()->drm_add_display_info()
Now the question is when did that start happening? Looks like it was
commit 4b4df570b41d ("drm: Update edid-derived drm_display_info fields
at edid property set [v2]") that started to call drm_add_display_info()
from drm_connector_update_edid_property(), and then commit 5186421cbfe2
("drm: Introduce epoch counter to drm_connector") started to call
drm_connector_update_edid_property() from drm_get_edid(). Before both
of those commits were in place display_info would still contain
some stale garbage during .detect().
That is the story I think we want in these commit messages since it
a) explains why the old code was directly parsing the edid instead
b) why it's now safe to change this
PS. connector->force handling in drm_get_edid() looks a bit busted
since it doesn't call drm_connector_update_edid_property() at all
in some cases. I think there might be some path that leads there
anywya if/when we change connector->force, but we should fix
drm_get_edid() to do the right thing regarless.
--
Ville Syrjälä
Intel
Re: [PATCH v3 13/13] drm/i915: replace drm_detect_hdmi_monitor() with drm_display_info.is_hdmi
On Fri, Oct 22, 2021 at 12:25:33PM +0200, Claudio Suarez wrote: > On Thu, Oct 21, 2021 at 04:49:59PM +0300, Ville Syrjälä wrote: > > On Wed, Oct 20, 2021 at 12:51:21AM +0200, Claudio Suarez wrote: > > > drm_get_edid() internally calls to drm_connector_update_edid_property() > > > and then drm_add_display_info(), which parses the EDID. > > > This happens in the function intel_hdmi_set_edid() and > > > intel_sdvo_tmds_sink_detect() (via intel_sdvo_get_edid()). > > > > > > Once EDID is parsed, the monitor HDMI support information is available > > > through drm_display_info.is_hdmi. Retriving the same information with > > > drm_detect_hdmi_monitor() is less efficient. Change to > > > drm_display_info.is_hdmi > > > > I meant we need to examine all call chains that can lead to > > .detect() to make sure all of them do in fact update the > > display_info beforehand. > > Well, I studied it carefully and, yes, all call chains that can lead to > drm_display_info.is_hdmi / drm_detect_hdmi_monitor() update display_info > beforehand. In the case that this doesn't happen, the code is unchanged. > > Do you want I explain the changes in the code here again ? Or do you want > to me change the commit message to be more clear ? In the first case, I can > write here a detailed explanation. In the second case I can make a longer > commit > message. > > Or both? I want all those call chains explained in the commit message, otherwise I have no easy way to confirm whether the change is correct or not. -- Ville Syrjälä Intel
Re: [PATCH v3 13/13] drm/i915: replace drm_detect_hdmi_monitor() with drm_display_info.is_hdmi
On Wed, Oct 20, 2021 at 12:51:21AM +0200, Claudio Suarez wrote:
> drm_get_edid() internally calls to drm_connector_update_edid_property()
> and then drm_add_display_info(), which parses the EDID.
> This happens in the function intel_hdmi_set_edid() and
> intel_sdvo_tmds_sink_detect() (via intel_sdvo_get_edid()).
>
> Once EDID is parsed, the monitor HDMI support information is available
> through drm_display_info.is_hdmi. Retriving the same information with
> drm_detect_hdmi_monitor() is less efficient. Change to
> drm_display_info.is_hdmi
I meant we need to examine all call chains that can lead to
.detect() to make sure all of them do in fact update the
display_info beforehand.
>
> This is a TODO task in Documentation/gpu/todo.rst
>
> Signed-off-by: Claudio Suarez
> ---
> drivers/gpu/drm/i915/display/intel_hdmi.c | 2 +-
> drivers/gpu/drm/i915/display/intel_sdvo.c | 3 ++-
> 2 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c
> b/drivers/gpu/drm/i915/display/intel_hdmi.c
> index b04685bb6439..008e5b0ba408 100644
> --- a/drivers/gpu/drm/i915/display/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
> @@ -2355,7 +2355,7 @@ intel_hdmi_set_edid(struct drm_connector *connector)
> to_intel_connector(connector)->detect_edid = edid;
> if (edid && edid->input & DRM_EDID_INPUT_DIGITAL) {
> intel_hdmi->has_audio = drm_detect_monitor_audio(edid);
> - intel_hdmi->has_hdmi_sink = drm_detect_hdmi_monitor(edid);
> + intel_hdmi->has_hdmi_sink = connector->display_info.is_hdmi;
>
> connected = true;
> }
> diff --git a/drivers/gpu/drm/i915/display/intel_sdvo.c
> b/drivers/gpu/drm/i915/display/intel_sdvo.c
> index 6cb27599ea03..b4065e4df644 100644
> --- a/drivers/gpu/drm/i915/display/intel_sdvo.c
> +++ b/drivers/gpu/drm/i915/display/intel_sdvo.c
> @@ -2060,8 +2060,9 @@ intel_sdvo_tmds_sink_detect(struct drm_connector
> *connector)
> if (edid->input & DRM_EDID_INPUT_DIGITAL) {
> status = connector_status_connected;
> if (intel_sdvo_connector->is_hdmi) {
> - intel_sdvo->has_hdmi_monitor =
> drm_detect_hdmi_monitor(edid);
> intel_sdvo->has_hdmi_audio =
> drm_detect_monitor_audio(edid);
> + intel_sdvo->has_hdmi_monitor =
> +
> connector->display_info.is_hdmi;
> }
> } else
> status = connector_status_disconnected;
> --
> 2.33.0
>
>
--
Ville Syrjälä
Intel
Re: [PATCH v2 13/13] drm/i915: replace drm_detect_hdmi_monitor() with drm_display_info.is_hdmi
On Sat, Oct 16, 2021 at 08:42:26PM +0200, Claudio Suarez wrote:
> Once EDID is parsed, the monitor HDMI support information is available
> through drm_display_info.is_hdmi. Retriving the same information with
> drm_detect_hdmi_monitor() is less efficient. Change to
> drm_display_info.is_hdmi where possible.
We still need proof in the commit message that display_info
is actually populated by the time this gets called.
>
> This is a TODO task in Documentation/gpu/todo.rst
>
> Signed-off-by: Claudio Suarez
> ---
> drivers/gpu/drm/i915/display/intel_hdmi.c | 2 +-
> drivers/gpu/drm/i915/display/intel_sdvo.c | 3 ++-
> 2 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c
> b/drivers/gpu/drm/i915/display/intel_hdmi.c
> index b04685bb6439..008e5b0ba408 100644
> --- a/drivers/gpu/drm/i915/display/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
> @@ -2355,7 +2355,7 @@ intel_hdmi_set_edid(struct drm_connector *connector)
> to_intel_connector(connector)->detect_edid = edid;
> if (edid && edid->input & DRM_EDID_INPUT_DIGITAL) {
> intel_hdmi->has_audio = drm_detect_monitor_audio(edid);
> - intel_hdmi->has_hdmi_sink = drm_detect_hdmi_monitor(edid);
> + intel_hdmi->has_hdmi_sink = connector->display_info.is_hdmi;
>
> connected = true;
> }
> diff --git a/drivers/gpu/drm/i915/display/intel_sdvo.c
> b/drivers/gpu/drm/i915/display/intel_sdvo.c
> index 6cb27599ea03..b4065e4df644 100644
> --- a/drivers/gpu/drm/i915/display/intel_sdvo.c
> +++ b/drivers/gpu/drm/i915/display/intel_sdvo.c
> @@ -2060,8 +2060,9 @@ intel_sdvo_tmds_sink_detect(struct drm_connector
> *connector)
> if (edid->input & DRM_EDID_INPUT_DIGITAL) {
> status = connector_status_connected;
> if (intel_sdvo_connector->is_hdmi) {
> - intel_sdvo->has_hdmi_monitor =
> drm_detect_hdmi_monitor(edid);
> intel_sdvo->has_hdmi_audio =
> drm_detect_monitor_audio(edid);
> + intel_sdvo->has_hdmi_monitor =
> +
> connector->display_info.is_hdmi;
> }
> } else
> status = connector_status_disconnected;
> --
> 2.33.0
>
--
Ville Syrjälä
Intel
Re: [PATCH v2 01/13] gpu/drm: make drm_add_edid_modes() consistent when updating connector->display_info
On Sat, Oct 16, 2021 at 08:42:14PM +0200, Claudio Suarez wrote:
> According to the documentation, drm_add_edid_modes
> "... Also fills out the &drm_display_info structure and ELD in @connector
> with any information which can be derived from the edid."
>
> drm_add_edid_modes accepts a struct edid *edid parameter which may have a
> value or may be null. When it is not null, connector->display_info and
> connector->eld are updated according to the edid. When edid=NULL, only
> connector->eld is reset. Reset connector->display_info to be consistent
> and accurate.
>
> Signed-off-by: Claudio Suarez
> ---
> drivers/gpu/drm/drm_edid.c | 11 +--
> 1 file changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 6325877c5fd6..c643db17782c 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -5356,14 +5356,13 @@ int drm_add_edid_modes(struct drm_connector
> *connector, struct edid *edid)
> int num_modes = 0;
> u32 quirks;
>
> - if (edid == NULL) {
> - clear_eld(connector);
> - return 0;
> - }
> if (!drm_edid_is_valid(edid)) {
OK, so drm_edid_is_valid() will happily accept NULL and considers
it invalid. You may want to mention that explicitly in the commit
message.
> + /* edid == NULL or invalid here */
> clear_eld(connector);
> - drm_warn(connector->dev, "%s: EDID invalid.\n",
> - connector->name);
> + drm_reset_display_info(connector);
> + if (edid)
> + drm_warn(connector->dev, "%s: EDID invalid.\n",
> + connector->name);
Could you respin this to use the standard [CONNECTOR:%d:%s] form
while at it? Or I guess a patch to mass convert the whole drm_edid.c
might be another option.
Patch looks good.
Reviewed-by: Ville Syrjälä
> return 0;
> }
>
> --
> 2.33.0
>
>
--
Ville Syrjälä
Intel
Re: [Intel-gfx] [PATCH 01/15] gpu/drm: make drm_add_edid_modes() consistent when updating connector->display_info
On Fri, Oct 15, 2021 at 10:33:29PM +0300, Ville Syrjälä wrote:
> On Fri, Oct 15, 2021 at 09:24:06PM +0200, Claudio Suarez wrote:
> > On Fri, Oct 15, 2021 at 03:03:13PM +0300, Ville Syrjälä wrote:
> > > On Fri, Oct 15, 2021 at 01:36:59PM +0200, Claudio Suarez wrote:
> > > > According to the documentation, drm_add_edid_modes
> > > > "... Also fills out the &drm_display_info structure and ELD in
> > > > @connector
> > > > with any information which can be derived from the edid."
> > > >
> > > > drm_add_edid_modes accepts a struct edid *edid parameter which may have
> > > > a
> > > > value or may be null. When it is not null, connector->display_info and
> > > > connector->eld are updated according to the edid. When edid=NULL, only
> > > > connector->eld is reset. Reset connector->display_info to be consistent
> > > > and accurate.
> > > >
> > > > Signed-off-by: Claudio Suarez
> > > > ---
> > > > drivers/gpu/drm/drm_edid.c | 2 ++
> > > > 1 file changed, 2 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > > > index 6325877c5fd6..6cbe09b2357c 100644
> > > > --- a/drivers/gpu/drm/drm_edid.c
> > > > +++ b/drivers/gpu/drm/drm_edid.c
> > > > @@ -5358,10 +5358,12 @@ int drm_add_edid_modes(struct drm_connector
> > > > *connector, struct edid *edid)
> > > >
> > > > if (edid == NULL) {
> > > > clear_eld(connector);
> > > > + drm_reset_display_info(connector);
> > > > return 0;
> > > > }
> > > > if (!drm_edid_is_valid(edid)) {
> > > > clear_eld(connector);
> > > > + drm_reset_display_info(connector);
> > >
> > > Looks easier if you pull both of those out from these branches and
> > > just call them unconditionally at the start.
> >
> > After looking at the full code, I am not sure. This is the code:
> > ==
> > int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid)
> > {
> > int num_modes = 0;
> > u32 quirks;
> >
> > if (edid == NULL) {
> > clear_eld(connector);
> > drm_reset_display_info(connector); <--- added by me
> > return 0;
> > }
> > if (!drm_edid_is_valid(edid)) {
> > clear_eld(connector);
> > drm_reset_display_info(connector); <--- added by me
> > drm_warn(connector->dev, "%s: EDID invalid.\n",
> > connector->name);
> > return 0;
> > }
> >
> > drm_edid_to_eld(connector, edid);
> >
> > quirks = drm_add_display_info(connector, edid);
> > etc...
> > =
> >
> > If we move those out of these branches and edid != NULL, we are executing an
> > unnecessary clear_eld(connector) and an unnecessary
> > drm_reset_display_info(connector)
> > because the fields will be set in the next drm_edid_to_eld(connector, edid)
> > and
> > drm_add_display_info(connector, edid)
> >
> > Do we want this ?
>
> Seems fine by me. And maybe we could nuke the second
> drm_reset_display_info() from deeper inside drm_add_display_info()?
> Not sure if drm_add_display_info() still has to be able to operate
> standalone or not.
>
> Hmm. Another option is to just move all these NULL/invalid edid
> checks into drm_edid_to_eld() and drm_add_display_info().
But maybe that's not so easy. Would still need to bail out
from drm_add_edid_modes() I guess.
--
Ville Syrjälä
Intel
Re: [PATCH 01/15] gpu/drm: make drm_add_edid_modes() consistent when updating connector->display_info
On Fri, Oct 15, 2021 at 09:24:06PM +0200, Claudio Suarez wrote:
> On Fri, Oct 15, 2021 at 03:03:13PM +0300, Ville Syrjälä wrote:
> > On Fri, Oct 15, 2021 at 01:36:59PM +0200, Claudio Suarez wrote:
> > > According to the documentation, drm_add_edid_modes
> > > "... Also fills out the &drm_display_info structure and ELD in @connector
> > > with any information which can be derived from the edid."
> > >
> > > drm_add_edid_modes accepts a struct edid *edid parameter which may have a
> > > value or may be null. When it is not null, connector->display_info and
> > > connector->eld are updated according to the edid. When edid=NULL, only
> > > connector->eld is reset. Reset connector->display_info to be consistent
> > > and accurate.
> > >
> > > Signed-off-by: Claudio Suarez
> > > ---
> > > drivers/gpu/drm/drm_edid.c | 2 ++
> > > 1 file changed, 2 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > > index 6325877c5fd6..6cbe09b2357c 100644
> > > --- a/drivers/gpu/drm/drm_edid.c
> > > +++ b/drivers/gpu/drm/drm_edid.c
> > > @@ -5358,10 +5358,12 @@ int drm_add_edid_modes(struct drm_connector
> > > *connector, struct edid *edid)
> > >
> > > if (edid == NULL) {
> > > clear_eld(connector);
> > > + drm_reset_display_info(connector);
> > > return 0;
> > > }
> > > if (!drm_edid_is_valid(edid)) {
> > > clear_eld(connector);
> > > + drm_reset_display_info(connector);
> >
> > Looks easier if you pull both of those out from these branches and
> > just call them unconditionally at the start.
>
> After looking at the full code, I am not sure. This is the code:
> ==
> int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid)
> {
> int num_modes = 0;
> u32 quirks;
>
> if (edid == NULL) {
> clear_eld(connector);
> drm_reset_display_info(connector); <--- added by me
> return 0;
> }
> if (!drm_edid_is_valid(edid)) {
> clear_eld(connector);
> drm_reset_display_info(connector); <--- added by me
> drm_warn(connector->dev, "%s: EDID invalid.\n",
> connector->name);
> return 0;
> }
>
> drm_edid_to_eld(connector, edid);
>
> quirks = drm_add_display_info(connector, edid);
> etc...
> =
>
> If we move those out of these branches and edid != NULL, we are executing an
> unnecessary clear_eld(connector) and an unnecessary
> drm_reset_display_info(connector)
> because the fields will be set in the next drm_edid_to_eld(connector, edid)
> and
> drm_add_display_info(connector, edid)
>
> Do we want this ?
Seems fine by me. And maybe we could nuke the second
drm_reset_display_info() from deeper inside drm_add_display_info()?
Not sure if drm_add_display_info() still has to be able to operate
standalone or not.
Hmm. Another option is to just move all these NULL/invalid edid
checks into drm_edid_to_eld() and drm_add_display_info().
--
Ville Syrjälä
Intel
Re: [Intel-gfx] [PATCH 15/15] drm/i915: replace drm_detect_hdmi_monitor() with drm_display_info.is_hdmi
On Fri, Oct 15, 2021 at 03:44:48PM +0300, Jani Nikula wrote:
> On Fri, 15 Oct 2021, Claudio Suarez wrote:
> > Once EDID is parsed, the monitor HDMI support information is available
> > through drm_display_info.is_hdmi. Retriving the same information with
> > drm_detect_hdmi_monitor() is less efficient. Change to
> > drm_display_info.is_hdmi where possible.
> >
> > This is a TODO task in Documentation/gpu/todo.rst
> >
> > Signed-off-by: Claudio Suarez
> > ---
> > drivers/gpu/drm/i915/display/intel_connector.c | 5 +
> > drivers/gpu/drm/i915/display/intel_connector.h | 1 +
> > drivers/gpu/drm/i915/display/intel_hdmi.c | 2 +-
> > drivers/gpu/drm/i915/display/intel_sdvo.c | 3 ++-
> > 4 files changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_connector.c
> > b/drivers/gpu/drm/i915/display/intel_connector.c
> > index 9bed1ccecea0..3346b55df6e1 100644
> > --- a/drivers/gpu/drm/i915/display/intel_connector.c
> > +++ b/drivers/gpu/drm/i915/display/intel_connector.c
> > @@ -213,6 +213,11 @@ int intel_ddc_get_modes(struct drm_connector
> > *connector,
> > return ret;
> > }
> >
> > +bool intel_connector_is_hdmi_monitor(struct drm_connector *connector)
> > +{
> > + return connector->display_info.is_hdmi;
> > +}
> > +
>
> A helper like this belongs in drm, not i915. Seems useful in other
> drivers too.
Not sure it's actually helpful for i915. We end up having to root around
in the display_info in a lot of places anyway. So a helper for single
boolean seems a bit out of place perhaps.
--
Ville Syrjälä
Intel
Re: [Intel-gfx] [PATCH 15/15] drm/i915: replace drm_detect_hdmi_monitor() with drm_display_info.is_hdmi
On Fri, Oct 15, 2021 at 01:37:13PM +0200, Claudio Suarez wrote:
> Once EDID is parsed, the monitor HDMI support information is available
> through drm_display_info.is_hdmi. Retriving the same information with
> drm_detect_hdmi_monitor() is less efficient. Change to
> drm_display_info.is_hdmi where possible.
>
> This is a TODO task in Documentation/gpu/todo.rst
>
> Signed-off-by: Claudio Suarez
> ---
> drivers/gpu/drm/i915/display/intel_connector.c | 5 +
> drivers/gpu/drm/i915/display/intel_connector.h | 1 +
> drivers/gpu/drm/i915/display/intel_hdmi.c | 2 +-
> drivers/gpu/drm/i915/display/intel_sdvo.c | 3 ++-
> 4 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_connector.c
> b/drivers/gpu/drm/i915/display/intel_connector.c
> index 9bed1ccecea0..3346b55df6e1 100644
> --- a/drivers/gpu/drm/i915/display/intel_connector.c
> +++ b/drivers/gpu/drm/i915/display/intel_connector.c
> @@ -213,6 +213,11 @@ int intel_ddc_get_modes(struct drm_connector *connector,
> return ret;
> }
>
> +bool intel_connector_is_hdmi_monitor(struct drm_connector *connector)
> +{
> + return connector->display_info.is_hdmi;
> +}
> +
> static const struct drm_prop_enum_list force_audio_names[] = {
> { HDMI_AUDIO_OFF_DVI, "force-dvi" },
> { HDMI_AUDIO_OFF, "off" },
> diff --git a/drivers/gpu/drm/i915/display/intel_connector.h
> b/drivers/gpu/drm/i915/display/intel_connector.h
> index 661a37a3c6d8..ceda6e72ece6 100644
> --- a/drivers/gpu/drm/i915/display/intel_connector.h
> +++ b/drivers/gpu/drm/i915/display/intel_connector.h
> @@ -27,6 +27,7 @@ enum pipe intel_connector_get_pipe(struct intel_connector
> *connector);
> int intel_connector_update_modes(struct drm_connector *connector,
>struct edid *edid);
> int intel_ddc_get_modes(struct drm_connector *c, struct i2c_adapter
> *adapter);
> +bool intel_connector_is_hdmi_monitor(struct drm_connector *connector);
> void intel_attach_force_audio_property(struct drm_connector *connector);
> void intel_attach_broadcast_rgb_property(struct drm_connector *connector);
> void intel_attach_aspect_ratio_property(struct drm_connector *connector);
> diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c
> b/drivers/gpu/drm/i915/display/intel_hdmi.c
> index b04685bb6439..2b1d7c5bebdd 100644
> --- a/drivers/gpu/drm/i915/display/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
> @@ -2355,7 +2355,7 @@ intel_hdmi_set_edid(struct drm_connector *connector)
> to_intel_connector(connector)->detect_edid = edid;
> if (edid && edid->input & DRM_EDID_INPUT_DIGITAL) {
> intel_hdmi->has_audio = drm_detect_monitor_audio(edid);
> - intel_hdmi->has_hdmi_sink = drm_detect_hdmi_monitor(edid);
> + intel_hdmi->has_hdmi_sink =
> intel_connector_is_hdmi_monitor(connector);
Hmm. Have we parse the EDID by this point actually? I don't think that
was the case in the past but maybe it changed at some point.
>
> connected = true;
> }
> diff --git a/drivers/gpu/drm/i915/display/intel_sdvo.c
> b/drivers/gpu/drm/i915/display/intel_sdvo.c
> index 6cb27599ea03..a32279e4fee8 100644
> --- a/drivers/gpu/drm/i915/display/intel_sdvo.c
> +++ b/drivers/gpu/drm/i915/display/intel_sdvo.c
> @@ -2060,8 +2060,9 @@ intel_sdvo_tmds_sink_detect(struct drm_connector
> *connector)
> if (edid->input & DRM_EDID_INPUT_DIGITAL) {
> status = connector_status_connected;
> if (intel_sdvo_connector->is_hdmi) {
> - intel_sdvo->has_hdmi_monitor =
> drm_detect_hdmi_monitor(edid);
> intel_sdvo->has_hdmi_audio =
> drm_detect_monitor_audio(edid);
> + intel_sdvo->has_hdmi_monitor =
> +
> intel_connector_is_hdmi_monitor(connector);
FYI there's a third copy of this in intel_dp.c
> }
> } else
> status = connector_status_disconnected;
> --
> 2.33.0
>
--
Ville Syrjälä
Intel
Re: [PATCH 01/15] gpu/drm: make drm_add_edid_modes() consistent when updating connector->display_info
On Fri, Oct 15, 2021 at 01:36:59PM +0200, Claudio Suarez wrote:
> According to the documentation, drm_add_edid_modes
> "... Also fills out the &drm_display_info structure and ELD in @connector
> with any information which can be derived from the edid."
>
> drm_add_edid_modes accepts a struct edid *edid parameter which may have a
> value or may be null. When it is not null, connector->display_info and
> connector->eld are updated according to the edid. When edid=NULL, only
> connector->eld is reset. Reset connector->display_info to be consistent
> and accurate.
>
> Signed-off-by: Claudio Suarez
> ---
> drivers/gpu/drm/drm_edid.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 6325877c5fd6..6cbe09b2357c 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -5358,10 +5358,12 @@ int drm_add_edid_modes(struct drm_connector
> *connector, struct edid *edid)
>
> if (edid == NULL) {
> clear_eld(connector);
> + drm_reset_display_info(connector);
> return 0;
> }
> if (!drm_edid_is_valid(edid)) {
> clear_eld(connector);
> + drm_reset_display_info(connector);
Looks easier if you pull both of those out from these branches and
just call them unconditionally at the start.
> drm_warn(connector->dev, "%s: EDID invalid.\n",
>connector->name);
> return 0;
> --
> 2.33.0
>
>
--
Ville Syrjälä
Intel
Re: [Intel-gfx] [PATCH v2 00/17] drm: cleanup: Use DRM_MODESET_LOCK_ALL_* helpers where possible
On Sun, Oct 03, 2021 at 12:32:14AM +0200, Fernando Ramos wrote: > On 21/10/02 09:13AM, Fernando Ramos wrote: > > > > Sean, could you revert the whole patch series? I'll have a deeper look into > > the > > patch set and come up with a v3 where all these issues will be addressed. > > > > Hi Sean, > > I now understand the nature of the issue that caused the problem with i915 and > have proceed to remove the global context structure (which revealed a similar > issue in the amdgpu driver). > > I have prepared a V3 version of the patch set where these issues should > hopefully be fixed for both the i915 and amdgpu drivers. > > In order to prevent causing more disruption, could you tell me what the proper > way to proceed would be? In particular: > > 1. Is there any place where I can push my changes so that they are tested > on a i915 machine? (Some type of automated pool) cc:intel-gfx, which it looks like you did, _but_ your patches did did not even apply against drm-tip so our CI rejected it. There was a reply to the patches from CI indicating that. And that is one reason I probably just ignored the whole thing. If it doesn't even apply/build it's not worth my time to read. > > 2. I can test the amdgpu driver on my machine but, what about all the other > architectures? What is the standard procedure? Should I simply publish V3 > and wait for feedback from the different vendors? (I would hate to cause > a > simular situation again) > > 3. Should I post V3 on top of drm-next or drm-misc-next? The normal rule is: always work on drm-tip. That is what gets tested by our CI as well. Yes, it does mean a bit of extra hurdles during development since drm-tip is a rebasing tree, but there are tools like dim retip to help out here. As for where to merge them. I would generally recommed against merging i915 patches through drm-misc unless there is a very compelling reason to do so. i915 is a fast moving target and if there are significant changes coming in via drm-misc they usually will cause conflicts for people during drm-tip rebuild. Also I would expect to see an ack requested from i915 maintainers for merging anything significant via drm-misc, which I don't think happened in this case. -- Ville Syrjälä Intel
Re: [Intel-gfx] [PATCH v2 00/17] drm: cleanup: Use DRM_MODESET_LOCK_ALL_* helpers where possible
On Sat, Oct 02, 2021 at 07:28:02PM +0200, Fernando Ramos wrote:
> On 21/10/02 09:13AM, Fernando Ramos wrote:
> > On 21/10/02 05:30AM, Ville Syrjälä wrote:
> > > On Sat, Oct 02, 2021 at 01:05:47AM +0300, Ville Syrjälä wrote:
> > > > On Fri, Oct 01, 2021 at 04:48:15PM -0400, Sean Paul wrote:
> > > > > On Fri, Oct 01, 2021 at 10:00:50PM +0300, Ville Syrjälä wrote:
> > > > > > On Fri, Oct 01, 2021 at 02:36:55PM -0400, Sean Paul wrote:
> > > > > > >
> > > > > > > Thank you for revising, Fernando! I've pushed the set to
> > > > > > > drm-misc-next (along
> > > > > > > with the necessary drm-tip conflict resolutions).
> > > > > >
> > > > > > Ugh. Did anyone actually review the locking changes this does?
> > > > > > I shot the previous i915 stuff down because the commit messages
> > > > > > did not address any of it.
> > > > >
> > > > > I reviewed the set on 9/17, I didn't see your feedback on that thread.
> > > >
> > > > It was much earlir than that.
> > > > https://lists.freedesktop.org/archives/dri-devel/2021-June/313193.html
>
> Sorry, I'm new to this and it did not occur to me to search for similar
> patches
> in the mailing list archives in case there were additional comments that
> applied
> to my change set.
>
> In case I had done that I would have found that, as you mentioned, you had
> already raised two issues back in June:
>
> On Tue, Jun 29, 2021, Ville Syrjälä wrote:
> >
> > That looks wrong. You're using a private ctx here, but still
> > passing dev->mode_config.acquire_ctx to the lower level stuff.
> >
> > Also DRM_MODESET_LOCK_ALL_{BEGIN,END}() do not seem to be
> > equivalent to drm_modeset_{lock,unlock}_all() when it comes to
> > mode_config.mutex. So would need a proper review whether we
> > actually need that lock or not.
>
> The first one was pointing out the same error I would later repeat in my patch
> series (ups).
>
> After further inspection of the code it looks to me that changing this:
>
> intel_modeset_setup_hw_state(dev, dev->mode_config.acquire_ctx);
>
> ...into this:
>
> intel_modeset_setup_hw_state(dev, &ctx);
>
> ...would be enough.
Yes.
>
> Why? The only difference between the old drm_modeset_{lock,unlock}_all()
> functions and the new DRM_MODESET_LOCK_ALL_{BEGIN,END}() macros is that the
> former use a global context stored in dev->mode_config.acquire_ctx while the
> latter depend on a user provided one (typically in the stack).
>
> In the old (working) code the global context structure is freed in
> drm_modeset_unlock_all() thus we are sure no one is holding a reference to it
> at
> that point. This means that as long as no one accesses the global
> dev->mode_config.acquire_ctx context in the block that runs between lock/BEGIN
> and unlock/END, the code should be equivalent before and after my changes.
>
> In fact, now that my patch series removes the drm_modeset_{lock,unlock}_all()
> functions, the acquire_ctx field of the drm_mode_config structure should be
> deleted:
>
> /**
> * @acquire_ctx:
> *
> * Global implicit acquire context used by atomic drivers for legacy
> * IOCTLs. Deprecated, since implicit locking contexts make it
> * impossible to use driver-private &struct drm_modeset_lock. Users of
> * this must hold @mutex.
> */
> struct drm_modeset_acquire_ctx *acquire_ctx;
>
> If I had done that (ie. removing this field) I would have detected the problem
> when compiling.
>
> There is another place (in the amdgpu driver) where this field is still being
> referenced, but before I investigate that I would like to know if you agree
> that
> this is a good path to follow.
Yeah, removing the mode_config.acquire_ctx is a good idea if it's
no longer needed.
>
> Regarding the second issue you raised...
>
> > Also DRM_MODESET_LOCK_ALL_{BEGIN,END}() do not seem to be
> > equivalent to drm_modeset_{lock,unlock}_all() when it comes to
> > mode_config.mutex. So would need a proper review whether we
> > actually need that lock or not.
>
> ...the only difference regarding mode_config.mutex I see is that in the new
> macros the mutex is locked only under this condition:
>
> if (!drm_drv_uses_atomic_modeset(dev))
>
> ...which seems reasonable, right? Is this what you were referring to or is it
> something else?
In order to eliminate the lock one first has to determine what that lock
might be protecting here, and then prove that such protection is not
actually needed.
--
Ville Syrjälä
Intel
Re: [Intel-gfx] [PATCH v2 00/17] drm: cleanup: Use DRM_MODESET_LOCK_ALL_* helpers where possible
On Sat, Oct 02, 2021 at 01:05:47AM +0300, Ville Syrjälä wrote:
> On Fri, Oct 01, 2021 at 04:48:15PM -0400, Sean Paul wrote:
> > On Fri, Oct 01, 2021 at 10:00:50PM +0300, Ville Syrjälä wrote:
> > > On Fri, Oct 01, 2021 at 02:36:55PM -0400, Sean Paul wrote:
> > > > On Fri, Sep 24, 2021 at 08:43:07AM +0200, Fernando Ramos wrote:
> > > > > Hi all,
> > > > >
> > > > > One of the things in the DRM TODO list ("Documentation/gpu/todo.rst")
> > > > > was to
> > > > > "use DRM_MODESET_LOCAL_ALL_* helpers instead of boilerplate". That's
> > > > > what this
> > > > > patch series is about.
> > > > >
> > > > > You will find two types of changes here:
> > > > >
> > > > > - Replacing "drm_modeset_lock_all_ctx()" (and surrounding
> > > > > boilerplate) with
> > > > > "DRM_MODESET_LOCK_ALL_BEGIN()/END()" in the remaining places (as
> > > > > it has
> > > > > already been done in previous commits such as b7ea04d2)
> > > > >
> > > > > - Replacing "drm_modeset_lock_all()" with
> > > > > "DRM_MODESET_LOCK_ALL_BEGIN()/END()"
> > > > > in the remaining places (as it has already been done in previous
> > > > > commits
> > > > > such as 57037094)
> > > > >
> > > > > Most of the changes are straight forward, except for a few cases in
> > > > > the "amd"
> > > > > and "i915" drivers where some extra dancing was needed to overcome the
> > > > > limitation that the DRM_MODESET_LOCK_ALL_BEGIN()/END() macros can
> > > > > only be used
> > > > > once inside the same function (the reason being that the macro
> > > > > expansion
> > > > > includes *labels*, and you can not have two labels named the same
> > > > > inside one
> > > > > function)
> > > > >
> > > > > Notice that, even after this patch series, some places remain where
> > > > > "drm_modeset_lock_all()" and "drm_modeset_lock_all_ctx()" are still
> > > > > present,
> > > > > all inside drm core (which makes sense), except for two (in "amd" and
> > > > > "i915")
> > > > > which cannot be replaced due to the way they are being used.
> > > > >
> > > > > Changes in v2:
> > > > >
> > > > > - Fix commit message typo
> > > > > - Use the value returned by DRM_MODESET_LOCK_ALL_END when possible
> > > > > - Split drm/i915 patch into two simpler ones
> > > > > - Remove drm_modeset_(un)lock_all()
> > > > > - Fix build problems in non-x86 platforms
> > > > >
> > > > > Fernando Ramos (17):
> > > > > drm: cleanup: drm_modeset_lock_all_ctx() -->
> > > > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > > > drm/i915: cleanup: drm_modeset_lock_all_ctx() -->
> > > > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > > > drm/msm: cleanup: drm_modeset_lock_all_ctx() -->
> > > > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > > > drm: cleanup: drm_modeset_lock_all() -->
> > > > > DRM_MODESET_LOCK_ALL_BEGIN() drm/vmwgfx: cleanup:
> > > > > drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN()
> > > > > drm/tegra: cleanup: drm_modeset_lock_all() -->
> > > > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > > > drm/shmobile: cleanup: drm_modeset_lock_all() -->
> > > > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > > > drm/radeon: cleanup: drm_modeset_lock_all() -->
> > > > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > > > drm/omapdrm: cleanup: drm_modeset_lock_all() -->
> > > > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > > > drm/nouveau: cleanup: drm_modeset_lock_all() -->
> > > > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > > > drm/msm: cleanup: drm_modeset_lock_all() -->
> > > > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > > > drm/i915: cleanup: drm_modeset_lock_all() -->
> > > > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > > > drm/i915: cleanup: drm_modeset_lock_all() -->
> > > > > DRM_MODESET_LOCK_ALL_BEGIN() part 2
&
Re: [PATCH v2 00/17] drm: cleanup: Use DRM_MODESET_LOCK_ALL_* helpers where possible
On Fri, Oct 01, 2021 at 04:48:15PM -0400, Sean Paul wrote:
> On Fri, Oct 01, 2021 at 10:00:50PM +0300, Ville Syrjälä wrote:
> > On Fri, Oct 01, 2021 at 02:36:55PM -0400, Sean Paul wrote:
> > > On Fri, Sep 24, 2021 at 08:43:07AM +0200, Fernando Ramos wrote:
> > > > Hi all,
> > > >
> > > > One of the things in the DRM TODO list ("Documentation/gpu/todo.rst")
> > > > was to
> > > > "use DRM_MODESET_LOCAL_ALL_* helpers instead of boilerplate". That's
> > > > what this
> > > > patch series is about.
> > > >
> > > > You will find two types of changes here:
> > > >
> > > > - Replacing "drm_modeset_lock_all_ctx()" (and surrounding
> > > > boilerplate) with
> > > > "DRM_MODESET_LOCK_ALL_BEGIN()/END()" in the remaining places (as it
> > > > has
> > > > already been done in previous commits such as b7ea04d2)
> > > >
> > > > - Replacing "drm_modeset_lock_all()" with
> > > > "DRM_MODESET_LOCK_ALL_BEGIN()/END()"
> > > > in the remaining places (as it has already been done in previous
> > > > commits
> > > > such as 57037094)
> > > >
> > > > Most of the changes are straight forward, except for a few cases in the
> > > > "amd"
> > > > and "i915" drivers where some extra dancing was needed to overcome the
> > > > limitation that the DRM_MODESET_LOCK_ALL_BEGIN()/END() macros can only
> > > > be used
> > > > once inside the same function (the reason being that the macro expansion
> > > > includes *labels*, and you can not have two labels named the same
> > > > inside one
> > > > function)
> > > >
> > > > Notice that, even after this patch series, some places remain where
> > > > "drm_modeset_lock_all()" and "drm_modeset_lock_all_ctx()" are still
> > > > present,
> > > > all inside drm core (which makes sense), except for two (in "amd" and
> > > > "i915")
> > > > which cannot be replaced due to the way they are being used.
> > > >
> > > > Changes in v2:
> > > >
> > > > - Fix commit message typo
> > > > - Use the value returned by DRM_MODESET_LOCK_ALL_END when possible
> > > > - Split drm/i915 patch into two simpler ones
> > > > - Remove drm_modeset_(un)lock_all()
> > > > - Fix build problems in non-x86 platforms
> > > >
> > > > Fernando Ramos (17):
> > > > drm: cleanup: drm_modeset_lock_all_ctx() -->
> > > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > > drm/i915: cleanup: drm_modeset_lock_all_ctx() -->
> > > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > > drm/msm: cleanup: drm_modeset_lock_all_ctx() -->
> > > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > > drm: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN()
> > > > drm/vmwgfx: cleanup: drm_modeset_lock_all() -->
> > > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > > drm/tegra: cleanup: drm_modeset_lock_all() -->
> > > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > > drm/shmobile: cleanup: drm_modeset_lock_all() -->
> > > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > > drm/radeon: cleanup: drm_modeset_lock_all() -->
> > > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > > drm/omapdrm: cleanup: drm_modeset_lock_all() -->
> > > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > > drm/nouveau: cleanup: drm_modeset_lock_all() -->
> > > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > > drm/msm: cleanup: drm_modeset_lock_all() -->
> > > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > > drm/i915: cleanup: drm_modeset_lock_all() -->
> > > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > > drm/i915: cleanup: drm_modeset_lock_all() -->
> > > > DRM_MODESET_LOCK_ALL_BEGIN() part 2
> > > > drm/gma500: cleanup: drm_modeset_lock_all() -->
> > > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > > drm/amd: cleanup: drm_modeset_lock_all() -->
> > > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > > drm: cleanup: remove drm_modeset_(un)lock_all()
> > > > doc: drm: remove TODO entry regarding DRM_MODSET_LOCK_ALL cleanup
> > > >
> > >
> > > Thank you for revising, Fernando! I've pushed the set to drm-misc-next
> > > (along
> > > with the necessary drm-tip conflict resolutions).
> >
> > Ugh. Did anyone actually review the locking changes this does?
> > I shot the previous i915 stuff down because the commit messages
> > did not address any of it.
>
> I reviewed the set on 9/17, I didn't see your feedback on that thread.
It was much earlir than that.
https://lists.freedesktop.org/archives/dri-devel/2021-June/313193.html
And I think I might have also shot down a similar thing earlier.
I was actually half considering sending a patch to nuke that
misleading TODO item. I don't think anything which changes
which locks are taken should be considred a starter level task.
And the commit messages here don't seem to address any of it.
--
Ville Syrjälä
Intel
Re: [PATCH v2 00/17] drm: cleanup: Use DRM_MODESET_LOCK_ALL_* helpers where possible
On Fri, Oct 01, 2021 at 02:36:55PM -0400, Sean Paul wrote:
> On Fri, Sep 24, 2021 at 08:43:07AM +0200, Fernando Ramos wrote:
> > Hi all,
> >
> > One of the things in the DRM TODO list ("Documentation/gpu/todo.rst") was to
> > "use DRM_MODESET_LOCAL_ALL_* helpers instead of boilerplate". That's what
> > this
> > patch series is about.
> >
> > You will find two types of changes here:
> >
> > - Replacing "drm_modeset_lock_all_ctx()" (and surrounding boilerplate)
> > with
> > "DRM_MODESET_LOCK_ALL_BEGIN()/END()" in the remaining places (as it has
> > already been done in previous commits such as b7ea04d2)
> >
> > - Replacing "drm_modeset_lock_all()" with
> > "DRM_MODESET_LOCK_ALL_BEGIN()/END()"
> > in the remaining places (as it has already been done in previous commits
> > such as 57037094)
> >
> > Most of the changes are straight forward, except for a few cases in the
> > "amd"
> > and "i915" drivers where some extra dancing was needed to overcome the
> > limitation that the DRM_MODESET_LOCK_ALL_BEGIN()/END() macros can only be
> > used
> > once inside the same function (the reason being that the macro expansion
> > includes *labels*, and you can not have two labels named the same inside one
> > function)
> >
> > Notice that, even after this patch series, some places remain where
> > "drm_modeset_lock_all()" and "drm_modeset_lock_all_ctx()" are still present,
> > all inside drm core (which makes sense), except for two (in "amd" and
> > "i915")
> > which cannot be replaced due to the way they are being used.
> >
> > Changes in v2:
> >
> > - Fix commit message typo
> > - Use the value returned by DRM_MODESET_LOCK_ALL_END when possible
> > - Split drm/i915 patch into two simpler ones
> > - Remove drm_modeset_(un)lock_all()
> > - Fix build problems in non-x86 platforms
> >
> > Fernando Ramos (17):
> > drm: cleanup: drm_modeset_lock_all_ctx() --> DRM_MODESET_LOCK_ALL_BEGIN()
> > drm/i915: cleanup: drm_modeset_lock_all_ctx() -->
> > DRM_MODESET_LOCK_ALL_BEGIN()
> > drm/msm: cleanup: drm_modeset_lock_all_ctx() -->
> > DRM_MODESET_LOCK_ALL_BEGIN()
> > drm: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN()
> > drm/vmwgfx: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN()
> > drm/tegra: cleanup: drm_modeset_lock_all() -->
> > DRM_MODESET_LOCK_ALL_BEGIN()
> > drm/shmobile: cleanup: drm_modeset_lock_all() -->
> > DRM_MODESET_LOCK_ALL_BEGIN()
> > drm/radeon: cleanup: drm_modeset_lock_all() -->
> > DRM_MODESET_LOCK_ALL_BEGIN()
> > drm/omapdrm: cleanup: drm_modeset_lock_all() -->
> > DRM_MODESET_LOCK_ALL_BEGIN()
> > drm/nouveau: cleanup: drm_modeset_lock_all() -->
> > DRM_MODESET_LOCK_ALL_BEGIN()
> > drm/msm: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN()
> > drm/i915: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN()
> > drm/i915: cleanup: drm_modeset_lock_all() -->
> > DRM_MODESET_LOCK_ALL_BEGIN() part 2
> > drm/gma500: cleanup: drm_modeset_lock_all() -->
> > DRM_MODESET_LOCK_ALL_BEGIN()
> > drm/amd: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN()
> > drm: cleanup: remove drm_modeset_(un)lock_all()
> > doc: drm: remove TODO entry regarding DRM_MODSET_LOCK_ALL cleanup
> >
>
> Thank you for revising, Fernando! I've pushed the set to drm-misc-next (along
> with the necessary drm-tip conflict resolutions).
Ugh. Did anyone actually review the locking changes this does?
I shot the previous i915 stuff down because the commit messages
did not address any of it.
--
Ville Syrjälä
Intel
Re: [PATCH 4/4] drm/i915/display: Add handling for new "active bpc" property
On Tue, Jun 08, 2021 at 07:19:31PM +0200, Werner Sembach wrote:
>
> Am 07.06.21 um 22:33 schrieb Werner Sembach:
> > Am 07.06.21 um 08:47 schrieb Werner Sembach:
> >>
> >> Am 04.06.21 um 19:30 schrieb Ville Syrjälä:
> >>> On Fri, Jun 04, 2021 at 07:17:23PM +0200, Werner Sembach wrote:
> >>>> This commits implements the "active bpc" drm property for the Intel
> >>>> GPU driver.
> >>>>
> >>>> Signed-off-by: Werner Sembach
> >>>> ---
> >>>> drivers/gpu/drm/i915/display/intel_display.c | 13 +
> >>>> drivers/gpu/drm/i915/display/intel_dp.c | 8 ++--
> >>>> drivers/gpu/drm/i915/display/intel_dp_mst.c | 4 +++-
> >>>> drivers/gpu/drm/i915/display/intel_hdmi.c | 4 +++-
> >>>> 4 files changed, 25 insertions(+), 4 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> >>>> b/drivers/gpu/drm/i915/display/intel_display.c
> >>>> index 64e9107d70f7..f7898d9d7438 100644
> >>>> --- a/drivers/gpu/drm/i915/display/intel_display.c
> >>>> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> >>>> @@ -10164,6 +10164,8 @@ static void intel_atomic_commit_tail(struct
> >>>> intel_atomic_state *state)
> >>>> struct drm_i915_private *dev_priv = to_i915(dev);
> >>>> struct intel_crtc_state *new_crtc_state, *old_crtc_state;
> >>>> struct intel_crtc *crtc;
> >>>> + struct drm_connector *connector;
> >>>> + struct drm_connector_state *new_conn_state;
> >>>> u64 put_domains[I915_MAX_PIPES] = {};
> >>>> intel_wakeref_t wakeref = 0;
> >>>> int i;
> >>>> @@ -10324,6 +10326,17 @@ static void
> >>>> intel_atomic_commit_tail(struct intel_atomic_state *state)
> >>>> }
> >>>> intel_runtime_pm_put(&dev_priv->runtime_pm, state->wakeref);
> >>>> + /* Extract information from crtc to communicate it to
> >>>> userspace as connector properties */
> >>>> + for_each_new_connector_in_state(&state->base, connector,
> >>>> new_conn_state, i) {
> >>>> + struct drm_crtc *crtc = new_conn_state->crtc;
> >>>> + if (crtc) {
> >>>> + new_crtc_state =
> >>>> to_intel_crtc_state(drm_atomic_get_new_crtc_state(&state->base,
> >>>> crtc));
> >>> intel_atomic_get_new_crtc_state()
> >> Thanks, will use that.
> >>>
> >>>> + new_conn_state->active_bpc = new_crtc_state->pipe_bpp / 3;
> >>>> + }
> >>>> + else
> >>>> + new_conn_state->active_bpc = 0;
> >>>> + }
> >>> This also seems too late. I think the whole thing should be
> >>> done somewhere around the normal swap_state() stuff.
> >> Ok, will look into it.
> > So I tried to put it in intel_atomic_commit() after
> > drm_atomic_helper_swap_state() and before
> > INIT_WORK(&state->base.commit_work, intel_atomic_commit_work) (which
> > creates a worker for intel_atomic_commit_tail), but somewhere in
> > between, the connector_state seems to change: The bpc written with the
> > for_each_new_connector_in_state() loop, gets discarded.
>
> This was not the problem. Setting the drm property immutable made it
> (also?) immutable from the driver context, which I didn't test separately.
>
> Removed the immutable again and moved the loop.
Immutable props are special. See __drm_object_property_get_value().
--
Ville Syrjälä
Intel
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v2 4/7] drm/i915/display: Add handling for new "active bpc" property
On Thu, Jun 10, 2021 at 02:50:36PM +0200, Maxime Ripard wrote:
> Hi
>
> On Tue, Jun 08, 2021 at 07:43:17PM +0200, Werner Sembach wrote:
> > This commits implements the "active bpc" drm property for the Intel GPU
> > driver.
> >
> > Signed-off-by: Werner Sembach
> > ---
> > drivers/gpu/drm/i915/display/intel_display.c | 14 ++
> > drivers/gpu/drm/i915/display/intel_dp.c | 8 ++--
> > drivers/gpu/drm/i915/display/intel_dp_mst.c | 4 +++-
> > drivers/gpu/drm/i915/display/intel_hdmi.c| 4 +++-
> > 4 files changed, 26 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > b/drivers/gpu/drm/i915/display/intel_display.c
> > index 64e9107d70f7..50c11b8770a7 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -10388,6 +10388,9 @@ static int intel_atomic_commit(struct drm_device
> > *dev,
> > {
> > struct intel_atomic_state *state = to_intel_atomic_state(_state);
> > struct drm_i915_private *dev_priv = to_i915(dev);
> > + struct drm_connector *connector;
> > + struct drm_connector_state *new_conn_state;
> > + int i;
> > int ret = 0;
> >
> > state->wakeref = intel_runtime_pm_get(&dev_priv->runtime_pm);
> > @@ -10456,6 +10459,17 @@ static int intel_atomic_commit(struct drm_device
> > *dev,
> > intel_shared_dpll_swap_state(state);
> > intel_atomic_track_fbs(state);
> >
> > + /* Extract information from crtc to communicate it to userspace as
> > connector properties */
> > + for_each_new_connector_in_state(&state->base, connector,
> > new_conn_state, i) {
> > + struct intel_crtc *crtc = to_intel_crtc(new_conn_state->crtc);
> > + if (crtc) {
> > + struct intel_crtc_state *new_crtc_state =
> > intel_atomic_get_new_crtc_state(state, crtc);
> > + new_conn_state->active_bpc = new_crtc_state->pipe_bpp /
> > 3;
> > + }
> > + else
> > + new_conn_state->active_bpc = 0;
> > + }
> > +
>
> This seems fairly intrusive, but also commit / commit_tail might not be
> the best place to put this, we want to support it at the connector
> level.
>
> Indeed, this will cause some issue if your HDMI output is a bridge for
> example, where the commit will be in an entirely different driver that
> has no dependency on the HDMI controller one.
>
> I think it would be best to do that assignment in atomic_check. That
> way, if the userspace does a commit with DRM_MODE_ATOMIC_TEST_ONLY it
> would know what the output state would have been like.
DRM_MODE_ATOMIC_TEST_ONLY isn't allowed to change anything.
--
Ville Syrjälä
Intel
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 2/4] drm/uAPI: Add "active bpc" as feedback channel for "max bpc" drm property
On Mon, Jun 07, 2021 at 08:46:47AM +0200, Werner Sembach wrote:
>
> Am 04.06.21 um 19:26 schrieb Ville Syrjälä:
> > On Fri, Jun 04, 2021 at 07:17:21PM +0200, Werner Sembach wrote:
> >> Add a new general drm property "active bpc" which can be used by graphic
> >> drivers
> >> to report the applied bit depth per pixel back to userspace.
> >>
> >> While "max bpc" can be used to change the color depth, there was no way to
> >> check
> >> which one actually got used. While in theory the driver chooses the
> >> best/highest
> >> color depth within the max bpc setting a user might not be fully aware
> >> what his
> >> hardware is or isn't capable off. This is meant as a quick way to double
> >> check
> >> the setup.
> >>
> >> In the future, automatic color calibration for screens might also depend
> >> on this
> >> information available.
> >>
> >> Signed-off-by: Werner Sembach
> >> ---
> >> drivers/gpu/drm/drm_atomic_uapi.c | 2 ++
> >> drivers/gpu/drm/drm_connector.c | 40 +++
> >> include/drm/drm_connector.h | 15
> >> 3 files changed, 57 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
> >> b/drivers/gpu/drm/drm_atomic_uapi.c
> >> index 268bb69c2e2f..7ae4e40936b5 100644
> >> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> >> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> >> @@ -873,6 +873,8 @@ drm_atomic_connector_get_property(struct drm_connector
> >> *connector,
> >>*val = 0;
> >>} else if (property == connector->max_bpc_property) {
> >>*val = state->max_requested_bpc;
> >> + } else if (property == connector->active_bpc_property) {
> >> + *val = state->active_bpc;
> >>} else if (connector->funcs->atomic_get_property) {
> >>return connector->funcs->atomic_get_property(connector,
> >>state, property, val);
> >> diff --git a/drivers/gpu/drm/drm_connector.c
> >> b/drivers/gpu/drm/drm_connector.c
> >> index 7631f76e7f34..5f42a5be5822 100644
> >> --- a/drivers/gpu/drm/drm_connector.c
> >> +++ b/drivers/gpu/drm/drm_connector.c
> >> @@ -1195,6 +1195,13 @@ static const struct drm_prop_enum_list
> >> dp_colorspaces[] = {
> >>* drm_connector_attach_max_bpc_property() to create and attach the
> >>* property to the connector during initialization.
> >>*
> >> + * active bpc:
> >> + *This read-only range property is used by userspace check the
> >> bit depth
> >> + *actually applied by the GPU driver after evaluation all hardware
> >> + *capabilities and max bpc. Drivers to use the function
> >> + *drm_connector_attach_active_bpc_property() to create and attach
> >> the
> >> + *property to the connector during initialization.
> >> + *
> >>* Connectors also have one standardized atomic property:
> >>*
> >>* CRTC_ID:
> >> @@ -2150,6 +2157,39 @@ int drm_connector_attach_max_bpc_property(struct
> >> drm_connector *connector,
> >> }
> >> EXPORT_SYMBOL(drm_connector_attach_max_bpc_property);
> >>
> >> +/**
> >> + * drm_connector_attach_active_bpc_property - attach "active bpc" property
> >> + * @connector: connector to attach active bpc property on.
> >> + * @min: The minimum bit depth supported by the connector.
> >> + * @max: The maximum bit depth supported by the connector.
> >> + *
> >> + * This is used to check the applied bit depth on a connector.
> >> + *
> >> + * Returns:
> >> + * Zero on success, negative errno on failure.
> >> + */
> >> +int drm_connector_attach_active_bpc_property(struct drm_connector
> >> *connector,
> >> +int min, int max)
> >> +{
> >> + struct drm_device *dev = connector->dev;
> >> + struct drm_property *prop;
> >> +
> >> + prop = connector->active_bpc_property;
> >> + if (!prop) {
> >> + prop = drm_property_create_range(dev, 0, "active bpc", min,
> >> max);
> > Should be immutable.
> Yes. I didn't know if there is a way to do this (or just don't define a
> set-function), but I think I found the define for this.
