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
>>
>
