i915 does not set DRIVER_ATOMIC by default yet but uses atomic_check and atomic_commit. drm_object_property_get_value() does not read the correct value of atomic properties if DRIVER_ATOMIC is not set. Checking whether the driver uses atomic modeset is a better check instead as the property values are tracked in the state structures.

v2: Included header

Cc: Daniel Vetter daniel.vetter@ffwll.ch Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Dhinakaran Pandiyan dhinakaran.pandiyan@intel.com --- drivers/gpu/drm/drm_mode_object.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_mode_object.c b/drivers/gpu/drm/drm_mode_object.c index 9f17085..14543ff 100644 --- a/drivers/gpu/drm/drm_mode_object.c +++ b/drivers/gpu/drm/drm_mode_object.c @@ -23,6 +23,7 @@ #include <linux/export.h> #include <drm/drmP.h> #include <drm/drm_mode_object.h> +#include <drm/drm_atomic.h>

#include "drm_crtc_internal.h"

@@ -273,7 +274,7 @@ int drm_object_property_get_value(struct drm_mode_object *obj, * their value in obj->properties->values[].. mostly to avoid * having to deal w/ EDID and similar props in atomic paths: */ - if (drm_core_check_feature(property->dev, DRIVER_ATOMIC) && + if (drm_drv_uses_atomic_modeset(property->dev) && !(property->flags & DRM_MODE_PROP_IMMUTABLE)) return drm_atomic_get_property(obj, property, val);

On Wed, Dec 21, 2016 at 12:12:08PM -0800, Dhinakaran Pandiyan wrote:

This check is useful for drivers that do not have DRIVER_ATOMIC set but have atomic modesetting internally implemented. Wrap the check into a function since this is used in many places and as a bonus, the function name helps to document what the check is for.

v2: Change return type to bool (Ville) Move the function drm_atomic.h (Daniel)

Suggested-by: Daniel Vetter daniel.vetter@ffwll.ch Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: Ben Skeggs bskeggs@redhat.com Signed-off-by: Dhinakaran Pandiyan dhinakaran.pandiyan@intel.com

---

drivers/gpu/drm/drm_fb_helper.c | 6 +++--- drivers/gpu/drm/nouveau/nouveau_connector.c | 5 +++-- drivers/gpu/drm/nouveau/nouveau_display.c | 6 +++--- drivers/gpu/drm/nouveau/nouveau_fbcon.c | 3 ++- include/drm/drm_atomic.h | 11 +++++++++++ 5 files changed, 22 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 145d55f..730342c 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -405,7 +405,7 @@ static int restore_fbdev_mode(struct drm_fb_helper *fb_helper)

drm_warn_on_modeset_not_all_locked(dev);

• if (dev->mode_config.funcs->atomic_commit)

• if (drm_drv_uses_atomic_modeset(dev)) return restore_fbdev_mode_atomic(fb_helper);

  drm_for_each_plane(plane, dev) {

@@ -1444,7 +1444,7 @@ int drm_fb_helper_pan_display(struct fb_var_screeninfo *var, return -EBUSY; }

• if (dev->mode_config.funcs->atomic_commit) {

• if (drm_drv_uses_atomic_modeset(dev)) { ret = pan_display_atomic(var, info); goto unlock; }

@@ -2060,7 +2060,7 @@ static int drm_pick_crtcs(struct drm_fb_helper *fb_helper, * NULL we fallback to the default drm_atomic_helper_best_encoder() * helper. */

• if (fb_helper->dev->mode_config.funcs->atomic_commit &&

• if (drm_drv_uses_atomic_modeset(fb_helper->dev) && !connector_funcs->best_encoder) encoder = drm_atomic_helper_best_encoder(connector); else

diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.c b/drivers/gpu/drm/nouveau/nouveau_connector.c index 947c200..966d20a 100644 --- a/drivers/gpu/drm/nouveau/nouveau_connector.c +++ b/drivers/gpu/drm/nouveau/nouveau_connector.c @@ -33,6 +33,7 @@ #include <drm/drm_atomic_helper.h> #include <drm/drm_edid.h> #include <drm/drm_crtc_helper.h> +#include <drm/drm_atomic.h>

#include "nouveau_reg.h" #include "nouveau_drv.h" @@ -769,7 +770,7 @@ nouveau_connector_set_property(struct drm_connector *connector, struct drm_encoder *encoder = to_drm_encoder(nv_encoder); int ret;

• if (connector->dev->mode_config.funcs->atomic_commit)

• if (drm_drv_uses_atomic_modeset(connector->dev)) return drm_atomic_helper_connector_set_property(connector, property, value);

  ret = connector->funcs->atomic_set_property(&nv_connector->base,

@@ -1074,7 +1075,7 @@ nouveau_connector_helper_funcs = { static int nouveau_connector_dpms(struct drm_connector *connector, int mode) {

• if (connector->dev->mode_config.funcs->atomic_commit)

• if (drm_drv_uses_atomic_modeset(connector->dev)) return drm_atomic_helper_connector_dpms(connector, mode); return drm_helper_connector_dpms(connector, mode);

} diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c b/drivers/gpu/drm/nouveau/nouveau_display.c index c5cf888..add353e 100644 --- a/drivers/gpu/drm/nouveau/nouveau_display.c +++ b/drivers/gpu/drm/nouveau/nouveau_display.c @@ -162,7 +162,7 @@ nouveau_display_vblstamp(struct drm_device *dev, unsigned int pipe, list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) { if (nouveau_crtc(crtc)->index == pipe) { struct drm_display_mode *mode;

• 	if (dev->mode_config.funcs->atomic_commit)
  

• 	if (drm_drv_uses_atomic_modeset(dev))
  	mode = &crtc->state->adjusted_mode;
  else
  	mode = &crtc->hwmode;
  

@@ -738,7 +738,7 @@ nouveau_display_suspend(struct drm_device *dev, bool runtime) struct nouveau_display *disp = nouveau_display(dev); struct drm_crtc *crtc;

• if (dev->mode_config.funcs->atomic_commit) {

• if (drm_drv_uses_atomic_modeset(dev)) { if (!runtime) { disp->suspend = nouveau_atomic_suspend(dev); if (IS_ERR(disp->suspend)) {

@@ -784,7 +784,7 @@ nouveau_display_resume(struct drm_device *dev, bool runtime) struct drm_crtc *crtc; int ret;

• if (dev->mode_config.funcs->atomic_commit) {

• if (drm_drv_uses_atomic_modeset(dev)) { nouveau_display_init(dev); if (disp->suspend) { drm_atomic_helper_resume(dev, disp->suspend);

diff --git a/drivers/gpu/drm/nouveau/nouveau_fbcon.c b/drivers/gpu/drm/nouveau/nouveau_fbcon.c index 5600f6c..9de6abb 100644 --- a/drivers/gpu/drm/nouveau/nouveau_fbcon.c +++ b/drivers/gpu/drm/nouveau/nouveau_fbcon.c @@ -41,6 +41,7 @@ #include <drm/drm_crtc.h> #include <drm/drm_crtc_helper.h> #include <drm/drm_fb_helper.h> +#include <drm/drm_atomic.h>

#include "nouveau_drv.h" #include "nouveau_gem.h" @@ -524,7 +525,7 @@ nouveau_fbcon_init(struct drm_device *dev) preferred_bpp = 32;

/* disable all the possible outputs/crtcs before entering KMS mode */

• if (!dev->mode_config.funcs->atomic_commit)

• if (!drm_drv_uses_atomic_modeset(dev)) drm_helper_disable_unused_functions(dev);

  ret = drm_fb_helper_initial_config(&fbcon->helper, preferred_bpp);

diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h index 8cc7ca2..43db162 100644 --- a/include/drm/drm_atomic.h +++ b/include/drm/drm_atomic.h @@ -419,5 +419,16 @@ drm_atomic_crtc_needs_modeset(const struct drm_crtc_state *state) state->connectors_changed; }

+/* drm_drv_uses_atomic_modeset - check if the driver implements

• • atomic_commit()
• • @dev: DRM device
• • This check is useful if drivers do not have DRIVER_ATOMIC set but
• • have atomic modesetting internally implemented.
• */

+static inline bool drm_drv_uses_atomic_modeset(struct drm_device *dev) +{

• return dev->mode_config.funcs->atomic_commit ? true : false;

You forgot the part from Ville's review that the entire "?:" at the end is redundant: A pointer in bool context automatically converts to a bool like this, and we use this _everywhere_. Please remove and respin.

Also when you resend the entire patch series, pls start a new thread. CI otherwise assumes that you're only partially resend patches and tries to merge old&new series, which doesn't apply ofc. Since it's already a mess you new to start a new series even if only patch 1 changed. -Daniel

On Wed, 2016-12-21 at 12:12 -0800, Dhinakaran Pandiyan wrote:

This check is useful for drivers that do not have DRIVER_ATOMIC set but have atomic modesetting internally implemented. Wrap the check into a function since this is used in many places and as a bonus, the function name helps to document what the check is for.

v2: Change return type to bool (Ville) Move the function drm_atomic.h (Daniel)

Suggested-by: Daniel Vetter daniel.vetter@ffwll.ch Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: Ben Skeggs bskeggs@redhat.com Signed-off-by: Dhinakaran Pandiyan dhinakaran.pandiyan@intel.com

---

drivers/gpu/drm/drm_fb_helper.c             |  6 +++---  drivers/gpu/drm/nouveau/nouveau_connector.c |  5 +++--  drivers/gpu/drm/nouveau/nouveau_display.c   |  6 +++---  drivers/gpu/drm/nouveau/nouveau_fbcon.c     |  3 ++-  include/drm/drm_atomic.h                    | 11 +++++++++++  5 files changed, 22 insertions(+), 9 deletions(-)

...

diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h index 8cc7ca2..43db162 100644 --- a/include/drm/drm_atomic.h +++ b/include/drm/drm_atomic.h @@ -419,5 +419,16 @@ drm_atomic_crtc_needs_modeset(const struct drm_crtc_state *state)          state->connectors_changed;  }   +/* drm_drv_uses_atomic_modeset - check if the driver implements

Shouldn't this be

/**  * drm_drv_uses_atomic_modeset - ...

so it is included in the generated documentation?

Ander

• • atomic_commit()
• • @dev: DRM device
• • This check is useful if drivers do not have DRIVER_ATOMIC set but
• • have atomic modesetting internally implemented.
• */

+static inline bool drm_drv_uses_atomic_modeset(struct drm_device *dev) +{

• return dev->mode_config.funcs->atomic_commit ? true : false;

+}    #endif /* DRM_ATOMIC_H_ */

Done, hope I got it right this time.

-DK

-----Original Message----- From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter Sent: Thursday, December 22, 2016 12:52 AM To: Ander Conselvan De Oliveira conselvan2@gmail.com Cc: Pandiyan, Dhinakaran dhinakaran.pandiyan@intel.com; intel-gfx@lists.freedesktop.org; Daniel Vetter daniel.vetter@ffwll.ch; dri-devel@lists.freedesktop.org; Ben Skeggs bskeggs@redhat.com; nouveau@lists.freedesktop.org Subject: Re: [Intel-gfx] [PATCH v2 1/2] drm: Wrap the check for atomic_commit implementation

On Thu, Dec 22, 2016 at 10:36:01AM +0200, Ander Conselvan De Oliveira wrote:

On Wed, 2016-12-21 at 12:12 -0800, Dhinakaran Pandiyan wrote:

...

This check is useful for drivers that do not have DRIVER_ATOMIC set but have atomic modesetting internally implemented. Wrap the check into a function since this is used in many places and as a bonus, the function name helps to document what the check is for.

v2: Change return type to bool (Ville) Move the function drm_atomic.h (Daniel)

Suggested-by: Daniel Vetter daniel.vetter@ffwll.ch Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: Ben Skeggs bskeggs@redhat.com Signed-off-by: Dhinakaran Pandiyan dhinakaran.pandiyan@intel.com

---

drivers/gpu/drm/drm_fb_helper.c             |  6 +++---  drivers/gpu/drm/nouveau/nouveau_connector.c |  5 +++--  drivers/gpu/drm/nouveau/nouveau_display.c   |  6 +++---  drivers/gpu/drm/nouveau/nouveau_fbcon.c     |  3 ++-  include/drm/drm_atomic.h                    | 11 +++++++++++  5 files changed, 22 insertions(+), 9 deletions(-)

...

...

diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h index 8cc7ca2..43db162 100644 --- a/include/drm/drm_atomic.h +++ b/include/drm/drm_atomic.h @@ -419,5 +419,16 @@ drm_atomic_crtc_needs_modeset(const struct drm_crtc_state *state)          state->connectors_changed;  }   +/* drm_drv_uses_atomic_modeset - check if the driver implements

Shouldn't this be

/**  * drm_drv_uses_atomic_modeset - ...

so it is included in the generated documentation?

Yup. I'm blind this week it seems. DK, please run

$ make DOCBOOKS="" htmldocs

and make sure your new documentation does get rendered correctly and shows up in the output. -Daniel

Ander

...

• • atomic_commit()
• • @dev: DRM device
• • This check is useful if drivers do not have DRIVER_ATOMIC set

+but

• • have atomic modesetting internally implemented.
• */

+static inline bool drm_drv_uses_atomic_modeset(struct drm_device +*dev) {

• return dev->mode_config.funcs->atomic_commit ? true : false; }

#endif /* DRM_ATOMIC_H_ */

-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch