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

[Matheson, Annie J]

Thank you guys!

Annie Matheson
Intel Corporation
Phone: (503) 712-0586 
Email: annie.j.matheson at intel.com

-----Original Message-----
From: Malladi, Kausal 
Sent: Wednesday, June 24, 2015 8:41 AM
To: Roper, Matthew D; Sharma, Shashank
Cc: Matheson, Annie J; R, Dhanya p; dri-devel at lists.freedesktop.org; Purushothaman, Vijay A; Barnes, Jesse; Vetter, Daniel; intel-gfx at lists.freedesktop.org; Maarten Lankhorst
Subject: Re: [Intel-gfx] [PATCH v2 06/10] drm: Avoid atomic commit path for CRTC property (Gamma)

On Saturday 20 June 2015 04:20 AM, Matt Roper wrote:
> On Mon, Jun 08, 2015 at 05:58:23PM -0700, Matt Roper wrote:
>> 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
> Kausal/Shashank, have you guys had a chance to check out Maarten's
> latest i915 atomic work?  His "part 2" series is already merged and
> "part 3" should be merged soon and those really help clean up a lot of
> the Intel-specific pain points caused by the atomic transition.  I think
> we need to make sure you can handle properties properly in an atomic
> manner, so if you find you're still blocked on the current code, we
> should look into how to address that so that we don't have to bring any
> temporary Intel hackiness into the shared DRM core code.
>
>
> Matt

Thanks Matt, for helping with a fix for this issue. As suggested by you, 
we will push this fix along with the other set of patches for color 
management.

Kausal
>
>>>>> 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
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx