Hi,
On Mon, Mar 7, 2016 at 9:57 AM, Heiko Stübner <heiko@xxxxxxxxx> wrote:
> Am Montag, 7. März 2016, 09:36:07 schrieb Doug Anderson:
>> Hi,
>>
>> On Mon, Mar 7, 2016 at 12:37 AM, Mark yao <mark.yao@xxxxxxxxxxxxxx> wrote:
>> > On 2016年03月05日 20:39, Russell King - ARM Linux wrote:
>> >> On Sat, Mar 05, 2016 at 12:11:16PM +0000, John Keeping wrote:
>> >>> On Fri, Mar 04, 2016 at 03:22:01PM -0800, Douglas Anderson wrote:
>> >>>> The drm_encoder_cleanup() was missing both from the error path of
>> >>>> dw_hdmi_rockchip_bind(). This caused a crash when slub_debug was
>> >>>> enabled and we ended up deferring probe of HDMI at boot.
>> >>>>
>> >>>> This call isn't needed from unbind() because if dw_hdmi_bind() returns
>> >>>> no error then it takes over the job of freeing the encoder (in
>> >>>> dw_hdmi_unbind).
>> >>>>
>> >>>> Signed-off-by: Douglas Anderson <dianders@xxxxxxxxxxxx>
>> >>>> ---
>> >>>
>> >>> Does dw_hdmi-imx need a similar change? I wonder if it would be cleaner
>> >>> to push this into dw_hdmi_bind() if it affects all of the platforms..
>> >>
>> >> I don't think moving it there would make sense - keep the initialisation
>> >> and cleanup together in the same file so that it's contained together.
>> >
>> > I don't like this patch too, initialisation and cleanup not in the same
>> > file looks bad,
>> >
>> > How about:
>> >
>> > drivers/gpu/drm/bridge/dw-hdmi.c
>> > void dw_hdmi_unbind(struct device *dev, struct device *master, void *data)
>> >
>> > hdmi_writeb(hdmi, ~0, HDMI_IH_MUTE_PHY_STAT0);
>> >
>> > hdmi->connector.funcs->destroy(&hdmi->connector);
>> > - hdmi->encoder->funcs->destroy(hdmi->encoder);
>> >
>> > drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
>> > static int dw_hdmi_rockchip_bind(struct device *dev, struct device
>> > *master,
>> >
>> > - return dw_hdmi_bind(dev, master, data, encoder, iores, irq,
>> > plat_data);
>> > + ret = dw_hdmi_bind(dev, master, data, encoder, iores, irq,
>> > plat_data);
>> > + if (ret)
>> > + drm_encoder_cleanup(encoder);
>> > +
>> > + return ret;
>> >
>> > }
>> >
>> > static void dw_hdmi_rockchip_unbind(struct device *dev, struct device
>> >
>> > *master,
>> >
>> > void *data)
>> >
>> > {
>> >
>> > + drm_encoder_cleanup(...);
>> >
>> > return dw_hdmi_unbind(dev, master, data);
>> >
>> > }
>>
>> That'a a reasonable suggestion in theory. ...but we run into the same
>> problem I've run into before with the strange relationship between
>> dw_hdmi and its descendants.
>
> I don't think handing off the cleanup responsibility is really in question
> here. I.e. I do believe it should also be fine to expect (as definition) the
> core driver to cleanup the encoder _after_ it sucessfully claimed it in
> dw_hdmi_bind().
>
> We do the same in the rockchip power-domains, handing off the struct clk-
> pointer to the pm_clk stuff (due to the clk-pointer being unique per-device
> nowadays).
>
> So just making sure it is sucessfully handed off should also be ok.
If I understand correctly, that means you'd be OK with the original
patch I posted? In that case cleanup continues to happen in the main
dw-hdmi.c if dw_hdmi_bind() succeeds and my patch fixes the cleanup
when dw_hdmi_bind() fails (and thus cleanup responsibility was not
handed off).
Also: I noticed that Russell also didn't seem to say that my original
patch was bad. I think he just said that he didn't like John
Keeping's suggestion.
Please correct any misunderstandings. Thanks!
-Doug
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
