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

[Sharma, Shashank]

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() ?
>>
>> 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
>>
>