Hi Daniel,
On Tuesday 25 August 2015 09:15:16 Daniel Vetter wrote:
> On Tue, Aug 18, 2015 at 09:35:44AM +0300, Laurent Pinchart wrote:
> > On Friday 14 August 2015 09:30:15 Daniel Vetter wrote:
> > > On Fri, Aug 14, 2015 at 12:19:03AM +0300, Laurent Pinchart wrote:
> > > > On Friday 07 August 2015 17:30:08 Laurent Pinchart wrote:
> > > > > On Friday 07 August 2015 14:53:22 Thierry Reding wrote:
> > > > > > On Thu, Aug 06, 2015 at 03:23:00AM +0300, Laurent Pinchart wrote:
> > > > > > > The plane reset handler frees the plane state and allocates a
> > > > > > > new default state, but when doing so attempt to free the plane
> > > > > > > state using the base plane state pointer instead of casting it
> > > > > > > to the driver-specific state object that has been allocated. Fix
> > > > > > > it by using the rcar_du_plane_atomic_destroy_state() function to
> > > > > > > destroy the plane state instead of duplicating the code.
> > > > > > >
> > > > > > > Signed-off-by: Laurent Pinchart
> > > > > > > <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx>
> > > > > > > ---
> > > > > > >
> > > > > > > drivers/gpu/drm/rcar-du/rcar_du_plane.c | 45 +++++++++---------
> > > > > > > 1 file changed, 22 insertions(+), 23 deletions(-)
> > > > > > >
> > > > > > > Should the DRM core free the atomic state before calling the
> > > > > > > reset operation ? That would simplify drivers.
> > > > > >
> > > > > > The core can't do that because drivers might have subclassed the
> > > > > > state.
> > > > >
> > > > > But the core can call the .atomic_destroy_state() operation, can't
> > > > > it ?
> > > >
> > > > Thierry, Daniel, any comment on this ?
> > >
> > > Doesn't really help you since the kzalloc is still in the helper. Btw
> > > this is all helper code, core won't do here anything at all ;-)
> >
> > Is it ? The .reset() and .atomic_destroy_state() are core plane
> > operations, not helper operations.
>
> Reset not being a helper func is an accident of history I think, it should
> be moved.
Fine with me.
> > My point is that, as .reset() needs to allocate the state if no state
> > exists, I'm wondering whether it wouldn't be simpler for drivers to free
> > the state in the core using .atomic_destroy_state() before calling
> > .reset() and always allocate a state in the driver's .reset()
> > implementation.
> >
> > In peudo-code, drivers currently do (or at least should do)
> >
> > atomic_destroy_state(state)
> > {
> > driver_state = cast_to_driver_state(state);
> >
> > clean up driver_state;
> > kfree(driver_state);
> > }
> >
> > reset()
> > {
> > if (state) {
> > driver_state = cast_to_driver_state(state);
> >
> > clean up driver_state;
>
> Why not call destroy_state here and make the kzalloc unconditional?
> Simpler and with that not much point in removing copypasting ...
So all drivers would have to unconditionally call their atomic_destroy_state()
handler at the beginning of reset() ? Wouldn't it be simpler to move that call
in the helpers before calling reset() ?
> > } else {
> > driver_state = kzalloc(...);
> > }
> >
> > set all fields of driver_state to default values;
> > }
> >
> > Wouldn't it be simpler to have the core call .atomic_destroy_state()
> > before .reset() and implement .reset() as
> >
> > reset()
> > driver_state = kzalloc(...);
> >
> > set all fields of driver_state to default values;
> > }
> >
> > ?
>
> Well all the reset stuff was pretty much stop-gap, ->reset really
> shouldn't be a core op. What I eventually wanted to do is lift the hw
> state readout logic from i915 as the proper way to do this, since without
> this you can't do fastboot. Eric is interested in fastboot for vc4, so we
> discussed this a bit at lpc.
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
