Thank you guys!
Annie Matheson
Intel Corporation
Phone: (503) 712-0586
Email: annie.j.matheson@xxxxxxxxx
-----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@xxxxxxxxxxxxxxxxxxxxx; Purushothaman, Vijay A; Barnes, Jesse; Vetter, Daniel; intel-gfx@xxxxxxxxxxxxxxxxxxxxx; 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@xxxxxxxxx>
>>>>>
>>>>> 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@xxxxxxxxx>
>>>>> Signed-off-by: Kausal Malladi <Kausal.Malladi@xxxxxxxxx>
>>>>> ---
>>>>> 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@xxxxxxxxxxxxxxxxxxxxx
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
