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

[Matt Roper]

On Sat, Jun 06, 2015 at 05:34:45PM +0530, Sharma, Shashank wrote:
> Regards
> Shashank
> 
> On 6/6/2015 6:31 AM, Matt Roper wrote:
> >On Thu, Jun 04, 2015 at 07:12:37PM +0530, Kausal Malladi wrote:
> >>From: Kausal Malladi <Kausal.Malladi at 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
> >
> I understood this point. But in this case, we will be blocked until
> set CRTC implementation comes in. Can you suggest a better way to
> handle this without getting blocked for set_crtc() ?

Is the functionality you need still lacking with Maarten Lankhorst's
"convert to atomic, part 2" series that recently landed upstream?  If
that series isn't sufficient, them I'm pretty sure that his "part 3"
series on the mailing list will fix it; that will hopefully be ready to
merge as soon as I get a chance to do a review of it.

I'll add Maarten to the Cc here in case he wants to comment...


Matt

> >>
> >>Signed-off-by: Shashank Sharma <shashank.sharma at intel.com>
> >>Signed-off-by: Kausal Malladi <Kausal.Malladi at 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