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@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
Signed-off-by: Shashank Sharma shashank.sharma@intel.com Signed-off-by: Kausal Malladi Kausal.Malladi@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@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx