Re: [PATCH v2 06/10] drm: Avoid atomic commit path for CRTC property (Gamma)

From: Matt Roper <matthew.d.roper@intel.com>
To: Kausal Malladi <Kausal.Malladi@intel.com>
Cc: annie.j.matheson@intel.com, dhanya.p.r@intel.com,
	dri-devel@lists.freedesktop.org, vijay.a.purushothaman@intel.com,
	jesse.barnes@intel.com, daniel.vetter@intel.com,
	intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH v2 06/10] drm: Avoid atomic commit path for CRTC property (Gamma)
Date: Fri, 5 Jun 2015 18:01:02 -0700	[thread overview]
Message-ID: <20150606010102.GI1099@intel.com> (raw)
In-Reply-To: <1433425361-17864-7-git-send-email-Kausal.Malladi@intel.com>

On Thu, Jun 04, 2015 at 07:12:37PM +0530, Kausal Malladi wrote:
> From: Kausal Malladi <Kausal.Malladi@intel.com>
> 
> The atomic CRTC set infrastructure is not available yet. So, the atomic
> check path throws error for any non-plane property.
> 
> This patch adds a diversion to avoid commit path for DRM atomic set Gamma
> property, until atomic CRTC infrastructure is ready.

This doesn't look right, but I don't quite understand what you're trying
to do from the commit message.

This function is what will implement legacy set_property ioctl's of a
CRTC on an atomic-based driver.  The idea is that when the ioctl is
issued, we should get (i.e., make a duplicate of) the current CRTC
state, change some of the values in that state (which is what the
.atomic_set_property function takes care of), and then submit that
modified state to the driver for checking and hw-programming.

Okay, I just took a quick peek ahead in your patch set and I think I
understand the confusion now...it looks like you're trying to actually
perform the full hardware update in the .atomic_set_property call chain,
which isn't what we want to be doing in an atomic driver.
.atomic_set_property() is just a matter of updating the state structures
to reflect the changes you *want* to make (but not actually performing
those changes right away).  It isn't until drm_atomic_commit() gets
called that we validate the new state structure and then write it to the
hardware if it looks okay.

Keep in mind that the overall goal behind atomic is that we want to be
able to supply several items to be updated (e.g., mode, CSC, gamma,
etc.) via the atomic ioctl and then have them all accepted (and
programmed) by the driver, or all rejected (so none get programmed) if
any of them are invalid.  Also, if the collection of properties that
you're updating fall within the "nuclear pageflip" subset of
functionality, you'll also get a guarantee that those items all get
updated within the same vblank; updates that straddle a vblank could
cause unwanted flickering or other artifacts.  The helper function
you're updating here only happens to update a single state value at a
time, but the same .atomic_set_property() is also used by the atomic
ioctl, so we need to make sure it's following the expected design.

Finally, keep in mind that the function you're updating here is a DRM
core function.  Even though i915 isn't quite ready for full atomic yet
and might have a bit of brain damage in areas, other vendors' drivers do
have complete atomic modesetting support (I think?), so adding
i915-specific workarounds like this to the core function could actually
hamper them.

Matt

> 
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> Signed-off-by: Kausal Malladi <Kausal.Malladi@intel.com>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index a900858..37dba55 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1696,6 +1696,8 @@ drm_atomic_helper_crtc_set_property(struct drm_crtc *crtc,
>  {
>  	struct drm_atomic_state *state;
>  	struct drm_crtc_state *crtc_state;
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_mode_config *config = &dev->mode_config;
>  	int ret = 0;
>  
>  	state = drm_atomic_state_alloc(crtc->dev);
> @@ -1716,9 +1718,18 @@ retry:
>  	if (ret)
>  		goto fail;
>  
> -	ret = drm_atomic_commit(state);
> -	if (ret != 0)
> -		goto fail;
> +	/**
> +	 * FIXME : This is a hack, to avoid atomic commit
> +	 * for CRTC, because i915 supports only
> +	 * atomic plane operations at the moment
> +	 */
> +	if (property == config->gamma_property)
> +		ret = 0;
> +	else {
> +		ret = drm_atomic_commit(state);
> +		if (ret != 0)
> +			goto fail;
> +	}
>  
>  	/* Driver takes ownership of state on successful commit. */
>  	return 0;
> -- 
> 2.4.2
> 

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx